[PATCH] pinctrl: amd: Disable and mask interrupts on resume

[PATCH] pinctrl: amd: Disable and mask interrupts on resume

[Kornel Dulęba]

This fixes a similar problem to the one observed in:
commit 4e5a04be88fe ("pinctrl: amd: disable and mask interrupts on probe").

On some systems, during suspend/resume cycle firmware leaves
an interrupt enabled on a pin that is not used by the kernel.
This confuses the AMD pinctrl driver and causes spurious interrupts.

The driver already has logic to detect if a pin is used by the kernel.
Leverage it to re-initialize interrupt fields of a pin only if it's not
used by us.

Signed-off-by: Kornel Dulęba <korneld@chromium.org>
---
 drivers/pinctrl/pinctrl-amd.c | 36 +++++++++++++++++++----------------
 1 file changed, 20 insertions(+), 16 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
index 9236a132c7ba..609821b756c2 100644
--- a/drivers/pinctrl/pinctrl-amd.c
+++ b/drivers/pinctrl/pinctrl-amd.c
@@ -872,32 +872,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)
+static void amd_gpio_irq_init_pin(struct amd_gpio *gpio_dev, int pin)
 {
-	struct pinctrl_desc *desc = gpio_dev->pctrl->desc;
+	const struct pin_desc *pd;
 	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;
+	pd = pin_desc_get(gpio_dev->pctrl, pin);
+	if (!pd)
+		return;
 
-		raw_spin_lock_irqsave(&gpio_dev->lock, flags);
+	raw_spin_lock_irqsave(&gpio_dev->lock, flags);
+	pin_reg = readl(gpio_dev->base + pin * 4);
+	pin_reg &= ~mask;
+	writel(pin_reg, gpio_dev->base + pin * 4);
+	raw_spin_unlock_irqrestore(&gpio_dev->lock, flags);
+}
 
-		pin_reg = readl(gpio_dev->base + i * 4);
-		pin_reg &= ~mask;
-		writel(pin_reg, gpio_dev->base + i * 4);
+static void amd_gpio_irq_init(struct amd_gpio *gpio_dev)
+{
+	struct pinctrl_desc *desc = gpio_dev->pctrl->desc;
+	int i;
 
-		raw_spin_unlock_irqrestore(&gpio_dev->lock, flags);
-	}
+	for (i = 0; i < desc->npins; i++)
+		amd_gpio_irq_init_pin(gpio_dev, i);
 }
 
 #ifdef CONFIG_PM_SLEEP
