| Message ID | 20220620122933.106035-1-u.kleine-koenig@pengutronix.de |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: grgpio: Fix device removing | expand |
On Mon, Jun 20, 2022 at 2:33 PM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > If a platform device's remove callback returns non-zero, the device core > emits a warning and still removes the device and calls the devm cleanup > callbacks. > > So it's not save to not unregister the gpiochip because on the next request safe > to a gpio the driver accesses kfree()'d memory. Also if an irq triggers, GPIO IRQ > the freed memory is accessed. > > Instead rely on the gpio framework to ensure that after gpiochip_remove() GPIO > all gpios are freed and so the corresponding irqs unmapped. (I'm think the GPIOs IRQs are unmapped I think > gpio framework doesn't guarantee that, but that's a bug there and out of GPIO > scope for this gpio driver to fix that.) GPIO > This is a preparation for making platform remove callbacks return void. ... What a bug are you seeing in the GPIO library? IIRC for IRQ over GPIO the GPIO holds the module reference count as well as GPIO device reference count. Am I wrong?
Hello Andy, On Mon, Jun 20, 2022 at 06:13:21PM +0200, Andy Shevchenko wrote: > On Mon, Jun 20, 2022 at 2:33 PM Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > > > > If a platform device's remove callback returns non-zero, the device core > > emits a warning and still removes the device and calls the devm cleanup > > callbacks. > > > > So it's not save to not unregister the gpiochip because on the next request > > safe > > > to a gpio the driver accesses kfree()'d memory. Also if an irq triggers, > > GPIO > IRQ > > > the freed memory is accessed. > > > > Instead rely on the gpio framework to ensure that after gpiochip_remove() > > GPIO > > > all gpios are freed and so the corresponding irqs unmapped. (I'm think the > > GPIOs > IRQs > are unmapped > > I think > > > gpio framework doesn't guarantee that, but that's a bug there and out of > > GPIO > > > scope for this gpio driver to fix that.) > > GPIO > > > This is a preparation for making platform remove callbacks return void. > > > ... > > What a bug are you seeing in the GPIO library? IIRC for IRQ over GPIO > the GPIO holds the module reference count as well as GPIO device > reference count. Am I wrong? I don't see a bug, I just looked into gpiochip_remove() to check if it ensures that all gpios go away and at one point is issues dev_crit(&gdev->dev, "REMOVING GPIOCHIP WITH GPIOS STILL REQUESTED\n"); and so I assumed that there is an issue, but maybe this isn't reachable in practise?! Best regards Uwe
diff --git a/drivers/gpio/gpio-grgpio.c b/drivers/gpio/gpio-grgpio.c index df563616f943..bea0e32c195d 100644 --- a/drivers/gpio/gpio-grgpio.c +++ b/drivers/gpio/gpio-grgpio.c @@ -434,25 +434,13 @@ static int grgpio_probe(struct platform_device *ofdev) static int grgpio_remove(struct platform_device *ofdev) { struct grgpio_priv *priv = platform_get_drvdata(ofdev); - int i; - int ret = 0; - - if (priv->domain) { - for (i = 0; i < GRGPIO_MAX_NGPIO; i++) { - if (priv->uirqs[i].refcnt != 0) { - ret = -EBUSY; - goto out; - } - } - } gpiochip_remove(&priv->gc); if (priv->domain) irq_domain_remove(priv->domain); -out: - return ret; + return 0; } static const struct of_device_id grgpio_match[] = {
If a platform device's remove callback returns non-zero, the device core emits a warning and still removes the device and calls the devm cleanup callbacks. So it's not save to not unregister the gpiochip because on the next request to a gpio the driver accesses kfree()'d memory. Also if an irq triggers, the freed memory is accessed. Instead rely on the gpio framework to ensure that after gpiochip_remove() all gpios are freed and so the corresponding irqs unmapped. (I'm think the gpio framework doesn't guarantee that, but that's a bug there and out of scope for this gpio driver to fix that.) This is a preparation for making platform remove callbacks return void. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- drivers/gpio/gpio-grgpio.c | 14 +------------- 1 file changed, 1 insertion(+), 13 deletions(-) base-commit: f2906aa863381afb0015a9eb7fefad885d4e5a56