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
