[cocci] [PATCH] Revert "scripts/coccinelle: Find PTR_ERR() to %pe candidates"

[cocci] [PATCH] Revert "scripts/coccinelle: Find PTR_ERR() to %pe candidates"

[Johan Hovold]

This reverts commit 57c49d2355729c12475554b4c51dbf830b02d08d.

Using "%pe" to print errnos is in no way mandated and a driver authors
may chose not to use it, for example, for consistency reasons.

Drop the recently added cocci script that has gotten the build bots to
send warning emails about perfectly valid code and which will likely
only result in churn and inconsistency.

Link: https://lore.kernel.org/all/aQHi4nUfIlcN1ac6@hovoldconsulting.com/
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 scripts/coccinelle/misc/ptr_err_to_pe.cocci | 34 ---------------------
 1 file changed, 34 deletions(-)
 delete mode 100644 scripts/coccinelle/misc/ptr_err_to_pe.cocci

diff --git a/scripts/coccinelle/misc/ptr_err_to_pe.cocci b/scripts/coccinelle/misc/ptr_err_to_pe.cocci
deleted file mode 100644
index 0494c7709245..000000000000
--- a/scripts/coccinelle/misc/ptr_err_to_pe.cocci
+++ /dev/null
@@ -1,34 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0-only
-/// Use %pe format specifier instead of PTR_ERR() for printing error pointers.
-///
-/// For printing error pointers (i.e., a pointer for which IS_ERR() is true)
-/// %pe will print a symbolic error name (e.g., -EINVAL), opposed to the raw
-/// errno (e.g., -22) produced by PTR_ERR().
-/// It also makes the code cleaner by saving a redundant call to PTR_ERR().
-///
-// Confidence: High
-// Copyright: (C) 2025 NVIDIA CORPORATION & AFFILIATES.
-// URL: https://coccinelle.gitlabpages.inria.fr/website
-// Options: --no-includes --include-headers
-
-virtual context
-virtual org
-virtual report
-
-@r@
-expression ptr;
-constant fmt;
-position p;
-identifier print_func;
-@@
-* print_func(..., fmt, ..., PTR_ERR@p(ptr), ...)
-
-@script:python depends on r && report@
-p << r.p;
-@@
-coccilib.report.print_report(p[0], "WARNING: Consider using %pe to print PTR_ERR()")
-
-@script:python depends on r && org@
-p << r.p;
-@@
-coccilib.org.print_todo(p[0], "WARNING: Consider using %pe to print PTR_ERR()")
-- 
2.51.0

Re: [cocci] [PATCH] Revert "scripts/coccinelle: Find PTR_ERR() to %pe candidates"

[Gal Pressman]

Hi Johan,

On 29/10/2025 15:29, Johan Hovold wrote:
> This reverts commit 57c49d2355729c12475554b4c51dbf830b02d08d.
> 
> Using "%pe" to print errnos is in no way mandated and a driver authors
> may chose not to use it, for example, for consistency reasons.
> 
> Drop the recently added cocci script that has gotten the build bots to
> send warning emails about perfectly valid code and which will likely
> only result in churn and inconsistency.
> 
> Link: https://lore.kernel.org/all/aQHi4nUfIlcN1ac6@hovoldconsulting.com/
> Signed-off-by: Johan Hovold <johan@kernel.org>

The test by no means mandates authors to use %pe, as the output says:
WARNING: Consider using %pe to print PTR_ERR()

"Consider" :).

I would consider it best practice to use it, and a few drivers were
converted thanks to this test.

If the issue is with automatic build bots, then maybe this test should
be excluded from them, rather than deleted?

Re: [cocci] [PATCH] Revert "scripts/coccinelle: Find PTR_ERR() to %pe candidates"

[Julia Lawall]

On Wed, 29 Oct 2025, Gal Pressman wrote:

