Message ID | 20190324032346.32394-4-olteanv@gmail.com |
---|---|
State | RFC |
Delegated to: | David Miller |
Headers | show |
Series | NXP SJA1105 DSA driver | expand |
On 3/23/19 8:23 PM, Vladimir Oltean wrote: > This refactors the two-phase transaction from dsa_slave_vlan_rx_add_vid > and also makes that code available for other functions from within DSA. > The newly exposed function either adds or deletes the specified VLAN > entry based on a boolean argument. > > Signed-off-by: Vladimir Oltean <olteanv@gmail.com> The name of the function does not make it particularly clear that passing false results in deleting the VLAN. Can you just wrap this under a different function name that is only doing the two-step adding of VLANs and keep using dsa_port_vlan_del() explicitly when you want to remove a VLAN? > --- > net/dsa/dsa_priv.h | 2 ++ > net/dsa/port.c | 24 ++++++++++++++++++++++++ > net/dsa/slave.c | 16 ++-------------- > 3 files changed, 28 insertions(+), 14 deletions(-) > > diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h > index 093b7d145eb1..8048ced3708f 100644 > --- a/net/dsa/dsa_priv.h > +++ b/net/dsa/dsa_priv.h > @@ -164,6 +164,8 @@ int dsa_port_pre_bridge_flags(const struct dsa_port *dp, unsigned long flags, > struct switchdev_trans *trans); > int dsa_port_bridge_flags(const struct dsa_port *dp, unsigned long flags, > struct switchdev_trans *trans); > +int dsa_port_trans_vlan_apply(struct dsa_port *dp, u16 vid, u16 flags, > + bool enabled); > int dsa_port_vlan_add(struct dsa_port *dp, > const struct switchdev_obj_port_vlan *vlan, > struct switchdev_trans *trans); > diff --git a/net/dsa/port.c b/net/dsa/port.c > index a86fe3be1261..9c7358f98004 100644 > --- a/net/dsa/port.c > +++ b/net/dsa/port.c > @@ -326,6 +326,30 @@ int dsa_port_vlan_del(struct dsa_port *dp, > return 0; > } > > +int dsa_port_trans_vlan_apply(struct dsa_port *dp, u16 vid, u16 flags, > + bool enabled) > +{ > + struct switchdev_obj_port_vlan vlan = { > + .obj.id = SWITCHDEV_OBJ_ID_PORT_VLAN, > + .flags = flags, > + .vid_begin = vid, > + .vid_end = vid, > + }; > + struct switchdev_trans trans; > + int err; > + > + if (!enabled) > + return dsa_port_vlan_del(dp, &vlan); > + > + trans.ph_prepare = true; > + err = dsa_port_vlan_add(dp, &vlan, &trans); > + if (err == -EOPNOTSUPP) > + return 0; > + > + trans.ph_prepare = false; > + return dsa_port_vlan_add(dp, &vlan, &trans); > +} > + > static struct phy_device *dsa_port_get_phy_device(struct dsa_port *dp) > { > struct device_node *phy_dn; > diff --git a/net/dsa/slave.c b/net/dsa/slave.c > index 093eef6f2599..3191ef74f6a1 100644 > --- a/net/dsa/slave.c > +++ b/net/dsa/slave.c > @@ -987,13 +987,6 @@ static int dsa_slave_vlan_rx_add_vid(struct net_device *dev, __be16 proto, > u16 vid) > { > struct dsa_port *dp = dsa_slave_to_port(dev); > - struct switchdev_obj_port_vlan vlan = { > - .vid_begin = vid, > - .vid_end = vid, > - /* This API only allows programming tagged, non-PVID VIDs */ > - .flags = 0, > - }; > - struct switchdev_trans trans; > struct bridge_vlan_info info; > int ret; > > @@ -1010,13 +1003,8 @@ static int dsa_slave_vlan_rx_add_vid(struct net_device *dev, __be16 proto, > return -EBUSY; > } > > - trans.ph_prepare = true; > - ret = dsa_port_vlan_add(dp, &vlan, &trans); > - if (ret == -EOPNOTSUPP) > - return 0; > - > - trans.ph_prepare = false; > - return dsa_port_vlan_add(dp, &vlan, &trans); > + /* This API only allows programming tagged, non-PVID VIDs */ > + return dsa_port_trans_vlan_apply(dp, vid, 0, true); > } > > static int dsa_slave_vlan_rx_kill_vid(struct net_device *dev, __be16 proto, >
On 3/25/19 7:06 PM, Florian Fainelli wrote: > On 3/23/19 8:23 PM, Vladimir Oltean wrote: >> This refactors the two-phase transaction from dsa_slave_vlan_rx_add_vid >> and also makes that code available for other functions from within DSA. >> The newly exposed function either adds or deletes the specified VLAN >> entry based on a boolean argument. >> >> Signed-off-by: Vladimir Oltean <olteanv@gmail.com> > > The name of the function does not make it particularly clear that > passing false results in deleting the VLAN. Can you just wrap this under > a different function name that is only doing the two-step adding of > VLANs and keep using dsa_port_vlan_del() explicitly when you want to > remove a VLAN? > The reason why I made it this way was mainly to make the switchdev_obj_port_vlan struct an implementation detail. That being so I wouldn't need to keep and continuously modify this struct during the 3 times I'm calling the function from within the 05/13 patch ("net: dsa: Optional VLAN-based port separation for switches without tagging"). I did try to make use of the .vid_begin/.vid_end feature and batch some of the function calls, but doing so would restrict possible values that the rx_vid and tx_vid functions may return - dsa_port_setup_8021q_tagging() would need to ensure that they are contiguous prior to batching them into a single vlan object. By the way, I don't think anybody is making any good use of this feature, it's just creating useless boilerplate as of now. The other thing is that if I were to wrap around dsa_port_vlan_add() and get rid of the switchdev_obj_port_vlan and just pass the vid as u16, I'd have to do the same wrapping for the dsa_port_vlan_del() function too. Plus I'd have to keep 3 'if' conditions just to decide whether to call *_add or *_del. > diff --git a/net/dsa/tag_8021q.c b/net/dsa/tag_8021q.c > index 221299b264f5..1b11b245e2d6 100644 > --- a/net/dsa/tag_8021q.c > +++ b/net/dsa/tag_8021q.c > @@ -120,8 +120,10 @@ int dsa_port_setup_8021q_tagging(struct dsa_switch *ds, int port, bool enabled) > /* The Rx VID is a regular VLAN on all others */ > flags = BRIDGE_VLAN_INFO_UNTAGGED; > > - err = dsa_port_trans_vlan_apply(other_dp, rx_vid, flags, > - enabled); > + if (enabled) > + err = __dsa_port_vlan_add(other_dp, rx_vid, flags); > + else > + err = __dsa_port_vlan_del(other_dp, rx_vid); > if (err) { > dev_err(ds->dev, "Failed to apply Rx VID %d to port %d: %d\n", > rx_vid, port, err); > @@ -129,14 +131,20 @@ int dsa_port_setup_8021q_tagging(struct dsa_switch *ds, int port, bool enabled) > } > } > /* Finally apply the Tx VID on this port and on the CPU port */ > - err = dsa_port_trans_vlan_apply(dp, tx_vid, BRIDGE_VLAN_INFO_UNTAGGED, > - enabled); > + if (enabled) > + err = __dsa_port_vlan_add(dp, tx_vid, > + BRIDGE_VLAN_INFO_UNTAGGED); > + else > + err = __dsa_port_vlan_del(dp, tx_vid); > if (err) { > dev_err(ds->dev, "Failed to apply Tx VID %d on port %d: %d\n", > tx_vid, port, err); > return err; > } > - err = dsa_port_trans_vlan_apply(upstream_dp, tx_vid, 0, enabled); > + if (enabled) > + err = __dsa_port_vlan_add(upstream_dp, tx_vid, 0); > + else > + err = __dsa_port_vlan_del(upstream_dp, tx_vid); > if (err) { > dev_err(ds->dev, "Failed to apply Tx VID %d on port %d: %d\n", > tx_vid, upstream, err); How does something like the above look like? Thanks, -Vladimir >> --- >> net/dsa/dsa_priv.h | 2 ++ >> net/dsa/port.c | 24 ++++++++++++++++++++++++ >> net/dsa/slave.c | 16 ++-------------- >> 3 files changed, 28 insertions(+), 14 deletions(-) >> >> diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h >> index 093b7d145eb1..8048ced3708f 100644 >> --- a/net/dsa/dsa_priv.h >> +++ b/net/dsa/dsa_priv.h >> @@ -164,6 +164,8 @@ int dsa_port_pre_bridge_flags(const struct dsa_port *dp, unsigned long flags, >> struct switchdev_trans *trans); >> int dsa_port_bridge_flags(const struct dsa_port *dp, unsigned long flags, >> struct switchdev_trans *trans); >> +int dsa_port_trans_vlan_apply(struct dsa_port *dp, u16 vid, u16 flags, >> + bool enabled); >> int dsa_port_vlan_add(struct dsa_port *dp, >> const struct switchdev_obj_port_vlan *vlan, >> struct switchdev_trans *trans); >> diff --git a/net/dsa/port.c b/net/dsa/port.c >> index a86fe3be1261..9c7358f98004 100644 >> --- a/net/dsa/port.c >> +++ b/net/dsa/port.c >> @@ -326,6 +326,30 @@ int dsa_port_vlan_del(struct dsa_port *dp, >> return 0; >> } >> >> +int dsa_port_trans_vlan_apply(struct dsa_port *dp, u16 vid, u16 flags, >> + bool enabled) >> +{ >> + struct switchdev_obj_port_vlan vlan = { >> + .obj.id = SWITCHDEV_OBJ_ID_PORT_VLAN, >> + .flags = flags, >> + .vid_begin = vid, >> + .vid_end = vid, >> + }; >> + struct switchdev_trans trans; >> + int err; >> + >> + if (!enabled) >> + return dsa_port_vlan_del(dp, &vlan); >> + >> + trans.ph_prepare = true; >> + err = dsa_port_vlan_add(dp, &vlan, &trans); >> + if (err == -EOPNOTSUPP) >> + return 0; >> + >> + trans.ph_prepare = false; >> + return dsa_port_vlan_add(dp, &vlan, &trans); >> +} >> + >> static struct phy_device *dsa_port_get_phy_device(struct dsa_port *dp) >> { >> struct device_node *phy_dn; >> diff --git a/net/dsa/slave.c b/net/dsa/slave.c >> index 093eef6f2599..3191ef74f6a1 100644 >> --- a/net/dsa/slave.c >> +++ b/net/dsa/slave.c >> @@ -987,13 +987,6 @@ static int dsa_slave_vlan_rx_add_vid(struct net_device *dev, __be16 proto, >> u16 vid) >> { >> struct dsa_port *dp = dsa_slave_to_port(dev); >> - struct switchdev_obj_port_vlan vlan = { >> - .vid_begin = vid, >> - .vid_end = vid, >> - /* This API only allows programming tagged, non-PVID VIDs */ >> - .flags = 0, >> - }; >> - struct switchdev_trans trans; >> struct bridge_vlan_info info; >> int ret; >> >> @@ -1010,13 +1003,8 @@ static int dsa_slave_vlan_rx_add_vid(struct net_device *dev, __be16 proto, >> return -EBUSY; >> } >> >> - trans.ph_prepare = true; >> - ret = dsa_port_vlan_add(dp, &vlan, &trans); >> - if (ret == -EOPNOTSUPP) >> - return 0; >> - >> - trans.ph_prepare = false; >> - return dsa_port_vlan_add(dp, &vlan, &trans); >> + /* This API only allows programming tagged, non-PVID VIDs */ >> + return dsa_port_trans_vlan_apply(dp, vid, 0, true); >> } >> >> static int dsa_slave_vlan_rx_kill_vid(struct net_device *dev, __be16 proto, >> > >
diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h index 093b7d145eb1..8048ced3708f 100644 --- a/net/dsa/dsa_priv.h +++ b/net/dsa/dsa_priv.h @@ -164,6 +164,8 @@ int dsa_port_pre_bridge_flags(const struct dsa_port *dp, unsigned long flags, struct switchdev_trans *trans); int dsa_port_bridge_flags(const struct dsa_port *dp, unsigned long flags, struct switchdev_trans *trans); +int dsa_port_trans_vlan_apply(struct dsa_port *dp, u16 vid, u16 flags, + bool enabled); int dsa_port_vlan_add(struct dsa_port *dp, const struct switchdev_obj_port_vlan *vlan, struct switchdev_trans *trans); diff --git a/net/dsa/port.c b/net/dsa/port.c index a86fe3be1261..9c7358f98004 100644 --- a/net/dsa/port.c +++ b/net/dsa/port.c @@ -326,6 +326,30 @@ int dsa_port_vlan_del(struct dsa_port *dp, return 0; } +int dsa_port_trans_vlan_apply(struct dsa_port *dp, u16 vid, u16 flags, + bool enabled) +{ + struct switchdev_obj_port_vlan vlan = { + .obj.id = SWITCHDEV_OBJ_ID_PORT_VLAN, + .flags = flags, + .vid_begin = vid, + .vid_end = vid, + }; + struct switchdev_trans trans; + int err; + + if (!enabled) + return dsa_port_vlan_del(dp, &vlan); + + trans.ph_prepare = true; + err = dsa_port_vlan_add(dp, &vlan, &trans); + if (err == -EOPNOTSUPP) + return 0; + + trans.ph_prepare = false; + return dsa_port_vlan_add(dp, &vlan, &trans); +} + static struct phy_device *dsa_port_get_phy_device(struct dsa_port *dp) { struct device_node *phy_dn; diff --git a/net/dsa/slave.c b/net/dsa/slave.c index 093eef6f2599..3191ef74f6a1 100644 --- a/net/dsa/slave.c +++ b/net/dsa/slave.c @@ -987,13 +987,6 @@ static int dsa_slave_vlan_rx_add_vid(struct net_device *dev, __be16 proto, u16 vid) { struct dsa_port *dp = dsa_slave_to_port(dev); - struct switchdev_obj_port_vlan vlan = { - .vid_begin = vid, - .vid_end = vid, - /* This API only allows programming tagged, non-PVID VIDs */ - .flags = 0, - }; - struct switchdev_trans trans; struct bridge_vlan_info info; int ret; @@ -1010,13 +1003,8 @@ static int dsa_slave_vlan_rx_add_vid(struct net_device *dev, __be16 proto, return -EBUSY; } - trans.ph_prepare = true; - ret = dsa_port_vlan_add(dp, &vlan, &trans); - if (ret == -EOPNOTSUPP) - return 0; - - trans.ph_prepare = false; - return dsa_port_vlan_add(dp, &vlan, &trans); + /* This API only allows programming tagged, non-PVID VIDs */ + return dsa_port_trans_vlan_apply(dp, vid, 0, true); } static int dsa_slave_vlan_rx_kill_vid(struct net_device *dev, __be16 proto,
This refactors the two-phase transaction from dsa_slave_vlan_rx_add_vid and also makes that code available for other functions from within DSA. The newly exposed function either adds or deletes the specified VLAN entry based on a boolean argument. Signed-off-by: Vladimir Oltean <olteanv@gmail.com> --- net/dsa/dsa_priv.h | 2 ++ net/dsa/port.c | 24 ++++++++++++++++++++++++ net/dsa/slave.c | 16 ++-------------- 3 files changed, 28 insertions(+), 14 deletions(-)