[PATCH v3 1/2] ACPI: Add stubs for wakeup handler functions

[PATCH v3 1/2] ACPI: Add stubs for wakeup handler functions

[Mario Limonciello]

commit ddfd9dcf270c ("ACPI: PM: Add acpi_[un]register_wakeup_handler()")
added new functions for drivers to use during the s2idle wakeup path, but
didn't add stubs for when CONFIG_ACPI wasn't set.

Add those stubs in for other drivers to be able to use.

Fixes: ddfd9dcf270c ("ACPI: PM: Add acpi_[un]register_wakeup_handler()")
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 include/linux/acpi.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 72e4f7fd268c..b31bcc0f4c89 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -976,6 +976,14 @@ static inline int acpi_get_local_address(acpi_handle handle, u32 *addr)
 	return -ENODEV;
 }
 
+static inline int acpi_register_wakeup_handler(
+	int wake_irq, bool (*wakeup)(void *context), void *context)
+{
+	return -EINVAL;
+}
+static inline void acpi_unregister_wakeup_handler(
+	bool (*wakeup)(void *context), void *context) { }
+
 #endif	/* !CONFIG_ACPI */
 
 #ifdef CONFIG_ACPI_HOTPLUG_IOAPIC
-- 
2.25.1

[PATCH v3 2/2] pinctrl: amd: Fix wakeups when IRQ is shared with SCI

[Mario Limonciello]

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://gitlab.freedesktop.org/drm/amd/-/issues/1738
Reported-by: Joerie de Gram <j.de.gram@gmail.com>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
Note:
 This is *possibly* a fix from fdde0ff8590b, 56b991849009 or other
 changes in the acpi_s2idle_wake handling, but AMD didn't support
 s2idle across the kernel widely until 5.14 or later. This is the
 reason for lack of a fixes tag.
Changes from v2->v3:
 * Add new precursor patch for fixing missing ACPI function stubs
 * Add __maybe_unused for unused function when set with COMPILE_TEST
Changes from v1->v2:
 * drop Kconfig changes to drop COMPILE_TEST, instead #ifdef CONFIG_ACPI
 * fix a logic error during wakeup
 * Use IRQ_RETVAL()
 drivers/pinctrl/pinctrl-amd.c | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
index d19974aceb2e..1272ecd8791f 100644
--- a/drivers/pinctrl/pinctrl-amd.c
+++ b/drivers/pinctrl/pinctrl-amd.c
@@ -598,16 +598,16 @@ 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 _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;
 	u32  regval;
 	u64 status, mask;
+	bool ret = false;
 
 	/* Read the wake status */
 	raw_spin_lock_irqsave(&gpio_dev->lock, flags);
@@ -627,6 +627,12 @@ 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);
+			/* called from resume context on a shared IRQ, just
+			 * checking wake source.
+			 */
+			if (irq < 0 && (regval & BIT(WAKE_STS_OFF)))
+				return true;
+
 			if (!(regval & PIN_IRQ_PENDING) ||
 			    !(regval & BIT(INTERRUPT_MASK_OFF)))
 				continue;
@@ -652,9 +658,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;
 		}
 	}
+	/* called from resume context on shared IRQ but didn't cause wake */
+	if (irq < 0)
+		return false;
 
 	/* Signal EOI to the GPIO unit */
 	raw_spin_lock_irqsave(&gpio_dev->lock, flags);
@@ -666,6 +675,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(_amd_gpio_irq_handler(irq, dev_id));
+}
+
+static bool __maybe_unused amd_gpio_check_wake(void *dev_id)
+{
+	return _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);
@@ -1004,6 +1023,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;
@@ -1021,6 +1041,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.25.1

Re: [PATCH v3 2/2] pinctrl: amd: Fix wakeups when IRQ is shared with SCI

[Basavaraj Natikar]

On 10/19/2021 9:34 PM, Mario Limonciello wrote:
> 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://gitlab.freedesktop.org/drm/amd/-/issues/1738
> Reported-by: Joerie de Gram <j.de.gram@gmail.com>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
> Note:
>  This is *possibly* a fix from fdde0ff8590b, 56b991849009 or other
>  changes in the acpi_s2idle_wake handling, but AMD didn't support
>  s2idle across the kernel widely until 5.14 or later. This is the
>  reason for lack of a fixes tag.
> Changes from v2->v3:
>  * Add new precursor patch for fixing missing ACPI function stubs
>  * Add __maybe_unused for unused function when set with COMPILE_TEST
> Changes from v1->v2:
>  * drop Kconfig changes to drop COMPILE_TEST, instead #ifdef CONFIG_ACPI
>  * fix a logic error during wakeup
>  * Use IRQ_RETVAL()
>  drivers/pinctrl/pinctrl-amd.c | 27 ++++++++++++++++++++++++---
>  1 file changed, 24 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
> index d19974aceb2e..1272ecd8791f 100644
> --- a/drivers/pinctrl/pinctrl-amd.c
> +++ b/drivers/pinctrl/pinctrl-amd.c
> @@ -598,16 +598,16 @@ 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 _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;
>  	u32  regval;
>  	u64 status, mask;
> +	bool ret = false;

Just a minor nit. Can you do it reverse Xmas?
With that change 

Acked-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>


Thanks,
Basavaraj

>  
>  	/* Read the wake status */
>  	raw_spin_lock_irqsave(&gpio_dev->lock, flags);
> @@ -627,6 +627,12 @@ 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);
> +			/* called from resume context on a shared IRQ, just
> +			 * checking wake source.
> +			 */
> +			if (irq < 0 && (regval & BIT(WAKE_STS_OFF)))
> +				return true;
> +
>  			if (!(regval & PIN_IRQ_PENDING) ||
>  			    !(regval & BIT(INTERRUPT_MASK_OFF)))
>  				continue;
> @@ -652,9 +658,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;
>  		}
>  	}
> +	/* called from resume context on shared IRQ but didn't cause wake */
> +	if (irq < 0)
> +		return false;
>  
>  	/* Signal EOI to the GPIO unit */
>  	raw_spin_lock_irqsave(&gpio_dev->lock, flags);
> @@ -666,6 +675,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(_amd_gpio_irq_handler(irq, dev_id));
> +}
> +
> +static bool __maybe_unused amd_gpio_check_wake(void *dev_id)
> +{
> +	return _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);
> @@ -1004,6 +1023,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;
> @@ -1021,6 +1041,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;
>  }

Re: [PATCH v3 1/2] ACPI: Add stubs for wakeup handler functions

[Basavaraj Natikar]

On 10/19/2021 9:34 PM, Mario Limonciello wrote:
> commit ddfd9dcf270c ("ACPI: PM: Add acpi_[un]register_wakeup_handler()")
> added new functions for drivers to use during the s2idle wakeup path, but
> didn't add stubs for when CONFIG_ACPI wasn't set.
>
> Add those stubs in for other drivers to be able to use.
>
> Fixes: ddfd9dcf270c ("ACPI: PM: Add acpi_[un]register_wakeup_handler()")
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  include/linux/acpi.h | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 72e4f7fd268c..b31bcc0f4c89 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -976,6 +976,14 @@ static inline int acpi_get_local_address(acpi_handle handle, u32 *addr)
>  	return -ENODEV;
>  }
>  
> +static inline int acpi_register_wakeup_handler(
> +	int wake_irq, bool (*wakeup)(void *context), void *context)
> +{
> +	return -EINVAL;
> +}

line break is missing.

Thanks,
Basavaraj

> +static inline void acpi_unregister_wakeup_handler(
> +	bool (*wakeup)(void *context), void *context) { }
> +
>  #endif	/* !CONFIG_ACPI */
>  
>  #ifdef CONFIG_ACPI_HOTPLUG_IOAPIC

Re: [PATCH v3 1/2] ACPI: Add stubs for wakeup handler functions

[Rafael J. Wysocki]

CC: linux-acpi

