* Re: Stop false review statements
From: Guenter Roeck @ 2026-05-16 19:13 UTC (permalink / raw)
To: Krzysztof Kozlowski, Roman Gushchin
Cc: Greg KH, Konstantin Ryabitsev, sashiko-bot, sashiko-reviews,
sashiko, Linux Kernel Workflows, Linux Kernel Mailing List,
devicetree, kfree
In-Reply-To: <b5989c0f-90da-42cc-a623-3b60df077848@kernel.org>
On 5/16/26 12:00, Krzysztof Kozlowski wrote:
...
>> It’s opt-in on per-subsystem basis, as well as all other email-related features.
>> I do rely on corresponding maintainers to decide if they want it or not.
>
> The trouble is that subsystem is mailing list, thus I still got all of
> them via b4, which is used to get the discussion.
>
> Send them only to the maintainer, for example. Or maintainer + authors.
>
For hwmon and watchdog I most definitely want the response sent to the
mailing list and to the patch author. That was the original configuration
for hwmon. Roman took it out because people who were copied on the
original patch complained that they did _not_ get Sashiko's reply.
That is a perfect lose-lose situation.
Guenter
^ permalink raw reply
* Re: Stop false review statements
From: Krzysztof Kozlowski @ 2026-05-16 19:00 UTC (permalink / raw)
To: Roman Gushchin
Cc: Greg KH, Konstantin Ryabitsev, Guenter Roeck, sashiko-bot,
sashiko-reviews, sashiko, Linux Kernel Workflows,
Linux Kernel Mailing List, devicetree, kfree
In-Reply-To: <70C5331E-06F1-48D5-A6BA-0CD130B69A45@linux.dev>
On 16/05/2026 20:56, Roman Gushchin wrote:
>
>
>> On May 16, 2026, at 11:29 AM, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>
>> On 16/05/2026 17:49, Roman Gushchin wrote:
>>>>
>>>>> I’m not attached to any specific form of it, I thought Reviewed-by is the most obvious form.
>>>>> And we use Reported-by: tags with various tooling for years.
>>>>
>>>> Reported-by: shows the existance of a problem that some tool found, a
>>>> subtle difference here.
>>>>
>>>>> What do you think is the best form?
>>>>>
>>>>> I’ll pause sending reviewed-by tags until we have a discussion and agreement here.
>>>>
>>>> Just say it in some other text form, that our tools will not pick up.
>>>> Like:
>>>> Tool XXXX reports that all is good:
>>>> https://....
>>>>
>>>> or something like that?
>>>
>>> Sure, works for me.
>> Roman,
>> Before implementing such changes, send a RFC or just ask a few folks for
>> opinions. We do use the tool, among other tools, so we will gladly
>> provide a feedback.
>>
>> Sashiko should in general not send such emails when not asked for. Why?
>> Because we have also other bots, like LKP, KernelCI, and imagine how
>> maintainer's mailbox will look like.
>>
>> LKP allows opt-in for your own repo, which for example I am using, so I
>> get confirmation of the success. But people are not receiving them. I
>> cannot imagine all the people getting these LKP-successfully-built
>> emails on every email.
>
> It’s opt-in on per-subsystem basis, as well as all other email-related features.
> I do rely on corresponding maintainers to decide if they want it or not.
The trouble is that subsystem is mailing list, thus I still got all of
them via b4, which is used to get the discussion.
Send them only to the maintainer, for example. Or maintainer + authors.
Basically the same as LKP is doing.
> If you’re saying that it should not send any non-personal emails in general, I disagree here,
> but happy to have a discussion, assuming it’s polite and constructive.
I meant it should not be send to people who did not request that. Opt-in
should be explicit and no mailing lists must be Cced (because then it is
sending to everyone).
>
> The reason why I disagree is simple: there are maintainers/subsystems who like Sashiko’s reviews
> and before introducing the email interface they had to manually send links to Sashiko’s reviews
> as replies to proposed patches. I’ve been explicitly asked to add an ability to send out
> emails with reviews.
Sure, I agree with the need for use-case.
Best regards,
Krzysztof
^ permalink raw reply
* Re: Stop false review statements
From: Roman Gushchin @ 2026-05-16 18:56 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Greg KH, Konstantin Ryabitsev, Guenter Roeck, sashiko-bot,
sashiko-reviews, sashiko, Linux Kernel Workflows,
Linux Kernel Mailing List, devicetree, kfree
In-Reply-To: <efc4d394-b328-4ccf-8c05-b6470ee4b88d@kernel.org>
> On May 16, 2026, at 11:29 AM, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 16/05/2026 17:49, Roman Gushchin wrote:
>>>
>>>> I’m not attached to any specific form of it, I thought Reviewed-by is the most obvious form.
>>>> And we use Reported-by: tags with various tooling for years.
>>>
>>> Reported-by: shows the existance of a problem that some tool found, a
>>> subtle difference here.
>>>
>>>> What do you think is the best form?
>>>>
>>>> I’ll pause sending reviewed-by tags until we have a discussion and agreement here.
>>>
>>> Just say it in some other text form, that our tools will not pick up.
>>> Like:
>>> Tool XXXX reports that all is good:
>>> https://....
>>>
>>> or something like that?
>>
>> Sure, works for me.
> Roman,
> Before implementing such changes, send a RFC or just ask a few folks for
> opinions. We do use the tool, among other tools, so we will gladly
> provide a feedback.
>
> Sashiko should in general not send such emails when not asked for. Why?
> Because we have also other bots, like LKP, KernelCI, and imagine how
> maintainer's mailbox will look like.
>
> LKP allows opt-in for your own repo, which for example I am using, so I
> get confirmation of the success. But people are not receiving them. I
> cannot imagine all the people getting these LKP-successfully-built
> emails on every email.
It’s opt-in on per-subsystem basis, as well as all other email-related features.
I do rely on corresponding maintainers to decide if they want it or not.
Even in the case which you was so unhappy about, I asked Guenter prior
to enabling it for hwmon.
If you’re saying that it should not send any non-personal emails in general, I disagree here,
but happy to have a discussion, assuming it’s polite and constructive.
The reason why I disagree is simple: there are maintainers/subsystems who like Sashiko’s reviews
and before introducing the email interface they had to manually send links to Sashiko’s reviews
as replies to proposed patches. I’ve been explicitly asked to add an ability to send out
emails with reviews.
Thanks
^ permalink raw reply
* Re: Stop false review statements
From: Krzysztof Kozlowski @ 2026-05-16 18:28 UTC (permalink / raw)
To: Roman Gushchin, Greg KH
Cc: Konstantin Ryabitsev, Guenter Roeck, sashiko-bot, sashiko-reviews,
sashiko, Linux Kernel Workflows, Linux Kernel Mailing List,
devicetree, kfree
In-Reply-To: <0902F8E6-C495-40A1-975D-92D3B72D44AE@linux.dev>
On 16/05/2026 17:49, Roman Gushchin wrote:
>>
>>> I’m not attached to any specific form of it, I thought Reviewed-by is the most obvious form.
>>> And we use Reported-by: tags with various tooling for years.
>>
>> Reported-by: shows the existance of a problem that some tool found, a
>> subtle difference here.
>>
>>> What do you think is the best form?
>>>
>>> I’ll pause sending reviewed-by tags until we have a discussion and agreement here.
>>
>> Just say it in some other text form, that our tools will not pick up.
>> Like:
>> Tool XXXX reports that all is good:
>> https://....
>>
>> or something like that?
>
> Sure, works for me.
Roman,
Before implementing such changes, send a RFC or just ask a few folks for
opinions. We do use the tool, among other tools, so we will gladly
provide a feedback.
Sashiko should in general not send such emails when not asked for. Why?
Because we have also other bots, like LKP, KernelCI, and imagine how
maintainer's mailbox will look like.
LKP allows opt-in for your own repo, which for example I am using, so I
get confirmation of the success. But people are not receiving them. I
cannot imagine all the people getting these LKP-successfully-built
emails on every email.
Best regards,
Krzysztof
^ permalink raw reply
* Re: Stop false review statements
From: Arnaldo Carvalho de Melo @ 2026-05-16 18:28 UTC (permalink / raw)
To: Roman Gushchin
Cc: Greg KH, Konstantin Ryabitsev, Guenter Roeck, Krzysztof Kozlowski,
sashiko-bot, sashiko-reviews, sashiko, Linux Kernel Workflows,
Linux Kernel Mailing List, devicetree, kfree
In-Reply-To: <0902F8E6-C495-40A1-975D-92D3B72D44AE@linux.dev>
On Sat, May 16, 2026 at 08:49:39AM -0700, Roman Gushchin wrote:
> > On May 16, 2026, at 8:45 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Sat, May 16, 2026 at 08:41:43AM -0700, Roman Gushchin wrote:
> >>>> On May 16, 2026, at 8:20 AM, Konstantin Ryabitsev <mricon@kernel.org> wrote:
> >>> On Sat, May 16, 2026 at 05:11:28AM -0700, Guenter Roeck wrote:
> >>>>> On Sat, May 16, 2026 at 10:05:02AM +0200, Krzysztof Kozlowski wrote:
> >>>>> What the hell is that:
>
> >>>>> https://lore.kernel.org/all/20260515190707.033BDC2BCB0@smtp.kernel.org/
> >>>>> As a bot you CANNOT MAKE a Reviewer's statement of oversight. You are
> >>>>> not a damn human do be able to make such statement. You are a bot, a tool.
> >>>> Where exactly do the rules say that ? I seem to miss that.
> >>>> There is a policy document about _contributions_ made by AI, but I don't
> >>>> see the one that says that AI agents must not provide Reviewed-by: tags.
> >>> From my perspective, AI agents must NOT use the Reviewed-by tag for the
> >>> following reasons:
> >>> - We consider this a "person-trailer" and it implies agency
> >>> - Adding yourself to a commit via a trailer is a *binding responsibility* for
> >>> the change. A lot of tooling will cc the Reviewed-by addresses on follow-up
> >>> messages regarding code in this commit. If the address is bogus or doesn't
> >>> go to a developer, this is both wasteful and potentially frustrating.
> >> Hi Konstantin!
> >> The goal here is to inform maintainers that sashiko has successfully reviewed the patch
> >> and there were no findings, otherwise maintainers have to go to the web site and check the status.
> > That's fine.
> >> I’m not attached to any specific form of it, I thought Reviewed-by is the most obvious form.
> >> And we use Reported-by: tags with various tooling for years.
> > Reported-by: shows the existance of a problem that some tool found, a
> > subtle difference here.
> >> What do you think is the best form?
> >> I’ll pause sending reviewed-by tags until we have a discussion and agreement here.
> > Just say it in some other text form, that our tools will not pick up.
> > Like:
> > Tool XXXX reports that all is good:
> > https://....
> > or something like that?
> Sure, works for me.
Couldn't this be something like:
AI-analysed-by: bot-X
Sometimes we expect them to run and produce results, that we should
check, and in my experience act upon and sometimes ignore, as usual with
any type of comments, when I don't get those results I usually go and
look at the web interface, be it the public one or the private one I use
(while contributing to one of them, sashiko for full disclosure), to see
if it is just that it is taking longer to process that specific patch or
if it really finished.
If I go in the morning to look at my inbox and see everything there it
reduces some steps for me.
So, yeah, Reviewed-by is definetly for definetly persons, but having
some tag that states that it went thru automated reviewing^Wanalysis by
a definetly bot/thing/whatever that some people think is useful seems
useful.
And of course I think this should be all opt-in by each and every
maintainer, this definetly is not something to be uniformly agreed on,
so if some mechanism is put in place for, say, sashiko, to reply that it
didn't find any problem with a particular patch for the subsystem I'm
involved with, then I'd like for that info to be provided as a reply to
the message.
Cheers,
- Arnaldo
^ permalink raw reply
* Re: [PATCH v2 1/3] Doc: deprecated.rst: add strlcat()
From: David Laight @ 2026-05-16 16:35 UTC (permalink / raw)
To: Heiko Carstens
Cc: Kees Cook, Manuel Ebner, Andy Shevchenko, Jonathan Corbet,
Shuah Khan, Andy Whitcroft, Joe Perches, Dwaipayan Ray,
Lukas Bulwahn, Geert Uytterhoeven, Randy Dunlap, Jani Nikula,
open list:DOCUMENTATION PROCESS, open list:DOCUMENTATION,
open list
In-Reply-To: <20260516152819.14597A76-hca@linux.ibm.com>
On Sat, 16 May 2026 17:28:19 +0200
Heiko Carstens <hca@linux.ibm.com> wrote:
> On Thu, May 14, 2026 at 09:31:46AM -0700, Kees Cook wrote:
> > On Thu, May 14, 2026 at 06:26:53PM +0200, Manuel Ebner wrote:
> > > add strlcat and alternatives
> > >
> > > Signed-off-by: Manuel Ebner <manuelebner@mailbox.org>
> > > ---
> > > Documentation/process/deprecated.rst | 7 +++++++
> > > 1 file changed, 7 insertions(+)
> > >
> > > diff --git a/Documentation/process/deprecated.rst b/Documentation/process/deprecated.rst
> > > index fed56864d036..06e802f4bbfd 100644
> > > --- a/Documentation/process/deprecated.rst
> > > +++ b/Documentation/process/deprecated.rst
> > > @@ -153,6 +153,13 @@ used, and the destinations should be marked with the `__nonstring
> > > attribute to avoid future compiler warnings. For cases still needing
> > > NUL-padding, strtomem_pad() can be used.
> > >
> > > +strlcat()
> > > +---------
> > > +strlcat() must re-scan the destination string from the beginning on each
> > > +call (O(n^2) behavior). Alternatives are seq_buf_puts() and seq_buf_printf().
> > > +snprintf(), scnprintf() and sysfs_emit() are possible aswell, but the adoption
> > > +of the arguments needs to be taken care off.
> > > +
> >
> > How about just:
> >
> > strlcat() must re-scan the destination string from the beginning on each
> > call (O(n^2) behavior). Use the seq_buf API or similar instead.
>
> seq_buf API for appending something to e.g. boot_command_line seems to be odd,
> since boot_command_line is usually "just there" (depending on architecture and
> boot loader).
Indeed, but ISTR that code uses strcat() a lot of the time.
The lengths are all known, so memcpy() can be used.
I don't really see why strlcat() should be deprecated.
Clearly there are many cases where there are better ways to do things.
The only problem with strlcat() is that it returns the 'required length'.
So there are some broken uses.
- fs/nfs/flexfilelayout/flexfilelayout.c
- lib/kunit/string-stream.c (although the preceding vsnprintf() looks like the actual bug).
There is also some very strange code in security/selinus/ima.c - but it may be ok.
In reality the return value of strlcat() isn't really much worse that that
of snprintf().
-- David
>
> So if I would remove strlcat() from appending something to boot_command_line I
> would end up open-coding strlcat(), including the chance for the usual
> off-by-one bugs. Looks like this would be true for nearly all architectures.
>
> Is performance really the only reason to deprecate strlcat()? This seems to be
> a bit questionable to me.
^ permalink raw reply
* Re: Stop false review statements
From: Roman Gushchin @ 2026-05-16 15:49 UTC (permalink / raw)
To: Greg KH
Cc: Konstantin Ryabitsev, Guenter Roeck, Krzysztof Kozlowski,
sashiko-bot, sashiko-reviews, sashiko, Linux Kernel Workflows,
Linux Kernel Mailing List, devicetree, kfree
In-Reply-To: <2026051631-trolling-juggling-da1c@gregkh>
> On May 16, 2026, at 8:45 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Sat, May 16, 2026 at 08:41:43AM -0700, Roman Gushchin wrote:
>>
>>>> On May 16, 2026, at 8:20 AM, Konstantin Ryabitsev <mricon@kernel.org> wrote:
>>>
>>> On Sat, May 16, 2026 at 05:11:28AM -0700, Guenter Roeck wrote:
>>>>> On Sat, May 16, 2026 at 10:05:02AM +0200, Krzysztof Kozlowski wrote:
>>>>> What the hell is that:
>>>>>
>>>>> https://lore.kernel.org/all/20260515190707.033BDC2BCB0@smtp.kernel.org/
>>>>>
>>>>> As a bot you CANNOT MAKE a Reviewer's statement of oversight. You are
>>>>> not a damn human do be able to make such statement. You are a bot, a tool.
>>>>>
>>>>
>>>> Where exactly do the rules say that ? I seem to miss that.
>>>>
>>>> There is a policy document about _contributions_ made by AI, but I don't
>>>> see the one that says that AI agents must not provide Reviewed-by: tags.
>>>
>>> From my perspective, AI agents must NOT use the Reviewed-by tag for the
>>> following reasons:
>>>
>>> - We consider this a "person-trailer" and it implies agency
>>> - Adding yourself to a commit via a trailer is a *binding responsibility* for
>>> the change. A lot of tooling will cc the Reviewed-by addresses on follow-up
>>> messages regarding code in this commit. If the address is bogus or doesn't
>>> go to a developer, this is both wasteful and potentially frustrating.
>>
>> Hi Konstantin!
>>
>> The goal here is to inform maintainers that sashiko has successfully reviewed the patch
>> and there were no findings, otherwise maintainers have to go to the web site and check the status.
>
> That's fine.
>
>> I’m not attached to any specific form of it, I thought Reviewed-by is the most obvious form.
>> And we use Reported-by: tags with various tooling for years.
>
> Reported-by: shows the existance of a problem that some tool found, a
> subtle difference here.
>
>> What do you think is the best form?
>>
>> I’ll pause sending reviewed-by tags until we have a discussion and agreement here.
>
> Just say it in some other text form, that our tools will not pick up.
> Like:
> Tool XXXX reports that all is good:
> https://....
>
> or something like that?
Sure, works for me.
Thanks,
Roman
^ permalink raw reply
* Re: Stop false review statements
From: Greg KH @ 2026-05-16 15:45 UTC (permalink / raw)
To: Roman Gushchin
Cc: Konstantin Ryabitsev, Guenter Roeck, Krzysztof Kozlowski,
sashiko-bot, sashiko-reviews, sashiko, Linux Kernel Workflows,
Linux Kernel Mailing List, devicetree, kfree
In-Reply-To: <D659E814-069C-439A-B816-1BC383F38E1F@linux.dev>
On Sat, May 16, 2026 at 08:41:43AM -0700, Roman Gushchin wrote:
>
> > On May 16, 2026, at 8:20 AM, Konstantin Ryabitsev <mricon@kernel.org> wrote:
> >
> > On Sat, May 16, 2026 at 05:11:28AM -0700, Guenter Roeck wrote:
> >>> On Sat, May 16, 2026 at 10:05:02AM +0200, Krzysztof Kozlowski wrote:
> >>> What the hell is that:
> >>>
> >>> https://lore.kernel.org/all/20260515190707.033BDC2BCB0@smtp.kernel.org/
> >>>
> >>> As a bot you CANNOT MAKE a Reviewer's statement of oversight. You are
> >>> not a damn human do be able to make such statement. You are a bot, a tool.
> >>>
> >>
> >> Where exactly do the rules say that ? I seem to miss that.
> >>
> >> There is a policy document about _contributions_ made by AI, but I don't
> >> see the one that says that AI agents must not provide Reviewed-by: tags.
> >
> > From my perspective, AI agents must NOT use the Reviewed-by tag for the
> > following reasons:
> >
> > - We consider this a "person-trailer" and it implies agency
> > - Adding yourself to a commit via a trailer is a *binding responsibility* for
> > the change. A lot of tooling will cc the Reviewed-by addresses on follow-up
> > messages regarding code in this commit. If the address is bogus or doesn't
> > go to a developer, this is both wasteful and potentially frustrating.
>
> Hi Konstantin!
>
> The goal here is to inform maintainers that sashiko has successfully reviewed the patch
> and there were no findings, otherwise maintainers have to go to the web site and check the status.
That's fine.
> I’m not attached to any specific form of it, I thought Reviewed-by is the most obvious form.
> And we use Reported-by: tags with various tooling for years.
Reported-by: shows the existance of a problem that some tool found, a
subtle difference here.
> What do you think is the best form?
>
> I’ll pause sending reviewed-by tags until we have a discussion and agreement here.
Just say it in some other text form, that our tools will not pick up.
Like:
Tool XXXX reports that all is good:
https://....
or something like that?
thanks,
greg k-h
^ permalink raw reply
* Re: Stop false review statements
From: Roman Gushchin @ 2026-05-16 15:41 UTC (permalink / raw)
To: Konstantin Ryabitsev
Cc: Guenter Roeck, Krzysztof Kozlowski, sashiko-bot, sashiko-reviews,
sashiko, Linux Kernel Workflows, Linux Kernel Mailing List,
devicetree, kfree
In-Reply-To: <20260516-upbeat-tody-of-feminism-4ab00a@lemur>
> On May 16, 2026, at 8:20 AM, Konstantin Ryabitsev <mricon@kernel.org> wrote:
>
> On Sat, May 16, 2026 at 05:11:28AM -0700, Guenter Roeck wrote:
>>> On Sat, May 16, 2026 at 10:05:02AM +0200, Krzysztof Kozlowski wrote:
>>> What the hell is that:
>>>
>>> https://lore.kernel.org/all/20260515190707.033BDC2BCB0@smtp.kernel.org/
>>>
>>> As a bot you CANNOT MAKE a Reviewer's statement of oversight. You are
>>> not a damn human do be able to make such statement. You are a bot, a tool.
>>>
>>
>> Where exactly do the rules say that ? I seem to miss that.
>>
>> There is a policy document about _contributions_ made by AI, but I don't
>> see the one that says that AI agents must not provide Reviewed-by: tags.
>
> From my perspective, AI agents must NOT use the Reviewed-by tag for the
> following reasons:
>
> - We consider this a "person-trailer" and it implies agency
> - Adding yourself to a commit via a trailer is a *binding responsibility* for
> the change. A lot of tooling will cc the Reviewed-by addresses on follow-up
> messages regarding code in this commit. If the address is bogus or doesn't
> go to a developer, this is both wasteful and potentially frustrating.
Hi Konstantin!
The goal here is to inform maintainers that sashiko has successfully reviewed the patch
and there were no findings, otherwise maintainers have to go to the web site and check the status.
I’m not attached to any specific form of it, I thought Reviewed-by is the most obvious form.
And we use Reported-by: tags with various tooling for years.
What do you think is the best form?
I’ll pause sending reviewed-by tags until we have a discussion and agreement here.
Thanks
^ permalink raw reply
* Re: Stop false review statements
From: Greg KH @ 2026-05-16 15:36 UTC (permalink / raw)
To: Konstantin Ryabitsev
Cc: Guenter Roeck, Krzysztof Kozlowski, sashiko-bot, sashiko-reviews,
sashiko, Linux Kernel Workflows, Linux Kernel Mailing List,
devicetree@vger.kernel.org, kfree
In-Reply-To: <20260516-upbeat-tody-of-feminism-4ab00a@lemur>
On Sat, May 16, 2026 at 11:20:33AM -0400, Konstantin Ryabitsev wrote:
> On Sat, May 16, 2026 at 05:11:28AM -0700, Guenter Roeck wrote:
> > On Sat, May 16, 2026 at 10:05:02AM +0200, Krzysztof Kozlowski wrote:
> > > What the hell is that:
> > >
> > > https://lore.kernel.org/all/20260515190707.033BDC2BCB0@smtp.kernel.org/
> > >
> > > As a bot you CANNOT MAKE a Reviewer's statement of oversight. You are
> > > not a damn human do be able to make such statement. You are a bot, a tool.
> > >
> >
> > Where exactly do the rules say that ? I seem to miss that.
> >
> > There is a policy document about _contributions_ made by AI, but I don't
> > see the one that says that AI agents must not provide Reviewed-by: tags.
>
> >From my perspective, AI agents must NOT use the Reviewed-by tag for the
> following reasons:
>
> - We consider this a "person-trailer" and it implies agency
> - Adding yourself to a commit via a trailer is a *binding responsibility* for
> the change. A lot of tooling will cc the Reviewed-by addresses on follow-up
> messages regarding code in this commit. If the address is bogus or doesn't
> go to a developer, this is both wasteful and potentially frustrating.
I agree, any sort of "automated" tool shouldn't be adding these types of
tags.
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH v2 1/3] Doc: deprecated.rst: add strlcat()
From: Heiko Carstens @ 2026-05-16 15:28 UTC (permalink / raw)
To: Kees Cook
Cc: Manuel Ebner, Andy Shevchenko, Jonathan Corbet, Shuah Khan,
Andy Whitcroft, Joe Perches, Dwaipayan Ray, Lukas Bulwahn,
Geert Uytterhoeven, David Laight, Randy Dunlap, Jani Nikula,
open list:DOCUMENTATION PROCESS, open list:DOCUMENTATION,
open list
In-Reply-To: <202605140931.913048A68B@keescook>
On Thu, May 14, 2026 at 09:31:46AM -0700, Kees Cook wrote:
> On Thu, May 14, 2026 at 06:26:53PM +0200, Manuel Ebner wrote:
> > add strlcat and alternatives
> >
> > Signed-off-by: Manuel Ebner <manuelebner@mailbox.org>
> > ---
> > Documentation/process/deprecated.rst | 7 +++++++
> > 1 file changed, 7 insertions(+)
> >
> > diff --git a/Documentation/process/deprecated.rst b/Documentation/process/deprecated.rst
> > index fed56864d036..06e802f4bbfd 100644
> > --- a/Documentation/process/deprecated.rst
> > +++ b/Documentation/process/deprecated.rst
> > @@ -153,6 +153,13 @@ used, and the destinations should be marked with the `__nonstring
> > attribute to avoid future compiler warnings. For cases still needing
> > NUL-padding, strtomem_pad() can be used.
> >
> > +strlcat()
> > +---------
> > +strlcat() must re-scan the destination string from the beginning on each
> > +call (O(n^2) behavior). Alternatives are seq_buf_puts() and seq_buf_printf().
> > +snprintf(), scnprintf() and sysfs_emit() are possible aswell, but the adoption
> > +of the arguments needs to be taken care off.
> > +
>
> How about just:
>
> strlcat() must re-scan the destination string from the beginning on each
> call (O(n^2) behavior). Use the seq_buf API or similar instead.
seq_buf API for appending something to e.g. boot_command_line seems to be odd,
since boot_command_line is usually "just there" (depending on architecture and
boot loader).
So if I would remove strlcat() from appending something to boot_command_line I
would end up open-coding strlcat(), including the chance for the usual
off-by-one bugs. Looks like this would be true for nearly all architectures.
Is performance really the only reason to deprecate strlcat()? This seems to be
a bit questionable to me.
^ permalink raw reply
* Re: Stop false review statements
From: Konstantin Ryabitsev @ 2026-05-16 15:20 UTC (permalink / raw)
To: Guenter Roeck
Cc: Krzysztof Kozlowski, sashiko-bot, sashiko-reviews, sashiko,
Linux Kernel Workflows, Linux Kernel Mailing List,
devicetree@vger.kernel.org, kfree
In-Reply-To: <fcc4b719-2696-4f31-bac4-6c07f8ddec47@roeck-us.net>
On Sat, May 16, 2026 at 05:11:28AM -0700, Guenter Roeck wrote:
> On Sat, May 16, 2026 at 10:05:02AM +0200, Krzysztof Kozlowski wrote:
> > What the hell is that:
> >
> > https://lore.kernel.org/all/20260515190707.033BDC2BCB0@smtp.kernel.org/
> >
> > As a bot you CANNOT MAKE a Reviewer's statement of oversight. You are
> > not a damn human do be able to make such statement. You are a bot, a tool.
> >
>
> Where exactly do the rules say that ? I seem to miss that.
>
> There is a policy document about _contributions_ made by AI, but I don't
> see the one that says that AI agents must not provide Reviewed-by: tags.
From my perspective, AI agents must NOT use the Reviewed-by tag for the
following reasons:
- We consider this a "person-trailer" and it implies agency
- Adding yourself to a commit via a trailer is a *binding responsibility* for
the change. A lot of tooling will cc the Reviewed-by addresses on follow-up
messages regarding code in this commit. If the address is bogus or doesn't
go to a developer, this is both wasteful and potentially frustrating.
-K
^ permalink raw reply
* Re: [PATCH] docs: submitting-patches: Clarify that in English "reviewer" is a person
From: Vlastimil Babka (SUSE) @ 2026-05-16 14:39 UTC (permalink / raw)
To: Krzysztof Kozlowski, Jonathan Corbet, Shuah Khan, workflows,
linux-doc, linux-kernel
Cc: Greg Kroah-Hartman, Andrew Morton, David Hildenbrand,
Linus Torvalds, Guenter Roeck
In-Reply-To: <20260516123846.63413-2-krzysztof.kozlowski@oss.qualcomm.com>
On 5/16/26 14:38, Krzysztof Kozlowski wrote:
> Common understanding of word "Reviewer" is: a person performing a review
> work [1]. Tools are not persons, thus cannot be reviewers in this term.
> Also tools cannot make statements ("A Reviewed-by tag is a statement of
> opinion"), since making a statement needs some sort of conscious mind.
>
> Our docs already clearly mark that "Reviewed-by" must come from a
> person:
>
> - "By offering my Reviewed-by: tag, I state that:"
>
> Usage of first person "I" and word "state"
>
> - "A Reviewed-by tag is *a statement of opinion* that the patch is an
> appropriate modification of the kernel without any remaining serious"
>
> Only a person can make a statement of opinion.
>
> - "Any interested reviewer (who has done the work) can offer a
> Reviewed-by"
>
> A person can offer a tag thus above does not grant the tool
> permission to offer a tag.
>
> However this is not enough and apparently English is not that precise,
> so let's clarify that only a person can state the "Reviewer's statement
> of oversight".
>
> Link: https://en.wiktionary.org/wiki/reviewer [1]
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Vlastimil Babka <vbabka@kernel.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: David Hildenbrand <david@kernel.org>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
I agree with the intent that the tag is for people (whether they use a tool
or not to help them). We also don't put "Tested-by: kernel test robot" or
syzkaller on every commit that they test and find no bugs. Review is also
not just about absence of bugs, but agreeing with the larger design and
whether the change makes sense to do in the first place.
So whether that's achieved with this particular wording or differently,
Acked-by: Vlastimil Babka (SUSE) <vbabka@kernel.org>
>
> ---
>
> I find it silly to need to describe English, but it seems it is needed.
>
> https://lore.kernel.org/all/fd3b2ca7-4d64-4c4b-98a3-7d3285fa6826@roeck-us.net/
> ---
> Documentation/process/submitting-patches.rst | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/process/submitting-patches.rst b/Documentation/process/submitting-patches.rst
> index d7290e208e72..a989de43f3db 100644
> --- a/Documentation/process/submitting-patches.rst
> +++ b/Documentation/process/submitting-patches.rst
> @@ -581,10 +581,10 @@ By offering my Reviewed-by: tag, I state that:
>
> A Reviewed-by tag is a statement of opinion that the patch is an
> appropriate modification of the kernel without any remaining serious
> -technical issues. Any interested reviewer (who has done the work) can
> -offer a Reviewed-by tag for a patch. This tag serves to give credit to
> -reviewers and to inform maintainers of the degree of review which has been
> -done on the patch. Reviewed-by: tags, when supplied by reviewers known to
> +technical issues. Any interested reviewer (who has done the work and is a
> +person) can offer a Reviewed-by tag for a patch. This tag serves to give
> +credit to reviewers and to inform maintainers of the degree of review which has
> +been done on the patch. Reviewed-by: tags, when supplied by reviewers known to
> understand the subject area and to perform thorough reviews, will normally
> increase the likelihood of your patch getting into the kernel.
>
^ permalink raw reply
* Re: Stop false review statements
From: Krzysztof Kozlowski @ 2026-05-16 13:45 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Guenter Roeck, sashiko-bot, sashiko-reviews, sashiko,
Linux Kernel Workflows, Linux Kernel Mailing List,
devicetree@vger.kernel.org, kfree
In-Reply-To: <20260516132407.GA163589@killaraus.ideasonboard.com>
On 16/05/2026 15:24, Laurent Pinchart wrote:
> On Sat, May 16, 2026 at 02:29:15PM +0200, Krzysztof Kozlowski wrote:
>> On 16/05/2026 14:23, Guenter Roeck wrote:
>>> On 5/16/26 05:16, Krzysztof Kozlowski wrote:
>>>> On 16/05/2026 14:11, Guenter Roeck wrote:
>>>>> On Sat, May 16, 2026 at 10:05:02AM +0200, Krzysztof Kozlowski wrote:
>>>>>> What the hell is that:
>>>>>>
>>>>>> https://lore.kernel.org/all/20260515190707.033BDC2BCB0@smtp.kernel.org/
>>>>>>
>>>>>> As a bot you CANNOT MAKE a Reviewer's statement of oversight. You are
>>>>>> not a damn human do be able to make such statement. You are a bot, a tool.
>>>>>>
>>>>>
>>>>> Where exactly do the rules say that ? I seem to miss that.
>>>>>
>>>>> There is a policy document about _contributions_ made by AI, but I don't
>>>>> see the one that says that AI agents must not provide Reviewed-by: tags.
>>>>
>>>> Quotes from the existing policy:
>>>>
>>>> 1. "By offering my Reviewed-by: tag, I state that:"
>>>>
>>>> Tool cannot use first person "I". Tool cannot "state that".
>>>>
>>>> 2. "A Reviewed-by tag is *a statement of opinion* that the patch is an
>>>> appropriate modification of the kernel without any remaining serious"
>>>>
>>>> Tool cannot make a statement of opinion.
>>>>
>>>> 3. "Any interested reviewer (who has done the work) can offer a
>>>> Reviewed-by".
>>>>
>>>> Tool is not a reviewer as a person, thus above does not grant the tool
>>>> permission to offer a tag.
>>>
>>> I'd like to see that explicitly spelled out. Until then it is your opinion.
>>
>> It is not an opinion. It is written. I gave you quotes.
>>
>> Do you want to spell the rules of English language? That tool is not a
>> person?
>>
>> Shall I send the patch like:
>>
>> Any interested reviewer (who has done the work) can offer a
>> Reviewed-by.
>> +In English "reviewer" is a person [1].
>> + [1] https://en.wiktionary.org/wiki/reviewer
>>
>> Seriously, you expect to document the English language?
>>
>>>>>> Stop faking tags.
>>>>>>
>>>>>> And really, considering how many false positives Sashiko produces, how
>>>>>> poor review comments it gives, how many misleading comments, it's
>>>>>> unacceptable to me to consider that a review.
>>>>>>
>>>>>> Amount of useless noise Sashiko produces already changed my mind how
>>>>>> useful that tool is.
>
> Note this isn't en entirely new situation. As a maintainer, you know how
> much you trust each reviewer. You will consider some R-b tags as a sign
> you don't even have to look at a patch, and will completely ignore some
> others. There's a whole continuum in the middle. In some ways, reviews
> by an LLM are similar. You will trust them or not trust them.
>
> Except they're also very different.
>
> The kernel needs more skilled reviewers (I don't think this is a
> controversial statement). We can't expect all newcomers to start with
> extensive experience from day one, so there's a learning curve. I
> believe it's fine for more junior reviewers to send R-b tags even if
> they miss some issue, as long as they genuinely try and improve (and, in
> some unfortunate cases, decide to leave if patch review turns out not to
> be for them). Those R-b tags may feel like a bit of noise in the
> beginning, but that's compensated by their value increasing over time.
Yes, I agree. Reviews from inexperienced people are sometimes fruitless
or pointless per actual value they bring, but they allow a person
(again: person) to grow in the community with a credits being the reward.
>
> Bot reviews are not the same. Not only are they generated at a much
> larger scale than human reviews, they also won't learn from feedback you
> give them. Sure, the tools may be improved when cases of false positives
> are identified, and new LLMs may be trained with more (and better ?)
> data to improve the output, but they won't learn from the interactions.
>
> How much value a maintainer sees in those reviews is up to individual
> maintainers. I will personally not consider a R-b tag from an LLM to
> mean that a patch is ready to be merged (and I believe you won't
> either). As such, I think that a R-b from an LLM is misleading and
> doesn't provide good value. At best it's free advertising for company
> making closed-source tools, which I don't think we should encourage.
That's different aspect than I raised. I agree with above approach but
it is more subjective.
What I brought is object: our docs clearly state that reviewer can offer
reviewed-by tag. They do not allow non-reviewers to offer a tag and
English is clear on that - only a person is a reviewer.
Dog is not a reviewer.
Hammer is not a reviewer.
Tool is not a reviewer.
Guenter did not bring any counter arguments that our docs ALLOW
non-person to provide a reviewed-by tag. I brought that arguments as
excerpt from our documented policy.
Best regards,
Krzysztof
^ permalink raw reply
* Re: Stop false review statements
From: Laurent Pinchart @ 2026-05-16 13:24 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Guenter Roeck, sashiko-bot, sashiko-reviews, sashiko,
Linux Kernel Workflows, Linux Kernel Mailing List,
devicetree@vger.kernel.org, kfree
In-Reply-To: <b5f2a21a-9530-4efe-aed5-cc96aab74e88@kernel.org>
On Sat, May 16, 2026 at 02:29:15PM +0200, Krzysztof Kozlowski wrote:
> On 16/05/2026 14:23, Guenter Roeck wrote:
> > On 5/16/26 05:16, Krzysztof Kozlowski wrote:
> >> On 16/05/2026 14:11, Guenter Roeck wrote:
> >>> On Sat, May 16, 2026 at 10:05:02AM +0200, Krzysztof Kozlowski wrote:
> >>>> What the hell is that:
> >>>>
> >>>> https://lore.kernel.org/all/20260515190707.033BDC2BCB0@smtp.kernel.org/
> >>>>
> >>>> As a bot you CANNOT MAKE a Reviewer's statement of oversight. You are
> >>>> not a damn human do be able to make such statement. You are a bot, a tool.
> >>>>
> >>>
> >>> Where exactly do the rules say that ? I seem to miss that.
> >>>
> >>> There is a policy document about _contributions_ made by AI, but I don't
> >>> see the one that says that AI agents must not provide Reviewed-by: tags.
> >>
> >> Quotes from the existing policy:
> >>
> >> 1. "By offering my Reviewed-by: tag, I state that:"
> >>
> >> Tool cannot use first person "I". Tool cannot "state that".
> >>
> >> 2. "A Reviewed-by tag is *a statement of opinion* that the patch is an
> >> appropriate modification of the kernel without any remaining serious"
> >>
> >> Tool cannot make a statement of opinion.
> >>
> >> 3. "Any interested reviewer (who has done the work) can offer a
> >> Reviewed-by".
> >>
> >> Tool is not a reviewer as a person, thus above does not grant the tool
> >> permission to offer a tag.
> >
> > I'd like to see that explicitly spelled out. Until then it is your opinion.
>
> It is not an opinion. It is written. I gave you quotes.
>
> Do you want to spell the rules of English language? That tool is not a
> person?
>
> Shall I send the patch like:
>
> Any interested reviewer (who has done the work) can offer a
> Reviewed-by.
> +In English "reviewer" is a person [1].
> + [1] https://en.wiktionary.org/wiki/reviewer
>
> Seriously, you expect to document the English language?
>
> >>>> Stop faking tags.
> >>>>
> >>>> And really, considering how many false positives Sashiko produces, how
> >>>> poor review comments it gives, how many misleading comments, it's
> >>>> unacceptable to me to consider that a review.
> >>>>
> >>>> Amount of useless noise Sashiko produces already changed my mind how
> >>>> useful that tool is.
Note this isn't en entirely new situation. As a maintainer, you know how
much you trust each reviewer. You will consider some R-b tags as a sign
you don't even have to look at a patch, and will completely ignore some
others. There's a whole continuum in the middle. In some ways, reviews
by an LLM are similar. You will trust them or not trust them.
Except they're also very different.
The kernel needs more skilled reviewers (I don't think this is a
controversial statement). We can't expect all newcomers to start with
extensive experience from day one, so there's a learning curve. I
believe it's fine for more junior reviewers to send R-b tags even if
they miss some issue, as long as they genuinely try and improve (and, in
some unfortunate cases, decide to leave if patch review turns out not to
be for them). Those R-b tags may feel like a bit of noise in the
beginning, but that's compensated by their value increasing over time.
Bot reviews are not the same. Not only are they generated at a much
larger scale than human reviews, they also won't learn from feedback you
give them. Sure, the tools may be improved when cases of false positives
are identified, and new LLMs may be trained with more (and better ?)
data to improve the output, but they won't learn from the interactions.
How much value a maintainer sees in those reviews is up to individual
maintainers. I will personally not consider a R-b tag from an LLM to
mean that a patch is ready to be merged (and I believe you won't
either). As such, I think that a R-b from an LLM is misleading and
doesn't provide good value. At best it's free advertising for company
making closed-source tools, which I don't think we should encourage.
If some maintainers want LLM reviews and want to act on them, that's
their personal prerogative. They're free to decide on how much value
they see in those reviews, as well as on whether or not they consider
usage of those tools compatible with FOSS ethics. Those are personal
decisions. However, given that the ethical decision is personal, I am
strongly against forcing patch authors to act on automated LLM review.
> >>> We seem to have completely different experiences. Yes, it does produce
> >>> false positives, just like humans do. However, I have seen it find many
> >>> real bugs, including many in patches which already had Reviewed-by: tags
> >>> from (presumably) human reviewers.
> >>
> >> Of course it finds bugs. But it also produces - roughly - 80-90% false
> >> positives, completely useless.
> >>
> >
> > Really ? The ones I have seen are - roughly, to use the same term - 80-90%
> > true positives. Maybe you should explicitly ask for no Sashiko reviews in
> > your scope of responsibility.
>
> I already sent a patch to stop receiving all these emails and I stopped
> reading them completely, when fetched via b4 for review in mutt workflow.
>
> But this is not the point.
>
> Our docs clearly state what Reviewed-by means, regardless of the quality
> of the actual review. Poor quality is just another reason, less
> important, though.
--
Regards,
Laurent Pinchart
^ permalink raw reply
* [PATCH] docs: submitting-patches: Clarify that in English "reviewer" is a person
From: Krzysztof Kozlowski @ 2026-05-16 12:38 UTC (permalink / raw)
To: Jonathan Corbet, Shuah Khan, workflows, linux-doc, linux-kernel
Cc: Krzysztof Kozlowski, Greg Kroah-Hartman, Vlastimil Babka,
Andrew Morton, David Hildenbrand, Linus Torvalds
Common understanding of word "Reviewer" is: a person performing a review
work [1]. Tools are not persons, thus cannot be reviewers in this term.
Also tools cannot make statements ("A Reviewed-by tag is a statement of
opinion"), since making a statement needs some sort of conscious mind.
Our docs already clearly mark that "Reviewed-by" must come from a
person:
- "By offering my Reviewed-by: tag, I state that:"
Usage of first person "I" and word "state"
- "A Reviewed-by tag is *a statement of opinion* that the patch is an
appropriate modification of the kernel without any remaining serious"
Only a person can make a statement of opinion.
- "Any interested reviewer (who has done the work) can offer a
Reviewed-by"
A person can offer a tag thus above does not grant the tool
permission to offer a tag.
However this is not enough and apparently English is not that precise,
so let's clarify that only a person can state the "Reviewer's statement
of oversight".
Link: https://en.wiktionary.org/wiki/reviewer [1]
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Vlastimil Babka <vbabka@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: David Hildenbrand <david@kernel.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
---
I find it silly to need to describe English, but it seems it is needed.
https://lore.kernel.org/all/fd3b2ca7-4d64-4c4b-98a3-7d3285fa6826@roeck-us.net/
---
Documentation/process/submitting-patches.rst | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/Documentation/process/submitting-patches.rst b/Documentation/process/submitting-patches.rst
index d7290e208e72..a989de43f3db 100644
--- a/Documentation/process/submitting-patches.rst
+++ b/Documentation/process/submitting-patches.rst
@@ -581,10 +581,10 @@ By offering my Reviewed-by: tag, I state that:
A Reviewed-by tag is a statement of opinion that the patch is an
appropriate modification of the kernel without any remaining serious
-technical issues. Any interested reviewer (who has done the work) can
-offer a Reviewed-by tag for a patch. This tag serves to give credit to
-reviewers and to inform maintainers of the degree of review which has been
-done on the patch. Reviewed-by: tags, when supplied by reviewers known to
+technical issues. Any interested reviewer (who has done the work and is a
+person) can offer a Reviewed-by tag for a patch. This tag serves to give
+credit to reviewers and to inform maintainers of the degree of review which has
+been done on the patch. Reviewed-by: tags, when supplied by reviewers known to
understand the subject area and to perform thorough reviews, will normally
increase the likelihood of your patch getting into the kernel.
--
2.51.0
^ permalink raw reply related
* Re: Stop false review statements
From: Krzysztof Kozlowski @ 2026-05-16 12:29 UTC (permalink / raw)
To: Guenter Roeck
Cc: sashiko-bot, sashiko-reviews, sashiko, Linux Kernel Workflows,
Linux Kernel Mailing List, devicetree@vger.kernel.org, kfree
In-Reply-To: <fd3b2ca7-4d64-4c4b-98a3-7d3285fa6826@roeck-us.net>
On 16/05/2026 14:23, Guenter Roeck wrote:
> On 5/16/26 05:16, Krzysztof Kozlowski wrote:
>> On 16/05/2026 14:11, Guenter Roeck wrote:
>>> On Sat, May 16, 2026 at 10:05:02AM +0200, Krzysztof Kozlowski wrote:
>>>> What the hell is that:
>>>>
>>>> https://lore.kernel.org/all/20260515190707.033BDC2BCB0@smtp.kernel.org/
>>>>
>>>> As a bot you CANNOT MAKE a Reviewer's statement of oversight. You are
>>>> not a damn human do be able to make such statement. You are a bot, a tool.
>>>>
>>>
>>> Where exactly do the rules say that ? I seem to miss that.
>>>
>>> There is a policy document about _contributions_ made by AI, but I don't
>>> see the one that says that AI agents must not provide Reviewed-by: tags.
>>
>> Quotes from the existing policy:
>>
>> 1. "By offering my Reviewed-by: tag, I state that:"
>>
>> Tool cannot use first person "I". Tool cannot "state that".
>>
>> 2. "A Reviewed-by tag is *a statement of opinion* that the patch is an
>> appropriate modification of the kernel without any remaining serious"
>>
>> Tool cannot make a statement of opinion.
>>
>> 3. "Any interested reviewer (who has done the work) can offer a
>> Reviewed-by".
>>
>> Tool is not a reviewer as a person, thus above does not grant the tool
>> permission to offer a tag.
>>
>
> I'd like to see that explicitly spelled out. Until then it is your opinion.
It is not an opinion. It is written. I gave you quotes.
Do you want to spell the rules of English language? That tool is not a
person?
Shall I send the patch like:
Any interested reviewer (who has done the work) can offer a
Reviewed-by.
+In English "reviewer" is a person [1].
+ [1] https://en.wiktionary.org/wiki/reviewer
Seriously, you expect to document the English language?
>
>>>
>>>> Stop faking tags.
>>>>
>>>> And really, considering how many false positives Sashiko produces, how
>>>> poor review comments it gives, how many misleading comments, it's
>>>> unacceptable to me to consider that a review.
>>>>
>>>> Amount of useless noise Sashiko produces already changed my mind how
>>>> useful that tool is.
>>>
>>> We seem to have completely different experiences. Yes, it does produce
>>> false positives, just like humans do. However, I have seen it find many
>>> real bugs, including many in patches which already had Reviewed-by: tags
>>> from (presumably) human reviewers.
>>
>> Of course it finds bugs. But it also produces - roughly - 80-90% false
>> positives, completely useless.
>>
>
> Really ? The ones I have seen are - roughly, to use the same term - 80-90%
> true positives. Maybe you should explicitly ask for no Sashiko reviews in
> your scope of responsibility.
I already sent a patch to stop receiving all these emails and I stopped
reading them completely, when fetched via b4 for review in mutt workflow.
But this is not the point.
Our docs clearly state what Reviewed-by means, regardless of the quality
of the actual review. Poor quality is just another reason, less
important, though.
Best regards,
Krzysztof
^ permalink raw reply
* Re: Stop false review statements
From: Guenter Roeck @ 2026-05-16 12:23 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: sashiko-bot, sashiko-reviews, sashiko, Linux Kernel Workflows,
Linux Kernel Mailing List, devicetree@vger.kernel.org, kfree
In-Reply-To: <221cc52e-9918-43ea-b196-622a8cc6db05@kernel.org>
On 5/16/26 05:16, Krzysztof Kozlowski wrote:
> On 16/05/2026 14:11, Guenter Roeck wrote:
>> On Sat, May 16, 2026 at 10:05:02AM +0200, Krzysztof Kozlowski wrote:
>>> What the hell is that:
>>>
>>> https://lore.kernel.org/all/20260515190707.033BDC2BCB0@smtp.kernel.org/
>>>
>>> As a bot you CANNOT MAKE a Reviewer's statement of oversight. You are
>>> not a damn human do be able to make such statement. You are a bot, a tool.
>>>
>>
>> Where exactly do the rules say that ? I seem to miss that.
>>
>> There is a policy document about _contributions_ made by AI, but I don't
>> see the one that says that AI agents must not provide Reviewed-by: tags.
>
> Quotes from the existing policy:
>
> 1. "By offering my Reviewed-by: tag, I state that:"
>
> Tool cannot use first person "I". Tool cannot "state that".
>
> 2. "A Reviewed-by tag is *a statement of opinion* that the patch is an
> appropriate modification of the kernel without any remaining serious"
>
> Tool cannot make a statement of opinion.
>
> 3. "Any interested reviewer (who has done the work) can offer a
> Reviewed-by".
>
> Tool is not a reviewer as a person, thus above does not grant the tool
> permission to offer a tag.
>
I'd like to see that explicitly spelled out. Until then it is your opinion.
>>
>>> Stop faking tags.
>>>
>>> And really, considering how many false positives Sashiko produces, how
>>> poor review comments it gives, how many misleading comments, it's
>>> unacceptable to me to consider that a review.
>>>
>>> Amount of useless noise Sashiko produces already changed my mind how
>>> useful that tool is.
>>
>> We seem to have completely different experiences. Yes, it does produce
>> false positives, just like humans do. However, I have seen it find many
>> real bugs, including many in patches which already had Reviewed-by: tags
>> from (presumably) human reviewers.
>
> Of course it finds bugs. But it also produces - roughly - 80-90% false
> positives, completely useless.
>
Really ? The ones I have seen are - roughly, to use the same term - 80-90%
true positives. Maybe you should explicitly ask for no Sashiko reviews in
your scope of responsibility.
Thanks,
Guenter
^ permalink raw reply
* Re: Stop false review statements
From: Krzysztof Kozlowski @ 2026-05-16 12:16 UTC (permalink / raw)
To: Guenter Roeck
Cc: sashiko-bot, sashiko-reviews, sashiko, Linux Kernel Workflows,
Linux Kernel Mailing List, devicetree@vger.kernel.org, kfree
In-Reply-To: <fcc4b719-2696-4f31-bac4-6c07f8ddec47@roeck-us.net>
On 16/05/2026 14:11, Guenter Roeck wrote:
> On Sat, May 16, 2026 at 10:05:02AM +0200, Krzysztof Kozlowski wrote:
>> What the hell is that:
>>
>> https://lore.kernel.org/all/20260515190707.033BDC2BCB0@smtp.kernel.org/
>>
>> As a bot you CANNOT MAKE a Reviewer's statement of oversight. You are
>> not a damn human do be able to make such statement. You are a bot, a tool.
>>
>
> Where exactly do the rules say that ? I seem to miss that.
>
> There is a policy document about _contributions_ made by AI, but I don't
> see the one that says that AI agents must not provide Reviewed-by: tags.
Quotes from the existing policy:
1. "By offering my Reviewed-by: tag, I state that:"
Tool cannot use first person "I". Tool cannot "state that".
2. "A Reviewed-by tag is *a statement of opinion* that the patch is an
appropriate modification of the kernel without any remaining serious"
Tool cannot make a statement of opinion.
3. "Any interested reviewer (who has done the work) can offer a
Reviewed-by".
Tool is not a reviewer as a person, thus above does not grant the tool
permission to offer a tag.
>
>> Stop faking tags.
>>
>> And really, considering how many false positives Sashiko produces, how
>> poor review comments it gives, how many misleading comments, it's
>> unacceptable to me to consider that a review.
>>
>> Amount of useless noise Sashiko produces already changed my mind how
>> useful that tool is.
>
> We seem to have completely different experiences. Yes, it does produce
> false positives, just like humans do. However, I have seen it find many
> real bugs, including many in patches which already had Reviewed-by: tags
> from (presumably) human reviewers.
Of course it finds bugs. But it also produces - roughly - 80-90% false
positives, completely useless.
This is very poor review score.
>
> Again, it appears that our experience is completely different than mine,
> but after several weeks of getting code reviews from sashiko I do have to
> say that I trust its review feedback significantly more than human reviews.
> Sure, it does not guarantee that a patch is indeed bug free. A human review
> doesn't guarantee it either.
>
>>
>> I will be NAKing every damn tag produced by such tools.
>
> I'd like to see an official policy. Until then I'll ignore your NAK in my
> scope of responsibility.
:)
Best regards,
Krzysztof
^ permalink raw reply
* Re: Stop false review statements
From: Guenter Roeck @ 2026-05-16 12:11 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: sashiko-bot, sashiko-reviews, sashiko, Linux Kernel Workflows,
Linux Kernel Mailing List, devicetree@vger.kernel.org, kfree
In-Reply-To: <ad139e54-a7f0-4d09-832c-6b2bf2e93e03@kernel.org>
On Sat, May 16, 2026 at 10:05:02AM +0200, Krzysztof Kozlowski wrote:
> What the hell is that:
>
> https://lore.kernel.org/all/20260515190707.033BDC2BCB0@smtp.kernel.org/
>
> As a bot you CANNOT MAKE a Reviewer's statement of oversight. You are
> not a damn human do be able to make such statement. You are a bot, a tool.
>
Where exactly do the rules say that ? I seem to miss that.
There is a policy document about _contributions_ made by AI, but I don't
see the one that says that AI agents must not provide Reviewed-by: tags.
> Stop faking tags.
>
> And really, considering how many false positives Sashiko produces, how
> poor review comments it gives, how many misleading comments, it's
> unacceptable to me to consider that a review.
>
> Amount of useless noise Sashiko produces already changed my mind how
> useful that tool is.
We seem to have completely different experiences. Yes, it does produce
false positives, just like humans do. However, I have seen it find many
real bugs, including many in patches which already had Reviewed-by: tags
from (presumably) human reviewers.
Again, it appears that our experience is completely different than mine,
but after several weeks of getting code reviews from sashiko I do have to
say that I trust its review feedback significantly more than human reviews.
Sure, it does not guarantee that a patch is indeed bug free. A human review
doesn't guarantee it either.
>
> I will be NAKing every damn tag produced by such tools.
I'd like to see an official policy. Until then I'll ignore your NAK in my
scope of responsibility.
Thanks,
Guenter
^ permalink raw reply
* Stop false review statements
From: Krzysztof Kozlowski @ 2026-05-16 8:05 UTC (permalink / raw)
To: sashiko-bot, sashiko-reviews, sashiko, Linux Kernel Workflows,
Linux Kernel Mailing List, devicetree@vger.kernel.org
What the hell is that:
https://lore.kernel.org/all/20260515190707.033BDC2BCB0@smtp.kernel.org/
As a bot you CANNOT MAKE a Reviewer's statement of oversight. You are
not a damn human do be able to make such statement. You are a bot, a tool.
Stop faking tags.
And really, considering how many false positives Sashiko produces, how
poor review comments it gives, how many misleading comments, it's
unacceptable to me to consider that a review.
Amount of useless noise Sashiko produces already changed my mind how
useful that tool is.
I will be NAKing every damn tag produced by such tools.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v13 0/4] kunit: Add support for suppressing warning backtraces
From: Albert Esteve @ 2026-05-15 14:25 UTC (permalink / raw)
To: Guenter Roeck
Cc: Arnd Bergmann, Brendan Higgins, David Gow, Rae Moar,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Simona Vetter, Jonathan Corbet, Shuah Khan, Andrew Morton,
Paul Walmsley, Palmer Dabbelt, Albert Ou, Alexandre Ghiti,
linux-kernel, linux-arch, linux-kselftest, kunit-dev, dri-devel,
workflows, linux-riscv, linux-doc, peterz, Alessandro Carminati,
Kees Cook, Linux Kernel Functional Testing, Maíra Canal,
Dan Carpenter, Simona Vetter
In-Reply-To: <7bcead90-7d96-4101-bd13-dde2c5ded1aa@roeck-us.net>
On Fri, May 15, 2026 at 3:51 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> Hi Albert,
>
> On 5/15/26 05:29, Albert Esteve wrote:
> ...
>
> > Guenter Roeck (3):
> > kunit: Add backtrace suppression self-tests
> > drm: Suppress intentional warning backtraces in scaling unit tests
> > kunit: Add documentation for warning backtrace suppression API
> >
>
>
> How much of that is from me at this point ? Wouldn't it make sense to drop me
> as "author" of those patches ?
Hi Guenter,
I do not mind authorship either. Signed-off-by lines already attest to
everyone's contribution.
In principle I am done with Sashiko's reviews. I addressed all
comments in their respective patches. Some lead to the rabbit holes I
put myself into for previous versions.
I do not plan to send a new version unless a human review requires a change.
If that happens, I may update the authorship. Otherwise, I'd keep it
as is, since you said you do not mind :)
Thanks!
BR,
Albert.
>
> I would not mind. I had the idea, but others like you are doing the hard work
> of pushing it through.
>
> Thanks,
> Guenter
>
^ permalink raw reply
* Re: [PATCH v13 2/4] kunit: Add backtrace suppression self-tests
From: Albert Esteve @ 2026-05-15 14:14 UTC (permalink / raw)
To: Arnd Bergmann, Brendan Higgins, David Gow, Rae Moar,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Simona Vetter, Jonathan Corbet, Shuah Khan, Andrew Morton,
Paul Walmsley, Palmer Dabbelt, Albert Ou, Alexandre Ghiti
Cc: linux-kernel, linux-arch, linux-kselftest, kunit-dev, dri-devel,
workflows, linux-riscv, linux-doc, peterz, Guenter Roeck,
Linux Kernel Functional Testing, Alessandro Carminati,
Dan Carpenter, Kees Cook
In-Reply-To: <20260515-kunit_add_support-v13-2-18ee42f96e7b@redhat.com>
On Fri, May 15, 2026 at 2:30 PM Albert Esteve <aesteve@redhat.com> wrote:
>
> From: Guenter Roeck <linux@roeck-us.net>
>
> Add unit tests to verify that warning backtrace suppression works.
>
> Tests cover both API forms:
> - Scoped: kunit_warning_suppress() with in-block count verification
> and post-block inactivity check.
> - Direct functions: kunit_start/end_suppress_warning() with
> sequential independent suppression blocks and per-block counts.
>
> Furthermore, tests verify incremental warning counting, that
> kunit_has_active_suppress_warning() transitions correctly around
> suppression boundaries, and that suppression active in the test
> kthread does not leak to a separate kthread.
>
> If backtrace suppression does _not_ work, the unit tests will likely
> trigger unsuppressed backtraces, which should actually help to get
> the affected architectures / platforms fixed.
>
Another set of sashiko comments for this patch
https://sashiko.dev/#/patchset/20260515-kunit_add_support-v13-0-18ee42f96e7b%40redhat.com?part=2
here:
1. CPU spike from while (!kthread_should_stop()) schedule()
Ha! I expected this one because I saw it in a previous review from the
bot. schedule() from TASK_RUNNING yields the CPU; it does not
spin-wait. The thread is rescheduled only when the scheduler gives it
time, not in a tight loop. But the important thing is that the window
where this loop actually runs is negligible: the parent calls
kthread_stop() immediately after wait_for_completion() returns. Using
set_current_state(TASK_INTERRUPTIBLE) would be slightly more
CPU-friendly, but for a test that probably runs and exits in
microseconds, it makes no practical difference. And it unnecessarily
adds complexity.
2. Orphaned kthread on early abort
This cannot happen in this test. The only KUNIT_ASSERT_* that could
abort early is KUNIT_ASSERT_FALSE(test, IS_ERR(task)). If that
assertion fails, it means kthread_run() itself returned an error,
therefore, the kthread was never started and there is nothing to
orphan. If kthread_run() succeeds, the assertion passes, and execution
continues sequentially to kthread_stop(). No code path allows a live
kthread to exist while bypassing kthread_stop().
> Tested-by: Linux Kernel Functional Testing <lkft@linaro.org>
> Acked-by: Dan Carpenter <dan.carpenter@linaro.org>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Alessandro Carminati <acarmina@redhat.com>
> Reviewed-by: David Gow <david@davidgow.net>
> Signed-off-by: Albert Esteve <aesteve@redhat.com>
> ---
> lib/kunit/Makefile | 1 +
> lib/kunit/backtrace-suppression-test.c | 192 +++++++++++++++++++++++++++++++++
> 2 files changed, 193 insertions(+)
>
> diff --git a/lib/kunit/Makefile b/lib/kunit/Makefile
> index 4592f9d0aa8dd..2e8a6b71a2ab0 100644
> --- a/lib/kunit/Makefile
> +++ b/lib/kunit/Makefile
> @@ -22,6 +22,7 @@ obj-$(if $(CONFIG_KUNIT),y) += hooks.o
>
> obj-$(CONFIG_KUNIT_TEST) += kunit-test.o
> obj-$(CONFIG_KUNIT_TEST) += platform-test.o
> +obj-$(CONFIG_KUNIT_TEST) += backtrace-suppression-test.o
>
> # string-stream-test compiles built-in only.
> ifeq ($(CONFIG_KUNIT_TEST),y)
> diff --git a/lib/kunit/backtrace-suppression-test.c b/lib/kunit/backtrace-suppression-test.c
> new file mode 100644
> index 0000000000000..59a038b2739f5
> --- /dev/null
> +++ b/lib/kunit/backtrace-suppression-test.c
> @@ -0,0 +1,192 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * KUnit test for suppressing warning tracebacks.
> + *
> + * Copyright (C) 2024, Guenter Roeck
> + * Author: Guenter Roeck <linux@roeck-us.net>
> + */
> +
> +#include <kunit/test.h>
> +#include <linux/bug.h>
> +#include <linux/completion.h>
> +#include <linux/kthread.h>
> +
> +static void backtrace_suppression_test_warn_direct(struct kunit *test)
> +{
> + if (!IS_ENABLED(CONFIG_BUG))
> + kunit_skip(test, "requires CONFIG_BUG");
> +
> + kunit_warning_suppress(test) {
> + WARN(1, "This backtrace should be suppressed");
> + /*
> + * Count must be checked inside the scope; the handle
> + * is not accessible after the block exits.
> + */
> + KUNIT_EXPECT_SUPPRESSED_WARNING_COUNT(test, 1);
> + }
> + KUNIT_EXPECT_FALSE(test, kunit_has_active_suppress_warning());
> +}
> +
> +static noinline void trigger_backtrace_warn(void)
> +{
> + WARN(1, "This backtrace should be suppressed");
> +}
> +
> +static void backtrace_suppression_test_warn_indirect(struct kunit *test)
> +{
> + if (!IS_ENABLED(CONFIG_BUG))
> + kunit_skip(test, "requires CONFIG_BUG");
> +
> + kunit_warning_suppress(test) {
> + trigger_backtrace_warn();
> + KUNIT_EXPECT_SUPPRESSED_WARNING_COUNT(test, 1);
> + }
> +}
> +
> +static void backtrace_suppression_test_warn_multi(struct kunit *test)
> +{
> + if (!IS_ENABLED(CONFIG_BUG))
> + kunit_skip(test, "requires CONFIG_BUG");
> +
> + kunit_warning_suppress(test) {
> + WARN(1, "This backtrace should be suppressed");
> + trigger_backtrace_warn();
> + KUNIT_EXPECT_SUPPRESSED_WARNING_COUNT(test, 2);
> + }
> +}
> +
> +static void backtrace_suppression_test_warn_on_direct(struct kunit *test)
> +{
> + if (!IS_ENABLED(CONFIG_BUG))
> + kunit_skip(test, "requires CONFIG_BUG");
> +
> + kunit_warning_suppress(test) {
> + WARN_ON(1);
> + KUNIT_EXPECT_SUPPRESSED_WARNING_COUNT(test, 1);
> + }
> +}
> +
> +static noinline void trigger_backtrace_warn_on(void)
> +{
> + WARN_ON(1);
> +}
> +
> +static void backtrace_suppression_test_warn_on_indirect(struct kunit *test)
> +{
> + if (!IS_ENABLED(CONFIG_BUG))
> + kunit_skip(test, "requires CONFIG_BUG");
> +
> + kunit_warning_suppress(test) {
> + trigger_backtrace_warn_on();
> + KUNIT_EXPECT_SUPPRESSED_WARNING_COUNT(test, 1);
> + }
> +}
> +
> +static void backtrace_suppression_test_count(struct kunit *test)
> +{
> + if (!IS_ENABLED(CONFIG_BUG))
> + kunit_skip(test, "requires CONFIG_BUG");
> +
> + kunit_warning_suppress(test) {
> + KUNIT_EXPECT_SUPPRESSED_WARNING_COUNT(test, 0);
> +
> + WARN(1, "suppressed");
> + KUNIT_EXPECT_SUPPRESSED_WARNING_COUNT(test, 1);
> +
> + WARN(1, "suppressed again");
> + KUNIT_EXPECT_SUPPRESSED_WARNING_COUNT(test, 2);
> + }
> +}
> +
> +static void backtrace_suppression_test_active_state(struct kunit *test)
> +{
> + KUNIT_EXPECT_FALSE(test, kunit_has_active_suppress_warning());
> +
> + kunit_warning_suppress(test) {
> + KUNIT_EXPECT_TRUE(test, kunit_has_active_suppress_warning());
> + }
> +
> + KUNIT_EXPECT_FALSE(test, kunit_has_active_suppress_warning());
> +
> + kunit_warning_suppress(test) {
> + KUNIT_EXPECT_TRUE(test, kunit_has_active_suppress_warning());
> + }
> +
> + KUNIT_EXPECT_FALSE(test, kunit_has_active_suppress_warning());
> +}
> +
> +static void backtrace_suppression_test_multi_scope(struct kunit *test)
> +{
> + struct kunit_suppressed_warning *sw1, *sw2;
> +
> + if (!IS_ENABLED(CONFIG_BUG))
> + kunit_skip(test, "requires CONFIG_BUG");
> +
> + sw1 = kunit_start_suppress_warning(test);
> + trigger_backtrace_warn_on();
> + WARN(1, "suppressed by sw1");
> + kunit_end_suppress_warning(test, sw1);
> +
> + sw2 = kunit_start_suppress_warning(test);
> + WARN(1, "suppressed by sw2");
> + kunit_end_suppress_warning(test, sw2);
> +
> + KUNIT_EXPECT_EQ(test, kunit_suppressed_warning_count(sw1), 2);
> + KUNIT_EXPECT_EQ(test, kunit_suppressed_warning_count(sw2), 1);
> +}
> +
> +struct cross_kthread_data {
> + bool was_active;
> + struct completion done;
> +};
> +
> +static int cross_kthread_fn(void *data)
> +{
> + struct cross_kthread_data *d = data;
> +
> + d->was_active = kunit_has_active_suppress_warning();
> + complete(&d->done);
> + while (!kthread_should_stop())
> + schedule();
> + return 0;
> +}
> +
> +static void backtrace_suppression_test_cross_kthread(struct kunit *test)
> +{
> + struct cross_kthread_data data;
> + struct task_struct *task;
> +
> + data.was_active = false;
> + init_completion(&data.done);
> +
> + kunit_warning_suppress(test) {
> + task = kthread_run(cross_kthread_fn, &data, "kunit-cross-test");
> + KUNIT_ASSERT_FALSE(test, IS_ERR(task));
> + wait_for_completion(&data.done);
> + kthread_stop(task);
> + }
> +
> + KUNIT_EXPECT_FALSE(test, data.was_active);
> +}
> +
> +static struct kunit_case backtrace_suppression_test_cases[] = {
> + KUNIT_CASE(backtrace_suppression_test_warn_direct),
> + KUNIT_CASE(backtrace_suppression_test_warn_indirect),
> + KUNIT_CASE(backtrace_suppression_test_warn_multi),
> + KUNIT_CASE(backtrace_suppression_test_warn_on_direct),
> + KUNIT_CASE(backtrace_suppression_test_warn_on_indirect),
> + KUNIT_CASE(backtrace_suppression_test_count),
> + KUNIT_CASE(backtrace_suppression_test_active_state),
> + KUNIT_CASE(backtrace_suppression_test_multi_scope),
> + KUNIT_CASE(backtrace_suppression_test_cross_kthread),
> + {}
> +};
> +
> +static struct kunit_suite backtrace_suppression_test_suite = {
> + .name = "backtrace-suppression-test",
> + .test_cases = backtrace_suppression_test_cases,
> +};
> +kunit_test_suites(&backtrace_suppression_test_suite);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("KUnit test to verify warning backtrace suppression");
>
> --
> 2.53.0
>
^ permalink raw reply
* Re: [PATCH v13 0/4] kunit: Add support for suppressing warning backtraces
From: Guenter Roeck @ 2026-05-15 13:51 UTC (permalink / raw)
To: Albert Esteve, Arnd Bergmann, Brendan Higgins, David Gow,
Rae Moar, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
David Airlie, Simona Vetter, Jonathan Corbet, Shuah Khan,
Andrew Morton, Paul Walmsley, Palmer Dabbelt, Albert Ou,
Alexandre Ghiti
Cc: linux-kernel, linux-arch, linux-kselftest, kunit-dev, dri-devel,
workflows, linux-riscv, linux-doc, peterz, Alessandro Carminati,
Kees Cook, Linux Kernel Functional Testing, Maíra Canal,
Dan Carpenter, Simona Vetter
In-Reply-To: <20260515-kunit_add_support-v13-0-18ee42f96e7b@redhat.com>
Hi Albert,
On 5/15/26 05:29, Albert Esteve wrote:
...
> Guenter Roeck (3):
> kunit: Add backtrace suppression self-tests
> drm: Suppress intentional warning backtraces in scaling unit tests
> kunit: Add documentation for warning backtrace suppression API
>
How much of that is from me at this point ? Wouldn't it make sense to drop me
as "author" of those patches ?
I would not mind. I had the idea, but others like you are doing the hard work
of pushing it through.
Thanks,
Guenter
^ permalink raw reply
* Re: [PATCH v13 1/4] bug/kunit: Core support for suppressing warning backtraces
From: Albert Esteve @ 2026-05-15 13:36 UTC (permalink / raw)
To: Arnd Bergmann, Brendan Higgins, David Gow, Rae Moar,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Simona Vetter, Jonathan Corbet, Shuah Khan, Andrew Morton,
Paul Walmsley, Palmer Dabbelt, Albert Ou, Alexandre Ghiti
Cc: linux-kernel, linux-arch, linux-kselftest, kunit-dev, dri-devel,
workflows, linux-riscv, linux-doc, peterz, Alessandro Carminati,
Guenter Roeck, Kees Cook
In-Reply-To: <20260515-kunit_add_support-v13-1-18ee42f96e7b@redhat.com>
On Fri, May 15, 2026 at 2:29 PM Albert Esteve <aesteve@redhat.com> wrote:
>
> From: Alessandro Carminati <acarmina@redhat.com>
>
> Some unit tests intentionally trigger warning backtraces by passing bad
> parameters to kernel API functions. Such unit tests typically check the
> return value from such calls, not the existence of the warning backtrace.
>
> Such intentionally generated warning backtraces are neither desirable
> nor useful for a number of reasons:
> - They can result in overlooked real problems.
> - A warning that suddenly starts to show up in unit tests needs to be
> investigated and has to be marked to be ignored, for example by
> adjusting filter scripts. Such filters are ad hoc because there is
> no real standard format for warnings. On top of that, such filter
> scripts would require constant maintenance.
>
> Solve the problem by providing a means to suppress warning backtraces
> originating from the current kthread while executing test code. Since
> each KUnit test runs in its own kthread, this effectively scopes
> suppression to the test that enabled it. Limit changes to generic code
> to the absolute minimum.
>
> Implementation details:
> Suppression is integrated into the existing KUnit hooks infrastructure
> in test-bug.h, reusing the kunit_running static branch for zero
> overhead when no tests are running.
>
> Suppression is checked at three points in the warning path:
> - In warn_slowpath_fmt(), the check runs before any output, fully
> suppressing both message and backtrace. This covers architectures
> without __WARN_FLAGS.
> - In __warn_printk(), the check suppresses the warning message text.
> This covers architectures that define __WARN_FLAGS but not their own
> __WARN_printf (arm64, loongarch, parisc, powerpc, riscv, sh), where
> the message is printed before the trap enters __report_bug().
> - In __report_bug(), the check runs before __warn() is called,
> suppressing the backtrace and stack dump.
>
> To avoid double-counting on architectures where both __warn_printk()
> and __report_bug() run for the same warning, kunit_is_suppressed_warning()
> takes a bool parameter: true to increment the suppression counter
> (used in warn_slowpath_fmt and __report_bug), false to check only
> (used in __warn_printk).
>
> The suppression state is dynamically allocated via kunit_kzalloc() and
> tied to the KUnit test lifecycle via kunit_add_action(), ensuring
> automatic cleanup at test exit. On cleanup, the node is removed with
> list_del_rcu() followed by synchronize_rcu() to wait for any concurrent
> RCU readers to finish. Because kunit_end_suppress_warning() (and the
> __cleanup wrapper) always runs from process context, synchronize_rcu()
> is safe. The handle memory remains valid until the test exits, so the
> suppression count can be read after the scope closes. Writer-side
> access to the global suppression list is serialized with a spinlock;
> readers use RCU. To avoid false suppression of warnings fired from
> hardware interrupt handlers (where current still points to the test
> task), the check exits early when not in task context.
>
> Two API forms are provided:
> - kunit_warning_suppress(test) { ... }: scoped, uses __cleanup for
> automatic teardown on scope exit, kunit_add_action() as safety net
> for abnormal exits (e.g. kthread_exit from failed assertions).
> Suppression handle is only accessible inside the block.
> - kunit_start/end_suppress_warning(test): direct functions returning
> an explicit handle, for retaining the handle within the test,
> or for cross-function usage.
Let me address sashiko's comments for
https://sashiko.dev/#/patchset/20260515-kunit_add_support-v13-0-18ee42f96e7b%40redhat.com?part=1
here:
1. "Is this assumption always accurate? Tests frequently acquire
spinlocks or RCU read locks."
The assumption is accurate because kunit_end_suppress_warning() and
the __cleanup wrapper fire at the closing brace of the
kunit_warning_suppress() scope. If a developer holds a spinlock or RCU
read lock when that scope closes, their test is structurally incorrect
regardless of this API. The API documentation notes that process
context is required.
2. "If kunit_start_suppress_warning() fails and returns NULL, will
this skip the entire loop body?"
Yes, intentionally. KUNIT_FAIL() is called before returning NULL, so
the test is already marked as failed at that point. Skipping the body
of a failed test is expected KUnit behavior. In patch 3,
scaling_factor is initialized to INT_MIN precisely for this reason.
3. "Does this mean the single-fire budget is consumed anyway on
non-CONFIG_GENERIC_BUG architectures?"
Yes, true, but it should only affect non-__WARN_FLAGS architectures.
If there is demand, it can be addressed in a follow-up series. It does
not affect current API users.
4. "Would GFP_KERNEL / synchronize_rcu() cause a sleep-in-atomic bug
if used in atomic context?"
Yes, and that would be a test design error. kunit_warning_suppress()
is a KUnit test API; KUnit tests run in process context by design. As
stated in `Documentation/core-api/memory-allocation.rst`, GFP_KERNEL
implies GFP_RECLAIM, which requires the calling context to be allowed
to sleep. Using it in an atomic context is incorrect regardless of
which API calls it. `kunit_kzalloc(test, ..., GFP_KERNEL)` is the
standard allocation pattern throughout KUnit itself (e.g.,
lib/kunit/assert_test.c, platform-test.c, ...), so this API follows
the same convention.
5. "Could a child kthread's task_struct be freed and reused, causing
false suppression?"
The API is designed to be called from the test task only. w->task =
current stores the caller's task_struct, and the inline comment
explains the stability guarantee: the test task cannot exit before
KUnit tears down the test. The correct pattern for child kthread is
demonstrated in backtrace_suppression_test_cross_kthread: the test
task opens and closes the suppression scope; child threads only read
the suppression state. Otherwise it is an API misuse.
>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Alessandro Carminati <acarmina@redhat.com>
> Reviewed-by: Kees Cook <kees@kernel.org>
> Reviewed-by: David Gow <david@davidgow.net>
> Signed-off-by: Albert Esteve <aesteve@redhat.com>
> ---
> include/kunit/test-bug.h | 26 ++++++++++
> include/kunit/test.h | 98 ++++++++++++++++++++++++++++++++++++
> kernel/panic.c | 11 ++++
> lib/bug.c | 12 ++++-
> lib/kunit/Makefile | 3 +-
> lib/kunit/bug.c | 127 +++++++++++++++++++++++++++++++++++++++++++++++
> lib/kunit/hooks-impl.h | 2 +
> 7 files changed, 276 insertions(+), 3 deletions(-)
>
> diff --git a/include/kunit/test-bug.h b/include/kunit/test-bug.h
> index 47aa8f21ccce8..99869029fc686 100644
> --- a/include/kunit/test-bug.h
> +++ b/include/kunit/test-bug.h
> @@ -10,6 +10,7 @@
> #define _KUNIT_TEST_BUG_H
>
> #include <linux/stddef.h> /* for NULL */
> +#include <linux/types.h> /* for bool */
>
> #if IS_ENABLED(CONFIG_KUNIT)
>
> @@ -23,6 +24,7 @@ DECLARE_STATIC_KEY_FALSE(kunit_running);
> extern struct kunit_hooks_table {
> __printf(3, 4) void (*fail_current_test)(const char*, int, const char*, ...);
> void *(*get_static_stub_address)(struct kunit *test, void *real_fn_addr);
> + bool (*is_suppressed_warning)(bool count);
> } kunit_hooks;
>
> /**
> @@ -60,9 +62,33 @@ static inline struct kunit *kunit_get_current_test(void)
> } \
> } while (0)
>
> +/**
> + * kunit_is_suppressed_warning() - Check if warnings are being suppressed
> + * by the current KUnit test.
> + * @count: if true, increment the suppression counter on match.
> + *
> + * Returns true if the current task has active warning suppression.
> + * Uses the kunit_running static branch for zero overhead when no tests run.
> + *
> + * A single WARN*() may traverse multiple call sites in the warning path
> + * (e.g., __warn_printk() and __report_bug()). Pass @count = true at the
> + * primary suppression point to count each warning exactly once, and
> + * @count = false at secondary points to suppress output without
> + * inflating the count.
> + */
> +static inline bool kunit_is_suppressed_warning(bool count)
> +{
> + if (!static_branch_unlikely(&kunit_running))
> + return false;
> +
> + return kunit_hooks.is_suppressed_warning &&
> + kunit_hooks.is_suppressed_warning(count);
> +}
> +
> #else
>
> static inline struct kunit *kunit_get_current_test(void) { return NULL; }
> +static inline bool kunit_is_suppressed_warning(bool count) { return false; }
>
> #define kunit_fail_current_test(fmt, ...) do {} while (0)
>
> diff --git a/include/kunit/test.h b/include/kunit/test.h
> index 9cd1594ab697d..be71612f61655 100644
> --- a/include/kunit/test.h
> +++ b/include/kunit/test.h
> @@ -1795,4 +1795,102 @@ do { \
> // include resource.h themselves if they need it.
> #include <kunit/resource.h>
>
> +/*
> + * Warning backtrace suppression API.
> + *
> + * Suppresses WARN*() backtraces on the current task while active. Two forms
> + * are provided:
> + *
> + * - Scoped: kunit_warning_suppress(test) { ... }
> + * Suppression is active for the duration of the block. On normal exit,
> + * the for-loop increment deactivates suppression. On early exit (break,
> + * return, goto), the __cleanup attribute fires. On kthread_exit() (e.g.,
> + * a failed KUnit assertion), kunit_add_action() cleans up at test
> + * teardown. The suppression handle is only accessible inside the block,
> + * so warning counts must be checked before the block exits.
> + *
> + * - Direct: kunit_start_suppress_warning() / kunit_end_suppress_warning()
> + * The underlying functions, returning an explicit handle pointer. Use
> + * when the handle needs to be retained (e.g., for post-suppression
> + * count checks) or passed across helper functions.
> + */
> +struct kunit_suppressed_warning;
> +
> +struct kunit_suppressed_warning *
> +kunit_start_suppress_warning(struct kunit *test);
> +void kunit_end_suppress_warning(struct kunit *test,
> + struct kunit_suppressed_warning *w);
> +int kunit_suppressed_warning_count(struct kunit_suppressed_warning *w);
> +void __kunit_suppress_auto_cleanup(struct kunit_suppressed_warning **wp);
> +bool kunit_has_active_suppress_warning(void);
> +
> +/**
> + * kunit_warning_suppress() - Suppress WARN*() backtraces for the duration
> + * of a block.
> + * @test: The test context object.
> + *
> + * Scoped form of the suppression API. Suppression starts when the block is
> + * entered and ends automatically when the block exits through any path. See
> + * the section comment above for the cleanup guarantees on each exit path.
> + * Fails the test if suppression is already active; nesting is not supported.
> + *
> + * The warning count can be checked inside the block via
> + * KUNIT_EXPECT_SUPPRESSED_WARNING_COUNT(). The handle is not accessible
> + * after the block exits.
> + *
> + * Example::
> + *
> + * kunit_warning_suppress(test) {
> + * trigger_warning();
> + * KUNIT_EXPECT_SUPPRESSED_WARNING_COUNT(test, 1);
> + * }
> + */
> +#define kunit_warning_suppress(test) \
> + for (struct kunit_suppressed_warning *__kunit_suppress \
> + __cleanup(__kunit_suppress_auto_cleanup) = \
> + kunit_start_suppress_warning(test); \
> + __kunit_suppress; \
> + kunit_end_suppress_warning(test, __kunit_suppress), \
> + __kunit_suppress = NULL)
> +
> +/**
> + * KUNIT_SUPPRESSED_WARNING_COUNT() - Returns the suppressed warning count.
> + *
> + * Returns the number of WARN*() calls suppressed since the current
> + * suppression block started, or 0 if the handle is NULL. Usable inside a
> + * kunit_warning_suppress() block.
> + */
> +#define KUNIT_SUPPRESSED_WARNING_COUNT() \
> + kunit_suppressed_warning_count(__kunit_suppress)
> +
> +/**
> + * KUNIT_EXPECT_SUPPRESSED_WARNING_COUNT() - Sets an expectation that the
> + * suppressed warning count equals
> + * @expected.
> + * @test: The test context object.
> + * @expected: an expression that evaluates to the expected warning count.
> + *
> + * Sets an expectation that the number of suppressed WARN*() calls equals
> + * @expected. This is semantically equivalent to
> + * KUNIT_EXPECT_EQ(@test, KUNIT_SUPPRESSED_WARNING_COUNT(), @expected).
> + * See KUNIT_EXPECT_EQ() for more information.
> + */
> +#define KUNIT_EXPECT_SUPPRESSED_WARNING_COUNT(test, expected) \
> + KUNIT_EXPECT_EQ(test, KUNIT_SUPPRESSED_WARNING_COUNT(), expected)
> +
> +/**
> + * KUNIT_ASSERT_SUPPRESSED_WARNING_COUNT() - Sets an assertion that the
> + * suppressed warning count equals
> + * @expected.
> + * @test: The test context object.
> + * @expected: an expression that evaluates to the expected warning count.
> + *
> + * Sets an assertion that the number of suppressed WARN*() calls equals
> + * @expected. This is the same as KUNIT_EXPECT_SUPPRESSED_WARNING_COUNT(),
> + * except it causes an assertion failure (see KUNIT_ASSERT_TRUE()) when the
> + * assertion is not met.
> + */
> +#define KUNIT_ASSERT_SUPPRESSED_WARNING_COUNT(test, expected) \
> + KUNIT_ASSERT_EQ(test, KUNIT_SUPPRESSED_WARNING_COUNT(), expected)
> +
> #endif /* _KUNIT_TEST_H */
> diff --git a/kernel/panic.c b/kernel/panic.c
> index 20feada5319d4..213725b612aa1 100644
> --- a/kernel/panic.c
> +++ b/kernel/panic.c
> @@ -39,6 +39,7 @@
> #include <linux/sys_info.h>
> #include <trace/events/error_report.h>
> #include <asm/sections.h>
> +#include <kunit/test-bug.h>
>
> #define PANIC_TIMER_STEP 100
> #define PANIC_BLINK_SPD 18
> @@ -1124,6 +1125,11 @@ void warn_slowpath_fmt(const char *file, int line, unsigned taint,
> bool rcu = warn_rcu_enter();
> struct warn_args args;
>
> + if (kunit_is_suppressed_warning(true)) {
> + warn_rcu_exit(rcu);
> + return;
> + }
> +
> pr_warn(CUT_HERE);
>
> if (!fmt) {
> @@ -1146,6 +1152,11 @@ void __warn_printk(const char *fmt, ...)
> bool rcu = warn_rcu_enter();
> va_list args;
>
> + if (kunit_is_suppressed_warning(false)) {
> + warn_rcu_exit(rcu);
> + return;
> + }
> +
> pr_warn(CUT_HERE);
>
> va_start(args, fmt);
> diff --git a/lib/bug.c b/lib/bug.c
> index 224f4cfa4aa31..874cb4ae4d047 100644
> --- a/lib/bug.c
> +++ b/lib/bug.c
> @@ -48,6 +48,7 @@
> #include <linux/rculist.h>
> #include <linux/ftrace.h>
> #include <linux/context_tracking.h>
> +#include <kunit/test-bug.h>
>
> extern struct bug_entry __start___bug_table[], __stop___bug_table[];
>
> @@ -209,8 +210,6 @@ static enum bug_trap_type __report_bug(struct bug_entry *bug, unsigned long buga
> return BUG_TRAP_TYPE_NONE;
> }
>
> - disable_trace_on_warning();
> -
> bug_get_file_line(bug, &file, &line);
> fmt = bug_get_format(bug);
>
> @@ -220,6 +219,15 @@ static enum bug_trap_type __report_bug(struct bug_entry *bug, unsigned long buga
> no_cut = bug->flags & BUGFLAG_NO_CUT_HERE;
> has_args = bug->flags & BUGFLAG_ARGS;
>
> + /*
> + * Before the once logic so suppressed warnings do not consume
> + * the single-fire budget of WARN_ON_ONCE().
> + */
> + if (warning && kunit_is_suppressed_warning(true))
> + return BUG_TRAP_TYPE_WARN;
> +
> + disable_trace_on_warning();
> +
> if (warning && once) {
> if (done)
> return BUG_TRAP_TYPE_WARN;
> diff --git a/lib/kunit/Makefile b/lib/kunit/Makefile
> index 656f1fa35abcc..4592f9d0aa8dd 100644
> --- a/lib/kunit/Makefile
> +++ b/lib/kunit/Makefile
> @@ -10,7 +10,8 @@ kunit-objs += test.o \
> executor.o \
> attributes.o \
> device.o \
> - platform.o
> + platform.o \
> + bug.o
>
> ifeq ($(CONFIG_KUNIT_DEBUGFS),y)
> kunit-objs += debugfs.o
> diff --git a/lib/kunit/bug.c b/lib/kunit/bug.c
> new file mode 100644
> index 0000000000000..6752b497aeefe
> --- /dev/null
> +++ b/lib/kunit/bug.c
> @@ -0,0 +1,127 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * KUnit helpers for backtrace suppression
> + *
> + * Copyright (C) 2025 Alessandro Carminati <acarmina@redhat.com>
> + * Copyright (C) 2024 Guenter Roeck <linux@roeck-us.net>
> + */
> +
> +#include <kunit/resource.h>
> +#include <linux/export.h>
> +#include <linux/rculist.h>
> +#include <linux/sched.h>
> +#include <linux/spinlock.h>
> +
> +#include "hooks-impl.h"
> +
> +struct kunit_suppressed_warning {
> + struct list_head node;
> + struct task_struct *task;
> + struct kunit *test;
> + atomic_t counter;
> +};
> +
> +static LIST_HEAD(suppressed_warnings);
> +static DEFINE_SPINLOCK(suppressed_warnings_lock);
> +
> +static void kunit_suppress_warning_remove(struct kunit_suppressed_warning *w)
> +{
> + unsigned long flags;
> +
> + spin_lock_irqsave(&suppressed_warnings_lock, flags);
> + list_del_rcu(&w->node);
> + spin_unlock_irqrestore(&suppressed_warnings_lock, flags);
> + synchronize_rcu(); /* Wait for readers to finish */
> +}
> +
> +KUNIT_DEFINE_ACTION_WRAPPER(kunit_suppress_warning_cleanup,
> + kunit_suppress_warning_remove,
> + struct kunit_suppressed_warning *);
> +
> +bool kunit_has_active_suppress_warning(void)
> +{
> + return __kunit_is_suppressed_warning_impl(false);
> +}
> +EXPORT_SYMBOL_GPL(kunit_has_active_suppress_warning);
> +
> +struct kunit_suppressed_warning *
> +kunit_start_suppress_warning(struct kunit *test)
> +{
> + struct kunit_suppressed_warning *w;
> + unsigned long flags;
> + int ret;
> +
> + if (kunit_has_active_suppress_warning()) {
> + KUNIT_FAIL(test, "Another suppression block is already active");
> + return NULL;
> + }
> +
> + w = kunit_kzalloc(test, sizeof(*w), GFP_KERNEL);
> + if (!w) {
> + KUNIT_FAIL(test, "Failed to allocate suppression handle.");
> + return NULL;
> + }
> +
> + /*
> + * Store current without taking a reference. The test task cannot
> + * exit before kunit tears down the test, so the pointer is stable
> + * for the lifetime of this handle.
> + */
> + w->task = current;
> + w->test = test;
> +
> + spin_lock_irqsave(&suppressed_warnings_lock, flags);
> + list_add_rcu(&w->node, &suppressed_warnings);
> + spin_unlock_irqrestore(&suppressed_warnings_lock, flags);
> +
> + ret = kunit_add_action_or_reset(test,
> + kunit_suppress_warning_cleanup, w);
> + if (ret) {
> + KUNIT_FAIL(test, "Failed to add suppression cleanup action.");
> + return NULL;
> + }
> +
> + return w;
> +}
> +EXPORT_SYMBOL_GPL(kunit_start_suppress_warning);
> +
> +void kunit_end_suppress_warning(struct kunit *test,
> + struct kunit_suppressed_warning *w)
> +{
> + if (!w)
> + return;
> + kunit_release_action(test, kunit_suppress_warning_cleanup, w);
> +}
> +EXPORT_SYMBOL_GPL(kunit_end_suppress_warning);
> +
> +void __kunit_suppress_auto_cleanup(struct kunit_suppressed_warning **wp)
> +{
> + if (*wp)
> + kunit_end_suppress_warning((*wp)->test, *wp);
> +}
> +EXPORT_SYMBOL_GPL(__kunit_suppress_auto_cleanup);
> +
> +int kunit_suppressed_warning_count(struct kunit_suppressed_warning *w)
> +{
> + return w ? atomic_read(&w->counter) : 0;
> +}
> +EXPORT_SYMBOL_GPL(kunit_suppressed_warning_count);
> +
> +bool __kunit_is_suppressed_warning_impl(bool count)
> +{
> + struct kunit_suppressed_warning *w;
> +
> + if (!in_task())
> + return false;
> +
> + guard(rcu)();
> + list_for_each_entry_rcu(w, &suppressed_warnings, node) {
> + if (w->task == current) {
> + if (count)
> + atomic_inc(&w->counter);
> + return true;
> + }
> + }
> +
> + return false;
> +}
> diff --git a/lib/kunit/hooks-impl.h b/lib/kunit/hooks-impl.h
> index 4e71b2d0143ba..d8720f2616925 100644
> --- a/lib/kunit/hooks-impl.h
> +++ b/lib/kunit/hooks-impl.h
> @@ -19,6 +19,7 @@ void __printf(3, 4) __kunit_fail_current_test_impl(const char *file,
> int line,
> const char *fmt, ...);
> void *__kunit_get_static_stub_address_impl(struct kunit *test, void *real_fn_addr);
> +bool __kunit_is_suppressed_warning_impl(bool count);
>
> /* Code to set all of the function pointers. */
> static inline void kunit_install_hooks(void)
> @@ -26,6 +27,7 @@ static inline void kunit_install_hooks(void)
> /* Install the KUnit hook functions. */
> kunit_hooks.fail_current_test = __kunit_fail_current_test_impl;
> kunit_hooks.get_static_stub_address = __kunit_get_static_stub_address_impl;
> + kunit_hooks.is_suppressed_warning = __kunit_is_suppressed_warning_impl;
> }
>
> #endif /* _KUNIT_HOOKS_IMPL_H */
>
> --
> 2.53.0
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox