Re: [PATCH net-next 5/9] net: squash switchdev attributes PRE_BRIDGE_FLAGS and BRIDGE_FLAGS

[Ioana Ciornei <ioana.ciornei@nxp.com>]

On Mon, Feb 08, 2021 at 01:21:37AM +0200, Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> There does not appear to be any strong reason why
> br_switchdev_set_port_flag issues a separate notification for checking
> the supported brport flags rather than just attempting to apply them and
> propagating the error if that fails.
> 
> However, there is a reason why this switchdev API is counterproductive
> for a driver writer, and that is because although br_switchdev_set_port_flag
> gets passed a "flags" and a "mask", those are passed piecemeal to the
> driver, so while the PRE_BRIDGE_FLAGS listener knows what changed
> because it has the "mask", the BRIDGE_FLAGS listener doesn't, because it
> only has the final value. This means that "edge detection" needs to be
> done by each individual BRIDGE_FLAGS listener by XOR-ing the old and the
> new flags, which in turn means that copying the flags into a driver
> private variable is strictly necessary.
> 
> This can be solved by passing the "flags" and the "value" together into
> a single switchdev attribute, and it also reduces some boilerplate in
> the drivers that offload this.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>  drivers/net/dsa/b53/b53_common.c              | 16 ++++-------
>  drivers/net/dsa/mv88e6xxx/chip.c              | 17 ++++-------
>  .../marvell/prestera/prestera_switchdev.c     | 16 +++++------
>  .../mellanox/mlxsw/spectrum_switchdev.c       | 28 ++++++-------------
>  drivers/net/ethernet/rocker/rocker_main.c     | 24 +++-------------
>  drivers/net/ethernet/ti/cpsw_switchdev.c      | 20 ++++---------
>  drivers/staging/fsl-dpaa2/ethsw/ethsw.c       | 22 ++++-----------
>  include/net/dsa.h                             |  4 +--
>  include/net/switchdev.h                       |  8 ++++--
>  net/bridge/br_switchdev.c                     | 19 ++++---------
>  net/dsa/dsa_priv.h                            |  4 +--
>  net/dsa/port.c                                | 15 ++--------
>  net/dsa/slave.c                               |  3 --
>  13 files changed, 58 insertions(+), 138 deletions(-)
> 

(..)

> --- a/drivers/staging/fsl-dpaa2/ethsw/ethsw.c
> +++ b/drivers/staging/fsl-dpaa2/ethsw/ethsw.c
> @@ -908,28 +908,22 @@ static int dpaa2_switch_port_attr_stp_state_set(struct net_device *netdev,
>  	return dpaa2_switch_port_set_stp_state(port_priv, state);
>  }
>  
> -static int dpaa2_switch_port_attr_br_flags_pre_set(struct net_device *netdev,
> -						   unsigned long flags)
> -{
> -	if (flags & ~(BR_LEARNING | BR_FLOOD))
> -		return -EINVAL;
> -
> -	return 0;
> -}
> -
>  static int dpaa2_switch_port_attr_br_flags_set(struct net_device *netdev,
> -					       unsigned long flags)
> +					       struct switchdev_brport_flags val)
>  {
>  	struct ethsw_port_priv *port_priv = netdev_priv(netdev);
>  	int err = 0;
>  
> +	if (val.mask & ~(BR_LEARNING | BR_FLOOD))
> +		return -EINVAL;
> +
>  	/* Learning is enabled per switch */
>  	err = dpaa2_switch_set_learning(port_priv->ethsw_data,
> -					!!(flags & BR_LEARNING));
> +					!!(val.flags & BR_LEARNING));
>  	if (err)
>  		goto exit;
>  
> -	err = dpaa2_switch_port_set_flood(port_priv, !!(flags & BR_FLOOD));
> +	err = dpaa2_switch_port_set_flood(port_priv, !!(val.flags & BR_FLOOD));


Could you also check the mask to see if the flag needs to be actually changed?

> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -621,10 +621,8 @@ struct dsa_switch_ops {
>  	void	(*port_stp_state_set)(struct dsa_switch *ds, int port,
>  				      u8 state);
>  	void	(*port_fast_age)(struct dsa_switch *ds, int port);
> -	int	(*port_pre_bridge_flags)(struct dsa_switch *ds, int port,
> -					 unsigned long mask);
>  	int	(*port_bridge_flags)(struct dsa_switch *ds, int port,
> -				     unsigned long flags);
> +				     struct switchdev_brport_flags val);
>  	int	(*port_set_mrouter)(struct dsa_switch *ds, int port,
>  				    bool mrouter);
>  

In the previous patch you add the .port_pre_bridge_flags()
dsa_switch_ops only to remove it here. Couldn't these two patches be in
reverse order so that there is no need to actually add the callback in
the first place?

Ioana