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@gmail.com> 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