On Sun, Aug 18, 2019 at 06:26:38PM +0200, Hans de Goede wrote:
> On 18-08-19 18:19, Hans de Goede wrote:
> > On 18-08-19 16:04, Andy Shevchenko wrote:
> > Thanks for the context, so for testing I need a Bay Trail device which
> > uses a GPIO interrupt line with a "Interrupt" descriptor, like this:
> >
> > Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusiv
> > {
> > 0x00000045,
> > }
> >
> > Instead of the more modern GpioInt descriptors. My Asus T100TA is in my
> > storage-bin at the localhackerspace ATM, but I have a T200TA here which
> > is the same wrt touchscreen IRQ routing.
> >
> > I'm currently charging it because its battery was very dead TM, but in
> > the mean time I've been looking at the code and I can already tell
> > that a kernel with the 2e65e0fad935f578e998656d3e7be5a26e072b0e
> > ("pinctrl: intel: baytrail: Pass irqchip when adding gpiochip")
> > is not going to boot, that commit moves the call to
> > byt_gpio_irq_init_hw() to before the call to devm_gpiochip_add_data().
> >
> > devm_gpiochip_add_data() allocs gpio_chip.irq.valid_mask and
> > byt_gpio_irq_init_hw() does:
> >
> > if (value & BYT_DIRECT_IRQ_EN) {
> > clear_bit(i, gc->irq.valid_mask);
> >
> > Which means it will trigger a NULL deref after the
> > "pinctrl: intel: baytrail: Pass irqchip when adding gpiochip"
> > commit, sine now byt_gpio_irq_init_hw() runs before
> > devm_gpiochip_add_data().
> >
> > Note that the gpio_chip structure already has a init_valid_mask
> > callback which runs after gpiochip_irqchip_init_valid_mask wich
> > allocs gpio_chip.irq.valid_mask, so we might use that, but:
> >
> > That is intended to setup the valid_mask for the pins, not for
> > the IRQs, which would mean we are abusing it a bit and it runs
> > after gpiochip_add_irqchip(), which might be too late I guess.
> >
> > So it looks like we need a gpio_chip.irq.init_valid_mask callback
> > to fix this ordering problem and until this is fixed we should revert
> > 2e65e0fad935f578e998656d3e7be5a26e072b0e.
>
> So thinking a bit more about this, I think a better fix would be to
> export gpiochip_irqchip_init_valid_mask and make it proof against
> multiple calls. Then drivers which need this before
> devm_gpiochip_add_data() time can simply call it in advance.
>
> Maybe (not sure if this is a good idea) we can eventually even
> drop gpio_chip.irq.need_valid_mask and simply have all users of
> the valid_mask explictly call gpiochip_irqchip_init_valid_mask.
> If this second bit is a good idea depends on how much users there
> are I guess and what the changes to those users would look like.
Thank you, Hans, very much!
I will simple drop the changes from all drivers which need a valid mask.
Linus, JFYI ^^^, It also makes your cherrivew RFC commented as no go.
--
With Best Regards,
Andy Shevchenko
