[PATCH v2] docs: submitting-patches: Clarify half-baked review tags

[PATCH v2] docs: submitting-patches: Clarify half-baked review tags

[Krzysztof Kozlowski]

Refactoring of driver subsystems or some core driver parts often come
with change of the actual drivers.  If that happens within one patch,
people giving "Reviewed-by" tag only to piece of the patch, like one
driver, and ignoring the rest including the main concept of the patch,
are not really reviewing "the patch".

As explained in the Submitting Patches document, the "Reviewer's
statement of oversight" is for patch as in "entire patch" meaning:

  (a) I have carried out a technical review of this patch to
      evaluate its appropriateness and readiness for inclusion into
      the mainline kernel.

The sentence is explicit and complete, meaning it does not allow
implicit variations like: "this patch or part of this patch".

Reviewing pieces of the patch while ignoring the main part of it, is
much closer to Acknowledgment, thus Ack, and "Reviewed-by" credit should
not be applicable.

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>

---

Changes in v2:
1. Few grammar fixes in text and commit msg.
2. Mention the "# subpart-name" marker solution.

Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Imagine a patch, doing massive refactoring of core SPI stuff, 1000 lines
of code, complex locking and also simultaneously updates drivers to the
new API changed in here.

Reviewing that 1000 lines of code in core SPI with complex locking takes
someone 30 minutes. He gives Rb tag, gets credit.

Now another person comes, looks at a SINGLE line in one driver he is
responsible for and gives Rb tag. For 10 seconds job of looking at one
changed call in one driver.
---
 Documentation/process/submitting-patches.rst | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/process/submitting-patches.rst b/Documentation/process/submitting-patches.rst
index f271d7413f72..678681b5c9c7 100644
--- a/Documentation/process/submitting-patches.rst
+++ b/Documentation/process/submitting-patches.rst
@@ -585,6 +585,11 @@ 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.
+If the reviewer checks only a piece of the patch, leaving the rest unreviewed,
+then more appropriate is Acked-by tag or adding a "# subpart-name" marker
+after the Reviewed-by tag:
+
+	Reviewed-by: I Did Not Read Everything <foo@example.org> # bar, baz
 
 Both Tested-by and Reviewed-by tags, once received on mailing list from tester
 or reviewer, should be added by author to the applicable patches when sending
-- 
2.53.0

Re: [PATCH v2] docs: submitting-patches: Clarify half-baked review tags

[Wolfram Sang]

> +If the reviewer checks only a piece of the patch, leaving the rest unreviewed,
> +then more appropriate is Acked-by tag or adding a "# subpart-name" marker

"... is an Acked-by ..."?

> +after the Reviewed-by tag:
> +
> +	Reviewed-by: I Did Not Read Everything <foo@example.org> # bar, baz

How about being more specific?

 +	Reviewed-by: Driver X author <foo@example.org> # for Driver X

TBH, I think the above is even better than a generic "Acked-by" which is
basically "Yeah, I didn't look in detail but the general approach looks
fine". Which is not what the above says. The above says: "I didn't check
the general approach, but assuming it is okay, the changes to my driver
are correct". If I get a couple of those, then I know which parts have
been reviewed and which have not. A list of generic Acked-bys will not
provide that information. So, actually, I think even the Acked-by should
have the "# subpart name" once only a part of the patch has been looked
at. Ack/Rev is then describing the thoroughness of that look.

Do you get what I mean?

Re: [PATCH v2] docs: submitting-patches: Clarify half-baked review tags

[Krzysztof Kozlowski]

On 05/11/2026 09:50, Wolfram Sang wrote:
> 
>> +If the reviewer checks only a piece of the patch, leaving the rest unreviewed,
>> +then more appropriate is Acked-by tag or adding a "# subpart-name" marker
> 
> "... is an Acked-by ..."?

ack

> 
>> +after the Reviewed-by tag:
>> +
>> +	Reviewed-by: I Did Not Read Everything <foo@example.org> # bar, baz
> 
> How about being more specific?
> 
>  +	Reviewed-by: Driver X author <foo@example.org> # for Driver X
> 

ack

> TBH, I think the above is even better than a generic "Acked-by" which is
> basically "Yeah, I didn't look in detail but the general approach looks
> fine". Which is not what the above says. The above says: "I didn't check
> the general approach, but assuming it is okay, the changes to my driver
> are correct". If I get a couple of those, then I know which parts have
> been reviewed and which have not. A list of generic Acked-bys will not
> provide that information. So, actually, I think even the Acked-by should
> have the "# subpart name" once only a part of the patch has been looked
> at. Ack/Rev is then describing the thoroughness of that look.
> 
> Do you get what I mean?

Yes. Part of the Acked-by tag is actually unspecified Acknowledgment and
many maintainers do not trail it with "# subpart" name. It's well
accepted and known in our community that Acked-by can be for everything
or for Maintainer's responsible area, implied by MAINTAINERS file.

Reviewed-by even with the "# subpart" still appears in person's credits
and reviewing entire patch of 700 lines of diff code (like Frank's SPI
patch) is not the same as 10 diff codes for one driver.

I'll send later v3 with the changes from above.

Best regards,
Krzysztof

Re: [PATCH v2] docs: submitting-patches: Clarify half-baked review tags

[Wolfram Sang]

> Reviewed-by even with the "# subpart" still appears in person's credits
> and reviewing entire patch of 700 lines of diff code (like Frank's SPI
> patch) is not the same as 10 diff codes for one driver.

Yes, if you want to spoil that, you'd need to introduce

	Partly-reviewed-by: ...

or something.

Re: [PATCH v2] docs: submitting-patches: Clarify half-baked review tags

[Krzysztof Kozlowski]

