Re: [PATCH AUTOSEL 5.10 10/28] pinctrl: amd: Fix wakeups when IRQ is shared with SCI
From: Sasha Levin
Date: Fri Dec 03 2021 - 13:25:48 EST
On Mon, Nov 29, 2021 at 06:53:01PM +0000, Limonciello, Mario wrote:
-----Original Message-----
From: Limonciello, Mario
Sent: Monday, November 29, 2021 08:48
To: Sasha Levin <sashal@xxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx;
stable@xxxxxxxxxxxxxxx
Cc: Joerie de Gram <j.de.gram@xxxxxxxxx>; Natikar, Basavaraj
<Basavaraj.Natikar@xxxxxxx>; Linus Walleij <linus.walleij@xxxxxxxxxx>; S-k,
Shyam-sundar <Shyam-sundar.S-k@xxxxxxx>; linux-gpio@xxxxxxxxxxxxxxx
Subject: RE: [PATCH AUTOSEL 5.10 10/28] pinctrl: amd: Fix wakeups when IRQ
is shared with SCI
> -----Original Message-----
> From: Sasha Levin <sashal@xxxxxxxxxx>
> Sent: Thursday, November 25, 2021 20:33
> To: linux-kernel@xxxxxxxxxxxxxxx; stable@xxxxxxxxxxxxxxx
> Cc: Limonciello, Mario <Mario.Limonciello@xxxxxxx>; Joerie de Gram
> <j.de.gram@xxxxxxxxx>; Natikar, Basavaraj
<Basavaraj.Natikar@xxxxxxx>;
> Linus Walleij <linus.walleij@xxxxxxxxxx>; Sasha Levin <sashal@xxxxxxxxxx>;
S-
> k, Shyam-sundar <Shyam-sundar.S-k@xxxxxxx>; linux-
> gpio@xxxxxxxxxxxxxxx
> Subject: [PATCH AUTOSEL 5.10 10/28] pinctrl: amd: Fix wakeups when IRQ
is
> shared with SCI
>
> From: Mario Limonciello <mario.limonciello@xxxxxxx>
>
> [ Upstream commit 2d54067fcd23aae61e23508425ae5b29e973573d ]
>
> On some Lenovo AMD Gen2 platforms the IRQ for the SCI and pinctrl
drivers
> are shared. Due to how the s2idle loop handling works, this case needs
> an extra explicit check whether the interrupt was caused by SCI or by
> the GPIO controller.
>
> To fix this rework the existing IRQ handler function to function as a
> checker and an IRQ handler depending on the calling arguments.
>
> BugLink:
>
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitla
> b.freedesktop.org%2Fdrm%2Famd%2F-
>
%2Fissues%2F1738&data=04%7C01%7Cmario.limonciello%40amd.com%
>
7Ce14b7ddf8d1143b6649208d9b0853519%7C3dd8961fe4884e608e11a82d994
>
e183d%7C0%7C0%7C637734908448086367%7CUnknown%7CTWFpbGZsb3d8
>
eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3
>
D%7C3000&sdata=BHHY3gLu2pQIJ1nvSE0VNQOaioTC0QdBM44ze3HXrtk
> %3D&reserved=0
> Reported-by: Joerie de Gram <j.de.gram@xxxxxxxxx>
> Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx>
> Acked-by: Basavaraj Natikar <Basavaraj.Natikar@xxxxxxx>
> Link:
>
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.
> kernel.org%2Fr%2F20211101014853.6177-2-
>
mario.limonciello%40amd.com&data=04%7C01%7Cmario.limonciello%4
>
0amd.com%7Ce14b7ddf8d1143b6649208d9b0853519%7C3dd8961fe4884e608
>
e11a82d994e183d%7C0%7C0%7C637734908448086367%7CUnknown%7CTWF
>
pbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXV
>
CI6Mn0%3D%7C3000&sdata=zUkTF851CdUmrY1z3YZbYrnVrjHQaVfb1%
> 2Bg2dx28ZNA%3D&reserved=0
> Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
> Signed-off-by: Sasha Levin <sashal@xxxxxxxxxx>
> ---
> drivers/pinctrl/pinctrl-amd.c | 29 ++++++++++++++++++++++++++---
> 1 file changed, 26 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
> index e20bcc835d6a8..54dfa0244422c 100644
> --- a/drivers/pinctrl/pinctrl-amd.c
> +++ b/drivers/pinctrl/pinctrl-amd.c
> @@ -520,14 +520,14 @@ static struct irq_chip amd_gpio_irqchip = {
>
> #define PIN_IRQ_PENDING (BIT(INTERRUPT_STS_OFF) |
> BIT(WAKE_STS_OFF))
>
> -static irqreturn_t amd_gpio_irq_handler(int irq, void *dev_id)
> +static bool do_amd_gpio_irq_handler(int irq, void *dev_id)
> {
> struct amd_gpio *gpio_dev = dev_id;
> struct gpio_chip *gc = &gpio_dev->gc;
> - irqreturn_t ret = IRQ_NONE;
> unsigned int i, irqnr;
> unsigned long flags;
> u32 __iomem *regs;
> + bool ret = false;
> u32 regval;
> u64 status, mask;
>
> @@ -549,6 +549,14 @@ static irqreturn_t amd_gpio_irq_handler(int irq,
void
> *dev_id)
> /* Each status bit covers four pins */
> for (i = 0; i < 4; i++) {
> regval = readl(regs + i);
> + /* caused wake on resume context for shared IRQ */
> + if (irq < 0 && (regval & BIT(WAKE_STS_OFF))) {
> + dev_dbg(&gpio_dev->pdev->dev,
> + "Waking due to GPIO %d: 0x%x",
> + irqnr + i, regval);
> + return true;
> + }
> +
> if (!(regval & PIN_IRQ_PENDING) ||
> !(regval & BIT(INTERRUPT_MASK_OFF)))
> continue;
> @@ -574,9 +582,12 @@ static irqreturn_t amd_gpio_irq_handler(int irq,
void
> *dev_id)
> }
> writel(regval, regs + i);
> raw_spin_unlock_irqrestore(&gpio_dev->lock, flags);
> - ret = IRQ_HANDLED;
> + ret = true;
> }
> }
> + /* did not cause wake on resume context for shared IRQ */
> + if (irq < 0)
> + return false;
>
> /* Signal EOI to the GPIO unit */
> raw_spin_lock_irqsave(&gpio_dev->lock, flags);
> @@ -588,6 +599,16 @@ static irqreturn_t amd_gpio_irq_handler(int irq,
void
> *dev_id)
> return ret;
> }
>
> +static irqreturn_t amd_gpio_irq_handler(int irq, void *dev_id)
> +{
> + return IRQ_RETVAL(do_amd_gpio_irq_handler(irq, dev_id));
> +}
> +
> +static bool __maybe_unused amd_gpio_check_wake(void *dev_id)
> +{
> + return do_amd_gpio_irq_handler(-1, dev_id);
> +}
> +
> static int amd_get_groups_count(struct pinctrl_dev *pctldev)
> {
> struct amd_gpio *gpio_dev = pinctrl_dev_get_drvdata(pctldev);
> @@ -958,6 +979,7 @@ static int amd_gpio_probe(struct platform_device
> *pdev)
> goto out2;
>
> platform_set_drvdata(pdev, gpio_dev);
> + acpi_register_wakeup_handler(gpio_dev->irq,
> amd_gpio_check_wake, gpio_dev);
>
> dev_dbg(&pdev->dev, "amd gpio driver loaded\n");
> return ret;
> @@ -975,6 +997,7 @@ static int amd_gpio_remove(struct platform_device
> *pdev)
> gpio_dev = platform_get_drvdata(pdev);
>
> gpiochip_remove(&gpio_dev->gc);
> + acpi_unregister_wakeup_handler(amd_gpio_check_wake,
> gpio_dev);
>
> return 0;
> }
> --
> 2.33.0
Sasha,
No concerns for me to 5.10, but would you mind also sending this to 5.14.y
and 5.15.y too? I didn't see it sent up for either.
5.14 was EOL at that time.
One more thought on this - please also take this (which wasn't part of autosel)
when this comes back:
e9380df85187 ACPI: Add stubs for wakeup handler functions
That prevents some compile errors in certain configurations.
I'll grab it, thanks!
--
Thanks,
Sasha