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 <[email protected]>
Cc: [email protected]
---
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
--
2.34.1
On Wed, 16 Mar 2022 14:51:02 +0000,
Andre Przywara <[email protected]> wrote:
>
> On Tue, 15 Mar 2022 16:50:32 +0000
> Marc Zyngier <[email protected]> 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 <[email protected]>
> > Cc: [email protected]
>
> Reviewed-by: Andre Przywara <[email protected]>
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
On Tue, 15 Mar 2022 16:50:32 +0000
Marc Zyngier <[email protected]> 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 <[email protected]>
> Cc: [email protected]
Reviewed-by: Andre Przywara <[email protected]>
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
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 <[email protected]>
> Cc: [email protected]
> ---
> drivers/irqchip/irq-gic-v3.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
Reviewed-by: Lorenzo Pieralisi <[email protected]>
> 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
>
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 <[email protected]>
AuthorDate: Tue, 15 Mar 2022 16:50:32
Committer: Marc Zyngier <[email protected]>
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 <[email protected]>
Cc: [email protected]
Reviewed-by: Andre Przywara <[email protected]>
Reviewed-by: Lorenzo Pieralisi <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
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
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 <[email protected]>
AuthorDate: Tue, 15 Mar 2022 16:50:32
Committer: Marc Zyngier <[email protected]>
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 <[email protected]>
Cc: [email protected]
Reviewed-by: Andre Przywara <[email protected]>
Reviewed-by: Lorenzo Pieralisi <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
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
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 <[email protected]>
AuthorDate: Tue, 15 Mar 2022 16:50:32
Committer: Marc Zyngier <[email protected]>
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 <[email protected]>
Cc: [email protected]
Reviewed-by: Andre Przywara <[email protected]>
Reviewed-by: Lorenzo Pieralisi <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
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