DCO chain of custody revisited (was Re: [PATCH v4 08/11] mfd: qcom-pm8xxx: drop unused PM8018 compatible)

DCO chain of custody revisited (was Re: [PATCH v4 08/11] mfd: qcom-pm8xxx: drop unused PM8018 compatible)

[Konstantin Ryabitsev]

Hijacking this thread for greater good.

On Mon, Oct 31, 2022 at 04:35:38PM +0100, Neil Armstrong wrote:
> Hi,
> 
> On 31/10/2022 16:32, Lee Jones wrote:
> > On Fri, 21 Oct 2022, Neil Armstrong wrote:
> > 
> > > The PM8018 compatible is always used with PM8921 fallback, so PM8018
> > > compatible can be safely removed from device ID table
> > > 
> > > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> > > Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> > 
> > Tags should appear chronologically.
> 
> Indeed, they were added by b4, I'll report this.

My trouble is that there are seemingly as many opinions about the order of
trailers as there are subsystem maintainers. The last time we had a long
discussion about this on the users list I got a strong message that what
matters most is the chain of custody, and the Signed-off-by trailer indicates
the chain of custody boundary.

In the scenario below, the chain consists of 3 people:

| Suggested-by: Reporter 1 <...>
| Signed-off-by: Developer 1 <...> -- initial DCO boundary
| Reviewed-by: Reviewer 1 <...>
| Tested-by: Tester 1 <...>
| Signed-off-by: Submaintainer 1 <...> -- intermediate DCO boundary
| Acked-by: Submaintainer 2 <...>
| Signed-off-by: Maintainer 1 <...> -- final DCO boundary

In terms of DCO, this makes the following claims:

Developer 1:
 - I am responsible for this change
 - It was suggested by Reporter 1

Submaintainer 1:
 - I am signing off on this change
 - I have collected the trailers from Reviewer 1 and Tester 1

Maintainer 1:
 - I am signing off on this change
 - I have collected the trailer from Submaintainer 2

In the current case, and using this principle, the following order is correct:

| Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
| Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>

Because the Reviewerd-by trailer was sent to the v2 of the series and was
collected by Neil, so Neil is the person who is the DCO signatory of that
chain of custody in the v4 of the series.

I assume that in the final commit Lee rearranged the tags in the following
order:

| Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
| Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
| Signed-off-by: Lee Jones <lee@kernel.org>

This would indicate that it's *Lee* who is claiming responsibility for
collecting the Reviewed-by tag from Krzysztof, because it is in his chain of
custody. However, this is not the case -- it was Neil who collected the tag,
and therefore the "more correct" order should be:

| Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
| Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
| Signed-off-by: Lee Jones <lee@kernel.org>

If my reasoning is incorrect, then I need to go back to the drawing board.

-K

Re: DCO chain of custody revisited (was Re: [PATCH v4 08/11] mfd: qcom-pm8xxx: drop unused PM8018 compatible)

[James Bottomley]

On Mon, 2022-10-31 at 12:58 -0400, Konstantin Ryabitsev wrote:
[...]
> I assume that in the final commit Lee rearranged the tags in the
> following order:
> 
> > Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> > Signed-off-by: Lee Jones <lee@kernel.org>
> 
> This would indicate that it's *Lee* who is claiming responsibility
> for collecting the Reviewed-by tag from Krzysztof, because it is in
> his chain of custody. However, this is not the case -- it was Neil
> who collected the tag, and therefore the "more correct" order should
> be:
> 
> > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> > Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> > Signed-off-by: Lee Jones <lee@kernel.org>
> 
> If my reasoning is incorrect, then I need to go back to the drawing
> board.

You're way over thinking this.  The only tag that matters from the DCO
point of view is Signed-off-by.  That's the ordering we care about for
the chain of custody.  All other tags are irrelevant.  Of course, it's
nice to think that reviews happen *after* the code was modified, which
is why most of us like to see the Reviewed-by after the initial author
signoff, but who added the tag has no DCO significance because it
doesn't affect the representations on the contribution.

Regards,

James

Re: DCO chain of custody revisited (was Re: [PATCH v4 08/11] mfd: qcom-pm8xxx: drop unused PM8018 compatible)

[Konstantin Ryabitsev]

