Message ID | 20190410005700.31582-6-olteanv@gmail.com |
---|---|
State | Changes Requested |
Delegated to: | David Miller |
Headers | show |
Series | NXP SJA1105 DSA driver | expand |
On 4/9/2019 5:56 PM, Vladimir Oltean wrote: > This hides the need to perform a two-phase transaction and construct a > switchdev_obj_port_vlan struct. > > Call graph (including a function that will be introduced in a follow-up > patch) looks like this now (same for the *_vlan_del function): > > dsa_slave_vlan_rx_add_vid dsa_port_setup_8021q_tagging > | | > | | > | +-------------+ > | | > v v > dsa_port_vid_add dsa_slave_port_obj_add > | | > +-------+ +-------+ > | | > v v > dsa_port_vlan_add > > Signed-off-by: Vladimir Oltean <olteanv@gmail.com> > --- > Changes in v2: > Renamed __dsa_port_vlan_add to dsa_port_vid_add and not to > dsa_port_vlan_add_trans, as suggested, because the corresponding _del function > does not have a transactional phase and the naming is more uniform this way. Thanks the name does look a lot better now. [snip] > +int dsa_port_vid_del(struct dsa_port *dp, u16 vid) > +{ > + struct switchdev_obj_port_vlan vlan = { > + .obj.id = SWITCHDEV_OBJ_ID_PORT_VLAN, > + .vid_begin = vid, > + .vid_end = vid, > + }; Any reasons why this function does not take a flags argument? Also, did you intend to migrate dsa_slave_vlan_rx_kill_vid() to use this helper for deleting a VLAN ID?
On 4/10/19 5:01 AM, Florian Fainelli wrote: > On 4/9/2019 5:56 PM, Vladimir Oltean wrote: >> This hides the need to perform a two-phase transaction and construct a >> switchdev_obj_port_vlan struct. >> >> Call graph (including a function that will be introduced in a follow-up >> patch) looks like this now (same for the *_vlan_del function): >> >> dsa_slave_vlan_rx_add_vid dsa_port_setup_8021q_tagging >> | | >> | | >> | +-------------+ >> | | >> v v >> dsa_port_vid_add dsa_slave_port_obj_add >> | | >> +-------+ +-------+ >> | | >> v v >> dsa_port_vlan_add >> >> Signed-off-by: Vladimir Oltean <olteanv@gmail.com> >> --- >> Changes in v2: >> Renamed __dsa_port_vlan_add to dsa_port_vid_add and not to >> dsa_port_vlan_add_trans, as suggested, because the corresponding _del function >> does not have a transactional phase and the naming is more uniform this way. > > Thanks the name does look a lot better now. > > [snip] > >> +int dsa_port_vid_del(struct dsa_port *dp, u16 vid) >> +{ >> + struct switchdev_obj_port_vlan vlan = { >> + .obj.id = SWITCHDEV_OBJ_ID_PORT_VLAN, >> + .vid_begin = vid, >> + .vid_end = vid, >> + }; > > Any reasons why this function does not take a flags argument? Also, did > you intend to migrate dsa_slave_vlan_rx_kill_vid() to use this helper > for deleting a VLAN ID? > Hi Florian, I don't think there's any semantics associated to deleting a VLAN with flags. The br_switchdev_port_vlan_del() function doesn't set the flags either. And as for the dsa_slave_vlan_rx_kill_vid(), it wasn't an oversight, I just thought that the benefits weren't as great as in the case of *_add. I do see that not doing it somewhat breaks the uniformity and makes the commit message slightly inaccurate, so I can fix that up if you think it's necessary. Thanks, -Vladimir
diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h index 093b7d145eb1..4246523e3133 100644 --- a/net/dsa/dsa_priv.h +++ b/net/dsa/dsa_priv.h @@ -169,6 +169,8 @@ int dsa_port_vlan_add(struct dsa_port *dp, struct switchdev_trans *trans); int dsa_port_vlan_del(struct dsa_port *dp, const struct switchdev_obj_port_vlan *vlan); +int dsa_port_vid_add(struct dsa_port *dp, u16 vid, u16 flags); +int dsa_port_vid_del(struct dsa_port *dp, u16 vid); int dsa_port_link_register_of(struct dsa_port *dp); void dsa_port_link_unregister_of(struct dsa_port *dp); diff --git a/net/dsa/port.c b/net/dsa/port.c index a86fe3be1261..19f3b91c648d 100644 --- a/net/dsa/port.c +++ b/net/dsa/port.c @@ -326,6 +326,37 @@ int dsa_port_vlan_del(struct dsa_port *dp, return 0; } +int dsa_port_vid_add(struct dsa_port *dp, u16 vid, u16 flags) +{ + 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; + + 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); +} + +int dsa_port_vid_del(struct dsa_port *dp, u16 vid) +{ + struct switchdev_obj_port_vlan vlan = { + .obj.id = SWITCHDEV_OBJ_ID_PORT_VLAN, + .vid_begin = vid, + .vid_end = vid, + }; + + return dsa_port_vlan_del(dp, &vlan); +} + 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 80be8e86c82d..d0905fd720b2 100644 --- a/net/dsa/slave.c +++ b/net/dsa/slave.c @@ -994,13 +994,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; @@ -1017,13 +1010,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_vid_add(dp, vid, 0); } static int dsa_slave_vlan_rx_kill_vid(struct net_device *dev, __be16 proto,
This hides the need to perform a two-phase transaction and construct a switchdev_obj_port_vlan struct. Call graph (including a function that will be introduced in a follow-up patch) looks like this now (same for the *_vlan_del function): dsa_slave_vlan_rx_add_vid dsa_port_setup_8021q_tagging | | | | | +-------------+ | | v v dsa_port_vid_add dsa_slave_port_obj_add | | +-------+ +-------+ | | v v dsa_port_vlan_add Signed-off-by: Vladimir Oltean <olteanv@gmail.com> --- Changes in v2: Renamed __dsa_port_vlan_add to dsa_port_vid_add and not to dsa_port_vlan_add_trans, as suggested, because the corresponding _del function does not have a transactional phase and the naming is more uniform this way. net/dsa/dsa_priv.h | 2 ++ net/dsa/port.c | 31 +++++++++++++++++++++++++++++++ net/dsa/slave.c | 16 ++-------------- 3 files changed, 35 insertions(+), 14 deletions(-)