linux-gpio.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/1] pinctrl: amd: pinctrl: amd: disable and mask interrupts on probe
@ 2021-10-01 16:17 Sachi King
  2021-10-01 16:17 ` [PATCH 1/1] " Sachi King
  0 siblings, 1 reply; 7+ messages in thread
From: Sachi King @ 2021-10-01 16:17 UTC (permalink / raw)
  To: linux-gpio, basavaraj.natikar; +Cc: linux-kernel, 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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 1/1] pinctrl: amd: disable and mask interrupts on probe
  2021-10-01 16:17 [PATCH 0/1] pinctrl: amd: pinctrl: amd: disable and mask interrupts on probe Sachi King
@ 2021-10-01 16:17 ` Sachi King
  2021-10-08 18:19   ` Limonciello, Mario
  2021-10-08 19:57   ` Basavaraj Natikar
  0 siblings, 2 replies; 7+ messages in thread
From: Sachi King @ 2021-10-01 16:17 UTC (permalink / raw)
  To: linux-gpio, basavaraj.natikar; +Cc: linux-kernel, 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


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/1] pinctrl: amd: disable and mask interrupts on probe
  2021-10-01 16:17 ` [PATCH 1/1] " Sachi King
@ 2021-10-08 18:19   ` Limonciello, Mario
  2021-10-09  3:42     ` Sachi King
  2021-10-08 19:57   ` Basavaraj Natikar
  1 sibling, 1 reply; 7+ messages in thread
From: Limonciello, Mario @ 2021-10-08 18:19 UTC (permalink / raw)
  To: Sachi King, linux-gpio, basavaraj.natikar
  Cc: linux-kernel, Shyam-sundar S-k, Shah, Nehal-bakulchandra

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 */
> 


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/1] pinctrl: amd: disable and mask interrupts on probe
  2021-10-01 16:17 ` [PATCH 1/1] " Sachi King
  2021-10-08 18:19   ` Limonciello, Mario
@ 2021-10-08 19:57   ` Basavaraj Natikar
  2021-10-09  3:32     ` [PATCH v2] " Sachi King
  1 sibling, 1 reply; 7+ messages in thread
From: Basavaraj Natikar @ 2021-10-08 19:57 UTC (permalink / raw)
  To: Sachi King, linux-gpio, basavaraj.natikar
  Cc: linux-kernel, Limonciello, Mario, Shyam-sundar S-k, Shah,
	Nehal-bakulchandra


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
>


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v2] pinctrl: amd: disable and mask interrupts on probe
  2021-10-08 19:57   ` Basavaraj Natikar
@ 2021-10-09  3:32     ` Sachi King
  2021-10-16 21:57       ` Linus Walleij
  0 siblings, 1 reply; 7+ messages in thread
From: Sachi King @ 2021-10-09  3:32 UTC (permalink / raw)
  To: bnatikar, mario.limonciello
  Cc: linux-gpio, Shyam-sundar.S-k, Nehal-bakulchandra.Shah,
	linus.walleij, linux-kernel, nakato

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


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/1] pinctrl: amd: disable and mask interrupts on probe
  2021-10-08 18:19   ` Limonciello, Mario
@ 2021-10-09  3:42     ` Sachi King
  0 siblings, 0 replies; 7+ messages in thread
From: Sachi King @ 2021-10-09  3:42 UTC (permalink / raw)
  To: linux-gpio, basavaraj.natikar, Limonciello, Mario
  Cc: linux-kernel, Shyam-sundar S-k, Shah, Nehal-bakulchandra

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.



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2] pinctrl: amd: disable and mask interrupts on probe
  2021-10-09  3:32     ` [PATCH v2] " Sachi King
@ 2021-10-16 21:57       ` Linus Walleij
  0 siblings, 0 replies; 7+ messages in thread
From: Linus Walleij @ 2021-10-16 21:57 UTC (permalink / raw)
  To: Sachi King
  Cc: bnatikar, Mario Limonciello, open list:GPIO SUBSYSTEM, S-k,
	Shyam-sundar, Shah, Nehal-bakulchandra, linux-kernel

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-10-16 21:58 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-01 16:17 [PATCH 0/1] pinctrl: amd: pinctrl: amd: disable and mask interrupts on probe Sachi King
2021-10-01 16:17 ` [PATCH 1/1] " Sachi King
2021-10-08 18:19   ` Limonciello, Mario
2021-10-09  3:42     ` Sachi King
2021-10-08 19:57   ` Basavaraj Natikar
2021-10-09  3:32     ` [PATCH v2] " Sachi King
2021-10-16 21:57       ` Linus Walleij

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).