On 05/11/2026 10:06, Wolfram Sang wrote:
> 
>> Reviewed-by even with the "# subpart" still appears in person's credits
>> and reviewing entire patch of 700 lines of diff code (like Frank's SPI
>> patch) is not the same as 10 diff codes for one driver.
> 
> Yes, if you want to spoil that, you'd need to introduce
> 
> 	Partly-reviewed-by: ...
> 
> or something.

Ack is for that, no need for a new tag, quoting:

"Acked-by.... user may not have carried out a technical review of the
patch, yet they may be satisfied ..."

"Reviewed-by:, instead, indicates that *the patch* has been reviewed and
found acceptable according to the Reviewer's Statement:"

Note that there is little sense in reviewing - as in statement of
oversight credit review - pieces of patch in context I discuss here,
e.g. one big complex rework combined with updates of drivers. Who cares
about the drivers? The complex rework is important and should be
reviewed, but people skip bigger picture and put their stamp of approval.

Stamp of approval is Ack, no need to mix it with reviewer's statement of
oversight.

Best regards,
Krzysztof

Re: [PATCH v2] docs: submitting-patches: Clarify half-baked review tags

[Andy Shevchenko]

On Thu, Nov 05, 2026 at 09:27:30AM +0100, Krzysztof Kozlowski wrote:
> Refactoring of driver subsystems or some core driver parts often come
> with change of the actual drivers.  If that happens within one patch,
> people giving "Reviewed-by" tag only to piece of the patch, like one
> driver, and ignoring the rest including the main concept of the patch,
> are not really reviewing "the patch".
> 
> As explained in the Submitting Patches document, the "Reviewer's
> statement of oversight" is for patch as in "entire patch" meaning:
> 
>   (a) I have carried out a technical review of this patch to
>       evaluate its appropriateness and readiness for inclusion into
>       the mainline kernel.
> 
> The sentence is explicit and complete, meaning it does not allow
> implicit variations like: "this patch or part of this patch".
> 
> Reviewing pieces of the patch while ignoring the main part of it, is
> much closer to Acknowledgment, thus Ack, and "Reviewed-by" credit should
> not be applicable.

I strongly disagree. It's all explained in the plenty threads you have started
on the Subject.

TL;DR: As a reviewer for the piece in question I have done my part fully. It's
not my issue that the piece is bundled into a single patch. We have options:
1) ask to split (and review as standalone piece);
2) add (partial) Rb with the pointers of what exactly has been reviewed;
3) ignore (silent agreement with the respective subsystem maintainer's opinion);
4) ...something I forgot?..

(even partial) Rb != Ack.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] docs: submitting-patches: Clarify half-baked review tags

[Andy Shevchenko]

On Thu, Nov 05, 2026 at 10:12:00AM +0100, Krzysztof Kozlowski wrote:
> On 05/11/2026 10:06, Wolfram Sang wrote:
> > 
> >> Reviewed-by even with the "# subpart" still appears in person's credits
> >> and reviewing entire patch of 700 lines of diff code (like Frank's SPI
> >> patch) is not the same as 10 diff codes for one driver.
> > 
> > Yes, if you want to spoil that, you'd need to introduce
> > 
> > 	Partly-reviewed-by: ...
> > 
> > or something.
> 
> Ack is for that, no need for a new tag, quoting:
> 
> "Acked-by.... user may not have carried out a technical review of the
> patch, yet they may be satisfied ..."
> 
> "Reviewed-by:, instead, indicates that *the patch* has been reviewed and
> found acceptable according to the Reviewer's Statement:"
> 
> Note that there is little sense in reviewing - as in statement of
> oversight credit review - pieces of patch in context I discuss here,
> e.g. one big complex rework combined with updates of drivers. Who cares
> about the drivers? The complex rework is important and should be
> reviewed, but people skip bigger picture and put their stamp of approval.

Nice, and what as a particular piece maintainer I should do if somebody
created some helper somewhere (not Cc'ing me), waited to a cycle and started
cleaning up or refactor the drivers based on that API? Basically I was stand
in front of a fact, I may tell NAK to that, but what should we do if half of
the kernel will use new API, half will use old one due to disagreements on
implementation, naming, et cetera? Rb in this sense tells that I have reviewed
the refactoring part that has been done on a piece that I'm in charge for and
it looks good. Breaking the flow or behaviour might have been done on the
upper layer for a bigger number of users.

> Stamp of approval is Ack, no need to mix it with reviewer's statement of
> oversight.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] docs: submitting-patches: Clarify half-baked review tags

[Krzysztof Kozlowski]

On 05/11/2026 10:19, Andy Shevchenko wrote:
> On Thu, Nov 05, 2026 at 09:27:30AM +0100, Krzysztof Kozlowski wrote:
>> Refactoring of driver subsystems or some core driver parts often come
>> with change of the actual drivers.  If that happens within one patch,
>> people giving "Reviewed-by" tag only to piece of the patch, like one
>> driver, and ignoring the rest including the main concept of the patch,
>> are not really reviewing "the patch".
>>
>> As explained in the Submitting Patches document, the "Reviewer's
>> statement of oversight" is for patch as in "entire patch" meaning:
>>
>>   (a) I have carried out a technical review of this patch to
>>       evaluate its appropriateness and readiness for inclusion into
>>       the mainline kernel.
>>
>> The sentence is explicit and complete, meaning it does not allow
>> implicit variations like: "this patch or part of this patch".
>>
>> Reviewing pieces of the patch while ignoring the main part of it, is
>> much closer to Acknowledgment, thus Ack, and "Reviewed-by" credit should
>> not be applicable.
> 
> I strongly disagree. It's all explained in the plenty threads you have started
> on the Subject.

