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

[Julien Grall <julien@xxxxxxx>]

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.
> 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.

Cheers,

--
Julien Grall