netdev - Re: [PATCHv4 net-next] net: modernize ioremap in probe

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: <09215f37-883b-4627-8f37-04a2a5ef8ae2@lunn.ch>
Date: Wed, 4 Dec 2024 00:43:13 +0100
From: Andrew Lunn <andrew@...n.ch>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
Cc: Rosen Penev <rosenp@...il.com>, netdev@...r.kernel.org,
	Kurt Kanzenbach <kurt@...utronix.de>,
	Vladimir Oltean <olteanv@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Chris Snook <chris.snook@...il.com>,
	Marcin Wojtas <marcin.s.wojtas@...il.com>,
	Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>,
	Niklas Söderlund <niklas.soderlund@...natech.se>,
	Richard Cochran <richardcochran@...il.com>,
	open list <linux-kernel@...r.kernel.org>,
	"open list:RENESAS ETHERNET SWITCH DRIVER" <linux-renesas-soc@...r.kernel.org>
Subject: Re: [PATCHv4 net-next] net: modernize ioremap in probe

> This is not equivalent. This means if ioremap() fails inside
> devm_platform_ioremap_resource(), we end up printing a message that
> blames the firmware, which is wrong.
> 
> It also changes a "resource missing, proceed anyway" situation into
> a failure situation.
> 
> Please drop this change, "cleaning" this up is introducing bugs.

https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html
says:

  1.6.5. Using device-managed and cleanup.h constructs

  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.

  1.6.6. Clean-up patches

  Netdev discourages patches which perform simple clean-ups, which are
  not in the context of other work. For example:

  Addressing checkpatch.pl warnings

  Addressing Local variable ordering issues

  Conversions to device-managed APIs (devm_ helpers)

  This is because it is felt that the churn that such changes produce
  comes at a greater cost than the value of such clean-ups.

As Russell points out, you are breaking things which probably worked
before. "If its not broken, don't fix it". These changes cost reviewer
time trying to find what you have broken, when it would be better
spent other places.

If you have the hardware, and wont to work on the driver to add new
features, then yes, you can do this sort of conversion, because you
should find your own bugs via testing. If you don't have the hardware,
please just leave it alone.

	Andrew