Re: [Intel-wired-lan] [PATCH 00/61] treewide: Use IS_ERR_OR_NULL over manual NULL check - refactor
On Tue, Mar 10, 2026 at 12:48:26PM +0100, Philipp Hahn wrote: > While doing some static code analysis I stumbled over a common pattern, > where IS_ERR() is combined with a NULL check. For that there is > IS_ERR_OR_NULL(). ... and valid uses of IS_ERR_OR_NULL are rare as hen teeth. Most of those are "I'm not sure how this function returns an error, let's use that just in case". Please, do not introduce more of that crap.
Re: [Intel-wired-lan] [PATCH 00/61] treewide: Use IS_ERR_OR_NULL over manual NULL check - refactor
On Thu, Mar 12, 2026 at 11:32:37AM -0400, James Bottomley wrote: > On Thu, 2026-03-12 at 09:57 -0300, Jason Gunthorpe wrote: > > On Wed, Mar 11, 2026 at 02:40:36AM +0800, Kuan-Wei Chiu wrote: > > > > > IMHO, the necessity of IS_ERR_OR_NULL() often highlights a > > > confusing or flawed API design. It usually implies that the caller > > > is unsure whether a failure results in an error pointer or a NULL > > > pointer. > > > > +1 > > > > IS_ERR_OR_NULL() should always be looked on with suspicion. Very > > little should be returning some tri-state 'ERR' 'NULL' 'SUCCESS' > > pointer. What does the middle condition even mean? IS_ERR_OR_NULL() > > implies ERR and NULL are semanticly the same, so fix the things to > > always use ERR. > > Not in any way supporting the original patch. However, the pattern > ERR, NULL, PTR is used extensively in the dentry code of filesystems. > See the try_lookup..() set of functions in fs/namei.c > > The meaning is > > PTR - I found it > NULL - It definitely doesn't exist > ERR - something went wrong during the lookup. > > So I don't think you can blanket say this pattern is wrong. Lots of places also would return ENOENT, I'd argue that is easier to use.. But yes, I did use the word "suspicion" not blanket wrong :) Jason
Re: [Intel-wired-lan] [PATCH 00/61] treewide: Use IS_ERR_OR_NULL over manual NULL check - refactor
On Thu, 2026-03-12 at 09:57 -0300, Jason Gunthorpe wrote: > On Wed, Mar 11, 2026 at 02:40:36AM +0800, Kuan-Wei Chiu wrote: > > > IMHO, the necessity of IS_ERR_OR_NULL() often highlights a > > confusing or flawed API design. It usually implies that the caller > > is unsure whether a failure results in an error pointer or a NULL > > pointer. > > +1 > > IS_ERR_OR_NULL() should always be looked on with suspicion. Very > little should be returning some tri-state 'ERR' 'NULL' 'SUCCESS' > pointer. What does the middle condition even mean? IS_ERR_OR_NULL() > implies ERR and NULL are semanticly the same, so fix the things to > always use ERR. Not in any way supporting the original patch. However, the pattern ERR, NULL, PTR is used extensively in the dentry code of filesystems. See the try_lookup..() set of functions in fs/namei.c The meaning is PTR - I found it NULL - It definitely doesn't exist ERR - something went wrong during the lookup. So I don't think you can blanket say this pattern is wrong. Regards, James
Re: [Intel-wired-lan] [PATCH 00/61] treewide: Use IS_ERR_OR_NULL over manual NULL check - refactor
On Wed, Mar 11, 2026 at 02:40:36AM +0800, Kuan-Wei Chiu wrote: > IMHO, the necessity of IS_ERR_OR_NULL() often highlights a confusing or > flawed API design. It usually implies that the caller is unsure whether > a failure results in an error pointer or a NULL pointer. +1 IS_ERR_OR_NULL() should always be looked on with suspicion. Very little should be returning some tri-state 'ERR' 'NULL' 'SUCCESS' pointer. What does the middle condition even mean? IS_ERR_OR_NULL() implies ERR and NULL are semanticly the same, so fix the things to always use ERR. If you want to improve things work to get rid of the NULL checks this script identifies. Remove ERR or NULL because only one can ever happen, or fix the source to consistently return ERR. Jason
Re: [Intel-wired-lan] [PATCH 00/61] treewide: Use IS_ERR_OR_NULL over manual NULL check - refactor
On Tue, Mar 10, 2026 at 12:48:26PM +0100, Philipp Hahn wrote: > While doing some static code analysis I stumbled over a common pattern, > where IS_ERR() is combined with a NULL check. For that there is > IS_ERR_OR_NULL(). One thing you need to check for each of these cases is whether the tests are actually correct. There are certainly cases amongst those that you have identified where the check for NULL is redundant. For example, get_phy_device() never returns NULL, yet in your netdev patch, you have at least one instance where the return value of get_phy_device() is checked for NULL and IS_ERR() which you then turn into IS_ERR_OR_NULL(). Instead, the NULL check should be dropped (as a separate patch.) -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
Re: [Intel-wired-lan] [PATCH 00/61] treewide: Use IS_ERR_OR_NULL over manual NULL check - refactor
Hi Philipp, On Tue, Mar 10, 2026 at 12:48:26PM +0100, Philipp Hahn wrote: > While doing some static code analysis I stumbled over a common pattern, > where IS_ERR() is combined with a NULL check. For that there is > IS_ERR_OR_NULL(). > > I've written a Coccinelle patch to find and patch those instances. > The patches follow grouped by subsystem. > > Patches 55-58 may be dropped as they have a (minor?) semantic change: > They use WARN_ON() or WARN_ON_ONCE(), but only in the IS_ERR() path, not > for the NULL check. Iff it is okay to print the warning also for NULL, > then the patches can be applied. > > While generating the patch set `checkpatch` complained about mixing > [un]likely() with IS_ERR_OR_NULL(), which already uses likely() > internally. I found and fixed several locations, where that combination > has been used. Thanks for the patchset. However, I think we need a explanation for why switching to IS_ERR_OR_NULL() is an improvement over the existing code. IMHO, the necessity of IS_ERR_OR_NULL() often highlights a confusing or flawed API design. It usually implies that the caller is unsure whether a failure results in an error pointer or a NULL pointer. Rather than doing a treewide conversion of this pattern, I believe it would be much more meaningful to review these instances case-by-case and fix the underlying APIs or caller logic instead. Additionally, a treewide refactoring like this has the practical drawback of creating unnecessary merge conflicts when backporting to stable trees. Regards, Kuan-Wei
Re: [Intel-wired-lan] [PATCH 00/61] treewide: Use IS_ERR_OR_NULL over manual NULL check - refactor
On Tue, Mar 10, 2026 at 12:48:26PM +0100, Philipp Hahn wrote: > While doing some static code analysis I stumbled over a common pattern, > where IS_ERR() is combined with a NULL check. For that there is > IS_ERR_OR_NULL(). > > I've written a Coccinelle patch to find and patch those instances. > The patches follow grouped by subsystem. I'm going to gently suggest that you *not* try to do this as a tree-wide change, since we don't need to change some interface requiring a global, flag day change. This is instead a cleanup, which maybe makes the code slightly better, but which also has a the downside of breaking lots of inflight development patches by potentially causing merge or patch conflicts. So why don't you send it to each subsystem as a separate patch or small patch series, instead of spamming a dozen-plus mailing lists, are probably hundreds of developers, most of whom aren't going to care about changs in some far flung part of the kernel? Regards, - Ted
Re: [Intel-wired-lan] [PATCH 00/61] treewide: Use IS_ERR_OR_NULL over manual NULL check - refactor
On Tue, 10 Mar 2026 12:48:26 +0100 Philipp Hahn wrote: > While doing some static code analysis I stumbled over a common pattern, > where IS_ERR() is combined with a NULL check. For that there is > IS_ERR_OR_NULL(). > > I've written a Coccinelle patch to find and patch those instances. > The patches follow grouped by subsystem. Honestly, the IS_ERR_OR_NULL() looks worse in a lot of the locations you updated. Just because we have IS_ERR_OR_NULL() doesn't mean we need to go and replace every location that can use it. NAK for any code this touches that I'm responsible for. -- Steve