@@ -950,8 +952,10 @@ static int amd_gpio_resume(struct device *dev)
 	for (i = 0; i < desc->npins; i++) {
 		int pin = desc->pins[i].number;
 
-		if (!amd_gpio_should_save(gpio_dev, pin))
+		if (!amd_gpio_should_save(gpio_dev, pin)) {
+			amd_gpio_irq_init_pin(gpio_dev, pin);
 			continue;
+		}
 
 		raw_spin_lock_irqsave(&gpio_dev->lock, flags);
 		gpio_dev->saved_regs[i] |= readl(gpio_dev->base + pin * 4) & PIN_IRQ_PENDING;
-- 
2.40.0.rc1.284.g88254d51c5-goog

Re: [PATCH] pinctrl: amd: Disable and mask interrupts on resume

[Linus Walleij]

On Mon, Mar 20, 2023 at 10:33 AM Kornel Dulęba <korneld@chromium.org> wrote:

> This fixes a similar problem to the one observed in:
> commit 4e5a04be88fe ("pinctrl: amd: disable and mask interrupts on probe").
>
> On some systems, during suspend/resume cycle firmware leaves
> an interrupt enabled on a pin that is not used by the kernel.
> This confuses the AMD pinctrl driver and causes spurious interrupts.
>
> The driver already has logic to detect if a pin is used by the kernel.
> Leverage it to re-initialize interrupt fields of a pin only if it's not
> used by us.
>
> Signed-off-by: Kornel Dulęba <korneld@chromium.org>

Uh oh this looks serious.
Do we need a Fixes: tag and Cc: stable on this patch?

Yours,
Linus Walleij

Re: [PATCH] pinctrl: amd: Disable and mask interrupts on resume

[Kornel Dulęba]

On Mon, Mar 20, 2023 at 11:05 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Mon, Mar 20, 2023 at 10:33 AM Kornel Dulęba <korneld@chromium.org> wrote:
>
> > This fixes a similar problem to the one observed in:
> > commit 4e5a04be88fe ("pinctrl: amd: disable and mask interrupts on probe").
> >
> > On some systems, during suspend/resume cycle firmware leaves
> > an interrupt enabled on a pin that is not used by the kernel.
> > This confuses the AMD pinctrl driver and causes spurious interrupts.
> >
> > The driver already has logic to detect if a pin is used by the kernel.
> > Leverage it to re-initialize interrupt fields of a pin only if it's not
> > used by us.
> >
> > Signed-off-by: Kornel Dulęba <korneld@chromium.org>
>
> Uh oh this looks serious.
> Do we need a Fixes: tag and Cc: stable on this patch?

I suppose so.
I didn't add them since I'm not sure what commit Fixes: should point to.
This issue seems to have always been there, so probably the first
commit of this driver?

Regards
Kornel Dulęba

Re: [PATCH] pinctrl: amd: Disable and mask interrupts on resume

[Linus Walleij]

On Mon, Mar 20, 2023 at 10:33 AM Kornel Dulęba <korneld@chromium.org> wrote:

> This fixes a similar problem to the one observed in:
> commit 4e5a04be88fe ("pinctrl: amd: disable and mask interrupts on probe").
>
> On some systems, during suspend/resume cycle firmware leaves
> an interrupt enabled on a pin that is not used by the kernel.
> This confuses the AMD pinctrl driver and causes spurious interrupts.
>
> The driver already has logic to detect if a pin is used by the kernel.
> Leverage it to re-initialize interrupt fields of a pin only if it's not
> used by us.
>
> Signed-off-by: Kornel Dulęba <korneld@chromium.org>

Patch applied for fixes with CC to stable!

Yours,
Linus Walleij

Re: [PATCH] pinctrl: amd: Disable and mask interrupts on resume

[Mario Limonciello]

On 3/20/23 04:32, Kornel Dulęba wrote:

> This fixes a similar problem to the one observed in:
> commit 4e5a04be88fe ("pinctrl: amd: disable and mask interrupts on probe").
>
> On some systems, during suspend/resume cycle firmware leaves
> an interrupt enabled on a pin that is not used by the kernel.
> This confuses the AMD pinctrl driver and causes spurious interrupts.
>
> The driver already has logic to detect if a pin is used by the kernel.
> Leverage it to re-initialize interrupt fields of a pin only if it's not
> used by us.
>
> Signed-off-by: Kornel Dulęba <korneld@chromium.org>
> ---
>   drivers/pinctrl/pinctrl-amd.c | 36 +++++++++++++++++++----------------
>   1 file changed, 20 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
> index 9236a132c7ba..609821b756c2 100644
> --- a/drivers/pinctrl/pinctrl-amd.c
> +++ b/drivers/pinctrl/pinctrl-amd.c
> @@ -872,32 +872,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)
> +static void amd_gpio_irq_init_pin(struct amd_gpio *gpio_dev, int pin)
>   {
> -	struct pinctrl_desc *desc = gpio_dev->pctrl->desc;
> +	const struct pin_desc *pd;
>   	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;
> +	pd = pin_desc_get(gpio_dev->pctrl, pin);
> +	if (!pd)
> +		return;
>   
> -		raw_spin_lock_irqsave(&gpio_dev->lock, flags);
> +	raw_spin_lock_irqsave(&gpio_dev->lock, flags);
> +	pin_reg = readl(gpio_dev->base + pin * 4);
> +	pin_reg &= ~mask;
> +	writel(pin_reg, gpio_dev->base + pin * 4);
> +	raw_spin_unlock_irqrestore(&gpio_dev->lock, flags);
> +}
>   
> -		pin_reg = readl(gpio_dev->base + i * 4);
> -		pin_reg &= ~mask;
> -		writel(pin_reg, gpio_dev->base + i * 4);
> +static void amd_gpio_irq_init(struct amd_gpio *gpio_dev)
> +{
> +	struct pinctrl_desc *desc = gpio_dev->pctrl->desc;
> +	int i;
>   
> -		raw_spin_unlock_irqrestore(&gpio_dev->lock, flags);
> -	}
> +	for (i = 0; i < desc->npins; i++)
> +		amd_gpio_irq_init_pin(gpio_dev, i);
>   }
>   
>   #ifdef CONFIG_PM_SLEEP
> @@ -950,8 +952,10 @@ static int amd_gpio_resume(struct device *dev)
>   	for (i = 0; i < desc->npins; i++) {
>   		int pin = desc->pins[i].number;
>   
> -		if (!amd_gpio_should_save(gpio_dev, pin))
> +		if (!amd_gpio_should_save(gpio_dev, pin)) {
> +			amd_gpio_irq_init_pin(gpio_dev, pin);
>   			continue;
> +		}
>   
>   		raw_spin_lock_irqsave(&gpio_dev->lock, flags);
>   		gpio_dev->saved_regs[i] |= readl(gpio_dev->base + pin * 4) & PIN_IRQ_PENDING;

Hello Kornel,

I've found that this commit which was included in 6.3-rc5 is causing a 
regression waking up from lid on a Lenovo Z13.

Reverting it on top of 6.3-rc6 resolves the problem.

I've collected what I can into this bug report:

https://bugzilla.kernel.org/show_bug.cgi?id=217315

Linus Walleij,

It looks like this was CC to stable.  If we can't get a quick solution 
we might want to pull this from stable.

Thanks,

Re: [PATCH] pinctrl: amd: Disable and mask interrupts on resume

[Gong, Richard]

On 4/10/2023 12:03 AM, Mario Limonciello wrote:
> On 3/20/23 04:32, Kornel Dulęba wrote:
>
>> This fixes a similar problem to the one observed in:
>> commit 4e5a04be88fe ("pinctrl: amd: disable and mask interrupts on 
>> probe").
>>
>> On some systems, during suspend/resume cycle firmware leaves
>> an interrupt enabled on a pin that is not used by the kernel.
>> This confuses the AMD pinctrl driver and causes spurious interrupts.
>>
>> The driver already has logic to detect if a pin is used by the kernel.
>> Leverage it to re-initialize interrupt fields of a pin only if it's not
>> used by us.
>>
>> Signed-off-by: Kornel Dulęba <korneld@chromium.org>
>> ---
>>   drivers/pinctrl/pinctrl-amd.c | 36 +++++++++++++++++++----------------
>>   1 file changed, 20 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/pinctrl/pinctrl-amd.c 
>> b/drivers/pinctrl/pinctrl-amd.c
>> index 9236a132c7ba..609821b756c2 100644
>> --- a/drivers/pinctrl/pinctrl-amd.c
>> +++ b/drivers/pinctrl/pinctrl-amd.c
>> @@ -872,32 +872,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)
>> +static void amd_gpio_irq_init_pin(struct amd_gpio *gpio_dev, int pin)
>>   {
>> -    struct pinctrl_desc *desc = gpio_dev->pctrl->desc;
>> +    const struct pin_desc *pd;
>>       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;
>> +    pd = pin_desc_get(gpio_dev->pctrl, pin);
>> +    if (!pd)
>> +        return;
>>   -        raw_spin_lock_irqsave(&gpio_dev->lock, flags);
>> +    raw_spin_lock_irqsave(&gpio_dev->lock, flags);
>> +    pin_reg = readl(gpio_dev->base + pin * 4);
>> +    pin_reg &= ~mask;
>> +    writel(pin_reg, gpio_dev->base + pin * 4);
>> +    raw_spin_unlock_irqrestore(&gpio_dev->lock, flags);
>> +}
>>   -        pin_reg = readl(gpio_dev->base + i * 4);
>> -        pin_reg &= ~mask;
>> -        writel(pin_reg, gpio_dev->base + i * 4);
>> +static void amd_gpio_irq_init(struct amd_gpio *gpio_dev)
>> +{
>> +    struct pinctrl_desc *desc = gpio_dev->pctrl->desc;
>> +    int i;
>>   -        raw_spin_unlock_irqrestore(&gpio_dev->lock, flags);
>> -    }
>> +    for (i = 0; i < desc->npins; i++)
>> +        amd_gpio_irq_init_pin(gpio_dev, i);
>>   }
>>     #ifdef CONFIG_PM_SLEEP
>> @@ -950,8 +952,10 @@ static int amd_gpio_resume(struct device *dev)
>>       for (i = 0; i < desc->npins; i++) {
>>           int pin = desc->pins[i].number;
>>   -        if (!amd_gpio_should_save(gpio_dev, pin))
>> +        if (!amd_gpio_should_save(gpio_dev, pin)) {
>> +            amd_gpio_irq_init_pin(gpio_dev, pin);
>>               continue;
>> +        }
>>             raw_spin_lock_irqsave(&gpio_dev->lock, flags);
>>           gpio_dev->saved_regs[i] |= readl(gpio_dev->base + pin * 4) 
>> & PIN_IRQ_PENDING;
>
> Hello Kornel,
>
> I've found that this commit which was included in 6.3-rc5 is causing a 
> regression waking up from lid on a Lenovo Z13.
observed "unable to wake from power button" on AMD based Dell platform. 
Reverting "pinctrl: amd: Disable and mask interrupts on resume" on the 
top of 6.3-rc6 does fix the issue.
>
> Reverting it on top of 6.3-rc6 resolves the problem.
>
> I've collected what I can into this bug report:
>
> https://bugzilla.kernel.org/show_bug.cgi?id=217315
>
> Linus Walleij,
>
> It looks like this was CC to stable.  If we can't get a quick solution 
> we might want to pull this from stable.

this commit landed into 6.1.23 as well

         d9c63daa576b2 pinctrl: amd: Disable and mask interrupts on resume

>
> Thanks,
>
>
Regards,

Richard

Re: [PATCH] pinctrl: amd: Disable and mask interrupts on resume

[Kornel Dulęba]

On Mon, Apr 10, 2023 at 5:29 PM Gong, Richard <richard.gong@amd.com> wrote:
>
> On 4/10/2023 12:03 AM, Mario Limonciello wrote:
> > On 3/20/23 04:32, Kornel Dulęba wrote:
> >
> >> This fixes a similar problem to the one observed in:
> >> commit 4e5a04be88fe ("pinctrl: amd: disable and mask interrupts on
> >> probe").
> >>
> >> On some systems, during suspend/resume cycle firmware leaves
> >> an interrupt enabled on a pin that is not used by the kernel.
> >> This confuses the AMD pinctrl driver and causes spurious interrupts.
> >>
> >> The driver already has logic to detect if a pin is used by the kernel.
> >> Leverage it to re-initialize interrupt fields of a pin only if it's not
> >> used by us.
> >>
> >> Signed-off-by: Kornel Dulęba <korneld@chromium.org>
> >> ---
> >>   drivers/pinctrl/pinctrl-amd.c | 36 +++++++++++++++++++----------------
> >>   1 file changed, 20 insertions(+), 16 deletions(-)
> >>
> >> diff --git a/drivers/pinctrl/pinctrl-amd.c
> >> b/drivers/pinctrl/pinctrl-amd.c
> >> index 9236a132c7ba..609821b756c2 100644
> >> --- a/drivers/pinctrl/pinctrl-amd.c
> >> +++ b/drivers/pinctrl/pinctrl-amd.c
> >> @@ -872,32 +872,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)
> >> +static void amd_gpio_irq_init_pin(struct amd_gpio *gpio_dev, int pin)
> >>   {
> >> -    struct pinctrl_desc *desc = gpio_dev->pctrl->desc;
> >> +    const struct pin_desc *pd;
> >>       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;
> >> +    pd = pin_desc_get(gpio_dev->pctrl, pin);
> >> +    if (!pd)
> >> +        return;
> >>   -        raw_spin_lock_irqsave(&gpio_dev->lock, flags);
> >> +    raw_spin_lock_irqsave(&gpio_dev->lock, flags);
> >> +    pin_reg = readl(gpio_dev->base + pin * 4);
> >> +    pin_reg &= ~mask;
> >> +    writel(pin_reg, gpio_dev->base + pin * 4);
> >> +    raw_spin_unlock_irqrestore(&gpio_dev->lock, flags);
> >> +}
> >>   -        pin_reg = readl(gpio_dev->base + i * 4);
> >> -        pin_reg &= ~mask;
> >> -        writel(pin_reg, gpio_dev->base + i * 4);
> >> +static void amd_gpio_irq_init(struct amd_gpio *gpio_dev)
> >> +{
> >> +    struct pinctrl_desc *desc = gpio_dev->pctrl->desc;
> >> +    int i;
> >>   -        raw_spin_unlock_irqrestore(&gpio_dev->lock, flags);
> >> -    }
> >> +    for (i = 0; i < desc->npins; i++)
> >> +        amd_gpio_irq_init_pin(gpio_dev, i);
> >>   }
> >>     #ifdef CONFIG_PM_SLEEP
> >> @@ -950,8 +952,10 @@ static int amd_gpio_resume(struct device *dev)
> >>       for (i = 0; i < desc->npins; i++) {
> >>           int pin = desc->pins[i].number;
> >>   -        if (!amd_gpio_should_save(gpio_dev, pin))
> >> +        if (!amd_gpio_should_save(gpio_dev, pin)) {
> >> +            amd_gpio_irq_init_pin(gpio_dev, pin);
> >>               continue;
> >> +        }
> >>             raw_spin_lock_irqsave(&gpio_dev->lock, flags);
> >>           gpio_dev->saved_regs[i] |= readl(gpio_dev->base + pin * 4)
> >> & PIN_IRQ_PENDING;
> >
> > Hello Kornel,
> >
> > I've found that this commit which was included in 6.3-rc5 is causing a
> > regression waking up from lid on a Lenovo Z13.
> observed "unable to wake from power button" on AMD based Dell platform.
> Reverting "pinctrl: amd: Disable and mask interrupts on resume" on the
> top of 6.3-rc6 does fix the issue.

Whoops, sorry for the breakage.
Could you please share the output of "/sys/kernel/debug/gpio" before
and after the first suspend/resume cycle.
I've looked at the patch again and found a rather silly mistake.
Please try the following.
Note that I don't have access to hardware with this controller at the
moment, so I've only compile tested it.

diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
index 609821b756c2..7e7770152ca8 100644
--- a/drivers/pinctrl/pinctrl-amd.c
+++ b/drivers/pinctrl/pinctrl-amd.c
@@ -899,7 +899,7 @@ static void amd_gpio_irq_init(struct amd_gpio *gpio_dev)
        int i;

        for (i = 0; i < desc->npins; i++)
-               amd_gpio_irq_init_pin(gpio_dev, i);
+               amd_gpio_irq_init_pin(gpio_dev, desc->pins[i].number);
 }