> Hi Johan,
>
> On 29/10/2025 15:29, Johan Hovold wrote:
> > This reverts commit 57c49d2355729c12475554b4c51dbf830b02d08d.
> >
> > Using "%pe" to print errnos is in no way mandated and a driver authors
> > may chose not to use it, for example, for consistency reasons.
> >
> > Drop the recently added cocci script that has gotten the build bots to
> > send warning emails about perfectly valid code and which will likely
> > only result in churn and inconsistency.
> >
> > Link: https://lore.kernel.org/all/aQHi4nUfIlcN1ac6@hovoldconsulting.com/
> > Signed-off-by: Johan Hovold <johan@kernel.org>
>
> The test by no means mandates authors to use %pe, as the output says:
> WARNING: Consider using %pe to print PTR_ERR()
>
> "Consider" :).
>
> I would consider it best practice to use it, and a few drivers were
> converted thanks to this test.
>
> If the issue is with automatic build bots, then maybe this test should
> be excluded from them, rather than deleted?

This is easy to do.  Or I can discard them when they come to me for
approval.

julia

Re: [cocci] [PATCH] Revert "scripts/coccinelle: Find PTR_ERR() to %pe candidates"

[Johan Hovold]

On Wed, Oct 29, 2025 at 03:59:50PM +0200, Gal Pressman wrote:
> On 29/10/2025 15:29, Johan Hovold wrote:
> > This reverts commit 57c49d2355729c12475554b4c51dbf830b02d08d.
> > 
> > Using "%pe" to print errnos is in no way mandated and a driver authors
> > may chose not to use it, for example, for consistency reasons.
> > 
> > Drop the recently added cocci script that has gotten the build bots to
> > send warning emails about perfectly valid code and which will likely
> > only result in churn and inconsistency.
> > 
> > Link: https://lore.kernel.org/all/aQHi4nUfIlcN1ac6@hovoldconsulting.com/
> > Signed-off-by: Johan Hovold <johan@kernel.org>
> 
> The test by no means mandates authors to use %pe, as the output says:
> WARNING: Consider using %pe to print PTR_ERR()
> 
> "Consider" :).

Right, but it's preceded by a big "WARNING".

> I would consider it best practice to use it, and a few drivers were
> converted thanks to this test.

Unlike the rest of the misc cocci scripts I skimmed, this one does not
guard against any bugs. Instead it's pushing for a subjective style
preference, which is just going to result in churn when the clean up
crew starts sending mindless conversions of individual printks.

By all means, use %pe for your drivers, but it should not be forced
upon the rest of us this way.

> If the issue is with automatic build bots, then maybe this test should
> be excluded from them, rather than deleted?

It's both; it's the noise the new warnings generate but also the coming
flood up patches to "fix" them. There are already some 40 commits or so
in linux-next referencing this script.

Johan

Re: [cocci] Revert "scripts/coccinelle: Find PTR_ERR() to %pe candidates"

[Markus Elfring]

>> The test by no means mandates authors to use %pe, as the output says:
>> WARNING: Consider using %pe to print PTR_ERR()
>>
>> "Consider" :).
> 
> Right, but it's preceded by a big "WARNING".

Would you find an other message prefix nicer?


>> I would consider it best practice to use it, and a few drivers were
>> converted thanks to this test.

Would there be more convincing arguments needed according to better practice?


> Unlike the rest of the misc cocci scripts I skimmed, this one does not
> guard against any bugs. Instead it's pushing for a subjective style
> preference, which is just going to result in churn when the clean up
> crew starts sending mindless conversions of individual printks.
> 
> By all means, use %pe for your drivers, but it should not be forced
> upon the rest of us this way.

Is there a need to mark any more SmPL scripts as “controversial”?


>> If the issue is with automatic build bots, then maybe this test should
>> be excluded from them, rather than deleted?
> 
> It's both; it's the noise the new warnings generate but also the coming
> flood up patches to "fix" them. There are already some 40 commits or so
> in linux-next referencing this script.

How will the change tolerance evolve further?

Regards,
Markus

Re: [cocci] [PATCH] Revert "scripts/coccinelle: Find PTR_ERR() to %pe candidates"

[Jakub Kicinski]