Please provide arguments. You keep responding "No" or "Disagree", yet yo
do not give any technical, discussible data.

You just wrote:
https://lore.kernel.org/all/auxGom0XR-Hax9b3@ashevche-desk.local/

"No, (partial) Rb != Ack."

to which I provided several quotes backing my point of view:
https://lore.kernel.org/all/00390a20-7630-4066-9779-06c8f915a176@kernel.org/


> 
> TL;DR: As a reviewer for the piece in question I have done my part fully. It's
> not my issue that the piece is bundled into a single patch. We have options:
> 1) ask to split (and review as standalone piece);
> 2) add (partial) Rb with the pointers of what exactly has been reviewed;
> 3) ignore (silent agreement with the respective subsystem maintainer's opinion);
> 4) ...something I forgot?..
> 
> (even partial) Rb != Ack.

oh, here you repeat.

There is no partial review tag. Please read arguments I already provided
to you and either respond with counter arguments or just stop repeating
empty statements.

Best regards,
Krzysztof

Re: [PATCH v2] docs: submitting-patches: Clarify half-baked review tags

[Wolfram Sang]

> > Yes, if you want to spoil that, you'd need to introduce
> > 
> > 	Partly-reviewed-by: ...
> > 
> > or something.
> 
> Ack is for that, no need for a new tag, quoting:
> 
> "Acked-by.... user may not have carried out a technical review of the
> patch, yet they may be satisfied ..."

It seems you are mainly aiming about the "Ack/Rev"-distinction problem.
I don't care so much about this. I care more about the "# subpart"
problem. A list of 10 Acks without subpart is to me as useless as 10
Revs without subpart. The subparts give me the idea which parts of the
patch have been looked at in some way, at least.

> "Reviewed-by:, instead, indicates that *the patch* has been reviewed and
> found acceptable according to the Reviewer's Statement:"

So, for the SPI patch: if someone spent time and reviewed the complex
core changes, but only 2 out of dozens of drivers, this would be an Ack
according to you. I would, however, understand that this person doesn't
want to repeat the boring driver review a number of times. I'd happily
accept an:

	Reviewed-by: <hero@reviwers.org> # for the core parts

But still, the driver parts need review. We got a similar patch for I2C
and 4 drivers out of those needed some cosmetic improvements. I reviewed
that. But I did not review the changes in the dmaengine core which lead
to the driver-fixes patch.

Re: [PATCH v2] docs: submitting-patches: Clarify half-baked review tags

[Geert Uytterhoeven]

On Thu, 5 Nov 2026 at 10:09, Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> > Reviewed-by even with the "# subpart" still appears in person's credits
> > and reviewing entire patch of 700 lines of diff code (like Frank's SPI
> > patch) is not the same as 10 diff codes for one driver.
>
> Yes, if you want to spoil that, you'd need to introduce
>
>         Partly-reviewed-by: ...
>
> or something.

With a parameter taking a percentage or number of files or lines,
to serve as input for the Big Statistics Calculation?

Doh...

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2] docs: submitting-patches: Clarify half-baked review tags

[Jürgen Groß]

[-- Attachment #1.1: Type: text/plain, Size: 2148 bytes --]

On 05.11.26 10:19, Andy Shevchenko wrote:
> On Thu, Nov 05, 2026 at 09:27:30AM +0100, Krzysztof Kozlowski wrote:
>> Refactoring of driver subsystems or some core driver parts often come
>> with change of the actual drivers.  If that happens within one patch,
>> people giving "Reviewed-by" tag only to piece of the patch, like one
>> driver, and ignoring the rest including the main concept of the patch,
>> are not really reviewing "the patch".
>>
>> As explained in the Submitting Patches document, the "Reviewer's
>> statement of oversight" is for patch as in "entire patch" meaning:
>>
>>    (a) I have carried out a technical review of this patch to
>>        evaluate its appropriateness and readiness for inclusion into
>>        the mainline kernel.
>>
>> The sentence is explicit and complete, meaning it does not allow
>> implicit variations like: "this patch or part of this patch".
>>
>> Reviewing pieces of the patch while ignoring the main part of it, is
>> much closer to Acknowledgment, thus Ack, and "Reviewed-by" credit should
>> not be applicable.
> 
> I strongly disagree. It's all explained in the plenty threads you have started
> on the Subject.
> 
> TL;DR: As a reviewer for the piece in question I have done my part fully. It's
> not my issue that the piece is bundled into a single patch. We have options:
> 1) ask to split (and review as standalone piece);
> 2) add (partial) Rb with the pointers of what exactly has been reviewed;
> 3) ignore (silent agreement with the respective subsystem maintainer's opinion);
> 4) ...something I forgot?..
> 
> (even partial) Rb != Ack.
> 

I fully agree with Andy.

As the Xen maintainer I often have to look into such rework patches and often
enough especially the Xen specific parts are mentioned in the patch description
as "compile tested only".

I see a "Reviewed-by: ... # Xen parts" in such cases as a feedback to the author
or committer that they don't have to look into those parts in more detail.

The value of such a R-b: is more questionable in case of a pure rename of a
function or struct member, of course.


Juergen

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH v2] docs: submitting-patches: Clarify half-baked review tags

[Wolfram Sang]

> > > Reviewed-by even with the "# subpart" still appears in person's credits
> > > and reviewing entire patch of 700 lines of diff code (like Frank's SPI
> > > patch) is not the same as 10 diff codes for one driver.
> >
> > Yes, if you want to spoil that, you'd need to introduce
> >
> >         Partly-reviewed-by: ...
> >
> > or something.
> 
> With a parameter taking a percentage or number of files or lines,
> to serve as input for the Big Statistics Calculation?

