[PATCH 0/1] pinctrl: amd: pinctrl: amd: disable and mask interrupts on probe

[PATCH 0/1] pinctrl: amd: pinctrl: amd: disable and mask interrupts on probe

[Sachi King]

On a Microsoft Surface Laptop 4, I've found that the GPIO controller is
preconfigured with a number of interrupts.  I'm not sure if this is
device specific, in which case I could propose a quirk, or a more
general issue.  Prior to https://git.kernel.org/torvalds/c/acd47b9f28e5
which will be in 5.15, this device exhibited failed resumes that would
result in high power consumption and no reliable way to resume the
device.  Following that patch the device exhibits spurious wakeups.

As far as I can tell interrupts should be disabled and masked as a
default state, so ensuring this is the initial state should be safe as a
default action.

Sachi King (1):
  pinctrl: amd: disable and mask interrupts on probe

 drivers/pinctrl/pinctrl-amd.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

-- 
2.33.0

[PATCH 1/1] pinctrl: amd: disable and mask interrupts on probe

[Sachi King]

Some systems such as the Microsoft Surface Laptop 4 leave interrupts
enabled and configured for use in sleep states on boot, which cause
unexpected behaviour such as spurious wakes and failed resumes in
s2idle states.

As interrupts should not be enabled until they are claimed and
explicitly enabled, disabling any interrupts mistakenly left enabled by
firmware should be safe.

Signed-off-by: Sachi King <nakato@nakato.io>
---
 drivers/pinctrl/pinctrl-amd.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
index c001f2ed20f8..aa4136cd312d 100644
--- a/drivers/pinctrl/pinctrl-amd.c
+++ b/drivers/pinctrl/pinctrl-amd.c
@@ -830,6 +830,32 @@ static const struct pinconf_ops amd_pinconf_ops = {
 	.pin_config_group_set = amd_pinconf_group_set,
 };
 
+static void amd_gpio_irq_init(struct amd_gpio *gpio_dev) {
+	struct pinctrl_desc *desc = gpio_dev->pctrl->desc;
+	unsigned long flags;
+	u32 pin_reg, mask;
+	int i;
+
+	mask = BIT(WAKE_CNTRL_OFF_S0I3) | BIT(WAKE_CNTRL_OFF_S3)
+		| BIT(INTERRUPT_MASK_OFF) | BIT(INTERRUPT_ENABLE_OFF)
+	        | BIT(INTERRUPT_MASK_OFF) | BIT(WAKE_CNTRL_OFF_S4);
+
+	for (i = 0; i < desc->npins; i++) {
+		int pin = desc->pins[i].number;
+		const struct pin_desc *pd = pin_desc_get(gpio_dev->pctrl, pin);
+		if (!pd)
+			continue;
+
+		raw_spin_lock_irqsave(&gpio_dev->lock, flags);
+
+		pin_reg = readl(gpio_dev->base + i * 4);
+		pin_reg &= ~mask;
+		writel(pin_reg, gpio_dev->base + i * 4);
+
+		raw_spin_unlock_irqrestore(&gpio_dev->lock, flags);
+	}
+}
+
 #ifdef CONFIG_PM_SLEEP
 static bool amd_gpio_should_save(struct amd_gpio *gpio_dev, unsigned int pin)
 {
@@ -967,6 +993,9 @@ static int amd_gpio_probe(struct platform_device *pdev)
 		return PTR_ERR(gpio_dev->pctrl);
 	}
 
+	/* Disable and mask interrupts */
+	amd_gpio_irq_init(gpio_dev);
+
 	girq = &gpio_dev->gc.irq;
 	girq->chip = &amd_gpio_irqchip;
 	/* This will let us handle the parent IRQ in the driver */
-- 
2.33.0

Re: [PATCH 1/1] pinctrl: amd: disable and mask interrupts on probe

[Limonciello, Mario]

On 10/1/2021 11:17, Sachi King wrote:
> Some systems such as the Microsoft Surface Laptop 4 leave interrupts
> enabled and configured for use in sleep states on boot, which cause
> unexpected behaviour such as spurious wakes and failed resumes in
> s2idle states.
> 
> As interrupts should not be enabled until they are claimed and
> explicitly enabled, disabling any interrupts mistakenly left enabled by
> firmware should be safe.
> 

