[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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





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



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.