Re: [net-next PATCH 07/10] net: dsa: realtek: rtl8365mb: add port_bridge_{join,leave}

[Luiz Angelo Daros de Luca]

> > +static int rtl8365mb_port_bridge_join(struct dsa_switch *ds, int port,
> > +                                     struct dsa_bridge bridge,
> > +                                     bool *tx_forward_offload,
> > +                                     struct netlink_ext_ack *extack)
> > +static void rtl8365mb_port_bridge_leave(struct dsa_switch *ds, int port,
> > +                                       struct dsa_bridge bridge)
>
> The content of these functions are very similar to
> rtl8366rb_port_bridge_join() and rtl8366rb_port_bridge_leave().

It is likely that these functions are a variant of those or they share
a common origin.

> What about moving this code to the shared library rtl8366-core as
> rtl8366_port_bridge_join() and rtl8366_port_bridge_leave()
> and add per-variant callbacks for ->port_set_isolation()
> ->port_add_isolation() and ->port_remove_isolation() to
> struct realtek_ops and share this with rtl8366rb by refactoring
> rtl8366rb.c to use the same callbacks.

I agree that moving this logic to rtl83xx.c and introducing the
necessary abstractions makes sense. However, I would prefer not to
refactor rtl8366rb.c within this specific patchset to keep the scope
manageable.

I will also use dsa_switch_for_each_user_port, as
dsa_port_offloads_bridge() already ensures that CPU ports are skipped
and it aligns with RTL8366RB logic.

> Using shared code will help us to avoid having to fix the same
> bugs in more than one place.
>
> I don't know about this EFID stuff in  rtl8365mb_port_set_efid()
> (maybe RTL8366RB needs something like that too) but certainly that
> can be an optional callback in struct realtek_ops for now.

I will keep this as an optional callback for now. If it eventually
becomes clear that the RTL8366RB requires it, we can update the logic
later to make the callback mandatory.

> Yours,
> Linus Walleij

Regards,

Luiz