| Message ID | 20220826122352.288438-1-shreeya.patel@collabora.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v2] iio: light: tsl2583: Fix module unloading | expand |
On Fri, 26 Aug 2022 17:53:52 +0530 Shreeya Patel <shreeya.patel@collabora.com> wrote: > tsl2583 uses devm_iio_device_register() function and > calling iio_device_unregister() in remove breaks the > module unloading. > Fix this by using iio_device_register() instead of > devm_iio_device_register() function in probe. Not sure why you are wrapping at 55 chars. I rewrapped this whilst applying. Reworded it a little too as I was touching it anyway. Applied to the fixes-togreg branch of iio.git. > > Cc: stable@vger.kernel.org > Fixes: 371894f5d1a0 ("iio: tsl2583: add runtime power management support") I took a look at this patch and it introduces the issue I just pointed out in replying to your v1 by dropping the /* Make sure the chip is on */ Which was correct even with runtime pm because it covered the case of runtime_pm being disabled. We probably need to bring that back as well, perhaps as part of a cleanup patch taking this fully devm_ This driver has another issue for working if runtime PM isn't built into the kernel which is that it checks the return of pm_runtime_put_autosuspend() which calls static inline int __pm_runtime_suspend(struct device *dev, int rpmflags) { return -ENOSYS; } I've been meaning to do an audit for drivers that have this problem for a while, but not yet gotten to it. An ideal IIO driver needs to work correctly whether or not CONFIG_PM is enabled. Jonathan > Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com> > --- > Changes in v2 > - Use iio_device_register() instead of devm_iio_device_register() > - Add fixes and stable tags > > drivers/iio/light/tsl2583.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/iio/light/tsl2583.c b/drivers/iio/light/tsl2583.c > index 82662dab87c0..94d75ec687c3 100644 > --- a/drivers/iio/light/tsl2583.c > +++ b/drivers/iio/light/tsl2583.c > @@ -858,7 +858,7 @@ static int tsl2583_probe(struct i2c_client *clientp, > TSL2583_POWER_OFF_DELAY_MS); > pm_runtime_use_autosuspend(&clientp->dev); > > - ret = devm_iio_device_register(indio_dev->dev.parent, indio_dev); > + ret = iio_device_register(indio_dev); > if (ret) { > dev_err(&clientp->dev, "%s: iio registration failed\n", > __func__);
On 28/08/22 22:03, Jonathan Cameron wrote: > On Fri, 26 Aug 2022 17:53:52 +0530 > Shreeya Patel <shreeya.patel@collabora.com> wrote: > >> tsl2583 uses devm_iio_device_register() function and >> calling iio_device_unregister() in remove breaks the >> module unloading. >> Fix this by using iio_device_register() instead of >> devm_iio_device_register() function in probe. > Not sure why you are wrapping at 55 chars. I rewrapped this whilst applying. > > Reworded it a little too as I was touching it anyway. > > Applied to the fixes-togreg branch of iio.git. > >> Cc: stable@vger.kernel.org >> Fixes: 371894f5d1a0 ("iio: tsl2583: add runtime power management support") > I took a look at this patch and it introduces the issue I just pointed > out in replying to your v1 by dropping the > /* Make sure the chip is on */ > Which was correct even with runtime pm because it covered the case of > runtime_pm being disabled. We probably need to bring that back as well, > perhaps as part of a cleanup patch taking this fully devm_ Hi Jonathan, I can work on fixing some of the issues with this driver in my free time. Thanks for taking a look at the patch and letting me know. FYI, I am also planning to work on ADT7316 driver to move out of staging. I have the adt7516 eval board with me which I'll be using for testing. Thanks, Shreeya Patel > This driver has another issue for working if runtime PM isn't built into > the kernel which is that it checks the return of pm_runtime_put_autosuspend() > which calls > > static inline int __pm_runtime_suspend(struct device *dev, int rpmflags) > { > return -ENOSYS; > } > > I've been meaning to do an audit for drivers that have this problem for > a while, but not yet gotten to it. > > An ideal IIO driver needs to work correctly whether or not CONFIG_PM is > enabled. > > Jonathan > > >> Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com> >> --- >> Changes in v2 >> - Use iio_device_register() instead of devm_iio_device_register() >> - Add fixes and stable tags >> >> drivers/iio/light/tsl2583.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/iio/light/tsl2583.c b/drivers/iio/light/tsl2583.c >> index 82662dab87c0..94d75ec687c3 100644 >> --- a/drivers/iio/light/tsl2583.c >> +++ b/drivers/iio/light/tsl2583.c >> @@ -858,7 +858,7 @@ static int tsl2583_probe(struct i2c_client *clientp, >> TSL2583_POWER_OFF_DELAY_MS); >> pm_runtime_use_autosuspend(&clientp->dev); >> >> - ret = devm_iio_device_register(indio_dev->dev.parent, indio_dev); >> + ret = iio_device_register(indio_dev); >> if (ret) { >> dev_err(&clientp->dev, "%s: iio registration failed\n", >> __func__); >
On 28/08/22 22:03, Jonathan Cameron wrote: > On Fri, 26 Aug 2022 17:53:52 +0530 > Shreeya Patel <shreeya.patel@collabora.com> wrote: > >> tsl2583 uses devm_iio_device_register() function and >> calling iio_device_unregister() in remove breaks the >> module unloading. >> Fix this by using iio_device_register() instead of >> devm_iio_device_register() function in probe. > Not sure why you are wrapping at 55 chars. I rewrapped this whilst applying. > > Reworded it a little too as I was touching it anyway. > > Applied to the fixes-togreg branch of iio.git. Hi Jonathan, I was wondering if this got picked by you. I don't see it in fixes-togreg that's why wanted to just confirm if you aren't looking for some extra changes in this. Thanks Shreeya Patel > >> Cc: stable@vger.kernel.org >> Fixes: 371894f5d1a0 ("iio: tsl2583: add runtime power management support") > I took a look at this patch and it introduces the issue I just pointed > out in replying to your v1 by dropping the > /* Make sure the chip is on */ > Which was correct even with runtime pm because it covered the case of > runtime_pm being disabled. We probably need to bring that back as well, > perhaps as part of a cleanup patch taking this fully devm_ > > This driver has another issue for working if runtime PM isn't built into > the kernel which is that it checks the return of pm_runtime_put_autosuspend() > which calls > > static inline int __pm_runtime_suspend(struct device *dev, int rpmflags) > { > return -ENOSYS; > } > > I've been meaning to do an audit for drivers that have this problem for > a while, but not yet gotten to it. > > An ideal IIO driver needs to work correctly whether or not CONFIG_PM is > enabled. > > Jonathan > > >> Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com> >> --- >> Changes in v2 >> - Use iio_device_register() instead of devm_iio_device_register() >> - Add fixes and stable tags >> >> drivers/iio/light/tsl2583.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/iio/light/tsl2583.c b/drivers/iio/light/tsl2583.c >> index 82662dab87c0..94d75ec687c3 100644 >> --- a/drivers/iio/light/tsl2583.c >> +++ b/drivers/iio/light/tsl2583.c >> @@ -858,7 +858,7 @@ static int tsl2583_probe(struct i2c_client *clientp, >> TSL2583_POWER_OFF_DELAY_MS); >> pm_runtime_use_autosuspend(&clientp->dev); >> >> - ret = devm_iio_device_register(indio_dev->dev.parent, indio_dev); >> + ret = iio_device_register(indio_dev); >> if (ret) { >> dev_err(&clientp->dev, "%s: iio registration failed\n", >> __func__); >
On Fri, 9 Sep 2022 00:25:49 +0530 Shreeya Patel <shreeya.patel@collabora.com> wrote: > On 28/08/22 22:03, Jonathan Cameron wrote: > > On Fri, 26 Aug 2022 17:53:52 +0530 > > Shreeya Patel <shreeya.patel@collabora.com> wrote: > > > >> tsl2583 uses devm_iio_device_register() function and > >> calling iio_device_unregister() in remove breaks the > >> module unloading. > >> Fix this by using iio_device_register() instead of > >> devm_iio_device_register() function in probe. > > Not sure why you are wrapping at 55 chars. I rewrapped this whilst applying. > > > > Reworded it a little too as I was touching it anyway. > > > > Applied to the fixes-togreg branch of iio.git. > > Hi Jonathan, > > I was wondering if this got picked by you. I don't see it in > fixes-togreg that's why wanted to just confirm if you aren't looking for > some extra changes in this. > > oops. I forgot to push that branch out. Done so now. Jonathan > Thanks > Shreeya Patel > > > > >> Cc: stable@vger.kernel.org > >> Fixes: 371894f5d1a0 ("iio: tsl2583: add runtime power management support") > > I took a look at this patch and it introduces the issue I just pointed > > out in replying to your v1 by dropping the > > /* Make sure the chip is on */ > > Which was correct even with runtime pm because it covered the case of > > runtime_pm being disabled. We probably need to bring that back as well, > > perhaps as part of a cleanup patch taking this fully devm_ > > > > This driver has another issue for working if runtime PM isn't built into > > the kernel which is that it checks the return of pm_runtime_put_autosuspend() > > which calls > > > > static inline int __pm_runtime_suspend(struct device *dev, int rpmflags) > > { > > return -ENOSYS; > > } > > > > I've been meaning to do an audit for drivers that have this problem for > > a while, but not yet gotten to it. > > > > An ideal IIO driver needs to work correctly whether or not CONFIG_PM is > > enabled. > > > > Jonathan > > > > > >> Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com> > >> --- > >> Changes in v2 > >> - Use iio_device_register() instead of devm_iio_device_register() > >> - Add fixes and stable tags > >> > >> drivers/iio/light/tsl2583.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/drivers/iio/light/tsl2583.c b/drivers/iio/light/tsl2583.c > >> index 82662dab87c0..94d75ec687c3 100644 > >> --- a/drivers/iio/light/tsl2583.c > >> +++ b/drivers/iio/light/tsl2583.c > >> @@ -858,7 +858,7 @@ static int tsl2583_probe(struct i2c_client *clientp, > >> TSL2583_POWER_OFF_DELAY_MS); > >> pm_runtime_use_autosuspend(&clientp->dev); > >> > >> - ret = devm_iio_device_register(indio_dev->dev.parent, indio_dev); > >> + ret = iio_device_register(indio_dev); > >> if (ret) { > >> dev_err(&clientp->dev, "%s: iio registration failed\n", > >> __func__); > >
diff --git a/drivers/iio/light/tsl2583.c b/drivers/iio/light/tsl2583.c index 82662dab87c0..94d75ec687c3 100644 --- a/drivers/iio/light/tsl2583.c +++ b/drivers/iio/light/tsl2583.c @@ -858,7 +858,7 @@ static int tsl2583_probe(struct i2c_client *clientp, TSL2583_POWER_OFF_DELAY_MS); pm_runtime_use_autosuspend(&clientp->dev); - ret = devm_iio_device_register(indio_dev->dev.parent, indio_dev); + ret = iio_device_register(indio_dev); if (ret) { dev_err(&clientp->dev, "%s: iio registration failed\n", __func__);
tsl2583 uses devm_iio_device_register() function and calling iio_device_unregister() in remove breaks the module unloading. Fix this by using iio_device_register() instead of devm_iio_device_register() function in probe. Cc: stable@vger.kernel.org Fixes: 371894f5d1a0 ("iio: tsl2583: add runtime power management support") Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com> --- Changes in v2 - Use iio_device_register() instead of devm_iio_device_register() - Add fixes and stable tags drivers/iio/light/tsl2583.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)