Re: [PATCH v2] gpio: Add Tegra186 support

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

On Fri, Mar 31, 2017 at 4:54 PM, Thierry Reding
<thierry.reding@xxxxxxxxx> wrote:
> On Fri, Mar 31, 2017 at 03:59:40PM +0200, Linus Walleij wrote:

> If I call this multiple times with different parent_irq parameters, then
> only the last one will be stored in gpiochip->irq_chained_parent. Later
> on, this is used to disconnect the chained handler and data upon GPIO
> chip removal, which means that handler and data for all but the last one
> end up dangling.

People are using this code already for chained handlers with several
parents.

I guess it works so nicely because gpiochips are often just added
and seldom removed.

Overall that means this is a bug and should be fixed, not that new
drivers should avoid using this approach, or that we should discourage
new drivers to use this API.

>>       /* Set the parent IRQ for all affected IRQs */
>>       for (offset = 0; offset < gpiochip->ngpio; offset++) {
>>               if (!gpiochip_irqchip_irq_valid(gpiochip, offset))
>>                       continue;
>>               irq_set_parent(irq_find_mapping(gpiochip->irqdomain, offset),
>>                              parent_irq);
>>       }
>> }
>
> Similarly here, we'd be setting the parent IRQ of all associated GPIOs
> to the first, second... last parent IRQ. This is completely unnecessary
> work and it's also setting the wrong parent. Note that we don't actually
> care about that in the driver right now, but it's still wrong.

Is that a way of saying there are bugs in the gpiolib?

I know there are, maintainers working on core code are always
lacking. I wrote this code and I admit I make mistakes, please help
me fix them instead of trying to make this into a reason not to
use this code.

>> Why are you keeping the irqs around after requesting?
>> Use devm_*
>
> First I prefer to keep resource request and driver initialization
> separate because it makes error recovery more robust. So this is used to
> first store the results from platform_get_irq() and later on to iterate
> over when installing the chained handlers.

OK no big deal. But it could be in a local variable rather than
in the driver state container, right?

> Also, devm_* doesn't exist for irq_set_chained_handler_and_data(). So I
> need to keep these around in order to properly uninstall the handlers
> again on removal.

OK is this something that should be fixed in the irqchip code?

> Like I explained above, I don't think it works the way it is supposed to
> for this case. The armada-37xx patch that you linked to earlier seems to
> suffer from the same issues in that all but the last parent IRQ handlers
> will be left dangling after removal, which would cause the kernel to run
> non-existing code if by any chance the interrupts were to still fire for
> some reason.

Again, bugs is not a reason not to use library code. We need to fix
those bugs and centralize more, or we get too much strange code
to maintain.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html