On Wed, 29 Oct 2025 15:04:36 +0100 (CET) Julia Lawall wrote:
> > The test by no means mandates authors to use %pe, as the output says:
> > WARNING: Consider using %pe to print PTR_ERR()
> >
> > "Consider" :).
> >
> > I would consider it best practice to use it, and a few drivers were
> > converted thanks to this test.
> >
> > If the issue is with automatic build bots, then maybe this test should
> > be excluded from them, rather than deleted?  
> 
> This is easy to do.  Or I can discard them when they come to me for
> approval.

FWIW I'd also prefer for the script to say in the tree.
Some kind of opt-out mechanism per subsystem would be ideal, 
and presumably belongs in the bots themselves. We maintain 
a list of regexp's in netdev CI to silence cocci checks we 
don't find worth complaining about. But the %pe check so far 
have not been too bad. 

And we have a policy against semi-automated patches which "clean up"
cocci or checkpatch warnings. Something we may want to adopt more
widely.

Re: [cocci] [PATCH] Revert "scripts/coccinelle: Find PTR_ERR() to %pe candidates"

[Gal Pressman]

On 29/10/2025 18:38, Johan Hovold wrote:
> On Wed, Oct 29, 2025 at 03:59:50PM +0200, Gal Pressman wrote:
>> On 29/10/2025 15:29, Johan Hovold wrote:
>>> This reverts commit 57c49d2355729c12475554b4c51dbf830b02d08d.
>>>
>>> Using "%pe" to print errnos is in no way mandated and a driver authors
>>> may chose not to use it, for example, for consistency reasons.
>>>
>>> Drop the recently added cocci script that has gotten the build bots to
>>> send warning emails about perfectly valid code and which will likely
>>> only result in churn and inconsistency.
>>>
>>> Link: https://lore.kernel.org/all/aQHi4nUfIlcN1ac6@hovoldconsulting.com/
>>> Signed-off-by: Johan Hovold <johan@kernel.org>
>>
>> The test by no means mandates authors to use %pe, as the output says:
>> WARNING: Consider using %pe to print PTR_ERR()
>>
>> "Consider" :).
> 
> Right, but it's preceded by a big "WARNING".

Right, I don't mind removing it.

> 
>> I would consider it best practice to use it, and a few drivers were
>> converted thanks to this test.
> 
> Unlike the rest of the misc cocci scripts I skimmed, this one does not
> guard against any bugs. Instead it's pushing for a subjective style
> preference, which is just going to result in churn when the clean up
> crew starts sending mindless conversions of individual printks.

Not all cocci scripts are used for fixing bugs:
err_cast.cocci
memdup.cocci
minmax.cocci
string_choices.cocci

They are used to improve the code.
We can probably agree that for new code %pe is preferable, but I
understand your concerns about the churn of conversions.

> 
> By all means, use %pe for your drivers, but it should not be forced
> upon the rest of us this way.

Agree.
This script helps us for our drivers.

> 
>> If the issue is with automatic build bots, then maybe this test should
>> be excluded from them, rather than deleted?
> 
> It's both; it's the noise the new warnings generate but also the coming
> flood up patches to "fix" them. There are already some 40 commits or so
> in linux-next referencing this script.

It's OK to not like these conversion patches, it's up to the maintainer
to accept/reject them.

For example, I know Jakub dislikes the string_choices.cocci script and
rejects conversion patches. But the script still exists and can be used
in other places in the kernel who might have a different opinion.

Re: [cocci] [PATCH] Revert "scripts/coccinelle: Find PTR_ERR() to %pe candidates"

[Julia Lawall]

I told the 0-day people to ignore this script, which they agreed to do.
We can see how it goes for a bit and reevaluate if needed.

julia

Re: [cocci] [PATCH] Revert "scripts/coccinelle: Find PTR_ERR() to %pe candidates"

[Johan Hovold]

On Wed, Oct 29, 2025 at 07:35:25PM +0200, Gal Pressman wrote:
> On 29/10/2025 18:38, Johan Hovold wrote:
> > On Wed, Oct 29, 2025 at 03:59:50PM +0200, Gal Pressman wrote:

