Re: pinctrl: mcp23s08: detected irqchip that is shared with multiple gpiochips - real of false?

[Marco Felsch <m.felsch@xxxxxxxxxxxxxx>]

Hi Linus,

On 19-06-09 12:58, Linus Walleij wrote:
> Added Marco and a few other MCP23s08 people who may be more familiar
> with this code. (The driver is a bit complex.)
> 
> On Thu, Jun 6, 2019 at 10:59 AM Phil Reid <preid@xxxxxxxxxxxxxxxxx> wrote:
> 
> > Using kernel 5.1 I'm getting the following message:
> > "detected irqchip that is shared with multiple gpiochips: please fix the driver"
> >
> > Which I believe should be fixed by:
> > 19ab5ca "pinctrl: mcp23s08: Allocate irq_chip dynamic"
> >
> > However mcp23s08_probe_one() ends up calling gpiochip_set_irq_hooks() twice.
> 
> It looks strange when I look at the mcp23s08_probe_one() function
> because it goes like:
> 
> if (mcp->irq && mcp->irq_controller) {
>     ret = mcp23s08_irqchip_setup(mcp);
>     if (ret)
>         goto fail;
> }
> (...)
> if (mcp->irq)
>     ret = mcp23s08_irq_setup(mcp);
> 
> That seems wrong... but overall the code in this probe_one is pretty hard to
> follow and probably needs some refactoring.

I don't think that this is wrong since I splitted only the irqchip setup
and the hw irq line setup.

> This comes from f259f896f234 ("pinctrl: mcp23s08: fix irq and irqchip
> setup order")
> by Marco, Marco can you look into this and help us figure out why this happens?

I greped the code and found the possible failure:


static int mcp23s08_probe_one()
{
	...

	if (mcp->irq && mcp->irq_controller) {
		ret = mcp23s08_irqchip_setup(mcp);
		if (ret)
			goto fail;
	}

	ret = devm_gpiochip_add_data(dev, &mcp->chip, mcp);
	if (ret < 0)
		goto fail;

	...
}

The mcp23s08_irqchip_setup() call hierarchy:
mcp23s08_irqchip_setup()
  gpiochip_irqchip_add_nested()
    gpiochip_irqchip_add_key()
      gpiochip_set_irq_hooks()

The devm_gpiochip_add_data() call hierarchy:

devm_gpiochip_add_data()
  gpiochip_add_data_with_key()
    gpiochip_add_irqchip()
      gpiochip_set_irq_hooks()

The gpiochip_add_irqchip() returns immediately if there isn't a irqchip
but we added a irqchip due to the previous mcp23s08_irqchip_setup()
call. So it calls gpiochip_set_irq_hooks() a second time. If I got this
right the proper fix is:

static int mcp23s08_probe_one()
{
	...

	ret = devm_gpiochip_add_data(dev, &mcp->chip, mcp);
	if (ret < 0)
		goto fail;

	if (mcp->irq && mcp->irq_controller) {
		ret = mcp23s08_irqchip_setup(mcp);
		if (ret)
			goto fail;
	}

	...
}

I checked other users of gpiochip_irqchip_add_nested() and they call
(devm_)gpiochip_add_data always infront of the
gpiochip_irqchip_add_nested() call.

I hope this helps you.

Regards,
  Marco

> 
> Yours,
> Linus Walleij
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |