| Message ID | 20190430101230.21794-8-lokeshvutla@ti.com |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
On Tue, Apr 30, 2019 at 3:14 AM Lokesh Vutla <lokeshvutla@ti.com> wrote: > > thunderx_gpio_irq_{request,release}_resources apis are trying to > {request,release} resources on parent interrupt. There are default > apis doing the same. Use the default parent apis instead of writing > the same code snippet. > > Cc: linux-gpio@vger.kernel.org > Cc: Linus Walleij <linus.walleij@linaro.org> > Acked-by: Linus Walleij <linus.walleij@linaro.org> > Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com> > --- > Changes since v7: > - None > > drivers/gpio/gpio-thunderx.c | 16 ++++------------ > 1 file changed, 4 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpio/gpio-thunderx.c b/drivers/gpio/gpio-thunderx.c > index 1306722faa5a..715371b5102a 100644 > --- a/drivers/gpio/gpio-thunderx.c > +++ b/drivers/gpio/gpio-thunderx.c > @@ -363,22 +363,16 @@ 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; > > - if (parent_data && parent_data->chip->irq_request_resources) { > - r = parent_data->chip->irq_request_resources(parent_data); > - if (r) > - goto error; > - } > + r = irq_chip_request_resources_parent(data); > + if (r) > + gpiochip_unlock_as_irq(&txgpio->chip, txline->line); Lokesh, This patch breaks irq resources for thunderx-gpio as parent_data->chip->irq_request_resources is undefined thus your new irq_chip_request_resources_parent() returns -ENOSYS causing this function to return an error where as before it would happily return 0. Is the following the correct fix or should we qualify data->parent_data->chip->irq_request_resources before calling irq_chip_request_resources_parent() in thunderx-gpio? diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c index b3fa2d8..b2435ecb 100644 --- a/kernel/irq/chip.c +++ b/kernel/irq/chip.c @@ -1525,7 +1525,7 @@ int irq_chip_request_resources_parent(struct irq_data *data) if (data->chip->irq_request_resources) return data->chip->irq_request_resources(data); - return -ENOSYS; + return 0; } EXPORT_SYMBOL_GPL(irq_chip_request_resources_parent); Regards, Tim
Hi Tim, On 11/03/20 4:57 AM, Tim Harvey wrote: > On Tue, Apr 30, 2019 at 3:14 AM Lokesh Vutla <lokeshvutla@ti.com> wrote: >> >> thunderx_gpio_irq_{request,release}_resources apis are trying to >> {request,release} resources on parent interrupt. There are default >> apis doing the same. Use the default parent apis instead of writing >> the same code snippet. >> >> Cc: linux-gpio@vger.kernel.org >> Cc: Linus Walleij <linus.walleij@linaro.org> >> Acked-by: Linus Walleij <linus.walleij@linaro.org> >> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com> >> --- >> Changes since v7: >> - None >> >> drivers/gpio/gpio-thunderx.c | 16 ++++------------ >> 1 file changed, 4 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/gpio/gpio-thunderx.c b/drivers/gpio/gpio-thunderx.c >> index 1306722faa5a..715371b5102a 100644 >> --- a/drivers/gpio/gpio-thunderx.c >> +++ b/drivers/gpio/gpio-thunderx.c >> @@ -363,22 +363,16 @@ 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; >> >> - if (parent_data && parent_data->chip->irq_request_resources) { >> - r = parent_data->chip->irq_request_resources(parent_data); >> - if (r) >> - goto error; >> - } >> + r = irq_chip_request_resources_parent(data); >> + if (r) >> + gpiochip_unlock_as_irq(&txgpio->chip, txline->line); > > Lokesh, > > This patch breaks irq resources for thunderx-gpio as > parent_data->chip->irq_request_resources is undefined thus your new > irq_chip_request_resources_parent() returns -ENOSYS causing this > function to return an error where as before it would happily return 0. Returning -ENOSYS is the right behaviour. If the parent doesn't have the resources, child driver should not hook it at all. > > Is the following the correct fix or should we qualify > data->parent_data->chip->irq_request_resources before calling > irq_chip_request_resources_parent() in thunderx-gpio? If there are no parent resources why should thunderx-gpio request parent resources at all? Thanks and regards, Lokesh
Tim, Tim Harvey <tharvey@gateworks.com> writes: > On Tue, Apr 30, 2019 at 3:14 AM Lokesh Vutla <lokeshvutla@ti.com> wrote: >> - if (parent_data && parent_data->chip->irq_request_resources) { >> - r = parent_data->chip->irq_request_resources(parent_data); >> - if (r) >> - goto error; >> - } >> + r = irq_chip_request_resources_parent(data); >> + if (r) >> + gpiochip_unlock_as_irq(&txgpio->chip, txline->line); > > This patch breaks irq resources for thunderx-gpio as > parent_data->chip->irq_request_resources is undefined thus your new > irq_chip_request_resources_parent() returns -ENOSYS causing this > function to return an error where as before it would happily return 0. > > Is the following the correct fix or should we qualify > data->parent_data->chip->irq_request_resources before calling > irq_chip_request_resources_parent() in thunderx-gpio? You are not supposed to fiddle with parent data at all. Just because C allows you is no excuse to violate abstractions in the first place. irq_chip_request_resources_parent() rightfully returns -ENOSYS when it can't request a resource from the parent chip because that chip does not have anything to offer. It's up to the caller to do something sensible with the return code. If your chip is happy with the parent not providing it then handle -ENOSYS. None of the chip callbacks should return -ENOSYS. If one does then that wants to be fixed. Thanks, tglx
On Wed, Mar 11, 2020 at 8:43 AM Thomas Gleixner <tglx@linutronix.de> wrote: > > Tim, > > Tim Harvey <tharvey@gateworks.com> writes: > > On Tue, Apr 30, 2019 at 3:14 AM Lokesh Vutla <lokeshvutla@ti.com> wrote: > >> - if (parent_data && parent_data->chip->irq_request_resources) { > >> - r = parent_data->chip->irq_request_resources(parent_data); > >> - if (r) > >> - goto error; > >> - } > >> + r = irq_chip_request_resources_parent(data); > >> + if (r) > >> + gpiochip_unlock_as_irq(&txgpio->chip, txline->line); > > > > This patch breaks irq resources for thunderx-gpio as > > parent_data->chip->irq_request_resources is undefined thus your new > > irq_chip_request_resources_parent() returns -ENOSYS causing this > > function to return an error where as before it would happily return 0. > > > > Is the following the correct fix or should we qualify > > data->parent_data->chip->irq_request_resources before calling > > irq_chip_request_resources_parent() in thunderx-gpio? > > You are not supposed to fiddle with parent data at all. Just because C > allows you is no excuse to violate abstractions in the first place. > > irq_chip_request_resources_parent() rightfully returns -ENOSYS when it > can't request a resource from the parent chip because that chip does not > have anything to offer. > > It's up to the caller to do something sensible with the return code. If > your chip is happy with the parent not providing it then handle > -ENOSYS. None of the chip callbacks should return -ENOSYS. If one does > then that wants to be fixed. > Ok, makes sense. Thank you and Lokesh for the feedback. I just submitted a patch to fix the thunderx-gpio breakage. Best Regards, Tim
diff --git a/drivers/gpio/gpio-thunderx.c b/drivers/gpio/gpio-thunderx.c index 1306722faa5a..715371b5102a 100644 --- a/drivers/gpio/gpio-thunderx.c +++ b/drivers/gpio/gpio-thunderx.c @@ -363,22 +363,16 @@ 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; - if (parent_data && parent_data->chip->irq_request_resources) { - r = parent_data->chip->irq_request_resources(parent_data); - if (r) - goto error; - } + r = irq_chip_request_resources_parent(data); + if (r) + gpiochip_unlock_as_irq(&txgpio->chip, txline->line); - return 0; -error: - gpiochip_unlock_as_irq(&txgpio->chip, txline->line); return r; } @@ -386,10 +380,8 @@ static void thunderx_gpio_irq_release_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; - if (parent_data && parent_data->chip->irq_release_resources) - parent_data->chip->irq_release_resources(parent_data); + irq_chip_release_resources_parent(data); gpiochip_unlock_as_irq(&txgpio->chip, txline->line); }
thunderx_gpio_irq_{request,release}_resources apis are trying to {request,release} resources on parent interrupt. There are default apis doing the same. Use the default parent apis instead of writing the same code snippet. Cc: linux-gpio@vger.kernel.org Cc: Linus Walleij <linus.walleij@linaro.org> Acked-by: Linus Walleij <linus.walleij@linaro.org> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com> --- Changes since v7: - None drivers/gpio/gpio-thunderx.c | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-)