Re: [PATCH] gpio: thunderx: fix irq_request_resources

Re: [PATCH] gpio: thunderx: fix irq_request_resources

[Linus Walleij]

On Wed, Mar 11, 2020 at 4:43 PM Tim Harvey <tharvey@gateworks.com> wrote:

> If there are no parent resources do not call irq_chip_request_resources_parent
> at all as this will return an error.
>
> This resolves a regression where devices using a thunderx gpio as an interrupt
> would fail probing.
>
> Fixes: 0d04d0c ("gpio: thunderx: Use the default parent apis for {request,release}_resources")
> Signed-off-by: Tim Harvey <tharvey@gateworks.com>

This patch does not apply to the mainline kernel or v5.6-rc1.

Please verify:
1. If the problem is still in v5.6 (we refactored the driver to
   use GPIOLIB_IRQCHIP)

2. If not, only propose it for linux-stable v5.5 etc.

Yours,
Linus Walleij

Re: [PATCH] gpio: thunderx: fix irq_request_resources

[Tim Harvey]

On Thu, Mar 12, 2020 at 6:42 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Wed, Mar 11, 2020 at 4:43 PM Tim Harvey <tharvey@gateworks.com> wrote:
>
> > If there are no parent resources do not call irq_chip_request_resources_parent
> > at all as this will return an error.
> >
> > This resolves a regression where devices using a thunderx gpio as an interrupt
> > would fail probing.
> >
> > Fixes: 0d04d0c ("gpio: thunderx: Use the default parent apis for {request,release}_resources")
> > Signed-off-by: Tim Harvey <tharvey@gateworks.com>
>
> This patch does not apply to the mainline kernel or v5.6-rc1.
>
> Please verify:
> 1. If the problem is still in v5.6 (we refactored the driver to
>    use GPIOLIB_IRQCHIP)

Linus,

Sorry, another issue was keeping me from being able to boot 5.6-rc but
that's now understood and I can confirm the issue is not present in
v5.6-rc5

>
> 2. If not, only propose it for linux-stable v5.5 etc.
>

Yes, needs to be applied to v5.2, v5.3, v5.4, v5.5. I cc'd stable. If
I need to re-submit please let me know.

Cc: stable@vger.kernel.org

Tim

Re: [PATCH] gpio: thunderx: fix irq_request_resources

[Sasha Levin]

On Thu, Mar 12, 2020 at 10:16:40AM -0700, Tim Harvey wrote:
>On Thu, Mar 12, 2020 at 6:42 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>>
>> On Wed, Mar 11, 2020 at 4:43 PM Tim Harvey <tharvey@gateworks.com> wrote:
>>
>> > If there are no parent resources do not call irq_chip_request_resources_parent
>> > at all as this will return an error.
>> >
>> > This resolves a regression where devices using a thunderx gpio as an interrupt
>> > would fail probing.
>> >
>> > Fixes: 0d04d0c ("gpio: thunderx: Use the default parent apis for {request,release}_resources")
>> > Signed-off-by: Tim Harvey <tharvey@gateworks.com>
>>
>> This patch does not apply to the mainline kernel or v5.6-rc1.
>>
>> Please verify:
>> 1. If the problem is still in v5.6 (we refactored the driver to
>>    use GPIOLIB_IRQCHIP)
>
>Linus,
>
>Sorry, another issue was keeping me from being able to boot 5.6-rc but
>that's now understood and I can confirm the issue is not present in
>v5.6-rc5
>
>>
>> 2. If not, only propose it for linux-stable v5.5 etc.
>>
>
>Yes, needs to be applied to v5.2, v5.3, v5.4, v5.5. I cc'd stable. If
>I need to re-submit please let me know.
>
>Cc: stable@vger.kernel.org

Linus, could you ack this patch for stable?

-- 
Thanks,
Sasha

Re: [PATCH] gpio: thunderx: fix irq_request_resources

[Linus Walleij]

