commit 5467801f1fcb ("gpio: Restrict usage of GPIO chip irq members before initialization") attempted to fix a race condition that lead to a NULL pointer, but in the process caused a regression for _AEI/_EVT declared GPIOs. This manifests in messages showing deferred probing while trying to allocate IRQs like so:
[ 0.688318] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x0000 to IRQ, err -517 [ 0.688337] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x002C to IRQ, err -517 [ 0.688348] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x003D to IRQ, err -517 [ 0.688359] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x003E to IRQ, err -517 [ 0.688369] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x003A to IRQ, err -517 [ 0.688379] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x003B to IRQ, err -517 [ 0.688389] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x0002 to IRQ, err -517 [ 0.688399] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x0011 to IRQ, err -517 [ 0.688410] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x0012 to IRQ, err -517 [ 0.688420] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x0007 to IRQ, err -517
The code for walking _AEI doesn't handle deferred probing and so this leads to non-functional GPIO interrupts.
Fix this issue by moving the call to `acpi_gpiochip_request_interrupts` to occur after gc->irc.initialized is set.
Cc: Shreeya Patel shreeya.patel@collabora.com Cc: stable@vger.kernel.org Fixes: 5467801f1fcb ("gpio: Restrict usage of GPIO chip irq members before initialization") Reported-by: Mario Limonciello mario.limonciello@amd.com Link: https://lore.kernel.org/linux-gpio/BL1PR12MB51577A77F000A008AA694675E2EF9@BL... Signed-off-by: Mario Limonciello mario.limonciello@amd.com --- drivers/gpio/gpiolib.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 085348e08986..b7694171655c 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -1601,8 +1601,6 @@ static int gpiochip_add_irqchip(struct gpio_chip *gc,
gpiochip_set_irq_hooks(gc);
- acpi_gpiochip_request_interrupts(gc); - /* * Using barrier() here to prevent compiler from reordering * gc->irq.initialized before initialization of above @@ -1612,6 +1610,8 @@ static int gpiochip_add_irqchip(struct gpio_chip *gc,
gc->irq.initialized = true;
+ acpi_gpiochip_request_interrupts(gc); + return 0; }
On Thu, Apr 14, 2022 at 5:57 AM Mario Limonciello mario.limonciello@amd.com wrote:
commit 5467801f1fcb ("gpio: Restrict usage of GPIO chip irq members before initialization") attempted to fix a race condition that lead to a NULL pointer, but in the process caused a regression for _AEI/_EVT declared GPIOs. This manifests in messages showing deferred probing while trying to allocate IRQs like so:
[ 0.688318] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x0000 to IRQ, err -517 [ 0.688337] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x002C to IRQ, err -517 [ 0.688348] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x003D to IRQ, err -517 [ 0.688359] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x003E to IRQ, err -517 [ 0.688369] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x003A to IRQ, err -517 [ 0.688379] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x003B to IRQ, err -517 [ 0.688389] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x0002 to IRQ, err -517 [ 0.688399] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x0011 to IRQ, err -517 [ 0.688410] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x0012 to IRQ, err -517 [ 0.688420] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x0007 to IRQ, err -517
The code for walking _AEI doesn't handle deferred probing and so this leads to non-functional GPIO interrupts.
Fix this issue by moving the call to `acpi_gpiochip_request_interrupts` to occur after gc->irc.initialized is set.
Good catch, Mario, and thanks for the prompt fix! Reviewed-by: Andy Shevchenko andy.shevchenko@gmail.com
Cc: Shreeya Patel shreeya.patel@collabora.com Cc: stable@vger.kernel.org Fixes: 5467801f1fcb ("gpio: Restrict usage of GPIO chip irq members before initialization") Reported-by: Mario Limonciello mario.limonciello@amd.com Link: https://lore.kernel.org/linux-gpio/BL1PR12MB51577A77F000A008AA694675E2EF9@BL... Signed-off-by: Mario Limonciello mario.limonciello@amd.com
drivers/gpio/gpiolib.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 085348e08986..b7694171655c 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -1601,8 +1601,6 @@ static int gpiochip_add_irqchip(struct gpio_chip *gc,
gpiochip_set_irq_hooks(gc);
acpi_gpiochip_request_interrupts(gc);/* * Using barrier() here to prevent compiler from reordering * gc->irq.initialized before initialization of above@@ -1612,6 +1610,8 @@ static int gpiochip_add_irqchip(struct gpio_chip *gc,
gc->irq.initialized = true;
acpi_gpiochip_request_interrupts(gc);return 0;}
-- 2.34.1
On 14/04/22 08:27, Mario Limonciello wrote:
commit 5467801f1fcb ("gpio: Restrict usage of GPIO chip irq members before initialization") attempted to fix a race condition that lead to a NULL pointer, but in the process caused a regression for _AEI/_EVT declared GPIOs. This manifests in messages showing deferred probing while trying to allocate IRQs like so:
[ 0.688318] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x0000 to IRQ, err -517 [ 0.688337] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x002C to IRQ, err -517 [ 0.688348] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x003D to IRQ, err -517 [ 0.688359] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x003E to IRQ, err -517 [ 0.688369] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x003A to IRQ, err -517 [ 0.688379] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x003B to IRQ, err -517 [ 0.688389] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x0002 to IRQ, err -517 [ 0.688399] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x0011 to IRQ, err -517 [ 0.688410] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x0012 to IRQ, err -517 [ 0.688420] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x0007 to IRQ, err -517
The code for walking _AEI doesn't handle deferred probing and so this leads to non-functional GPIO interrupts.
Fix this issue by moving the call to `acpi_gpiochip_request_interrupts` to occur after gc->irc.initialized is set.
Thanks Mario for sending a fix for this issue, I didn't realize gpiod_to_irq() was also being called through acpi_gpiochip_request_interrupts(). Change looks good to me. Reviewed-by: Shreeya Patel shreeya.patel@collabora.com
Cc: Shreeya Patel shreeya.patel@collabora.com Cc: stable@vger.kernel.org Fixes: 5467801f1fcb ("gpio: Restrict usage of GPIO chip irq members before initialization") Reported-by: Mario Limonciello mario.limonciello@amd.com Link: https://lore.kernel.org/linux-gpio/BL1PR12MB51577A77F000A008AA694675E2EF9@BL... Signed-off-by: Mario Limonciello mario.limonciello@amd.com
drivers/gpio/gpiolib.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 085348e08986..b7694171655c 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -1601,8 +1601,6 @@ static int gpiochip_add_irqchip(struct gpio_chip *gc, gpiochip_set_irq_hooks(gc);
- acpi_gpiochip_request_interrupts(gc);
- /*
- Using barrier() here to prevent compiler from reordering
- gc->irq.initialized before initialization of above
@@ -1612,6 +1610,8 @@ static int gpiochip_add_irqchip(struct gpio_chip *gc, gc->irq.initialized = true;
- acpi_gpiochip_request_interrupts(gc);
- return 0; }
Tested-By: lukeluk498@gmail.com Link: https://gitlab.freedesktop.org/drm/amd/-/issues/1976
On 4/14/22 05:57, Mario Limonciello wrote:
commit 5467801f1fcb ("gpio: Restrict usage of GPIO chip irq members before initialization") attempted to fix a race condition that lead to a NULL pointer, but in the process caused a regression for _AEI/_EVT declared GPIOs. This manifests in messages showing deferred probing while trying to allocate IRQs like so:
[ 0.688318] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x0000 to IRQ, err -517 [ 0.688337] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x002C to IRQ, err -517 [ 0.688348] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x003D to IRQ, err -517 [ 0.688359] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x003E to IRQ, err -517 [ 0.688369] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x003A to IRQ, err -517 [ 0.688379] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x003B to IRQ, err -517 [ 0.688389] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x0002 to IRQ, err -517 [ 0.688399] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x0011 to IRQ, err -517 [ 0.688410] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x0012 to IRQ, err -517 [ 0.688420] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x0007 to IRQ, err -517
The code for walking _AEI doesn't handle deferred probing and so this leads to non-functional GPIO interrupts.
Fix this issue by moving the call to `acpi_gpiochip_request_interrupts` to occur after gc->irc.initialized is set.
Cc: Shreeya Patel shreeya.patel@collabora.com Cc: stable@vger.kernel.org Fixes: 5467801f1fcb ("gpio: Restrict usage of GPIO chip irq members before initialization") Reported-by: Mario Limonciello mario.limonciello@amd.com Link: https://lore.kernel.org/linux-gpio/BL1PR12MB51577A77F000A008AA694675E2EF9@BL... Signed-off-by: Mario Limonciello mario.limonciello@amd.com
drivers/gpio/gpiolib.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 085348e08986..b7694171655c 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -1601,8 +1601,6 @@ static int gpiochip_add_irqchip(struct gpio_chip *gc, gpiochip_set_irq_hooks(gc);
- acpi_gpiochip_request_interrupts(gc);
- /*
- Using barrier() here to prevent compiler from reordering
- gc->irq.initialized before initialization of above
@@ -1612,6 +1610,8 @@ static int gpiochip_add_irqchip(struct gpio_chip *gc, gc->irq.initialized = true;
- acpi_gpiochip_request_interrupts(gc);
- return 0; }
Tested-By:firew4lker@gmail.com
This patch addresses the issue. Tested on a Lenovo T14 with AMD Ryzen 5 PRO 4650U with Radeon Graphics Graphics.
Without this patch the laptop is impossible to wake from S3 and S0ix.
On 4/17/22 07:24, firew4lker wrote:
On 4/14/22 05:57, Mario Limonciello wrote:
commit 5467801f1fcb ("gpio: Restrict usage of GPIO chip irq members before initialization") attempted to fix a race condition that lead to a NULL pointer, but in the process caused a regression for _AEI/_EVT declared GPIOs. This manifests in messages showing deferred probing while trying to allocate IRQs like so:
[ 0.688318] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x0000 to IRQ, err -517 [ 0.688337] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x002C to IRQ, err -517 [ 0.688348] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x003D to IRQ, err -517 [ 0.688359] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x003E to IRQ, err -517 [ 0.688369] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x003A to IRQ, err -517 [ 0.688379] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x003B to IRQ, err -517 [ 0.688389] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x0002 to IRQ, err -517 [ 0.688399] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x0011 to IRQ, err -517 [ 0.688410] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x0012 to IRQ, err -517 [ 0.688420] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x0007 to IRQ, err -517
The code for walking _AEI doesn't handle deferred probing and so this leads to non-functional GPIO interrupts.
Fix this issue by moving the call to `acpi_gpiochip_request_interrupts` to occur after gc->irc.initialized is set.
Cc: Shreeya Patel shreeya.patel@collabora.com Cc: stable@vger.kernel.org Fixes: 5467801f1fcb ("gpio: Restrict usage of GPIO chip irq members before initialization") Reported-by: Mario Limonciello mario.limonciello@amd.com Link: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kerne...
Signed-off-by: Mario Limonciello mario.limonciello@amd.com
drivers/gpio/gpiolib.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 085348e08986..b7694171655c 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -1601,8 +1601,6 @@ static int gpiochip_add_irqchip(struct gpio_chip *gc, gpiochip_set_irq_hooks(gc); - acpi_gpiochip_request_interrupts(gc);
/* * Using barrier() here to prevent compiler from reordering * gc->irq.initialized before initialization of above @@ -1612,6 +1610,8 @@ static int gpiochip_add_irqchip(struct gpio_chip *gc, gc->irq.initialized = true; + acpi_gpiochip_request_interrupts(gc);
return 0; }
Tested-By:firew4lker@gmail.com
This patch addresses the issue. Tested on a Lenovo T14 with AMD Ryzen 5 PRO 4650U with Radeon Graphics Graphics.
Without this patch the laptop is impossible to wake from S3 and S0ix.
Thanks for testing it!
Linus Walleij,
As this is backported to 5.15.y, 5.16.y, 5.17.y and those all had point releases a bunch of people are hitting it now. If you choose to adopt this patch instead of revert the broken one, you can add to the commit message too:
On Mon, Apr 18, 2022 at 7:34 AM Mario Limonciello mario.limonciello@amd.com wrote:
On 4/17/22 07:24, firew4lker wrote:
...
Linus Walleij,
As this is backported to 5.15.y, 5.16.y, 5.17.y and those all had point releases a bunch of people are hitting it now. If you choose to adopt this patch instead of revert the broken one, you can add to the commit message too:
I prefer to explicitly tell that this is a link to a bug report, hence BugLink:. But this is just my 2 cents.
On 18.04.22 13:42, Andy Shevchenko wrote:
On Mon, Apr 18, 2022 at 7:34 AM Mario Limonciello mario.limonciello@amd.com wrote:
On 4/17/22 07:24, firew4lker wrote:
...
Linus Walleij,
As this is backported to 5.15.y, 5.16.y, 5.17.y and those all had point releases a bunch of people are hitting it now. If you choose to adopt this patch instead of revert the broken one, you can add to the commit message too:
I prefer to explicitly tell that this is a link to a bug report, hence BugLink:. But this is just my 2 cents.
Please use "Link:" as explained by the kernel's documentation in Documentation/process/submitting-patches.rst Documentation/process/5.Posting.rst (disclaimer: I recently made this more explicit, but the concept it old). That's important, as people have tools that rely on it -- I for example run one to track regressions, but I might not be the only one running a tool that relies on proper tags.
And FWIW: I'm all for making this more explicit, but people already use various different tags (BugLink is just one of them) for that and that just results in a mess. I proposed consistent tags, but that didn't get much feedback. Maybe I should try again. Makes me wonder: where does BugLink come from? Is that something that people are used to from GitLab, GitHub, or something?
Ciao, Thorsten
On Mon, Apr 18, 2022 at 4:58 PM Thorsten Leemhuis regressions@leemhuis.info wrote:
On 18.04.22 13:42, Andy Shevchenko wrote:
On Mon, Apr 18, 2022 at 7:34 AM Mario Limonciello mario.limonciello@amd.com wrote:
On 4/17/22 07:24, firew4lker wrote:
...
Linus Walleij,
As this is backported to 5.15.y, 5.16.y, 5.17.y and those all had point releases a bunch of people are hitting it now. If you choose to adopt this patch instead of revert the broken one, you can add to the commit message too:
I prefer to explicitly tell that this is a link to a bug report, hence BugLink:. But this is just my 2 cents.
Please use "Link:" as explained by the kernel's documentation in Documentation/process/submitting-patches.rst Documentation/process/5.Posting.rst (disclaimer: I recently made this more explicit, but the concept it old). That's important, as people have tools that rely on it -- I for example run one to track regressions, but I might not be the only one running a tool that relies on proper tags.
To me it looks like a documentation confusion since Link is what is added automatically by `b4` tool. Having Link from the patch thread (and not always the one with the discussion) as well as link to the issue will be confusing.
And FWIW: I'm all for making this more explicit, but people already use various different tags (BugLink is just one of them) for that and that just results in a mess.
Nope, it results otherwise. The Link is Link to the thread, which you may find a lot in the kernel history. Making bug report links and links to the patch threads that's what results in a mess.
I proposed consistent tags, but that didn't get much feedback. Maybe I should try again. Makes me wonder: where does BugLink come from? Is that something that people are used to from GitLab, GitHub, or something?
It comes from kernel history :-)
On 18.04.22 16:17, Andy Shevchenko wrote:
On Mon, Apr 18, 2022 at 4:58 PM Thorsten Leemhuis regressions@leemhuis.info wrote:
On 18.04.22 13:42, Andy Shevchenko wrote:
On Mon, Apr 18, 2022 at 7:34 AM Mario Limonciello mario.limonciello@amd.com wrote:
On 4/17/22 07:24, firew4lker wrote:
...
Linus Walleij,
As this is backported to 5.15.y, 5.16.y, 5.17.y and those all had point releases a bunch of people are hitting it now. If you choose to adopt this patch instead of revert the broken one, you can add to the commit message too:
I prefer to explicitly tell that this is a link to a bug report, hence BugLink:. But this is just my 2 cents.
Please use "Link:" as explained by the kernel's documentation in Documentation/process/submitting-patches.rst Documentation/process/5.Posting.rst (disclaimer: I recently made this more explicit, but the concept it old). That's important, as people have tools that rely on it -- I for example run one to track regressions, but I might not be the only one running a tool that relies on proper tags.
To me it looks like a documentation confusion since Link is what is added automatically by `b4` tool.
Since some time now, yes, but the "Link:" tags are way older and used to link to all sorts of places that are relevant.
Having Link from the patch thread (and not always the one with the discussion) as well as link to the issue will be confusing.
Yup, but that's how it is for years already (and in the muscle memory of some -- that's why I might make sense to teach b4 to set something else, but that's a different story). Linus himself does it like that. Recent commits showing that are for example 901c7280ca0d or 0313bc278dac. And for links bug trackers, too, as 80d47f5de5e3 or 14e3e989f6a5 show.
And FWIW: I'm all for making this more explicit, but people already use various different tags (BugLink is just one of them) for that and that just results in a mess.
Nope, it results otherwise. The Link is Link to the thread, which you may find a lot in the kernel history. Making bug report links and links to the patch threads that's what results in a mess.
Yeah, but we are in that mess already and people inventing different tags; some of the DRM people for example use(d?) "References", but there were others iirc.
I proposed consistent tags, but that didn't get much feedback. Maybe I should try again. Makes me wonder: where does BugLink come from? Is that something that people are used to from GitLab, GitHub, or something?
It comes from kernel history :-)
Okay, thx, had just been wondering if people are used to it from some platform.
Ciao, Thorsten
[Public]
-----Original Message----- From: Limonciello, Mario Sent: Sunday, April 17, 2022 23:35 To: firew4lker firew4lker@gmail.com; Linus Walleij linus.walleij@linaro.org; Bartosz Golaszewski brgl@bgdev.pl; Andy Shevchenko andy.shevchenko@gmail.com; Shreeya Patel shreeya.patel@collabora.com; open list:GPIO SUBSYSTEM <linux- gpio@vger.kernel.org>; open list linux-kernel@vger.kernel.org Cc: Natikar, Basavaraj Basavaraj.Natikar@amd.com; Gong, Richard Richard.Gong@amd.com; stable@vger.kernel.org Subject: Re: [PATCH] gpio: Request interrupts after IRQ is initialized
On 4/17/22 07:24, firew4lker wrote:
On 4/14/22 05:57, Mario Limonciello wrote:
commit 5467801f1fcb ("gpio: Restrict usage of GPIO chip irq members before initialization") attempted to fix a race condition that lead to a NULL pointer, but in the process caused a regression for _AEI/_EVT declared GPIOs. This manifests in messages showing deferred probing while trying to allocate IRQs like so:
[ 0.688318] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x0000 to IRQ, err -517 [ 0.688337] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x002C to IRQ, err -517 [ 0.688348] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x003D to IRQ, err -517 [ 0.688359] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x003E to IRQ, err -517 [ 0.688369] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x003A to IRQ, err -517 [ 0.688379] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x003B to IRQ, err -517 [ 0.688389] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x0002 to IRQ, err -517 [ 0.688399] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x0011 to IRQ, err -517 [ 0.688410] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x0012 to IRQ, err -517 [ 0.688420] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x0007 to IRQ, err -517
The code for walking _AEI doesn't handle deferred probing and so this leads to non-functional GPIO interrupts.
Fix this issue by moving the call to `acpi_gpiochip_request_interrupts` to occur after gc->irc.initialized is set.
Cc: Shreeya Patel shreeya.patel@collabora.com Cc: stable@vger.kernel.org Fixes: 5467801f1fcb ("gpio: Restrict usage of GPIO chip irq members before initialization") Reported-by: Mario Limonciello mario.limonciello@amd.com Link:
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore. kernel.org%2Flinux- gpio%2FBL1PR12MB51577A77F000A008AA694675E2EF9%40BL1PR12MB5157. namprd12.prod.outlook.com%2FT%2F%23u&data=04%7C01%7Cmario.li monciello%40amd.com%7C96ec39c78488493fd5ca08da206d3c7b%7C3dd8961 fe4884e608e11a82d994e183d%7C0%7C0%7C637857951204650754%7CUnkno wn%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1 haWwiLCJXVCI6Mn0%3D%7C3000&sdata=xZbNC%2F50JqlNwcTYAtGLn6 z0%2FEPbfCKKOc%2BlZlMh0EQ%3D&reserved=0
Signed-off-by: Mario Limonciello mario.limonciello@amd.com
drivers/gpio/gpiolib.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 085348e08986..b7694171655c 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -1601,8 +1601,6 @@ static int gpiochip_add_irqchip(struct gpio_chip *gc, gpiochip_set_irq_hooks(gc); - acpi_gpiochip_request_interrupts(gc);
/* * Using barrier() here to prevent compiler from reordering * gc->irq.initialized before initialization of above @@ -1612,6 +1610,8 @@ static int gpiochip_add_irqchip(struct gpio_chip *gc, gc->irq.initialized = true; + acpi_gpiochip_request_interrupts(gc);
return 0; }
Tested-By:firew4lker@gmail.com
This patch addresses the issue. Tested on a Lenovo T14 with AMD Ryzen 5 PRO 4650U with Radeon Graphics Graphics.
Without this patch the laptop is impossible to wake from S3 and S0ix.
Thanks for testing it!
Linus Walleij,
As this is backported to 5.15.y, 5.16.y, 5.17.y and those all had point releases a bunch of people are hitting it now. If you choose to adopt this patch instead of revert the broken one, you can add to the commit message too:
Here's a few more links to add.
Link: https://gitlab.freedesktop.org/drm/amd/-/issues/1979
Link: https://bugzilla.kernel.org/show_bug.cgi?id=215850
Thanks,
On Mon, Apr 18, 2022 at 6:34 AM Mario Limonciello mario.limonciello@amd.com wrote:
Linus Walleij,
As this is backported to 5.15.y, 5.16.y, 5.17.y and those all had point releases a bunch of people are hitting it now. If you choose to adopt this patch instead of revert the broken one, you can add to the commit message too:
I am on parental leave kind of, but Bartosz knows what to do, in this case, since it is ACPI-related, Andy knows best what to do, and I see he also replied.
Yours, Linus Walleij
On 20.04.22 00:02, Linus Walleij wrote:
On Mon, Apr 18, 2022 at 6:34 AM Mario Limonciello mario.limonciello@amd.com wrote:
Linus Walleij,
As this is backported to 5.15.y, 5.16.y, 5.17.y and those all had point releases a bunch of people are hitting it now. If you choose to adopt this patch instead of revert the broken one, you can add to the commit message too:
I am on parental leave kind of, but Bartosz knows what to do, in this case, since it is ACPI-related, Andy knows best what to do, and I see he also replied.
Bartosz, Andy, what's the status here? It looks like the patch didn't make any progress in the past few days (or did I miss it?). I'd really like to see this patch or a revert of 5467801f1fcb ("gpio: Restrict usage of GPIO chip irq members before initialization") mainlined by rc4, so Greg (CCed) can fix it in the next round of stable updates, as it seems quite a few people are affected by the problem.
Reminder: this is one of those issue that we IMHO really should fix quickly, as explained by a text recently added to the documentation:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Docu...
Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
P.S.: As the Linux kernel's regression tracker I deal with a lot of reports and sometimes miss something important when writing mails like this. If that's the case here, don't hesitate to tell me in a public reply, it's in everyone's interest to set the public record straight.
On 21.04.22 18:07, Thorsten Leemhuis wrote:
On 20.04.22 00:02, Linus Walleij wrote:
On Mon, Apr 18, 2022 at 6:34 AM Mario Limonciello mario.limonciello@amd.com wrote:
Linus Walleij,
As this is backported to 5.15.y, 5.16.y, 5.17.y and those all had point releases a bunch of people are hitting it now. If you choose to adopt this patch instead of revert the broken one, you can add to the commit message too:
I am on parental leave kind of, but Bartosz knows what to do, in this case, since it is ACPI-related, Andy knows best what to do, and I see he also replied.
Bartosz, Andy, what's the status here? It looks like the patch didn't make any progress in the past few days (or did I miss it?). I'd really like to see this patch or a revert of 5467801f1fcb ("gpio: Restrict usage of GPIO chip irq members before initialization") mainlined by rc4, so Greg (CCed) can fix it in the next round of stable updates, as it seems quite a few people are affected by the problem.
Mario, are you aware if this patch made any progress towards getting merged? If not, I wonder if we (you?) maybe should ask Linus to pick this up directly giving the circumstances to speed things up (or maybe a v2 that incorporates all the Reviewed-by/ACKs that accumulated).
Ciao, Thorsten
Reminder: this is one of those issue that we IMHO really should fix quickly, as explained by a text recently added to the documentation:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Docu...
Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
P.S.: As the Linux kernel's regression tracker I deal with a lot of reports and sometimes miss something important when writing mails like this. If that's the case here, don't hesitate to tell me in a public reply, it's in everyone's interest to set the public record straight.
[Public]
-----Original Message----- From: Thorsten Leemhuis regressions@leemhuis.info Sent: Friday, April 22, 2022 07:19 To: Limonciello, Mario Mario.Limonciello@amd.com Cc: firew4lker firew4lker@gmail.com; Bartosz Golaszewski brgl@bgdev.pl; Andy Shevchenko andy.shevchenko@gmail.com; Shreeya Patel shreeya.patel@collabora.com; open list:GPIO SUBSYSTEM linux-gpio@vger.kernel.org; open list linux-kernel@vger.kernel.org; Natikar, Basavaraj Basavaraj.Natikar@amd.com; Gong, Richard Richard.Gong@amd.com; stable@vger.kernel.org; Greg KH gregkh@linuxfoundation.org; regressions@lists.linux.dev; Linus Walleij linus.walleij@linaro.org Subject: Re: [PATCH] gpio: Request interrupts after IRQ is initialized
On 21.04.22 18:07, Thorsten Leemhuis wrote:
On 20.04.22 00:02, Linus Walleij wrote:
On Mon, Apr 18, 2022 at 6:34 AM Mario Limonciello mario.limonciello@amd.com wrote:
Linus Walleij,
As this is backported to 5.15.y, 5.16.y, 5.17.y and those all had point releases a bunch of people are hitting it now. If you choose to adopt this patch instead of revert the broken one, you can add to the commit message too:
Link:
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitla b.freedesktop.org%2Fdrm%2Famd%2F- %2Fissues%2F1976&data=05%7C01%7Cmario.limonciello%40amd.com% 7C1bfce4a8186b4b1a4ef208da245a4411%7C3dd8961fe4884e608e11a82d994e 183d%7C0%7C0%7C637862267399285803%7CUnknown%7CTWFpbGZsb3d8ey JWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D% 7C3000%7C%7C%7C&sdata=cJvkTifRPkrl8kRaYffMmb0RGkoHb0krYMkj2 Ufsg5k%3D&reserved=0
I am on parental leave kind of, but Bartosz knows what to do, in this case, since it is ACPI-related, Andy knows best what to do, and I see he also replied.
Bartosz, Andy, what's the status here? It looks like the patch didn't make any progress in the past few days (or did I miss it?). I'd really like to see this patch or a revert of 5467801f1fcb ("gpio: Restrict usage of GPIO chip irq members before initialization") mainlined by rc4, so Greg (CCed) can fix it in the next round of stable updates, as it seems quite a few people are affected by the problem.
Mario, are you aware if this patch made any progress towards getting merged? If not, I wonder if we (you?) maybe should ask Linus to pick this up directly giving the circumstances to speed things up (or maybe a v2 that incorporates all the Reviewed-by/ACKs that accumulated).
I don't see it in Bartosz or Andy's trees. I'm fine to send up a v2 directly to Linus with this audience on CC.
Ciao, Thorsten
Reminder: this is one of those issue that we IMHO really should fix quickly, as explained by a text recently added to the documentation:
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.k ernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git% 2Ftree%2FDocumentation%2Fprocess%2Fhandling- regressions.rst%23n131&data=05%7C01%7Cmario.limonciello%40amd.c om%7C1bfce4a8186b4b1a4ef208da245a4411%7C3dd8961fe4884e608e11a82d 994e183d%7C0%7C0%7C637862267399285803%7CUnknown%7CTWFpbGZsb3 d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0 %3D%7C3000%7C%7C%7C&sdata=ttOhIvDHhi8fIjrpuNcgXKeZaM%2F%2 BaosUhn2mGEaC67M%3D&reserved=0
Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
P.S.: As the Linux kernel's regression tracker I deal with a lot of reports and sometimes miss something important when writing mails like this. If that's the case here, don't hesitate to tell me in a public reply, it's in everyone's interest to set the public record straight.
Hi, this is your Linux kernel regression tracker. Top-posting for once, to make this easily accessible to everyone.
Greg, this seems to be a regression that made the news https://www.reddit.com/r/linux/comments/u5hbk6/psa_linux_5173_on_dell_amd_la...
That made me wonder "how can we get issues like this fixed really quickly in stable". Are you in cases like this maybe willing to drop the backport of 5467801f1fcb quickly (in this case maybe even for the new stable kernel versions that just were sent out as rc1) and then reapply it later together with below fix once that was reviewed and merged to mainline? Or is that too much of a hassle even for special case like this?
Ciao, Thorsten
On 14.04.22 04:57, Mario Limonciello wrote:
commit 5467801f1fcb ("gpio: Restrict usage of GPIO chip irq members before initialization") attempted to fix a race condition that lead to a NULL pointer, but in the process caused a regression for _AEI/_EVT declared GPIOs. This manifests in messages showing deferred probing while trying to allocate IRQs like so:
[ 0.688318] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x0000 to IRQ, err -517 [ 0.688337] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x002C to IRQ, err -517 [ 0.688348] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x003D to IRQ, err -517 [ 0.688359] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x003E to IRQ, err -517 [ 0.688369] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x003A to IRQ, err -517 [ 0.688379] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x003B to IRQ, err -517 [ 0.688389] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x0002 to IRQ, err -517 [ 0.688399] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x0011 to IRQ, err -517 [ 0.688410] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x0012 to IRQ, err -517 [ 0.688420] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x0007 to IRQ, err -517
The code for walking _AEI doesn't handle deferred probing and so this leads to non-functional GPIO interrupts.
Fix this issue by moving the call to `acpi_gpiochip_request_interrupts` to occur after gc->irc.initialized is set.
Cc: Shreeya Patel shreeya.patel@collabora.com Cc: stable@vger.kernel.org Fixes: 5467801f1fcb ("gpio: Restrict usage of GPIO chip irq members before initialization") Reported-by: Mario Limonciello mario.limonciello@amd.com Link: https://lore.kernel.org/linux-gpio/BL1PR12MB51577A77F000A008AA694675E2EF9@BL... Signed-off-by: Mario Limonciello mario.limonciello@amd.com
drivers/gpio/gpiolib.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 085348e08986..b7694171655c 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -1601,8 +1601,6 @@ static int gpiochip_add_irqchip(struct gpio_chip *gc, gpiochip_set_irq_hooks(gc);
- acpi_gpiochip_request_interrupts(gc);
- /*
- Using barrier() here to prevent compiler from reordering
- gc->irq.initialized before initialization of above
@@ -1612,6 +1610,8 @@ static int gpiochip_add_irqchip(struct gpio_chip *gc, gc->irq.initialized = true;
- acpi_gpiochip_request_interrupts(gc);
- return 0;
}
Thanks,
this fixes the issue on an ASUS UM325 laptop.
On 2022-04-14 04:57, Mario Limonciello wrote:
commit 5467801f1fcb ("gpio: Restrict usage of GPIO chip irq members before initialization") attempted to fix a race condition that lead to a NULL pointer, but in the process caused a regression for _AEI/_EVT declared GPIOs. This manifests in messages showing deferred probing while trying to allocate IRQs like so:
[ 0.688318] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x0000 to IRQ, err -517 [ 0.688337] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x002C to IRQ, err -517 [ 0.688348] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x003D to IRQ, err -517 [ 0.688359] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x003E to IRQ, err -517 [ 0.688369] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x003A to IRQ, err -517 [ 0.688379] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x003B to IRQ, err -517 [ 0.688389] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x0002 to IRQ, err -517 [ 0.688399] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x0011 to IRQ, err -517 [ 0.688410] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x0012 to IRQ, err -517 [ 0.688420] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x0007 to IRQ, err -517
The code for walking _AEI doesn't handle deferred probing and so this leads to non-functional GPIO interrupts.
Fix this issue by moving the call to `acpi_gpiochip_request_interrupts` to occur after gc->irc.initialized is set.
Cc: Shreeya Patel shreeya.patel@collabora.com Cc: stable@vger.kernel.org Fixes: 5467801f1fcb ("gpio: Restrict usage of GPIO chip irq members before initialization") Reported-by: Mario Limonciello mario.limonciello@amd.com Link: https://lore.kernel.org/linux-gpio/BL1PR12MB51577A77F000A008AA694675E2EF9@BL... Signed-off-by: Mario Limonciello mario.limonciello@amd.com
Tested-By: Samuel Čavoj samuel@cavoj.net
drivers/gpio/gpiolib.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 085348e08986..b7694171655c 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -1601,8 +1601,6 @@ static int gpiochip_add_irqchip(struct gpio_chip *gc,
gpiochip_set_irq_hooks(gc);
- acpi_gpiochip_request_interrupts(gc);
- /*
- Using barrier() here to prevent compiler from reordering
- gc->irq.initialized before initialization of above
@@ -1612,6 +1610,8 @@ static int gpiochip_add_irqchip(struct gpio_chip *gc,
gc->irq.initialized = true;
- acpi_gpiochip_request_interrupts(gc);
- return 0;
}
Sam
On Thu, Apr 14, 2022 at 4:57 AM Mario Limonciello mario.limonciello@amd.com wrote:
commit 5467801f1fcb ("gpio: Restrict usage of GPIO chip irq members before initialization") attempted to fix a race condition that lead to a NULL pointer, but in the process caused a regression for _AEI/_EVT declared GPIOs. This manifests in messages showing deferred probing while trying to allocate IRQs like so:
[ 0.688318] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x0000 to IRQ, err -517 [ 0.688337] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x002C to IRQ, err -517 [ 0.688348] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x003D to IRQ, err -517 [ 0.688359] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x003E to IRQ, err -517 [ 0.688369] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x003A to IRQ, err -517 [ 0.688379] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x003B to IRQ, err -517 [ 0.688389] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x0002 to IRQ, err -517 [ 0.688399] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x0011 to IRQ, err -517 [ 0.688410] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x0012 to IRQ, err -517 [ 0.688420] amd_gpio AMDI0030:00: Failed to translate GPIO pin 0x0007 to IRQ, err -517
The code for walking _AEI doesn't handle deferred probing and so this leads to non-functional GPIO interrupts.
Fix this issue by moving the call to `acpi_gpiochip_request_interrupts` to occur after gc->irc.initialized is set.
Cc: Shreeya Patel shreeya.patel@collabora.com Cc: stable@vger.kernel.org Fixes: 5467801f1fcb ("gpio: Restrict usage of GPIO chip irq members before initialization") Reported-by: Mario Limonciello mario.limonciello@amd.com Link: https://lore.kernel.org/linux-gpio/BL1PR12MB51577A77F000A008AA694675E2EF9@BL... Signed-off-by: Mario Limonciello mario.limonciello@amd.com
Acked-by: Linus Walleij linus.walleij@linaro.org
Yours, Linus Walleij
linux-stable-mirror@lists.linaro.org
-
Andy Shevchenko -
firew4lker -
Limonciello, Mario -
Linus Walleij -
luke -
Mario Limonciello
-
Samuel Čavoj -
Shreeya Patel -
Thorsten Leemhuis