I assume Krzysztof addresses people playing number games. While I do
understand this motivation to raise the bar, I also know people will
find ways to jump over the higher bar etc...

Quoting Joshua/WOPR: "A STRANGE GAME. THE ONLY WINNING MOVE IS NOT TO
PLAY"

Maybe we should stop publishing development statistics? ;)

Seriously, the main thing I am aiming at is to improve code maintenance.
And so, the "# subparts" are much more interesting to me.

Re: [PATCH v2] docs: submitting-patches: Clarify half-baked review tags

[Andy Shevchenko]

On Thu, Nov 05, 2026 at 11:44:47AM +0200, Wolfram Sang wrote:
> 
> > > > Reviewed-by even with the "# subpart" still appears in person's credits
> > > > and reviewing entire patch of 700 lines of diff code (like Frank's SPI
> > > > patch) is not the same as 10 diff codes for one driver.
> > >
> > > Yes, if you want to spoil that, you'd need to introduce
> > >
> > >         Partly-reviewed-by: ...
> > >
> > > or something.
> > 
> > With a parameter taking a percentage or number of files or lines,
> > to serve as input for the Big Statistics Calculation?
> 
> I assume Krzysztof addresses people playing number games. While I do
> understand this motivation to raise the bar, I also know people will
> find ways to jump over the higher bar etc...
> 
> Quoting Joshua/WOPR: "A STRANGE GAME. THE ONLY WINNING MOVE IS NOT TO
> PLAY"
> 
> Maybe we should stop publishing development statistics? ;)
> 
> Seriously, the main thing I am aiming at is to improve code maintenance.
> And so, the "# subparts" are much more interesting to me.

Yep, we need tools to support that!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] docs: submitting-patches: Clarify half-baked review tags

[Andy Shevchenko]

On Thu, Nov 05, 2026 at 10:27:17AM +0100, Krzysztof Kozlowski wrote:
> On 05/11/2026 10:19, Andy Shevchenko wrote:
> > On Thu, Nov 05, 2026 at 09:27:30AM +0100, Krzysztof Kozlowski wrote:
> >> Refactoring of driver subsystems or some core driver parts often come
> >> with change of the actual drivers.  If that happens within one patch,
> >> people giving "Reviewed-by" tag only to piece of the patch, like one
> >> driver, and ignoring the rest including the main concept of the patch,
> >> are not really reviewing "the patch".
> >>
> >> As explained in the Submitting Patches document, the "Reviewer's
> >> statement of oversight" is for patch as in "entire patch" meaning:
> >>
> >>   (a) I have carried out a technical review of this patch to
> >>       evaluate its appropriateness and readiness for inclusion into
> >>       the mainline kernel.
> >>
> >> The sentence is explicit and complete, meaning it does not allow
> >> implicit variations like: "this patch or part of this patch".
> >>
> >> Reviewing pieces of the patch while ignoring the main part of it, is
> >> much closer to Acknowledgment, thus Ack, and "Reviewed-by" credit should
> >> not be applicable.
> > 
> > I strongly disagree. It's all explained in the plenty threads you have started
> > on the Subject.
> 
> Please provide arguments. You keep responding "No" or "Disagree", yet yo
> do not give any technical, discussible data.
> 
> You just wrote:
> https://lore.kernel.org/all/auxGom0XR-Hax9b3@ashevche-desk.local/
> 
> "No, (partial) Rb != Ack."
> 
> to which I provided several quotes backing my point of view:
> https://lore.kernel.org/all/00390a20-7630-4066-9779-06c8f915a176@kernel.org/
> 
> > TL;DR: As a reviewer for the piece in question I have done my part fully. It's
> > not my issue that the piece is bundled into a single patch. We have options:

^^^ (1)

> > 1) ask to split (and review as standalone piece);
> > 2) add (partial) Rb with the pointers of what exactly has been reviewed;
> > 3) ignore (silent agreement with the respective subsystem maintainer's opinion);
> > 4) ...something I forgot?..
> > 
> > (even partial) Rb != Ack.
> 
> oh, here you repeat.
> 
> There is no partial review tag. Please read arguments I already provided
> to you and either respond with counter arguments or just stop repeating
> empty statements.

I already provided you the argument, you seems missed it (see 1 ^^^).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] docs: submitting-patches: Clarify half-baked review tags

[Krzysztof Kozlowski]

On 05/11/2026 10:48, Andy Shevchenko wrote:
> On Thu, Nov 05, 2026 at 10:27:17AM +0100, Krzysztof Kozlowski wrote:
>> On 05/11/2026 10:19, Andy Shevchenko wrote:
>>> On Thu, Nov 05, 2026 at 09:27:30AM +0100, Krzysztof Kozlowski wrote:
>>>> Refactoring of driver subsystems or some core driver parts often come
>>>> with change of the actual drivers.  If that happens within one patch,
>>>> people giving "Reviewed-by" tag only to piece of the patch, like one
>>>> driver, and ignoring the rest including the main concept of the patch,
>>>> are not really reviewing "the patch".
>>>>
>>>> As explained in the Submitting Patches document, the "Reviewer's
>>>> statement of oversight" is for patch as in "entire patch" meaning:
>>>>
>>>>   (a) I have carried out a technical review of this patch to
>>>>       evaluate its appropriateness and readiness for inclusion into
>>>>       the mainline kernel.
>>>>
>>>> The sentence is explicit and complete, meaning it does not allow
>>>> implicit variations like: "this patch or part of this patch".
>>>>
>>>> Reviewing pieces of the patch while ignoring the main part of it, is
>>>> much closer to Acknowledgment, thus Ack, and "Reviewed-by" credit should
>>>> not be applicable.
>>>
>>> I strongly disagree. It's all explained in the plenty threads you have started
>>> on the Subject.
>>
>> Please provide arguments. You keep responding "No" or "Disagree", yet yo
>> do not give any technical, discussible data.
>>
>> You just wrote:
>> https://lore.kernel.org/all/auxGom0XR-Hax9b3@ashevche-desk.local/
>>
>> "No, (partial) Rb != Ack."
>>
>> to which I provided several quotes backing my point of view:
>> https://lore.kernel.org/all/00390a20-7630-4066-9779-06c8f915a176@kernel.org/
>>
>>> TL;DR: As a reviewer for the piece in question I have done my part fully. It's
>>> not my issue that the piece is bundled into a single patch. We have options:
> 
> ^^^ (1)
> 
>>> 1) ask to split (and review as standalone piece);
>>> 2) add (partial) Rb with the pointers of what exactly has been reviewed;
>>> 3) ignore (silent agreement with the respective subsystem maintainer's opinion);
>>> 4) ...something I forgot?..
>>>
>>> (even partial) Rb != Ack.
>>
>> oh, here you repeat.
>>
>> There is no partial review tag. Please read arguments I already provided
>> to you and either respond with counter arguments or just stop repeating
>> empty statements.
> 
> I already provided you the argument, you seems missed it (see 1 ^^^).

