Messages in this thread | ![/](/images/icornerl.gif) | | Date | Fri, 5 Apr 2024 19:07:12 +0300 | From | Vladimir Oltean <> | Subject | Re: [PATCH net-next v8 07/16] net: dsa: vsc73xx: Add vlan filtering |
| |
On Wed, Apr 03, 2024 at 12:37:23PM +0200, Pawel Dembicki wrote: > +static int > +vsc73xx_port_vlan_filtering(struct dsa_switch *ds, int port, > + bool vlan_filtering, struct netlink_ext_ack *extack) > +{ > + enum vsc73xx_port_vlan_conf port_vlan_conf = VSC73XX_VLAN_IGNORE; > + u16 vid_pvid = 0, vid_untagged = 0; > + struct vsc73xx_portinfo *portinfo; > + struct vsc73xx *vsc = ds->priv; > + bool set_untagged = false; > + bool set_pvid = false; > + > + portinfo = &vsc->portinfo[port]; > + > + /* The swap processed below is required because vsc73xx is using > + * tag_8021q. When vlan_filtering is disabled, tag_8021q uses > + * pvid/untagged vlans for port recognition. The values configured for > + * vlans and pvid/untagged states are stored in portinfo structure. > + * When vlan_filtering is enabled, we need to restore pvid/untagged from > + * portinfo structure. Analogic routine is processed when vlan_filtering
Analogous
> + * is disabled, but values used for tag_8021q are restored. > + */ > + if (vlan_filtering) { > + struct vsc73xx_vlan_summary summary; > + > + /* Use VLAN_N_VID to count all vlans */ > + vsc73xx_bridge_vlan_summary(vsc, port, &summary, VLAN_N_VID); > + > + port_vlan_conf = (summary.num_untagged > 1) ? > + VSC73XX_VLAN_FILTER_UNTAG_ALL : > + VSC73XX_VLAN_FILTER; > + > + if (summary.num_untagged == 1) { > + vid_untagged = vsc73xx_find_first_vlan_untagged(vsc, > + port); > + set_untagged = true; > + }
Is there really any functional difference between the above, vs this in port_vlan_add():
port_vlan_conf = VSC73XX_VLAN_FILTER;
if (summary.num_tagged == 0 && untagged) port_vlan_conf = VSC73XX_VLAN_FILTER_UNTAG_ALL; vsc73xx_set_vlan_conf(vsc, port, port_vlan_conf);
if (port_vlan_conf == VSC73XX_VLAN_FILTER_UNTAG_ALL) goto update_vlan_table;
vs this in port_vlan_del():
enum vsc73xx_port_vlan_conf port_vlan_conf = VSC73XX_VLAN_FILTER;
if (summary.num_tagged == 0) port_vlan_conf = VSC73XX_VLAN_FILTER_UNTAG_ALL; vsc73xx_set_vlan_conf(vsc, port, port_vlan_conf);
if (summary.num_untagged <= 1) { vid = vsc73xx_find_first_vlan_untagged(vsc, port); vsc73xx_vlan_change_untagged_hw(vsc, port, vid, summary.num_untagged); }
?
If not, I have to agree with Florian that it's dizzying to follow the vsc73xx_set_vlan_conf() calls, all with slightly different input. Even worse now than before, I think.
I see you've changed the storage variable names, so maybe you are open to some more refactoring, to make the code more maintainable.
I would suggest calling a single vsc73xx_update_vlan_conf() from all places, which figures out what to do by itself, based on the current (updated) state. All ifs and buts are on the inside, not on the outside.
Also, the operate_on_storage variable is not a great name now. Because you always operate on storage. It means "operate _just_ on storage", aka "don't commit to hardware".
I wanted to avoid requesting you to make more changes to the implementation, but you started it, and now it's a confusing mix of concepts from old and new patch sets.
I would like to also see the "operate_on_storage" logic rewritten and embedded into those functions which must update shared resources (port PVID, port untagged VID, etc).
Basically, in the end I would like the driver to be written in the style of every other similar function: ocelot_port_manage_port_tag(), sja1105_commit_pvid(), mv88e6xxx_commit_pvid() etc. No arguments, except the port on which to run. Figure out what to do from the current state, and make sure it is called from all places that change that state. It also serves very well as self-documenting code, because "how register X should be set" is centralized into a single function.
Sorry for making this request so late in the development process. I hope it's not discouraging.
| ![\](/images/icornerr.gif) |