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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 4 Aug 2020 08:13:18 +0000
From:   Hongbo Wang <hongbo.wang@....com>
To:     Florian Fainelli <f.fainelli@...il.com>,
        Xiaoliang Yang <xiaoliang.yang_1@....com>,
        "allan.nielsen@...rochip.com" <allan.nielsen@...rochip.com>,
        Po Liu <po.liu@....com>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Alexandru Marginean <alexandru.marginean@....com>,
        Vladimir Oltean <vladimir.oltean@....com>,
        Leo Li <leoyang.li@....com>, Mingkai Hu <mingkai.hu@....com>,
        "andrew@...n.ch" <andrew@...n.ch>,
        "vivien.didelot@...il.com" <vivien.didelot@...il.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "jiri@...nulli.us" <jiri@...nulli.us>,
        "idosch@...sch.org" <idosch@...sch.org>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "vinicius.gomes@...el.com" <vinicius.gomes@...el.com>,
        "nikolay@...ulusnetworks.com" <nikolay@...ulusnetworks.com>,
        "roopa@...ulusnetworks.com" <roopa@...ulusnetworks.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "horatiu.vultur@...rochip.com" <horatiu.vultur@...rochip.com>,
        "alexandre.belloni@...tlin.com" <alexandre.belloni@...tlin.com>,
        "UNGLinuxDriver@...rochip.com" <UNGLinuxDriver@...rochip.com>,
        "ivecera@...hat.com" <ivecera@...hat.com>
Subject: RE: [EXT] Re: [PATCH v4 2/2] net: dsa: ocelot: Add support for QinQ
 Operation

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