On Fri, Mar 13, 2020 at 4:01 PM Sasha Levin <sashal@kernel.org> wrote:
> On Thu, Mar 12, 2020 at 10:16:40AM -0700, Tim Harvey wrote:
> >On Thu, Mar 12, 2020 at 6:42 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> >>
> >> On Wed, Mar 11, 2020 at 4:43 PM Tim Harvey <tharvey@gateworks.com> wrote:
> >>
> >> > If there are no parent resources do not call irq_chip_request_resources_parent
> >> > at all as this will return an error.
> >> >
> >> > This resolves a regression where devices using a thunderx gpio as an interrupt
> >> > would fail probing.
> >> >
> >> > Fixes: 0d04d0c ("gpio: thunderx: Use the default parent apis for {request,release}_resources")
> >> > Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> >>
> >> This patch does not apply to the mainline kernel or v5.6-rc1.
> >>
> >> Please verify:
> >> 1. If the problem is still in v5.6 (we refactored the driver to
> >>    use GPIOLIB_IRQCHIP)
> >
> >Linus,
> >
> >Sorry, another issue was keeping me from being able to boot 5.6-rc but
> >that's now understood and I can confirm the issue is not present in
> >v5.6-rc5
> >
> >>
> >> 2. If not, only propose it for linux-stable v5.5 etc.
> >>
> >
> >Yes, needs to be applied to v5.2, v5.3, v5.4, v5.5. I cc'd stable. If
> >I need to re-submit please let me know.
> >
> >Cc: stable@vger.kernel.org
>
> Linus, could you ack this patch for stable?

Acked-by: Linus Walleij <linus.walleij@linaro.org>

Sorry for the delay :/

Yours,
Linus Walleij

Re: [PATCH] gpio: thunderx: fix irq_request_resources

[Robert Richter]

On 11.03.20 08:43:53, Tim Harvey wrote:
> If there are no parent resources do not call irq_chip_request_resources_parent
> at all as this will return an error.
> 
> This resolves a regression where devices using a thunderx gpio as an interrupt
> would fail probing.
> 
> Fixes: 0d04d0c ("gpio: thunderx: Use the default parent apis for {request,release}_resources")
> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> ---
>  drivers/gpio/gpio-thunderx.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-thunderx.c b/drivers/gpio/gpio-thunderx.c
> index 4627704..f84b9b1 100644
> --- a/drivers/gpio/gpio-thunderx.c
> +++ b/drivers/gpio/gpio-thunderx.c
> @@ -366,15 +366,18 @@ static int thunderx_gpio_irq_request_resources(struct irq_data *data)
>  {
>  	struct thunderx_line *txline = irq_data_get_irq_chip_data(data);
>  	struct thunderx_gpio *txgpio = txline->txgpio;
> +	struct irq_data *parent_data = data->parent_data;
>  	int r;
>  
>  	r = gpiochip_lock_as_irq(&txgpio->chip, txline->line);
>  	if (r)
>  		return r;
>  
> -	r = irq_chip_request_resources_parent(data);
> -	if (r)
> -		gpiochip_unlock_as_irq(&txgpio->chip, txline->line);
> +	if (parent_data && parent_data->chip->irq_request_resources) {
> +		r = irq_chip_request_resources_parent(data);
> +		if (r)
> +			gpiochip_unlock_as_irq(&txgpio->chip, txline->line);
> +	}

There is no unlocking for the else case. I would assume the
parent_data should be checked before grabbing the lock, or is this
intended?

-Robert

>  
>  	return r;
>  }
> -- 
> 2.7.4
> 

Re: [PATCH] gpio: thunderx: fix irq_request_resources

[Robert Richter]

On 13.03.20 16:31:51, Robert Richter wrote:
> On 11.03.20 08:43:53, Tim Harvey wrote:
> > If there are no parent resources do not call irq_chip_request_resources_parent
> > at all as this will return an error.
> > 
> > This resolves a regression where devices using a thunderx gpio as an interrupt
> > would fail probing.
> > 
> > Fixes: 0d04d0c ("gpio: thunderx: Use the default parent apis for {request,release}_resources")
> > Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> > ---
> >  drivers/gpio/gpio-thunderx.c | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpio/gpio-thunderx.c b/drivers/gpio/gpio-thunderx.c
> > index 4627704..f84b9b1 100644
> > --- a/drivers/gpio/gpio-thunderx.c
> > +++ b/drivers/gpio/gpio-thunderx.c
> > @@ -366,15 +366,18 @@ static int thunderx_gpio_irq_request_resources(struct irq_data *data)
> >  {
> >  	struct thunderx_line *txline = irq_data_get_irq_chip_data(data);
> >  	struct thunderx_gpio *txgpio = txline->txgpio;
> > +	struct irq_data *parent_data = data->parent_data;
> >  	int r;
> >  
> >  	r = gpiochip_lock_as_irq(&txgpio->chip, txline->line);
> >  	if (r)
> >  		return r;
> >  
> > -	r = irq_chip_request_resources_parent(data);
> > -	if (r)
> > -		gpiochip_unlock_as_irq(&txgpio->chip, txline->line);
> > +	if (parent_data && parent_data->chip->irq_request_resources) {
> > +		r = irq_chip_request_resources_parent(data);
> > +		if (r)
> > +			gpiochip_unlock_as_irq(&txgpio->chip, txline->line);
> > +	}
> 
> There is no unlocking for the else case. I would assume the
> parent_data should be checked before grabbing the lock, or is this
> intended?

Looking at the original code, the parent resources are requested only
if existing. So the change is ok.

On the other hand, the overall change using irq_chip_{request,
release}_resources_parent() became pointless now. It is unreadable and
more complex now. Thus, commit 0d04d0c should just be reverted.

The function interface is limited. Instead of letting the child device
deal with the parent, parent requests should be handled directly in
irq_request_resources(). Another aspect is that the code for this
driver has been already removed upstream and ti_sci_inta_msi.c is the
last remaining user of it. This speaks also for a removal by a revert.

Thanks,

-Robert

> >  
> >  	return r;
> >  }
> > -- 
> > 2.7.4
> > 

