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@xxxxxxxxxxxxx>
> > ---
> > 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
> >