> >
> > Reverting it on top of 6.3-rc6 resolves the problem.
> >
> > I've collected what I can into this bug report:
> >
> > https://bugzilla.kernel.org/show_bug.cgi?id=217315
> >
> > Linus Walleij,
> >
> > It looks like this was CC to stable.  If we can't get a quick solution
> > we might want to pull this from stable.
>
> this commit landed into 6.1.23 as well
>
>          d9c63daa576b2 pinctrl: amd: Disable and mask interrupts on resume
>
> >
> > Thanks,
> >
> >
> Regards,
>
> Richard
>

Re: [PATCH] pinctrl: amd: Disable and mask interrupts on resume

[Kornel Dulęba]

On Mon, Apr 10, 2023 at 6:17 PM Kornel Dulęba <korneld@chromium.org> wrote:
>
> On Mon, Apr 10, 2023 at 5:29 PM Gong, Richard <richard.gong@amd.com> wrote:
> >
> > On 4/10/2023 12:03 AM, Mario Limonciello wrote:
> > > On 3/20/23 04:32, Kornel Dulęba wrote:
> > >
> > >> This fixes a similar problem to the one observed in:
> > >> commit 4e5a04be88fe ("pinctrl: amd: disable and mask interrupts on
> > >> probe").
> > >>
> > >> On some systems, during suspend/resume cycle firmware leaves
> > >> an interrupt enabled on a pin that is not used by the kernel.
> > >> This confuses the AMD pinctrl driver and causes spurious interrupts.
> > >>
> > >> The driver already has logic to detect if a pin is used by the kernel.
> > >> Leverage it to re-initialize interrupt fields of a pin only if it's not
> > >> used by us.
> > >>
> > >> Signed-off-by: Kornel Dulęba <korneld@chromium.org>
> > >> ---
> > >>   drivers/pinctrl/pinctrl-amd.c | 36 +++++++++++++++++++----------------
> > >>   1 file changed, 20 insertions(+), 16 deletions(-)
> > >>
> > >> diff --git a/drivers/pinctrl/pinctrl-amd.c
> > >> b/drivers/pinctrl/pinctrl-amd.c
> > >> index 9236a132c7ba..609821b756c2 100644
> > >> --- a/drivers/pinctrl/pinctrl-amd.c
> > >> +++ b/drivers/pinctrl/pinctrl-amd.c
> > >> @@ -872,32 +872,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)
> > >> +static void amd_gpio_irq_init_pin(struct amd_gpio *gpio_dev, int pin)
> > >>   {
> > >> -    struct pinctrl_desc *desc = gpio_dev->pctrl->desc;
> > >> +    const struct pin_desc *pd;
> > >>       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;
> > >> +    pd = pin_desc_get(gpio_dev->pctrl, pin);
> > >> +    if (!pd)
> > >> +        return;
> > >>   -        raw_spin_lock_irqsave(&gpio_dev->lock, flags);
> > >> +    raw_spin_lock_irqsave(&gpio_dev->lock, flags);
> > >> +    pin_reg = readl(gpio_dev->base + pin * 4);
> > >> +    pin_reg &= ~mask;
> > >> +    writel(pin_reg, gpio_dev->base + pin * 4);
> > >> +    raw_spin_unlock_irqrestore(&gpio_dev->lock, flags);
> > >> +}
> > >>   -        pin_reg = readl(gpio_dev->base + i * 4);
> > >> -        pin_reg &= ~mask;
> > >> -        writel(pin_reg, gpio_dev->base + i * 4);
> > >> +static void amd_gpio_irq_init(struct amd_gpio *gpio_dev)
> > >> +{
> > >> +    struct pinctrl_desc *desc = gpio_dev->pctrl->desc;
> > >> +    int i;
> > >>   -        raw_spin_unlock_irqrestore(&gpio_dev->lock, flags);
> > >> -    }
> > >> +    for (i = 0; i < desc->npins; i++)
> > >> +        amd_gpio_irq_init_pin(gpio_dev, i);
> > >>   }
> > >>     #ifdef CONFIG_PM_SLEEP
> > >> @@ -950,8 +952,10 @@ static int amd_gpio_resume(struct device *dev)
> > >>       for (i = 0; i < desc->npins; i++) {
> > >>           int pin = desc->pins[i].number;
> > >>   -        if (!amd_gpio_should_save(gpio_dev, pin))
> > >> +        if (!amd_gpio_should_save(gpio_dev, pin)) {
> > >> +            amd_gpio_irq_init_pin(gpio_dev, pin);
> > >>               continue;
> > >> +        }
> > >>             raw_spin_lock_irqsave(&gpio_dev->lock, flags);
> > >>           gpio_dev->saved_regs[i] |= readl(gpio_dev->base + pin * 4)
> > >> & PIN_IRQ_PENDING;
> > >
> > > Hello Kornel,
> > >
> > > I've found that this commit which was included in 6.3-rc5 is causing a
> > > regression waking up from lid on a Lenovo Z13.
> > observed "unable to wake from power button" on AMD based Dell platform.
> > Reverting "pinctrl: amd: Disable and mask interrupts on resume" on the
> > top of 6.3-rc6 does fix the issue.
>
> Whoops, sorry for the breakage.
> Could you please share the output of "/sys/kernel/debug/gpio" before
> and after the first suspend/resume cycle.
Oh and also I'd need to compare the output from this with and without
this patch reverted.
> I've looked at the patch again and found a rather silly mistake.
> Please try the following.
> Note that I don't have access to hardware with this controller at the
> moment, so I've only compile tested it.
>
> diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
> index 609821b756c2..7e7770152ca8 100644
> --- a/drivers/pinctrl/pinctrl-amd.c
> +++ b/drivers/pinctrl/pinctrl-amd.c
> @@ -899,7 +899,7 @@ static void amd_gpio_irq_init(struct amd_gpio *gpio_dev)
>         int i;
>
>         for (i = 0; i < desc->npins; i++)
> -               amd_gpio_irq_init_pin(gpio_dev, i);
> +               amd_gpio_irq_init_pin(gpio_dev, desc->pins[i].number);
>  }
>
>
> > >
> > > Reverting it on top of 6.3-rc6 resolves the problem.
> > >
> > > I've collected what I can into this bug report:
> > >
> > > https://bugzilla.kernel.org/show_bug.cgi?id=217315
> > >
> > > Linus Walleij,
> > >
> > > It looks like this was CC to stable.  If we can't get a quick solution
> > > we might want to pull this from stable.
> >
> > this commit landed into 6.1.23 as well
> >
> >          d9c63daa576b2 pinctrl: amd: Disable and mask interrupts on resume
> >
> > >
> > > Thanks,
> > >
> > >
> > Regards,
> >
> > Richard
> >

Re: [PATCH] pinctrl: amd: Disable and mask interrupts on resume

[Mario Limonciello]