So I did test this on a handful of platforms and confirmed that the 
events declared in _AEI are still being enabled and passed properly.

So if no other changes needed you can add my:
Tested-by: Mario Limonciello <mario.limonciello@amd.com>

> Signed-off-by: Sachi King <nakato@nakato.io>
> ---
>   drivers/pinctrl/pinctrl-amd.c | 29 +++++++++++++++++++++++++++++
>   1 file changed, 29 insertions(+)
> 
> diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
> index c001f2ed20f8..aa4136cd312d 100644
> --- a/drivers/pinctrl/pinctrl-amd.c
> +++ b/drivers/pinctrl/pinctrl-amd.c
> @@ -830,6 +830,32 @@ static const struct pinconf_ops amd_pinconf_ops = {
>   	.pin_config_group_set = amd_pinconf_group_set,
>   };
>   
> +static void amd_gpio_irq_init(struct amd_gpio *gpio_dev) {
> +	struct pinctrl_desc *desc = gpio_dev->pctrl->desc;
> +	unsigned long flags;
> +	u32 pin_reg, mask;
> +	int i;
> +
> +	mask = BIT(WAKE_CNTRL_OFF_S0I3) | BIT(WAKE_CNTRL_OFF_S3)
> +		| BIT(INTERRUPT_MASK_OFF) | BIT(INTERRUPT_ENABLE_OFF)
> +	        | BIT(INTERRUPT_MASK_OFF) | BIT(WAKE_CNTRL_OFF_S4);
> +
> +	for (i = 0; i < desc->npins; i++) {
> +		int pin = desc->pins[i].number;
> +		const struct pin_desc *pd = pin_desc_get(gpio_dev->pctrl, pin);
> +		if (!pd)
> +			continue;
> +
> +		raw_spin_lock_irqsave(&gpio_dev->lock, flags);
> +
> +		pin_reg = readl(gpio_dev->base + i * 4);
> +		pin_reg &= ~mask;
> +		writel(pin_reg, gpio_dev->base + i * 4);
> +
> +		raw_spin_unlock_irqrestore(&gpio_dev->lock, flags);
> +	}
> +}
> +
>   #ifdef CONFIG_PM_SLEEP
>   static bool amd_gpio_should_save(struct amd_gpio *gpio_dev, unsigned int pin)
>   {
> @@ -967,6 +993,9 @@ static int amd_gpio_probe(struct platform_device *pdev)
>   		return PTR_ERR(gpio_dev->pctrl);
>   	}
>   
> +	/* Disable and mask interrupts */
> +	amd_gpio_irq_init(gpio_dev);
> +

As the pinctrl device was just registered, I do wonder if this actually 
needs a mutex in case another thread tries to enable the pins at the 
same time.  I might be wrong here though and things are OK because the 
pin range isn't added until later on in probe.


>   	girq = &gpio_dev->gc.irq;
>   	girq->chip = &amd_gpio_irqchip;
>   	/* This will let us handle the parent IRQ in the driver */
> 

Re: [PATCH 1/1] pinctrl: amd: disable and mask interrupts on probe

[Sachi King]

On Saturday, 9 October 2021 05:19:17 AEDT Limonciello, Mario wrote:
> On 10/1/2021 11:17, Sachi King wrote:
> > Some systems such as the Microsoft Surface Laptop 4 leave interrupts
> > enabled and configured for use in sleep states on boot, which cause
> > unexpected behaviour such as spurious wakes and failed resumes in
> > s2idle states.
> > 
> > As interrupts should not be enabled until they are claimed and
> > explicitly enabled, disabling any interrupts mistakenly left enabled by
> > firmware should be safe.
> > 
> 
> So I did test this on a handful of platforms and confirmed that the 
> events declared in _AEI are still being enabled and passed properly.
> 
> So if no other changes needed you can add my:
> Tested-by: Mario Limonciello <mario.limonciello@amd.com>

