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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <c0a72a9a-3c1e-4bdf-9647-6cb9d962fad7@kernel.org>
Date: Thu, 16 Jan 2025 14:07:40 +0200
From: Roger Quadros <rogerq@...nel.org>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
Cc: Andrew Lunn <andrew+netdev@...n.ch>, "David S. Miller"
 <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 Alexei Starovoitov <ast@...nel.org>, Daniel Borkmann <daniel@...earbox.net>,
 Jesper Dangaard Brouer <hawk@...nel.org>,
 John Fastabend <john.fastabend@...il.com>,
 Siddharth Vadapalli <s-vadapalli@...com>, srk@...com, danishanwar@...com,
 netdev@...r.kernel.org, linux-kernel@...r.kernel.org, bpf@...r.kernel.org
Subject: Re: [PATCH net-next 2/4] net: ethernet: ti: am65-cpsw: streamline
 .probe() error handling

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