Re: [PATCH] dsa: vsc73xx: add support for vlan filtering

[Paweł Dembicki]

czw., 21 sty 2021 o 23:45 Vladimir Oltean <olteanv@xxxxxxxxx> napisał(a):
>
> Hi Pawel,
>

Hi Vladimir,
Thank You for Your answer.

> On Wed, Jan 20, 2021 at 07:30:18AM +0100, Pawel Dembicki wrote:
> > This patch adds support for vlan filtering in vsc73xx driver.
> >
> > After vlan filtering enable, CPU_PORT is configured as trunk, without
> > non-tagged frames. This allows to avoid problems with transmit untagged
> > frames because vsc73xx is DSA_TAG_PROTO_NONE.
> >
> > Signed-off-by: Pawel Dembicki <paweldembicki@xxxxxxxxx>
>
> What are the issues that are preventing you from getting rid of
> DSA_TAG_PROTO_NONE? Not saying that making the driver VLAN aware is a
> bad idea, but maybe also adding a tagging driver should really be the
> path going forward. If there are hardware issues surrounding the native
> tagging support, then DSA can make use of your VLAN features by
> transforming them into a software-defined tagger, see
> net/dsa/tag_8021q.c. But using a trunk CPU port with 8021q uppers on top
> of the DSA master is a poor job of achieving that.
>

I was planning to prepare tagging for the next step. Without VLAN
filtering and/or tagging it is usable only as a full bridge.
Vsc73xx devices support QinQ. I can use double tagging for port
separation, but then it's impossible to filter vlans.
So, I'm planning to start working with tagging based on
net/dsa/tag_8021q.c. Should I wait with this patch and send the
corrected version with tagging support?

> > ---
> > +static int
> > +vsc73xx_port_read_vlan_table_entry(struct dsa_switch *ds, u16 vid, u8 *portmap)
> > +{
> > +     struct vsc73xx *vsc = ds->priv;
> > +     u32 val;
> > +     int ret;
> > +
> > +     if (vid > 4095)
> > +             return -EPERM;
>
> This is a paranoid check and should be removed (not only here but
> everywhere).
>
> > +static int vsc73xx_port_vlan_prepare(struct dsa_switch *ds, int port,
> > +                                  const struct switchdev_obj_port_vlan *vlan)
> > +{
> > +     /* nothing needed */
> > +     return 0;
> > +}
>
> Can you please rebase your work on top of the net-next/master branch?
> You will see that the API has changed.
> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git
>
> > +
> > +static void vsc73xx_port_vlan_add(struct dsa_switch *ds, int port,
> > +                               const struct switchdev_obj_port_vlan *vlan)
> > +{
> > +     bool untagged = vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED;
> > +     bool pvid = vlan->flags & BRIDGE_VLAN_INFO_PVID;
> > +     struct vsc73xx *vsc = ds->priv;
> > +     int ret;
> > +     u32 tmp;
> > +
> > +     if (!dsa_port_is_vlan_filtering(dsa_to_port(ds, port)))
> > +             return;
>
> Sorry, but no. You need to support the case where the bridge (or 8021q
> module) adds a VLAN even when the port is not enforcing VLAN filtering.
> See commit:
> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=0ee2af4ebbe3c4364429859acd571018ebfb3424
>
> > +
> > +     ret = vsc73xx_port_update_vlan_table(ds, port, vlan->vid_begin,
> > +                                          vlan->vid_end, 1);
> > +     if (ret)
> > +             return;
> > +
> > +     if (untagged && port != CPU_PORT) {
> > +             /* VSC73xx can have only one untagged vid per port. */
> > +             vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port,
> > +                          VSC73XX_TXUPDCFG, &tmp);
> > +
> > +             if (tmp & VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA)
> > +                     dev_warn(vsc->dev,
> > +                              "Chip support only one untagged VID per port. Overwriting...\n");
>
> Just return an error, don't overwrite, this leaves the bridge VLAN
> information out of sync with the hardware otherwise, which is not a
> great idea.
>

But it's a void return function. It always will be out of sync after
the second untagged VID attemption.
Should I give warning without changing untagged vlan?

> FWIW the drivers/net/dsa/ocelot/felix.c and drivers/net/mscc/ocelot.c
> files support switching chips from the same vendor. The VSC73XX family
> is much older, but some of the limitations apply to both architectures
> nonetheless (like this one), you can surely borrow some ideas from
> ocelot - in this case search for ocelot_vlan_prepare.
>

As Linus said, It's impossible because vsc73xx doesn't support tags
for external cpu via rgmii or regular port. This chip is very limited.

> > +
> > +             vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> > +                                 VSC73XX_TXUPDCFG,
> > +                                 VSC73XX_TXUPDCFG_TX_UNTAGGED_VID,
> > +                                 (vlan->vid_end <<
> > +                                 VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_SHIFT) &
> > +                                 VSC73XX_TXUPDCFG_TX_UNTAGGED_VID);
> > +             vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> > +                                 VSC73XX_TXUPDCFG,
> > +                                 VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA,
> > +                                 VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA);
> > +     }
> > +     if (pvid && port != CPU_PORT) {
> > +             vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> > +                                 VSC73XX_CAT_DROP,
> > +                                 VSC73XX_CAT_DROP_UNTAGGED_ENA,
> > +                                 ~VSC73XX_CAT_DROP_UNTAGGED_ENA);
> > +             vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> > +                                 VSC73XX_CAT_PORT_VLAN,
> > +                                 VSC73XX_CAT_PORT_VLAN_VLAN_VID,
> > +                                 vlan->vid_end &
> > +                                 VSC73XX_CAT_PORT_VLAN_VLAN_VID);
> > +     }
> > +}


Best Regards,
Pawel Dembicki