lkml.org 
[lkml]   [2021]   [Feb]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH net-next 5/9] net: squash switchdev attributes PRE_BRIDGE_FLAGS and BRIDGE_FLAGS
Date
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
\
 
 \ /
  Last update: 2021-02-08 19:30    [W:0.127 / U:0.060 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site