On Sat, Apr 01, 2017 at 07:46:24PM +0200, Linus Walleij wrote:
> 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.
Fair enough. I'll see if I can turn this into something that I can use
for this particular driver. I suspect it'll be a bit cumbersome because
of the parent/child relationship, but I have a couple of ideas that I'd
like to try out.
> >> 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?
Well, I also need the same array of interrupts to remove the handlers
again, so I don't think local would be enough. But...
>
> > 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?
... with this in place it wouldn't be necessary. Cross-subsystem series
are unlikely to make it for v4.12 at this point, so here go my hopes to
get this merged sometime soon...
Thierry
Attachment:
signature.asc
Description: PGP signature
