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 1/2] net: dsa: Add protocol support for 802.1AD when
 adding or deleting vlan for dsa switch port
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-2-hongbo.wang@nxp.com>
From:   Florian Fainelli <f.fainelli@gmail.com>
Message-ID: <effcb1fe-79ed-ed79-ffe3-977ed9aa006e@gmail.com>
Date:   Mon, 3 Aug 2020 15:38:31 -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-2-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>
> 
> the following command will be supported:
> 
> Set bridge's vlan protocol:
>     ip link set br0 type bridge vlan_protocol 802.1ad
> Add VLAN:
>     ip link add link swp1 name swp1.100 type vlan protocol 802.1ad id 100
> Delete VLAN:
>     ip link del link swp1 name swp1.100
> 
> Signed-off-by: hongbo.wang <hongbo.wang@nxp.com>
> ---
>  include/net/switchdev.h |  1 +
>  net/dsa/dsa_priv.h      |  4 ++--
>  net/dsa/port.c          |  6 ++++--
>  net/dsa/slave.c         | 27 +++++++++++++++++++++------
>  net/dsa/tag_8021q.c     |  4 ++--
>  5 files changed, 30 insertions(+), 12 deletions(-)
> 
> diff --git a/include/net/switchdev.h b/include/net/switchdev.h
> index ff2246914301..7594ea82879f 100644
> --- a/include/net/switchdev.h
> +++ b/include/net/switchdev.h
> @@ -97,6 +97,7 @@ struct switchdev_obj_port_vlan {
>  	u16 flags;
>  	u16 vid_begin;
>  	u16 vid_end;
> +	u16 proto;

You are adding a new member to the switchdev VLAN object, so you should
make sure that all call paths creating and parsing that object get
updated as well, for now, you are doing this solely within DSA which is
probably reasonable if we assume proto is uninitialized and unused
elsewhere, there is no change of functionality.

[snip]

> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> index 41d60eeefdbd..2a03da92af0a 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -1233,7 +1233,10 @@ static int dsa_slave_vlan_rx_add_vid(struct net_device *dev, __be16 proto,
>  				     u16 vid)
>  {
>  	struct dsa_port *dp = dsa_slave_to_port(dev);
> +	u16 vlan_proto = ntohs(proto);
>  	struct bridge_vlan_info info;
> +	bool change_proto = false;
> +	u16 br_proto = 0;
>  	int ret;
>  
>  	/* Check for a possible bridge VLAN entry now since there is no
> @@ -1243,20 +1246,24 @@ static int dsa_slave_vlan_rx_add_vid(struct net_device *dev, __be16 proto,
>  		if (dsa_port_skip_vlan_configuration(dp))
>  			return 0;
>  
> +		ret = br_vlan_get_proto(dp->bridge_dev, &br_proto);
> +		if (ret == 0 && br_proto != vlan_proto)
> +			change_proto = true;


This deserves a comment, because the change_proto variable is not really
explaining what this is about, maybe more like "incompatible_proto" would?

First you query the VLAN protocol currently configured on the bridge
master device, and if this VLAN protocol is different than the one being
requested, then you treat this as an error. It might make sense to also
print a message towards the user that the bridge device protocol should
be changed, or that the bridge device should be removed and re-created
accordingly.

Does it not work if we have a bridge currently configured with 802.1ad
and a 802.1q VLAN programming request comes in? In premise it should,
right? Likewise, if we had a 802.1ad bridge configured already and we
want to configure a 802.1Q VLAN on a bridged port, there should be a way
for this configuration to work.

And both cases, it ought to be possible to configure the switch in
double tagged mode and just make sure that there is no S-tag being added
unless requested.

> +
>  		/* br_vlan_get_info() returns -EINVAL or -ENOENT if the
>  		 * device, respectively the VID is not found, returning
>  		 * 0 means success, which is a failure for us here.
>  		 */
>  		ret = br_vlan_get_info(dp->bridge_dev, vid, &info);
> -		if (ret == 0)
> +		if (ret == 0 && !change_proto)
>  			return -EBUSY;
>  	}
>  
> -	ret = dsa_port_vid_add(dp, vid, 0);
> +	ret = dsa_port_vid_add(dp, vid, vlan_proto, 0);
>  	if (ret)
>  		return ret;
>  
> -	ret = dsa_port_vid_add(dp->cpu_dp, vid, 0);
> +	ret = dsa_port_vid_add(dp->cpu_dp, vid, 0, 0);
>  	if (ret)
>  		return ret;
>  
> @@ -1267,7 +1274,10 @@ static int dsa_slave_vlan_rx_kill_vid(struct net_device *dev, __be16 proto,
>  				      u16 vid)
>  {
>  	struct dsa_port *dp = dsa_slave_to_port(dev);
> +	u16 vlan_proto = ntohs(proto);
>  	struct bridge_vlan_info info;
> +	bool change_proto = false;
> +	u16 br_proto = 0;
>  	int ret;
>  
>  	/* Check for a possible bridge VLAN entry now since there is no
> @@ -1277,19 +1287,23 @@ static int dsa_slave_vlan_rx_kill_vid(struct net_device *dev, __be16 proto,
>  		if (dsa_port_skip_vlan_configuration(dp))
>  			return 0;
>  
> +		ret = br_vlan_get_proto(dp->bridge_dev, &br_proto);
> +		if (ret == 0 && br_proto != vlan_proto)
> +			change_proto = true;
> +
>  		/* br_vlan_get_info() returns -EINVAL or -ENOENT if the
>  		 * device, respectively the VID is not found, returning
>  		 * 0 means success, which is a failure for us here.
>  		 */
>  		ret = br_vlan_get_info(dp->bridge_dev, vid, &info);
> -		if (ret == 0)
> +		if (ret == 0 && !change_proto)
>  			return -EBUSY;

Since we are copying the same code than in the add_vid path, it might
make sense to extract this to a helper function eventually.

>  	}
>  
>  	/* Do not deprogram the CPU port as it may be shared with other user
>  	 * ports which can be members of this VLAN as well.
>  	 */
> -	return dsa_port_vid_del(dp, vid);
> +	return dsa_port_vid_del(dp, vid, vlan_proto);
>  }
>  
>  struct dsa_hw_port {
> @@ -1744,7 +1758,8 @@ int dsa_slave_create(struct dsa_port *port)
>  
>  	slave_dev->features = master->vlan_features | NETIF_F_HW_TC;
>  	if (ds->ops->port_vlan_add && ds->ops->port_vlan_del)
> -		slave_dev->features |= NETIF_F_HW_VLAN_CTAG_FILTER;
> +		slave_dev->features |= NETIF_F_HW_VLAN_CTAG_FILTER |
> +				       NETIF_F_HW_VLAN_STAG_FILTER;

You cannot advertise this netdev feature for *all* DSA switch driver
unless you have verified that each DSA driver implementing
port_vlan_add() will work correctly. Please assign this flag from within
the ocelot driver for now.

>  	slave_dev->hw_features |= NETIF_F_HW_TC;
>  	slave_dev->features |= NETIF_F_LLTX;
>  	slave_dev->ethtool_ops = &dsa_slave_ethtool_ops;
> diff --git a/net/dsa/tag_8021q.c b/net/dsa/tag_8021q.c
> index 780b2a15ac9b..848f85ed5c0f 100644
> --- a/net/dsa/tag_8021q.c
> +++ b/net/dsa/tag_8021q.c
> @@ -152,9 +152,9 @@ static int dsa_8021q_vid_apply(struct dsa_switch *ds, int port, u16 vid,
>  	struct dsa_port *dp = dsa_to_port(ds, port);
>  
>  	if (enabled)
> -		return dsa_port_vid_add(dp, vid, flags);
> +		return dsa_port_vid_add(dp, vid, 0, flags);

Why not pass ETH_P_8021Q here to indicate we want a 802.1Q, not .AD
configuration request?
-- 
Florian