On 08/02/2021 13:45, Vladimir Oltean wrote:

On Mon, Feb 08, 2021 at 01:37:03PM +0200, Nikolay Aleksandrov wrote:

quoted

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.

quoted

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.