Hi Florian,
On Mon, 19 Aug 2019 10:14:24 -0700, Florian Fainelli <f.faine...@gmail.com>
wrote:
> On 8/18/19 10:35 AM, Vivien Didelot wrote:
> > It is currently difficult to read the different steps involved in the
> > setup and teardown of ports in the DSA code. Keep it simple with a
> > single switch statement for each port type: UNUSED, CPU, DSA, or USER.
> >
> > Also no need to call devlink_port_unregister from within dsa_port_setup
> > as this step is inconditionally handled by dsa_port_teardown on error.
> >
> > Signed-off-by: Vivien Didelot <vivien.dide...@gmail.com>
> > ---
>
> [snip]
>
> > case DSA_PORT_TYPE_CPU:
> > + memset(dlp, 0, sizeof(*dlp));
> > + devlink_port_attrs_set(dlp, DEVLINK_PORT_FLAVOUR_CPU,
> > + dp->index, false, 0, id, len);
> > + err = devlink_port_register(dl, dlp, dp->index);
> > + if (err)
> > + return err;
>
> This is shared between all port flavors with the exception that the
> flavor type is different, maybe we should create a helper function and
> factor out even more code. I don't feel great about repeating 3 times t
> the same code without making use of a fall through.
Nah I did not want to use an helper because the code is still too
noodly, if you look at user ports setup, we continue to setup devlink
after the slave creation, so here I prefered to keep things readable
and expose all steps first, before writing yet another helper.
Thanks,
Vivien
