| Message ID | 20220523083947.840708-1-u.kleine-koenig@pengutronix.de |
|---|---|
| State | Accepted |
| Commit | 7bb8a0cf49d5fede1104afdcb43bd2f8a1df3253 |
| Headers | show |
| Series | gpio: adp5588: Remove support for platform setup and teardown callbacks | expand |
On Mon, May 23, 2022 at 10:40 AM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > If the teardown callback failed in the gpio driver, it fails to free the > irq (if there is one). The device is removed anyhow. If later on the irq > triggers, all sorts of unpleasant things might happen (e.g. accessing > the struct adp5588_gpio which is already freed in the meantime or starting > i2c bus transfers for an unregistered device). Even before irq support was > added to this driver, exiting early was wrong; back then it failed to > unregister the gpiochip. > > Fortunately these callbacks aren't used any more since at least blackfin > was removed in 2018. So just drop them. > > Note that they are not removed from struct adp5588_gpio_platform_data > because the keyboard driver adp5588-keys.c also makes use of them. > (I didn't check if the callbacks might have been called twice, maybe there > is another reason hidden to better not call these functions.) > > This patch is a preparation for making i2c remove callbacks return void. > > Fixes: 80884094e344 ("gpio: adp5588-gpio: new driver for ADP5588 GPIO expanders") > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Good riddance! But also remove the setup and teardown prototypes in include/linux/platform_data/adp5588.h With that fixed: Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Mon, May 23, 2022 at 11:09 AM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > On Mon, May 23, 2022 at 10:43:20AM +0200, Linus Walleij wrote: > > On Mon, May 23, 2022 at 10:40 AM Uwe Kleine-König > > <u.kleine-koenig@pengutronix.de> wrote: > > > > > If the teardown callback failed in the gpio driver, it fails to free the > > > irq (if there is one). The device is removed anyhow. If later on the irq > > > triggers, all sorts of unpleasant things might happen (e.g. accessing > > > the struct adp5588_gpio which is already freed in the meantime or starting > > > i2c bus transfers for an unregistered device). Even before irq support was > > > added to this driver, exiting early was wrong; back then it failed to > > > unregister the gpiochip. > > > > > > Fortunately these callbacks aren't used any more since at least blackfin > > > was removed in 2018. So just drop them. > > > > > > Note that they are not removed from struct adp5588_gpio_platform_data > > > because the keyboard driver adp5588-keys.c also makes use of them. > > > (I didn't check if the callbacks might have been called twice, maybe there > > > is another reason hidden to better not call these functions.) > > > > > > This patch is a preparation for making i2c remove callbacks return void. > > > > > > Fixes: 80884094e344 ("gpio: adp5588-gpio: new driver for ADP5588 GPIO expanders") > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > > > Good riddance! > > But also remove the setup and teardown prototypes in > > include/linux/platform_data/adp5588.h > > Please reread the commit log. They were not remove on purpose. If you > missed that part, you also missed half of the "fun". :-P Ugh never review patches before the second cup of coffee. What a weird reuse of callbacks. Oh well they are not so weird after this. Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
> -----Original Message----- > From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > Sent: Montag, 23. Mai 2022 10:40 > To: Hennerich, Michael <Michael.Hennerich@analog.com>; Linus Walleij > <linus.walleij@linaro.org>; Bartosz Golaszewski <brgl@bgdev.pl> > Cc: linux-gpio@vger.kernel.org; kernel@pengutronix.de > Subject: [PATCH] gpio: adp5588: Remove support for platform setup and > teardown callbacks > > > If the teardown callback failed in the gpio driver, it fails to free the irq (if there is > one). The device is removed anyhow. If later on the irq triggers, all sorts of > unpleasant things might happen (e.g. accessing the struct adp5588_gpio which > is already freed in the meantime or starting i2c bus transfers for an unregistered > device). Even before irq support was added to this driver, exiting early was > wrong; back then it failed to unregister the gpiochip. > > Fortunately these callbacks aren't used any more since at least blackfin was > removed in 2018. So just drop them. > > Note that they are not removed from struct adp5588_gpio_platform_data > because the keyboard driver adp5588-keys.c also makes use of them. > (I didn't check if the callbacks might have been called twice, maybe there is > another reason hidden to better not call these functions.) > > This patch is a preparation for making i2c remove callbacks return void. > > Fixes: 80884094e344 ("gpio: adp5588-gpio: new driver for ADP5588 GPIO > expanders") > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Acked-by: Michael Hennerich <michael.hennerich@analog.com> > --- > drivers/gpio/gpio-adp5588.c | 19 ------------------- > 1 file changed, 19 deletions(-) > > diff --git a/drivers/gpio/gpio-adp5588.c b/drivers/gpio/gpio-adp5588.c index > f1e4ac90e7d3..e388e75103f4 100644 > --- a/drivers/gpio/gpio-adp5588.c > +++ b/drivers/gpio/gpio-adp5588.c > @@ -406,12 +406,6 @@ static int adp5588_gpio_probe(struct i2c_client > *client) > if (ret) > return ret; > > - if (pdata && pdata->setup) { > - ret = pdata->setup(client, gc->base, gc->ngpio, pdata- > >context); > - if (ret < 0) > - dev_warn(&client->dev, "setup failed, %d\n", ret); > - } > - > i2c_set_clientdata(client, dev); > > return 0; > @@ -419,20 +413,7 @@ static int adp5588_gpio_probe(struct i2c_client > *client) > > static int adp5588_gpio_remove(struct i2c_client *client) { > - struct adp5588_gpio_platform_data *pdata = > - dev_get_platdata(&client->dev); > struct adp5588_gpio *dev = i2c_get_clientdata(client); > - int ret; > - > - if (pdata && pdata->teardown) { > - ret = pdata->teardown(client, > - dev->gpio_chip.base, dev- > >gpio_chip.ngpio, > - pdata->context); > - if (ret < 0) { > - dev_err(&client->dev, "teardown failed %d\n", ret); > - return ret; > - } > - } > > if (dev->client->irq) > free_irq(dev->client->irq, dev); > > base-commit: 4b0986a3613c92f4ec1bdc7f60ec66fea135991f > -- > 2.35.1
On Mon, May 23, 2022 at 10:40 AM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > If the teardown callback failed in the gpio driver, it fails to free the > irq (if there is one). The device is removed anyhow. If later on the irq > triggers, all sorts of unpleasant things might happen (e.g. accessing > the struct adp5588_gpio which is already freed in the meantime or starting > i2c bus transfers for an unregistered device). Even before irq support was > added to this driver, exiting early was wrong; back then it failed to > unregister the gpiochip. > > Fortunately these callbacks aren't used any more since at least blackfin > was removed in 2018. So just drop them. > > Note that they are not removed from struct adp5588_gpio_platform_data > because the keyboard driver adp5588-keys.c also makes use of them. > (I didn't check if the callbacks might have been called twice, maybe there > is another reason hidden to better not call these functions.) > > This patch is a preparation for making i2c remove callbacks return void. > > Fixes: 80884094e344 ("gpio: adp5588-gpio: new driver for ADP5588 GPIO expanders") > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > --- Applied, thanks! Bart
diff --git a/drivers/gpio/gpio-adp5588.c b/drivers/gpio/gpio-adp5588.c index f1e4ac90e7d3..e388e75103f4 100644 --- a/drivers/gpio/gpio-adp5588.c +++ b/drivers/gpio/gpio-adp5588.c @@ -406,12 +406,6 @@ static int adp5588_gpio_probe(struct i2c_client *client) if (ret) return ret; - if (pdata && pdata->setup) { - ret = pdata->setup(client, gc->base, gc->ngpio, pdata->context); - if (ret < 0) - dev_warn(&client->dev, "setup failed, %d\n", ret); - } - i2c_set_clientdata(client, dev); return 0; @@ -419,20 +413,7 @@ static int adp5588_gpio_probe(struct i2c_client *client) static int adp5588_gpio_remove(struct i2c_client *client) { - struct adp5588_gpio_platform_data *pdata = - dev_get_platdata(&client->dev); struct adp5588_gpio *dev = i2c_get_clientdata(client); - int ret; - - if (pdata && pdata->teardown) { - ret = pdata->teardown(client, - dev->gpio_chip.base, dev->gpio_chip.ngpio, - pdata->context); - if (ret < 0) { - dev_err(&client->dev, "teardown failed %d\n", ret); - return ret; - } - } if (dev->client->irq) free_irq(dev->client->irq, dev);
If the teardown callback failed in the gpio driver, it fails to free the irq (if there is one). The device is removed anyhow. If later on the irq triggers, all sorts of unpleasant things might happen (e.g. accessing the struct adp5588_gpio which is already freed in the meantime or starting i2c bus transfers for an unregistered device). Even before irq support was added to this driver, exiting early was wrong; back then it failed to unregister the gpiochip. Fortunately these callbacks aren't used any more since at least blackfin was removed in 2018. So just drop them. Note that they are not removed from struct adp5588_gpio_platform_data because the keyboard driver adp5588-keys.c also makes use of them. (I didn't check if the callbacks might have been called twice, maybe there is another reason hidden to better not call these functions.) This patch is a preparation for making i2c remove callbacks return void. Fixes: 80884094e344 ("gpio: adp5588-gpio: new driver for ADP5588 GPIO expanders") Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- drivers/gpio/gpio-adp5588.c | 19 ------------------- 1 file changed, 19 deletions(-) base-commit: 4b0986a3613c92f4ec1bdc7f60ec66fea135991f