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 <[email protected]>
---
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);
+ }
return r;
}
--
2.7.4
On Wed, Mar 11, 2020 at 4:43 PM Tim Harvey <[email protected]> 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 <[email protected]>
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
On Thu, Mar 12, 2020 at 6:42 AM Linus Walleij <[email protected]> wrote:
>
> On Wed, Mar 11, 2020 at 4:43 PM Tim Harvey <[email protected]> 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 <[email protected]>
>
> 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: [email protected]
Tim
On Thu, Mar 12, 2020 at 10:16:40AM -0700, Tim Harvey wrote:
>On Thu, Mar 12, 2020 at 6:42 AM Linus Walleij <[email protected]> wrote:
>>
>> On Wed, Mar 11, 2020 at 4:43 PM Tim Harvey <[email protected]> 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 <[email protected]>
>>
>> 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: [email protected]
Linus, could you ack this patch for stable?
--
Thanks,
Sasha
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 <[email protected]>
> ---
> 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
>
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 <[email protected]>
> > ---
> > 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
> >
On Fri, Mar 13, 2020 at 12:12 PM Robert Richter <[email protected]> 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 <[email protected]>
> > > ---
> > > 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
On 13.03.20 12:41:19, Tim Harvey wrote:
> On Fri, Mar 13, 2020 at 12:12 PM Robert Richter <[email protected]> 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 <[email protected]>
> > > > ---
> > > > 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
On Fri, Mar 13, 2020 at 1:24 PM Robert Richter <[email protected]> wrote:
>
> On 13.03.20 12:41:19, Tim Harvey wrote:
> > On Fri, Mar 13, 2020 at 12:12 PM Robert Richter <[email protected]> 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 <[email protected]>
> > > > > ---
> > > > > 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
On Fri, Mar 13, 2020 at 4:01 PM Sasha Levin <[email protected]> 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 <[email protected]> wrote:
> >>
> >> On Wed, Mar 11, 2020 at 4:43 PM Tim Harvey <[email protected]> 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 <[email protected]>
> >>
> >> 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: [email protected]
>
> Linus, could you ack this patch for stable?
Acked-by: Linus Walleij <[email protected]>
Sorry for the delay :/
Yours,
Linus Walleij