I've sent a second version of the patch to remove the duplicate
INTERRUPT_MASK_OFF.  I also fixed a number of style and whitespace issues.

Would you like to test this again?


> > diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
> > index c001f2ed20f8..aa4136cd312d 100644
> > --- a/drivers/pinctrl/pinctrl-amd.c
> > +++ b/drivers/pinctrl/pinctrl-amd.c
> > @@ -967,6 +993,9 @@ static int amd_gpio_probe(struct platform_device *pdev)
> >   		return PTR_ERR(gpio_dev->pctrl);
> >   	}
> >   
> > +	/* Disable and mask interrupts */
> > +	amd_gpio_irq_init(gpio_dev);
> > +
> 
> As the pinctrl device was just registered, I do wonder if this actually 
> needs a mutex in case another thread tries to enable the pins at the 
> same time.  I might be wrong here though and things are OK because the 
> pin range isn't added until later on in probe.

If we need to add a mutex then I think it would be more safe to rework
the amd_gpio_init_irq function to not depend on amd_gpio and move it
before devm_pinctrl_register, as I'm not sure a mutex around
amd_gpio_irq_init would save us from a race condition occurring between the
devm_pinctrl_register and getting the mutex following that. I'm hoping to
avoid that however as it would be a bit messy without pin_desc_get.

Re: [PATCH 1/1] pinctrl: amd: disable and mask interrupts on probe

[Basavaraj Natikar]

On 10/1/2021 9:47 PM, Sachi King wrote:
> [CAUTION: External Email]
>
> Some systems such as the Microsoft Surface Laptop 4 leave interrupts
> enabled and configured for use in sleep states on boot, which cause
> unexpected behaviour such as spurious wakes and failed resumes in
> s2idle states.
>
> As interrupts should not be enabled until they are claimed and
> explicitly enabled, disabling any interrupts mistakenly left enabled by
> firmware should be safe.
>
> Signed-off-by: Sachi King <nakato@nakato.io>
> ---
>  drivers/pinctrl/pinctrl-amd.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
>
> diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
> index c001f2ed20f8..aa4136cd312d 100644
> --- a/drivers/pinctrl/pinctrl-amd.c
> +++ b/drivers/pinctrl/pinctrl-amd.c
> @@ -830,6 +830,32 @@ static const struct pinconf_ops amd_pinconf_ops = {
>         .pin_config_group_set = amd_pinconf_group_set,
>  };
>
> +static void amd_gpio_irq_init(struct amd_gpio *gpio_dev) {
> +       struct pinctrl_desc *desc = gpio_dev->pctrl->desc;
> +       unsigned long flags;
> +       u32 pin_reg, mask;
> +       int i;
> +
> +       mask = BIT(WAKE_CNTRL_OFF_S0I3) | BIT(WAKE_CNTRL_OFF_S3)
> +               | BIT(INTERRUPT_MASK_OFF) | BIT(INTERRUPT_ENABLE_OFF)
> +               | BIT(INTERRUPT_MASK_OFF) | BIT(WAKE_CNTRL_OFF_S4);

Could you please correct INTERRUPT_MASK_OFF , added twice.

Thanks,
Basavaraj 

> +
> +       for (i = 0; i < desc->npins; i++) {
> +               int pin = desc->pins[i].number;
> +               const struct pin_desc *pd = pin_desc_get(gpio_dev->pctrl, pin);
> +               if (!pd)
> +                       continue;
> +
> +               raw_spin_lock_irqsave(&gpio_dev->lock, flags);
> +
> +               pin_reg = readl(gpio_dev->base + i * 4);
> +               pin_reg &= ~mask;
> +               writel(pin_reg, gpio_dev->base + i * 4);
> +
> +               raw_spin_unlock_irqrestore(&gpio_dev->lock, flags);
> +       }
> +}
> +
>  #ifdef CONFIG_PM_SLEEP
>  static bool amd_gpio_should_save(struct amd_gpio *gpio_dev, unsigned int pin)
>  {
> @@ -967,6 +993,9 @@ static int amd_gpio_probe(struct platform_device *pdev)
>                 return PTR_ERR(gpio_dev->pctrl);
>         }
>
> +       /* Disable and mask interrupts */
> +       amd_gpio_irq_init(gpio_dev);
> +
>         girq = &gpio_dev->gc.irq;
>         girq->chip = &amd_gpio_irqchip;
>         /* This will let us handle the parent IRQ in the driver */
> --
> 2.33.0
>

