czw., 21 sty 2021 o 23:45 Vladimir Oltean <olte...@gmail.com> 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 <paweldembi...@gmail.com>
>
> 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
