Re: [PATCH net-next 0/7] net: sparx5: clean up probe/remove init and deinit paths

[Daniel Machon]

Hi Russell,

> > This series refactors the sparx5 init and deinit code out of
> > sparx5_start() and into probe(), adding proper per-subsystem cleanup
> > labels and deinit functions.
> >
> > Currently, the sparx5 driver initializes most subsystems inside
> > sparx5_start(), which is called from probe(). This includes registering
> > netdevs, starting worker threads for stats and MAC table polling,
> > requesting PTP IRQs, and initializing VCAP. The function has grown to
> > handle many unrelated subsystems, and has no granular error handling —
> > it either succeeds entirely or returns an error, leaving cleanup to a
> > single catch-all label in probe().
> >
> > The remove() path has a similar problem: teardown is not structured as
> > the reverse of initialization, and several subsystems lack proper deinit
> > functions. For example, the stats workqueue has no corresponding
> > cleanup, and the mact workqueue is destroyed without first cancelling
> > its delayed work.
> >
> > Refactor this by moving each init function out of sparx5_start() and
> > into probe(), with a corresponding goto-based cleanup label. Add deinit
> > functions for subsystems that allocate resources, to properly cancel
> > work and destroy workqueues. Ensure that cleanup order in both error
> > paths and remove() follows the reverse of initialization order. What
> > remains in sparx5_start() is only hardware register setup and FDMA/XTR
> > initialization that does not require cleanup.
> >
> > Before this series, most init functions live inside sparx5_start() with
> > no individual cleanup:
> >
> >   probe():
> >     sparx5_start():              <- no granular error handling
> >       sparx5_mact_init()
> >       sparx_stats_init()           <- starts worker, no cleanup
> >       mact_queue setup             <- no cancel on teardown
> >       sparx5_register_netdevs()
> >       sparx5_register_notifier_blocks()
> >       sparx5_vcap_init()
> >     sparx5_ptp_init()
> >
> >     probe() error path:
> >       cleanup_ports:
> >         sparx5_cleanup_ports()
> >         destroy_workqueue(mact_queue)
> >
> > After this series, probe() initializes subsystems in order with
> > matching cleanup labels, and remove() tears down in reverse:
> >
> > probe():
> >     sparx5_ptp_init()
> >     sparx5_vcap_init()
> >     sparx5_mact_init()
> >     sparx5_stats_init()
> >     sparx5_register_netdevs()
> >     sparx5_register_notifier_blocks()
> >     sparx5_start()
> >
> >   remove():
> >     sparx5_unregister_notifier_blocks()
> >     sparx5_unregister_netdevs()
> >     sparx5_stats_deinit()
> >     sparx5_mact_deinit()
> >     sparx5_vcap_deinit()
> >     sparx5_ptp_deinit()
> >     sparx5_destroy_netdevs()
> >
> > Signed-off-by: Daniel Machon <daniel.machon@xxxxxxxxxxxxx>
> 
> Note that there is the general principle that drivers should not
> "publish" themselves (aka register their netdevs and/or ptp) until
> they have initialised enough so the facility that has been published
> is functional.
> 
> That, in general, means that there should be very little initialisation
> after the calls to register the netdevs and ptp.
> 
> It's fine if something gets published and then a later publication of
> another interface fails, causing the first publication to be withdrawn,
> that is pretty much unavoidable, but in such scenarios, one would want
> to do as much of the initialisation that may fail before any
> publication of any user interfaces.
> 
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!


Thanks for pointing this out.

Before this series, quite a bit of initialization was already done after
publication. But as a consequence of moving netdev registration out of
sparx5_start() (patch #2), some initialization that previously happened before
publication now happens after — which is the wrong direction.

I will do some reordering to fix this in v2.

Thanks!