RE: [EXT] Re: [PATCH v4 2/2] net: dsa: ocelot: Add support for QinQ Operation

From: Hongbo Wang
Date: Tue Aug 04 2020 - 04:13:26 EST


> > This featue can be test using network test tools
>
> mispelled: feature, can be used to test network test tools? or can be used to
> exercise network test tool?

when testing this feature, I need network tool to send packet with VLAN tag(pcp proto and vid), I will change it to avoid ambiguity.

>
> > struct ocelot *ocelot = ds->priv;
> > + struct ocelot_port *ocelot_port = ocelot->ports[port];
> > u16 vid;
> > int err;
> >
> > + if (vlan->proto == ETH_P_8021AD) {
> > + ocelot->enable_qinq = false;
> > + ocelot_port->qinq_mode = false;
> > + }
>
> You need the delete part to be reference counted, otherwise the first 802.1AD
> VLAN delete request that comes in, regardless of whether other 802.1AD VLAN
> entries are installed will disable qinq_mode and enable_qinq for the entire
> port and switch, that does not sound like what you want.

I will add reference count in next version.
maybe I can disable qinq_mode and enable_qinq only when deleting pvid 1, I will test and change it.

>
> Is not ocelot->enable_qinq the logical or of all ocelo_port instances's
> qinq_mode as well?

enable_qinq is flag of switch, and qinq_mode is flag of single port,
if switch is in working in QinQ mode, some ports that linked to ISP networking should enable qinq_mode,
other ports that linked to customer networking don't need set qinq_mode. these two types of port have different action.

> > else
> > /* Tag all frames */
> > val = REW_TAG_CFG_TAG_CFG(3);
> > +
> > + if (ocelot_port->qinq_mode)
> > + tag_tpid = REW_TAG_CFG_TAG_TPID_CFG(1);
> > + else
> > + tag_tpid = REW_TAG_CFG_TAG_TPID_CFG(0);
> > } else {
> > - /* Port tagging disabled. */
> > - val = REW_TAG_CFG_TAG_CFG(0);
> > + if (ocelot_port->qinq_mode) {
> > + if (ocelot_port->vid)
> > + val = REW_TAG_CFG_TAG_CFG(1);
> > + else
> > + val = REW_TAG_CFG_TAG_CFG(3);
> > +> + tag_tpid = REW_TAG_CFG_TAG_TPID_CFG(1);
>
> This is nearly the same branch as the one above, can you merge the conditions
> vlan_aware || qinq_mode and just set an appropriate TAG_CFG() register value
> based on either booleans?

this feature needs vlan_filter=1, so the branch if (vlan_filter == 0 && qinq_mode) can be deleted now.
I will optimize the related code.

>
> Are you also not possibly missing a if (untaged || qinq_mode) check in
> ocelot_vlan_add() to call into ocelot_port_set_native_vlan()?

The qinq_mode action can be triggered by ocelot_port_vlan_filtering, so don't need if (untagged || qinq_mode).

Thanks!
Hongbo