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
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);@@ -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 resumeThanks,
Regards,
Richard
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);@@ -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 resumeThanks,
Regards,
Richard
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);@@ -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 resumeThanks,
Regards,
Richard
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
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);@@ -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 resumeIt 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
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);@@ -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 resumeIt 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
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);@@ -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 resumeIt 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.
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 resumeIt 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
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
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
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
linux-stable-mirror@lists.linaro.org
-
Gong, Richard -
Greg KH -
Kornel Dulęba -
Linus Walleij -
Linux regression tracking (Thorsten Leemhuis) -
Mario Limonciello