Re: [PATCH v2 net-next 7/7] net: dsa: eliminate dsa_switch_ops :: port_bridge_tx_fwd_{,un}offload

["Alvin Šipraga" <ALSI@bang-olufsen.dk>]

On 12/4/21 21:11, Vladimir Oltean wrote:
> We don't really need new switch API for these, and with new switches
> which intend to add support for this feature, it will become cumbersome
> to maintain.
> 
> The change consists in restructuring the two drivers that implement this
> offload (sja1105 and mv88e6xxx) such that the offload is enabled and
> disabled from the ->port_bridge_{join,leave} methods instead of the old
> ->port_bridge_tx_fwd_{,un}offload.
> 
> The only non-trivial change is that mv88e6xxx_map_virtual_bridge_to_pvt()
> has been moved to avoid a forward declaration, and the
> mv88e6xxx_reg_lock() calls from inside it have been removed, since
> locking is now done from mv88e6xxx_port_bridge_{join,leave}.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---

Reviewed-by: Alvin Šipraga <alsi@bang-olufsen.dk>

> v1->v2: avoid a deadlock in mv88e6xxx pointed out by Tobias due to the
>          register lock being already held in the port_bridge_join/leave
>          methods. Again had to drop Alvin's Reviewed-by tag.
> 
>   drivers/net/dsa/mv88e6xxx/chip.c       | 63 ++++++++++----------------
>   drivers/net/dsa/sja1105/sja1105_main.c | 19 ++++++--
>   include/net/dsa.h                      |  7 +--
>   net/dsa/port.c                         | 39 ++--------------
>   4 files changed, 45 insertions(+), 83 deletions(-)
> 
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> index c30d1f825776..ec515045c16e 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -2451,6 +2451,19 @@ static int mv88e6xxx_bridge_map(struct mv88e6xxx_chip *chip,
>   	return 0;
>   }
>   
> +/* Treat the software bridge as a virtual single-port switch behind the
> + * CPU and map in the PVT. First dst->last_switch elements are taken by
> + * physical switches, so start from beyond that range.
> + */
> +static int mv88e6xxx_map_virtual_bridge_to_pvt(struct dsa_switch *ds,
> +					       unsigned int bridge_num)
> +{
> +	u8 dev = bridge_num + ds->dst->last_switch;
> +	struct mv88e6xxx_chip *chip = ds->priv;
> +
> +	return mv88e6xxx_pvt_map(chip, dev, 0);
> +}
> +
>   static int mv88e6xxx_port_bridge_join(struct dsa_switch *ds, int port,
>   				      struct dsa_bridge bridge,
>   				      bool *tx_fwd_offload)
> @@ -2468,6 +2481,14 @@ static int mv88e6xxx_port_bridge_join(struct dsa_switch *ds, int port,
>   	if (err)
>   		goto unlock;
>   
> +	if (mv88e6xxx_has_pvt(chip)) {
> +		err = mv88e6xxx_map_virtual_bridge_to_pvt(ds, bridge.num);
> +		if (err)
> +			goto unlock;
> +
> +		*tx_fwd_offload = true;
> +	}
> +
>   unlock:
>   	mv88e6xxx_reg_unlock(chip);
>   
> @@ -2482,6 +2503,10 @@ static void mv88e6xxx_port_bridge_leave(struct dsa_switch *ds, int port,
>   
>   	mv88e6xxx_reg_lock(chip);
>   
> +	if (bridge.tx_fwd_offload &&
> +	    mv88e6xxx_map_virtual_bridge_to_pvt(ds, bridge.num))
> +		dev_err(ds->dev, "failed to remap cross-chip Port VLAN\n");

I must admit I don't see how this would ever fail in practice. Moreover, 
isn't this the exact same call made in port_bridge_join, hence a noop? 
Anyway, that seems to have been the case before as well, so it is not 
really a problem for your patch series.

> +
>   	if (mv88e6xxx_bridge_map(chip, bridge) ||
>   	    mv88e6xxx_port_vlan_map(chip, port))
>   		dev_err(ds->dev, "failed to remap in-chip Port VLAN\n");
> @@ -2527,42 +2552,6 @@ static void mv88e6xxx_crosschip_bridge_leave(struct dsa_switch *ds,
>   	mv88e6xxx_reg_unlock(chip);
>   }
>   
> -/* Treat the software bridge as a virtual single-port switch behind the
> - * CPU and map in the PVT. First dst->last_switch elements are taken by
> - * physical switches, so start from beyond that range.
> - */
> -static int mv88e6xxx_map_virtual_bridge_to_pvt(struct dsa_switch *ds,
> -					       unsigned int bridge_num)
> -{
> -	u8 dev = bridge_num + ds->dst->last_switch;
> -	struct mv88e6xxx_chip *chip = ds->priv;
> -	int err;
> -
> -	mv88e6xxx_reg_lock(chip);
> -	err = mv88e6xxx_pvt_map(chip, dev, 0);
> -	mv88e6xxx_reg_unlock(chip);
> -
> -	return err;
> -}
> -
> -static int mv88e6xxx_bridge_tx_fwd_offload(struct dsa_switch *ds, int port,
> -					   struct dsa_bridge bridge)
> -{
> -	return mv88e6xxx_map_virtual_bridge_to_pvt(ds, bridge.num);
> -}
> -
> -static void mv88e6xxx_bridge_tx_fwd_unoffload(struct dsa_switch *ds, int port,
> -					      struct dsa_bridge bridge)
> -{
> -	int err;
> -
> -	err = mv88e6xxx_map_virtual_bridge_to_pvt(ds, bridge.num);
> -	if (err) {
> -		dev_err(ds->dev, "failed to remap cross-chip Port VLAN: %pe\n",
> -			ERR_PTR(err));
> -	}
> -}
> -
>   static int mv88e6xxx_software_reset(struct mv88e6xxx_chip *chip)
>   {
>   	if (chip->info->ops->reset)
> @@ -6282,8 +6271,6 @@ static const struct dsa_switch_ops mv88e6xxx_switch_ops = {
>   	.crosschip_lag_change	= mv88e6xxx_crosschip_lag_change,
>   	.crosschip_lag_join	= mv88e6xxx_crosschip_lag_join,
>   	.crosschip_lag_leave	= mv88e6xxx_crosschip_lag_leave,
> -	.port_bridge_tx_fwd_offload = mv88e6xxx_bridge_tx_fwd_offload,
> -	.port_bridge_tx_fwd_unoffload = mv88e6xxx_bridge_tx_fwd_unoffload,
>   };
>   
>   static int mv88e6xxx_register_switch(struct mv88e6xxx_chip *chip)
> diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
> index 21622c60faab..cefde41ce8d6 100644
> --- a/drivers/net/dsa/sja1105/sja1105_main.c
> +++ b/drivers/net/dsa/sja1105/sja1105_main.c
> @@ -2077,12 +2077,27 @@ static int sja1105_bridge_join(struct dsa_switch *ds, int port,
>   			       struct dsa_bridge bridge,
>   			       bool *tx_fwd_offload)
>   {
> -	return sja1105_bridge_member(ds, port, bridge, true);
> +	int rc;
> +
> +	rc = sja1105_bridge_member(ds, port, bridge, true);
> +	if (rc)
> +		return rc;
> +
> +	rc = dsa_tag_8021q_bridge_tx_fwd_offload(ds, port, bridge);
> +	if (rc) {
> +		sja1105_bridge_member(ds, port, bridge, false);
> +		return rc;
> +	}
> +
> +	*tx_fwd_offload = true;
> +
> +	return 0;
>   }
>   
>   static void sja1105_bridge_leave(struct dsa_switch *ds, int port,
>   				 struct dsa_bridge bridge)
>   {
> +	dsa_tag_8021q_bridge_tx_fwd_unoffload(ds, port, bridge);
>   	sja1105_bridge_member(ds, port, bridge, false);
>   }
>   
> @@ -3231,8 +3246,6 @@ static const struct dsa_switch_ops sja1105_switch_ops = {
>   	.tag_8021q_vlan_add	= sja1105_dsa_8021q_vlan_add,
>   	.tag_8021q_vlan_del	= sja1105_dsa_8021q_vlan_del,
>   	.port_prechangeupper	= sja1105_prechangeupper,
> -	.port_bridge_tx_fwd_offload = dsa_tag_8021q_bridge_tx_fwd_offload,
> -	.port_bridge_tx_fwd_unoffload = dsa_tag_8021q_bridge_tx_fwd_unoffload,
>   };
>   
>   static const struct of_device_id sja1105_dt_ids[];
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index 584b3f9462a0..bdf308a5c55e 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -222,6 +222,7 @@ struct dsa_mall_tc_entry {
>   struct dsa_bridge {
>   	struct net_device *dev;
>   	unsigned int num;
> +	bool tx_fwd_offload;
>   	refcount_t refcount;
>   };
>   
> @@ -826,12 +827,6 @@ struct dsa_switch_ops {
>   				    bool *tx_fwd_offload);
>   	void	(*port_bridge_leave)(struct dsa_switch *ds, int port,
>   				     struct dsa_bridge bridge);
> -	/* Called right after .port_bridge_join() */
> -	int	(*port_bridge_tx_fwd_offload)(struct dsa_switch *ds, int port,
> -					      struct dsa_bridge bridge);
> -	/* Called right before .port_bridge_leave() */
> -	void	(*port_bridge_tx_fwd_unoffload)(struct dsa_switch *ds, int port,
> -						struct dsa_bridge bridge);
>   	void	(*port_stp_state_set)(struct dsa_switch *ds, int port,
>   				      u8 state);
>   	void	(*port_fast_age)(struct dsa_switch *ds, int port);
> diff --git a/net/dsa/port.c b/net/dsa/port.c
> index b6c8a1a9ec18..fbe8c054d2b8 100644
> --- a/net/dsa/port.c
> +++ b/net/dsa/port.c
> @@ -270,37 +270,6 @@ static void dsa_port_switchdev_unsync_attrs(struct dsa_port *dp)
>   	 */
>   }
>   
> -static void dsa_port_bridge_tx_fwd_unoffload(struct dsa_port *dp,
> -					     struct dsa_bridge bridge)
> -{
> -	struct dsa_switch *ds = dp->ds;
> -
> -	/* No bridge TX forwarding offload => do nothing */
> -	if (!ds->ops->port_bridge_tx_fwd_unoffload || !bridge.num)
> -		return;
> -
> -	/* Notify the chips only once the offload has been deactivated, so
> -	 * that they can update their configuration accordingly.
> -	 */
> -	ds->ops->port_bridge_tx_fwd_unoffload(ds, dp->index, bridge);
> -}
> -
> -static bool dsa_port_bridge_tx_fwd_offload(struct dsa_port *dp,
> -					   struct dsa_bridge bridge)
> -{
> -	struct dsa_switch *ds = dp->ds;
> -	int err;
> -
> -	/* FDB isolation is required for TX forwarding offload */
> -	if (!ds->ops->port_bridge_tx_fwd_offload || !bridge.num)
> -		return false;
> -
> -	/* Notify the driver */
> -	err = ds->ops->port_bridge_tx_fwd_offload(ds, dp->index, bridge);
> -
> -	return err ? false : true;
> -}
> -
>   static int dsa_port_bridge_create(struct dsa_port *dp,
>   				  struct net_device *br,
>   				  struct netlink_ext_ack *extack)
> @@ -361,7 +330,6 @@ int dsa_port_bridge_join(struct dsa_port *dp, struct net_device *br,
>   	};
>   	struct net_device *dev = dp->slave;
>   	struct net_device *brport_dev;
> -	bool tx_fwd_offload;
>   	int err;
>   
>   	/* Here the interface is already bridged. Reflect the current
> @@ -378,12 +346,13 @@ int dsa_port_bridge_join(struct dsa_port *dp, struct net_device *br,
>   	if (err)
>   		goto out_rollback;
>   
> -	tx_fwd_offload = dsa_port_bridge_tx_fwd_offload(dp, info.bridge);
> +	/* Drivers which support bridge TX forwarding should set this */
> +	dp->bridge->tx_fwd_offload = info.tx_fwd_offload;
>   
>   	err = switchdev_bridge_port_offload(brport_dev, dev, dp,
>   					    &dsa_slave_switchdev_notifier,
>   					    &dsa_slave_switchdev_blocking_notifier,
> -					    tx_fwd_offload, extack);
> +					    dp->bridge->tx_fwd_offload, extack);
>   	if (err)
>   		goto out_rollback_unbridge;
>   
> @@ -434,8 +403,6 @@ void dsa_port_bridge_leave(struct dsa_port *dp, struct net_device *br)
>   	 */
>   	dsa_port_bridge_destroy(dp, br);
>   
> -	dsa_port_bridge_tx_fwd_unoffload(dp, info.bridge);
> -
>   	err = dsa_broadcast(DSA_NOTIFIER_BRIDGE_LEAVE, &info);
>   	if (err)
>   		dev_err(dp->ds->dev,
>