The (1) above does not address that statement of oversight does not
include possibility of partial review.

Best regards,
Krzysztof

Re: [PATCH v2] docs: submitting-patches: Clarify half-baked review tags

[Krzysztof Kozlowski]

On 05/11/2026 10:26, Andy Shevchenko wrote:
> On Thu, Nov 05, 2026 at 10:12:00AM +0100, Krzysztof Kozlowski wrote:
>> On 05/11/2026 10:06, Wolfram Sang wrote:
>>>
>>>> Reviewed-by even with the "# subpart" still appears in person's credits
>>>> and reviewing entire patch of 700 lines of diff code (like Frank's SPI
>>>> patch) is not the same as 10 diff codes for one driver.
>>>
>>> Yes, if you want to spoil that, you'd need to introduce
>>>
>>> 	Partly-reviewed-by: ...
>>>
>>> or something.
>>
>> Ack is for that, no need for a new tag, quoting:
>>
>> "Acked-by.... user may not have carried out a technical review of the
>> patch, yet they may be satisfied ..."
>>
>> "Reviewed-by:, instead, indicates that *the patch* has been reviewed and
>> found acceptable according to the Reviewer's Statement:"
>>
>> Note that there is little sense in reviewing - as in statement of
>> oversight credit review - pieces of patch in context I discuss here,
>> e.g. one big complex rework combined with updates of drivers. Who cares
>> about the drivers? The complex rework is important and should be
>> reviewed, but people skip bigger picture and put their stamp of approval.
> 
> Nice, and what as a particular piece maintainer I should do if somebody
> created some helper somewhere (not Cc'ing me), waited to a cycle and started
> cleaning up or refactor the drivers based on that API? Basically I was stand
> in front of a fact, I may tell NAK to that, but what should we do if half of
> the kernel will use new API, half will use old one due to disagreements on
> implementation, naming, et cetera? Rb in this sense tells that I have reviewed
> the refactoring part that has been done on a piece that I'm in charge for and

Do the review, no one questions that.

Statement of oversight, thus tag, how expressed in the docs applies
though to entire patch.

> it looks good. Breaking the flow or behaviour might have been done on the
> upper layer for a bigger number of users.

Best regards,
Krzysztof

Re: [PATCH v2] docs: submitting-patches: Clarify half-baked review tags

[Krzysztof Kozlowski]

On 05/11/2026 10:28, Wolfram Sang wrote:
> 
>>> Yes, if you want to spoil that, you'd need to introduce
>>>
>>> 	Partly-reviewed-by: ...
>>>
>>> or something.
>>
>> Ack is for that, no need for a new tag, quoting:
>>
>> "Acked-by.... user may not have carried out a technical review of the
>> patch, yet they may be satisfied ..."
> 
> It seems you are mainly aiming about the "Ack/Rev"-distinction problem.
> I don't care so much about this. I care more about the "# subpart"
> problem. A list of 10 Acks without subpart is to me as useless as 10
> Revs without subpart. The subparts give me the idea which parts of the
> patch have been looked at in some way, at least.

Sure, let's encourage that. I do not oppose "# subpart" usage as you can
see in this version of patch. So far it was simply implied. We know many
names of maintainers so when Mark acks patch under regulator part of
files, we know he looked and acked at regulator part.

> 
>> "Reviewed-by:, instead, indicates that *the patch* has been reviewed and
>> found acceptable according to the Reviewer's Statement:"
> 
> So, for the SPI patch: if someone spent time and reviewed the complex
> core changes, but only 2 out of dozens of drivers, this would be an Ack
> according to you. I would, however, understand that this person doesn't

No, and I explained this in this commit msg. That person reviewed the
MAIN concept of the patch.

"Reviewing pieces of the patch while ignoring the main part of it, is
much closer to Acknowledgment, thus Ack, and "Reviewed-by" credit should
not be applicable."


> want to repeat the boring driver review a number of times. I'd happily
> accept an:
> 
> 	Reviewed-by: <hero@reviwers.org> # for the core parts
> 
> But still, the driver parts need review. We got a similar patch for I2C
> and 4 drivers out of those needed some cosmetic improvements. I reviewed
> that. But I did not review the changes in the dmaengine core which lead
> to the driver-fixes patch.

Nowhere in my patch here I said you CANNOT review part of the patch or
cannot even leave Rb tag for such case, right? What are we discussing
now, exactly?

Do you agree that if person skips the main point of the patch, the hard
part, and reviews only less important part, that looks more as an Ack,
than Rb, considering how "Reviewer's statement of oversight" is worded?


Best regards,
Krzysztof

Re: [PATCH v2] docs: submitting-patches: Clarify half-baked review tags

[Wolfram Sang]

> >> "Reviewed-by:, instead, indicates that *the patch* has been reviewed and
> >> found acceptable according to the Reviewer's Statement:"
> > 
> > So, for the SPI patch: if someone spent time and reviewed the complex
> > core changes, but only 2 out of dozens of drivers, this would be an Ack
> > according to you. I would, however, understand that this person doesn't
> 
> No, and I explained this in this commit msg. That person reviewed the
> MAIN concept of the patch.

With that MAIN concept emphasized, I am okay with your reasoning and
agree. Before (I cite, upmost paragraph) "*the patch*" could be
interpreted as 100% of the patch (including dumb driver conversions).

> "Reviewing pieces of the patch while ignoring the main part of it, is
> much closer to Acknowledgment, thus Ack, and "Reviewed-by" credit should
> not be applicable."

I agree. As stated before, I don't care so much. I am glad you do :)
I'll happily take the "# subpart" potion of your patch here.

> Nowhere in my patch here I said you CANNOT review part of the patch or
> cannot even leave Rb tag for such case, right? What are we discussing
> now, exactly?

I hope the misunderstanding has been resolved in the first paragraph.

> Do you agree that if person skips the main point of the patch, the hard
> part, and reviews only less important part, that looks more as an Ack,
> than Rb, considering how "Reviewer's statement of oversight" is worded?

Sure. It is not so important to me, but yes, I agree.

Re: [PATCH v2] docs: submitting-patches: Clarify half-baked review tags

[Andy Shevchenko]

On Thu, Nov 05, 2026 at 01:19:14PM +0200, Wolfram Sang wrote:
> 
> > >> "Reviewed-by:, instead, indicates that *the patch* has been reviewed and
> > >> found acceptable according to the Reviewer's Statement:"
> > > 
> > > So, for the SPI patch: if someone spent time and reviewed the complex
> > > core changes, but only 2 out of dozens of drivers, this would be an Ack
> > > according to you. I would, however, understand that this person doesn't
> > 
> > No, and I explained this in this commit msg. That person reviewed the
> > MAIN concept of the patch.
> 
> With that MAIN concept emphasized, I am okay with your reasoning and
> agree. Before (I cite, upmost paragraph) "*the patch*" could be
> interpreted as 100% of the patch (including dumb driver conversions).

...which may lead to discouragement of reviewing such a patch.
At least that's how I read the point.

> > "Reviewing pieces of the patch while ignoring the main part of it, is
> > much closer to Acknowledgment, thus Ack, and "Reviewed-by" credit should
> > not be applicable."
> 
> I agree. As stated before, I don't care so much. I am glad you do :)
> I'll happily take the "# subpart" potion of your patch here.
> 
> > Nowhere in my patch here I said you CANNOT review part of the patch or
> > cannot even leave Rb tag for such case, right? What are we discussing
> > now, exactly?
> 
> I hope the misunderstanding has been resolved in the first paragraph.
> 
> > Do you agree that if person skips the main point of the patch, the hard
> > part, and reviews only less important part, that looks more as an Ack,
> > than Rb, considering how "Reviewer's statement of oversight" is worded?
> 
> Sure. It is not so important to me, but yes, I agree.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] docs: submitting-patches: Clarify half-baked review tags

