From: Johan Hovold <johan@kernel.org>
To: Gal Pressman <gal@nvidia.com>
Cc: Julia Lawall <Julia.Lawall@inria.fr>,
Nicolas Palix <nicolas.palix@imag.fr>,
Jakub Kicinski <kuba@kernel.org>,
Alexei Lazar <alazar@nvidia.com>, Simon Horman <horms@kernel.org>,
cocci@inria.fr, linux-kernel@vger.kernel.org
Subject: Re: [cocci] [PATCH] Revert "scripts/coccinelle: Find PTR_ERR() to %pe candidates"
Date: Thu, 30 Oct 2025 15:06:28 +0100 [thread overview]
Message-ID: <aQNw5NqZSZk5JNxn@hovoldconsulting.com> (raw)
In-Reply-To: <3b04f763-aaba-41ee-a81f-94195043fd4b@nvidia.com>
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
next prev parent reply other threads:[~2025-10-30 14:27 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-10-29 13:29 [cocci] [PATCH] Revert "scripts/coccinelle: Find PTR_ERR() to %pe candidates" Johan Hovold
2025-10-29 13:59 ` Gal Pressman
2025-10-29 14:04 ` Julia Lawall
2025-10-29 17:26 ` Jakub Kicinski
2025-10-29 16:38 ` Johan Hovold
2025-10-29 17:00 ` [cocci] " Markus Elfring
2025-10-29 17:35 ` [cocci] [PATCH] " Gal Pressman
2025-10-30 14:06 ` Johan Hovold [this message]
2025-10-30 14:36 ` Gal Pressman
2025-10-31 14:10 ` Johan Hovold
2025-10-30 7:36 ` Julia Lawall
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=aQNw5NqZSZk5JNxn@hovoldconsulting.com \
--to=johan@kernel.org \
--cc=Julia.Lawall@inria.fr \
--cc=alazar@nvidia.com \
--cc=cocci@inria.fr \
--cc=gal@nvidia.com \
--cc=horms@kernel.org \
--cc=kuba@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=nicolas.palix@imag.fr \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).