Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v4 2/2] net: dsa: ocelot: Add support for QinQ Operation
To:     hongbo.wang@nxp.com, xiaoliang.yang_1@nxp.com,
        allan.nielsen@microchip.com, po.liu@nxp.com,
        claudiu.manoil@nxp.com, alexandru.marginean@nxp.com,
        vladimir.oltean@nxp.com, leoyang.li@nxp.com, mingkai.hu@nxp.com,
        andrew@lunn.ch, vivien.didelot@gmail.com, davem@davemloft.net,
        jiri@resnulli.us, idosch@idosch.org, kuba@kernel.org,
        vinicius.gomes@intel.com, nikolay@cumulusnetworks.com,
        roopa@cumulusnetworks.com, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org, horatiu.vultur@microchip.com,
        alexandre.belloni@bootlin.com, UNGLinuxDriver@microchip.com,
        ivecera@redhat.com
References: <20200730102505.27039-1-hongbo.wang@nxp.com>
 <20200730102505.27039-3-hongbo.wang@nxp.com>
From:   Florian Fainelli <f.fainelli@gmail.com>
Message-ID: <b7021ec2-b0bb-d5bf-9b69-2e490d7191e8@gmail.com>
Date:   Mon, 3 Aug 2020 15:47:00 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101
 Firefox/68.0 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <20200730102505.27039-3-hongbo.wang@nxp.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


On 7/30/2020 3:25 AM, hongbo.wang@nxp.com wrote:
> From: "hongbo.wang" <hongbo.wang@nxp.com>
> 
> 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?

> TX-tool -----> swp0  -----> swp1 -----> RX-tool
> 
> TX-tool simulates Customer that will send and receive packets with single
> VLAN tag(CTAG), RX-tool simulates Service-Provider that will send and
> receive packets with double VLAN tag(STAG and CTAG). This refers to
> "4.3.3 Provider Bridges and Q-in-Q Operation" in VSC99599_1_00_TS.pdf.
> 
> The related test commands:
> 1.
> ip link add dev br0 type bridge
> ip link set dev swp1 master br0
> ip link set br0 type bridge vlan_protocol 802.1ad
> ip link set dev swp0 master br0
> 
> 2.
> ip link set dev br0 type bridge vlan_filtering 1
> bridge vlan add dev swp0 vid 100 pvid
> bridge vlan add dev swp1 vid 100
> Result:
> Customer(tpid:8100 vid:111) -> swp0 -> swp1 -> ISP(STAG \
>                 tpid:88A8 vid:100, CTAG tpid:8100 vid:111)
> 
> 3.
> bridge vlan del dev swp0 vid 1 pvid
> bridge vlan add dev swp0 vid 100 pvid untagged
> Result:
> ISP(tpid:88A8 vid:100 tpid:8100 vid:222) -> swp1 -> swp0 ->\
> 		Customer(tpid:8100 vid:222)
> 
> Signed-off-by: hongbo.wang <hongbo.wang@nxp.com>
> ---
>  drivers/net/dsa/ocelot/felix.c     | 12 +++++++
>  drivers/net/ethernet/mscc/ocelot.c | 53 +++++++++++++++++++++++++-----
>  include/soc/mscc/ocelot.h          |  2 ++
>  3 files changed, 59 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
> index c69d9592a2b7..72a27b61080e 100644
> --- a/drivers/net/dsa/ocelot/felix.c
> +++ b/drivers/net/dsa/ocelot/felix.c
> @@ -131,10 +131,16 @@ static void felix_vlan_add(struct dsa_switch *ds, int port,
>  			   const struct switchdev_obj_port_vlan *vlan)
>  {
>  	struct ocelot *ocelot = ds->priv;
> +	struct ocelot_port *ocelot_port = ocelot->ports[port];
>  	u16 flags = vlan->flags;
>  	u16 vid;
>  	int err;
>  
> +	if (vlan->proto == ETH_P_8021AD) {
> +		ocelot->enable_qinq = true;
> +		ocelot_port->qinq_mode = true;
> +	}
> +
>  	if (dsa_is_cpu_port(ds, port))
>  		flags &= ~BRIDGE_VLAN_INFO_UNTAGGED;
>  
> @@ -154,9 +160,15 @@ static int felix_vlan_del(struct dsa_switch *ds, int port,
>  			  const struct switchdev_obj_port_vlan *vlan)
>  {
>  	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.

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

> +
>  	for (vid = vlan->vid_begin; vid <= vlan->vid_end; vid++) {
>  		err = ocelot_vlan_del(ocelot, port, vid);
>  		if (err) {
> diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
> index f2d94b026d88..b5fec6855afd 100644
> --- a/drivers/net/ethernet/mscc/ocelot.c
> +++ b/drivers/net/ethernet/mscc/ocelot.c
> @@ -143,6 +143,8 @@ static int ocelot_port_set_native_vlan(struct ocelot *ocelot, int port,
>  				       u16 vid)
>  {
>  	struct ocelot_port *ocelot_port = ocelot->ports[port];
> +	u32 port_tpid = 0;
> +	u32 tag_tpid = 0;
>  	u32 val = 0;
>  
>  	if (ocelot_port->vid != vid) {
> @@ -156,8 +158,14 @@ static int ocelot_port_set_native_vlan(struct ocelot *ocelot, int port,
>  		ocelot_port->vid = vid;
>  	}
>  
> -	ocelot_rmw_gix(ocelot, REW_PORT_VLAN_CFG_PORT_VID(vid),
> -		       REW_PORT_VLAN_CFG_PORT_VID_M,
> +	if (ocelot_port->qinq_mode)
> +		port_tpid = REW_PORT_VLAN_CFG_PORT_TPID(ETH_P_8021AD);
> +	else
> +		port_tpid = REW_PORT_VLAN_CFG_PORT_TPID(ETH_P_8021Q);
> +
> +	ocelot_rmw_gix(ocelot, REW_PORT_VLAN_CFG_PORT_VID(vid) | port_tpid,
> +		       REW_PORT_VLAN_CFG_PORT_VID_M |
> +		       REW_PORT_VLAN_CFG_PORT_TPID_M,
>  		       REW_PORT_VLAN_CFG, port);
>  
>  	if (ocelot_port->vlan_aware && !ocelot_port->vid)
> @@ -180,12 +188,28 @@ static int ocelot_port_set_native_vlan(struct ocelot *ocelot, int port,
>  		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?

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