[Krzysztof Kozlowski]

On 05/11/2026 12:53, Andy Shevchenko wrote:
> On Thu, Nov 05, 2026 at 01:19:14PM +0200, Wolfram Sang wrote:
>>
>>>>> "Reviewed-by:, instead, indicates that *the patch* has been reviewed and
>>>>> found acceptable according to the Reviewer's Statement:"
>>>>
>>>> So, for the SPI patch: if someone spent time and reviewed the complex
>>>> core changes, but only 2 out of dozens of drivers, this would be an Ack
>>>> according to you. I would, however, understand that this person doesn't
>>>
>>> No, and I explained this in this commit msg. That person reviewed the
>>> MAIN concept of the patch.
>>
>> With that MAIN concept emphasized, I am okay with your reasoning and
>> agree. Before (I cite, upmost paragraph) "*the patch*" could be
>> interpreted as 100% of the patch (including dumb driver conversions).
> 
> ...which may lead to discouragement of reviewing such a patch.
> At least that's how I read the point.

Why Ack instead of Review would discourage? Are you saying people give
Reviewed-by tags for incomplete reviews only because they want the credits?

Best regards,
Krzysztof

Re: [PATCH v2] docs: submitting-patches: Clarify half-baked review tags

[Andy Shevchenko]

On Thu, Nov 05, 2026 at 01:56:10PM +0100, Krzysztof Kozlowski wrote:
> On 05/11/2026 12:53, Andy Shevchenko wrote:
> > On Thu, Nov 05, 2026 at 01:19:14PM +0200, Wolfram Sang wrote:
> >>
> >>>>> "Reviewed-by:, instead, indicates that *the patch* has been reviewed and
> >>>>> found acceptable according to the Reviewer's Statement:"
> >>>>
> >>>> So, for the SPI patch: if someone spent time and reviewed the complex
> >>>> core changes, but only 2 out of dozens of drivers, this would be an Ack
> >>>> according to you. I would, however, understand that this person doesn't
> >>>
> >>> No, and I explained this in this commit msg. That person reviewed the
> >>> MAIN concept of the patch.
> >>
> >> With that MAIN concept emphasized, I am okay with your reasoning and
> >> agree. Before (I cite, upmost paragraph) "*the patch*" could be
> >> interpreted as 100% of the patch (including dumb driver conversions).
> > 
> > ...which may lead to discouragement of reviewing such a patch.
> > At least that's how I read the point.
> 
> Why Ack instead of Review would discourage?

