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 <olte...@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, > -- Florian