* [PATCH][RESEND] xen/arm: vgic-v3: fix GICD_ISACTIVER range
@ 2020-04-17 22:16 Stefano Stabellini
2020-04-17 22:51 ` Julien Grall
0 siblings, 1 reply; 6+ messages in thread
From: Stefano Stabellini @ 2020-04-17 22:16 UTC (permalink / raw)
To: julien; +Cc: xen-devel, Peng Fan, sstabellini, Stefano Stabellini
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>
---
xen/arch/arm/vgic-v3.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
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;
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH][RESEND] xen/arm: vgic-v3: fix GICD_ISACTIVER range
2020-04-17 22:16 [PATCH][RESEND] xen/arm: vgic-v3: fix GICD_ISACTIVER range Stefano Stabellini
@ 2020-04-17 22:51 ` Julien Grall
2020-04-17 23:12 ` Stefano Stabellini
0 siblings, 1 reply; 6+ messages in thread
From: Julien Grall @ 2020-04-17 22:51 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel, Peng Fan, Stefano Stabellini
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,
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH][RESEND] xen/arm: vgic-v3: fix GICD_ISACTIVER range
2020-04-17 22:51 ` Julien Grall
@ 2020-04-17 23:12 ` Stefano Stabellini
2020-04-18 9:49 ` Julien Grall
0 siblings, 1 reply; 6+ messages in thread
From: Stefano Stabellini @ 2020-04-17 23:12 UTC (permalink / raw)
To: Julien Grall; +Cc: xen-devel, Peng Fan, Stefano Stabellini, Stefano Stabellini
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH][RESEND] xen/arm: vgic-v3: fix GICD_ISACTIVER range
2020-04-17 23:12 ` Stefano Stabellini
@ 2020-04-18 9:49 ` Julien Grall
2020-04-21 18:49 ` Stefano Stabellini
0 siblings, 1 reply; 6+ messages in thread
From: Julien Grall @ 2020-04-18 9:49 UTC (permalink / raw)
To: Stefano Stabellini, Julien Grall; +Cc: xen-devel, Peng Fan, Stefano Stabellini
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,
--
Julien Grall
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH][RESEND] xen/arm: vgic-v3: fix GICD_ISACTIVER range
2020-04-18 9:49 ` Julien Grall
@ 2020-04-21 18:49 ` Stefano Stabellini
2020-04-21 19:04 ` Julien Grall
0 siblings, 1 reply; 6+ messages in thread
From: Stefano Stabellini @ 2020-04-21 18:49 UTC (permalink / raw)
To: Julien Grall
Cc: Peng Fan, Stefano Stabellini, George.Dunlap, xen-devel,
Stefano Stabellini, Julien Grall
+ 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?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH][RESEND] xen/arm: vgic-v3: fix GICD_ISACTIVER range
2020-04-21 18:49 ` Stefano Stabellini
@ 2020-04-21 19:04 ` Julien Grall
0 siblings, 0 replies; 6+ messages in thread
From: Julien Grall @ 2020-04-21 19:04 UTC (permalink / raw)
To: Stefano Stabellini
Cc: xen-devel, Peng Fan, Stefano Stabellini, George.Dunlap, Julien Grall
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,
--
Julien Grall
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-04-21 19:04 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-17 22:16 [PATCH][RESEND] xen/arm: vgic-v3: fix GICD_ISACTIVER range Stefano Stabellini
2020-04-17 22:51 ` Julien Grall
2020-04-17 23:12 ` Stefano Stabellini
2020-04-18 9:49 ` Julien Grall
2020-04-21 18:49 ` Stefano Stabellini
2020-04-21 19:04 ` Julien Grall
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.