Re: [Patch RFC net-next 4/4] net: dsa: microchip: use private pvid for bridge_vlan_unwaware

[<Arun.Ramadoss@microchip.com>]

On Tue, 2022-08-02 at 13:59 +0300, Vladimir Oltean wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> On Fri, Jul 29, 2022 at 08:47:33PM +0530, Arun Ramadoss wrote:
> > diff --git a/drivers/net/dsa/microchip/ksz_common.c
> > b/drivers/net/dsa/microchip/ksz_common.c
> > index 516fb9d35c87..8a5583b1f2f4 100644
> > --- a/drivers/net/dsa/microchip/ksz_common.c
> > +++ b/drivers/net/dsa/microchip/ksz_common.c
> > @@ -161,6 +161,7 @@ static const struct ksz_dev_ops ksz8_dev_ops =
> > {
> >       .vlan_filtering = ksz8_port_vlan_filtering,
> >       .vlan_add = ksz8_port_vlan_add,
> >       .vlan_del = ksz8_port_vlan_del,
> > +     .drop_untagged = ksz8_port_enable_pvid,
> 
> You'll have to explain this one. What impact does PVID insertion on
> KSZ8
> have upon dropping/not dropping untagged packets? This patch is
> saying
> that when untagged packets should be dropped, PVID insertion should
> be
> enabled, and when untagged packets should be accepted, PVID insertion
> should be disabled. How come?

Its my mistake. I referred KSZ87xx datasheet but I couldn't find the
register for the dropping the untagged packet. If that is the case,
shall I remove the dropping of untagged packet feature from the ksz8
switches?

> 
> >       .mirror_add = ksz8_port_mirror_add,
> >       .mirror_del = ksz8_port_mirror_del,
> >       .get_caps = ksz8_get_caps,
> > @@ -187,6 +188,7 @@ static const struct ksz_dev_ops ksz9477_dev_ops
> > = {
> >       .vlan_filtering = ksz9477_port_vlan_filtering,
> >       .vlan_add = ksz9477_port_vlan_add,
> >       .vlan_del = ksz9477_port_vlan_del,
> > +     .drop_untagged = ksz9477_port_drop_untagged,
> >       .mirror_add = ksz9477_port_mirror_add,
> >       .mirror_del = ksz9477_port_mirror_del,
> >       .get_caps = ksz9477_get_caps,
> > @@ -220,6 +222,7 @@ static const struct ksz_dev_ops lan937x_dev_ops
> > = {
> >       .vlan_filtering = ksz9477_port_vlan_filtering,
> >       .vlan_add = ksz9477_port_vlan_add,
> >       .vlan_del = ksz9477_port_vlan_del,
> > +     .drop_untagged = ksz9477_port_drop_untagged,
> >       .mirror_add = ksz9477_port_mirror_add,
> >       .mirror_del = ksz9477_port_mirror_del,
> >       .get_caps = lan937x_phylink_get_caps,
> > @@ -1254,6 +1257,9 @@ static int ksz_enable_port(struct dsa_switch
> > *ds, int port,
> >  {
> >       struct ksz_device *dev = ds->priv;
> > 
> > +     dev->dev_ops->vlan_add(dev, port, KSZ_DEFAULT_VLAN,
> > +                            BRIDGE_VLAN_INFO_UNTAGGED);
> > +
> 
> How many times can this be executed before the VLAN add operation
> fails
> due to the VLAN already being present on the port? I notice you're
> ignoring the return code. Wouldn't it be better to do this at
> port_setup() time instead?
> 
> (side note, the PVID for standalone mode can be added at port_setup
> time. The PVID to use for bridges in VLAN-unaware mode can be
> allocated
> at port_bridge_join time)

Ok. I will add in port_bridge_join function for bridge vlan_aware
ports.

> 
> >       if (!dsa_is_user_port(ds, port))
> >               return 0;
> > 
> > +static int ksz_commit_pvid(struct dsa_switch *ds, int port)
> > +{
> > +     struct dsa_port *dp = dsa_to_port(ds, port);
> > +     struct net_device *br = dsa_port_bridge_dev_get(dp);
> > +     struct ksz_device *dev = ds->priv;
> > +     u16 pvid = KSZ_DEFAULT_VLAN;
> > +     bool drop_untagged = false;
> > +     struct ksz_port *p;
> > +
> > +     p = &dev->ports[port];
> > +
> > +     if (br && br_vlan_enabled(br)) {
> > +             pvid = p->bridge_pvid.vid;
> > +             drop_untagged = !p->bridge_pvid.valid;
> > +     }
> 
> This is better in the sense that it resolves the need for the
> configure_vlan_while_not_filtering hack. But standalone and VLAN-
> unaware
> bridge ports still share the same PVID. Even more so, standalone
> ports
> have address learning enabled, which will poison the address database
> of
> VLAN-unaware bridge ports (and of other standalone ports):
> 
https://patchwork.kernel.org/project/netdevbpf/patch/20220802002636.3963025-1-vladimir.oltean@nxp.com/
> 
> Are you going to do further work in this area?

For now, I thought I can fix the issue for bridge vlan unaware port. I
have few other patch series to be submitted like gPTP, tc commands. If
standalone port fix also needed for your patch series I can work on it
otherwise I can take up later stage.


> 
> > +
> > +     ksz_set_pvid(dev, port, pvid);
> > +
> > +     if (dev->dev_ops->drop_untagged)
> > +             dev->dev_ops->drop_untagged(dev, port,
> > drop_untagged);
> > +
> > +     return 0;
> > +}