On 4/10/23 11:32, Kornel Dulęba wrote:
> On Mon, Apr 10, 2023 at 6:17 PM Kornel Dulęba <korneld@chromium.org> wrote:
>> On Mon, Apr 10, 2023 at 5:29 PM Gong, Richard <richard.gong@amd.com> wrote:
>>> On 4/10/2023 12:03 AM, Mario Limonciello wrote:
>>>> On 3/20/23 04:32, Kornel Dulęba wrote:
>>>>
>>>>> This fixes a similar problem to the one observed in:
>>>>> commit 4e5a04be88fe ("pinctrl: amd: disable and mask interrupts on
>>>>> probe").
>>>>>
>>>>> On some systems, during suspend/resume cycle firmware leaves
>>>>> an interrupt enabled on a pin that is not used by the kernel.
>>>>> This confuses the AMD pinctrl driver and causes spurious interrupts.
>>>>>
>>>>> The driver already has logic to detect if a pin is used by the kernel.
>>>>> Leverage it to re-initialize interrupt fields of a pin only if it's not
>>>>> used by us.
>>>>>
>>>>> Signed-off-by: Kornel Dulęba <korneld@chromium.org>
>>>>> ---
>>>>>    drivers/pinctrl/pinctrl-amd.c | 36 +++++++++++++++++++----------------
>>>>>    1 file changed, 20 insertions(+), 16 deletions(-)
>>>>>
>>>>> diff --git a/drivers/pinctrl/pinctrl-amd.c
>>>>> b/drivers/pinctrl/pinctrl-amd.c
>>>>> index 9236a132c7ba..609821b756c2 100644
>>>>> --- a/drivers/pinctrl/pinctrl-amd.c
>>>>> +++ b/drivers/pinctrl/pinctrl-amd.c
>>>>> @@ -872,32 +872,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)
>>>>> +static void amd_gpio_irq_init_pin(struct amd_gpio *gpio_dev, int pin)
>>>>>    {
>>>>> -    struct pinctrl_desc *desc = gpio_dev->pctrl->desc;
>>>>> +    const struct pin_desc *pd;
>>>>>        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;
>>>>> +    pd = pin_desc_get(gpio_dev->pctrl, pin);
>>>>> +    if (!pd)
>>>>> +        return;
>>>>>    -        raw_spin_lock_irqsave(&gpio_dev->lock, flags);
>>>>> +    raw_spin_lock_irqsave(&gpio_dev->lock, flags);
>>>>> +    pin_reg = readl(gpio_dev->base + pin * 4);
>>>>> +    pin_reg &= ~mask;
>>>>> +    writel(pin_reg, gpio_dev->base + pin * 4);
>>>>> +    raw_spin_unlock_irqrestore(&gpio_dev->lock, flags);
>>>>> +}
>>>>>    -        pin_reg = readl(gpio_dev->base + i * 4);
>>>>> -        pin_reg &= ~mask;
>>>>> -        writel(pin_reg, gpio_dev->base + i * 4);
>>>>> +static void amd_gpio_irq_init(struct amd_gpio *gpio_dev)
>>>>> +{
>>>>> +    struct pinctrl_desc *desc = gpio_dev->pctrl->desc;
>>>>> +    int i;
>>>>>    -        raw_spin_unlock_irqrestore(&gpio_dev->lock, flags);
>>>>> -    }
>>>>> +    for (i = 0; i < desc->npins; i++)
>>>>> +        amd_gpio_irq_init_pin(gpio_dev, i);
>>>>>    }
>>>>>      #ifdef CONFIG_PM_SLEEP
>>>>> @@ -950,8 +952,10 @@ static int amd_gpio_resume(struct device *dev)
>>>>>        for (i = 0; i < desc->npins; i++) {
>>>>>            int pin = desc->pins[i].number;
>>>>>    -        if (!amd_gpio_should_save(gpio_dev, pin))
>>>>> +        if (!amd_gpio_should_save(gpio_dev, pin)) {
>>>>> +            amd_gpio_irq_init_pin(gpio_dev, pin);
>>>>>                continue;
>>>>> +        }
>>>>>              raw_spin_lock_irqsave(&gpio_dev->lock, flags);
>>>>>            gpio_dev->saved_regs[i] |= readl(gpio_dev->base + pin * 4)
>>>>> & PIN_IRQ_PENDING;
>>>> Hello Kornel,
>>>>
>>>> I've found that this commit which was included in 6.3-rc5 is causing a
>>>> regression waking up from lid on a Lenovo Z13.
>>> observed "unable to wake from power button" on AMD based Dell platform.
>>> Reverting "pinctrl: amd: Disable and mask interrupts on resume" on the
>>> top of 6.3-rc6 does fix the issue.
>> Whoops, sorry for the breakage.
>> Could you please share the output of "/sys/kernel/debug/gpio" before
>> and after the first suspend/resume cycle.
> Oh and also I'd need to compare the output from this with and without
> this patch reverted.

I've attached the requested output to the bug for all 3 cases for the Z13.

6.3-rc6 (broken lid resume)

6.3-rc6 + patch below (broken lid resume)

6.3-rc6 + revert (works from lid)

https://bugzilla.kernel.org/show_bug.cgi?id=217315

>> I've looked at the patch again and found a rather silly mistake.
>> Please try the following.
>> Note that I don't have access to hardware with this controller at the
>> moment, so I've only compile tested it.
>>
>> diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
>> index 609821b756c2..7e7770152ca8 100644
>> --- a/drivers/pinctrl/pinctrl-amd.c
>> +++ b/drivers/pinctrl/pinctrl-amd.c
>> @@ -899,7 +899,7 @@ static void amd_gpio_irq_init(struct amd_gpio *gpio_dev)
>>          int i;
>>
>>          for (i = 0; i < desc->npins; i++)
>> -               amd_gpio_irq_init_pin(gpio_dev, i);
>> +               amd_gpio_irq_init_pin(gpio_dev, desc->pins[i].number);
>>   }
>>
This unfortunately doesn't help the behavior at all.
>>>> Reverting it on top of 6.3-rc6 resolves the problem.
>>>>
>>>> I've collected what I can into this bug report:
>>>>
>>>> https://bugzilla.kernel.org/show_bug.cgi?id=217315
>>>>
>>>> Linus Walleij,
>>>>
>>>> It looks like this was CC to stable.  If we can't get a quick solution
>>>> we might want to pull this from stable.
>>> this commit landed into 6.1.23 as well
>>>
>>>           d9c63daa576b2 pinctrl: amd: Disable and mask interrupts on resume
>>>
>>>> Thanks,
>>>>
>>>>
>>> Regards,
>>>
>>> Richard
>>>

Re: [PATCH] pinctrl: amd: Disable and mask interrupts on resume

[Linux regression tracking (Thorsten Leemhuis)]

