| Message ID | 20200417221609.19928-1-sstabellini@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RESEND] xen/arm: vgic-v3: fix GICD_ISACTIVER range | expand |
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? On Fri, 17 Apr 2020 at 23:16, Stefano Stabellini <sstabellini@kernel.org> wrote: > > From: Peng Fan <peng.fan@nxp.com> > > 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@nxp.com> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com> > Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> 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 ;). Cheers,
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. 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 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@kernel.org> wrote: > > > > From: Peng Fan <peng.fan@nxp.com> > > > > 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@nxp.com> > > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com> > > Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > > 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.
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@kernel.org> wrote: >>> >>> From: Peng Fan <peng.fan@nxp.com> >>> >>> 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@nxp.com> >>> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com> >>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> >> >> 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,
+ 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. > > 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@kernel.org> > > > wrote: > > > > > > > > From: Peng Fan <peng.fan@nxp.com> > > > > > > > > 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@nxp.com> > > > > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com> > > > > Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > > > > > > 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: commit b2a84de2a2deb76a6a51609845341f508c518c03 Reported-by: Linus Torvalds <torvalds@linux-foundation.org> Acked-by: Linus Torvalds <torvalds@linux-foundation.org> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com> Signed-off-by: Will Deacon <will@kernel.org> Signed-off-by: Catalin Marinas <catalin.marinas@arm.com> commit 33e45234987ea3ed4b05fc512f4441696478f12d Reviewed-by: Kees Cook <keescook@chromium.org> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com> Reviewed-by: Vincenzo Frascino <Vincenzo.Frascino@arm.com> Signed-off-by: Kristina Martsenko <kristina.martsenko@arm.com> [Amit: Modified secondary cores key structure, comments] Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com> Signed-off-by: Catalin Marinas <catalin.marinas@arm.com> On QEMU: commit 22cd0945b8429f818a2d90f06f02bb526bfb319d Signed-off-by: Francisco Iglesias <frasse.iglesias@gmail.com> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> Message-id: 20180503214201.29082-2-frasse.iglesias@gmail.com Signed-off-by: Peter Maydell <peter.maydell@linaro.org> commit 133d23b3ad1be53105b9950fb18858cf059f2da6 Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> 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? 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?
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@amazon.com> > > >>> 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@kernel.org> >>>> wrote: >>>>> >>>>> From: Peng Fan <peng.fan@nxp.com> >>>>> >>>>> 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@nxp.com> >>>>> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com> >>>>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> >>>> >>>> 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@linux-foundation.org> > Acked-by: Linus Torvalds <torvalds@linux-foundation.org> > Reviewed-by: Catalin Marinas <catalin.marinas@arm.com> > Signed-off-by: Will Deacon <will@kernel.org> > Signed-off-by: Catalin Marinas <catalin.marinas@arm.com> > > commit 33e45234987ea3ed4b05fc512f4441696478f12d > > Reviewed-by: Kees Cook <keescook@chromium.org> > Reviewed-by: Catalin Marinas <catalin.marinas@arm.com> > Reviewed-by: Vincenzo Frascino <Vincenzo.Frascino@arm.com> > Signed-off-by: Kristina Martsenko <kristina.martsenko@arm.com> > [Amit: Modified secondary cores key structure, comments] > Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com> > Signed-off-by: Catalin Marinas <catalin.marinas@arm.com> > > > On QEMU: > > > commit 22cd0945b8429f818a2d90f06f02bb526bfb319d > > Signed-off-by: Francisco Iglesias <frasse.iglesias@gmail.com> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> > Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> > Message-id: 20180503214201.29082-2-frasse.iglesias@gmail.com > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > > commit 133d23b3ad1be53105b9950fb18858cf059f2da6 > > Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> > Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> > > > 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,
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c index 4e60ba15cc..fd8cfc156d 100644 --- a/xen/arch/arm/vgic-v3.c +++ b/xen/arch/arm/vgic-v3.c @@ -713,7 +713,7 @@ static int __vgic_v3_distr_common_mmio_read(const char *name, struct vcpu *v, goto read_as_zero; /* Read the active status of an IRQ via GICD/GICR is not supported */ - case VRANGE32(GICD_ISACTIVER, GICD_ISACTIVER): + case VRANGE32(GICD_ISACTIVER, GICD_ISACTIVERN): case VRANGE32(GICD_ICACTIVER, GICD_ICACTIVERN): goto read_as_zero;