| Message ID | 1583941433-15876-1-git-send-email-tharvey@gateworks.com |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: thunderx: fix irq_request_resources | expand |
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
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
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?
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 >
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 > >
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
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
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
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
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; }
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(-)