Re: [PATCH net-next v3 4/7] net: lan966x: Extend lan966x_foreign_bridging_check

From: Vladimir Oltean <vladimir.oltean@nxp.com>
To: Horatiu Vultur <horatiu.vultur@microchip.com>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"UNGLinuxDriver@microchip.com" <UNGLinuxDriver@microchip.com>,
	"davem@davemloft.net" <davem@davemloft.net>,
	"edumazet@google.com" <edumazet@google.com>,
	"kuba@kernel.org" <kuba@kernel.org>,
	"pabeni@redhat.com" <pabeni@redhat.com>
Subject: Re: [PATCH net-next v3 4/7] net: lan966x: Extend lan966x_foreign_bridging_check
Date: Sat, 2 Jul 2022 14:30:08 +0000	[thread overview]
Message-ID: <20220702143007.wdnup4sapkl2247p@skbuf> (raw)
In-Reply-To: <20220701205227.1337160-5-horatiu.vultur@microchip.com>

On Fri, Jul 01, 2022 at 10:52:24PM +0200, Horatiu Vultur wrote:
> Extend lan966x_foreign_bridging_check to check also if the upper
> interface is a lag device. Don't allow a lan966x port to be part of a
> lag if it has foreign interfaces.
> 
> Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> ---
>  .../microchip/lan966x/lan966x_switchdev.c     | 32 ++++++++++++++-----
>  1 file changed, 24 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c b/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c
> index d9b3ca5f6214..fe872edfcdca 100644
> --- a/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c
> +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c
> @@ -326,23 +326,25 @@ static int lan966x_port_prechangeupper(struct net_device *dev,
>  	return NOTIFY_DONE;
>  }
>  
> -static int lan966x_foreign_bridging_check(struct net_device *bridge,
> +static int lan966x_foreign_bridging_check(struct net_device *upper,
> +					  bool *has_foreign,
> +					  bool *seen_lan966x,
>  					  struct netlink_ext_ack *extack)
>  {
>  	struct lan966x *lan966x = NULL;
> -	bool has_foreign = false;
>  	struct net_device *dev;
>  	struct list_head *iter;
>  
> -	if (!netif_is_bridge_master(bridge))
> +	if (!netif_is_bridge_master(upper) &&
> +	    !netif_is_lag_master(upper))
>  		return 0;
>  
> -	netdev_for_each_lower_dev(bridge, dev, iter) {
> +	netdev_for_each_lower_dev(upper, dev, iter) {
>  		if (lan966x_netdevice_check(dev)) {
>  			struct lan966x_port *port = netdev_priv(dev);
>  
>  			if (lan966x) {
> -				/* Bridge already has at least one port of a
> +				/* Upper already has at least one port of a
>  				 * lan966x switch inside it, check that it's
>  				 * the same instance of the driver.
>  				 */
> @@ -353,15 +355,24 @@ static int lan966x_foreign_bridging_check(struct net_device *bridge,
>  				}
>  			} else {
>  				/* This is the first lan966x port inside this
> -				 * bridge
> +				 * upper device
>  				 */
>  				lan966x = port->lan966x;
> +				*seen_lan966x = true;
>  			}
> +		} else if (netif_is_lag_master(dev)) {
> +			/* Allow to have bond interfaces that have only lan966x
> +			 * devices
> +			 */
> +			if (lan966x_foreign_bridging_check(dev, has_foreign,
> +							   seen_lan966x,
> +							   extack))
> +				*has_foreign = true;

Not clear why you set *has_foreign here and not just stop and return.
The extack has presumably already been populated by the called function,
there is absolutely no need to continue if an error has already been found.

>  		} else {
> -			has_foreign = true;
> +			*has_foreign = true;
>  		}
>  
> -		if (lan966x && has_foreign) {
> +		if (*seen_lan966x && *has_foreign) {
>  			NL_SET_ERR_MSG_MOD(extack,
>  					   "Bridging lan966x ports with foreign interfaces disallowed");
>  			return -EINVAL;
> @@ -374,7 +385,12 @@ static int lan966x_foreign_bridging_check(struct net_device *bridge,
>  static int lan966x_bridge_check(struct net_device *dev,
>  				struct netdev_notifier_changeupper_info *info)
>  {
> +	bool has_foreign = false;
> +	bool seen_lan966x = false;
> +
>  	return lan966x_foreign_bridging_check(info->upper_dev,
> +					      &has_foreign,
> +					      &seen_lan966x,
>  					      info->info.extack);
>  }
>  
> -- 
> 2.33.0
>