Because Ack doesn't imply *any* review.

> Are you saying people give Reviewed-by tags for incomplete reviews only because they want the credits?

No. We seem to discuss different issues. You are talking about the credits,
and I'm talking about the fact of the review versus acknowledgement.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] docs: submitting-patches: Clarify half-baked review tags

[Krzysztof Kozlowski]

On 05/11/2026 10:30, Jürgen Groß wrote:
> On 05.11.26 10:19, Andy Shevchenko wrote:
>> On Thu, Nov 05, 2026 at 09:27:30AM +0100, Krzysztof Kozlowski wrote:
>>> Refactoring of driver subsystems or some core driver parts often come
>>> with change of the actual drivers.  If that happens within one patch,
>>> people giving "Reviewed-by" tag only to piece of the patch, like one
>>> driver, and ignoring the rest including the main concept of the patch,
>>> are not really reviewing "the patch".
>>>
>>> As explained in the Submitting Patches document, the "Reviewer's
>>> statement of oversight" is for patch as in "entire patch" meaning:
>>>
>>>    (a) I have carried out a technical review of this patch to
>>>        evaluate its appropriateness and readiness for inclusion into
>>>        the mainline kernel.
>>>
>>> The sentence is explicit and complete, meaning it does not allow
>>> implicit variations like: "this patch or part of this patch".
>>>
>>> Reviewing pieces of the patch while ignoring the main part of it, is
>>> much closer to Acknowledgment, thus Ack, and "Reviewed-by" credit should
>>> not be applicable.
>>
>> I strongly disagree. It's all explained in the plenty threads you have started
>> on the Subject.
>>
>> TL;DR: As a reviewer for the piece in question I have done my part fully. It's
>> not my issue that the piece is bundled into a single patch. We have options:
>> 1) ask to split (and review as standalone piece);
>> 2) add (partial) Rb with the pointers of what exactly has been reviewed;
>> 3) ignore (silent agreement with the respective subsystem maintainer's opinion);
>> 4) ...something I forgot?..
>>
>> (even partial) Rb != Ack.
>>
> 
> I fully agree with Andy.
> 
> As the Xen maintainer I often have to look into such rework patches and often
> enough especially the Xen specific parts are mentioned in the patch description
> as "compile tested only".
> 
> I see a "Reviewed-by: ... # Xen parts" in such cases as a feedback to the author
> or committer that they don't have to look into those parts in more detail.
> 
> The value of such a R-b: is more questionable in case of a pure rename of a
> function or struct member, of course.
>

So you don't agree with "Reviewed-by: # subpart"? Sorry, but I have
troubles pointing your disagreement arguments to actual patch content.

Best regards,
Krzysztof

Re: [PATCH v2] docs: submitting-patches: Clarify half-baked review tags

[Juergen Gross]

[-- Attachment #1.1: Type: text/plain, Size: 2691 bytes --]

On 05.11.26 16:33, Krzysztof Kozlowski wrote:
> On 05/11/2026 10:30, Jürgen Groß wrote:
>> On 05.11.26 10:19, Andy Shevchenko wrote:
>>> On Thu, Nov 05, 2026 at 09:27:30AM +0100, Krzysztof Kozlowski wrote:
>>>> Refactoring of driver subsystems or some core driver parts often come
>>>> with change of the actual drivers.  If that happens within one patch,
>>>> people giving "Reviewed-by" tag only to piece of the patch, like one
>>>> driver, and ignoring the rest including the main concept of the patch,
>>>> are not really reviewing "the patch".
>>>>
>>>> As explained in the Submitting Patches document, the "Reviewer's
>>>> statement of oversight" is for patch as in "entire patch" meaning:
>>>>
>>>>     (a) I have carried out a technical review of this patch to
>>>>         evaluate its appropriateness and readiness for inclusion into
>>>>         the mainline kernel.
>>>>
>>>> The sentence is explicit and complete, meaning it does not allow
>>>> implicit variations like: "this patch or part of this patch".
>>>>
>>>> Reviewing pieces of the patch while ignoring the main part of it, is
>>>> much closer to Acknowledgment, thus Ack, and "Reviewed-by" credit should
>>>> not be applicable.
>>>
>>> I strongly disagree. It's all explained in the plenty threads you have started
>>> on the Subject.
>>>
>>> TL;DR: As a reviewer for the piece in question I have done my part fully. It's
>>> not my issue that the piece is bundled into a single patch. We have options:
>>> 1) ask to split (and review as standalone piece);
>>> 2) add (partial) Rb with the pointers of what exactly has been reviewed;
>>> 3) ignore (silent agreement with the respective subsystem maintainer's opinion);
>>> 4) ...something I forgot?..
>>>
>>> (even partial) Rb != Ack.
>>>
>>
>> I fully agree with Andy.
>>
>> As the Xen maintainer I often have to look into such rework patches and often
>> enough especially the Xen specific parts are mentioned in the patch description
>> as "compile tested only".
>>
>> I see a "Reviewed-by: ... # Xen parts" in such cases as a feedback to the author
>> or committer that they don't have to look into those parts in more detail.
>>
>> The value of such a R-b: is more questionable in case of a pure rename of a
>> function or struct member, of course.
>>
> 
> So you don't agree with "Reviewed-by: # subpart"? Sorry, but I have
> troubles pointing your disagreement arguments to actual patch content.
I agree that "Reviewed-by: # subpart" is very valuable and should be used
in cases where the review really adds some value.

