| Message ID | 1521124287-25009-1-git-send-email-shankerd@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 15 Mar 2018 09:31:27 -0500 Shanker Donthineni <shankerd@codeaurora.org> wrote: > The definition of the GICR_CTLR.RWP control bit was expanded to indicate > status of changing GICR_CTLR.EnableLPI from 1 to 0 is being in progress > or completed. Software must observe GICR_CTLR.RWP==0 after clearing > GICR_CTLR.EnableLPI from 1 to 0 and before writing GICR_PENDBASER and/or > GICR_PROPBASER, otherwise behavior is UNPREDICTABLE. > > Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org> > --- > Changes since v1: > -Moved LPI disable code to a seperate function as Marc suggested. > -Mark's suggestion to use readl_relaxed_poll_timeout() helper functions. > > drivers/irqchip/irq-gic-v3-its.c | 66 ++++++++++++++++++++++++++++++-------- > include/linux/irqchip/arm-gic-v3.h | 1 + > 2 files changed, 53 insertions(+), 14 deletions(-) > > diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c > index 1d3056f..cba71a7 100644 > --- a/drivers/irqchip/irq-gic-v3-its.c > +++ b/drivers/irqchip/irq-gic-v3-its.c > @@ -33,6 +33,8 @@ > #include <linux/of_platform.h> > #include <linux/percpu.h> > #include <linux/slab.h> > +#include <linux/time64.h> > +#include <linux/iopoll.h> > > #include <linux/irqchip.h> > #include <linux/irqchip/arm-gic-v3.h> > @@ -1875,16 +1877,6 @@ static void its_cpu_init_lpis(void) > gic_data_rdist()->pend_page = pend_page; > } > > - /* Disable LPIs */ > - val = readl_relaxed(rbase + GICR_CTLR); > - val &= ~GICR_CTLR_ENABLE_LPIS; > - writel_relaxed(val, rbase + GICR_CTLR); > - > - /* > - * Make sure any change to the table is observable by the GIC. > - */ > - dsb(sy); > - > /* set PROPBASE */ > val = (page_to_phys(gic_rdists->prop_page) | > GICR_PROPBASER_InnerShareable | > @@ -3288,13 +3280,59 @@ static bool gic_rdists_supports_plpis(void) > return !!(gic_read_typer(gic_data_rdist_rd_base() + GICR_TYPER) & GICR_TYPER_PLPIS); > } > > +static int redist_disable_lpis(void) > +{ > + void __iomem *rbase = gic_data_rdist_rd_base(); > + u64 val; > + int ret; > + > + if (!gic_rdists_supports_plpis()) { > + pr_info("CPU%d: LPIs not supported\n", smp_processor_id()); > + return -ENXIO; > + } > + > + val = readl_relaxed(rbase + GICR_CTLR); > + if (!(val & GICR_CTLR_ENABLE_LPIS)) > + return 0; > + > + /* Disable LPIs */ > + val &= ~GICR_CTLR_ENABLE_LPIS; > + writel_relaxed(val, rbase + GICR_CTLR); > + > + /* Make sure any change to GICR_CTLR is observable by the GIC */ > + dsb(sy); > + > + /* Wait for GICR_CTLR.GICR_CTLR_ENABLE_LPIS==0 or timeout */ > + ret = readl_relaxed_poll_timeout_atomic(rbase + GICR_CTLR, val, > + !(val & GICR_CTLR_ENABLE_LPIS), 1, > + USEC_PER_SEC); Why do you need to poll on EnableLPIs? It is supposed to be immediately written. It seems to me that the sequence should be: - Check EnableLPis - If set to 1, scream about bootloader being braindead - Write EnableLPIs to 0 - Read back EnableLpis: - If still set 1, bail out, we're doomed. Scream about the HW being braindead - Poll RWP - If timeout, bail out, the HW has locked up. Scream again. I think this should cover the whole thing. An alternative would be to place the poll between the write and read-back, but that doesn't change anything. > + if (ret) { > + pr_err("CPU%d: Failed to disable LPIs\n", smp_processor_id()); > + return -EBUSY; > + } > + > + /* Wait for GICR_CTLR.RWP==0 or timeout */ > + ret = readl_relaxed_poll_timeout_atomic(rbase + GICR_CTLR, val, > + !(val & GICR_CTLR_RWP), 1, > + USEC_PER_SEC); > + if (ret) { > + pr_err("CPU%d: Failed to observe RWP==0 after enabling LPIs\n", > + smp_processor_id()); > + return -EBUSY; > + } > + > + return 0; > +} > + > int its_cpu_init(void) > { > + int ret; > + > if (!list_empty(&its_nodes)) { nit: move 'ret' here. > - if (!gic_rdists_supports_plpis()) { > - pr_info("CPU%d: LPIs not supported\n", smp_processor_id()); > - return -ENXIO; > - } > + ret = redist_disable_lpis(); > + if (ret) > + return ret; > + > its_cpu_init_lpis(); > its_cpu_init_collection(); > } > diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h > index c00c4c33..4d5fb60 100644 > --- a/include/linux/irqchip/arm-gic-v3.h > +++ b/include/linux/irqchip/arm-gic-v3.h > @@ -106,6 +106,7 @@ > #define GICR_PIDR2 GICD_PIDR2 > > #define GICR_CTLR_ENABLE_LPIS (1UL << 0) > +#define GICR_CTLR_RWP (1UL << 3) > > #define GICR_TYPER_CPU_NUMBER(r) (((r) >> 8) & 0xffff) > Thanks, M.
Hi Marc, On 03/17/2018 08:34 AM, Marc Zyngier wrote: > On Thu, 15 Mar 2018 09:31:27 -0500 > Shanker Donthineni <shankerd@codeaurora.org> wrote: > >> The definition of the GICR_CTLR.RWP control bit was expanded to indicate >> status of changing GICR_CTLR.EnableLPI from 1 to 0 is being in progress >> or completed. Software must observe GICR_CTLR.RWP==0 after clearing >> GICR_CTLR.EnableLPI from 1 to 0 and before writing GICR_PENDBASER and/or >> GICR_PROPBASER, otherwise behavior is UNPREDICTABLE. >> >> Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org> >> --- >> Changes since v1: >> -Moved LPI disable code to a seperate function as Marc suggested. >> -Mark's suggestion to use readl_relaxed_poll_timeout() helper functions. >> >> drivers/irqchip/irq-gic-v3-its.c | 66 ++++++++++++++++++++++++++++++-------- >> include/linux/irqchip/arm-gic-v3.h | 1 + >> 2 files changed, 53 insertions(+), 14 deletions(-) >> >> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c >> index 1d3056f..cba71a7 100644 >> --- a/drivers/irqchip/irq-gic-v3-its.c >> +++ b/drivers/irqchip/irq-gic-v3-its.c >> @@ -33,6 +33,8 @@ >> #include <linux/of_platform.h> >> #include <linux/percpu.h> >> #include <linux/slab.h> >> +#include <linux/time64.h> >> +#include <linux/iopoll.h> >> >> #include <linux/irqchip.h> >> #include <linux/irqchip/arm-gic-v3.h> >> @@ -1875,16 +1877,6 @@ static void its_cpu_init_lpis(void) >> gic_data_rdist()->pend_page = pend_page; >> } >> >> - /* Disable LPIs */ >> - val = readl_relaxed(rbase + GICR_CTLR); >> - val &= ~GICR_CTLR_ENABLE_LPIS; >> - writel_relaxed(val, rbase + GICR_CTLR); >> - >> - /* >> - * Make sure any change to the table is observable by the GIC. >> - */ >> - dsb(sy); >> - >> /* set PROPBASE */ >> val = (page_to_phys(gic_rdists->prop_page) | >> GICR_PROPBASER_InnerShareable | >> @@ -3288,13 +3280,59 @@ static bool gic_rdists_supports_plpis(void) >> return !!(gic_read_typer(gic_data_rdist_rd_base() + GICR_TYPER) & GICR_TYPER_PLPIS); >> } >> >> +static int redist_disable_lpis(void) >> +{ >> + void __iomem *rbase = gic_data_rdist_rd_base(); >> + u64 val; >> + int ret; >> + >> + if (!gic_rdists_supports_plpis()) { >> + pr_info("CPU%d: LPIs not supported\n", smp_processor_id()); >> + return -ENXIO; >> + } >> + >> + val = readl_relaxed(rbase + GICR_CTLR); >> + if (!(val & GICR_CTLR_ENABLE_LPIS)) >> + return 0; >> + >> + /* Disable LPIs */ >> + val &= ~GICR_CTLR_ENABLE_LPIS; >> + writel_relaxed(val, rbase + GICR_CTLR); >> + >> + /* Make sure any change to GICR_CTLR is observable by the GIC */ >> + dsb(sy); >> + >> + /* Wait for GICR_CTLR.GICR_CTLR_ENABLE_LPIS==0 or timeout */ >> + ret = readl_relaxed_poll_timeout_atomic(rbase + GICR_CTLR, val, >> + !(val & GICR_CTLR_ENABLE_LPIS), 1, >> + USEC_PER_SEC); > > Why do you need to poll on EnableLPIs? It is supposed to be immediately > written. It seems to me that the sequence should be: > Agree we don't need to poll EnableLPIs here. I'll remove. > - Check EnableLPis > - If set to 1, scream about bootloader being braindead > - Write EnableLPIs to 0 > - Read back EnableLpis: > - If still set 1, bail out, we're doomed. Scream about the HW being > braindead > - Poll RWP > - If timeout, bail out, the HW has locked up. Scream again. > > I think this should cover the whole thing. An alternative would be to > place the poll between the write and read-back, but that doesn't change > anything. > I'll prefer your second approach. Please comment on the below change before I post v3. static int redist_disable_lpis(void) { void __iomem *rbase = gic_data_rdist_rd_base(); u64 val; int ret; if (!gic_rdists_supports_plpis()) { pr_info("CPU%d: LPIs not supported\n", smp_processor_id()); return -ENXIO; } if (!(readl(rbase + GICR_CTLR) & GICR_CTLR_ENABLE_LPIS)) return 0; pr_warn("CPU%d: Trying to disable LPIs\n", smp_processor_id()); val &= ~GICR_CTLR_ENABLE_LPIS; writel_relaxed(val, rbase + GICR_CTLR); /* Make sure any change to GICR_CTLR is observable by the GIC */ dsb(sy); /** * Software must observe RWP==0 after clearing GICR_CTLR.EnableLPIs * from 1 to 0 before programming GICR_PEND{PROP}BASER registers. * Bail out the driver probe() in case of timeout. */ ret = readl_relaxed_poll_timeout_atomic(rbase + GICR_CTLR, val, !(val & GICR_CTLR_RWP), 1, USEC_PER_SEC); if (ret) { pr_err("CPU%d: EnableLPIs not observed\n", smp_processor_id()); return ret; } /** * After it has been written to 1, it is IMPLEMENTATION DEFINED whether * the bit GICR_CTLR.EnableLPI becomes RES1 or can be cleared to 0. * Bail out the driver probe() on systems where it's RES1. */ if (readl_relaxed(rbase + GICR_CTLR) & GICR_CTLR_ENABLE_LPIS) { pr_err("CPU%d: Failed to disable LPIs\n", smp_processor_id()); return -EPERM; } return 0; } >> + if (ret) { >> + pr_err("CPU%d: Failed to disable LPIs\n", smp_processor_id()); >> + return -EBUSY; >> + } >> + >> + /* Wait for GICR_CTLR.RWP==0 or timeout */ >> + ret = readl_relaxed_poll_timeout_atomic(rbase + GICR_CTLR, val, >> + !(val & GICR_CTLR_RWP), 1, >> + USEC_PER_SEC); >> + if (ret) { >> + pr_err("CPU%d: Failed to observe RWP==0 after enabling LPIs\n", >> + smp_processor_id()); >> + return -EBUSY; >> + } >> + >> + return 0; >> +} >> + >> int its_cpu_init(void) >> { >> + int ret; >> + >> if (!list_empty(&its_nodes)) { > > nit: move 'ret' here. > >> - if (!gic_rdists_supports_plpis()) { >> - pr_info("CPU%d: LPIs not supported\n", smp_processor_id()); >> - return -ENXIO; >> - } >> + ret = redist_disable_lpis(); >> + if (ret) >> + return ret; >> + >> its_cpu_init_lpis(); >> its_cpu_init_collection(); >> } >> diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h >> index c00c4c33..4d5fb60 100644 >> --- a/include/linux/irqchip/arm-gic-v3.h >> +++ b/include/linux/irqchip/arm-gic-v3.h >> @@ -106,6 +106,7 @@ >> #define GICR_PIDR2 GICD_PIDR2 >> >> #define GICR_CTLR_ENABLE_LPIS (1UL << 0) >> +#define GICR_CTLR_RWP (1UL << 3) >> >> #define GICR_TYPER_CPU_NUMBER(r) (((r) >> 8) & 0xffff) >> > > Thanks, > > M. >
On Sat, 17 Mar 2018 16:11:06 +0000, Shanker Donthineni wrote: > > Hi Marc, > > On 03/17/2018 08:34 AM, Marc Zyngier wrote: > > On Thu, 15 Mar 2018 09:31:27 -0500 > > Shanker Donthineni <shankerd@codeaurora.org> wrote: > > > >> The definition of the GICR_CTLR.RWP control bit was expanded to indicate > >> status of changing GICR_CTLR.EnableLPI from 1 to 0 is being in progress > >> or completed. Software must observe GICR_CTLR.RWP==0 after clearing > >> GICR_CTLR.EnableLPI from 1 to 0 and before writing GICR_PENDBASER and/or > >> GICR_PROPBASER, otherwise behavior is UNPREDICTABLE. > >> > >> Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org> > >> --- > >> Changes since v1: > >> -Moved LPI disable code to a seperate function as Marc suggested. > >> -Mark's suggestion to use readl_relaxed_poll_timeout() helper functions. > >> > >> drivers/irqchip/irq-gic-v3-its.c | 66 ++++++++++++++++++++++++++++++-------- > >> include/linux/irqchip/arm-gic-v3.h | 1 + > >> 2 files changed, 53 insertions(+), 14 deletions(-) > >> > >> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c > >> index 1d3056f..cba71a7 100644 > >> --- a/drivers/irqchip/irq-gic-v3-its.c > >> +++ b/drivers/irqchip/irq-gic-v3-its.c > >> @@ -33,6 +33,8 @@ > >> #include <linux/of_platform.h> > >> #include <linux/percpu.h> > >> #include <linux/slab.h> > >> +#include <linux/time64.h> > >> +#include <linux/iopoll.h> > >> > >> #include <linux/irqchip.h> > >> #include <linux/irqchip/arm-gic-v3.h> > >> @@ -1875,16 +1877,6 @@ static void its_cpu_init_lpis(void) > >> gic_data_rdist()->pend_page = pend_page; > >> } > >> > >> - /* Disable LPIs */ > >> - val = readl_relaxed(rbase + GICR_CTLR); > >> - val &= ~GICR_CTLR_ENABLE_LPIS; > >> - writel_relaxed(val, rbase + GICR_CTLR); > >> - > >> - /* > >> - * Make sure any change to the table is observable by the GIC. > >> - */ > >> - dsb(sy); > >> - > >> /* set PROPBASE */ > >> val = (page_to_phys(gic_rdists->prop_page) | > >> GICR_PROPBASER_InnerShareable | > >> @@ -3288,13 +3280,59 @@ static bool gic_rdists_supports_plpis(void) > >> return !!(gic_read_typer(gic_data_rdist_rd_base() + GICR_TYPER) & GICR_TYPER_PLPIS); > >> } > >> > >> +static int redist_disable_lpis(void) > >> +{ > >> + void __iomem *rbase = gic_data_rdist_rd_base(); > >> + u64 val; > >> + int ret; > >> + > >> + if (!gic_rdists_supports_plpis()) { > >> + pr_info("CPU%d: LPIs not supported\n", smp_processor_id()); > >> + return -ENXIO; > >> + } > >> + > >> + val = readl_relaxed(rbase + GICR_CTLR); > >> + if (!(val & GICR_CTLR_ENABLE_LPIS)) > >> + return 0; > >> + > >> + /* Disable LPIs */ > >> + val &= ~GICR_CTLR_ENABLE_LPIS; > >> + writel_relaxed(val, rbase + GICR_CTLR); > >> + > >> + /* Make sure any change to GICR_CTLR is observable by the GIC */ > >> + dsb(sy); > >> + > >> + /* Wait for GICR_CTLR.GICR_CTLR_ENABLE_LPIS==0 or timeout */ > >> + ret = readl_relaxed_poll_timeout_atomic(rbase + GICR_CTLR, val, > >> + !(val & GICR_CTLR_ENABLE_LPIS), 1, > >> + USEC_PER_SEC); > > > > Why do you need to poll on EnableLPIs? It is supposed to be immediately > > written. It seems to me that the sequence should be: > > > > Agree we don't need to poll EnableLPIs here. I'll remove. > > > - Check EnableLPis > > - If set to 1, scream about bootloader being braindead > > - Write EnableLPIs to 0 > > - Read back EnableLpis: > > - If still set 1, bail out, we're doomed. Scream about the HW being > > braindead > > - Poll RWP > > - If timeout, bail out, the HW has locked up. Scream again. > > > > I think this should cover the whole thing. An alternative would be to > > place the poll between the write and read-back, but that doesn't change > > anything. > > > > I'll prefer your second approach. Please comment on the below change before > I post v3. > > static int redist_disable_lpis(void) > { > void __iomem *rbase = gic_data_rdist_rd_base(); > u64 val; > int ret; > > if (!gic_rdists_supports_plpis()) { > pr_info("CPU%d: LPIs not supported\n", smp_processor_id()); > return -ENXIO; > } > > if (!(readl(rbase + GICR_CTLR) & GICR_CTLR_ENABLE_LPIS)) > return 0; > > pr_warn("CPU%d: Trying to disable LPIs\n", smp_processor_id()); I'd be much more clear: pr_warn("CPU%d: Booted with LPIs enabled, memory probably corrupted\n", smp_processor_id()); add_taint(TAINT_CRAP, LOCKDEP_STILL_OK); > val &= ~GICR_CTLR_ENABLE_LPIS; > writel_relaxed(val, rbase + GICR_CTLR); > > /* Make sure any change to GICR_CTLR is observable by the GIC */ > dsb(sy); > > /** > * Software must observe RWP==0 after clearing GICR_CTLR.EnableLPIs > * from 1 to 0 before programming GICR_PEND{PROP}BASER registers. > * Bail out the driver probe() in case of timeout. > */ > ret = readl_relaxed_poll_timeout_atomic(rbase + GICR_CTLR, val, > !(val & GICR_CTLR_RWP), 1, > USEC_PER_SEC); > if (ret) { > pr_err("CPU%d: EnableLPIs not observed\n", smp_processor_id()); Let's call a spade a spade: the redistributor hasn't reacted after a whole second, it has probably locked-up. Just say so, because your message doesn't mean much to me. > return ret; > } > > /** > * After it has been written to 1, it is IMPLEMENTATION DEFINED whether > * the bit GICR_CTLR.EnableLPI becomes RES1 or can be cleared to 0. > * Bail out the driver probe() on systems where it's RES1. > */ > if (readl_relaxed(rbase + GICR_CTLR) & GICR_CTLR_ENABLE_LPIS) { > pr_err("CPU%d: Failed to disable LPIs\n", smp_processor_id()); > return -EPERM; EPERM? EBUSY was a better approximation... Thanks, M.
Hi Marc, On 03/17/2018 12:13 PM, Marc Zyngier wrote: > On Sat, 17 Mar 2018 16:11:06 +0000, > Shanker Donthineni wrote: >> >> Hi Marc, >> >> On 03/17/2018 08:34 AM, Marc Zyngier wrote: >>> On Thu, 15 Mar 2018 09:31:27 -0500 >>> Shanker Donthineni <shankerd@codeaurora.org> wrote: >>> >>>> The definition of the GICR_CTLR.RWP control bit was expanded to indicate >>>> status of changing GICR_CTLR.EnableLPI from 1 to 0 is being in progress >>>> or completed. Software must observe GICR_CTLR.RWP==0 after clearing >>>> GICR_CTLR.EnableLPI from 1 to 0 and before writing GICR_PENDBASER and/or >>>> GICR_PROPBASER, otherwise behavior is UNPREDICTABLE. >>>> >>>> Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org> >>>> --- >>>> Changes since v1: >>>> -Moved LPI disable code to a seperate function as Marc suggested. >>>> -Mark's suggestion to use readl_relaxed_poll_timeout() helper functions. >>>> >>>> drivers/irqchip/irq-gic-v3-its.c | 66 ++++++++++++++++++++++++++++++-------- >>>> include/linux/irqchip/arm-gic-v3.h | 1 + >>>> 2 files changed, 53 insertions(+), 14 deletions(-) >>>> >>>> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c >>>> index 1d3056f..cba71a7 100644 >>>> --- a/drivers/irqchip/irq-gic-v3-its.c >>>> +++ b/drivers/irqchip/irq-gic-v3-its.c >>>> @@ -33,6 +33,8 @@ >>>> #include <linux/of_platform.h> >>>> #include <linux/percpu.h> >>>> #include <linux/slab.h> >>>> +#include <linux/time64.h> >>>> +#include <linux/iopoll.h> >>>> >>>> #include <linux/irqchip.h> >>>> #include <linux/irqchip/arm-gic-v3.h> >>>> @@ -1875,16 +1877,6 @@ static void its_cpu_init_lpis(void) >>>> gic_data_rdist()->pend_page = pend_page; >>>> } >>>> >>>> - /* Disable LPIs */ >>>> - val = readl_relaxed(rbase + GICR_CTLR); >>>> - val &= ~GICR_CTLR_ENABLE_LPIS; >>>> - writel_relaxed(val, rbase + GICR_CTLR); >>>> - >>>> - /* >>>> - * Make sure any change to the table is observable by the GIC. >>>> - */ >>>> - dsb(sy); >>>> - >>>> /* set PROPBASE */ >>>> val = (page_to_phys(gic_rdists->prop_page) | >>>> GICR_PROPBASER_InnerShareable | >>>> @@ -3288,13 +3280,59 @@ static bool gic_rdists_supports_plpis(void) >>>> return !!(gic_read_typer(gic_data_rdist_rd_base() + GICR_TYPER) & GICR_TYPER_PLPIS); >>>> } >>>> >>>> +static int redist_disable_lpis(void) >>>> +{ >>>> + void __iomem *rbase = gic_data_rdist_rd_base(); >>>> + u64 val; >>>> + int ret; >>>> + >>>> + if (!gic_rdists_supports_plpis()) { >>>> + pr_info("CPU%d: LPIs not supported\n", smp_processor_id()); >>>> + return -ENXIO; >>>> + } >>>> + >>>> + val = readl_relaxed(rbase + GICR_CTLR); >>>> + if (!(val & GICR_CTLR_ENABLE_LPIS)) >>>> + return 0; >>>> + >>>> + /* Disable LPIs */ >>>> + val &= ~GICR_CTLR_ENABLE_LPIS; >>>> + writel_relaxed(val, rbase + GICR_CTLR); >>>> + >>>> + /* Make sure any change to GICR_CTLR is observable by the GIC */ >>>> + dsb(sy); >>>> + >>>> + /* Wait for GICR_CTLR.GICR_CTLR_ENABLE_LPIS==0 or timeout */ >>>> + ret = readl_relaxed_poll_timeout_atomic(rbase + GICR_CTLR, val, >>>> + !(val & GICR_CTLR_ENABLE_LPIS), 1, >>>> + USEC_PER_SEC); >>> >>> Why do you need to poll on EnableLPIs? It is supposed to be immediately >>> written. It seems to me that the sequence should be: >>> >> >> Agree we don't need to poll EnableLPIs here. I'll remove. >> >>> - Check EnableLPis >>> - If set to 1, scream about bootloader being braindead >>> - Write EnableLPIs to 0 >>> - Read back EnableLpis: >>> - If still set 1, bail out, we're doomed. Scream about the HW being >>> braindead >>> - Poll RWP >>> - If timeout, bail out, the HW has locked up. Scream again. >>> >>> I think this should cover the whole thing. An alternative would be to >>> place the poll between the write and read-back, but that doesn't change >>> anything. >>> >> >> I'll prefer your second approach. Please comment on the below change before >> I post v3. >> >> static int redist_disable_lpis(void) >> { >> void __iomem *rbase = gic_data_rdist_rd_base(); >> u64 val; >> int ret; >> >> if (!gic_rdists_supports_plpis()) { >> pr_info("CPU%d: LPIs not supported\n", smp_processor_id()); >> return -ENXIO; >> } >> >> if (!(readl(rbase + GICR_CTLR) & GICR_CTLR_ENABLE_LPIS)) >> return 0; >> >> pr_warn("CPU%d: Trying to disable LPIs\n", smp_processor_id()); > > I'd be much more clear: > > pr_warn("CPU%d: Booted with LPIs enabled, memory probably corrupted\n", > smp_processor_id()); > add_taint(TAINT_CRAP, LOCKDEP_STILL_OK); > >> val &= ~GICR_CTLR_ENABLE_LPIS; >> writel_relaxed(val, rbase + GICR_CTLR); >> >> /* Make sure any change to GICR_CTLR is observable by the GIC */ >> dsb(sy); >> >> /** >> * Software must observe RWP==0 after clearing GICR_CTLR.EnableLPIs >> * from 1 to 0 before programming GICR_PEND{PROP}BASER registers. >> * Bail out the driver probe() in case of timeout. >> */ >> ret = readl_relaxed_poll_timeout_atomic(rbase + GICR_CTLR, val, >> !(val & GICR_CTLR_RWP), 1, >> USEC_PER_SEC); >> if (ret) { >> pr_err("CPU%d: EnableLPIs not observed\n", smp_processor_id()); > > Let's call a spade a spade: the redistributor hasn't reacted after a > whole second, it has probably locked-up. Just say so, because your > message doesn't mean much to me. > Sure I'll change, and also readX_relaxed_poll_XXX() helper macros aren't usable during GIC/ITS probe time, noticed kernel crashes when I tested KEXEC/KDUMP kernel on QDF2400. I believe this's because of the timer subsystem "timer_init()" is initialized after calling init_IRQ(). I've to go back to v1 change for handling timeout. while (readl_relaxed(rbase + GICR_CTLR) & GICR_CTLR_RWP) { if (!timeout) { pr_err("CPU%d: Failed to observe RWP==0 after disabling LPIs\n", smp_processor_id()); return -ETIMEDOUT; } udelay(1); timeout--; cpu_relax(); } >> return ret; >> } >> >> /** >> * After it has been written to 1, it is IMPLEMENTATION DEFINED whether >> * the bit GICR_CTLR.EnableLPI becomes RES1 or can be cleared to 0. >> * Bail out the driver probe() on systems where it's RES1. >> */ >> if (readl_relaxed(rbase + GICR_CTLR) & GICR_CTLR_ENABLE_LPIS) { >> pr_err("CPU%d: Failed to disable LPIs\n", smp_processor_id()); >> return -EPERM; > > EPERM? EBUSY was a better approximation... > > Thanks, > > M. >
diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c index 1d3056f..cba71a7 100644 --- a/drivers/irqchip/irq-gic-v3-its.c +++ b/drivers/irqchip/irq-gic-v3-its.c @@ -33,6 +33,8 @@ #include <linux/of_platform.h> #include <linux/percpu.h> #include <linux/slab.h> +#include <linux/time64.h> +#include <linux/iopoll.h> #include <linux/irqchip.h> #include <linux/irqchip/arm-gic-v3.h> @@ -1875,16 +1877,6 @@ static void its_cpu_init_lpis(void) gic_data_rdist()->pend_page = pend_page; } - /* Disable LPIs */ - val = readl_relaxed(rbase + GICR_CTLR); - val &= ~GICR_CTLR_ENABLE_LPIS; - writel_relaxed(val, rbase + GICR_CTLR); - - /* - * Make sure any change to the table is observable by the GIC. - */ - dsb(sy); - /* set PROPBASE */ val = (page_to_phys(gic_rdists->prop_page) | GICR_PROPBASER_InnerShareable | @@ -3288,13 +3280,59 @@ static bool gic_rdists_supports_plpis(void) return !!(gic_read_typer(gic_data_rdist_rd_base() + GICR_TYPER) & GICR_TYPER_PLPIS); } +static int redist_disable_lpis(void) +{ + void __iomem *rbase = gic_data_rdist_rd_base(); + u64 val; + int ret; + + if (!gic_rdists_supports_plpis()) { + pr_info("CPU%d: LPIs not supported\n", smp_processor_id()); + return -ENXIO; + } + + val = readl_relaxed(rbase + GICR_CTLR); + if (!(val & GICR_CTLR_ENABLE_LPIS)) + return 0; + + /* Disable LPIs */ + val &= ~GICR_CTLR_ENABLE_LPIS; + writel_relaxed(val, rbase + GICR_CTLR); + + /* Make sure any change to GICR_CTLR is observable by the GIC */ + dsb(sy); + + /* Wait for GICR_CTLR.GICR_CTLR_ENABLE_LPIS==0 or timeout */ + ret = readl_relaxed_poll_timeout_atomic(rbase + GICR_CTLR, val, + !(val & GICR_CTLR_ENABLE_LPIS), 1, + USEC_PER_SEC); + if (ret) { + pr_err("CPU%d: Failed to disable LPIs\n", smp_processor_id()); + return -EBUSY; + } + + /* Wait for GICR_CTLR.RWP==0 or timeout */ + ret = readl_relaxed_poll_timeout_atomic(rbase + GICR_CTLR, val, + !(val & GICR_CTLR_RWP), 1, + USEC_PER_SEC); + if (ret) { + pr_err("CPU%d: Failed to observe RWP==0 after enabling LPIs\n", + smp_processor_id()); + return -EBUSY; + } + + return 0; +} + int its_cpu_init(void) { + int ret; + if (!list_empty(&its_nodes)) { - if (!gic_rdists_supports_plpis()) { - pr_info("CPU%d: LPIs not supported\n", smp_processor_id()); - return -ENXIO; - } + ret = redist_disable_lpis(); + if (ret) + return ret; + its_cpu_init_lpis(); its_cpu_init_collection(); } diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h index c00c4c33..4d5fb60 100644 --- a/include/linux/irqchip/arm-gic-v3.h +++ b/include/linux/irqchip/arm-gic-v3.h @@ -106,6 +106,7 @@ #define GICR_PIDR2 GICD_PIDR2 #define GICR_CTLR_ENABLE_LPIS (1UL << 0) +#define GICR_CTLR_RWP (1UL << 3) #define GICR_TYPER_CPU_NUMBER(r) (((r) >> 8) & 0xffff)
The definition of the GICR_CTLR.RWP control bit was expanded to indicate status of changing GICR_CTLR.EnableLPI from 1 to 0 is being in progress or completed. Software must observe GICR_CTLR.RWP==0 after clearing GICR_CTLR.EnableLPI from 1 to 0 and before writing GICR_PENDBASER and/or GICR_PROPBASER, otherwise behavior is UNPREDICTABLE. Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org> --- Changes since v1: -Moved LPI disable code to a seperate function as Marc suggested. -Mark's suggestion to use readl_relaxed_poll_timeout() helper functions. drivers/irqchip/irq-gic-v3-its.c | 66 ++++++++++++++++++++++++++++++-------- include/linux/irqchip/arm-gic-v3.h | 1 + 2 files changed, 53 insertions(+), 14 deletions(-)