Message ID | 20210503215131.2829257-2-dann.frazier@canonical.com |
---|---|
State | New |
Headers | show |
Series | Fix boot crash on ThunderX systems in ACPI mode | expand |
On 03/05/2021 17:51, dann frazier wrote: > From: Marc Zyngier <maz@kernel.org> > > BugLink: https://bugs.launchpad.net/bugs/1925075 > > When failing the driver probe because of invalid firmware properties, > the GTDT driver unmaps the interrupt that it mapped earlier. > > However, it never checks whether the mapping of the interrupt actially > succeeded. Even more, should the firmware report an illegal interrupt > number that overlaps with the GIC SGI range, this can result in an > IPI being unmapped, and subsequent fireworks (as reported by Dann > Frazier). > > Rework the driver to have a slightly saner behaviour and actually > check whether the interrupt has been mapped before unmapping things. > > Reported-by: dann frazier <dann.frazier@canonical.com> > Fixes: ca9ae5ec4ef0 ("acpi/arm64: Add SBSA Generic Watchdog support in GTDT driver") > Signed-off-by: Marc Zyngier <maz@kernel.org> > Link: https://lore.kernel.org/r/YH87dtTfwYgavusz@xps13.dannf > Cc: <stable@vger.kernel.org> > Cc: Fu Wei <wefu@redhat.com> > Reviewed-by: Sudeep Holla <sudeep.holla@arm.com> > Tested-by: dann frazier <dann.frazier@canonical.com> > Tested-by: Hanjun Guo <guohanjun@huawei.com> > Reviewed-by: Hanjun Guo <guohanjun@huawei.com> > Reviewed-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > Link: https://lore.kernel.org/r/20210421164317.1718831-2-maz@kernel.org > Signed-off-by: Catalin Marinas <catalin.marinas@arm.com> > (cherry picked from commit 1ecd5b129252249b9bc03d7645a7bda512747277 linux-next) > Signed-off-by: dann frazier <dann.frazier@canonical.com> > --- > drivers/acpi/arm64/gtdt.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> Best regards, Krzysztof
On 03/05/2021 22:51, dann frazier wrote: > From: Marc Zyngier <maz@kernel.org> > > BugLink: https://bugs.launchpad.net/bugs/1925075 > > When failing the driver probe because of invalid firmware properties, > the GTDT driver unmaps the interrupt that it mapped earlier. > > However, it never checks whether the mapping of the interrupt actially > succeeded. Even more, should the firmware report an illegal interrupt > number that overlaps with the GIC SGI range, this can result in an > IPI being unmapped, and subsequent fireworks (as reported by Dann > Frazier). > > Rework the driver to have a slightly saner behaviour and actually > check whether the interrupt has been mapped before unmapping things. > > Reported-by: dann frazier <dann.frazier@canonical.com> > Fixes: ca9ae5ec4ef0 ("acpi/arm64: Add SBSA Generic Watchdog support in GTDT driver") > Signed-off-by: Marc Zyngier <maz@kernel.org> > Link: https://lore.kernel.org/r/YH87dtTfwYgavusz@xps13.dannf > Cc: <stable@vger.kernel.org> > Cc: Fu Wei <wefu@redhat.com> > Reviewed-by: Sudeep Holla <sudeep.holla@arm.com> > Tested-by: dann frazier <dann.frazier@canonical.com> > Tested-by: Hanjun Guo <guohanjun@huawei.com> > Reviewed-by: Hanjun Guo <guohanjun@huawei.com> > Reviewed-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > Link: https://lore.kernel.org/r/20210421164317.1718831-2-maz@kernel.org > Signed-off-by: Catalin Marinas <catalin.marinas@arm.com> > (cherry picked from commit 1ecd5b129252249b9bc03d7645a7bda512747277 linux-next) > Signed-off-by: dann frazier <dann.frazier@canonical.com> > --- > drivers/acpi/arm64/gtdt.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/acpi/arm64/gtdt.c b/drivers/acpi/arm64/gtdt.c > index f2d0e5915dab..0a0a982f9c28 100644 > --- a/drivers/acpi/arm64/gtdt.c > +++ b/drivers/acpi/arm64/gtdt.c > @@ -329,7 +329,7 @@ static int __init gtdt_import_sbsa_gwdt(struct acpi_gtdt_watchdog *wd, > int index) > { > struct platform_device *pdev; > - int irq = map_gt_gsi(wd->timer_interrupt, wd->timer_flags); > + int irq; > > /* > * According to SBSA specification the size of refresh and control > @@ -338,7 +338,7 @@ static int __init gtdt_import_sbsa_gwdt(struct acpi_gtdt_watchdog *wd, > struct resource res[] = { > DEFINE_RES_MEM(wd->control_frame_address, SZ_4K), > DEFINE_RES_MEM(wd->refresh_frame_address, SZ_4K), > - DEFINE_RES_IRQ(irq), > + {}, > }; > int nr_res = ARRAY_SIZE(res); > > @@ -348,10 +348,11 @@ static int __init gtdt_import_sbsa_gwdt(struct acpi_gtdt_watchdog *wd, > > if (!(wd->refresh_frame_address && wd->control_frame_address)) { > pr_err(FW_BUG "failed to get the Watchdog base address.\n"); > - acpi_unregister_gsi(wd->timer_interrupt); > return -EINVAL; > } > > + irq = map_gt_gsi(wd->timer_interrupt, wd->timer_flags); > + res[2] = (struct resource)DEFINE_RES_IRQ(irq); > if (irq <= 0) { > pr_warn("failed to map the Watchdog interrupt.\n"); > nr_res--; > @@ -364,7 +365,8 @@ static int __init gtdt_import_sbsa_gwdt(struct acpi_gtdt_watchdog *wd, > */ > pdev = platform_device_register_simple("sbsa-gwdt", index, res, nr_res); > if (IS_ERR(pdev)) { > - acpi_unregister_gsi(wd->timer_interrupt); > + if (irq > 0) > + acpi_unregister_gsi(wd->timer_interrupt); > return PTR_ERR(pdev); > } > > Makes sense, positive test results. Acked-by: Colin Ian King <colin.king@canonical.com>
diff --git a/drivers/acpi/arm64/gtdt.c b/drivers/acpi/arm64/gtdt.c index f2d0e5915dab..0a0a982f9c28 100644 --- a/drivers/acpi/arm64/gtdt.c +++ b/drivers/acpi/arm64/gtdt.c @@ -329,7 +329,7 @@ static int __init gtdt_import_sbsa_gwdt(struct acpi_gtdt_watchdog *wd, int index) { struct platform_device *pdev; - int irq = map_gt_gsi(wd->timer_interrupt, wd->timer_flags); + int irq; /* * According to SBSA specification the size of refresh and control @@ -338,7 +338,7 @@ static int __init gtdt_import_sbsa_gwdt(struct acpi_gtdt_watchdog *wd, struct resource res[] = { DEFINE_RES_MEM(wd->control_frame_address, SZ_4K), DEFINE_RES_MEM(wd->refresh_frame_address, SZ_4K), - DEFINE_RES_IRQ(irq), + {}, }; int nr_res = ARRAY_SIZE(res); @@ -348,10 +348,11 @@ static int __init gtdt_import_sbsa_gwdt(struct acpi_gtdt_watchdog *wd, if (!(wd->refresh_frame_address && wd->control_frame_address)) { pr_err(FW_BUG "failed to get the Watchdog base address.\n"); - acpi_unregister_gsi(wd->timer_interrupt); return -EINVAL; } + irq = map_gt_gsi(wd->timer_interrupt, wd->timer_flags); + res[2] = (struct resource)DEFINE_RES_IRQ(irq); if (irq <= 0) { pr_warn("failed to map the Watchdog interrupt.\n"); nr_res--; @@ -364,7 +365,8 @@ static int __init gtdt_import_sbsa_gwdt(struct acpi_gtdt_watchdog *wd, */ pdev = platform_device_register_simple("sbsa-gwdt", index, res, nr_res); if (IS_ERR(pdev)) { - acpi_unregister_gsi(wd->timer_interrupt); + if (irq > 0) + acpi_unregister_gsi(wd->timer_interrupt); return PTR_ERR(pdev); }