* [PATCH] irqchip/gic-v3: Use wmb() instead of smb_wmb() in gic_raise_softirq()
@ 2018-02-01 0:03 Shanker Donthineni
2018-02-01 10:33 ` Will Deacon
2022-02-18 11:27 ` Barry Song
0 siblings, 2 replies; 6+ messages in thread
From: Shanker Donthineni @ 2018-02-01 0:03 UTC (permalink / raw)
To: linux-arm-kernel
A DMB instruction can be used to ensure the relative order of only
memory accesses before and after the barrier. Since writes to system
registers are not memory operations, barrier DMB is not sufficient
for observability of memory accesses that occur before ICC_SGI1R_EL1
writes.
A DSB instruction ensures that no instructions that appear in program
order after the DSB instruction, can execute until the DSB instruction
has completed.
Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org>
---
drivers/irqchip/irq-gic-v3.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index b56c3e2..980ae8e 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -688,7 +688,7 @@ static void gic_raise_softirq(const struct cpumask *mask, unsigned int irq)
* Ensure that stores to Normal memory are visible to the
* other CPUs before issuing the IPI.
*/
- smp_wmb();
+ wmb();
for_each_cpu(cpu, mask) {
u64 cluster_id = MPIDR_TO_SGI_CLUSTER_ID(cpu_logical_map(cpu));
--
Qualcomm Datacenter Technologies, Inc. on behalf of the Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH] irqchip/gic-v3: Use wmb() instead of smb_wmb() in gic_raise_softirq()
2018-02-01 0:03 [PATCH] irqchip/gic-v3: Use wmb() instead of smb_wmb() in gic_raise_softirq() Shanker Donthineni
@ 2018-02-01 10:33 ` Will Deacon
2018-02-01 12:55 ` Shanker Donthineni
2022-02-18 11:27 ` Barry Song
1 sibling, 1 reply; 6+ messages in thread
From: Will Deacon @ 2018-02-01 10:33 UTC (permalink / raw)
To: linux-arm-kernel
Hi Shanker,
On Wed, Jan 31, 2018 at 06:03:42PM -0600, Shanker Donthineni wrote:
> A DMB instruction can be used to ensure the relative order of only
> memory accesses before and after the barrier. Since writes to system
> registers are not memory operations, barrier DMB is not sufficient
> for observability of memory accesses that occur before ICC_SGI1R_EL1
> writes.
>
> A DSB instruction ensures that no instructions that appear in program
> order after the DSB instruction, can execute until the DSB instruction
> has completed.
>
> Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org>
> ---
> drivers/irqchip/irq-gic-v3.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> index b56c3e2..980ae8e 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -688,7 +688,7 @@ static void gic_raise_softirq(const struct cpumask *mask, unsigned int irq)
> * Ensure that stores to Normal memory are visible to the
> * other CPUs before issuing the IPI.
> */
> - smp_wmb();
> + wmb();
I think this is the right thing to do and the smp_wmb() was accidentally
pulled in here as a copy-paste from the GICv2 driver where it is sufficient
in practice.
Did you spot this by code inspection, or did the DMB actually cause
observable failures? (trying to figure out whether or not this need to go
to -stable).
Anyway:
Acked-by: Will Deacon <will.deacon@arm.com>
Cheers,
Will
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] irqchip/gic-v3: Use wmb() instead of smb_wmb() in gic_raise_softirq()
2018-02-01 10:33 ` Will Deacon
@ 2018-02-01 12:55 ` Shanker Donthineni
2018-02-01 13:24 ` Marc Zyngier
0 siblings, 1 reply; 6+ messages in thread
From: Shanker Donthineni @ 2018-02-01 12:55 UTC (permalink / raw)
To: linux-arm-kernel
Hi Will, Thanks for your quick reply.
On 02/01/2018 04:33 AM, Will Deacon wrote:
> Hi Shanker,
>
> On Wed, Jan 31, 2018 at 06:03:42PM -0600, Shanker Donthineni wrote:
>> A DMB instruction can be used to ensure the relative order of only
>> memory accesses before and after the barrier. Since writes to system
>> registers are not memory operations, barrier DMB is not sufficient
>> for observability of memory accesses that occur before ICC_SGI1R_EL1
>> writes.
>>
>> A DSB instruction ensures that no instructions that appear in program
>> order after the DSB instruction, can execute until the DSB instruction
>> has completed.
>>
>> Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org>
>> ---
>> drivers/irqchip/irq-gic-v3.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
>> index b56c3e2..980ae8e 100644
>> --- a/drivers/irqchip/irq-gic-v3.c
>> +++ b/drivers/irqchip/irq-gic-v3.c
>> @@ -688,7 +688,7 @@ static void gic_raise_softirq(const struct cpumask *mask, unsigned int irq)
>> * Ensure that stores to Normal memory are visible to the
>> * other CPUs before issuing the IPI.
>> */
>> - smp_wmb();
>> + wmb();
>
> I think this is the right thing to do and the smp_wmb() was accidentally
> pulled in here as a copy-paste from the GICv2 driver where it is sufficient
> in practice.
>
> Did you spot this by code inspection, or did the DMB actually cause
> observable failures? (trying to figure out whether or not this need to go
> to -stable).
>
We've inspected the code because kernel was causing failures in scheduler/IPI_RESCHDULE.
After some time of debugging, we landed in GIC driver and found that the issue was due
to the DMB barrier.
Side note, we're also missing synchronization barriers in GIC driver after writing some
of the ICC_XXX system registers. I'm planning to post those changes for comments.
e.g: gic_write_sgi1r(val) and gic_write_eoir(irqnr);
> Anyway:
>
> Acked-by: Will Deacon <will.deacon@arm.com>
>
> Cheers,
>
> Will
>
--
Shanker Donthineni
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] irqchip/gic-v3: Use wmb() instead of smb_wmb() in gic_raise_softirq()
2018-02-01 12:55 ` Shanker Donthineni
@ 2018-02-01 13:24 ` Marc Zyngier
2018-02-01 13:42 ` Adam Wallis
0 siblings, 1 reply; 6+ messages in thread
From: Marc Zyngier @ 2018-02-01 13:24 UTC (permalink / raw)
To: linux-arm-kernel
On 01/02/18 12:55, Shanker Donthineni wrote:
> Hi Will, Thanks for your quick reply.
>
> On 02/01/2018 04:33 AM, Will Deacon wrote:
>> Hi Shanker,
>>
>> On Wed, Jan 31, 2018 at 06:03:42PM -0600, Shanker Donthineni wrote:
>>> A DMB instruction can be used to ensure the relative order of only
>>> memory accesses before and after the barrier. Since writes to system
>>> registers are not memory operations, barrier DMB is not sufficient
>>> for observability of memory accesses that occur before ICC_SGI1R_EL1
>>> writes.
>>>
>>> A DSB instruction ensures that no instructions that appear in program
>>> order after the DSB instruction, can execute until the DSB instruction
>>> has completed.
>>>
>>> Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org>
>>> ---
>>> drivers/irqchip/irq-gic-v3.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
>>> index b56c3e2..980ae8e 100644
>>> --- a/drivers/irqchip/irq-gic-v3.c
>>> +++ b/drivers/irqchip/irq-gic-v3.c
>>> @@ -688,7 +688,7 @@ static void gic_raise_softirq(const struct cpumask *mask, unsigned int irq)
>>> * Ensure that stores to Normal memory are visible to the
>>> * other CPUs before issuing the IPI.
>>> */
>>> - smp_wmb();
>>> + wmb();
>>
>> I think this is the right thing to do and the smp_wmb() was accidentally
>> pulled in here as a copy-paste from the GICv2 driver where it is sufficient
>> in practice.
>>
>> Did you spot this by code inspection, or did the DMB actually cause
>> observable failures? (trying to figure out whether or not this need to go
>> to -stable).
>>
>
> We've inspected the code because kernel was causing failures in scheduler/IPI_RESCHDULE.
> After some time of debugging, we landed in GIC driver and found that the issue was due
> to the DMB barrier.
OK. I've applied this with a cc: stable and Will's Ack.
> Side note, we're also missing synchronization barriers in GIC driver after writing some
> of the ICC_XXX system registers. I'm planning to post those changes for comments.
>
> e.g: gic_write_sgi1r(val) and gic_write_eoir(irqnr);
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] irqchip/gic-v3: Use wmb() instead of smb_wmb() in gic_raise_softirq()
2018-02-01 13:24 ` Marc Zyngier
@ 2018-02-01 13:42 ` Adam Wallis
0 siblings, 0 replies; 6+ messages in thread
From: Adam Wallis @ 2018-02-01 13:42 UTC (permalink / raw)
To: linux-arm-kernel
On 2/1/2018 8:24 AM, Marc Zyngier wrote:
> On 01/02/18 12:55, Shanker Donthineni wrote:
>> Hi Will, Thanks for your quick reply.
>>
>> On 02/01/2018 04:33 AM, Will Deacon wrote:
>>> Hi Shanker,
>>>
>>> On Wed, Jan 31, 2018 at 06:03:42PM -0600, Shanker Donthineni wrote:
>>>> A DMB instruction can be used to ensure the relative order of only
>>>> memory accesses before and after the barrier. Since writes to system
>>>> registers are not memory operations, barrier DMB is not sufficient
>>>> for observability of memory accesses that occur before ICC_SGI1R_EL1
>>>> writes.
>>>>
>>>> A DSB instruction ensures that no instructions that appear in program
>>>> order after the DSB instruction, can execute until the DSB instruction
>>>> has completed.
>>>>
>>>> Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org>
>>>> ---
>>>> drivers/irqchip/irq-gic-v3.c | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
>>>> index b56c3e2..980ae8e 100644
>>>> --- a/drivers/irqchip/irq-gic-v3.c
>>>> +++ b/drivers/irqchip/irq-gic-v3.c
>>>> @@ -688,7 +688,7 @@ static void gic_raise_softirq(const struct cpumask *mask, unsigned int irq)
>>>> * Ensure that stores to Normal memory are visible to the
>>>> * other CPUs before issuing the IPI.
>>>> */
>>>> - smp_wmb();
>>>> + wmb();
>>>
>>> I think this is the right thing to do and the smp_wmb() was accidentally
>>> pulled in here as a copy-paste from the GICv2 driver where it is sufficient
>>> in practice.
>>>
>>> Did you spot this by code inspection, or did the DMB actually cause
>>> observable failures? (trying to figure out whether or not this need to go
>>> to -stable).
>>>
>>
>> We've inspected the code because kernel was causing failures in scheduler/IPI_RESCHDULE.
>> After some time of debugging, we landed in GIC driver and found that the issue was due
>> to the DMB barrier.
>
> OK. I've applied this with a cc: stable and Will's Ack.
>
>> Side note, we're also missing synchronization barriers in GIC driver after writing some
>> of the ICC_XXX system registers. I'm planning to post those changes for comments.
>>
>> e.g: gic_write_sgi1r(val) and gic_write_eoir(irqnr);
>
> Thanks,
>
> M.
>
Tested-by: Adam Wallis <awallis@codeaurora.org>
--
Adam Wallis
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] irqchip/gic-v3: Use wmb() instead of smb_wmb() in gic_raise_softirq()
2018-02-01 0:03 [PATCH] irqchip/gic-v3: Use wmb() instead of smb_wmb() in gic_raise_softirq() Shanker Donthineni
2018-02-01 10:33 ` Will Deacon
@ 2022-02-18 11:27 ` Barry Song
1 sibling, 0 replies; 6+ messages in thread
From: Barry Song @ 2022-02-18 11:27 UTC (permalink / raw)
To: shankerd, will
Cc: kvmarm, linux-arm-kernel, linux-kernel, marc.zyngier, scampbel,
tglx, tspeier, vikrams
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -688,7 +688,7 @@ static void gic_raise_softirq(const struct cpumask *mask, unsigned int irq)
> * Ensure that stores to Normal memory are visible to the
> * other CPUs before issuing the IPI.
> */
> - smp_wmb();
> + wmb();
Sorry for waking up the old thread. isn't dsb(ishst) enough here as we only
need to guarantee the visibility of data to other CPUs in smp inner domain
before we send the ipi?
>
> for_each_cpu(cpu, mask) {
> u64 cluster_id = MPIDR_TO_SGI_CLUSTER_ID(cpu_logical_map(cpu));
Thanks
Barry
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-02-18 11:29 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-02-01 0:03 [PATCH] irqchip/gic-v3: Use wmb() instead of smb_wmb() in gic_raise_softirq() Shanker Donthineni
2018-02-01 10:33 ` Will Deacon
2018-02-01 12:55 ` Shanker Donthineni
2018-02-01 13:24 ` Marc Zyngier
2018-02-01 13:42 ` Adam Wallis
2022-02-18 11:27 ` Barry Song
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).