Re: [PATCH v2 net-next 9/9] net: dsa: microchip: add support for vlan operations

[Prasanna Vengateshan <prasanna.vengateshan@microchip.com>]

Hi Vladimir,

On Thu, 2021-04-22 at 22:03 +0300, Vladimir Oltean wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> On Thu, Apr 22, 2021 at 03:12:57PM +0530, Prasanna Vengateshan wrote:
> > Support for VLAN add, del, prepare and filtering operations.
> > 
> > It aligns with latest update of removing switchdev
> > transactional logic from VLAN objects
> 
> Maybe more in the commit message about what the patch does, as opposed
> to mentioning that you had to rebase it, would be helpful.
Sure.

> 
> > 
> > Signed-off-by: Prasanna Vengateshan <prasanna.vengateshan@microchip.com>
> > ---
> >  drivers/net/dsa/microchip/lan937x_main.c | 214 +++++++++++++++++++++++
> >  1 file changed, 214 insertions(+)
> > 
> > diff --git a/drivers/net/dsa/microchip/lan937x_main.c
> > b/drivers/net/dsa/microchip/lan937x_main.c
> > index 7f6183dc0e31..35f3456c3506 100644
> 
> > +
> > +             rc = lan937x_cfg(dev, REG_SW_LUE_CTRL_0, SW_VLAN_ENABLE,
> > true);
> 
> How about this bit?

> 
> I see one bit is per port and the other is global.
> Just FYI, you can have this configuration:
> 
> ip link add br0 type bridge vlan_filtering 0
> ip link add br1 type bridge vlan_filtering 1
> ip link set swp0 master br0
> ip link set swp1 master br0
> ip link set swp2 master br1
> ip link set swp3 master br1
> 
> Do the swp0 and swp1 ports remain VLAN-unaware after you touch this
> REG_SW_LUE_CTRL_0 bit?
vlan aware is global, so ds->vlan_filtering_is_global needs to be true if VLAN
aware is global, will fix this in the next version