On 10.04.23 17:29, Gong, Richard wrote:
> On 4/10/2023 12:03 AM, Mario Limonciello wrote:
>> On 3/20/23 04:32, Kornel Dulęba wrote:
>>
>>> This fixes a similar problem to the one observed in:
>>> commit 4e5a04be88fe ("pinctrl: amd: disable and mask interrupts on
>>> probe").
>>>
>>> On some systems, during suspend/resume cycle firmware leaves
>>> an interrupt enabled on a pin that is not used by the kernel.
>>> This confuses the AMD pinctrl driver and causes spurious interrupts.
>>>
>>> The driver already has logic to detect if a pin is used by the kernel.
>>> Leverage it to re-initialize interrupt fields of a pin only if it's not
>>> used by us.
>>>
>>> Signed-off-by: Kornel Dulęba <korneld@chromium.org>
>>> ---
>>>   drivers/pinctrl/pinctrl-amd.c | 36 +++++++++++++++++++----------------
>>>   1 file changed, 20 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/drivers/pinctrl/pinctrl-amd.c
>>> b/drivers/pinctrl/pinctrl-amd.c
>>> index 9236a132c7ba..609821b756c2 100644
>>> --- a/drivers/pinctrl/pinctrl-amd.c
>>> +++ b/drivers/pinctrl/pinctrl-amd.c
>>> @@ -872,32 +872,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)
>>> +static void amd_gpio_irq_init_pin(struct amd_gpio *gpio_dev, int pin)
>>>   {
>>> -    struct pinctrl_desc *desc = gpio_dev->pctrl->desc;
>>> +    const struct pin_desc *pd;
>>>       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;
>>> +    pd = pin_desc_get(gpio_dev->pctrl, pin);
>>> +    if (!pd)
>>> +        return;
>>>   -        raw_spin_lock_irqsave(&gpio_dev->lock, flags);
>>> +    raw_spin_lock_irqsave(&gpio_dev->lock, flags);
>>> +    pin_reg = readl(gpio_dev->base + pin * 4);
>>> +    pin_reg &= ~mask;
>>> +    writel(pin_reg, gpio_dev->base + pin * 4);
>>> +    raw_spin_unlock_irqrestore(&gpio_dev->lock, flags);
>>> +}
>>>   -        pin_reg = readl(gpio_dev->base + i * 4);
>>> -        pin_reg &= ~mask;
>>> -        writel(pin_reg, gpio_dev->base + i * 4);
>>> +static void amd_gpio_irq_init(struct amd_gpio *gpio_dev)
>>> +{
>>> +    struct pinctrl_desc *desc = gpio_dev->pctrl->desc;
>>> +    int i;
>>>   -        raw_spin_unlock_irqrestore(&gpio_dev->lock, flags);
>>> -    }
>>> +    for (i = 0; i < desc->npins; i++)
>>> +        amd_gpio_irq_init_pin(gpio_dev, i);
>>>   }
>>>     #ifdef CONFIG_PM_SLEEP
>>> @@ -950,8 +952,10 @@ static int amd_gpio_resume(struct device *dev)
>>>       for (i = 0; i < desc->npins; i++) {
>>>           int pin = desc->pins[i].number;
>>>   -        if (!amd_gpio_should_save(gpio_dev, pin))
>>> +        if (!amd_gpio_should_save(gpio_dev, pin)) {
>>> +            amd_gpio_irq_init_pin(gpio_dev, pin);
>>>               continue;
>>> +        }
>>>             raw_spin_lock_irqsave(&gpio_dev->lock, flags);
>>>           gpio_dev->saved_regs[i] |= readl(gpio_dev->base + pin * 4)
>>> & PIN_IRQ_PENDING;
>>
>> Hello Kornel,
>>
>> I've found that this commit which was included in 6.3-rc5 is causing a
>> regression waking up from lid on a Lenovo Z13.
> observed "unable to wake from power button" on AMD based Dell platform.

This sounds like something that we want to fix quickly.

> Reverting "pinctrl: amd: Disable and mask interrupts on resume" on the
> top of 6.3-rc6 does fix the issue.
>>
>> Reverting it on top of 6.3-rc6 resolves the problem.
>>
>> I've collected what I can into this bug report:
>>
>> https://bugzilla.kernel.org/show_bug.cgi?id=217315
>>
>> Linus Walleij,
>>
>> It looks like this was CC to stable.  If we can't get a quick solution
>> we might want to pull this from stable.
> 
> this commit landed into 6.1.23 as well
> 
>         d9c63daa576b2 pinctrl: amd: Disable and mask interrupts on resume

It made it back up to 5.10.y afaics.

The culprit has no fixes tag, which makes me wonder: should we quickly
(e.g. today) revert this in mainline to get back to the previous state,
so that Greg can pick up the revert for the next stable releases he
apparently currently prepares?

Greg, is there another way to make you quickly fix this in the stable
trees? One option obviously would be "revert this now in stable, reapply
it later together with a fix ". But I'm under the impression that this
is too much of a hassle and thus something you only do in dire situations?

I'm asking because I over time noticed that quite a few regressions are
in a similar situation -- and quite a few of them take quite some time
to get fixed even when a developer provided a fix, because reviewing and
mainlining the fix takes a week or two (sometimes more). And that is a
situation that is more and more hitting a nerve here. :-/

Ciao, Thorsten

Re: [PATCH] pinctrl: amd: Disable and mask interrupts on resume

[Kornel Dulęba]

On Tue, Apr 11, 2023 at 2:50 PM Linux regression tracking (Thorsten
Leemhuis) <regressions@leemhuis.info> wrote:
>
>
>
> On 10.04.23 17:29, Gong, Richard wrote:
> > On 4/10/2023 12:03 AM, Mario Limonciello wrote:
> >> On 3/20/23 04:32, Kornel Dulęba wrote:
> >>
> >>> This fixes a similar problem to the one observed in:
> >>> commit 4e5a04be88fe ("pinctrl: amd: disable and mask interrupts on
> >>> probe").
> >>>
> >>> On some systems, during suspend/resume cycle firmware leaves
> >>> an interrupt enabled on a pin that is not used by the kernel.
> >>> This confuses the AMD pinctrl driver and causes spurious interrupts.
> >>>
> >>> The driver already has logic to detect if a pin is used by the kernel.
> >>> Leverage it to re-initialize interrupt fields of a pin only if it's not
> >>> used by us.
> >>>
> >>> Signed-off-by: Kornel Dulęba <korneld@chromium.org>
> >>> ---
> >>>   drivers/pinctrl/pinctrl-amd.c | 36 +++++++++++++++++++----------------
> >>>   1 file changed, 20 insertions(+), 16 deletions(-)
> >>>
> >>> diff --git a/drivers/pinctrl/pinctrl-amd.c
> >>> b/drivers/pinctrl/pinctrl-amd.c
> >>> index 9236a132c7ba..609821b756c2 100644
> >>> --- a/drivers/pinctrl/pinctrl-amd.c
> >>> +++ b/drivers/pinctrl/pinctrl-amd.c
> >>> @@ -872,32 +872,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)
> >>> +static void amd_gpio_irq_init_pin(struct amd_gpio *gpio_dev, int pin)
> >>>   {
> >>> -    struct pinctrl_desc *desc = gpio_dev->pctrl->desc;
> >>> +    const struct pin_desc *pd;
> >>>       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;
> >>> +    pd = pin_desc_get(gpio_dev->pctrl, pin);
> >>> +    if (!pd)
> >>> +        return;
> >>>   -        raw_spin_lock_irqsave(&gpio_dev->lock, flags);
> >>> +    raw_spin_lock_irqsave(&gpio_dev->lock, flags);
> >>> +    pin_reg = readl(gpio_dev->base + pin * 4);
> >>> +    pin_reg &= ~mask;
> >>> +    writel(pin_reg, gpio_dev->base + pin * 4);
> >>> +    raw_spin_unlock_irqrestore(&gpio_dev->lock, flags);
> >>> +}
> >>>   -        pin_reg = readl(gpio_dev->base + i * 4);
> >>> -        pin_reg &= ~mask;
> >>> -        writel(pin_reg, gpio_dev->base + i * 4);
> >>> +static void amd_gpio_irq_init(struct amd_gpio *gpio_dev)
> >>> +{
> >>> +    struct pinctrl_desc *desc = gpio_dev->pctrl->desc;
> >>> +    int i;
> >>>   -        raw_spin_unlock_irqrestore(&gpio_dev->lock, flags);
> >>> -    }
> >>> +    for (i = 0; i < desc->npins; i++)
> >>> +        amd_gpio_irq_init_pin(gpio_dev, i);
> >>>   }
> >>>     #ifdef CONFIG_PM_SLEEP
> >>> @@ -950,8 +952,10 @@ static int amd_gpio_resume(struct device *dev)
> >>>       for (i = 0; i < desc->npins; i++) {
> >>>           int pin = desc->pins[i].number;
> >>>   -        if (!amd_gpio_should_save(gpio_dev, pin))
> >>> +        if (!amd_gpio_should_save(gpio_dev, pin)) {
> >>> +            amd_gpio_irq_init_pin(gpio_dev, pin);
> >>>               continue;
> >>> +        }
> >>>             raw_spin_lock_irqsave(&gpio_dev->lock, flags);
> >>>           gpio_dev->saved_regs[i] |= readl(gpio_dev->base + pin * 4)
> >>> & PIN_IRQ_PENDING;
> >>
> >> Hello Kornel,
> >>
> >> I've found that this commit which was included in 6.3-rc5 is causing a
> >> regression waking up from lid on a Lenovo Z13.
> > observed "unable to wake from power button" on AMD based Dell platform.
>
> This sounds like something that we want to fix quickly.
>
> > Reverting "pinctrl: amd: Disable and mask interrupts on resume" on the
> > top of 6.3-rc6 does fix the issue.
> >>
> >> Reverting it on top of 6.3-rc6 resolves the problem.
> >>
> >> I've collected what I can into this bug report:
> >>
> >> https://bugzilla.kernel.org/show_bug.cgi?id=217315
> >>
> >> Linus Walleij,
> >>
> >> It looks like this was CC to stable.  If we can't get a quick solution
> >> we might want to pull this from stable.
> >
> > this commit landed into 6.1.23 as well
> >
> >         d9c63daa576b2 pinctrl: amd: Disable and mask interrupts on resume
>
> It made it back up to 5.10.y afaics.
>
> The culprit has no fixes tag, which makes me wonder: should we quickly
> (e.g. today) revert this in mainline to get back to the previous state,
> so that Greg can pick up the revert for the next stable releases he
> apparently currently prepares?
>
> Greg, is there another way to make you quickly fix this in the stable
> trees? One option obviously would be "revert this now in stable, reapply
> it later together with a fix ". But I'm under the impression that this
> is too much of a hassle and thus something you only do in dire situations?
>
> I'm asking because I over time noticed that quite a few regressions are
> in a similar situation -- and quite a few of them take quite some time
> to get fixed even when a developer provided a fix, because reviewing and
> mainlining the fix takes a week or two (sometimes more). And that is a
> situation that is more and more hitting a nerve here. :-/

I've looked into this and at this moment I can't really find a quick fix.
See https://bugzilla.kernel.org/show_bug.cgi?id=217315#c3.
It seems that reverting this might be the best solution for now.

Regards
Kornel

Re: [PATCH] pinctrl: amd: Disable and mask interrupts on resume

[Linux regression tracking (Thorsten Leemhuis)]

On 11.04.23 15:09, Kornel Dulęba wrote:
> On Tue, Apr 11, 2023 at 2:50 PM Linux regression tracking (Thorsten
> Leemhuis) <regressions@leemhuis.info> wrote:
>> On 10.04.23 17:29, Gong, Richard wrote:
>>> On 4/10/2023 12:03 AM, Mario Limonciello wrote:
>>>> On 3/20/23 04:32, Kornel Dulęba wrote:
>>>>
>>>>> This fixes a similar problem to the one observed in:
>>>>> commit 4e5a04be88fe ("pinctrl: amd: disable and mask interrupts on
>>>>> probe").
>>>>>
>>>>> On some systems, during suspend/resume cycle firmware leaves
>>>>> an interrupt enabled on a pin that is not used by the kernel.
>>>>> This confuses the AMD pinctrl driver and causes spurious interrupts.
>>>>>
>>>>> The driver already has logic to detect if a pin is used by the kernel.
>>>>> Leverage it to re-initialize interrupt fields of a pin only if it's not
>>>>> used by us.
>>>>>
>>>>> Signed-off-by: Kornel Dulęba <korneld@chromium.org>
>>>>> ---
>>>>>   drivers/pinctrl/pinctrl-amd.c | 36 +++++++++++++++++++----------------
>>>>>   1 file changed, 20 insertions(+), 16 deletions(-)
>>>>>
>>>>> diff --git a/drivers/pinctrl/pinctrl-amd.c
>>>>> b/drivers/pinctrl/pinctrl-amd.c
>>>>> index 9236a132c7ba..609821b756c2 100644
>>>>> --- a/drivers/pinctrl/pinctrl-amd.c
>>>>> +++ b/drivers/pinctrl/pinctrl-amd.c
>>>>> @@ -872,32 +872,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)
>>>>> +static void amd_gpio_irq_init_pin(struct amd_gpio *gpio_dev, int pin)
>>>>>   {
>>>>> -    struct pinctrl_desc *desc = gpio_dev->pctrl->desc;
>>>>> +    const struct pin_desc *pd;
>>>>>       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;
>>>>> +    pd = pin_desc_get(gpio_dev->pctrl, pin);
>>>>> +    if (!pd)
>>>>> +        return;
>>>>>   -        raw_spin_lock_irqsave(&gpio_dev->lock, flags);
>>>>> +    raw_spin_lock_irqsave(&gpio_dev->lock, flags);
>>>>> +    pin_reg = readl(gpio_dev->base + pin * 4);
>>>>> +    pin_reg &= ~mask;
>>>>> +    writel(pin_reg, gpio_dev->base + pin * 4);
>>>>> +    raw_spin_unlock_irqrestore(&gpio_dev->lock, flags);
>>>>> +}
>>>>>   -        pin_reg = readl(gpio_dev->base + i * 4);
>>>>> -        pin_reg &= ~mask;
>>>>> -        writel(pin_reg, gpio_dev->base + i * 4);
>>>>> +static void amd_gpio_irq_init(struct amd_gpio *gpio_dev)
>>>>> +{
>>>>> +    struct pinctrl_desc *desc = gpio_dev->pctrl->desc;
>>>>> +    int i;
>>>>>   -        raw_spin_unlock_irqrestore(&gpio_dev->lock, flags);
>>>>> -    }
>>>>> +    for (i = 0; i < desc->npins; i++)
>>>>> +        amd_gpio_irq_init_pin(gpio_dev, i);
>>>>>   }
>>>>>     #ifdef CONFIG_PM_SLEEP
>>>>> @@ -950,8 +952,10 @@ static int amd_gpio_resume(struct device *dev)
>>>>>       for (i = 0; i < desc->npins; i++) {
>>>>>           int pin = desc->pins[i].number;
>>>>>   -        if (!amd_gpio_should_save(gpio_dev, pin))
>>>>> +        if (!amd_gpio_should_save(gpio_dev, pin)) {
>>>>> +            amd_gpio_irq_init_pin(gpio_dev, pin);
>>>>>               continue;
>>>>> +        }
>>>>>             raw_spin_lock_irqsave(&gpio_dev->lock, flags);
>>>>>           gpio_dev->saved_regs[i] |= readl(gpio_dev->base + pin * 4)
>>>>> & PIN_IRQ_PENDING;
>>>>
>>>> Hello Kornel,
>>>>
>>>> I've found that this commit which was included in 6.3-rc5 is causing a
>>>> regression waking up from lid on a Lenovo Z13.
>>> observed "unable to wake from power button" on AMD based Dell platform.
>>
>> This sounds like something that we want to fix quickly.
>>
>>> Reverting "pinctrl: amd: Disable and mask interrupts on resume" on the
>>> top of 6.3-rc6 does fix the issue.
>>>>
>>>> Reverting it on top of 6.3-rc6 resolves the problem.
>>>>
>>>> I've collected what I can into this bug report:
>>>>
>>>> https://bugzilla.kernel.org/show_bug.cgi?id=217315
>>>>
>>>> Linus Walleij,
>>>>
>>>> It looks like this was CC to stable.  If we can't get a quick solution
>>>> we might want to pull this from stable.
>>>
>>> this commit landed into 6.1.23 as well
>>>
>>>         d9c63daa576b2 pinctrl: amd: Disable and mask interrupts on resume
>>
>> It made it back up to 5.10.y afaics.
>>
>> The culprit has no fixes tag, which makes me wonder: should we quickly
>> (e.g. today) revert this in mainline to get back to the previous state,
>> so that Greg can pick up the revert for the next stable releases he
>> apparently currently prepares?
>>
>> Greg, is there another way to make you quickly fix this in the stable
>> trees? One option obviously would be "revert this now in stable, reapply
>> it later together with a fix ". But I'm under the impression that this
>> is too much of a hassle and thus something you only do in dire situations?
>>
>> I'm asking because I over time noticed that quite a few regressions are
>> in a similar situation -- and quite a few of them take quite some time
>> to get fixed even when a developer provided a fix, because reviewing and
>> mainlining the fix takes a week or two (sometimes more). And that is a
>> situation that is more and more hitting a nerve here. :-/
> 
> I've looked into this and at this moment I can't really find a quick fix.
> See https://bugzilla.kernel.org/show_bug.cgi?id=217315#c3.
> It seems that reverting this might be the best solution for now.

Great, thx for the update (and BTW: Greg, thx for your answer, too).

To speed things up a quick question:

Linusw, what's your preferred course to realize this revert quickly?

 * someone (Kornel?) sends a revert with a commit msg for review, which
you then apply and pass on to the other Linus?

 * someone (Kornel?) sends a revert with a commit msg for review that
immediately asks the other Linus to pick this up directly?

 * we ask the other Linus directly to revert this (who then has to come
up with a commit msg on his own)?

Ciao, Thorsten

Re: [PATCH] pinctrl: amd: Disable and mask interrupts on resume

[Kornel Dulęba]

On Tue, Apr 11, 2023 at 3:29 PM Linux regression tracking (Thorsten
Leemhuis) <regressions@leemhuis.info> wrote:
>
> On 11.04.23 15:09, Kornel Dulęba wrote:
> > On Tue, Apr 11, 2023 at 2:50 PM Linux regression tracking (Thorsten
> > Leemhuis) <regressions@leemhuis.info> wrote:
> >> On 10.04.23 17:29, Gong, Richard wrote:
> >>> On 4/10/2023 12:03 AM, Mario Limonciello wrote:
> >>>> On 3/20/23 04:32, Kornel Dulęba wrote:
> >>>>
> >>>>> This fixes a similar problem to the one observed in:
> >>>>> commit 4e5a04be88fe ("pinctrl: amd: disable and mask interrupts on
> >>>>> probe").
> >>>>>
> >>>>> On some systems, during suspend/resume cycle firmware leaves
> >>>>> an interrupt enabled on a pin that is not used by the kernel.
> >>>>> This confuses the AMD pinctrl driver and causes spurious interrupts.
> >>>>>
> >>>>> The driver already has logic to detect if a pin is used by the kernel.
> >>>>> Leverage it to re-initialize interrupt fields of a pin only if it's not
> >>>>> used by us.
> >>>>>
> >>>>> Signed-off-by: Kornel Dulęba <korneld@chromium.org>
> >>>>> ---
> >>>>>   drivers/pinctrl/pinctrl-amd.c | 36 +++++++++++++++++++----------------
> >>>>>   1 file changed, 20 insertions(+), 16 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/pinctrl/pinctrl-amd.c
> >>>>> b/drivers/pinctrl/pinctrl-amd.c
> >>>>> index 9236a132c7ba..609821b756c2 100644
> >>>>> --- a/drivers/pinctrl/pinctrl-amd.c
> >>>>> +++ b/drivers/pinctrl/pinctrl-amd.c
> >>>>> @@ -872,32 +872,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)
> >>>>> +static void amd_gpio_irq_init_pin(struct amd_gpio *gpio_dev, int pin)
> >>>>>   {
> >>>>> -    struct pinctrl_desc *desc = gpio_dev->pctrl->desc;
> >>>>> +    const struct pin_desc *pd;
> >>>>>       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;
> >>>>> +    pd = pin_desc_get(gpio_dev->pctrl, pin);
> >>>>> +    if (!pd)
> >>>>> +        return;
> >>>>>   -        raw_spin_lock_irqsave(&gpio_dev->lock, flags);
> >>>>> +    raw_spin_lock_irqsave(&gpio_dev->lock, flags);
> >>>>> +    pin_reg = readl(gpio_dev->base + pin * 4);
> >>>>> +    pin_reg &= ~mask;
> >>>>> +    writel(pin_reg, gpio_dev->base + pin * 4);
> >>>>> +    raw_spin_unlock_irqrestore(&gpio_dev->lock, flags);
> >>>>> +}
> >>>>>   -        pin_reg = readl(gpio_dev->base + i * 4);
> >>>>> -        pin_reg &= ~mask;
> >>>>> -        writel(pin_reg, gpio_dev->base + i * 4);
> >>>>> +static void amd_gpio_irq_init(struct amd_gpio *gpio_dev)
> >>>>> +{
> >>>>> +    struct pinctrl_desc *desc = gpio_dev->pctrl->desc;
> >>>>> +    int i;
> >>>>>   -        raw_spin_unlock_irqrestore(&gpio_dev->lock, flags);
> >>>>> -    }
> >>>>> +    for (i = 0; i < desc->npins; i++)
> >>>>> +        amd_gpio_irq_init_pin(gpio_dev, i);
> >>>>>   }
> >>>>>     #ifdef CONFIG_PM_SLEEP
> >>>>> @@ -950,8 +952,10 @@ static int amd_gpio_resume(struct device *dev)
> >>>>>       for (i = 0; i < desc->npins; i++) {
> >>>>>           int pin = desc->pins[i].number;
> >>>>>   -        if (!amd_gpio_should_save(gpio_dev, pin))
> >>>>> +        if (!amd_gpio_should_save(gpio_dev, pin)) {
> >>>>> +            amd_gpio_irq_init_pin(gpio_dev, pin);
> >>>>>               continue;
> >>>>> +        }
> >>>>>             raw_spin_lock_irqsave(&gpio_dev->lock, flags);
> >>>>>           gpio_dev->saved_regs[i] |= readl(gpio_dev->base + pin * 4)
> >>>>> & PIN_IRQ_PENDING;
> >>>>
> >>>> Hello Kornel,
> >>>>
> >>>> I've found that this commit which was included in 6.3-rc5 is causing a
> >>>> regression waking up from lid on a Lenovo Z13.
> >>> observed "unable to wake from power button" on AMD based Dell platform.
> >>
> >> This sounds like something that we want to fix quickly.
> >>
> >>> Reverting "pinctrl: amd: Disable and mask interrupts on resume" on the
> >>> top of 6.3-rc6 does fix the issue.
> >>>>
> >>>> Reverting it on top of 6.3-rc6 resolves the problem.
> >>>>
> >>>> I've collected what I can into this bug report:
> >>>>
> >>>> https://bugzilla.kernel.org/show_bug.cgi?id=217315
> >>>>
> >>>> Linus Walleij,
> >>>>
> >>>> It looks like this was CC to stable.  If we can't get a quick solution
> >>>> we might want to pull this from stable.
> >>>
> >>> this commit landed into 6.1.23 as well
> >>>
> >>>         d9c63daa576b2 pinctrl: amd: Disable and mask interrupts on resume
> >>
> >> It made it back up to 5.10.y afaics.
> >>
> >> The culprit has no fixes tag, which makes me wonder: should we quickly
> >> (e.g. today) revert this in mainline to get back to the previous state,
> >> so that Greg can pick up the revert for the next stable releases he
> >> apparently currently prepares?
> >>
> >> Greg, is there another way to make you quickly fix this in the stable
> >> trees? One option obviously would be "revert this now in stable, reapply
> >> it later together with a fix ". But I'm under the impression that this
> >> is too much of a hassle and thus something you only do in dire situations?
> >>
> >> I'm asking because I over time noticed that quite a few regressions are
> >> in a similar situation -- and quite a few of them take quite some time
> >> to get fixed even when a developer provided a fix, because reviewing and
> >> mainlining the fix takes a week or two (sometimes more). And that is a
> >> situation that is more and more hitting a nerve here. :-/
> >
> > I've looked into this and at this moment I can't really find a quick fix.
> > See https://bugzilla.kernel.org/show_bug.cgi?id=217315#c3.
> > It seems that reverting this might be the best solution for now.
>
> Great, thx for the update (and BTW: Greg, thx for your answer, too).
>
> To speed things up a quick question:
>
> Linusw, what's your preferred course to realize this revert quickly?
>
>  * someone (Kornel?) sends a revert with a commit msg for review, which
> you then apply and pass on to the other Linus?
>
>  * someone (Kornel?) sends a revert with a commit msg for review that
> immediately asks the other Linus to pick this up directly?
>
>  * we ask the other Linus directly to revert this (who then has to come
> up with a commit msg on his own)?

Would you like me to send a reverting change?
I can do this right away.
The commit message would contain something like:

This patch introduces a regression on Lenovo Z13, which can't wake
from the lid with it applied.

Re: [PATCH] pinctrl: amd: Disable and mask interrupts on resume

[Linux regression tracking (Thorsten Leemhuis)]

On 11.04.23 15:35, Kornel Dulęba wrote:
> On Tue, Apr 11, 2023 at 3:29 PM Linux regression tracking (Thorsten
> Leemhuis) <regressions@leemhuis.info> wrote:
>>
>> On 11.04.23 15:09, Kornel Dulęba wrote:
>>> On Tue, Apr 11, 2023 at 2:50 PM Linux regression tracking (Thorsten
>>> Leemhuis) <regressions@leemhuis.info> wrote:
>>>> On 10.04.23 17:29, Gong, Richard wrote:
>>>>> On 4/10/2023 12:03 AM, Mario Limonciello wrote:
>>>>>> On 3/20/23 04:32, Kornel Dulęba wrote:
>>>>>>
>>>>>>> This fixes a similar problem to the one observed in:
>>>>>>> commit 4e5a04be88fe ("pinctrl: amd: disable and mask interrupts on
>>>>>>> probe").
>>>>>>>
>>>>>>> On some systems, during suspend/resume cycle firmware leaves
>>>>>>> an interrupt enabled on a pin that is not used by the kernel.
>>>>>>> This confuses the AMD pinctrl driver and causes spurious interrupts.
>>>>>>>
>>>>>>> The driver already has logic to detect if a pin is used by the kernel.
>>>>>>> Leverage it to re-initialize interrupt fields of a pin only if it's not
>>>>>>> used by us.
>>>>>>>
>>>>>>> Signed-off-by: Kornel Dulęba <korneld@chromium.org>
>>>>>>> ---
>>>>>>>   drivers/pinctrl/pinctrl-amd.c | 36 +++++++++++++++++++----------------
>>>>>>>   1 file changed, 20 insertions(+), 16 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/pinctrl/pinctrl-amd.c
>>>>>>> b/drivers/pinctrl/pinctrl-amd.c
>>>>>>> index 9236a132c7ba..609821b756c2 100644
>>>>>>> --- a/drivers/pinctrl/pinctrl-amd.c
>>>>>>> +++ b/drivers/pinctrl/pinctrl-amd.c
>>>>>>> @@ -872,32 +872,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)
>>>>>>> +static void amd_gpio_irq_init_pin(struct amd_gpio *gpio_dev, int pin)
>>>>>>>   {
>>>>>>> -    struct pinctrl_desc *desc = gpio_dev->pctrl->desc;
>>>>>>> +    const struct pin_desc *pd;
>>>>>>>       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;
>>>>>>> +    pd = pin_desc_get(gpio_dev->pctrl, pin);
>>>>>>> +    if (!pd)
>>>>>>> +        return;
>>>>>>>   -        raw_spin_lock_irqsave(&gpio_dev->lock, flags);
>>>>>>> +    raw_spin_lock_irqsave(&gpio_dev->lock, flags);
>>>>>>> +    pin_reg = readl(gpio_dev->base + pin * 4);
>>>>>>> +    pin_reg &= ~mask;
>>>>>>> +    writel(pin_reg, gpio_dev->base + pin * 4);
>>>>>>> +    raw_spin_unlock_irqrestore(&gpio_dev->lock, flags);
>>>>>>> +}
>>>>>>>   -        pin_reg = readl(gpio_dev->base + i * 4);
>>>>>>> -        pin_reg &= ~mask;
>>>>>>> -        writel(pin_reg, gpio_dev->base + i * 4);
>>>>>>> +static void amd_gpio_irq_init(struct amd_gpio *gpio_dev)
>>>>>>> +{
>>>>>>> +    struct pinctrl_desc *desc = gpio_dev->pctrl->desc;
>>>>>>> +    int i;
>>>>>>>   -        raw_spin_unlock_irqrestore(&gpio_dev->lock, flags);
>>>>>>> -    }
>>>>>>> +    for (i = 0; i < desc->npins; i++)
>>>>>>> +        amd_gpio_irq_init_pin(gpio_dev, i);
>>>>>>>   }
>>>>>>>     #ifdef CONFIG_PM_SLEEP
>>>>>>> @@ -950,8 +952,10 @@ static int amd_gpio_resume(struct device *dev)
>>>>>>>       for (i = 0; i < desc->npins; i++) {
>>>>>>>           int pin = desc->pins[i].number;
>>>>>>>   -        if (!amd_gpio_should_save(gpio_dev, pin))
>>>>>>> +        if (!amd_gpio_should_save(gpio_dev, pin)) {
>>>>>>> +            amd_gpio_irq_init_pin(gpio_dev, pin);
>>>>>>>               continue;
>>>>>>> +        }
>>>>>>>             raw_spin_lock_irqsave(&gpio_dev->lock, flags);
>>>>>>>           gpio_dev->saved_regs[i] |= readl(gpio_dev->base + pin * 4)
>>>>>>> & PIN_IRQ_PENDING;
>>>>>>
>>>>>> Hello Kornel,
>>>>>>
>>>>>> I've found that this commit which was included in 6.3-rc5 is causing a
>>>>>> regression waking up from lid on a Lenovo Z13.
>>>>> observed "unable to wake from power button" on AMD based Dell platform.
>>>>
>>>> This sounds like something that we want to fix quickly.
>>>>
>>>>> Reverting "pinctrl: amd: Disable and mask interrupts on resume" on the
>>>>> top of 6.3-rc6 does fix the issue.
>>>>>>
>>>>>> Reverting it on top of 6.3-rc6 resolves the problem.
>>>>>>
>>>>>> I've collected what I can into this bug report:
>>>>>>
>>>>>> https://bugzilla.kernel.org/show_bug.cgi?id=217315
>>>>>>
>>>>>> Linus Walleij,
>>>>>>
>>>>>> It looks like this was CC to stable.  If we can't get a quick solution
>>>>>> we might want to pull this from stable.
>>>>>
>>>>> this commit landed into 6.1.23 as well
>>>>>
>>>>>         d9c63daa576b2 pinctrl: amd: Disable and mask interrupts on resume
>>>>
>>>> It made it back up to 5.10.y afaics.
>>>>
>>>> The culprit has no fixes tag, which makes me wonder: should we quickly
>>>> (e.g. today) revert this in mainline to get back to the previous state,
>>>> so that Greg can pick up the revert for the next stable releases he
>>>> apparently currently prepares?
>>>>
>>>> Greg, is there another way to make you quickly fix this in the stable
>>>> trees? One option obviously would be "revert this now in stable, reapply
>>>> it later together with a fix ". But I'm under the impression that this
>>>> is too much of a hassle and thus something you only do in dire situations?
>>>>
>>>> I'm asking because I over time noticed that quite a few regressions are
>>>> in a similar situation -- and quite a few of them take quite some time
>>>> to get fixed even when a developer provided a fix, because reviewing and
>>>> mainlining the fix takes a week or two (sometimes more). And that is a
>>>> situation that is more and more hitting a nerve here. :-/
>>>
>>> I've looked into this and at this moment I can't really find a quick fix.
>>> See https://bugzilla.kernel.org/show_bug.cgi?id=217315#c3.
>>> It seems that reverting this might be the best solution for now.
>>
>> Great, thx for the update (and BTW: Greg, thx for your answer, too).
>>
>> To speed things up a quick question:
>>
>> Linusw, what's your preferred course to realize this revert quickly?
>>
>>  * someone (Kornel?) sends a revert with a commit msg for review, which
>> you then apply and pass on to the other Linus?
>>
>>  * someone (Kornel?) sends a revert with a commit msg for review that
>> immediately asks the other Linus to pick this up directly?
>>
>>  * we ask the other Linus directly to revert this (who then has to come
>> up with a commit msg on his own)?
> 
> Would you like me to send a reverting change?
> I can do this right away.

Guess it would be helpful, as then we are down to option one or two.
Many thx!

> The commit message would contain something like:
> 
> This patch introduces a regression on Lenovo Z13, which can't wake
> from the lid with it applied.

Maybe add "; and some unspecified AMD based Dell platforms are unable to
wake from hitting the power button". (see Richard's mail earlier in the
thread).

