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

[Hongbo Wang]

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