Re: [PATCH][RESEND] xen/arm: vgic-v3: fix GICD_ISACTIVER range

[Julien Grall <julien@xxxxxxx>]

On 21/04/2020 19:49, Stefano Stabellini wrote:
> + George
>
> On Sat, 18 Apr 2020, Julien Grall wrote:
> > Hi,
> >
> > On 18/04/2020 00:12, Stefano Stabellini wrote:
> > > On Fri, 17 Apr 2020, Julien Grall wrote:
> > > > Hi,
> > > >
> > > > The title claim this is a resend, but this is actually not the latest
> > > > version of this patch. Can you explain why you decided to use v1
> > > > rather than v2?
> > > Because v2 added a printk for every read, and I thought you only wanted
> > > the range fix. Also, the printk is not a "warn once" so after the
> > > discussion where it became apparent that the register can be read many
> > > times, it sounded undesirable.
> > You previously mentionned that you will use Peng's patch. From my perspective,
> > this meant you are using the latest version of a patch not a previous one.
> >
> > So this was a bit of a surprised to me.
> >
> > > Nonetheless I don't have a strong preference between the two. If you
> > > prefer v2 it is here:
> > >
> > > https://marc.info/?l=xen-devel&m=157440872522065
> > I understand the "warn" issue but we did agree with it back then. I am ok to
> > drop it. However, may I request to be more forthcoming in your patch in the
> > future?
> >
> > For instance in this case, this a link to the original patch and an
> > explanation why you selected v1 would have been really useful.
> That's a good point, I can add it. So, for clarity, I'll keep v1 but
> expand on the commit message to say that we kept v1 to avoid spamming
> the console.
I am fine with that:

Acked-by: Julien Grall <jgrall@xxxxxxxxxx>

> > > Do you need me to resent it? If it is OK for you as-is, you can give
> > > your ack here, and I'll go ahead and commit it.
> > >
> > >
> > > > On Fri, 17 Apr 2020 at 23:16, Stefano Stabellini <sstabellini@xxxxxxxxxx>
> > > > wrote:
> > > > > From: Peng Fan <peng.fan@xxxxxxx>
> > > > >
> > > > > The end should be GICD_ISACTIVERN not GICD_ISACTIVER.
> > > > >
> > > > > (See https://marc.info/?l=xen-devel&m=158527653730795 for a discussion
> > > > > on
> > > > > what it would take to implement GICD_ISACTIVER/GICD_ICACTIVER properly.)
> > > > >
> > > > > Signed-off-by: Peng Fan <peng.fan@xxxxxxx>
> > > > > Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>
> > > > > Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> > > > I don't think you can be at the same time an author of the patch and a
> > > > reviewer. Otherwise, I could review my own patch ;).
> > > Yeah ... that was not the intention. I changed the commit message so I
> > > had to add my signed-off-by for copyright reasons.
> > > At the same time I
> > > reviewed it even before changing the commit message so I added the
> > > reviewed-by. I agree it looks kind of weird.
> > I don't feel this should go as-is. It is not clear in the commit message the
> > changes you did and could lead confusion once merged. One may think you
> > reviewed your own patch.
> >
> > In general when I tweak a commit message, I will not add my signed-off-by but
> > just add [julieng: Tweak commit message] above my reviewed-by/acked-by tag.
> >
> > Alternatively, you can remove your reviewed-by and let another maintainer
> > reviewing it.
> >
> > I will let you decide your preference and resend the patch appropriately.
>   
> The Linux policy on Signed-off-by is really strict: basically any time a
> person touches a patch, even just to commit the patch (no changes to
> it), they add a Signed-off-by. So it is common there and other projects
> to have both Signed-off-by and Reviewed-by from the same individual. For
> instance on Linux:
I don't think we used this policy so far for Xen Project. Before 
pointing out the Signed-off-by vs Reviewed-by, I actually looked online 
but I wasn't able to find why Signed-off-by and Reviewed-by was added 
together.
> commit b2a84de2a2deb76a6a51609845341f508c518c03
>
>      Reported-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
>      Acked-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
>      Reviewed-by: Catalin Marinas <catalin.marinas@xxxxxxx>
>      Signed-off-by: Will Deacon <will@xxxxxxxxxx>
>      Signed-off-by: Catalin Marinas <catalin.marinas@xxxxxxx>
>
> commit 33e45234987ea3ed4b05fc512f4441696478f12d
>
>      Reviewed-by: Kees Cook <keescook@xxxxxxxxxxxx>
>      Reviewed-by: Catalin Marinas <catalin.marinas@xxxxxxx>
>      Reviewed-by: Vincenzo Frascino <Vincenzo.Frascino@xxxxxxx>
>      Signed-off-by: Kristina Martsenko <kristina.martsenko@xxxxxxx>
>      [Amit: Modified secondary cores key structure, comments]
>      Signed-off-by: Amit Daniel Kachhap <amit.kachhap@xxxxxxx>
>      Signed-off-by: Catalin Marinas <catalin.marinas@xxxxxxx>
>
>
> On QEMU:
>
>
> commit 22cd0945b8429f818a2d90f06f02bb526bfb319d
>
>      Signed-off-by: Francisco Iglesias <frasse.iglesias@xxxxxxxxx>
>      Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xxxxxxxxxx>
>      Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xxxxxxxxxx>
>      Message-id: 20180503214201.29082-2-frasse.iglesias@xxxxxxxxx
>      Signed-off-by: Peter Maydell <peter.maydell@xxxxxxxxxx>
>
> commit 133d23b3ad1be53105b9950fb18858cf059f2da6
>
>      Signed-off-by: Alistair Francis <alistair.francis@xxxxxxxxxx>
>      Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xxxxxxxxxx>
>      Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xxxxxxxxxx>
>
>
> Your suggestion of adding something between brackets like:
>
>    [stefano: update commit message]
>
> is good because it clarifies things and I'd like to do that. But still,
> I think it would require the addition of my Signed-off-by. At the same
> time it doesn't look like we want to avoiding adding a Reviewed-by
> because a reviewer made a change to the commit message too?
Agree, I also think we need to clarify in this case as it is more 
difficult to understand if the signed-off-by was by a contributor or 
someone who merged it.
> Of course, for this patch, I am happy to drop my Reviewed-by and resend
> and I'll do that. But I think it would be worth clarifying this point at
> the project level. George, do we or the LinuxFoundation have a policy on
> whether we can or cannot have reviewed-by and signed-off-by from the same
> individual?
Cheers,

--
Julien Grall