by Nikolay Aleksandrov

[permalink] [raw]

Subject: Re: [PATCH net-next 2/9] net: bridge: offload initial and final port flags through switchdev

On 08/02/2021 13:45, Vladimir Oltean wrote:
> On Mon, Feb 08, 2021 at 01:37:03PM +0200, Nikolay Aleksandrov wrote:
>> Hi Vladimir,
>> I think this patch potentially breaks some use cases. There are a few problems, I'll
>> start with the more serious one: before the ports would have a set of flags that were
>> always set when joining, now due to how nbp_flags_change() handles flag setting some might
>> not be set which would immediately change behaviour w.r.t software fwding. I'll use your
>> example of BR_BCAST_FLOOD: a lot of drivers will return an error for it and any broadcast
>> towards these ports will be dropped, we have mixed environments with software ports that
>> sometimes have traffic (e.g. decapped ARP requests) software forwarded which will stop working.
>
> Yes, you're right. The only solution I can think of is to add a "bool ignore_errors"
> to nbp_flags_change, set to true from new_nbp and del_nbp, and to false from the
> netlink code.
>

Indeed, I can't think of any better solution right now, but that would make it more or less
equal to the current situation where the flags are just set. You can read/restore them on add/del
of bridge port, but I guess that's what you'd like to avoid. :)
I don't mind adding the add/del_nbp() notifications, but both of them seem redundant with
the port add/del notifications which you can handle in the driver.

>> The other lesser issue is with the style below, I mean these three calls for each flag are
>> just ugly and look weird as you've also noted, since these APIs are internal can we do better?
>
> Doing better would mean allowing nbp_flags_change() to have a bit mask with
> potentially more brport flags set, and to call br_switchdev_set_port_flag in
> a for_each_set_bit() loop?
>

Sure, that sounds better for now. I think you've described the ideal case in your
commit message.

2021-02-08 18:30:40

by Ioana Ciornei

[permalink] [raw]

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

On Mon, Feb 08, 2021 at 01:21:37AM +0200, Vladimir Oltean wrote:
> From: Vladimir Oltean <[email protected]>
>
> 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 <[email protected]>
> ---
> 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