> > Unlike the rest of the misc cocci scripts I skimmed, this one does not
> > guard against any bugs. Instead it's pushing for a subjective style
> > preference, which is just going to result in churn when the clean up
> > crew starts sending mindless conversions of individual printks.
> 
> Not all cocci scripts are used for fixing bugs:
> err_cast.cocci
> memdup.cocci
> minmax.cocci
> string_choices.cocci
> 
> They are used to improve the code.

I only skimmed the misc ones, but I still things there's a difference
here since "%pe" is changing the output (e.g. unlike err_cast.cocci) and
is essentially a subjective preference.

> We can probably agree that for new code %pe is preferable, but I
> understand your concerns about the churn of conversions.

I'm not even convinced about new drivers. For consistency you'd need to
add *ERR_PTR()* to more or less every dev_err() in the driver (which
becomes ugly):

	if (ret)
		dev_err(dev, "failed to ...: %pe\n", ERR_PTR(ret));

And by now many of us recognise (or can easily lookup) the errnos used
in the occasional error message if at all needed.

Note that in most cases you have ret variable that holds the errno,
which would not be caught by this cocci script either:

	ret = PTR_ERR(p);
	dev_err(dev, "failed to ...: %d\n", ret);
	return ret; // or goto out;

> >> If the issue is with automatic build bots, then maybe this test should
> >> be excluded from them, rather than deleted?
> > 
> > It's both; it's the noise the new warnings generate but also the coming
> > flood up patches to "fix" them. There are already some 40 commits or so
> > in linux-next referencing this script.
> 
> It's OK to not like these conversion patches, it's up to the maintainer
> to accept/reject them.
> 
> For example, I know Jakub dislikes the string_choices.cocci script and
> rejects conversion patches. But the script still exists and can be used
> in other places in the kernel who might have a different opinion.

It still generates noise and extra work for already overworked
maintainers that would need to explain over and over again why they are
rejecting patches that appears to fix "warnings". Some will just take
the patches, which leads to inconsistencies (as only a handful of
printks will be converted) and a push for a style which again only some
people prefer.

So I still think this script should be dropped. And you still need to
review drivers manually if you really want to use %pe consistently (e.g.
for all the cases where there is no error pointer to begin with).

Johan

Re: [cocci] [PATCH] Revert "scripts/coccinelle: Find PTR_ERR() to %pe candidates"

[Gal Pressman]

On 30/10/2025 16:06, Johan Hovold wrote:
> Note that in most cases you have ret variable that holds the errno,
> which would not be caught by this cocci script either:
> 
> 	ret = PTR_ERR(p);
> 	dev_err(dev, "failed to ...: %d\n", ret);
> 	return ret; // or goto out;

I have a followup patch that catches these kinds of cases as well.

> It still generates noise and extra work for already overworked
> maintainers that would need to explain over and over again why they are
> rejecting patches that appears to fix "warnings". Some will just take
> the patches, which leads to inconsistencies (as only a handful of
> printks will be converted) and a push for a style which again only some
> people prefer.

There's the subsystem maintainer "rules" documentation in
Documentation/process/maintainer-*.rst which can document these kinds of
stuff.

> 
> So I still think this script should be dropped. And you still need to
> review drivers manually if you really want to use %pe consistently (e.g.
> for all the cases where there is no error pointer to begin with).

I am not sure who is to decide, obviously I prefer not to revert it, but
I understand your concerns.

Re: [cocci] [PATCH] Revert "scripts/coccinelle: Find PTR_ERR() to %pe candidates"

[Johan Hovold]

On Thu, Oct 30, 2025 at 04:36:46PM +0200, Gal Pressman wrote:
> On 30/10/2025 16:06, Johan Hovold wrote:

> > It still generates noise and extra work for already overworked
> > maintainers that would need to explain over and over again why they are
> > rejecting patches that appears to fix "warnings". Some will just take
> > the patches, which leads to inconsistencies (as only a handful of
> > printks will be converted) and a push for a style which again only some
> > people prefer.
> 
> There's the subsystem maintainer "rules" documentation in
> Documentation/process/maintainer-*.rst which can document these kinds of
> stuff.

There's already too many rules in there and I guess not many people
actually read it so that doesn't really scale.

Johan