Re: [PATCH net-next 2/4] net: ethernet: ti: am65-cpsw: streamline .probe() error handling

[Roger Quadros <rogerq@kernel.org>]

On 15/01/2025 22:30, Russell King (Oracle) wrote:
> On Wed, Jan 15, 2025 at 06:43:01PM +0200, Roger Quadros wrote:
>> Keep things simple by explicitly cleaning up on .probe() error
>> path or .remove(). Get rid of devm_add/remove_action() usage.
>>
>> Rename am65_cpsw_disable_serdes_phy() to
>> am65_cpsw_nuss_cleanup_slave_ports() and move it right before
>> am65_cpsw_nuss_init_slave_ports().
>>
>> Get rid of am65_cpsw_nuss_phylink_cleanup() and introduce
>> am65_cpsw_nuss_cleanup_ndevs() right before am65_cpsw_nuss_init_ndevs()
>>
>> Move channel initiailzation code out of am65_cpsw_nuss_register_ndevs()
>> into new function am65_cpsw_nuss_init_chns().
>> Add am65_cpsw_nuss_remove_chns() to do reverse of
>> am65_cpsw_nuss_init_chns().
>>
>> Add am65_cpsw_nuss_unregister_ndev() to do reverse of
>> am65_cpsw_nuss_register_ndevs().
>>
>> Use the introduced helpers in probe/remove.
> 
> Wow, so we're now saying that devm shouldn't be used? Given that patch 1

No. We're not getting rid of all devm_ users in the driver, just the case
for am65_cpsw_nuss_free_rx/tx_chns(). I'll explain below.

> is wrong, I'm not sure I'd trust this patch to be correct either as it
> goes against what I understand is preferred - to avoid explicit cleanups
> that can get in the wrong order or be missed.
> 

In this particular case, we had to repeatedly call devm_remove/add_action()
on am65_cpsw_nuss_free_rx/tx_chns() at multiple places as number of channel
can be changed at runtime.

I thought it would be easier to keep track of the cleanup than to keep track
of devm_remove/add_action().

Another motivation for doing so was this paragraph found in
Documentation/process/maintainer-netdev.rst:

|Netdev remains skeptical about promises of all "auto-cleanup" APIs,
|including even ``devm_`` helpers, historically. They are not the preferred
|style of implementation, merely an acceptable one.

-- 
cheers,
-roger