Ciao, Thorsten

Re: [PATCH] pinctrl: amd: Disable and mask interrupts on resume

[Linus Walleij]

On Tue, Apr 11, 2023 at 3:35 PM Kornel Dulęba <korneld@chromium.org> wrote:
> On Tue, Apr 11, 2023 at 3:29 PM Linux regression tracking (Thorsten
> Leemhuis) <regressions@leemhuis.info> wrote:

> > Linusw, what's your preferred course to realize this revert quickly?
> >
> >  * someone (Kornel?) sends a revert with a commit msg for review, which
> > you then apply and pass on to the other Linus?
> >
> >  * someone (Kornel?) sends a revert with a commit msg for review that
> > immediately asks the other Linus to pick this up directly?
> >
> >  * we ask the other Linus directly to revert this (who then has to come
> > up with a commit msg on his own)?
>
> Would you like me to send a reverting change?
> I can do this right away.

Yeah that is best.

> The commit message would contain something like:
>
> This patch introduces a regression on Lenovo Z13, which can't wake
> from the lid with it applied.

Looks good!

Thorsten: thanks for watching over this issue, we'll have it
resolved pronto.

Yours,
Linus Walleij

Re: [PATCH] pinctrl: amd: Disable and mask interrupts on resume

[Linux regression tracking (Thorsten Leemhuis)]