On Mon, Oct 31, 2022 at 01:10:58PM -0400, James Bottomley wrote:
> > > Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> > > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> > > Signed-off-by: Lee Jones <lee@kernel.org>
> > 
> > This would indicate that it's *Lee* who is claiming responsibility
> > for collecting the Reviewed-by tag from Krzysztof, because it is in
> > his chain of custody. However, this is not the case -- it was Neil
> > who collected the tag, and therefore the "more correct" order should
> > be:
> > 
> > > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> > > Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> > > Signed-off-by: Lee Jones <lee@kernel.org>
> > 
> > If my reasoning is incorrect, then I need to go back to the drawing
> > board.
> 
> You're way over thinking this.

Yes, but it's my job to overthink this, so nobody else has to. :)

> The only tag that matters from the DCO point of view is Signed-off-by.
> That's the ordering we care about for the chain of custody.  All other tags
> are irrelevant.  Of course, it's nice to think that reviews happen *after*
> the code was modified, which is why most of us like to see the Reviewed-by
> after the initial author signoff,

But this is where it becomes complicated. The Reviewed-By trailer was sent to
the v2 of the series, and incorporated into v3(via b4 trailers). If we stick
it below Signed-off-by, then it may suggest that Krzysztof reviewed the v4 of
the patch.

By placing it above the Signed-off-by line, we at least clearly indicate that
it's Neil who put it there.

-K

Re: DCO chain of custody revisited (was Re: [PATCH v4 08/11] mfd: qcom-pm8xxx: drop unused PM8018 compatible)

[James Bottomley]

On Mon, 2022-10-31 at 13:23 -0400, Konstantin Ryabitsev wrote:
> On Mon, Oct 31, 2022 at 01:10:58PM -0400, James Bottomley wrote:
> > > > Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> > > > Reviewed-by: Krzysztof Kozlowski <
> > > > krzysztof.kozlowski@linaro.org>
> > > > Signed-off-by: Lee Jones <lee@kernel.org>
> > > 
> > > This would indicate that it's *Lee* who is claiming
> > > responsibility
> > > for collecting the Reviewed-by tag from Krzysztof, because it is
> > > in
> > > his chain of custody. However, this is not the case -- it was
> > > Neil
> > > who collected the tag, and therefore the "more correct" order
> > > should
> > > be:
> > > 
> > > > Reviewed-by: Krzysztof Kozlowski <
> > > > krzysztof.kozlowski@linaro.org>
> > > > Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> > > > Signed-off-by: Lee Jones <lee@kernel.org>
> > > 
> > > If my reasoning is incorrect, then I need to go back to the
> > > drawing
> > > board.
> > 
> > You're way over thinking this.
> 
> Yes, but it's my job to overthink this, so nobody else has to. :)
> 
> > The only tag that matters from the DCO point of view is Signed-off-
> > by. That's the ordering we care about for the chain of
> > custody.  All other tags are irrelevant.  Of course, it's nice to
> > think that reviews happen *after* the code was modified, which is
> > why most of us like to see the Reviewed-by after the initial author
> > signoff,
> 
> But this is where it becomes complicated. The Reviewed-By trailer was
> sent to the v2 of the series, and incorporated into v3(via b4
> trailers). If we stick it below Signed-off-by, then it may suggest
> that Krzysztof reviewed the v4 of the patch.

If the changes from v2 to v4 were material enough for that question to
be relevant then the Reviewed-by tag shouldn't have been kept because
the patch needed reviewing again.

> By placing it above the Signed-off-by line, we at least clearly
> indicate that it's Neil who put it there.

Who put it there does not matter, so it's not really a problem that
needs solving.

However, there is a problem if b4 is preserving Reviewed-by tags for
patches with material changes ... the review has to be redone and thus
the tag should be lost.  You can only keep Reviewed-by tags for
cosmetic changes (or obviously patches which don't change from version
to version).

James

Re: DCO chain of custody revisited (was Re: [PATCH v4 08/11] mfd: qcom-pm8xxx: drop unused PM8018 compatible)

[Alex Elder]