> 
> > +     } else {
> > +             rc = lan937x_cfg(dev, REG_SW_LUE_CTRL_0, SW_VLAN_ENABLE,
> > false);
> > +             if (rc < 0)
> > +                     return rc;
> > +
> > +             rc = lan937x_port_cfg(dev, port, REG_PORT_LUE_CTRL,
> > +                                   PORT_VLAN_LOOKUP_VID_0, false);
> > +     }
> > +
> > +     return rc;
> > +}
> > +
> > +static int lan937x_port_vlan_add(struct dsa_switch *ds, int port,
> > +                              const struct switchdev_obj_port_vlan *vlan,
> > +                              struct netlink_ext_ack *extack)
> > +{
> > +     bool untagged = vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED;
> > +     struct ksz_device *dev = ds->priv;
> > +     u32 vlan_table[3];
> 
> Maybe a structure would be nicer to read than an u32 array?
Okay, will make a structure.

> 
> > +     int rc;
> > +
> > +     rc = lan937x_get_vlan_table(dev, vlan->vid, vlan_table);
> > +     if (rc < 0) {
> > +             dev_err(dev->dev, "Failed to get vlan table\n");
> 
> One of the reasons for which the extack exists is so that you can report
> errors to user space and not to the console.
Sure, will add it for port_vlan_del() as well

> 
>                 NL_SET_ERR_MSG_MOD(extack, "Failed to get vlan table");
> 
> > +             return rc;
> > +     }
> > +
> > +     vlan_table[0] = VLAN_VALID | (vlan->vid & VLAN_FID_M);
> > +
> > +     /* set/clear switch port when updating vlan table registers */
> > +     if (untagged)
> > +             vlan_table[1] |= BIT(port);
> > +     else
> > +             vlan_table[1] &= ~BIT(port);
> > +     vlan_table[1] &= ~(BIT(dev->cpu_port));
> > +
> > +     vlan_table[2] |= BIT(port) | BIT(dev->cpu_port);
> 
> What's the business with the CPU port here? Does DSA not call
> .port_vlan_add for the CPU port separately?
Calls for CPU port as well. This is to be removed.

> 
> > +
> > +     rc = lan937x_set_vlan_table(dev, vlan->vid, vlan_table);
> > +     if (rc < 0) {
> > +             dev_err(dev->dev, "Failed to set vlan table\n");
> > +             return rc;
> > +     }
> > +
> > +     /* change PVID */
> > +     if (vlan->flags & BRIDGE_VLAN_INFO_PVID) {
> > +             rc = lan937x_pwrite16(dev, port, REG_PORT_DEFAULT_VID, vlan-
> > >vid);
> > +
> > +             if (rc < 0) {
> > +                     dev_err(dev->dev, "Failed to set pvid\n");
> > +                     return rc;
> > +             }
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static int lan937x_port_vlan_del(struct dsa_switch *ds, int port,
> > +                              const struct switchdev_obj_port_vlan *vlan)
> > +{
> > +     bool untagged = vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED;
> > +     struct ksz_device *dev = ds->priv;
> > +     u32 vlan_table[3];
> > +     u16 pvid;
> > +     int rc;
> > +
> > +     lan937x_pread16(dev, port, REG_PORT_DEFAULT_VID, &pvid);
> > +     pvid &= 0xFFF;
> > +
> > +     rc = lan937x_get_vlan_table(dev, vlan->vid, vlan_table);
> > +
> > +     if (rc < 0) {
> > +             dev_err(dev->dev, "Failed to get vlan table\n");
> > +             return rc;
> > +     }
> > +     /* clear switch port number */
> > +     vlan_table[2] &= ~BIT(port);
> > +
> > +     if (pvid == vlan->vid)
> > +             pvid = 1;
> 
> According to Documentation/networking/switchdev.rst:
> 
> When the bridge has VLAN filtering enabled and a PVID is not configured on the
> ingress port, untagged and 802.1p tagged packets must be dropped. When the
> bridge
> has VLAN filtering enabled and a PVID exists on the ingress port, untagged and
> priority-tagged packets must be accepted and forwarded according to the
> bridge's port membership of the PVID VLAN. When the bridge has VLAN filtering
> disabled, the presence/lack of a PVID should not influence the packet
> forwarding decision.
> 
> So please don't reset the pvid.
Will remove it in the next rev.

> 
> > +
> > +     if (untagged)
> > +             vlan_table[1] &= ~BIT(port);
> > +
> > +     rc = lan937x_set_vlan_table(dev, vlan->vid, vlan_table);
> > +     if (rc < 0) {
> > +             dev_err(dev->dev, "Failed to set vlan table\n");
> > +             return rc;
> > +     }
> > +
> > +     rc = lan937x_pwrite16(dev, port, REG_PORT_DEFAULT_VID, pvid);
> > +
> > +     if (rc < 0) {
> > +             dev_err(dev->dev, "Failed to set pvid\n");
> > +             return rc;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> >  static u8 lan937x_get_fid(u16 vid)
> >  {
> >       if (vid > ALU_FID_SIZE)
> > @@ -955,6 +1166,9 @@ const struct dsa_switch_ops lan937x_switch_ops = {
> >       .port_bridge_flags      = lan937x_port_bridge_flags,
> >       .port_stp_state_set     = lan937x_port_stp_state_set,
> >       .port_fast_age          = ksz_port_fast_age,
> > +     .port_vlan_filtering    = lan937x_port_vlan_filtering,
> > +     .port_vlan_add          = lan937x_port_vlan_add,
> > +     .port_vlan_del          = lan937x_port_vlan_del,
> >       .port_fdb_dump          = lan937x_port_fdb_dump,
> >       .port_fdb_add           = lan937x_port_fdb_add,
> >       .port_fdb_del           = lan937x_port_fdb_del,
> > --
> > 2.27.0
> >