| Message ID | 20180925145616.12610-3-m.felsch@pengutronix.de |
|---|---|
| State | New |
| Headers | show |
| Series | Fixes for mcp23s08 | expand |
G'day Marco, On 25/09/2018 10:56 PM, Marco Felsch wrote: > Since 'commit 02e389e63e35 ("pinctrl: mcp23s08: fix irq setup order")' the > irq request isn't the last devm_* allocation. Without a deeper look at > the irq and testing this isn't a good solution. Since this driver relies > on the devm mechanism, requesting a interrupt should be the last thing > to avoid memory corruptions during unbinding. > > 'Commit 02e389e63e35 ("pinctrl: mcp23s08: fix irq setup order")' fixed the > order for the interrupt-controller use case only. The > mcp23s08_irq_setup() must be split into two to fix it for the > interrupt-controller use case and to register the irq at last. So the > irq will be freed first during unbind. > I'm no expert on the irq's, but after a bit of reading the patch makes sense to me. I've got one question below. > Cc: stable@vger.kernel.org > Cc: Dmitry Mastykin <mastichi@gmail.com> > Cc: Sebastian Reichel <sebastian.reichel@collabora.co.uk> > Fixes: 82039d244f87 ("pinctrl: mcp23s08: add pinconf support") > Fixes: 02e389e63e35 ("pinctrl: mcp23s08: fix irq setup order") > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > --- > drivers/pinctrl/pinctrl-mcp23s08.c | 15 +++++++++++++-- > 1 file changed, 13 insertions(+), 2 deletions(-) > > diff --git a/drivers/pinctrl/pinctrl-mcp23s08.c b/drivers/pinctrl/pinctrl-mcp23s08.c > index 472746931ea8..367b648be7c7 100644 > --- a/drivers/pinctrl/pinctrl-mcp23s08.c > +++ b/drivers/pinctrl/pinctrl-mcp23s08.c > @@ -636,6 +636,14 @@ static int mcp23s08_irq_setup(struct mcp23s08 *mcp) > return err; > } > > + return 0; > +} > + > +static int mcp23s08_irqchip_setup(struct mcp23s08 *mcp) > +{ > + struct gpio_chip *chip = &mcp->chip; > + int err; > + > err = gpiochip_irqchip_add_nested(chip, > &mcp23s08_irq_chip, > 0, > @@ -908,8 +916,8 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev, > goto fail; > } > > - if (mcp->irq && mcp->irq_controller) { > - ret = mcp23s08_irq_setup(mcp); > + if (mcp->irq_controller) { > + ret = mcp23s08_irqchip_setup(mcp); The condition check changes, which may make sense to someone more knowledgeable., but gpiochip_set_nested_irqchip in mcp23s08_irqchip_setup references mcp->irq as well so I'm a little confused. > if (ret) > goto fail; > } > @@ -941,6 +949,9 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev, > goto fail; > } > > + if (mcp->irq) > + ret = mcp23s08_irq_setup(mcp); > + Is there any point if it's not a irq_controller? > fail: > if (ret < 0) > dev_dbg(dev, "can't setup chip %d, --> %d\n", addr, ret); > Otherwise LGTM.
Hi Phil, On 18-09-27 21:05, Phil Reid wrote: > G'day Marco, > > > On 25/09/2018 10:56 PM, Marco Felsch wrote: > > Since 'commit 02e389e63e35 ("pinctrl: mcp23s08: fix irq setup order")' the > > irq request isn't the last devm_* allocation. Without a deeper look at > > the irq and testing this isn't a good solution. Since this driver relies > > on the devm mechanism, requesting a interrupt should be the last thing > > to avoid memory corruptions during unbinding. > > > > 'Commit 02e389e63e35 ("pinctrl: mcp23s08: fix irq setup order")' fixed the > > order for the interrupt-controller use case only. The > > mcp23s08_irq_setup() must be split into two to fix it for the > > interrupt-controller use case and to register the irq at last. So the > > irq will be freed first during unbind. > > > > I'm no expert on the irq's, but after a bit of reading the patch makes sense to me. > I've got one question below. Thanks for review. > > > Cc: stable@vger.kernel.org > > Cc: Dmitry Mastykin <mastichi@gmail.com> > > Cc: Sebastian Reichel <sebastian.reichel@collabora.co.uk> > > Fixes: 82039d244f87 ("pinctrl: mcp23s08: add pinconf support") > > Fixes: 02e389e63e35 ("pinctrl: mcp23s08: fix irq setup order") > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > > --- > > drivers/pinctrl/pinctrl-mcp23s08.c | 15 +++++++++++++-- > > 1 file changed, 13 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/pinctrl/pinctrl-mcp23s08.c b/drivers/pinctrl/pinctrl-mcp23s08.c > > index 472746931ea8..367b648be7c7 100644 > > --- a/drivers/pinctrl/pinctrl-mcp23s08.c > > +++ b/drivers/pinctrl/pinctrl-mcp23s08.c > > @@ -636,6 +636,14 @@ static int mcp23s08_irq_setup(struct mcp23s08 *mcp) > > return err; > > } > > + return 0; > > +} > > + > > +static int mcp23s08_irqchip_setup(struct mcp23s08 *mcp) > > +{ > > + struct gpio_chip *chip = &mcp->chip; > > + int err; > > + > > err = gpiochip_irqchip_add_nested(chip, > > &mcp23s08_irq_chip, > > 0, > > @@ -908,8 +916,8 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev, > > goto fail; > > } > > - if (mcp->irq && mcp->irq_controller) { > > - ret = mcp23s08_irq_setup(mcp); > > + if (mcp->irq_controller) { > > + ret = mcp23s08_irqchip_setup(mcp); > The condition check changes, which may make sense to someone more knowledgeable., > but gpiochip_set_nested_irqchip in mcp23s08_irqchip_setup references mcp->irq as well > so I'm a little confused. You're right, it make no sense to setup a irqchip without having a upstream irq. I will fix this for a v2. > > if (ret) > > goto fail; > > } > > @@ -941,6 +949,9 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev, > > goto fail; > > } > > + if (mcp->irq) > > + ret = mcp23s08_irq_setup(mcp); > > + > Is there any point if it's not a irq_controller? AFAK, if it is a irqchip/irq_controller it can be used as a irq-parent for other devices. The irq itself can be setup without irq_controller must be set. E.g. Inform the System about a changed gpio state. Kind regards, Marco > > > fail: > > if (ret < 0) > > dev_dbg(dev, "can't setup chip %d, --> %d\n", addr, ret); > > > > Otherwise LGTM. > > -- > Regards > Phil Reid >
diff --git a/drivers/pinctrl/pinctrl-mcp23s08.c b/drivers/pinctrl/pinctrl-mcp23s08.c index 472746931ea8..367b648be7c7 100644 --- a/drivers/pinctrl/pinctrl-mcp23s08.c +++ b/drivers/pinctrl/pinctrl-mcp23s08.c @@ -636,6 +636,14 @@ static int mcp23s08_irq_setup(struct mcp23s08 *mcp) return err; } + return 0; +} + +static int mcp23s08_irqchip_setup(struct mcp23s08 *mcp) +{ + struct gpio_chip *chip = &mcp->chip; + int err; + err = gpiochip_irqchip_add_nested(chip, &mcp23s08_irq_chip, 0, @@ -908,8 +916,8 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev, goto fail; } - if (mcp->irq && mcp->irq_controller) { - ret = mcp23s08_irq_setup(mcp); + if (mcp->irq_controller) { + ret = mcp23s08_irqchip_setup(mcp); if (ret) goto fail; } @@ -941,6 +949,9 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev, goto fail; } + if (mcp->irq) + ret = mcp23s08_irq_setup(mcp); + fail: if (ret < 0) dev_dbg(dev, "can't setup chip %d, --> %d\n", addr, ret);
Since 'commit 02e389e63e35 ("pinctrl: mcp23s08: fix irq setup order")' the irq request isn't the last devm_* allocation. Without a deeper look at the irq and testing this isn't a good solution. Since this driver relies on the devm mechanism, requesting a interrupt should be the last thing to avoid memory corruptions during unbinding. 'Commit 02e389e63e35 ("pinctrl: mcp23s08: fix irq setup order")' fixed the order for the interrupt-controller use case only. The mcp23s08_irq_setup() must be split into two to fix it for the interrupt-controller use case and to register the irq at last. So the irq will be freed first during unbind. Cc: stable@vger.kernel.org Cc: Dmitry Mastykin <mastichi@gmail.com> Cc: Sebastian Reichel <sebastian.reichel@collabora.co.uk> Fixes: 82039d244f87 ("pinctrl: mcp23s08: add pinconf support") Fixes: 02e389e63e35 ("pinctrl: mcp23s08: fix irq setup order") Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> --- drivers/pinctrl/pinctrl-mcp23s08.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-)