On 10/31/22 12:23 PM, Konstantin Ryabitsev wrote:
> On Mon, Oct 31, 2022 at 01:10:58PM -0400, James Bottomley wrote:
>>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
>>>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>>> Signed-off-by: Lee Jones <lee@kernel.org>
>>>
>>> This would indicate that it's *Lee* who is claiming responsibility
>>> for collecting the Reviewed-by tag from Krzysztof, because it is in
>>> his chain of custody. However, this is not the case -- it was Neil
>>> who collected the tag, and therefore the "more correct" order should
>>> be:
>>>
>>>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
>>>> Signed-off-by: Lee Jones <lee@kernel.org>
>>>
>>> If my reasoning is incorrect, then I need to go back to the drawing
>>> board.
>>
>> You're way over thinking this.
> 
> Yes, but it's my job to overthink this, so nobody else has to. :)
> 
>> The only tag that matters from the DCO point of view is Signed-off-by.
>> That's the ordering we care about for the chain of custody.  All other tags
>> are irrelevant.  Of course, it's nice to think that reviews happen *after*
>> the code was modified, which is why most of us like to see the Reviewed-by
>> after the initial author signoff,
> 
> But this is where it becomes complicated. The Reviewed-By trailer was sent to
> the v2 of the series, and incorporated into v3(via b4 trailers). If we stick
> it below Signed-off-by, then it may suggest that Krzysztof reviewed the v4 of
> the patch.
> 
> By placing it above the Signed-off-by line, we at least clearly indicate that
> it's Neil who put it there.

I prefer this interpretation.  That is, the "outer" (last) sign-off
is essentially indicating that everything within (above) it is what
their sign-off applies to.

Most (all?) of the other tags (like Suggested-by, etc.) don't 
necessarily have a strong requirement or precise meaning, so in
a way it doesn't matter that much.  But I think this convention
adds a small amount of value.

					-Alex

> 
> -K
> 

Re: DCO chain of custody revisited (was Re: [PATCH v4 08/11] mfd: qcom-pm8xxx: drop unused PM8018 compatible)

[Michael S. Tsirkin]

On Mon, Oct 31, 2022 at 12:58:42PM -0400, Konstantin Ryabitsev wrote:
> Hijacking this thread for greater good.
> 
> On Mon, Oct 31, 2022 at 04:35:38PM +0100, Neil Armstrong wrote:
> > Hi,
> > 
> > On 31/10/2022 16:32, Lee Jones wrote:
> > > On Fri, 21 Oct 2022, Neil Armstrong wrote:
> > > 
> > > > The PM8018 compatible is always used with PM8921 fallback, so PM8018
> > > > compatible can be safely removed from device ID table
> > > > 
> > > > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> > > > Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> > > 
> > > Tags should appear chronologically.
> > 
> > Indeed, they were added by b4, I'll report this.
> 
> My trouble is that there are seemingly as many opinions about the order of
> trailers as there are subsystem maintainers. The last time we had a long
> discussion about this on the users list I got a strong message that what
> matters most is the chain of custody, and the Signed-off-by trailer indicates
> the chain of custody boundary.
> 
> In the scenario below, the chain consists of 3 people:
> 
> | Suggested-by: Reporter 1 <...>
> | Signed-off-by: Developer 1 <...> -- initial DCO boundary
> | Reviewed-by: Reviewer 1 <...>
> | Tested-by: Tester 1 <...>
> | Signed-off-by: Submaintainer 1 <...> -- intermediate DCO boundary
> | Acked-by: Submaintainer 2 <...>
> | Signed-off-by: Maintainer 1 <...> -- final DCO boundary
> 
> In terms of DCO, this makes the following claims:
> 
> Developer 1:
>  - I am responsible for this change
>  - It was suggested by Reporter 1
> 
> Submaintainer 1:
>  - I am signing off on this change
>  - I have collected the trailers from Reviewer 1 and Tester 1
> 
> Maintainer 1:
>  - I am signing off on this change
>  - I have collected the trailer from Submaintainer 2
> 
> In the current case, and using this principle, the following order is correct:
> 
> | Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> | Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> 
> Because the Reviewerd-by trailer was sent to the v2 of the series and was
> collected by Neil, so Neil is the person who is the DCO signatory of that
> chain of custody in the v4 of the series.
> 
> I assume that in the final commit Lee rearranged the tags in the following
> order:
> 
> | Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> | Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> | Signed-off-by: Lee Jones <lee@kernel.org>
> 
> This would indicate that it's *Lee* who is claiming responsibility for
> collecting the Reviewed-by tag from Krzysztof, because it is in his chain of
> custody. However, this is not the case -- it was Neil who collected the tag,
> and therefore the "more correct" order should be:


I think this is often incorrect:
I often pick a patch, then amend it adding tags from reviewers,
typically I add tags at the end just because it's easier,
and tools like git-interpret-trailers exist.

> | Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> | Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> | Signed-off-by: Lee Jones <lee@kernel.org>
> 
> If my reasoning is incorrect, then I need to go back to the drawing board.
> 
> -K

Re: DCO chain of custody revisited (was Re: [PATCH v4 08/11] mfd: qcom-pm8xxx: drop unused PM8018 compatible)

[Konstantin Ryabitsev]

On Mon, Oct 31, 2022 at 01:33:27PM -0400, James Bottomley wrote:
> > But this is where it becomes complicated. The Reviewed-By trailer was
> > sent to the v2 of the series, and incorporated into v3(via b4
> > trailers). If we stick it below Signed-off-by, then it may suggest
> > that Krzysztof reviewed the v4 of the patch.
> 
> If the changes from v2 to v4 were material enough for that question to
> be relevant then the Reviewed-by tag shouldn't have been kept because
> the patch needed reviewing again.
> 
> > By placing it above the Signed-off-by line, we at least clearly
> > indicate that it's Neil who put it there.
> 
> Who put it there does not matter, so it's not really a problem that
> needs solving.
> 
> However, there is a problem if b4 is preserving Reviewed-by tags for
> patches with material changes ... the review has to be redone and thus
> the tag should be lost.  You can only keep Reviewed-by tags for
> cosmetic changes (or obviously patches which don't change from version
> to version).

I am not disagreeing, but as far as b4 is concerned, there is no way to
recognize this reliably. A rebase to a newer tree may have resulted in the
patch-id changing, with the actual logic remaining exactly the same. It's the
responsibility of the person submitting the series to make sure that the
trailers are still relevant -- the best I can do is to clearly indicate who
was the guilty party behind the decision.

-K

Re: DCO chain of custody revisited (was Re: [PATCH v4 08/11] mfd: qcom-pm8xxx: drop unused PM8018 compatible)

[Willy Tarreau]

On Mon, Oct 31, 2022 at 12:33:30PM -0500, Alex Elder wrote:
> On 10/31/22 12:23 PM, Konstantin Ryabitsev wrote:
> > On Mon, Oct 31, 2022 at 01:10:58PM -0400, James Bottomley wrote:
> > > > > Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> > > > > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> > > > > Signed-off-by: Lee Jones <lee@kernel.org>
> > > > 
> > > > This would indicate that it's *Lee* who is claiming responsibility
> > > > for collecting the Reviewed-by tag from Krzysztof, because it is in
> > > > his chain of custody. However, this is not the case -- it was Neil
> > > > who collected the tag, and therefore the "more correct" order should
> > > > be:
> > > > 
> > > > > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> > > > > Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> > > > > Signed-off-by: Lee Jones <lee@kernel.org>
> > > > 
> > > > If my reasoning is incorrect, then I need to go back to the drawing
> > > > board.
> > > 
> > > You're way over thinking this.
> > 
> > Yes, but it's my job to overthink this, so nobody else has to. :)
> > 
> > > The only tag that matters from the DCO point of view is Signed-off-by.
> > > That's the ordering we care about for the chain of custody.  All other tags
> > > are irrelevant.  Of course, it's nice to think that reviews happen *after*
> > > the code was modified, which is why most of us like to see the Reviewed-by
> > > after the initial author signoff,
> > 
> > But this is where it becomes complicated. The Reviewed-By trailer was sent to
> > the v2 of the series, and incorporated into v3(via b4 trailers). If we stick
> > it below Signed-off-by, then it may suggest that Krzysztof reviewed the v4 of
> > the patch.
> > 
> > By placing it above the Signed-off-by line, we at least clearly indicate that
> > it's Neil who put it there.
> 
> I prefer this interpretation.  That is, the "outer" (last) sign-off
> is essentially indicating that everything within (above) it is what
> their sign-off applies to.
> 
> Most (all?) of the other tags (like Suggested-by, etc.) don't necessarily
> have a strong requirement or precise meaning, so in
> a way it doesn't matter that much.  But I think this convention
> adds a small amount of value.

