| Message ID | 20240727123034.5541-3-benato.denis96@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | iio: fix bug with triggers not resuming after sleep | expand |
On Sat, 27 Jul 2024 14:30:34 +0200 Denis Benato <benato.denis96@gmail.com> wrote: > Prevent triggers from stop working after the device has entered sleep: > use iio_device_suspend_triggering and iio_device_resume_triggering helpers. Hi Denis, I'd got it into my head this was about main suspend / resume, but it's runtime PM. I assume the s2idle uses only that level which is interesting. Anyhow, solution seems safe. We might be able to do something nicer in the long run as potentially we could have the trigger driver notified when all consumers have entered this state at which point it could stop generating triggers at all. Anyhow, that's a job for when we actually care about it. Applied to the togreg branch of iio.git and pushed out as testing for 0-day to poke at it. For now I'm not keen to see this pushed into drivers where we don't know if anyone is running into this particular situation. We can reevaluate that if we start getting lots of reports of this. I'm also not going to rush this in as a fix. We can consider backporting it once it's been in mainline for a bit and no side effects have shown up. Thanks, Jonathan > > Signed-off-by: Denis Benato <benato.denis96@gmail.com> > --- > drivers/iio/imu/bmi323/bmi323.h | 1 + > drivers/iio/imu/bmi323/bmi323_core.c | 23 +++++++++++++++++++++++ > drivers/iio/imu/bmi323/bmi323_i2c.c | 1 + > drivers/iio/imu/bmi323/bmi323_spi.c | 1 + > 4 files changed, 26 insertions(+) > > diff --git a/drivers/iio/imu/bmi323/bmi323.h b/drivers/iio/imu/bmi323/bmi323.h > index dff126d41658..209bccb1f335 100644 > --- a/drivers/iio/imu/bmi323/bmi323.h > +++ b/drivers/iio/imu/bmi323/bmi323.h > @@ -205,5 +205,6 @@ > struct device; > int bmi323_core_probe(struct device *dev); > extern const struct regmap_config bmi323_regmap_config; > +extern const struct dev_pm_ops bmi323_core_pm_ops; > > #endif > diff --git a/drivers/iio/imu/bmi323/bmi323_core.c b/drivers/iio/imu/bmi323/bmi323_core.c > index d708d1fe3e42..4b2b211a3e88 100644 > --- a/drivers/iio/imu/bmi323/bmi323_core.c > +++ b/drivers/iio/imu/bmi323/bmi323_core.c > @@ -2121,6 +2121,29 @@ int bmi323_core_probe(struct device *dev) > } > EXPORT_SYMBOL_NS_GPL(bmi323_core_probe, IIO_BMI323); > > +#if defined(CONFIG_PM) > +static int bmi323_core_runtime_suspend(struct device *dev) > +{ > + struct iio_dev *indio_dev = dev_get_drvdata(dev); > + > + return iio_device_suspend_triggering(indio_dev); > +} > + > +static int bmi323_core_runtime_resume(struct device *dev) > +{ > + struct iio_dev *indio_dev = dev_get_drvdata(dev); > + > + return iio_device_resume_triggering(indio_dev); > +} > + > +#endif > + > +const struct dev_pm_ops bmi323_core_pm_ops = { > + SET_RUNTIME_PM_OPS(bmi323_core_runtime_suspend, > + bmi323_core_runtime_resume, NULL) > +}; > +EXPORT_SYMBOL_NS_GPL(bmi323_core_pm_ops, IIO_BMI323); > + > MODULE_DESCRIPTION("Bosch BMI323 IMU driver"); > MODULE_AUTHOR("Jagath Jog J <jagathjog1996@gmail.com>"); > MODULE_LICENSE("GPL"); > diff --git a/drivers/iio/imu/bmi323/bmi323_i2c.c b/drivers/iio/imu/bmi323/bmi323_i2c.c > index 52140bf05765..057342f4f816 100644 > --- a/drivers/iio/imu/bmi323/bmi323_i2c.c > +++ b/drivers/iio/imu/bmi323/bmi323_i2c.c > @@ -128,6 +128,7 @@ MODULE_DEVICE_TABLE(of, bmi323_of_i2c_match); > static struct i2c_driver bmi323_i2c_driver = { > .driver = { > .name = "bmi323", > + .pm = pm_ptr(&bmi323_core_pm_ops), > .of_match_table = bmi323_of_i2c_match, > .acpi_match_table = bmi323_acpi_match, > }, > diff --git a/drivers/iio/imu/bmi323/bmi323_spi.c b/drivers/iio/imu/bmi323/bmi323_spi.c > index 7b1e8127d0dd..487d4ee05246 100644 > --- a/drivers/iio/imu/bmi323/bmi323_spi.c > +++ b/drivers/iio/imu/bmi323/bmi323_spi.c > @@ -79,6 +79,7 @@ MODULE_DEVICE_TABLE(of, bmi323_of_spi_match); > static struct spi_driver bmi323_spi_driver = { > .driver = { > .name = "bmi323", > + .pm = pm_ptr(&bmi323_core_pm_ops), > .of_match_table = bmi323_of_spi_match, > }, > .probe = bmi323_spi_probe,
On 03/08/24 17:44, Jonathan Cameron wrote: > On Sat, 27 Jul 2024 14:30:34 +0200 > Denis Benato <benato.denis96@gmail.com> wrote: > >> Prevent triggers from stop working after the device has entered sleep: >> use iio_device_suspend_triggering and iio_device_resume_triggering helpers. > > Hi Denis, > Hello Jonathan, > I'd got it into my head this was about main suspend / resume, but > it's runtime PM. I assume the s2idle uses only that level which is > interesting. > I have catched the problem with s2idle, but I don-t fully understand it will manifest outside of said scenario, nor if it will at all and only s2idle is affected. > Anyhow, solution seems safe. We might be able to do something nicer > in the long run as potentially we could have the trigger driver > notified when all consumers have entered this state at which point it > could stop generating triggers at all. > Totally agree. > Anyhow, that's a job for when we actually care about it. > > Applied to the togreg branch of iio.git and pushed out as testing > for 0-day to poke at it. > I have made a mistake while cleaning up patch 1/2 for submission and lost a piece: the pollfunc->irq=0 you suggested in your first mail. I would be more than happy to provide a v3, but if you prefer I can also send a separate patch. I am sorry about that and I would like guidance on what to do in cases like this. > For now I'm not keen to see this pushed into drivers where we don't > know if anyone is running into this particular situation. We can > reevaluate that if we start getting lots of reports of this. > I catched the issue while developing an application for a handheld PC. As the application will target these kind of devices we can apply the fix to every relevant driver (bmi260 comes to mind) and have that well-tested on multiple drivers. > I'm also not going to rush this in as a fix. We can consider backporting > it once it's been in mainline for a bit and no side effects have > shown up. > > Thanks, > > Jonathan > Thanks, Denis >> >> Signed-off-by: Denis Benato <benato.denis96@gmail.com> >> --- >> drivers/iio/imu/bmi323/bmi323.h | 1 + >> drivers/iio/imu/bmi323/bmi323_core.c | 23 +++++++++++++++++++++++ >> drivers/iio/imu/bmi323/bmi323_i2c.c | 1 + >> drivers/iio/imu/bmi323/bmi323_spi.c | 1 + >> 4 files changed, 26 insertions(+) >> >> diff --git a/drivers/iio/imu/bmi323/bmi323.h b/drivers/iio/imu/bmi323/bmi323.h >> index dff126d41658..209bccb1f335 100644 >> --- a/drivers/iio/imu/bmi323/bmi323.h >> +++ b/drivers/iio/imu/bmi323/bmi323.h >> @@ -205,5 +205,6 @@ >> struct device; >> int bmi323_core_probe(struct device *dev); >> extern const struct regmap_config bmi323_regmap_config; >> +extern const struct dev_pm_ops bmi323_core_pm_ops; >> >> #endif >> diff --git a/drivers/iio/imu/bmi323/bmi323_core.c b/drivers/iio/imu/bmi323/bmi323_core.c >> index d708d1fe3e42..4b2b211a3e88 100644 >> --- a/drivers/iio/imu/bmi323/bmi323_core.c >> +++ b/drivers/iio/imu/bmi323/bmi323_core.c >> @@ -2121,6 +2121,29 @@ int bmi323_core_probe(struct device *dev) >> } >> EXPORT_SYMBOL_NS_GPL(bmi323_core_probe, IIO_BMI323); >> >> +#if defined(CONFIG_PM) >> +static int bmi323_core_runtime_suspend(struct device *dev) >> +{ >> + struct iio_dev *indio_dev = dev_get_drvdata(dev); >> + >> + return iio_device_suspend_triggering(indio_dev); >> +} >> + >> +static int bmi323_core_runtime_resume(struct device *dev) >> +{ >> + struct iio_dev *indio_dev = dev_get_drvdata(dev); >> + >> + return iio_device_resume_triggering(indio_dev); >> +} >> + >> +#endif >> + >> +const struct dev_pm_ops bmi323_core_pm_ops = { >> + SET_RUNTIME_PM_OPS(bmi323_core_runtime_suspend, >> + bmi323_core_runtime_resume, NULL) >> +}; >> +EXPORT_SYMBOL_NS_GPL(bmi323_core_pm_ops, IIO_BMI323); >> + >> MODULE_DESCRIPTION("Bosch BMI323 IMU driver"); >> MODULE_AUTHOR("Jagath Jog J <jagathjog1996@gmail.com>"); >> MODULE_LICENSE("GPL"); >> diff --git a/drivers/iio/imu/bmi323/bmi323_i2c.c b/drivers/iio/imu/bmi323/bmi323_i2c.c >> index 52140bf05765..057342f4f816 100644 >> --- a/drivers/iio/imu/bmi323/bmi323_i2c.c >> +++ b/drivers/iio/imu/bmi323/bmi323_i2c.c >> @@ -128,6 +128,7 @@ MODULE_DEVICE_TABLE(of, bmi323_of_i2c_match); >> static struct i2c_driver bmi323_i2c_driver = { >> .driver = { >> .name = "bmi323", >> + .pm = pm_ptr(&bmi323_core_pm_ops), >> .of_match_table = bmi323_of_i2c_match, >> .acpi_match_table = bmi323_acpi_match, >> }, >> diff --git a/drivers/iio/imu/bmi323/bmi323_spi.c b/drivers/iio/imu/bmi323/bmi323_spi.c >> index 7b1e8127d0dd..487d4ee05246 100644 >> --- a/drivers/iio/imu/bmi323/bmi323_spi.c >> +++ b/drivers/iio/imu/bmi323/bmi323_spi.c >> @@ -79,6 +79,7 @@ MODULE_DEVICE_TABLE(of, bmi323_of_spi_match); >> static struct spi_driver bmi323_spi_driver = { >> .driver = { >> .name = "bmi323", >> + .pm = pm_ptr(&bmi323_core_pm_ops), >> .of_match_table = bmi323_of_spi_match, >> }, >> .probe = bmi323_spi_probe, >
On Sun, 4 Aug 2024 17:40:49 +0200 Denis Benato <benato.denis96@gmail.com> wrote: > On 03/08/24 17:44, Jonathan Cameron wrote: > > On Sat, 27 Jul 2024 14:30:34 +0200 > > Denis Benato <benato.denis96@gmail.com> wrote: > > > >> Prevent triggers from stop working after the device has entered sleep: > >> use iio_device_suspend_triggering and iio_device_resume_triggering helpers. > > > > Hi Denis, > > > Hello Jonathan, > > I'd got it into my head this was about main suspend / resume, but > > it's runtime PM. I assume the s2idle uses only that level which is > > interesting. > > > I have catched the problem with s2idle, but I don-t fully understand > it will manifest outside of said scenario, nor if it will at all and > only s2idle is affected. > > > Anyhow, solution seems safe. We might be able to do something nicer > > in the long run as potentially we could have the trigger driver > > notified when all consumers have entered this state at which point it > > could stop generating triggers at all. > > Totally agree. > > Anyhow, that's a job for when we actually care about it. > > > > Applied to the togreg branch of iio.git and pushed out as testing > > for 0-day to poke at it. > > > I have made a mistake while cleaning up patch 1/2 for submission and lost a piece: > the pollfunc->irq=0 you suggested in your first mail. > > I would be more than happy to provide a v3, but if you prefer I can also send > a separate patch. Send a v3. I'll try and remember to drop the v2 before sending a pull request upstream. > > I am sorry about that and I would like guidance on what to do in cases like this. Not a problem, we all do stuff like this from time to time! > > For now I'm not keen to see this pushed into drivers where we don't > > know if anyone is running into this particular situation. We can > > reevaluate that if we start getting lots of reports of this. > > > I catched the issue while developing an application for a handheld PC. > > As the application will target these kind of devices we can apply the fix > to every relevant driver (bmi260 comes to mind) and have that well-tested > on multiple drivers. Ok. Let's see how it goes with a few drivers. Jonathan > > I'm also not going to rush this in as a fix. We can consider backporting > > it once it's been in mainline for a bit and no side effects have > > shown up. > > > > Thanks, > > > > Jonathan > > > Thanks, > > Denis > >> > >> Signed-off-by: Denis Benato <benato.denis96@gmail.com> > >> --- > >> drivers/iio/imu/bmi323/bmi323.h | 1 + > >> drivers/iio/imu/bmi323/bmi323_core.c | 23 +++++++++++++++++++++++ > >> drivers/iio/imu/bmi323/bmi323_i2c.c | 1 + > >> drivers/iio/imu/bmi323/bmi323_spi.c | 1 + > >> 4 files changed, 26 insertions(+) > >> > >> diff --git a/drivers/iio/imu/bmi323/bmi323.h b/drivers/iio/imu/bmi323/bmi323.h > >> index dff126d41658..209bccb1f335 100644 > >> --- a/drivers/iio/imu/bmi323/bmi323.h > >> +++ b/drivers/iio/imu/bmi323/bmi323.h > >> @@ -205,5 +205,6 @@ > >> struct device; > >> int bmi323_core_probe(struct device *dev); > >> extern const struct regmap_config bmi323_regmap_config; > >> +extern const struct dev_pm_ops bmi323_core_pm_ops; > >> > >> #endif > >> diff --git a/drivers/iio/imu/bmi323/bmi323_core.c b/drivers/iio/imu/bmi323/bmi323_core.c > >> index d708d1fe3e42..4b2b211a3e88 100644 > >> --- a/drivers/iio/imu/bmi323/bmi323_core.c > >> +++ b/drivers/iio/imu/bmi323/bmi323_core.c > >> @@ -2121,6 +2121,29 @@ int bmi323_core_probe(struct device *dev) > >> } > >> EXPORT_SYMBOL_NS_GPL(bmi323_core_probe, IIO_BMI323); > >> > >> +#if defined(CONFIG_PM) > >> +static int bmi323_core_runtime_suspend(struct device *dev) > >> +{ > >> + struct iio_dev *indio_dev = dev_get_drvdata(dev); > >> + > >> + return iio_device_suspend_triggering(indio_dev); > >> +} > >> + > >> +static int bmi323_core_runtime_resume(struct device *dev) > >> +{ > >> + struct iio_dev *indio_dev = dev_get_drvdata(dev); > >> + > >> + return iio_device_resume_triggering(indio_dev); > >> +} > >> + > >> +#endif > >> + > >> +const struct dev_pm_ops bmi323_core_pm_ops = { > >> + SET_RUNTIME_PM_OPS(bmi323_core_runtime_suspend, > >> + bmi323_core_runtime_resume, NULL) > >> +}; > >> +EXPORT_SYMBOL_NS_GPL(bmi323_core_pm_ops, IIO_BMI323); > >> + > >> MODULE_DESCRIPTION("Bosch BMI323 IMU driver"); > >> MODULE_AUTHOR("Jagath Jog J <jagathjog1996@gmail.com>"); > >> MODULE_LICENSE("GPL"); > >> diff --git a/drivers/iio/imu/bmi323/bmi323_i2c.c b/drivers/iio/imu/bmi323/bmi323_i2c.c > >> index 52140bf05765..057342f4f816 100644 > >> --- a/drivers/iio/imu/bmi323/bmi323_i2c.c > >> +++ b/drivers/iio/imu/bmi323/bmi323_i2c.c > >> @@ -128,6 +128,7 @@ MODULE_DEVICE_TABLE(of, bmi323_of_i2c_match); > >> static struct i2c_driver bmi323_i2c_driver = { > >> .driver = { > >> .name = "bmi323", > >> + .pm = pm_ptr(&bmi323_core_pm_ops), > >> .of_match_table = bmi323_of_i2c_match, > >> .acpi_match_table = bmi323_acpi_match, > >> }, > >> diff --git a/drivers/iio/imu/bmi323/bmi323_spi.c b/drivers/iio/imu/bmi323/bmi323_spi.c > >> index 7b1e8127d0dd..487d4ee05246 100644 > >> --- a/drivers/iio/imu/bmi323/bmi323_spi.c > >> +++ b/drivers/iio/imu/bmi323/bmi323_spi.c > >> @@ -79,6 +79,7 @@ MODULE_DEVICE_TABLE(of, bmi323_of_spi_match); > >> static struct spi_driver bmi323_spi_driver = { > >> .driver = { > >> .name = "bmi323", > >> + .pm = pm_ptr(&bmi323_core_pm_ops), > >> .of_match_table = bmi323_of_spi_match, > >> }, > >> .probe = bmi323_spi_probe, > > > >
diff --git a/drivers/iio/imu/bmi323/bmi323.h b/drivers/iio/imu/bmi323/bmi323.h index dff126d41658..209bccb1f335 100644 --- a/drivers/iio/imu/bmi323/bmi323.h +++ b/drivers/iio/imu/bmi323/bmi323.h @@ -205,5 +205,6 @@ struct device; int bmi323_core_probe(struct device *dev); extern const struct regmap_config bmi323_regmap_config; +extern const struct dev_pm_ops bmi323_core_pm_ops; #endif diff --git a/drivers/iio/imu/bmi323/bmi323_core.c b/drivers/iio/imu/bmi323/bmi323_core.c index d708d1fe3e42..4b2b211a3e88 100644 --- a/drivers/iio/imu/bmi323/bmi323_core.c +++ b/drivers/iio/imu/bmi323/bmi323_core.c @@ -2121,6 +2121,29 @@ int bmi323_core_probe(struct device *dev) } EXPORT_SYMBOL_NS_GPL(bmi323_core_probe, IIO_BMI323); +#if defined(CONFIG_PM) +static int bmi323_core_runtime_suspend(struct device *dev) +{ + struct iio_dev *indio_dev = dev_get_drvdata(dev); + + return iio_device_suspend_triggering(indio_dev); +} + +static int bmi323_core_runtime_resume(struct device *dev) +{ + struct iio_dev *indio_dev = dev_get_drvdata(dev); + + return iio_device_resume_triggering(indio_dev); +} + +#endif + +const struct dev_pm_ops bmi323_core_pm_ops = { + SET_RUNTIME_PM_OPS(bmi323_core_runtime_suspend, + bmi323_core_runtime_resume, NULL) +}; +EXPORT_SYMBOL_NS_GPL(bmi323_core_pm_ops, IIO_BMI323); + MODULE_DESCRIPTION("Bosch BMI323 IMU driver"); MODULE_AUTHOR("Jagath Jog J <jagathjog1996@gmail.com>"); MODULE_LICENSE("GPL"); diff --git a/drivers/iio/imu/bmi323/bmi323_i2c.c b/drivers/iio/imu/bmi323/bmi323_i2c.c index 52140bf05765..057342f4f816 100644 --- a/drivers/iio/imu/bmi323/bmi323_i2c.c +++ b/drivers/iio/imu/bmi323/bmi323_i2c.c @@ -128,6 +128,7 @@ MODULE_DEVICE_TABLE(of, bmi323_of_i2c_match); static struct i2c_driver bmi323_i2c_driver = { .driver = { .name = "bmi323", + .pm = pm_ptr(&bmi323_core_pm_ops), .of_match_table = bmi323_of_i2c_match, .acpi_match_table = bmi323_acpi_match, }, diff --git a/drivers/iio/imu/bmi323/bmi323_spi.c b/drivers/iio/imu/bmi323/bmi323_spi.c index 7b1e8127d0dd..487d4ee05246 100644 --- a/drivers/iio/imu/bmi323/bmi323_spi.c +++ b/drivers/iio/imu/bmi323/bmi323_spi.c @@ -79,6 +79,7 @@ MODULE_DEVICE_TABLE(of, bmi323_of_spi_match); static struct spi_driver bmi323_spi_driver = { .driver = { .name = "bmi323", + .pm = pm_ptr(&bmi323_core_pm_ops), .of_match_table = bmi323_of_spi_match, }, .probe = bmi323_spi_probe,
Prevent triggers from stop working after the device has entered sleep: use iio_device_suspend_triggering and iio_device_resume_triggering helpers. Signed-off-by: Denis Benato <benato.denis96@gmail.com> --- drivers/iio/imu/bmi323/bmi323.h | 1 + drivers/iio/imu/bmi323/bmi323_core.c | 23 +++++++++++++++++++++++ drivers/iio/imu/bmi323/bmi323_i2c.c | 1 + drivers/iio/imu/bmi323/bmi323_spi.c | 1 + 4 files changed, 26 insertions(+)