* Re: [PATCH 1/3] irqchip/gic-v3: Fix GICR_CTLR.RWP polling
2022-03-15 16:50 ` [PATCH 1/3] irqchip/gic-v3: Fix GICR_CTLR.RWP polling Marc Zyngier
@ 2022-03-16 14:51 ` Andre Przywara
2022-03-16 15:19 ` Marc Zyngier
2022-03-17 17:03 ` Lorenzo Pieralisi
` (3 subsequent siblings)
4 siblings, 1 reply; 7+ messages in thread
From: Andre Przywara @ 2022-03-16 14:51 UTC (permalink / raw)
To: Marc Zyngier
Cc: linux-kernel, Lorenzo Pieralisi, Thomas Gleixner, Eric Auger, stable
On Tue, 15 Mar 2022 16:50:32 +0000
Marc Zyngier <maz@kernel.org> wrote:
> It turns out that our polling of RWP is totally wrong when checking
> for it in the redistributors, as we test the *distributor* bit index,
> whereas it is a different bit number in the RDs... Oopsie boo.
>
> This is embarassing. Not only because it is wrong, but also because
> it took *8 years* to notice the blunder...
Indeed, I wonder why we didn't see issues before. I guess it's either
the UWP bit at position GICR_CTLR[31] having a similar implementation,
or the MMIO access alone providing enough delay for the writes to
finish.
Anyway:
> Just fix the damn thing.
>
> Fixes: 021f653791ad ("irqchip: gic-v3: Initial support for GICv3")
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> Cc: stable@vger.kernel.org
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
Cheers,
Andre
> ---
> drivers/irqchip/irq-gic-v3.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> index 5e935d97207d..736163d36b13 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -206,11 +206,11 @@ static inline void __iomem *gic_dist_base(struct irq_data *d)
> }
> }
>
> -static void gic_do_wait_for_rwp(void __iomem *base)
> +static void gic_do_wait_for_rwp(void __iomem *base, u32 bit)
> {
> u32 count = 1000000; /* 1s! */
>
> - while (readl_relaxed(base + GICD_CTLR) & GICD_CTLR_RWP) {
> + while (readl_relaxed(base + GICD_CTLR) & bit) {
> count--;
> if (!count) {
> pr_err_ratelimited("RWP timeout, gone fishing\n");
> @@ -224,13 +224,13 @@ static void gic_do_wait_for_rwp(void __iomem *base)
> /* Wait for completion of a distributor change */
> static void gic_dist_wait_for_rwp(void)
> {
> - gic_do_wait_for_rwp(gic_data.dist_base);
> + gic_do_wait_for_rwp(gic_data.dist_base, GICD_CTLR_RWP);
> }
>
> /* Wait for completion of a redistributor change */
> static void gic_redist_wait_for_rwp(void)
> {
> - gic_do_wait_for_rwp(gic_data_rdist_rd_base());
> + gic_do_wait_for_rwp(gic_data_rdist_rd_base(), GICR_CTLR_RWP);
> }
>
> #ifdef CONFIG_ARM64
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH 1/3] irqchip/gic-v3: Fix GICR_CTLR.RWP polling
2022-03-16 14:51 ` Andre Przywara
@ 2022-03-16 15:19 ` Marc Zyngier
0 siblings, 0 replies; 7+ messages in thread
From: Marc Zyngier @ 2022-03-16 15:19 UTC (permalink / raw)
To: Andre Przywara
Cc: linux-kernel, Lorenzo Pieralisi, Thomas Gleixner, Eric Auger, stable
On Wed, 16 Mar 2022 14:51:02 +0000,
Andre Przywara <andre.przywara@arm.com> wrote:
>
> On Tue, 15 Mar 2022 16:50:32 +0000
> Marc Zyngier <maz@kernel.org> wrote:
>
> > It turns out that our polling of RWP is totally wrong when checking
> > for it in the redistributors, as we test the *distributor* bit index,
> > whereas it is a different bit number in the RDs... Oopsie boo.
> >
> > This is embarassing. Not only because it is wrong, but also because
> > it took *8 years* to notice the blunder...
>
> Indeed, I wonder why we didn't see issues before. I guess it's either
> the UWP bit at position GICR_CTLR[31] having a similar implementation,
> or the MMIO access alone providing enough delay for the writes to
> finish.
Because we don't strictly need to wait. Most of the time, the
write will have taken place long before we can observe any effect of
it. And how often do we disable a SGI or a PPI? Almost never (the PMU
is the only one that I can think of).
> Anyway:
>
> > Just fix the damn thing.
> >
> > Fixes: 021f653791ad ("irqchip: gic-v3: Initial support for GICv3")
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > Cc: stable@vger.kernel.org
>
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/3] irqchip/gic-v3: Fix GICR_CTLR.RWP polling
2022-03-15 16:50 ` [PATCH 1/3] irqchip/gic-v3: Fix GICR_CTLR.RWP polling Marc Zyngier
2022-03-16 14:51 ` Andre Przywara
@ 2022-03-17 17:03 ` Lorenzo Pieralisi
2022-03-21 9:19 ` [irqchip: irq/irqchip-next] " irqchip-bot for Marc Zyngier
` (2 subsequent siblings)
4 siblings, 0 replies; 7+ messages in thread
From: Lorenzo Pieralisi @ 2022-03-17 17:03 UTC (permalink / raw)
To: Marc Zyngier
Cc: linux-kernel, Andre Przywara, Thomas Gleixner, Eric Auger, stable
On Tue, Mar 15, 2022 at 04:50:32PM +0000, Marc Zyngier wrote:
> It turns out that our polling of RWP is totally wrong when checking
> for it in the redistributors, as we test the *distributor* bit index,
> whereas it is a different bit number in the RDs... Oopsie boo.
>
> This is embarassing. Not only because it is wrong, but also because
> it took *8 years* to notice the blunder...
>
> Just fix the damn thing.
>
> Fixes: 021f653791ad ("irqchip: gic-v3: Initial support for GICv3")
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> Cc: stable@vger.kernel.org
> ---
> drivers/irqchip/irq-gic-v3.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
Reviewed-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> index 5e935d97207d..736163d36b13 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -206,11 +206,11 @@ static inline void __iomem *gic_dist_base(struct irq_data *d)
> }
> }
>
> -static void gic_do_wait_for_rwp(void __iomem *base)
> +static void gic_do_wait_for_rwp(void __iomem *base, u32 bit)
> {
> u32 count = 1000000; /* 1s! */
>
> - while (readl_relaxed(base + GICD_CTLR) & GICD_CTLR_RWP) {
> + while (readl_relaxed(base + GICD_CTLR) & bit) {
> count--;
> if (!count) {
> pr_err_ratelimited("RWP timeout, gone fishing\n");
> @@ -224,13 +224,13 @@ static void gic_do_wait_for_rwp(void __iomem *base)
> /* Wait for completion of a distributor change */
> static void gic_dist_wait_for_rwp(void)
> {
> - gic_do_wait_for_rwp(gic_data.dist_base);
> + gic_do_wait_for_rwp(gic_data.dist_base, GICD_CTLR_RWP);
> }
>
> /* Wait for completion of a redistributor change */
> static void gic_redist_wait_for_rwp(void)
> {
> - gic_do_wait_for_rwp(gic_data_rdist_rd_base());
> + gic_do_wait_for_rwp(gic_data_rdist_rd_base(), GICR_CTLR_RWP);
> }
>
> #ifdef CONFIG_ARM64
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 7+ messages in thread* [irqchip: irq/irqchip-next] irqchip/gic-v3: Fix GICR_CTLR.RWP polling
2022-03-15 16:50 ` [PATCH 1/3] irqchip/gic-v3: Fix GICR_CTLR.RWP polling Marc Zyngier
2022-03-16 14:51 ` Andre Przywara
2022-03-17 17:03 ` Lorenzo Pieralisi
@ 2022-03-21 9:19 ` irqchip-bot for Marc Zyngier
2022-03-21 14:07 ` irqchip-bot for Marc Zyngier
2022-04-05 15:39 ` [irqchip: irq/irqchip-fixes] " irqchip-bot for Marc Zyngier
4 siblings, 0 replies; 7+ messages in thread
From: irqchip-bot for Marc Zyngier @ 2022-03-21 9:19 UTC (permalink / raw)
To: linux-kernel
Cc: Marc Zyngier, stable, Andre Przywara, Lorenzo Pieralisi, tglx
The following commit has been merged into the irq/irqchip-next branch of irqchip:
Commit-ID: c114741827436ad1f1d465f3719f77b996ea6eca
Gitweb: https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms/c114741827436ad1f1d465f3719f77b996ea6eca
Author: Marc Zyngier <maz@kernel.org>
AuthorDate: Tue, 15 Mar 2022 16:50:32
Committer: Marc Zyngier <maz@kernel.org>
CommitterDate: Mon, 21 Mar 2022 09:17:13
irqchip/gic-v3: Fix GICR_CTLR.RWP polling
It turns out that our polling of RWP is totally wrong when checking
for it in the redistributors, as we test the *distributor* bit index,
whereas it is a different bit number in the RDs... Oopsie boo.
This is embarassing. Not only because it is wrong, but also because
it took *8 years* to notice the blunder...
Just fix the damn thing.
Fixes: 021f653791ad ("irqchip: gic-v3: Initial support for GICv3")
Signed-off-by: Marc Zyngier <maz@kernel.org>
Cc: stable@vger.kernel.org
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
Reviewed-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Link: https://lore.kernel.org/r/20220315165034.794482-2-maz@kernel.org
---
drivers/irqchip/irq-gic-v3.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index 0efe1a9..9b63165 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -206,11 +206,11 @@ static inline void __iomem *gic_dist_base(struct irq_data *d)
}
}
-static void gic_do_wait_for_rwp(void __iomem *base)
+static void gic_do_wait_for_rwp(void __iomem *base, u32 bit)
{
u32 count = 1000000; /* 1s! */
- while (readl_relaxed(base + GICD_CTLR) & GICD_CTLR_RWP) {
+ while (readl_relaxed(base + GICD_CTLR) & bit) {
count--;
if (!count) {
pr_err_ratelimited("RWP timeout, gone fishing\n");
@@ -224,13 +224,13 @@ static void gic_do_wait_for_rwp(void __iomem *base)
/* Wait for completion of a distributor change */
static void gic_dist_wait_for_rwp(void)
{
- gic_do_wait_for_rwp(gic_data.dist_base);
+ gic_do_wait_for_rwp(gic_data.dist_base, GICD_CTLR_RWP);
}
/* Wait for completion of a redistributor change */
static void gic_redist_wait_for_rwp(void)
{
- gic_do_wait_for_rwp(gic_data_rdist_rd_base());
+ gic_do_wait_for_rwp(gic_data_rdist_rd_base(), GICR_CTLR_RWP);
}
#ifdef CONFIG_ARM64
^ permalink raw reply related [flat|nested] 7+ messages in thread* [irqchip: irq/irqchip-next] irqchip/gic-v3: Fix GICR_CTLR.RWP polling
2022-03-15 16:50 ` [PATCH 1/3] irqchip/gic-v3: Fix GICR_CTLR.RWP polling Marc Zyngier
` (2 preceding siblings ...)
2022-03-21 9:19 ` [irqchip: irq/irqchip-next] " irqchip-bot for Marc Zyngier
@ 2022-03-21 14:07 ` irqchip-bot for Marc Zyngier
2022-04-05 15:39 ` [irqchip: irq/irqchip-fixes] " irqchip-bot for Marc Zyngier
4 siblings, 0 replies; 7+ messages in thread
From: irqchip-bot for Marc Zyngier @ 2022-03-21 14:07 UTC (permalink / raw)
To: linux-kernel
Cc: Marc Zyngier, stable, Andre Przywara, Lorenzo Pieralisi, tglx
The following commit has been merged into the irq/irqchip-next branch of irqchip:
Commit-ID: 49cdcea1b0772c29abb9468d82809933c718110e
Gitweb: https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms/49cdcea1b0772c29abb9468d82809933c718110e
Author: Marc Zyngier <maz@kernel.org>
AuthorDate: Tue, 15 Mar 2022 16:50:32
Committer: Marc Zyngier <maz@kernel.org>
CommitterDate: Mon, 21 Mar 2022 14:02:44
irqchip/gic-v3: Fix GICR_CTLR.RWP polling
It turns out that our polling of RWP is totally wrong when checking
for it in the redistributors, as we test the *distributor* bit index,
whereas it is a different bit number in the RDs... Oopsie boo.
This is embarassing. Not only because it is wrong, but also because
it took *8 years* to notice the blunder...
Just fix the damn thing.
Fixes: 021f653791ad ("irqchip: gic-v3: Initial support for GICv3")
Signed-off-by: Marc Zyngier <maz@kernel.org>
Cc: stable@vger.kernel.org
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
Reviewed-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Link: https://lore.kernel.org/r/20220315165034.794482-2-maz@kernel.org
---
drivers/irqchip/irq-gic-v3.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index 0efe1a9..9b63165 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -206,11 +206,11 @@ static inline void __iomem *gic_dist_base(struct irq_data *d)
}
}
-static void gic_do_wait_for_rwp(void __iomem *base)
+static void gic_do_wait_for_rwp(void __iomem *base, u32 bit)
{
u32 count = 1000000; /* 1s! */
- while (readl_relaxed(base + GICD_CTLR) & GICD_CTLR_RWP) {
+ while (readl_relaxed(base + GICD_CTLR) & bit) {
count--;
if (!count) {
pr_err_ratelimited("RWP timeout, gone fishing\n");
@@ -224,13 +224,13 @@ static void gic_do_wait_for_rwp(void __iomem *base)
/* Wait for completion of a distributor change */
static void gic_dist_wait_for_rwp(void)
{
- gic_do_wait_for_rwp(gic_data.dist_base);
+ gic_do_wait_for_rwp(gic_data.dist_base, GICD_CTLR_RWP);
}
/* Wait for completion of a redistributor change */
static void gic_redist_wait_for_rwp(void)
{
- gic_do_wait_for_rwp(gic_data_rdist_rd_base());
+ gic_do_wait_for_rwp(gic_data_rdist_rd_base(), GICR_CTLR_RWP);
}
#ifdef CONFIG_ARM64
^ permalink raw reply related [flat|nested] 7+ messages in thread* [irqchip: irq/irqchip-fixes] irqchip/gic-v3: Fix GICR_CTLR.RWP polling
2022-03-15 16:50 ` [PATCH 1/3] irqchip/gic-v3: Fix GICR_CTLR.RWP polling Marc Zyngier
` (3 preceding siblings ...)
2022-03-21 14:07 ` irqchip-bot for Marc Zyngier
@ 2022-04-05 15:39 ` irqchip-bot for Marc Zyngier
4 siblings, 0 replies; 7+ messages in thread
From: irqchip-bot for Marc Zyngier @ 2022-04-05 15:39 UTC (permalink / raw)
To: linux-kernel
Cc: Marc Zyngier, stable, Andre Przywara, Lorenzo Pieralisi, tglx
The following commit has been merged into the irq/irqchip-fixes branch of irqchip:
Commit-ID: 0df6664531a12cdd8fc873f0cac0dcb40243d3e9
Gitweb: https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms/0df6664531a12cdd8fc873f0cac0dcb40243d3e9
Author: Marc Zyngier <maz@kernel.org>
AuthorDate: Tue, 15 Mar 2022 16:50:32
Committer: Marc Zyngier <maz@kernel.org>
CommitterDate: Tue, 05 Apr 2022 16:33:13 +01:00
irqchip/gic-v3: Fix GICR_CTLR.RWP polling
It turns out that our polling of RWP is totally wrong when checking
for it in the redistributors, as we test the *distributor* bit index,
whereas it is a different bit number in the RDs... Oopsie boo.
This is embarassing. Not only because it is wrong, but also because
it took *8 years* to notice the blunder...
Just fix the damn thing.
Fixes: 021f653791ad ("irqchip: gic-v3: Initial support for GICv3")
Signed-off-by: Marc Zyngier <maz@kernel.org>
Cc: stable@vger.kernel.org
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
Reviewed-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Link: https://lore.kernel.org/r/20220315165034.794482-2-maz@kernel.org
---
drivers/irqchip/irq-gic-v3.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index 0efe1a9..9b63165 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -206,11 +206,11 @@ static inline void __iomem *gic_dist_base(struct irq_data *d)
}
}
-static void gic_do_wait_for_rwp(void __iomem *base)
+static void gic_do_wait_for_rwp(void __iomem *base, u32 bit)
{
u32 count = 1000000; /* 1s! */
- while (readl_relaxed(base + GICD_CTLR) & GICD_CTLR_RWP) {
+ while (readl_relaxed(base + GICD_CTLR) & bit) {
count--;
if (!count) {
pr_err_ratelimited("RWP timeout, gone fishing\n");
@@ -224,13 +224,13 @@ static void gic_do_wait_for_rwp(void __iomem *base)
/* Wait for completion of a distributor change */
static void gic_dist_wait_for_rwp(void)
{
- gic_do_wait_for_rwp(gic_data.dist_base);
+ gic_do_wait_for_rwp(gic_data.dist_base, GICD_CTLR_RWP);
}
/* Wait for completion of a redistributor change */
static void gic_redist_wait_for_rwp(void)
{
- gic_do_wait_for_rwp(gic_data_rdist_rd_base());
+ gic_do_wait_for_rwp(gic_data_rdist_rd_base(), GICR_CTLR_RWP);
}
#ifdef CONFIG_ARM64
^ permalink raw reply related [flat|nested] 7+ messages in thread