On Tuesday, October 19, 2021 6:04:00 PM CEST Mario Limonciello wrote:
> commit ddfd9dcf270c ("ACPI: PM: Add acpi_[un]register_wakeup_handler()")
> added new functions for drivers to use during the s2idle wakeup path, but
> didn't add stubs for when CONFIG_ACPI wasn't set.
> 
> Add those stubs in for other drivers to be able to use.
> 
> Fixes: ddfd9dcf270c ("ACPI: PM: Add acpi_[un]register_wakeup_handler()")
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  include/linux/acpi.h | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 72e4f7fd268c..b31bcc0f4c89 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -976,6 +976,14 @@ static inline int acpi_get_local_address(acpi_handle handle, u32 *addr)
>  	return -ENODEV;
>  }
>  
> +static inline int acpi_register_wakeup_handler(
> +	int wake_irq, bool (*wakeup)(void *context), void *context)
> +{
> +	return -EINVAL;

-ENOTSUPP ?

> +}
> +static inline void acpi_unregister_wakeup_handler(
> +	bool (*wakeup)(void *context), void *context) { }
> +
>  #endif	/* !CONFIG_ACPI */
>  
>  #ifdef CONFIG_ACPI_HOTPLUG_IOAPIC
> 

RE: [PATCH v3 1/2] ACPI: Add stubs for wakeup handler functions

[Limonciello, Mario]

[Public]



> -----Original Message-----
> From: Rafael J. Wysocki <rjw@rjwysocki.net>
> Sent: Friday, October 22, 2021 07:54
> To: Limonciello, Mario <Mario.Limonciello@amd.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>; Natikar, Basavaraj
> <Basavaraj.Natikar@amd.com>; S-k, Shyam-sundar <Shyam-sundar.S-
> k@amd.com>; open list:PIN CONTROL SUBSYSTEM <linux-
> gpio@vger.kernel.org>; open list <linux-kernel@vger.kernel.org>; Shah, Nehal-
> bakulchandra <Nehal-bakulchandra.Shah@amd.com>; Limonciello, Mario
> <Mario.Limonciello@amd.com>; Linux ACPI <linux-acpi@vger.kernel.org>
> Subject: Re: [PATCH v3 1/2] ACPI: Add stubs for wakeup handler functions
> 
> CC: linux-acpi
> 
> On Tuesday, October 19, 2021 6:04:00 PM CEST Mario Limonciello wrote:
> > commit ddfd9dcf270c ("ACPI: PM: Add acpi_[un]register_wakeup_handler()")
> > added new functions for drivers to use during the s2idle wakeup path, but
> > didn't add stubs for when CONFIG_ACPI wasn't set.
> >
> > Add those stubs in for other drivers to be able to use.
> >
> > Fixes: ddfd9dcf270c ("ACPI: PM: Add acpi_[un]register_wakeup_handler()")
> > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> > ---
> >  include/linux/acpi.h | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> > index 72e4f7fd268c..b31bcc0f4c89 100644
> > --- a/include/linux/acpi.h
> > +++ b/include/linux/acpi.h
> > @@ -976,6 +976,14 @@ static inline int acpi_get_local_address(acpi_handle
> handle, u32 *addr)
> >  	return -ENODEV;
> >  }
> >
> > +static inline int acpi_register_wakeup_handler(
> > +	int wake_irq, bool (*wakeup)(void *context), void *context)
> > +{
> > +	return -EINVAL;
> 
> -ENOTSUPP ?
> 

checkpatch reports:

WARNING: ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP

So what to use?

> > +}
> > +static inline void acpi_unregister_wakeup_handler(
> > +	bool (*wakeup)(void *context), void *context) { }
> > +
> >  #endif	/* !CONFIG_ACPI */
> >
> >  #ifdef CONFIG_ACPI_HOTPLUG_IOAPIC
> >
> 
> 
> 

Re: [PATCH v3 1/2] ACPI: Add stubs for wakeup handler functions

[Andy Shevchenko]

On Tue, Oct 19, 2021 at 7:05 PM Mario Limonciello
<mario.limonciello@amd.com> wrote:

Thanks! My comments below.

> commit ddfd9dcf270c ("ACPI: PM: Add acpi_[un]register_wakeup_handler()")

The commit

> added new functions for drivers to use during the s2idle wakeup path, but
> didn't add stubs for when CONFIG_ACPI wasn't set.
>
> Add those stubs in for other drivers to be able to use.

...

> +static inline int acpi_register_wakeup_handler(
> +       int wake_irq, bool (*wakeup)(void *context), void *context)

A bit of a strange indentation. Can wake_irq be on the previous line?

-- 
With Best Regards,
Andy Shevchenko