At least for stable backports we've been using this a lot because we
often have to adapt patches that get backported, and it's often
convenient to add a small comment summarizing the changes after the
last s-o-b and before the backporter's. But I agree that most tags
probably do not care much about ordering if the contents are not
changed.

Sometimes a maintainer might reword a commit message and in this case
maybe the reviewed-by order may be relevant.

Willy

Re: DCO chain of custody revisited (was Re: [PATCH v4 08/11] mfd: qcom-pm8xxx: drop unused PM8018 compatible)

[Theodore Ts'o]

On Mon, Oct 31, 2022 at 01:33:27PM -0400, James Bottomley wrote:
> Who put it there does not matter, so it's not really a problem that
> needs solving.
> 
> However, there is a problem if b4 is preserving Reviewed-by tags for
> patches with material changes ... the review has to be redone and thus
> the tag should be lost.  You can only keep Reviewed-by tags for
> cosmetic changes (or obviously patches which don't change from version
> to version).

What I've seen some reviewers do is to request some additional changes
(please add more details in the comments, or please drop the test if
the poitner is non-NULL before calling kfree(), and then they add the
Reviewed-by tag.  In fact, in some cases they explicitly say, "please
fix these things and then you may add "Reviewed-by: Jon Q. Random
<random@example.com>"

(For example, IIRC, Jan Kara does this a fair amount.)

I understand that b4 can't possibly parse english, and if it does
start gaining Skynet abilities, we've got bigger problems, but suffice
it to say, there *are* times the Reviewed-By tag should be kept even
though non-commit-description and/or non-commit changes were made.

       			      	     			     - Ted

Re: DCO chain of custody revisited (was Re: [PATCH v4 08/11] mfd: qcom-pm8xxx: drop unused PM8018 compatible)

[Krzysztof Kozlowski]

On 31/10/2022 13:23, Konstantin Ryabitsev wrote:
> On Mon, Oct 31, 2022 at 01:10:58PM -0400, James Bottomley wrote:
>>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
>>>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>>> Signed-off-by: Lee Jones <lee@kernel.org>
>>>
>>> This would indicate that it's *Lee* who is claiming responsibility
>>> for collecting the Reviewed-by tag from Krzysztof, because it is in
>>> his chain of custody. However, this is not the case -- it was Neil
>>> who collected the tag, and therefore the "more correct" order should
>>> be:
>>>
>>>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
>>>> Signed-off-by: Lee Jones <lee@kernel.org>
>>>
>>> If my reasoning is incorrect, then I need to go back to the drawing
>>> board.
>>
>> You're way over thinking this.
> 
> Yes, but it's my job to overthink this, so nobody else has to. :)

I appreciate overthinking, because it makes the life of patch submitters
easier - they can rely on a tool (b4) as a source of the process instead
of being corrected by maintainers with conflicting points of view.

> 
>> The only tag that matters from the DCO point of view is Signed-off-by.
>> That's the ordering we care about for the chain of custody.  All other tags
>> are irrelevant.  Of course, it's nice to think that reviews happen *after*
>> the code was modified, which is why most of us like to see the Reviewed-by
>> after the initial author signoff,

I guess it depends then what is the meaning of SoB. In my understanding,
DCO (and SoB) only state the contents of the contribution, so:
1. I write patch (or collect it from somewhere etc).
2. I signed it with SoB.
3. There was a review tag given, so I add a review tag.

My SoB does not certify order of review tags or any other pieces than
what is explained in DCO: I created the contribution and I am allowed to
send it.

Therefore Rb/Ack/tags go after my SoB.

https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html#:~:text=The%20Signed%2Doff%2Dby%3A,in%20the%20patch's%20delivery%20path.
is even saying:
"Some people also put extra tags at the end. "
so some tags are *after*.

> 
> But this is where it becomes complicated. The Reviewed-By trailer was sent to
> the v2 of the series, and incorporated into v3(via b4 trailers). If we stick
> it below Signed-off-by, then it may suggest that Krzysztof reviewed the v4 of
> the patch.

No, you cannot imply the order from that. That's only one possibility.

> By placing it above the Signed-off-by line, we at least clearly indicate that
> it's Neil who put it there.

True, but that's not the point of SoB... I can also argue that this
implies that review happened before contribution was signed off.

Best regards,
Krzysztof