Re: [PATCH AUTOSEL 5.10 10/28] pinctrl: amd: Fix wakeups when IRQ is shared with SCI

[Sasha Levin <sashal@kernel.org>]

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@kernel.org>; linux-kernel@vger.kernel.org;
>> stable@vger.kernel.org
>> Cc: Joerie de Gram <j.de.gram@gmail.com>; Natikar, Basavaraj
>> <Basavaraj.Natikar@amd.com>; Linus Walleij <linus.walleij@linaro.org>; S-k,
>> Shyam-sundar <Shyam-sundar.S-k@amd.com>; linux-gpio@vger.kernel.org
>> Subject: RE: [PATCH AUTOSEL 5.10 10/28] pinctrl: amd: Fix wakeups when IRQ
>> is shared with SCI
>>
>>
>>
>> > -----Original Message-----
>> > From: Sasha Levin <sashal@kernel.org>
>> > Sent: Thursday, November 25, 2021 20:33
>> > To: linux-kernel@vger.kernel.org; stable@vger.kernel.org
>> > Cc: Limonciello, Mario <Mario.Limonciello@amd.com>; Joerie de Gram
>> > <j.de.gram@gmail.com>; Natikar, Basavaraj
>> <Basavaraj.Natikar@amd.com>;
>> > Linus Walleij <linus.walleij@linaro.org>; Sasha Levin <sashal@kernel.org>;
>> S-
>> > k, Shyam-sundar <Shyam-sundar.S-k@amd.com>; linux-
>> > gpio@vger.kernel.org
>> > Subject: [PATCH AUTOSEL 5.10 10/28] pinctrl: amd: Fix wakeups when IRQ
>> is
>> > shared with SCI
>> >
>> > From: Mario Limonciello <mario.limonciello@amd.com>
>> >
>> > [ 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&amp;data=04%7C01%7Cmario.limonciello%40amd.com%
>> >
>> 7Ce14b7ddf8d1143b6649208d9b0853519%7C3dd8961fe4884e608e11a82d994
>> >
>> e183d%7C0%7C0%7C637734908448086367%7CUnknown%7CTWFpbGZsb3d8
>> >
>> eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3
>> >
>> D%7C3000&amp;sdata=BHHY3gLu2pQIJ1nvSE0VNQOaioTC0QdBM44ze3HXrtk
>> > %3D&amp;reserved=0
>> > Reported-by: Joerie de Gram <j.de.gram@gmail.com>
>> > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> > Acked-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
>> > Link:
>> >
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.
>> > kernel.org%2Fr%2F20211101014853.6177-2-
>> >
>> mario.limonciello%40amd.com&amp;data=04%7C01%7Cmario.limonciello%4
>> >
>> 0amd.com%7Ce14b7ddf8d1143b6649208d9b0853519%7C3dd8961fe4884e608
>> >
>> e11a82d994e183d%7C0%7C0%7C637734908448086367%7CUnknown%7CTWF
>> >
>> pbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXV
>> >
>> CI6Mn0%3D%7C3000&amp;sdata=zUkTF851CdUmrY1z3YZbYrnVrjHQaVfb1%
>> > 2Bg2dx28ZNA%3D&amp;reserved=0
>> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
>> > Signed-off-by: Sasha Levin <sashal@kernel.org>
>> > ---
>> >  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