[PATCH v2] pinctrl: amd: disable and mask interrupts on probe

[Sachi King]

Some systems such as the Microsoft Surface Laptop 4 leave interrupts
enabled and configured for use in sleep states on boot, which cause
unexpected behaviour such as spurious wakes and failed resumes in
s2idle states.

As interrupts should not be enabled until they are claimed and
explicitly enabled, disabling any interrupts mistakenly left enabled by
firmware should be safe.

Signed-off-by: Sachi King <nakato@nakato.io>
---
Changes since v1:
- Removed duplicate INTERRUPT_MASK_OFF
- Corrected incorrect use of spaces instead of tab on third mask line
- Moved bitwise or to end of line to follow convention
- Corrected missing blank line after declaration
- Corrected open brace following function definitions
---
 drivers/pinctrl/pinctrl-amd.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
index 8d0f88e9ca88..bae9d429b813 100644
--- a/drivers/pinctrl/pinctrl-amd.c
+++ b/drivers/pinctrl/pinctrl-amd.c
@@ -840,6 +840,34 @@ static const struct pinconf_ops amd_pinconf_ops = {
 	.pin_config_group_set = amd_pinconf_group_set,
 };
 
+static void amd_gpio_irq_init(struct amd_gpio *gpio_dev)
+{
+	struct pinctrl_desc *desc = gpio_dev->pctrl->desc;
+	unsigned long flags;
+	u32 pin_reg, mask;
+	int i;
+
+	mask = BIT(WAKE_CNTRL_OFF_S0I3) | BIT(WAKE_CNTRL_OFF_S3) |
+		BIT(INTERRUPT_MASK_OFF) | BIT(INTERRUPT_ENABLE_OFF) |
+		BIT(WAKE_CNTRL_OFF_S4);
+
+	for (i = 0; i < desc->npins; i++) {
+		int pin = desc->pins[i].number;
+		const struct pin_desc *pd = pin_desc_get(gpio_dev->pctrl, pin);
+
+		if (!pd)
+			continue;
+
+		raw_spin_lock_irqsave(&gpio_dev->lock, flags);
+
+		pin_reg = readl(gpio_dev->base + i * 4);
+		pin_reg &= ~mask;
+		writel(pin_reg, gpio_dev->base + i * 4);
+
+		raw_spin_unlock_irqrestore(&gpio_dev->lock, flags);
+	}
+}
+
 #ifdef CONFIG_PM_SLEEP
 static bool amd_gpio_should_save(struct amd_gpio *gpio_dev, unsigned int pin)
 {
@@ -976,6 +1004,9 @@ static int amd_gpio_probe(struct platform_device *pdev)
 		return PTR_ERR(gpio_dev->pctrl);
 	}
 
+	/* Disable and mask interrupts */
+	amd_gpio_irq_init(gpio_dev);
+
 	girq = &gpio_dev->gc.irq;
 	girq->chip = &amd_gpio_irqchip;
 	/* This will let us handle the parent IRQ in the driver */
-- 
2.33.0

Re: [PATCH v2] pinctrl: amd: disable and mask interrupts on probe

[Linus Walleij]

On Sat, Oct 9, 2021 at 5:33 AM Sachi King <nakato@nakato.io> wrote:

> Some systems such as the Microsoft Surface Laptop 4 leave interrupts
> enabled and configured for use in sleep states on boot, which cause
> unexpected behaviour such as spurious wakes and failed resumes in
> s2idle states.
>
> As interrupts should not be enabled until they are claimed and
> explicitly enabled, disabling any interrupts mistakenly left enabled by
> firmware should be safe.
>
> Signed-off-by: Sachi King <nakato@nakato.io>

This v2 version applied for fixes.

Yours,
Linus Walleij