Re: [PATCH] gpio: thunderx: fix irq_request_resources

[Tim Harvey]

On Fri, Mar 13, 2020 at 12:12 PM Robert Richter <rrichter@marvell.com> wrote:
>
> On 13.03.20 16:31:51, Robert Richter wrote:
> > On 11.03.20 08:43:53, Tim Harvey wrote:
> > > If there are no parent resources do not call irq_chip_request_resources_parent
> > > at all as this will return an error.
> > >
> > > This resolves a regression where devices using a thunderx gpio as an interrupt
> > > would fail probing.
> > >
> > > Fixes: 0d04d0c ("gpio: thunderx: Use the default parent apis for {request,release}_resources")
> > > Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> > > ---
> > >  drivers/gpio/gpio-thunderx.c | 9 ++++++---
> > >  1 file changed, 6 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/gpio/gpio-thunderx.c b/drivers/gpio/gpio-thunderx.c
> > > index 4627704..f84b9b1 100644
> > > --- a/drivers/gpio/gpio-thunderx.c
> > > +++ b/drivers/gpio/gpio-thunderx.c
> > > @@ -366,15 +366,18 @@ static int thunderx_gpio_irq_request_resources(struct irq_data *data)
> > >  {
> > >     struct thunderx_line *txline = irq_data_get_irq_chip_data(data);
> > >     struct thunderx_gpio *txgpio = txline->txgpio;
> > > +   struct irq_data *parent_data = data->parent_data;
> > >     int r;
> > >
> > >     r = gpiochip_lock_as_irq(&txgpio->chip, txline->line);
> > >     if (r)
> > >             return r;
> > >
> > > -   r = irq_chip_request_resources_parent(data);
> > > -   if (r)
> > > -           gpiochip_unlock_as_irq(&txgpio->chip, txline->line);
> > > +   if (parent_data && parent_data->chip->irq_request_resources) {
> > > +           r = irq_chip_request_resources_parent(data);
> > > +           if (r)
> > > +                   gpiochip_unlock_as_irq(&txgpio->chip, txline->line);
> > > +   }
> >
> > There is no unlocking for the else case. I would assume the
> > parent_data should be checked before grabbing the lock, or is this
> > intended?
>
> Looking at the original code, the parent resources are requested only
> if existing. So the change is ok.
>
> On the other hand, the overall change using irq_chip_{request,
> release}_resources_parent() became pointless now. It is unreadable and
> more complex now. Thus, commit 0d04d0c should just be reverted.
>
> The function interface is limited. Instead of letting the child device
> deal with the parent, parent requests should be handled directly in
> irq_request_resources(). Another aspect is that the code for this
> driver has been already removed upstream and ti_sci_inta_msi.c is the
> last remaining user of it. This speaks also for a removal by a revert.
>

Robert,

A revert does make the most sense to me and it works for 5.2, 5.3, and
5.5 but the revert fails for 5.4 and needs some manual intervention.

I'm not sure how to proceed with this.

Tim

Re: [PATCH] gpio: thunderx: fix irq_request_resources

[Robert Richter]

On 13.03.20 12:41:19, Tim Harvey wrote:
> On Fri, Mar 13, 2020 at 12:12 PM Robert Richter <rrichter@marvell.com> wrote:
> >
> > On 13.03.20 16:31:51, Robert Richter wrote:
> > > On 11.03.20 08:43:53, Tim Harvey wrote:
> > > > If there are no parent resources do not call irq_chip_request_resources_parent
> > > > at all as this will return an error.
> > > >
> > > > This resolves a regression where devices using a thunderx gpio as an interrupt
> > > > would fail probing.
> > > >
> > > > Fixes: 0d04d0c ("gpio: thunderx: Use the default parent apis for {request,release}_resources")
> > > > Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> > > > ---
> > > >  drivers/gpio/gpio-thunderx.c | 9 ++++++---
> > > >  1 file changed, 6 insertions(+), 3 deletions(-)

> > Looking at the original code, the parent resources are requested only
> > if existing. So the change is ok.
> >
> > On the other hand, the overall change using irq_chip_{request,
> > release}_resources_parent() became pointless now. It is unreadable and
> > more complex now. Thus, commit 0d04d0c should just be reverted.
> >
> > The function interface is limited. Instead of letting the child device
> > deal with the parent, parent requests should be handled directly in
> > irq_request_resources(). Another aspect is that the code for this
> > driver has been already removed upstream and ti_sci_inta_msi.c is the
> > last remaining user of it. This speaks also for a removal by a revert.

> A revert does make the most sense to me and it works for 5.2, 5.3, and
> 5.5 but the revert fails for 5.4 and needs some manual intervention.

v5.4 should additionally revert a7fc89f9d5fc ("gpio: thunderx: Switch
to GPIOLIB_IRQCHIP"). v5.5 contains this revert too (a564ac35d605
Revert "gpio: thunderx: Switch to GPIOLIB_IRQCHIP") and the code in
that area is the same then for all kernels from 5.2 to 5.5, which is
basically a revert back to 5.1. I think this is ok.

Do you have a particular test case to test the driver that I can use
for my own testing?

Thanks,

-Robert

Re: [PATCH] gpio: thunderx: fix irq_request_resources

[Tim Harvey]

On Fri, Mar 13, 2020 at 1:24 PM Robert Richter <rrichter@marvell.com> wrote:
>
> On 13.03.20 12:41:19, Tim Harvey wrote:
> > On Fri, Mar 13, 2020 at 12:12 PM Robert Richter <rrichter@marvell.com> wrote:
> > >
> > > On 13.03.20 16:31:51, Robert Richter wrote:
> > > > On 11.03.20 08:43:53, Tim Harvey wrote:
> > > > > If there are no parent resources do not call irq_chip_request_resources_parent
> > > > > at all as this will return an error.
> > > > >
> > > > > This resolves a regression where devices using a thunderx gpio as an interrupt
> > > > > would fail probing.
> > > > >
> > > > > Fixes: 0d04d0c ("gpio: thunderx: Use the default parent apis for {request,release}_resources")
> > > > > Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> > > > > ---
> > > > >  drivers/gpio/gpio-thunderx.c | 9 ++++++---
> > > > >  1 file changed, 6 insertions(+), 3 deletions(-)
>
> > > Looking at the original code, the parent resources are requested only
> > > if existing. So the change is ok.
> > >
> > > On the other hand, the overall change using irq_chip_{request,
> > > release}_resources_parent() became pointless now. It is unreadable and
> > > more complex now. Thus, commit 0d04d0c should just be reverted.
> > >
> > > The function interface is limited. Instead of letting the child device
> > > deal with the parent, parent requests should be handled directly in
> > > irq_request_resources(). Another aspect is that the code for this
> > > driver has been already removed upstream and ti_sci_inta_msi.c is the
> > > last remaining user of it. This speaks also for a removal by a revert.
>
> > A revert does make the most sense to me and it works for 5.2, 5.3, and
> > 5.5 but the revert fails for 5.4 and needs some manual intervention.
>
> v5.4 should additionally revert a7fc89f9d5fc ("gpio: thunderx: Switch
> to GPIOLIB_IRQCHIP"). v5.5 contains this revert too (a564ac35d605
> Revert "gpio: thunderx: Switch to GPIOLIB_IRQCHIP") and the code in
> that area is the same then for all kernels from 5.2 to 5.5, which is
> basically a revert back to 5.1. I think this is ok.
>
> Do you have a particular test case to test the driver that I can use
> for my own testing?
>

Robert,

The hardware I have has an interrupt controller with its upstream
interrupt to an OctoenTX GPIO (and its driver is in progress and not
yet accepted upstream which means this issue in 5.2/5.3/5.4 doesn't
affect me). I'm unclear if you just need a device that has an
interrupt on the OcteonTX GPIO or if the device has to be an interrupt
controller to trigger the issue as is my case.

Tim