On 11.04.23 22:44, Linus Walleij wrote:
> On Tue, Apr 11, 2023 at 3:35 PM Kornel Dulęba <korneld@chromium.org> wrote:
>> On Tue, Apr 11, 2023 at 3:29 PM Linux regression tracking (Thorsten
>> Leemhuis) <regressions@leemhuis.info> wrote:
> 
>>> Linusw, what's your preferred course to realize this revert quickly?
>>>
>>>  * someone (Kornel?) sends a revert with a commit msg for review, which
>>> you then apply and pass on to the other Linus?
>>>
>>>  * someone (Kornel?) sends a revert with a commit msg for review that
>>> immediately asks the other Linus to pick this up directly?
>>>
>>>  * we ask the other Linus directly to revert this (who then has to come
>>> up with a commit msg on his own)?
>>
>> Would you like me to send a reverting change?
>> I can do this right away.
> 
> Yeah that is best.
> 
>> The commit message would contain something like:
>>
>> This patch introduces a regression on Lenovo Z13, which can't wake
>> from the lid with it applied.
> 
> Looks good!
> 
> Thorsten: thanks for watching over this issue, we'll have it
> resolved pronto.

Thx for saying that, in cases like this there is always something in my
head that says "maybe the developers and maintainers would have done
everything just the same way and just as quickly, even if I hadn't made
so much annoying noise". But well, better save that sorry.

