[PATCH] gpio: zynq: restore zynq_gpio_irq_reqres/zynq_gpio_irq_relres callbacks

[PATCH] gpio: zynq: restore zynq_gpio_irq_reqres/zynq_gpio_irq_relres callbacks

[Daniel Mack]

Commit f56914393537 ("gpio: zynq: fix zynqmp_gpio not an immutable chip
warning") ditched the open-coded resource allocation handlers in favor
of the generic ones. These generic handlers don't maintain the PM
runtime anymore, which causes a regression in that level IRQs are no
longer reported.

Restore the original handlers to fix this.

Signed-off-by: Daniel Mack <daniel@zonque.org>
Fixes: f56914393537 ("gpio: zynq: fix zynqmp_gpio not an immutable chip warning")
Cc: stable@kernel.org
---
 drivers/gpio/gpio-zynq.c | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpio-zynq.c b/drivers/gpio/gpio-zynq.c
index 0a7264aabe48..324e942c0650 100644
--- a/drivers/gpio/gpio-zynq.c
+++ b/drivers/gpio/gpio-zynq.c
@@ -575,6 +575,26 @@ static int zynq_gpio_set_wake(struct irq_data *data, unsigned int on)
 	return 0;
 }
 
+static int zynq_gpio_irq_reqres(struct irq_data *d)
+{
+	struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
+	int ret;
+
+	ret = pm_runtime_resume_and_get(chip->parent);
+	if (ret < 0)
+		return ret;
+
+	return gpiochip_reqres_irq(chip, d->hwirq);
+}
+
+static void zynq_gpio_irq_relres(struct irq_data *d)
+{
+	struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
+
+	gpiochip_relres_irq(chip, d->hwirq);
+	pm_runtime_put(chip->parent);
+}
+
 /* irq chip descriptor */
 static const struct irq_chip zynq_gpio_level_irqchip = {
 	.name		= DRIVER_NAME,
@@ -584,9 +604,10 @@ static const struct irq_chip zynq_gpio_level_irqchip = {
 	.irq_unmask	= zynq_gpio_irq_unmask,
 	.irq_set_type	= zynq_gpio_set_irq_type,
 	.irq_set_wake	= zynq_gpio_set_wake,
+	.irq_request_resources = zynq_gpio_irq_reqres,
+	.irq_release_resources = zynq_gpio_irq_relres,
 	.flags		= IRQCHIP_EOI_THREADED | IRQCHIP_EOI_IF_HANDLED |
 			  IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_IMMUTABLE,
-	GPIOCHIP_IRQ_RESOURCE_HELPERS,
 };
 
 static const struct irq_chip zynq_gpio_edge_irqchip = {
@@ -597,8 +618,9 @@ static const struct irq_chip zynq_gpio_edge_irqchip = {
 	.irq_unmask	= zynq_gpio_irq_unmask,
 	.irq_set_type	= zynq_gpio_set_irq_type,
 	.irq_set_wake	= zynq_gpio_set_wake,
+	.irq_request_resources = zynq_gpio_irq_reqres,
+	.irq_release_resources = zynq_gpio_irq_relres,
 	.flags		= IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_IMMUTABLE,
-	GPIOCHIP_IRQ_RESOURCE_HELPERS,
 };
 
 static void zynq_gpio_handle_bank_irq(struct zynq_gpio *gpio,
-- 
2.41.0

Re: [PATCH] gpio: zynq: restore zynq_gpio_irq_reqres/zynq_gpio_irq_relres callbacks

[Thorsten Leemhuis]

CCing Arnd (wrote the culprit), Linus (reviewed it), Bartosz (applied
it), and the regressions mailing list

On 01.09.23 14:24, Daniel Mack wrote:
> Commit f56914393537 ("gpio: zynq: fix zynqmp_gpio not an immutable chip
> warning") ditched the open-coded resource allocation handlers in favor
> of the generic ones. These generic handlers don't maintain the PM
> runtime anymore, which causes a regression in that level IRQs are no
> longer reported.
> 
> Restore the original handlers to fix this.
> 
> Signed-off-by: Daniel Mack <daniel@zonque.org>
> Fixes: f56914393537 ("gpio: zynq: fix zynqmp_gpio not an immutable chip warning")
> Cc: stable@kernel.org

This seems to be a regression report that comes straight with a fix, but
there wasn't a single reply yet afaics. :-/ Maybe the extended list of
recipients will get things moving. But to ensure this doesn't fall
through the cracks, I'll add it to the list of tracked regressions.

#regzbot ^introduced f56914393537
#regzbot title gpio: zynq: in that level IRQs are no longer reported
#regzbot fix: gpio: zynq: restore
zynq_gpio_irq_reqres/zynq_gpio_irq_relres callbacks
#regzbot ignore-activity

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
That page also explains what to do if mails like this annoy you.

> ---
>  drivers/gpio/gpio-zynq.c | 26 ++++++++++++++++++++++++--
>  1 file changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-zynq.c b/drivers/gpio/gpio-zynq.c
> index 0a7264aabe48..324e942c0650 100644
> --- a/drivers/gpio/gpio-zynq.c
> +++ b/drivers/gpio/gpio-zynq.c
> @@ -575,6 +575,26 @@ static int zynq_gpio_set_wake(struct irq_data *data, unsigned int on)
>  	return 0;
>  }
>  
> +static int zynq_gpio_irq_reqres(struct irq_data *d)
> +{
> +	struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
> +	int ret;
> +
> +	ret = pm_runtime_resume_and_get(chip->parent);
> +	if (ret < 0)
> +		return ret;
> +
> +	return gpiochip_reqres_irq(chip, d->hwirq);
> +}
> +
> +static void zynq_gpio_irq_relres(struct irq_data *d)
> +{
> +	struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
> +
> +	gpiochip_relres_irq(chip, d->hwirq);
> +	pm_runtime_put(chip->parent);
> +}
> +
>  /* irq chip descriptor */
>  static const struct irq_chip zynq_gpio_level_irqchip = {
>  	.name		= DRIVER_NAME,
> @@ -584,9 +604,10 @@ static const struct irq_chip zynq_gpio_level_irqchip = {
>  	.irq_unmask	= zynq_gpio_irq_unmask,
>  	.irq_set_type	= zynq_gpio_set_irq_type,
>  	.irq_set_wake	= zynq_gpio_set_wake,
> +	.irq_request_resources = zynq_gpio_irq_reqres,
> +	.irq_release_resources = zynq_gpio_irq_relres,
>  	.flags		= IRQCHIP_EOI_THREADED | IRQCHIP_EOI_IF_HANDLED |
>  			  IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_IMMUTABLE,
> -	GPIOCHIP_IRQ_RESOURCE_HELPERS,
>  };
>  
>  static const struct irq_chip zynq_gpio_edge_irqchip = {
> @@ -597,8 +618,9 @@ static const struct irq_chip zynq_gpio_edge_irqchip = {
>  	.irq_unmask	= zynq_gpio_irq_unmask,
>  	.irq_set_type	= zynq_gpio_set_irq_type,
>  	.irq_set_wake	= zynq_gpio_set_wake,
> +	.irq_request_resources = zynq_gpio_irq_reqres,
> +	.irq_release_resources = zynq_gpio_irq_relres,
>  	.flags		= IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_IMMUTABLE,
> -	GPIOCHIP_IRQ_RESOURCE_HELPERS,
>  };
>  
>  static void zynq_gpio_handle_bank_irq(struct zynq_gpio *gpio,

Re: [PATCH] gpio: zynq: restore zynq_gpio_irq_reqres/zynq_gpio_irq_relres callbacks

[Bartosz Golaszewski]

On Fri, Sep 1, 2023 at 2:34 PM Daniel Mack <daniel@zonque.org> wrote:
>
> Commit f56914393537 ("gpio: zynq: fix zynqmp_gpio not an immutable chip
> warning") ditched the open-coded resource allocation handlers in favor
> of the generic ones. These generic handlers don't maintain the PM
> runtime anymore, which causes a regression in that level IRQs are no
> longer reported.
>
> Restore the original handlers to fix this.
>
> Signed-off-by: Daniel Mack <daniel@zonque.org>
> Fixes: f56914393537 ("gpio: zynq: fix zynqmp_gpio not an immutable chip warning")
> Cc: stable@kernel.org
> ---
>  drivers/gpio/gpio-zynq.c | 26 ++++++++++++++++++++++++--
>  1 file changed, 24 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpio/gpio-zynq.c b/drivers/gpio/gpio-zynq.c
> index 0a7264aabe48..324e942c0650 100644
> --- a/drivers/gpio/gpio-zynq.c
> +++ b/drivers/gpio/gpio-zynq.c
> @@ -575,6 +575,26 @@ static int zynq_gpio_set_wake(struct irq_data *data, unsigned int on)
>         return 0;
>  }
>
> +static int zynq_gpio_irq_reqres(struct irq_data *d)
> +{
> +       struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
> +       int ret;
> +
> +       ret = pm_runtime_resume_and_get(chip->parent);
> +       if (ret < 0)
> +               return ret;
> +
> +       return gpiochip_reqres_irq(chip, d->hwirq);
> +}
> +
> +static void zynq_gpio_irq_relres(struct irq_data *d)
> +{
> +       struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
> +
> +       gpiochip_relres_irq(chip, d->hwirq);
> +       pm_runtime_put(chip->parent);
> +}
> +
>  /* irq chip descriptor */
>  static const struct irq_chip zynq_gpio_level_irqchip = {
>         .name           = DRIVER_NAME,
> @@ -584,9 +604,10 @@ static const struct irq_chip zynq_gpio_level_irqchip = {
>         .irq_unmask     = zynq_gpio_irq_unmask,
>         .irq_set_type   = zynq_gpio_set_irq_type,
>         .irq_set_wake   = zynq_gpio_set_wake,
> +       .irq_request_resources = zynq_gpio_irq_reqres,
> +       .irq_release_resources = zynq_gpio_irq_relres,
>         .flags          = IRQCHIP_EOI_THREADED | IRQCHIP_EOI_IF_HANDLED |
>                           IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_IMMUTABLE,
> -       GPIOCHIP_IRQ_RESOURCE_HELPERS,
>  };
>
>  static const struct irq_chip zynq_gpio_edge_irqchip = {
> @@ -597,8 +618,9 @@ static const struct irq_chip zynq_gpio_edge_irqchip = {
>         .irq_unmask     = zynq_gpio_irq_unmask,
>         .irq_set_type   = zynq_gpio_set_irq_type,
>         .irq_set_wake   = zynq_gpio_set_wake,
> +       .irq_request_resources = zynq_gpio_irq_reqres,
> +       .irq_release_resources = zynq_gpio_irq_relres,
>         .flags          = IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_IMMUTABLE,
> -       GPIOCHIP_IRQ_RESOURCE_HELPERS,
>  };
>
>  static void zynq_gpio_handle_bank_irq(struct zynq_gpio *gpio,
> --
> 2.41.0
>

Thanks Thorsten for bringing it to my attention.

Daniel: please use scripts/get_maintainers.pl when sending patches.
This was sent neither to my email address from the MAINTAINERS file
nor to my linaro address.

Other than that, this looks good, applied for fixes.

Bartosz

Re: [PATCH] gpio: zynq: restore zynq_gpio_irq_reqres/zynq_gpio_irq_relres callbacks

[Arnd Bergmann]

On Wed, Sep 6, 2023, at 09:49, Thorsten Leemhuis wrote:
> CCing Arnd (wrote the culprit), Linus (reviewed it), Bartosz (applied
> it), and the regressions mailing list

Thanks

> On 01.09.23 14:24, Daniel Mack wrote:
>> Commit f56914393537 ("gpio: zynq: fix zynqmp_gpio not an immutable chip
>> warning") ditched the open-coded resource allocation handlers in favor
>> of the generic ones. These generic handlers don't maintain the PM
>> runtime anymore, which causes a regression in that level IRQs are no
>> longer reported.
>> 
>> Restore the original handlers to fix this.
>> 
>> Signed-off-by: Daniel Mack <daniel@zonque.org>
>> Fixes: f56914393537 ("gpio: zynq: fix zynqmp_gpio not an immutable chip warning")
>> Cc: stable@kernel.org
>
> This seems to be a regression report that comes straight with a fix, but
> there wasn't a single reply yet afaics. :-/ Maybe the extended list of
> recipients will get things moving. But to ensure this doesn't fall
> through the cracks, I'll add it to the list of tracked regressions.

I don't understand what the GPIOCHIP_IRQ_RESOURCE_HELPERS change
intended to do in the first place: Manikanta's patch changed the behavior
here with the addition of GPIOCHIP_IRQ_RESOURCE_HELPERS, while my patch
was a cleanup that removed the dead code.

Daniel's fix reverts both my cleanup patch and part of the original
change, which may or may not be what we want here.

      Arnd

Re: [PATCH] gpio: zynq: restore zynq_gpio_irq_reqres/zynq_gpio_irq_relres callbacks

[Daniel Mack]

On 9/6/23 22:12, Arnd Bergmann wrote:
>> On 01.09.23 14:24, Daniel Mack wrote:
>>> Commit f56914393537 ("gpio: zynq: fix zynqmp_gpio not an immutable chip
>>> warning") ditched the open-coded resource allocation handlers in favor
>>> of the generic ones. These generic handlers don't maintain the PM
>>> runtime anymore, which causes a regression in that level IRQs are no
>>> longer reported.
>>>
>>> Restore the original handlers to fix this.
>>>
>>> Signed-off-by: Daniel Mack <daniel@zonque.org>
>>> Fixes: f56914393537 ("gpio: zynq: fix zynqmp_gpio not an immutable chip warning")
>>> Cc: stable@kernel.org
>>
>> This seems to be a regression report that comes straight with a fix, but
>> there wasn't a single reply yet afaics. :-/ Maybe the extended list of
>> recipients will get things moving. But to ensure this doesn't fall
>> through the cracks, I'll add it to the list of tracked regressions.
> 
> I don't understand what the GPIOCHIP_IRQ_RESOURCE_HELPERS change
> intended to do in the first place: Manikanta's patch changed the behavior
> here with the addition of GPIOCHIP_IRQ_RESOURCE_HELPERS, while my patch
> was a cleanup that removed the dead code.

Manikanta's patch (f56914393537 "gpio: zynq: fix zynqmp_gpio not an
immutable chip warning") did many things at once, the move to the
generic resource handlers was just one of the changes. I can only guess
that it intended to simply the code a bit, and the dropped pm runtime
detail was just an oversight.

> Daniel's fix reverts both my cleanup patch and part of the original
> change, which may or may not be what we want here.

Enabling the pm runtime is necessary on my boards, at least for level
IRQs. Edge IRQs are still handled fine, interestingly.


Thanks,
Daniel