On Mon, Feb 08, 2021 at 01:21:37AM +0200, Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@xxxxxxx>
>
> 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@xxxxxxx>
> ---
> 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