It is different to "Acked-by:" and IMHO not interchangeable with it.


Juergen

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH v2] docs: submitting-patches: Clarify half-baked review tags

[Krzysztof Kozlowski]

On 05/11/2026 16:44, Juergen Gross wrote:
> On 05.11.26 16:33, Krzysztof Kozlowski wrote:
>> On 05/11/2026 10:30, Jürgen Groß wrote:
>>> On 05.11.26 10:19, Andy Shevchenko wrote:
>>>> On Thu, Nov 05, 2026 at 09:27:30AM +0100, Krzysztof Kozlowski wrote:
>>>>> Refactoring of driver subsystems or some core driver parts often come
>>>>> with change of the actual drivers.  If that happens within one patch,
>>>>> people giving "Reviewed-by" tag only to piece of the patch, like one
>>>>> driver, and ignoring the rest including the main concept of the patch,
>>>>> are not really reviewing "the patch".
>>>>>
>>>>> As explained in the Submitting Patches document, the "Reviewer's
>>>>> statement of oversight" is for patch as in "entire patch" meaning:
>>>>>
>>>>>     (a) I have carried out a technical review of this patch to
>>>>>         evaluate its appropriateness and readiness for inclusion into
>>>>>         the mainline kernel.
>>>>>
>>>>> The sentence is explicit and complete, meaning it does not allow
>>>>> implicit variations like: "this patch or part of this patch".
>>>>>
>>>>> Reviewing pieces of the patch while ignoring the main part of it, is
>>>>> much closer to Acknowledgment, thus Ack, and "Reviewed-by" credit should
>>>>> not be applicable.
>>>>
>>>> I strongly disagree. It's all explained in the plenty threads you have started
>>>> on the Subject.
>>>>
>>>> TL;DR: As a reviewer for the piece in question I have done my part fully. It's
>>>> not my issue that the piece is bundled into a single patch. We have options:
>>>> 1) ask to split (and review as standalone piece);
>>>> 2) add (partial) Rb with the pointers of what exactly has been reviewed;
>>>> 3) ignore (silent agreement with the respective subsystem maintainer's opinion);
>>>> 4) ...something I forgot?..
>>>>
>>>> (even partial) Rb != Ack.
>>>>
>>>
>>> I fully agree with Andy.
>>>
>>> As the Xen maintainer I often have to look into such rework patches and often
>>> enough especially the Xen specific parts are mentioned in the patch description
>>> as "compile tested only".
>>>
>>> I see a "Reviewed-by: ... # Xen parts" in such cases as a feedback to the author
>>> or committer that they don't have to look into those parts in more detail.
>>>
>>> The value of such a R-b: is more questionable in case of a pure rename of a
>>> function or struct member, of course.
>>>
>>
>> So you don't agree with "Reviewed-by: # subpart"? Sorry, but I have
>> troubles pointing your disagreement arguments to actual patch content.
> I agree that "Reviewed-by: # subpart" is very valuable and should be used
> in cases where the review really adds some value.
> 
> It is different to "Acked-by:" and IMHO not interchangeable with it.

The patch does not make them interchangeable. The patch suggest two
choices if the reviewer's statement of oversight does not apply.

So repeating - which part of patch you disagree with? That reviewer's
statement of oversight says something else?

Best regards,
Krzysztof

Re: [PATCH v2] docs: submitting-patches: Clarify half-baked review tags

[Jürgen Groß]

[-- Attachment #1.1: Type: text/plain, Size: 922 bytes --]

On 05.11.26 16:53, Krzysztof Kozlowski wrote:
> Reviewing pieces of the patch while ignoring the main part of it, is
> much closer to Acknowledgment, thus Ack, and "Reviewed-by" credit should
> not be applicable.

I disagree with:

 >> Reviewing pieces of the patch while ignoring the main part of it, is
 >> much closer to Acknowledgment, thus Ack, and "Reviewed-by" credit should
 >> not be applicable.

And I agree with Andy's statement:

 >> TL;DR: As a reviewer for the piece in question I have done my part fully. It's
 >> not my issue that the piece is bundled into a single patch. We have options:
 >> 1) ask to split (and review as standalone piece);
 >> 2) add (partial) Rb with the pointers of what exactly has been reviewed;
 >> 3) ignore (silent agreement with the respective subsystem maintainer's opinion);
 >> 4) ...something I forgot?..
 >>
 >> (even partial) Rb != Ack.


Juergen

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH v2] docs: submitting-patches: Clarify half-baked review tags

[Krzysztof Kozlowski]

On 05/11/2026 16:59, Jürgen Groß wrote:
> On 05.11.26 16:53, Krzysztof Kozlowski wrote:
>> Reviewing pieces of the patch while ignoring the main part of it, is
>> much closer to Acknowledgment, thus Ack, and "Reviewed-by" credit should
>> not be applicable.
> 
> I disagree with:
> 
>  >> Reviewing pieces of the patch while ignoring the main part of it, is
>  >> much closer to Acknowledgment, thus Ack, and "Reviewed-by" credit should
>  >> not be applicable.
> 
> And I agree with Andy's statement:
> 
>  >> TL;DR: As a reviewer for the piece in question I have done my part fully. It's
>  >> not my issue that the piece is bundled into a single patch. We have options:
>  >> 1) ask to split (and review as standalone piece);
>  >> 2) add (partial) Rb with the pointers of what exactly has been reviewed;
>  >> 3) ignore (silent agreement with the respective subsystem maintainer's opinion);
>  >> 4) ...something I forgot?..
>  >>
>  >> (even partial) Rb != Ack.

So the patch contents is fine, but commit msg is not? OK, thank you.

Best regards,
Krzysztof