Side note: I sometimes wonder if reverts like this maybe should be send
directly to LinusT for mainlining with a ACK from the maintainer (e.g.
you in this case) to speed things up. Sure, having things in next for at
least a day (and running it through subsystem specific CI) can help
finding unexpected problems, but it delays things, too -- which
sometimes might result in a revert coming to late for a new stable
release. And that easily can mean that our users have to face known
issues for about one more week. :-/

Ciao, Thorsten

/me meanwhile wonders if Greg will still pick the revert up for the next
stable releases (currently in rc), if the revert reaches mainline before
they are released on Friday

/me will wait for the revert to hit mainline before he'll prod Greg
about it

Re: [PATCH] pinctrl: amd: Disable and mask interrupts on resume

[Greg KH]

On Tue, Apr 11, 2023 at 02:50:40PM +0200, Linux regression tracking (Thorsten Leemhuis) wrote:
> 
> 
> On 10.04.23 17:29, Gong, Richard wrote:
> > On 4/10/2023 12:03 AM, Mario Limonciello wrote:
> >> On 3/20/23 04:32, Kornel Dulęba wrote:
> >>
> >>> This fixes a similar problem to the one observed in:
> >>> commit 4e5a04be88fe ("pinctrl: amd: disable and mask interrupts on
> >>> probe").
> >>>
> >>> On some systems, during suspend/resume cycle firmware leaves
> >>> an interrupt enabled on a pin that is not used by the kernel.
> >>> This confuses the AMD pinctrl driver and causes spurious interrupts.
> >>>
> >>> The driver already has logic to detect if a pin is used by the kernel.
> >>> Leverage it to re-initialize interrupt fields of a pin only if it's not
> >>> used by us.
> >>>
> >>> Signed-off-by: Kornel Dulęba <korneld@chromium.org>
> >>> ---
> >>>   drivers/pinctrl/pinctrl-amd.c | 36 +++++++++++++++++++----------------
> >>>   1 file changed, 20 insertions(+), 16 deletions(-)
> >>>
> >>> diff --git a/drivers/pinctrl/pinctrl-amd.c
> >>> b/drivers/pinctrl/pinctrl-amd.c
> >>> index 9236a132c7ba..609821b756c2 100644
> >>> --- a/drivers/pinctrl/pinctrl-amd.c
> >>> +++ b/drivers/pinctrl/pinctrl-amd.c
> >>> @@ -872,32 +872,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)
> >>> +static void amd_gpio_irq_init_pin(struct amd_gpio *gpio_dev, int pin)
> >>>   {
> >>> -    struct pinctrl_desc *desc = gpio_dev->pctrl->desc;
> >>> +    const struct pin_desc *pd;
> >>>       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;
> >>> +    pd = pin_desc_get(gpio_dev->pctrl, pin);
> >>> +    if (!pd)
> >>> +        return;
> >>>   -        raw_spin_lock_irqsave(&gpio_dev->lock, flags);
> >>> +    raw_spin_lock_irqsave(&gpio_dev->lock, flags);
> >>> +    pin_reg = readl(gpio_dev->base + pin * 4);
> >>> +    pin_reg &= ~mask;
> >>> +    writel(pin_reg, gpio_dev->base + pin * 4);
> >>> +    raw_spin_unlock_irqrestore(&gpio_dev->lock, flags);
> >>> +}
> >>>   -        pin_reg = readl(gpio_dev->base + i * 4);
> >>> -        pin_reg &= ~mask;
> >>> -        writel(pin_reg, gpio_dev->base + i * 4);
> >>> +static void amd_gpio_irq_init(struct amd_gpio *gpio_dev)
> >>> +{
> >>> +    struct pinctrl_desc *desc = gpio_dev->pctrl->desc;
> >>> +    int i;
> >>>   -        raw_spin_unlock_irqrestore(&gpio_dev->lock, flags);
> >>> -    }
> >>> +    for (i = 0; i < desc->npins; i++)
> >>> +        amd_gpio_irq_init_pin(gpio_dev, i);
> >>>   }
> >>>     #ifdef CONFIG_PM_SLEEP
> >>> @@ -950,8 +952,10 @@ static int amd_gpio_resume(struct device *dev)
> >>>       for (i = 0; i < desc->npins; i++) {
> >>>           int pin = desc->pins[i].number;
> >>>   -        if (!amd_gpio_should_save(gpio_dev, pin))
> >>> +        if (!amd_gpio_should_save(gpio_dev, pin)) {
> >>> +            amd_gpio_irq_init_pin(gpio_dev, pin);
> >>>               continue;
> >>> +        }
> >>>             raw_spin_lock_irqsave(&gpio_dev->lock, flags);
> >>>           gpio_dev->saved_regs[i] |= readl(gpio_dev->base + pin * 4)
> >>> & PIN_IRQ_PENDING;
> >>
> >> Hello Kornel,
> >>
> >> I've found that this commit which was included in 6.3-rc5 is causing a
> >> regression waking up from lid on a Lenovo Z13.
> > observed "unable to wake from power button" on AMD based Dell platform.
> 
> This sounds like something that we want to fix quickly.
> 
> > Reverting "pinctrl: amd: Disable and mask interrupts on resume" on the
> > top of 6.3-rc6 does fix the issue.
> >>
> >> Reverting it on top of 6.3-rc6 resolves the problem.
> >>
> >> I've collected what I can into this bug report:
> >>
> >> https://bugzilla.kernel.org/show_bug.cgi?id=217315
> >>
> >> Linus Walleij,
> >>
> >> It looks like this was CC to stable.  If we can't get a quick solution
> >> we might want to pull this from stable.
> > 
> > this commit landed into 6.1.23 as well
> > 
> >         d9c63daa576b2 pinctrl: amd: Disable and mask interrupts on resume
> 
> It made it back up to 5.10.y afaics.
> 
> The culprit has no fixes tag, which makes me wonder: should we quickly
> (e.g. today) revert this in mainline to get back to the previous state,
> so that Greg can pick up the revert for the next stable releases he
> apparently currently prepares?
> 
> Greg, is there another way to make you quickly fix this in the stable
> trees? One option obviously would be "revert this now in stable, reapply
> it later together with a fix ". But I'm under the impression that this
> is too much of a hassle and thus something you only do in dire situations?

I would like to see this reverted in Linus's tree first please.

thanks,

greg k-h