PM: runtime: Convert pm_runtime_put_autosuspend() and friends to a void function
Subsystems (33)
cover PM: runtime: Convert pm_runtime_put_autosuspend() and friends to a void function
Similar to this effort:
Subject: [PATCH v1 00/23] PM: runtime: Convert pm_runtime_put() to a void function
https://lore.kernel.org/all/[email protected]/
i.e., commit 3afd8df02433 ("PM: runtime: Change pm_runtime_put() return type to void") and friends.
As with pm_runtime_put(), there is very little value in return-code checking for any of the other async __pm_runtime_suspend() and __pm_runtime_idle() APIs:
pm_runtime_put_autosuspend()
pm_request_idle()
pm_request_autosuspend()
Checking the return code is more likely to
produce a race condition (fighting with user space writing "on" to the "control" attribute) than to tell a consumer useful info.
Nobody outside KUnit tests checks pm_request_idle() or pm_request_autosuspend() return codes, but there are a number of pm_runtime_put_autosuspend() callers that check its return code. Thus, this series largely deals with pm_runtime_put_autosuspend().
Patches 1-11 are independent of each other, and similarly to the last series, I expect the respective maintainers can pick them up. We can apply patch 12 later. Patch 13 is also independent, but included in this topic for completeness. It can be applied any time.
This touches a lot of trees/maintainers, so I tried to keep CC's only for the relevant patch(es), plus the cover letter. I hope that's not too noisy.
- Brian
Brian Norris (13):
hwrng: cctrng: Discard pm_runtime_put_autosuspend() return value
drm: Discard pm_runtime_put_autosuspend() return values
iio: Discard pm_runtime_put_autosuspend() return values
media: venus: Discard pm_runtime_put_autosuspend() return values
pinctrl: qcom: lpass-lpi: Discard pm_runtime_put_autosuspend() return
values
spi: ti-qspi: Discard pm_runtime_put_autosuspend() return value
greybus: Discard pm_runtime_put_autosuspend() return value
ALSA: hda: Discard pm_runtime_put_autosuspend() return value
ASoC: codecs: lpass: Discard pm_runtime_put_autosuspend() return
values
ASoC: intel: atom: Discard pm_runtime_put_autosuspend() return value
ASoC: SOF: Discard pm_runtime_put_autosuspend() return values
PM: runtime: Drop pm_runtime_put_autosuspend() return code
PM: runtime: Drop pm_request_{idle,autosuspend}() return codes
drivers/base/power/runtime-test.c | 8 --- drivers/char/hw_random/cctrng.c | 6 +- drivers/gpu/drm/panel/panel-simple.c | 8 +-- drivers/gpu/drm/tidss/tidss_drv.c | 7 +-- drivers/gpu/drm/v3d/v3d_drv.h | 4 +- drivers/iio/accel/bmc150-accel-core.c | 14 ++--- drivers/iio/accel/fxls8962af-core.c | 11 +--- drivers/iio/accel/kxcjk-1013.c | 15 ++--- drivers/iio/accel/mma8452.c | 15 +++-- drivers/iio/accel/mma9551_core.c | 15 +++-- drivers/iio/adc/rcar-gyroadc.c | 4 +- drivers/iio/adc/ti-ads1015.c | 8 +-- drivers/iio/chemical/atlas-sensor.c | 4 +- .../common/hid-sensors/hid-sensor-trigger.c | 14 ++--- drivers/iio/gyro/bmg160_core.c | 17 +++-- drivers/iio/gyro/fxas21002c_core.c | 12 ++-- drivers/iio/imu/kmx61.c | 17 +++-- drivers/iio/light/apds9960.c | 2 +- drivers/iio/light/isl29028.c | 15 +---- drivers/iio/light/pa12203001.c | 3 +- drivers/iio/light/rpr0521.c | 15 +++-- drivers/iio/light/tsl2583.c | 27 ++------ drivers/iio/light/us5182d.c | 3 +- drivers/media/platform/qcom/venus/vdec.c | 10 ++- drivers/media/platform/qcom/venus/venc.c | 10 ++- drivers/pinctrl/qcom/pinctrl-lpass-lpi.c | 16 +++-- drivers/spi/spi-ti-qspi.c | 6 +- drivers/staging/greybus/power_supply.c | 8 +-- include/linux/greybus/bundle.h | 12 +--- include/linux/pm_runtime.h | 63 +++---------------- include/sound/hdaudio.h | 8 +-- sound/hda/core/device.c | 13 ++-- sound/soc/codecs/lpass-rx-macro.c | 4 +- sound/soc/codecs/lpass-tx-macro.c | 4 +- sound/soc/codecs/lpass-va-macro.c | 17 ++--- sound/soc/codecs/lpass-wsa-macro.c | 8 +-- sound/soc/intel/atom/sst/sst.h | 2 +- sound/soc/intel/atom/sst/sst_drv_interface.c | 4 +- sound/soc/intel/atom/sst/sst_pvt.c | 9 +-- sound/soc/sof/control.c | 6 +- sound/soc/sof/sof-client-ipc-flood-test.c | 5 +- .../soc/sof/sof-client-ipc-kernel-injector.c | 4 +- sound/soc/sof/sof-client-ipc-msg-injector.c | 6 +- sound/soc/sof/sof-client-probes.c | 18 ++---- 44 files changed, 152 insertions(+), 325 deletions(-)
Signature · 1 line
2.56.0.360.g66cac248cb-goog
PATCH 1/13 hwrng: cctrng: Discard pm_runtime_put_autosuspend() return value
Printing an error message on pm_runtime_put_autosuspend() returning a non-zero value is not particularly useful.
Returning an error code from pm_runtime_put_autosuspend() merely means that it has not queued up a timer or work item to check whether or not the device can be suspended and there are many perfectly valid situations in which that can happen, like after writing "on" to the device's runtime PM "control" attribute in sysfs for one example.
Accordingly, update cc_trng_pm_put_suspend() to simply discard the return value of pm_runtime_put_autosuspend().
This will facilitate a planned change of the pm_runtime_put_autosuspend() return type to void in the future, similar to commit 3afd8df02433 ("PM: runtime: Change pm_runtime_put() return type to void").
---
This patch is independent of the rest of the series, except for the end (changing the return type). I expect it can be applied by individual maintainers, and we pick up the end once the dust is settled.
drivers/char/hw_random/cctrng.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)
drivers/char/hw_random/cctrng.c +1 -5
@@ -96,11 +96,7 @@ static int cc_trng_pm_get(struct device *dev)static void cc_trng_pm_put_suspend (struct device *dev) { -int rc =0 ; - - rc =pm_runtime_put_autosuspend (dev); -if (rc) -dev_err (dev,"pm_runtime_put_autosuspend returned %x\n" , rc); +pm_runtime_put_autosuspend (dev); }static int cc_trng_pm_init (struct cctrng_drvdata *drvdata)
Signature · 1 line
2.56.0.360.g66cac248cb-goog
Herbert Xu
maintainerRe: [PATCH 01/13] hwrng: cctrng: Discard pm_runtime_put_autosuspend() return value
Quoted 17 lines · Brian Norris
Printing an error message on
pm_runtime_put_autosuspend()returning a non-zero value is not particularly useful.Returning an error code from
pm_runtime_put_autosuspend()merely means that it has not queued up a timer or work item to check whether or not the device can be suspended and there are many perfectly valid situations in which that can happen, like after writing "on" to the device's runtime PM "control" attribute in sysfs for one example.Accordingly, update
cc_trng_pm_put_suspend()to simply discard the return value ofpm_runtime_put_autosuspend().This will facilitate a planned change of the
pm_runtime_put_autosuspend()return type to void in the future, similar to commit3afd8df02433("PM: runtime: Changepm_runtime_put()return type to void").
Quoted 8 lines
Signed-off-by Brian Norris <[email protected]>---
This patch is independent of the rest of the series, except for the end (changing the return type). I expect it can be applied by individual maintainers, and we pick up the end once the dust is settled.drivers/char/hw_random/cctrng.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)
Patch applied. Thanks.
Signature · 3 lines
Email: Herbert Xu <[email protected]> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
PATCH 2/13 drm: Discard pm_runtime_put_autosuspend() return values
In reply to Brian NorrisSeveral DRM drivers use or propagate the pm_runtime_put_autosuspend() return value, which is not particularly useful.
Returning an error code from pm_runtime_put_autosuspend() merely means that it has not queued up a timer or work item to check whether or not the device can be suspended and there are many perfectly valid situations in which that can happen, like after writing "on" to the device's runtime PM "control" attribute in sysfs for one example.
For this reason, modify these drivers to simply discard the pm_runtime_put_autosuspend() return value, and change the return type of v3d_pm_runtime_put() (whose return value is never used) to void. Also drop the redundant pm_runtime_mark_last_busy() calls right before pm_runtime_put_autosuspend().
This will facilitate a planned change of the pm_runtime_put_autosuspend() return type to void in the future, similar to commit 3afd8df02433 ("PM: runtime: Change pm_runtime_put() return type to void").
---
This patch is independent of the rest of the series, except for the end (changing the return type). I expect it can be applied by individual maintainers, and we pick up the end once the dust is settled.
drivers/gpu/drm/panel/panel-simple.c | 8 +------- drivers/gpu/drm/tidss/tidss_drv.c | 7 +------ drivers/gpu/drm/v3d/v3d_drv.h | 4 ++-- 3 files changed, 4 insertions(+), 15 deletions(-)
drivers/gpu/drm/panel/panel-simple.c +1 -7
@@ -318,12 +318,7 @@ static int panel_simple_suspend(struct device *dev)static int panel_simple_unprepare (struct drm_panel *panel) { -int ret; - -pm_runtime_mark_last_busy (panel->dev); - ret =pm_runtime_put_autosuspend (panel->dev); -if (ret <0 ) -return ret; +pm_runtime_put_autosuspend (panel->dev);return 0 ; }
@@ -389,7 +384,6 @@ static int panel_simple_get_modes(struct drm_panel *panel, num +=drm_edid_connector_add_modes (connector); -pm_runtime_mark_last_busy (panel->dev);pm_runtime_put_autosuspend (panel->dev); }
drivers/gpu/drm/tidss/tidss_drv.c +1 -6
@@ -40,12 +40,7 @@ int tidss_runtime_get(struct tidss_device *tidss)void tidss_runtime_put (struct tidss_device *tidss) { -int r; - -pm_runtime_mark_last_busy (tidss->dev); - - r =pm_runtime_put_autosuspend (tidss->dev); -WARN_ON (r <0 ); +pm_runtime_put_autosuspend (tidss->dev); }static int __maybe_unused tidss_pm_runtime_suspend (struct device *dev)
drivers/gpu/drm/v3d/v3d_drv.h +2 -2
@@ -669,9 +669,9 @@ static __always_inline int v3d_pm_runtime_get(struct v3d_dev *v3d)return pm_runtime_resume_and_get (v3d->drm.dev); } -static __always_inline int v3d_pm_runtime_put(struct v3d_dev *v3d) +static __always_inline void v3d_pm_runtime_put(struct v3d_dev *v3d) { -return pm_runtime_put_autosuspend (v3d->drm.dev); +pm_runtime_put_autosuspend (v3d->drm.dev); }/* v3d_sched.c */
Signature · 1 line
2.56.0.360.g66cac248cb-goog
Re: [PATCH 02/13] drm: Discard pm_runtime_put_autosuspend() return values
Hi Brian,
Quoted 15 lines · Brian Norris
Several DRM drivers use or propagate the
pm_runtime_put_autosuspend()return value, which is not particularly useful.Returning an error code from
pm_runtime_put_autosuspend()merely means that it has not queued up a timer or work item to check whether or not the device can be suspended and there are many perfectly valid situations in which that can happen, like after writing "on" to the device's runtime PM "control" attribute in sysfs for one example.For this reason, modify these drivers to simply discard the
pm_runtime_put_autosuspend()return value, and change the return type ofv3d_pm_runtime_put()(whose return value is never used) to void. Also drop the redundantpm_runtime_mark_last_busy()calls right beforepm_runtime_put_autosuspend().
Quoted 6 lines
This will facilitate a planned change of the
pm_runtime_put_autosuspend()return type to void in the future, similar to commit3afd8df02433("PM: runtime: Changepm_runtime_put()return type to void").Signed-off-by Brian Norris <[email protected]>
For V3D,
Best regards,
- Maíra
Quoted 70 lines
---
This patch is independent of the rest of the series, except for the end (changing the return type). I expect it can be applied by individual maintainers, and we pick up the end once the dust is settled.drivers/gpu/drm/panel/panel-simple.c | 8 +------- drivers/gpu/drm/tidss/tidss_drv.c | 7 +------ drivers/gpu/drm/v3d/v3d_drv.h | 4 ++-- 3 files changed, 4 insertions(+), 15 deletions(-)drivers/gpu/drm/panel/panel-simple.c +1
-7@@ -318,12 +318,7 @@ static int panel_simple_suspend(struct device *dev)static int panel_simple_unprepare (struct drm_panel *panel) { -int ret; - -pm_runtime_mark_last_busy (panel->dev); - ret =pm_runtime_put_autosuspend (panel->dev); -if (ret <0 ) -return ret; +pm_runtime_put_autosuspend (panel->dev);return 0 ; }@@ -389,7 +384,6 @@ static int panel_simple_get_modes(struct drm_panel *panel, num +=drm_edid_connector_add_modes (connector); -pm_runtime_mark_last_busy (panel->dev);pm_runtime_put_autosuspend (panel->dev); }drivers/gpu/drm/tidss/tidss_drv.c +1
-6@@ -40,12 +40,7 @@ int tidss_runtime_get(struct tidss_device *tidss)void tidss_runtime_put (struct tidss_device *tidss) { -int r; - -pm_runtime_mark_last_busy (tidss->dev); - - r =pm_runtime_put_autosuspend (tidss->dev); -WARN_ON (r <0 ); +pm_runtime_put_autosuspend (tidss->dev); }static int __maybe_unused tidss_pm_runtime_suspend (struct device *dev)drivers/gpu/drm/v3d/v3d_drv.h +2
-2@@ -669,9 +669,9 @@ static __always_inline int v3d_pm_runtime_get(struct v3d_dev *v3d)return pm_runtime_resume_and_get (v3d->drm.dev); } -static __always_inline int v3d_pm_runtime_put(struct v3d_dev *v3d) +static __always_inline void v3d_pm_runtime_put(struct v3d_dev *v3d) { -return pm_runtime_put_autosuspend (v3d->drm.dev); +pm_runtime_put_autosuspend (v3d->drm.dev); }/* v3d_sched.c */
PATCH 3/13 iio: Discard pm_runtime_put_autosuspend() return values
In reply to Brian NorrisMultiple IIO drivers inspect or propagate the return value of pm_runtime_put_autosuspend(), which is not particularly useful.
Returning an error code from pm_runtime_put_autosuspend() merely means that it has not queued up a timer or work item to check whether or not the device can be suspended and there are many perfectly valid situations in which that can happen, like after writing "on" to the device's runtime PM "control" attribute in sysfs for one example.
For this reason, modify these drivers to simply discard the pm_runtime_put_autosuspend() return value, and change the return types of fxls8962af_power_off() and fxas21002c_pm_put() to void.
This will facilitate a planned change of the pm_runtime_put_autosuspend() return type to void in the future, similar to commit 3afd8df02433 ("PM: runtime: Change pm_runtime_put() return type to void").
---
This patch is independent of the rest of the series, except for the end (changing the return type). I expect it can be applied by individual maintainers, and we pick up the end once the dust is settled.
I couldn't decide if I should split this up by individual driver, or just 1 for the whole drivers/iio/ subsystem. Many of these drivers have borrowed the same patterns, so I chose to just send 1. Let me know if I should split it up.
drivers/iio/accel/bmc150-accel-core.c | 14 +++++----- drivers/iio/accel/fxls8962af-core.c | 11 +++----- drivers/iio/accel/kxcjk-1013.c | 15 ++++++----- drivers/iio/accel/mma8452.c | 15 +++++------ drivers/iio/accel/mma9551_core.c | 15 +++++------ drivers/iio/adc/rcar-gyroadc.c | 4 +-- drivers/iio/adc/ti-ads1015.c | 8 +++--- drivers/iio/chemical/atlas-sensor.c | 4 +-- .../common/hid-sensors/hid-sensor-trigger.c | 14 ++++------ drivers/iio/gyro/bmg160_core.c | 17 +++++------- drivers/iio/gyro/fxas21002c_core.c | 12 +++------ drivers/iio/imu/kmx61.c | 17 ++++++------ drivers/iio/light/apds9960.c | 2 +- drivers/iio/light/isl29028.c | 15 +++-------- drivers/iio/light/pa12203001.c | 3 ++- drivers/iio/light/rpr0521.c | 15 +++++------ drivers/iio/light/tsl2583.c | 27 +++---------------- drivers/iio/light/us5182d.c | 3 ++- 18 files changed, 80 insertions(+), 131 deletions(-)
drivers/iio/accel/bmc150-accel-core.c +7 -7
@@ -332,14 +332,14 @@ static int bmc150_accel_set_power_state(struct bmc150_accel_data *data, bool on)struct device *dev =regmap_get_device (data->regmap);int ret; -if (on) +if (on) { ret =pm_runtime_resume_and_get (dev); -else - ret =pm_runtime_put_autosuspend (dev); -if (ret <0 ) { -dev_err (dev, -"Failed: %s for %d\n" , __func__, on); -return ret; +if (ret <0 ) { +dev_err (dev,"%s: failed to resume: %d\n" , __func__, ret); +return ret; + } + }else { +pm_runtime_put_autosuspend (dev); }return 0 ;
drivers/iio/accel/fxls8962af-core.c +3 -8
@@ -216,16 +216,11 @@ static int fxls8962af_power_on(struct fxls8962af_data *data)return ret; } -static int fxls8962af_power_off (struct fxls8962af_data *data) +static void fxls8962af_power_off (struct fxls8962af_data *data) {struct device *dev =regmap_get_device (data->regmap); -int ret; - - ret =pm_runtime_put_autosuspend (dev); -if (ret) -dev_err (dev,"failed to power off\n" ); -return ret; +pm_runtime_put_autosuspend (dev); }static int fxls8962af_standby (struct fxls8962af_data *data)
@@ -692,7 +687,7 @@ fxls8962af_write_event_config(struct iio_dev *indio_dev,return -EBUSY ;/* Not in buffered mode so disable power */ - ret =fxls8962af_power_off (data); +fxls8962af_power_off (data);iio_device_release_direct (indio_dev); }
drivers/iio/accel/kxcjk-1013.c +8 -7
@@ -633,14 +633,15 @@ static int kxcjk1013_set_power_state(struct kxcjk1013_data *data, bool on)#ifdef CONFIG_PM int ret; -if (on) +if (on) { ret =pm_runtime_resume_and_get (&data->client->dev); -else - ret =pm_runtime_put_autosuspend (&data->client->dev); -if (ret <0 ) { -dev_err (&data->client->dev, -"Failed: %s for %d\n" , __func__, on); -return ret; +if (ret <0 ) { +dev_err (&data->client->dev, +"%s: failed to resume: %d\n" , __func__, ret); +return ret; + } + }else { +pm_runtime_put_autosuspend (&data->client->dev); }#endif
drivers/iio/accel/mma8452.c +7 -8
@@ -224,15 +224,14 @@ static int mma8452_set_runtime_pm_state(struct i2c_client *client, bool on)#ifdef CONFIG_PM int ret; -if (on) +if (on) { ret =pm_runtime_resume_and_get (&client->dev); -else - ret =pm_runtime_put_autosuspend (&client->dev); -if (ret <0 ) { -dev_err (&client->dev, -"failed to change power state to %d\n" , on); - -return ret; +if (ret <0 ) { +dev_err (&client->dev,"failed to resume: %d\n" , ret); +return ret; + } + }else { +pm_runtime_put_autosuspend (&client->dev); }#endif
drivers/iio/accel/mma9551_core.c +7 -8
@@ -669,15 +669,14 @@ int mma9551_set_power_state(struct i2c_client *client, bool on)#ifdef CONFIG_PM int ret; -if (on) +if (on) { ret =pm_runtime_resume_and_get (&client->dev); -else - ret =pm_runtime_put_autosuspend (&client->dev); -if (ret <0 ) { -dev_err (&client->dev, -"failed to change power state to %d\n" , on); - -return ret; +if (ret <0 ) { +dev_err (&client->dev,"failed to resume: %d\n" , ret); +return ret; + } + }else { +pm_runtime_put_autosuspend (&client->dev); }#endif
drivers/iio/adc/rcar-gyroadc.c +1 -3
@@ -199,10 +199,8 @@ static int rcar_gyroadc_read_raw(struct iio_dev *indio_dev, *val =readl (priv->regs + datareg); *val &=BIT (priv->sample_width) -1 ; - ret =pm_runtime_put_autosuspend (priv->dev); +pm_runtime_put_autosuspend (priv->dev);iio_device_release_direct (indio_dev); -if (ret <0 ) -return ret;return IIO_VAL_INT ;case IIO_CHAN_INFO_SCALE :
drivers/iio/adc/ti-ads1015.c +3 -5
@@ -370,15 +370,13 @@ static const struct iio_chan_spec tla2024_channels[] = {#ifdef CONFIG_PM static int ads1015_set_power_state (struct ads1015_data *data,bool on) { -int ret;struct device *dev =regmap_get_device (data->regmap);if (on) - ret =pm_runtime_resume_and_get (dev); -else - ret =pm_runtime_put_autosuspend (dev); +return pm_runtime_resume_and_get (dev); -return ret <0 ? ret :0 ; +pm_runtime_put_autosuspend (dev); +return 0 ; }#else /* !CONFIG_PM */
drivers/iio/chemical/atlas-sensor.c +1 -3
@@ -426,9 +426,7 @@ static int atlas_buffer_predisable(struct iio_dev *indio_dev)if (ret)return ret; - ret =pm_runtime_put_autosuspend (&data->client->dev); -if (ret) -return ret; +pm_runtime_put_autosuspend (&data->client->dev);return 0 ; }
drivers/iio/common/hid-sensors/hid-sensor-trigger.c +5 -9
@@ -154,21 +154,17 @@ int hid_sensor_power_state(struct hid_sensor_common *st, bool state) {#ifdef CONFIG_PM -int ret; -if (atomic_add_unless (&st->runtime_pm_enable,1 ,1 ))pm_runtime_enable (&st->pdev->dev);if (state) {atomic_inc (&st->user_requested_state); - ret =pm_runtime_resume_and_get (&st->pdev->dev); - }else { -atomic_dec (&st->user_requested_state); -pm_runtime_use_autosuspend (&st->pdev->dev); - ret =pm_runtime_put_autosuspend (&st->pdev->dev); +return pm_runtime_resume_and_get (&st->pdev->dev); } -if (ret <0 ) -return ret; + +atomic_dec (&st->user_requested_state); +pm_runtime_use_autosuspend (&st->pdev->dev); +pm_runtime_put_autosuspend (&st->pdev->dev);return 0 ;#else
drivers/iio/gyro/bmg160_core.c +7 -10
@@ -319,18 +319,15 @@ static int bmg160_set_power_state(struct bmg160_data *data, bool on)struct device *dev =regmap_get_device (data->regmap);int ret; -if (on) +if (on) { ret =pm_runtime_get_sync (dev); -else - ret =pm_runtime_put_autosuspend (dev); - -if (ret <0 ) { -dev_err (dev,"Failed: bmg160_set_power_state for %d\n" , on); - -if (on) +if (ret <0 ) { +dev_err (dev,"%s: failed to resume: %d\n" , __func__, ret);pm_runtime_put_noidle (dev); - -return ret; +return ret; + } + }else { +pm_runtime_put_autosuspend (dev); }#endif
drivers/iio/gyro/fxas21002c_core.c +4 -8
@@ -369,11 +369,11 @@ static int fxas21002c_pm_get(struct fxas21002c_data *data)return pm_runtime_resume_and_get (regmap_get_device (data->regmap)); } -static int fxas21002c_pm_put (struct fxas21002c_data *data) +static void fxas21002c_pm_put (struct fxas21002c_data *data) {struct device *dev =regmap_get_device (data->regmap); -return pm_runtime_put_autosuspend (dev); +pm_runtime_put_autosuspend (dev); }static int fxas21002c_temp_get (struct fxas21002c_data *data,int *val)
@@ -396,9 +396,7 @@ static int fxas21002c_temp_get(struct fxas21002c_data *data, int *val) *val =sign_extend32 (temp,7 ); - ret =fxas21002c_pm_put (data); -if (ret <0 ) - goto data_unlock; +fxas21002c_pm_put (data); ret =IIO_VAL_INT ;
@@ -430,9 +428,7 @@ static int fxas21002c_axis_get(struct fxas21002c_data *data, *val =sign_extend32 (be16_to_cpu (axis_be),15 ); - ret =fxas21002c_pm_put (data); -if (ret <0 ) - goto data_unlock; +fxas21002c_pm_put (data); ret =IIO_VAL_INT ;
drivers/iio/imu/kmx61.c +8 -9
@@ -747,16 +747,15 @@ static int kmx61_set_power_state(struct kmx61_data *data, bool on, u8 device) data->mag_ps = on; } -if (on) +if (on) { ret =pm_runtime_resume_and_get (&data->client->dev); -else - ret =pm_runtime_put_autosuspend (&data->client->dev); -if (ret <0 ) { -dev_err (&data->client->dev, -"Failed: kmx61_set_power_state for %d, ret %d\n" , - on, ret); - -return ret; +if (ret <0 ) { +dev_err (&data->client->dev, +"%s: failed to resume: %d\n" , __func__, ret); +return ret; + } + }else { +pm_runtime_put_autosuspend (&data->client->dev); }#endif return 0 ;
drivers/iio/light/apds9960.c +1 -1
@@ -495,7 +495,7 @@ static int apds9960_set_power_state(struct apds9960_data *data, bool on)usleep_range (data->als_adc_int_us,APDS9960_MAX_INT_TIME_IN_US ); }else { - ret =pm_runtime_put_autosuspend (dev); +pm_runtime_put_autosuspend (dev); }mutex_unlock (&data->lock);
drivers/iio/light/isl29028.c +3 -12
@@ -395,9 +395,7 @@ static int isl29028_write_raw(struct iio_dev *indio_dev,if (ret <0 )return ret; - ret =pm_runtime_put_autosuspend (dev); -if (ret <0 ) -return ret; +pm_runtime_put_autosuspend (dev);return 0 ; }
@@ -408,7 +406,7 @@ static int isl29028_read_raw(struct iio_dev *indio_dev, {struct isl29028_chip *chip =iio_priv (indio_dev);struct device *dev =regmap_get_device (chip->regmap); -int ret, pm_ret; +int ret; ret =pm_runtime_resume_and_get (dev);if (ret <0 )
@@ -464,14 +462,7 @@ static int isl29028_read_raw(struct iio_dev *indio_dev,if (ret <0 )return ret; -/** -* Preserve the ret variable if the call to -* pm_runtime_put_autosuspend() is successful so the reading -* (if applicable) is returned to user space. -*/ - pm_ret =pm_runtime_put_autosuspend (dev); -if (pm_ret <0 ) -return pm_ret; +pm_runtime_put_autosuspend (dev);return ret; }
drivers/iio/light/pa12203001.c +2 -1
@@ -188,7 +188,8 @@ static int pa12203001_set_power_state(struct pa12203001_data *data, bool on,if (on)return pm_runtime_resume_and_get (&data->client->dev); -return pm_runtime_put_autosuspend (&data->client->dev); +pm_runtime_put_autosuspend (&data->client->dev); +return 0 ; err:mutex_unlock (&data->lock);
drivers/iio/light/rpr0521.c +7 -8
@@ -357,15 +357,14 @@ static int rpr0521_set_power_state(struct rpr0521_data *data, bool on,* Note: If either measurement is re-enabled before _suspend(), * both stay enabled until _suspend(). */ -if (on) +if (on) { ret =pm_runtime_resume_and_get (&data->client->dev); -else - ret =pm_runtime_put_autosuspend (&data->client->dev); -if (ret <0 ) { -dev_err (&data->client->dev, -"Failed: rpr0521_set_power_state for %d, ret %d\n" , - on, ret); -return ret; +if (ret <0 ) { +dev_err (&data->client->dev,"Failed to resume: %d\n" , ret); +return ret; + } + }else { +pm_runtime_put_autosuspend (&data->client->dev); }if (on) {
drivers/iio/light/tsl2583.c +4 -23
@@ -640,7 +640,7 @@ static int tsl2583_read_raw(struct iio_dev *indio_dev, int *val, int *val2, long mask) { struct tsl2583_chip *chip = iio_priv(indio_dev); -int ret, pm_ret; +int ret; ret =pm_runtime_resume_and_get (&chip->client->dev);if (ret <0 )
@@ -709,19 +709,7 @@ static int tsl2583_read_raw(struct iio_dev *indio_dev, read_done:mutex_unlock (&chip->als_mutex); -if (ret <0 ) { -pm_runtime_put_autosuspend (&chip->client->dev); -return ret; - } - -/* -* Preserve the ret variable if the call to -* pm_runtime_put_autosuspend() is successful so the reading -* (if applicable) is returned to user space. -*/ - pm_ret =pm_runtime_put_autosuspend (&chip->client->dev); -if (pm_ret <0 ) -return pm_ret; +pm_runtime_put_autosuspend (&chip->client->dev);return ret; }
@@ -773,16 +761,9 @@ static int tsl2583_write_raw(struct iio_dev *indio_dev,mutex_unlock (&chip->als_mutex); -if (ret <0 ) { -pm_runtime_put_autosuspend (&chip->client->dev); -return ret; - } +pm_runtime_put_autosuspend (&chip->client->dev); - ret =pm_runtime_put_autosuspend (&chip->client->dev); -if (ret <0 ) -return ret; - -return 0 ; +return ret; }static const struct iio_info tsl2583_info = {
drivers/iio/light/us5182d.c +2 -1
@@ -366,7 +366,8 @@ static int us5182d_set_power_state(struct us5182d_data *data, bool on)if (on)return pm_runtime_resume_and_get (&data->client->dev); -return pm_runtime_put_autosuspend (&data->client->dev); +pm_runtime_put_autosuspend (&data->client->dev); +return 0 ; } static int us5182d_read_value(struct us5182d_data *data,
Signature · 1 line
2.56.0.360.g66cac248cb-goog
Re: [PATCH 03/13] iio: Discard pm_runtime_put_autosuspend() return values
Quoted 9 lines · Brian Norris
Multiple IIO drivers inspect or propagate the return value of
pm_runtime_put_autosuspend(), which is not particularly useful.Returning an error code from
pm_runtime_put_autosuspend()merely means that it has not queued up a timer or work item to check whether or not the device can be suspended and there are many perfectly valid situations in which that can happen, like after writing "on" to the device's runtime PM "control" attribute in sysfs for one example.
Quoted 8 lines
For this reason, modify these drivers to simply discard the
pm_runtime_put_autosuspend()return value, and change the return types offxls8962af_power_off()andfxas21002c_pm_put()to void.This will facilitate a planned change of the
pm_runtime_put_autosuspend()return type to void in the future, similar to commit3afd8df02433("PM: runtime: Changepm_runtime_put()return type to void").
but see below.
Quoted 10 lines
---
This patch is independent of the rest of the series, except for the end (changing the return type). I expect it can be applied by individual maintainers, and we pick up the end once the dust is settled.I couldn't decide if I should split this up by individual driver, or just 1 for the whole
drivers/iio/subsystem. Many of these drivers have borrowed the same patterns, so I chose to just send 1. Let me know if I should split it up.
Quoted 5 lines
drivers/iio/accel/bmc150-accel-core.c | 14 +++++----- drivers/iio/accel/fxls8962af-core.c | 11 +++----- drivers/iio/accel/kxcjk-1013.c | 15 ++++++----- drivers/iio/accel/mma8452.c | 15 +++++------ drivers/iio/accel/mma9551_core.c | 15 +++++------
One patch (iio: accel: ...)
Quoted 2 lines
drivers/iio/adc/rcar-gyroadc.c | 4 +-- drivers/iio/adc/ti-ads1015.c | 8 +++---
Second patch (iio: adc: ...)
Quoted 1 line
drivers/iio/chemical/atlas-sensor.c | 4 +--
Third patch (iio: chemical: atlas-sensor: ...)
Quoted 1 line
.../common/hid-sensors/hid-sensor-trigger.c | 14 ++++------
...and so on...
Quoted 8 lines
drivers/iio/gyro/bmg160_core.c | 17 +++++------- drivers/iio/gyro/fxas21002c_core.c | 12 +++------drivers/iio/imu/kmx61.c | 17 ++++++------
Quoted 6 lines
drivers/iio/light/apds9960.c | 2 +- drivers/iio/light/isl29028.c | 15 +++-------- drivers/iio/light/pa12203001.c | 3 ++- drivers/iio/light/rpr0521.c | 15 +++++------ drivers/iio/light/tsl2583.c | 27 +++---------------- drivers/iio/light/us5182d.c | 3 ++-
So, I get 7 patches.
Signature · 2 lines
With Best Regards, Andy Shevchenko
Jonathan Cameron
maintainerRe: [PATCH 03/13] iio: Discard pm_runtime_put_autosuspend() return values
In reply to Brian NorrisQuoted 24 lines · On Tue, 6 Oct 2026 16:13:31 -0700 Brian Norris <[email protected]>…
Multiple IIO drivers inspect or propagate the return value of
pm_runtime_put_autosuspend(), which is not particularly useful.Returning an error code from
pm_runtime_put_autosuspend()merely means that it has not queued up a timer or work item to check whether or not the device can be suspended and there are many perfectly valid situations in which that can happen, like after writing "on" to the device's runtime PM "control" attribute in sysfs for one example.For this reason, modify these drivers to simply discard the
pm_runtime_put_autosuspend()return value, and change the return types offxls8962af_power_off()andfxas21002c_pm_put()to void.This will facilitate a planned change of the
pm_runtime_put_autosuspend()return type to void in the future, similar to commit3afd8df02433("PM: runtime: Changepm_runtime_put()return type to void").Signed-off-by Brian Norris <[email protected]>---
This patch is independent of the rest of the series, except for the end (changing the return type). I expect it can be applied by individual maintainers, and we pick up the end once the dust is settled.
Quoted 4 lines
I couldn't decide if I should split this up by individual driver, or just 1 for the whole
drivers/iio/subsystem. Many of these drivers have borrowed the same patterns, so I chose to just send 1. Let me know if I should split it up.
Hi Brian,
Great that you are doing this, but sadly it opens the can of worms that are the helpers a lot of these drivers have. They just about made sense before various recent runtime_pm useability improvements. Now they make very little sense at all. Dropping the return value here also removes the reason for a bunch of odd defensive ifdefs.
As such, I'm requesting some slightly more involved changes. Some of them I think are coupled tightly to what you are doing here so should be in this series, others can be left for later.
With that in mind, I think the changes will become a little more complex and so we probably end up with one patch per driver.
Jonathan
p.s. I'm feeling a bit mean asking for so many additional changes, so if you don't have time say that and I'll see if I can find some time (or someone else!) to do the extra work.
Quoted 26 lines
drivers/iio/accel/bmc150-accel-core.c | 14 +++++----- drivers/iio/accel/fxls8962af-core.c | 11 +++----- drivers/iio/accel/kxcjk-1013.c | 15 ++++++----- drivers/iio/accel/mma8452.c | 15 +++++------ drivers/iio/accel/mma9551_core.c | 15 +++++------ drivers/iio/adc/rcar-gyroadc.c | 4 +-- drivers/iio/adc/ti-ads1015.c | 8 +++--- drivers/iio/chemical/atlas-sensor.c | 4 +-- .../common/hid-sensors/hid-sensor-trigger.c | 14 ++++------ drivers/iio/gyro/bmg160_core.c | 17 +++++------- drivers/iio/gyro/fxas21002c_core.c | 12 +++------ drivers/iio/imu/kmx61.c | 17 ++++++------ drivers/iio/light/apds9960.c | 2 +- drivers/iio/light/isl29028.c | 15 +++-------- drivers/iio/light/pa12203001.c | 3 ++- drivers/iio/light/rpr0521.c | 15 +++++------ drivers/iio/light/tsl2583.c | 27 +++---------------- drivers/iio/light/us5182d.c | 3 ++- 18 files changed, 80 insertions(+), 131 deletions(-)drivers/iio/accel/bmc150-accel-core.c +0
-0
drivers/iio/accel/bmc150-accel-core.c static int bmc150_accel_set_power_state(struct bmc150_accel_data *data, bool on) · line 332
Quoted 1 line
drivers/iio/accel/bmc150-accel-core.c +0
-0@@ -332,14 +332,14 @@ static int bmc150_accel_set_power_state(struct bmc150_accel_data *data, bool on)struct device *dev =regmap_get_device (data->regmap);
This driver could do with modernizing to use the stuff that lets the compiler drop unused pm functions. That is a bit of a bigger change though so entirely optional wrt to this set.
Quoted 25 lines
drivers/iio/accel/bmc150-accel-core.c +7
-7int ret; -if (on) +if (on) { ret =pm_runtime_resume_and_get (dev); -else - ret =pm_runtime_put_autosuspend (dev); -if (ret <0 ) { -dev_err (dev, -"Failed: %s for %d\n" , __func__, on); -return ret; +if (ret <0 ) { +dev_err (dev,"%s: failed to resume: %d\n" , __func__, ret); +return ret; + } + }else { +pm_runtime_put_autosuspend (dev); }return 0 ;drivers/iio/accel/fxls8962af-core.c +0
-0
drivers/iio/accel/fxls8962af-core.c static int fxls8962af_power_on(struct fxls8962af_data *data) · line 219
Quoted 5 lines
drivers/iio/accel/fxls8962af-core.c +1
-1@@ -216,16 +216,11 @@ static int fxls8962af_power_on(struct fxls8962af_data *data)return ret; } -static int fxls8962af_power_off (struct fxls8962af_data *data) +static void fxls8962af_power_off (struct fxls8962af_data *data)
This helper is completely pointless after you cleanup. Please drop it. The _on() one just exists to print an error. So ideally drop that one as well.
drivers/iio/accel/kxcjk-1013.c static int kxcjk1013_set_power_state(struct kxcjk1013_data *data, bool on)
Quoted 27 lines
drivers/iio/accel/fxls8962af-core.c +2
-7{struct device *dev =regmap_get_device (data->regmap); -int ret; - - ret =pm_runtime_put_autosuspend (dev); -if (ret) -dev_err (dev,"failed to power off\n" ); -return ret; +pm_runtime_put_autosuspend (dev); }static int fxls8962af_standby (struct fxls8962af_data *data)@@ -692,7 +687,7 @@ fxls8962af_write_event_config(struct iio_dev *indio_dev,return -EBUSY ;/* Not in buffered mode so disable power */ - ret =fxls8962af_power_off (data); +fxls8962af_power_off (data);iio_device_release_direct (indio_dev); }drivers/iio/accel/kxcjk-1013.c +0
-0@@ -633,14 +633,15 @@ static int kxcjk1013_set_power_state(struct kxcjk1013_data *data, bool on)
Same as the one below.
drivers/iio/accel/mma8452.c static int mma8452_set_runtime_pm_state(struct i2c_client *client, bool on)
Quoted 27 lines
drivers/iio/accel/kxcjk-1013.c +8
-7#ifdef CONFIG_PM int ret; -if (on) +if (on) { ret =pm_runtime_resume_and_get (&data->client->dev); -else - ret =pm_runtime_put_autosuspend (&data->client->dev); -if (ret <0 ) { -dev_err (&data->client->dev, -"Failed: %s for %d\n" , __func__, on); -return ret; +if (ret <0 ) { +dev_err (&data->client->dev, +"%s: failed to resume: %d\n" , __func__, ret); +return ret; + } + }else { +pm_runtime_put_autosuspend (&data->client->dev); }#endif drivers/iio/accel/mma8452.c +0
-0@@ -224,15 +224,14 @@ static int mma8452_set_runtime_pm_state(struct i2c_client *client, bool on)
As with so many of the others. This is now a pointless function. The reason for the ifdef is gone I believe after this patch so dropping that would be minimum. Better would be to get rid of the helper and just make the runtime pm calls directly wherever this was called.
Quoted 27 lines
drivers/iio/accel/mma8452.c +7
-8#ifdef CONFIG_PM int ret; -if (on) +if (on) { ret =pm_runtime_resume_and_get (&client->dev); -else - ret =pm_runtime_put_autosuspend (&client->dev); -if (ret <0 ) { -dev_err (&client->dev, -"failed to change power state to %d\n" , on); - -return ret; +if (ret <0 ) { +dev_err (&client->dev,"failed to resume: %d\n" , ret); +return ret; + } + }else { +pm_runtime_put_autosuspend (&client->dev); }#endif drivers/iio/accel/mma9551_core.c +0
-0
drivers/iio/accel/mma9551_core.c int mma9551_set_power_state(struct i2c_client *client, bool on) · line 670
Quoted 2 lines
drivers/iio/accel/mma9551_core.c +0
-0@@ -669,15 +669,14 @@ int mma9551_set_power_state(struct i2c_client *client, bool on)#ifdef CONFIG_PM int ret;
Another one where I think the ifdef was for the return value of stubbed pm_runtime_put_autosuspend. So we can drop that.
Also the function no longer has any real purpose - so if you don't mind going a little further, dropping this helper and calling the pm_runtime functions inline would be much nicer.
Quoted 34 lines
drivers/iio/accel/mma9551_core.c +7
-8-if (on) +if (on) { ret =pm_runtime_resume_and_get (&client->dev); -else - ret =pm_runtime_put_autosuspend (&client->dev); -if (ret <0 ) { -dev_err (&client->dev, -"failed to change power state to %d\n" , on); - -return ret; +if (ret <0 ) { +dev_err (&client->dev,"failed to resume: %d\n" , ret); +return ret; + } + }else { +pm_runtime_put_autosuspend (&client->dev); }#endif drivers/iio/adc/ti-ads1015.c +0
-2@@ -370,15 +370,13 @@ static const struct iio_chan_spec tla2024_channels[] = {#ifdef CONFIG_PM static int ads1015_set_power_state (struct ads1015_data *data,bool on) { -int ret;struct device *dev =regmap_get_device (data->regmap);if (on) - ret =pm_runtime_resume_and_get (dev);
drivers/iio/adc/ti-ads1015.c static const struct iio_chan_spec tla2024_channels[] = { · line 382
Quoted 10 lines
drivers/iio/adc/ti-ads1015.c +3
-3- else - ret =pm_runtime_put_autosuspend (dev); +return pm_runtime_resume_and_get (dev); - return ret < 0 ? ret : 0; +pm_runtime_put_autosuspend (dev); + return 0; }#else /* !CONFIG_PM */
I think the ifdef stuff here was to paper over the return value you are removing. Should be able to drop it now.
drivers/iio/gyro/bmg160_core.c static int bmg160_set_power_state(struct bmg160_data *data, bool on) · line 323
Quoted 11 lines
drivers/iio/gyro/bmg160_core.c +1
-1@@ -319,18 +319,15 @@ static int bmg160_set_power_state(struct bmg160_data *data, bool on)struct device *dev =regmap_get_device (data->regmap);int ret; -if (on) +if (on) { ret =pm_runtime_get_sync (dev);
gah. Please just change this to use pm_runtime_resume_and_get() whilst you are here.
Ideally squash the helper in favour of direct calls to the runtime pm. Yet another pace that the recent improvements in runtime pm useability have made what was of minor benefit entirely pointless.
The ifdef can go away as well as I think the only thing that helped with was the return value of the put you are removing.
Quoted 24 lines
drivers/iio/gyro/bmg160_core.c +6
-9- else - ret =pm_runtime_put_autosuspend (dev); - -if (ret <0 ) { -dev_err (dev,"Failed: bmg160_set_power_state for %d\n" , on); - -if (on) +if (ret <0 ) { +dev_err (dev,"%s: failed to resume: %d\n" , __func__, ret);pm_runtime_put_noidle (dev); - -return ret; +return ret; + } + }else { +pm_runtime_put_autosuspend (dev); }#endif drivers/iio/gyro/fxas21002c_core.c +0
-0
drivers/iio/gyro/fxas21002c_core.c static int fxas21002c_pm_get(struct fxas21002c_data *data) · line 372
Quoted 5 lines
drivers/iio/gyro/fxas21002c_core.c +1
-1@@ -369,11 +369,11 @@ static int fxas21002c_pm_get(struct fxas21002c_data *data)return pm_runtime_resume_and_get (regmap_get_device (data->regmap)); } -static int fxas21002c_pm_put (struct fxas21002c_data *data) +static void fxas21002c_pm_put (struct fxas21002c_data *data)
The two runtime pm helpers in here have evolved to become a terrible idea.
Can we just drop them as part of your change and make the runtime pm calls directly instead? The dev is always available at the call sites so this should be trivial.
drivers/iio/light/apds9960.c
Quoted 35 lines
drivers/iio/gyro/fxas21002c_core.c +3
-7{struct device *dev =regmap_get_device (data->regmap); -return pm_runtime_put_autosuspend (dev); +pm_runtime_put_autosuspend (dev); }static int fxas21002c_temp_get (struct fxas21002c_data *data,int *val)@@ -396,9 +396,7 @@ static int fxas21002c_temp_get(struct fxas21002c_data *data, int *val) *val =sign_extend32 (temp,7 ); - ret =fxas21002c_pm_put (data); -if (ret <0 ) - goto data_unlock; +fxas21002c_pm_put (data); ret =IIO_VAL_INT ;@@ -430,9 +428,7 @@ static int fxas21002c_axis_get(struct fxas21002c_data *data, *val =sign_extend32 (be16_to_cpu (axis_be),15 ); - ret =fxas21002c_pm_put (data); -if (ret <0 ) - goto data_unlock; +fxas21002c_pm_put (data); ret =IIO_VAL_INT ;drivers/iio/light/apds9960.c +0
-0
Not needed for this series but...
Another one that has become rather pointless and should just be split into separate functions for enable and disable (where disable one is just pm_runtime_put_autosuspend)
Quoted 25 lines
drivers/iio/light/apds9960.c +1
-1@@ -495,7 +495,7 @@ static int apds9960_set_power_state(struct apds9960_data *data, bool on)usleep_range (data->als_adc_int_us,APDS9960_MAX_INT_TIME_IN_US ); }else { - ret =pm_runtime_put_autosuspend (dev); +pm_runtime_put_autosuspend (dev); }mutex_unlock (&data->lock);drivers/iio/light/isl29028.c +1
-3@@ -395,9 +395,7 @@ static int isl29028_write_raw(struct iio_dev *indio_dev,if (ret <0 )return ret; - ret =pm_runtime_put_autosuspend (dev); -if (ret <0 ) -return ret; +pm_runtime_put_autosuspend (dev);return 0 ; }
drivers/iio/light/isl29028.c static int isl29028_read_raw(struct iio_dev *indio_dev, · line 409
Quoted 5 lines
drivers/iio/light/isl29028.c +1
-1@@ -408,7 +406,7 @@ static int isl29028_read_raw(struct iio_dev *indio_dev, {struct isl29028_chip *chip =iio_priv (indio_dev);struct device *dev =regmap_get_device (chip->regmap); -int ret, pm_ret; +int ret;
Not one for this series (the patch for this LGTM),
but if anyone is reading and bored, cleanup.h magic would simplify this code nicely.
Quoted 24 lines
drivers/iio/light/isl29028.c +1
-8ret =pm_runtime_resume_and_get (dev);if (ret <0 )@@ -464,14 +462,7 @@ static int isl29028_read_raw(struct iio_dev *indio_dev,if (ret <0 )return ret; -/** -* Preserve the ret variable if the call to -* pm_runtime_put_autosuspend() is successful so the reading -* (if applicable) is returned to user space. -*/ - pm_ret =pm_runtime_put_autosuspend (dev); -if (pm_ret <0 ) -return pm_ret; +pm_runtime_put_autosuspend (dev);return ret; }drivers/iio/light/pa12203001.c +0
-0
drivers/iio/light/pa12203001.c static int pa12203001_set_power_state(struct pa12203001_data *data, bool on, · line 192
Quoted 6 lines
drivers/iio/light/pa12203001.c +2
-1@@ -188,7 +188,8 @@ static int pa12203001_set_power_state(struct pa12203001_data *data, bool on,if (on)return pm_runtime_resume_and_get (&data->client->dev); -return pm_runtime_put_autosuspend (&data->client->dev); +pm_runtime_put_autosuspend (&data->client->dev); +return 0 ;
This function has evolved to the point of silliness.
Given how minor your change is, feel free to ignore the following however!
There is zero shared code in here between the on and off paths so rename the function pa12203001_runtime_enable() and cover only the 'on' case. Then just put the pm_runtime_put_autosuspend() in wherever on was previously false.
Given that presumably one of the things is being turned on whenever we enable it, also pull the mutex out of the if blocks and use guard() to simplify that.
drivers/iio/light/rpr0521.c static int rpr0521_set_power_state(struct rpr0521_data *data, bool on, · line 358
Quoted 10 lines
drivers/iio/light/pa12203001.c +0
-0err:mutex_unlock (&data->lock);drivers/iio/light/rpr0521.c +0
-0@@ -357,15 +357,14 @@ static int rpr0521_set_power_state(struct rpr0521_data *data, bool on,* Note: If either measurement is re-enabled before _suspend(), * both stay enabled until _suspend().
Not asking for it in your series as beyond the scope, but this function should just be split in two. Almost no code is shared between the get and put paths.
However...
Quoted 10 lines
drivers/iio/light/rpr0521.c +1
-7*/ -if (on) +if (on) { ret =pm_runtime_resume_and_get (&data->client->dev); -else - ret =pm_runtime_put_autosuspend (&data->client->dev); -if (ret <0 ) { -dev_err (&data->client->dev, -"Failed: rpr0521_set_power_state for %d, ret %d\n" , - on, ret);
drivers/iio/light/rpr0521.c static int rpr0521_set_power_state(struct rpr0521_data *data, bool on, · line 370
Quoted 10 lines
drivers/iio/light/rpr0521.c +6
-1-return ret; +if (ret <0 ) { +dev_err (&data->client->dev,"Failed to resume: %d\n" , ret); +return ret; + } + }else { +pm_runtime_put_autosuspend (&data->client->dev); }if (on) {
This no longer makes any sense so maybe it is better to just split the code as part of your change set or at minimum change to
if (!on) {
pm_runtime_put_autosuspend(&data->client->dev);
return 0;
}
ret = pm_runtime_resume_and_get(&data->client->dev);
if (ret < 0) {
dev_err(&data->client->dev, "Failed to resume: %d\n", ret);
return ret;
}
/* If _resume() was not called, enable measurement now. */
if (data->als_ps_need_en) {
ret = rpr0521_als_enable(data, RPR0521_MODE_ALS_ENABLE);
if (ret)
return ret;
data->als_ps_need_en = false;
}
if (data->pxs_ps_need_en) {
ret = rpr0521_pxs_enable(data, RPR0521_MODE_PXS_ENABLE);
if (ret)
return ret;
data->pxs_ps_need_en = false;
}
#endif //mess that makes sense with more context!
return 0;
}Quoted 40 lines
drivers/iio/light/tsl2583.c +2
-16@@ -640,7 +640,7 @@ static int tsl2583_read_raw(struct iio_dev *indio_dev, int *val, int *val2, long mask) { struct tsl2583_chip *chip = iio_priv(indio_dev); -int ret, pm_ret; +int ret; ret =pm_runtime_resume_and_get (&chip->client->dev);if (ret <0 )@@ -709,19 +709,7 @@ static int tsl2583_read_raw(struct iio_dev *indio_dev, read_done:mutex_unlock (&chip->als_mutex); -if (ret <0 ) { -pm_runtime_put_autosuspend (&chip->client->dev); -return ret; - } - -/* -* Preserve the ret variable if the call to -* pm_runtime_put_autosuspend() is successful so the reading -* (if applicable) is returned to user space. -*/ - pm_ret =pm_runtime_put_autosuspend (&chip->client->dev); -if (pm_ret <0 ) -return pm_ret; +pm_runtime_put_autosuspend (&chip->client->dev);return ret; }@@ -773,16 +761,9 @@ static int tsl2583_write_raw(struct iio_dev *indio_dev,mutex_unlock (&chip->als_mutex); -if (ret <0 ) { -pm_runtime_put_autosuspend (&chip->client->dev);
drivers/iio/light/tsl2583.c static int tsl2583_write_raw(struct iio_dev *indio_dev, · line 766
Quoted 10 lines
drivers/iio/light/tsl2583.c +2
-7-return ret; - } +pm_runtime_put_autosuspend (&chip->client->dev); - ret =pm_runtime_put_autosuspend (&chip->client->dev); -if (ret <0 ) -return ret; - -return 0 ; +return ret;
Yuck this was indeed a mess, so great to clean that up!
Not asking for it in this series, but this is a great place to use some cleanup.h magic for the mutex and the runtime pm.
drivers/iio/light/us5182d.c static int us5182d_set_power_state(struct us5182d_data *data, bool on) · line 369
Quoted 13 lines
drivers/iio/light/tsl2583.c +0
-0}static const struct iio_info tsl2583_info = {drivers/iio/light/us5182d.c +1
-1@@ -366,7 +366,8 @@ static int us5182d_set_power_state(struct us5182d_data *data, bool on)if (on)return pm_runtime_resume_and_get (&data->client->dev); -return pm_runtime_put_autosuspend (&data->client->dev); +pm_runtime_put_autosuspend (&data->client->dev);
Prefer a blank line here. Always nice before a simple return like this.
Quoted 4 lines
drivers/iio/light/us5182d.c +1
-0+return 0 ; } static int us5182d_read_value(struct us5182d_data *data,
Re: [PATCH 03/13] iio: Discard pm_runtime_put_autosuspend() return values
Hi Jonathan,
Quoted 40 lines · Jonathan Cameron
Quoted 24 lines · On Tue, 6 Oct 2026 16:13:31 -0700 Brian Norris <[email protected]>…
Multiple IIO drivers inspect or propagate the return value of
pm_runtime_put_autosuspend(), which is not particularly useful.Returning an error code from
pm_runtime_put_autosuspend()merely means that it has not queued up a timer or work item to check whether or not the device can be suspended and there are many perfectly valid situations in which that can happen, like after writing "on" to the device's runtime PM "control" attribute in sysfs for one example.For this reason, modify these drivers to simply discard the
pm_runtime_put_autosuspend()return value, and change the return types offxls8962af_power_off()andfxas21002c_pm_put()to void.This will facilitate a planned change of the
pm_runtime_put_autosuspend()return type to void in the future, similar to commit3afd8df02433("PM: runtime: Changepm_runtime_put()return type to void").Signed-off-by Brian Norris <[email protected]>---
This patch is independent of the rest of the series, except for the end (changing the return type). I expect it can be applied by individual maintainers, and we pick up the end once the dust is settled.Quoted 4 lines
I couldn't decide if I should split this up by individual driver, or just 1 for the whole
drivers/iio/subsystem. Many of these drivers have borrowed the same patterns, so I chose to just send 1. Let me know if I should split it up.Hi Brian,
Great that you are doing this, but sadly it opens the can of worms that are the helpers a lot of these drivers have. They just about made sense before various recent
runtime_pmuseability improvements. Now they make very little sense at all. Dropping the return value here also removes the reason for a bunch of odd defensive ifdefs.
Quoted 3 lines
As such, I'm requesting some slightly more involved changes. Some of them I think are coupled tightly to what you are doing here so should be in this series, others can be left for later.
Thanks for the notes. I'll handle the ones that are at least approximately related to the code I'm touching. I think there are a few bugs that get resolved along the way too.
I may not get the more tangential ones though.
Also, unless you'd prefer otherwise, I plan to wait a bit more to send v2, since the merge window only just opened, and some of this series will just be a RESEND. I'd prefer maximizing the chance that the recipients will actually notice the patches, by submitting outside that window.
(OTOH, the bulk of these changes are in IIO, so I can just make IIO its own series if that helps things along.)
I'll leave a few comments below.
Quoted 68 lines
With that in mind, I think the changes will become a little more complex and so we probably end up with one patch per driver.
Jonathan
p.s. I'm feeling a bit mean asking for so many additional changes, so if you don't have time say that and I'll see if I can find some time (or someone else!) to do the extra work.
Quoted 26 lines
drivers/iio/accel/bmc150-accel-core.c | 14 +++++----- drivers/iio/accel/fxls8962af-core.c | 11 +++----- drivers/iio/accel/kxcjk-1013.c | 15 ++++++----- drivers/iio/accel/mma8452.c | 15 +++++------ drivers/iio/accel/mma9551_core.c | 15 +++++------ drivers/iio/adc/rcar-gyroadc.c | 4 +-- drivers/iio/adc/ti-ads1015.c | 8 +++--- drivers/iio/chemical/atlas-sensor.c | 4 +-- .../common/hid-sensors/hid-sensor-trigger.c | 14 ++++------ drivers/iio/gyro/bmg160_core.c | 17 +++++------- drivers/iio/gyro/fxas21002c_core.c | 12 +++------ drivers/iio/imu/kmx61.c | 17 ++++++------ drivers/iio/light/apds9960.c | 2 +- drivers/iio/light/isl29028.c | 15 +++-------- drivers/iio/light/pa12203001.c | 3 ++- drivers/iio/light/rpr0521.c | 15 +++++------ drivers/iio/light/tsl2583.c | 27 +++---------------- drivers/iio/light/us5182d.c | 3 ++- 18 files changed, 80 insertions(+), 131 deletions(-)drivers/iio/accel/bmc150-accel-core.c +0
-0drivers/iio/accel/bmc150-accel-core.c static int bmc150_accel_set_power_state(struct bmc150_accel_data *data, bool on) · line 332
Quoted 1 line
drivers/iio/accel/bmc150-accel-core.c +0
-0@@ -332,14 +332,14 @@ static int bmc150_accel_set_power_state(struct bmc150_accel_data *data, bool on)struct device *dev =regmap_get_device (data->regmap);This driver could do with modernizing to use the stuff that lets the compiler drop unused pm functions. That is a bit of a bigger change though so entirely optional wrt to this set.
Quoted 28 lines
drivers/iio/accel/bmc150-accel-core.c +7
-7int ret; -if (on) +if (on) { ret =pm_runtime_resume_and_get (dev); -else - ret =pm_runtime_put_autosuspend (dev); -if (ret <0 ) { -dev_err (dev, -"Failed: %s for %d\n" , __func__, on); -return ret; +if (ret <0 ) { +dev_err (dev,"%s: failed to resume: %d\n" , __func__, ret); +return ret; + } + }else { +pm_runtime_put_autosuspend (dev); }return 0 ;drivers/iio/accel/fxls8962af-core.c +0
-0@@ -216,16 +216,11 @@ static int fxls8962af_power_on(struct fxls8962af_data *data)return ret; }
Quoted 6 lines
drivers/iio/accel/fxls8962af-core.c static int fxls8962af_power_on(struct fxls8962af_data *data) · line 219
Quoted 2 lines
drivers/iio/accel/fxls8962af-core.c +1
-1-static int fxls8962af_power_off (struct fxls8962af_data *data) +static void fxls8962af_power_off (struct fxls8962af_data *data)This helper is completely pointless after you cleanup. Please drop it. The
_on()one just exists to print an error. So ideally drop that one as well.
Ack.
Quoted 20 lines
Quoted 20 lines
drivers/iio/accel/fxls8962af-core.c +2
-7{struct device *dev =regmap_get_device (data->regmap); -int ret; - - ret =pm_runtime_put_autosuspend (dev); -if (ret) -dev_err (dev,"failed to power off\n" ); -return ret; +pm_runtime_put_autosuspend (dev); }static int fxls8962af_standby (struct fxls8962af_data *data)@@ -692,7 +687,7 @@ fxls8962af_write_event_config(struct iio_dev *indio_dev,return -EBUSY ;/* Not in buffered mode so disable power */ - ret =fxls8962af_power_off (data); +fxls8962af_power_off (data);
Quoted 9 lines
drivers/iio/accel/kxcjk-1013.c static int kxcjk1013_set_power_state(struct kxcjk1013_data *data, bool on)
Quoted 7 lines
drivers/iio/accel/fxls8962af-core.c +0
-0iio_device_release_direct (indio_dev); }drivers/iio/accel/kxcjk-1013.c +0
-0@@ -633,14 +633,15 @@ static int kxcjk1013_set_power_state(struct kxcjk1013_data *data, bool on)Same as the one below.
Ack.
Quoted 22 lines
Quoted 22 lines
drivers/iio/accel/kxcjk-1013.c +8
-7#ifdef CONFIG_PM int ret; -if (on) +if (on) { ret =pm_runtime_resume_and_get (&data->client->dev); -else - ret =pm_runtime_put_autosuspend (&data->client->dev); -if (ret <0 ) { -dev_err (&data->client->dev, -"Failed: %s for %d\n" , __func__, on); -return ret; +if (ret <0 ) { +dev_err (&data->client->dev, +"%s: failed to resume: %d\n" , __func__, ret); +return ret; + } + }else { +pm_runtime_put_autosuspend (&data->client->dev); }#endif
Quoted 11 lines
drivers/iio/accel/mma8452.c static int mma8452_set_runtime_pm_state(struct i2c_client *client, bool on)
Quoted 5 lines
drivers/iio/accel/mma8452.c +0
-0@@ -224,15 +224,14 @@ static int mma8452_set_runtime_pm_state(struct i2c_client *client, bool on)As with so many of the others. This is now a pointless function. The reason for the ifdef is gone I believe after this patch so dropping that would be minimum. Better would be to get rid of the helper and just make the runtime pm calls directly wherever this was called.
Ack.
Quoted 22 lines
Quoted 22 lines
drivers/iio/accel/mma8452.c +7
-8#ifdef CONFIG_PM int ret; -if (on) +if (on) { ret =pm_runtime_resume_and_get (&client->dev); -else - ret =pm_runtime_put_autosuspend (&client->dev); -if (ret <0 ) { -dev_err (&client->dev, -"failed to change power state to %d\n" , on); - -return ret; +if (ret <0 ) { +dev_err (&client->dev,"failed to resume: %d\n" , ret); +return ret; + } + }else { +pm_runtime_put_autosuspend (&client->dev); }#endif
Quoted 13 lines
drivers/iio/accel/mma9551_core.c int mma9551_set_power_state(struct i2c_client *client, bool on) · line 670
Quoted 7 lines
drivers/iio/accel/mma9551_core.c +0
-0@@ -669,15 +669,14 @@ int mma9551_set_power_state(struct i2c_client *client, bool on)#ifdef CONFIG_PM int ret;Another one where I think the ifdef was for the return value of stubbed
pm_runtime_put_autosuspend. So we can drop that.
Also the function no longer has any real purpose - so if you don't mind going a little further, dropping this helper and calling thepm_runtimefunctions inline would be much nicer.
Ack.
Quoted 64 lines
Quoted 34 lines
drivers/iio/accel/mma9551_core.c +7
-8-if (on) +if (on) { ret =pm_runtime_resume_and_get (&client->dev); -else - ret =pm_runtime_put_autosuspend (&client->dev); -if (ret <0 ) { -dev_err (&client->dev, -"failed to change power state to %d\n" , on); - -return ret; +if (ret <0 ) { +dev_err (&client->dev,"failed to resume: %d\n" , ret); +return ret; + } + }else { +pm_runtime_put_autosuspend (&client->dev); }#endif drivers/iio/adc/ti-ads1015.c +0
-2@@ -370,15 +370,13 @@ static const struct iio_chan_spec tla2024_channels[] = {#ifdef CONFIG_PM static int ads1015_set_power_state (struct ads1015_data *data,bool on) { -int ret;struct device *dev =regmap_get_device (data->regmap);if (on) - ret =pm_runtime_resume_and_get (dev);drivers/iio/adc/ti-ads1015.c static const struct iio_chan_spec tla2024_channels[] = { · line 382
Quoted 10 lines
drivers/iio/adc/ti-ads1015.c +3
-3- else - ret =pm_runtime_put_autosuspend (dev); +return pm_runtime_resume_and_get (dev); - return ret < 0 ? ret : 0; +pm_runtime_put_autosuspend (dev); + return 0; }#else /* !CONFIG_PM */ I think the ifdef stuff here was to paper over the return value you are removing. Should be able to drop it now.
drivers/iio/gyro/bmg160_core.c static int bmg160_set_power_state(struct bmg160_data *data, bool on) · line 323
Quoted 11 lines
drivers/iio/gyro/bmg160_core.c +1
-1@@ -319,18 +319,15 @@ static int bmg160_set_power_state(struct bmg160_data *data, bool on)struct device *dev =regmap_get_device (data->regmap);int ret; -if (on) +if (on) { ret =pm_runtime_get_sync (dev);gah. Please just change this to use
pm_runtime_resume_and_get()whilst you are here.
Quoted 6 lines
Ideally squash the helper in favour of direct calls to the runtime pm. Yet another pace that the recent improvements in runtime pm useability have made what was of minor benefit entirely pointless.
The ifdef can go away as well as I think the only thing that helped with was the return value of the put you are removing.
One thing I found while looking through this driver:
bmg160_write_event_config() and bmg160_data_rdy_trigger_set_state() do some ghastly things w.r.t. runtime PM. In particular, they seem to perform put_autosuspend() *before* they're done touching the hardware, and then rely on the (very large) autosuspend delay to defer the runtime_suspend() operation. It even has a bunch of comments about the incorrect ordering.
That's one giant race condition. And the autosuspend delay is configurable via user space, so it's extra dangerous.
Anyway, that's to say: I plan to refactor/reorder the put_autosuspend() ordering there while I'm at it.
(There's also some strange error handling that potentially leads to runtime PM underflow. I'll fix that one while I'm here.)
I think this one is imitated in another few drivers as well.
Quoted 27 lines
Quoted 27 lines
drivers/iio/gyro/bmg160_core.c +6
-9- else - ret =pm_runtime_put_autosuspend (dev); - -if (ret <0 ) { -dev_err (dev,"Failed: bmg160_set_power_state for %d\n" , on); - -if (on) +if (ret <0 ) { +dev_err (dev,"%s: failed to resume: %d\n" , __func__, ret);pm_runtime_put_noidle (dev); - -return ret; +return ret; + } + }else { +pm_runtime_put_autosuspend (dev); }#endif drivers/iio/gyro/fxas21002c_core.c +0
-0@@ -369,11 +369,11 @@ static int fxas21002c_pm_get(struct fxas21002c_data *data)return pm_runtime_resume_and_get (regmap_get_device (data->regmap)); }
Quoted 8 lines
drivers/iio/gyro/fxas21002c_core.c static int fxas21002c_pm_get(struct fxas21002c_data *data) · line 372
Quoted 2 lines
drivers/iio/gyro/fxas21002c_core.c +1
-1-static int fxas21002c_pm_put (struct fxas21002c_data *data) +static void fxas21002c_pm_put (struct fxas21002c_data *data)The two runtime pm helpers in here have evolved to become a terrible idea.
Can we just drop them as part of your change and make the runtime pm calls directly instead? The dev is always available at the call sites so this should be trivial.
Ack.
Quoted 112 lines
drivers/iio/light/apds9960.c
Quoted 35 lines
drivers/iio/gyro/fxas21002c_core.c +3
-7{struct device *dev =regmap_get_device (data->regmap); -return pm_runtime_put_autosuspend (dev); +pm_runtime_put_autosuspend (dev); }static int fxas21002c_temp_get (struct fxas21002c_data *data,int *val)@@ -396,9 +396,7 @@ static int fxas21002c_temp_get(struct fxas21002c_data *data, int *val) *val =sign_extend32 (temp,7 ); - ret =fxas21002c_pm_put (data); -if (ret <0 ) - goto data_unlock; +fxas21002c_pm_put (data); ret =IIO_VAL_INT ;@@ -430,9 +428,7 @@ static int fxas21002c_axis_get(struct fxas21002c_data *data, *val =sign_extend32 (be16_to_cpu (axis_be),15 ); - ret =fxas21002c_pm_put (data); -if (ret <0 ) - goto data_unlock; +fxas21002c_pm_put (data); ret =IIO_VAL_INT ;drivers/iio/light/apds9960.c +0
-0Not needed for this series but...
Another one that has become rather pointless and should just be split into separate functions for enable and disable (where disable one is just
pm_runtime_put_autosuspend)Quoted 25 lines
drivers/iio/light/apds9960.c +1
-1@@ -495,7 +495,7 @@ static int apds9960_set_power_state(struct apds9960_data *data, bool on)usleep_range (data->als_adc_int_us,APDS9960_MAX_INT_TIME_IN_US ); }else { - ret =pm_runtime_put_autosuspend (dev); +pm_runtime_put_autosuspend (dev); }mutex_unlock (&data->lock);drivers/iio/light/isl29028.c +1
-3@@ -395,9 +395,7 @@ static int isl29028_write_raw(struct iio_dev *indio_dev,if (ret <0 )return ret; - ret =pm_runtime_put_autosuspend (dev); -if (ret <0 ) -return ret; +pm_runtime_put_autosuspend (dev);return 0 ; }drivers/iio/light/isl29028.c static int isl29028_read_raw(struct iio_dev *indio_dev, · line 409
Quoted 5 lines
drivers/iio/light/isl29028.c +1
-1@@ -408,7 +406,7 @@ static int isl29028_read_raw(struct iio_dev *indio_dev, {struct isl29028_chip *chip =iio_priv (indio_dev);struct device *dev =regmap_get_device (chip->regmap); -int ret, pm_ret; +int ret;Not one for this series (the patch for this LGTM),
but if anyone is reading and bored,cleanup.hmagic would simplify this code nicely.Quoted 24 lines
drivers/iio/light/isl29028.c +1
-8ret =pm_runtime_resume_and_get (dev);if (ret <0 )@@ -464,14 +462,7 @@ static int isl29028_read_raw(struct iio_dev *indio_dev,if (ret <0 )return ret; -/** -* Preserve the ret variable if the call to -* pm_runtime_put_autosuspend() is successful so the reading -* (if applicable) is returned to user space. -*/ - pm_ret =pm_runtime_put_autosuspend (dev); -if (pm_ret <0 ) -return pm_ret; +pm_runtime_put_autosuspend (dev);return ret; }drivers/iio/light/pa12203001.c +0
-0drivers/iio/light/pa12203001.c static int pa12203001_set_power_state(struct pa12203001_data *data, bool on, · line 192
Quoted 6 lines
drivers/iio/light/pa12203001.c +2
-1@@ -188,7 +188,8 @@ static int pa12203001_set_power_state(struct pa12203001_data *data, bool on,if (on)return pm_runtime_resume_and_get (&data->client->dev); -return pm_runtime_put_autosuspend (&data->client->dev); +pm_runtime_put_autosuspend (&data->client->dev); +return 0 ;This function has evolved to the point of silliness.
Given how minor your change is, feel free to ignore the following however!
Quoted 4 lines
There is zero shared code in here between the on and off paths so rename the function
pa12203001_runtime_enable()and cover only the 'on' case. Then just put thepm_runtime_put_autosuspend()in wherever on was previously false.
Ack. But because "runtime_enable()" has a specific runtime PM meaning that is very different than the usage here, I've chosen the name "pa12203001_runtime_resume_and_get()" instead, which I think makes for more API symmetry.
Quoted 3 lines
Given that presumably one of the things is being turned on whenever we enable it, also pull the mutex out of the if blocks and use
guard()to simplify that.
guard() is tricky, since it produces a deadlock if you hold the lock across pm_runtime_resume_and_get(). So I'll skip that part and leave it to someone (or no one) who is more particularly interested in this driver.
Quoted 62 lines
drivers/iio/light/rpr0521.c static int rpr0521_set_power_state(struct rpr0521_data *data, bool on, · line 358
Quoted 10 lines
drivers/iio/light/pa12203001.c +0
-0err:mutex_unlock (&data->lock);drivers/iio/light/rpr0521.c +0
-0@@ -357,15 +357,14 @@ static int rpr0521_set_power_state(struct rpr0521_data *data, bool on,* Note: If either measurement is re-enabled before _suspend(), * both stay enabled until _suspend(). Not asking for it in your series as beyond the scope, but this function should just be split in two. Almost no code is shared between the get and put paths.
However...
Quoted 10 lines
drivers/iio/light/rpr0521.c +1
-7*/ -if (on) +if (on) { ret =pm_runtime_resume_and_get (&data->client->dev); -else - ret =pm_runtime_put_autosuspend (&data->client->dev); -if (ret <0 ) { -dev_err (&data->client->dev, -"Failed: rpr0521_set_power_state for %d, ret %d\n" , - on, ret);drivers/iio/light/rpr0521.c static int rpr0521_set_power_state(struct rpr0521_data *data, bool on, · line 370
Quoted 10 lines
drivers/iio/light/rpr0521.c +6
-1-return ret; +if (ret <0 ) { +dev_err (&data->client->dev,"Failed to resume: %d\n" , ret); +return ret; + } + }else { +pm_runtime_put_autosuspend (&data->client->dev); }if (on) {This no longer makes any sense so maybe it is better to just split the code as part of your change set or at minimum change to
if (!on) { pm_runtime_put_autosuspend(&data->client->dev); return 0; } ret = pm_runtime_resume_and_get(&data->client->dev); if (ret < 0) { dev_err(&data->client->dev, "Failed to resume: %d\n", ret); return ret; } /* If _resume() was not called, enable measurement now. */ if (data->als_ps_need_en) { ret = rpr0521_als_enable(data, RPR0521_MODE_ALS_ENABLE); if (ret) return ret; data->als_ps_need_en = false; } if (data->pxs_ps_need_en) {
Quoted 8 lines
ret = rpr0521_pxs_enable(data, RPR0521_MODE_PXS_ENABLE); if (ret) return ret; data->pxs_ps_need_en = false; } #endif //mess that makes sense with more context! return 0; }
Ack.
Quoted 67 lines
Quoted 40 lines
drivers/iio/light/tsl2583.c +2
-16@@ -640,7 +640,7 @@ static int tsl2583_read_raw(struct iio_dev *indio_dev, int *val, int *val2, long mask) { struct tsl2583_chip *chip = iio_priv(indio_dev); -int ret, pm_ret; +int ret; ret =pm_runtime_resume_and_get (&chip->client->dev);if (ret <0 )@@ -709,19 +709,7 @@ static int tsl2583_read_raw(struct iio_dev *indio_dev, read_done:mutex_unlock (&chip->als_mutex); -if (ret <0 ) { -pm_runtime_put_autosuspend (&chip->client->dev); -return ret; - } - -/* -* Preserve the ret variable if the call to -* pm_runtime_put_autosuspend() is successful so the reading -* (if applicable) is returned to user space. -*/ - pm_ret =pm_runtime_put_autosuspend (&chip->client->dev); -if (pm_ret <0 ) -return pm_ret; +pm_runtime_put_autosuspend (&chip->client->dev);return ret; }@@ -773,16 +761,9 @@ static int tsl2583_write_raw(struct iio_dev *indio_dev,mutex_unlock (&chip->als_mutex); -if (ret <0 ) { -pm_runtime_put_autosuspend (&chip->client->dev);drivers/iio/light/tsl2583.c static int tsl2583_write_raw(struct iio_dev *indio_dev, · line 766
Quoted 10 lines
drivers/iio/light/tsl2583.c +2
-7-return ret; - } +pm_runtime_put_autosuspend (&chip->client->dev); - ret =pm_runtime_put_autosuspend (&chip->client->dev); -if (ret <0 ) -return ret; - -return 0 ; +return ret;Yuck this was indeed a mess, so great to clean that up!
Not asking for it in this series, but this is a great place to use some
cleanup.hmagic for the mutex and the runtime pm.Quoted 11 lines
drivers/iio/light/tsl2583.c +0
-0}static const struct iio_info tsl2583_info = {drivers/iio/light/us5182d.c +0
-0@@ -366,7 +366,8 @@ static int us5182d_set_power_state(struct us5182d_data *data, bool on)if (on)return pm_runtime_resume_and_get (&data->client->dev);
Quoted 4 lines
drivers/iio/light/us5182d.c static int us5182d_set_power_state(struct us5182d_data *data, bool on) · line 369
Quoted 2 lines
drivers/iio/light/us5182d.c +1
-1-return pm_runtime_put_autosuspend (&data->client->dev); +pm_runtime_put_autosuspend (&data->client->dev);Prefer a blank line here. Always nice before a simple return like this.
Ack.
Brian
Quoted 5 lines
Quoted 4 lines
drivers/iio/light/us5182d.c +1
-0+return 0 ; } static int us5182d_read_value(struct us5182d_data *data,
Jonathan Cameron
maintainerRe: [PATCH 03/13] iio: Discard pm_runtime_put_autosuspend() return values
Quoted 53 lines · On Mon, 19 Oct 2026 15:02:08 -0700 Brian Norris <[email protected]>…
Hi Jonathan,
Quoted 40 lines · Jonathan Cameron
Quoted 24 lines · On Tue, 6 Oct 2026 16:13:31 -0700 Brian Norris <[email protected]>…
Multiple IIO drivers inspect or propagate the return value of
pm_runtime_put_autosuspend(), which is not particularly useful.Returning an error code from
pm_runtime_put_autosuspend()merely means that it has not queued up a timer or work item to check whether or not the device can be suspended and there are many perfectly valid situations in which that can happen, like after writing "on" to the device's runtime PM "control" attribute in sysfs for one example.For this reason, modify these drivers to simply discard the
pm_runtime_put_autosuspend()return value, and change the return types offxls8962af_power_off()andfxas21002c_pm_put()to void.This will facilitate a planned change of the
pm_runtime_put_autosuspend()return type to void in the future, similar to commit3afd8df02433("PM: runtime: Changepm_runtime_put()return type to void").Signed-off-by Brian Norris <[email protected]>---
This patch is independent of the rest of the series, except for the end (changing the return type). I expect it can be applied by individual maintainers, and we pick up the end once the dust is settled.Quoted 4 lines
I couldn't decide if I should split this up by individual driver, or just 1 for the whole
drivers/iio/subsystem. Many of these drivers have borrowed the same patterns, so I chose to just send 1. Let me know if I should split it up.Hi Brian,
Great that you are doing this, but sadly it opens the can of worms that are the helpers a lot of these drivers have. They just about made sense before various recent
runtime_pmuseability improvements. Now they make very little sense at all. Dropping the return value here also removes the reason for a bunch of odd defensive ifdefs.Quoted 3 lines
As such, I'm requesting some slightly more involved changes. Some of them I think are coupled tightly to what you are doing here so should be in this series, others can be left for later.
Thanks for the notes. I'll handle the ones that are at least approximately related to the code I'm touching. I think there are a few bugs that get resolved along the way too.
I may not get the more tangential ones though.
Quoted 8 lines
Also, unless you'd prefer otherwise, I plan to wait a bit more to send v2, since the merge window only just opened, and some of this series will just be a RESEND. I'd prefer maximizing the chance that the recipients will actually notice the patches, by submitting outside that window.
(OTOH, the bulk of these changes are in IIO, so I can just make IIO its own series if that helps things along.)
I'd split out an IIO series but take your time as no particular rush given these won't merge until next time around.
Quoted 31 lines
Quoted 15 lines
drivers/iio/gyro/bmg160_core.c static int bmg160_set_power_state(struct bmg160_data *data, bool on) · line 323
Quoted 11 lines
drivers/iio/gyro/bmg160_core.c +1
-1@@ -319,18 +319,15 @@ static int bmg160_set_power_state(struct bmg160_data *data, bool on)struct device *dev =regmap_get_device (data->regmap);int ret; -if (on) +if (on) { ret =pm_runtime_get_sync (dev);gah. Please just change this to use
pm_runtime_resume_and_get()whilst you are here.Quoted 6 lines
Ideally squash the helper in favour of direct calls to the runtime pm. Yet another pace that the recent improvements in runtime pm useability have made what was of minor benefit entirely pointless.
The ifdef can go away as well as I think the only thing that helped with was the return value of the put you are removing.
One thing I found while looking through this driver:
bmg160_write_event_config()andbmg160_data_rdy_trigger_set_state()do some ghastly things w.r.t. runtime PM. In particular, they seem to performput_autosuspend()*before* they're done touching the hardware, and then rely on the (very large) autosuspend delay to defer theruntime_suspend()operation. It even has a bunch of comments about the incorrect ordering.
Quoted 5 lines
That's one giant race condition. And the autosuspend delay is configurable via user space, so it's extra dangerous.
Anyway, that's to say: I plan to refactor/reorder the
put_autosuspend()ordering there while I'm at it.
Excellent!
Quoted 5 lines
(There's also some strange error handling that potentially leads to runtime PM underflow. I'll fix that one while I'm here.)
I think this one is imitated in another few drivers as well.
yeah. We do seem to cut and paste bad practice and bugs around :(
Quoted 41 lines
Quoted 36 lines
Quoted 26 lines
drivers/iio/gyro/bmg160_core.c +6
-9- else - ret =pm_runtime_put_autosuspend (dev); - -if (ret <0 ) { -dev_err (dev,"Failed: bmg160_set_power_state for %d\n" , on); - -if (on) +if (ret <0 ) { +dev_err (dev,"%s: failed to resume: %d\n" , __func__, ret);pm_runtime_put_noidle (dev); - -return ret; +return ret; + } + }else { +pm_runtime_put_autosuspend (dev); }#endif drivers/iio/light/pa12203001.c +0
-0drivers/iio/light/pa12203001.c static int pa12203001_set_power_state(struct pa12203001_data *data, bool on, · line 192
Quoted 6 lines
drivers/iio/light/pa12203001.c +2
-1@@ -188,7 +188,8 @@ static int pa12203001_set_power_state(struct pa12203001_data *data, bool on,if (on)return pm_runtime_resume_and_get (&data->client->dev); -return pm_runtime_put_autosuspend (&data->client->dev); +pm_runtime_put_autosuspend (&data->client->dev); +return 0 ;This function has evolved to the point of silliness.
Given how minor your change is, feel free to ignore the following however!Quoted 4 lines
There is zero shared code in here between the on and off paths so rename the function
pa12203001_runtime_enable()and cover only the 'on' case. Then just put thepm_runtime_put_autosuspend()in wherever on was previously false.
Quoted 4 lines
Ack. But because "
runtime_enable()" has a specific runtime PM meaning that is very different than the usage here, I've chosen the name "pa12203001_runtime_resume_and_get()" instead, which I think makes for more API symmetry.
Good point on naming.
Quoted 9 lines
Quoted 4 lines
Given that presumably one of the things is being turned on whenever we enable it, also pull the mutex out of the if blocks and use
guard()to simplify that.
guard()is tricky, since it produces a deadlock if you hold the lock acrosspm_runtime_resume_and_get(). So I'll skip that part and leave it to someone (or no one) who is more particularly interested in this driver.
Ah indeed. Should be scoped_guard() but can wait for someone else who cares enough to do that and maybe not worth it anyway. I got lost in imagining what the other refactors would look like.
Thanks,
Jonathan
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
drivers/iio/light/pa12203001.c +0
-0err:mutex_unlock (&data->lock);
Re: [PATCH 03/13] iio: Discard pm_runtime_put_autosuspend() return values
Quoted 8 lines · Jonathan Cameron
Quoted 6 lines · On Mon, 19 Oct 2026 15:02:08 -0700 Brian Norris <[email protected]>…
Also, unless you'd prefer otherwise, I plan to wait a bit more to send v2, since the merge window only just opened, and some of this series will just be a RESEND. I'd prefer maximizing the chance that the recipients will actually notice the patches, by submitting outside that window.
Quoted 5 lines
Quoted 2 lines
(OTOH, the bulk of these changes are in IIO, so I can just make IIO its own series if that helps things along.)
I'd split out an IIO series but take your time as no particular rush given these won't merge until next time around.
OK, I've sent out an IIO-specific v2. I'll resend non-IIO separately later on.
Brian
PATCH 4/13 media: venus: Discard pm_runtime_put_autosuspend() return values
In reply to Brian NorrisPropagating the pm_runtime_put_autosuspend() return value to callers is not particularly useful.
Returning an error code from pm_runtime_put_autosuspend() merely means that it has not queued up a timer or work item to check whether or not the device can be suspended and there are many perfectly valid situations in which that can happen, like after writing "on" to the device's runtime PM "control" attribute in sysfs for one example.
Accordingly, update the Venus vdec and venc drivers to simply discard the return value of pm_runtime_put_autosuspend().
This will facilitate a planned change of the pm_runtime_put_autosuspend() return type to void in the future, similar to commit 3afd8df02433 ("PM: runtime: Change pm_runtime_put() return type to void").
---
This patch is independent of the rest of the series, except for the end (changing the return type). I expect it can be applied by individual maintainers, and we pick up the end once the dust is settled.
drivers/media/platform/qcom/venus/vdec.c | 10 ++++------ drivers/media/platform/qcom/venus/venc.c | 10 ++++------ 2 files changed, 8 insertions(+), 12 deletions(-)
drivers/media/platform/qcom/venus/vdec.c +4 -6
@@ -644,12 +644,12 @@ static int vdec_pm_put(struct venus_inst *inst, bool autosuspend) {struct venus_core *core = inst->core;struct device *dev = core->dev_dec; -int ret; +int ret =0 ;mutex_lock (&core->pm_lock);if (autosuspend) - ret =pm_runtime_put_autosuspend (dev); +pm_runtime_put_autosuspend (dev);else ret =pm_runtime_put_sync (dev);
@@ -671,7 +671,7 @@ static int vdec_pm_get_put(struct venus_inst *inst)if (ret <0 ) goto error; - ret =pm_runtime_put_autosuspend (dev); +pm_runtime_put_autosuspend (dev); } error:
@@ -1200,9 +1200,7 @@ static int vdec_start_streaming(struct vb2_queue *q, unsigned int count)if (ret) goto put_power; - ret =vdec_pm_put (inst, true); -if (ret) - goto error; +vdec_pm_put (inst, true); ret =vdec_start_output (inst); }
drivers/media/platform/qcom/venus/venc.c +4 -6
@@ -617,12 +617,12 @@ static int venc_pm_put(struct venus_inst *inst, bool autosuspend) {struct venus_core *core = inst->core;struct device *dev = core->dev_enc; -int ret; +int ret =0 ;mutex_lock (&core->pm_lock);if (autosuspend) - ret =pm_runtime_put_autosuspend (dev); +pm_runtime_put_autosuspend (dev);else ret =pm_runtime_put_sync (dev);
@@ -644,7 +644,7 @@ static int venc_pm_get_put(struct venus_inst *inst)if (ret <0 ) goto error; - ret =pm_runtime_put_autosuspend (dev); +pm_runtime_put_autosuspend (dev); } error:
@@ -1275,9 +1275,7 @@ static int venc_start_streaming(struct vb2_queue *q, unsigned int count)if (ret) goto put_power; - ret =venc_pm_put (inst, true); -if (ret) - goto error; +venc_pm_put (inst, true); ret =venc_set_properties (inst);if (ret)
Signature · 1 line
2.56.0.360.g66cac248cb-goog
PATCH 5/13 pinctrl: qcom: lpass-lpi: Discard pm_runtime_put_autosuspend() return values
In reply to Brian NorrisPassing the pm_runtime_put_autosuspend() return value to callers is not particularly useful.
Returning an error code from pm_runtime_put_autosuspend() merely means that it has not queued up a timer or work item to check whether or not the device can be suspended and there are many perfectly valid situations in which that can happen, like after writing "on" to the device's runtime PM "control" attribute in sysfs for one example.
Accordingly, update the LPASS LPI pinctrl driver to simply discard the return value of pm_runtime_put_autosuspend() and return 0 to the callers.
This will facilitate a planned change of the pm_runtime_put_autosuspend() return type to void in the future, similar to commit 3afd8df02433 ("PM: runtime: Change pm_runtime_put() return type to void").
---
This patch is independent of the rest of the series, except for the end (changing the return type). I expect it can be applied by individual maintainers, and we pick up the end once the dust is settled.
drivers/pinctrl/qcom/pinctrl-lpass-lpi.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)
drivers/pinctrl/qcom/pinctrl-lpass-lpi.c +12 -4
@@ -76,7 +76,9 @@ static int lpi_gpio_read(struct lpi_pinctrl *state, unsigned int pin,lpi_gpio_read_reg (state, pin, addr, val); -return pm_runtime_put_autosuspend (state->dev); +pm_runtime_put_autosuspend (state->dev); + +return 0 ; }static const struct pinctrl_ops lpi_gpio_pinctrl_ops = {
@@ -164,7 +166,9 @@ static int lpi_gpio_set_mux(struct pinctrl_dev *pctldev, unsigned int function,u32p_replace_bits (&val, i,LPI_GPIO_FUNCTION_MASK );lpi_gpio_write_reg (pctrl, pin,LPI_GPIO_CFG_REG , val); -return pm_runtime_put_autosuspend (pctrl->dev); +pm_runtime_put_autosuspend (pctrl->dev); + +return 0 ; }static const struct pinmux_ops lpi_gpio_pinmux_ops = {
@@ -261,7 +265,9 @@ static int lpi_config_set_slew_rate(struct lpi_pinctrl *pctrl,mutex_unlock (&pctrl->lock); -return pm_runtime_put_autosuspend (pctrl->dev); +pm_runtime_put_autosuspend (pctrl->dev); + +return 0 ; } static int lpi_config_set(struct pinctrl_dev *pctldev, unsigned int group,
@@ -335,7 +341,9 @@ static int lpi_config_set(struct pinctrl_dev *pctldev, unsigned int group,lpi_gpio_write_reg (pctrl, group,LPI_GPIO_CFG_REG , val); -return pm_runtime_put_autosuspend (pctrl->dev); +pm_runtime_put_autosuspend (pctrl->dev); + +return 0 ; }static const struct pinconf_ops lpi_gpio_pinconf_ops = {
Signature · 1 line
2.56.0.360.g66cac248cb-goog
Re: [PATCH 05/13] pinctrl: qcom: lpass-lpi: Discard pm_runtime_put_autosuspend() return values
Quoted 13 lines · Brian Norris
Passing the
pm_runtime_put_autosuspend()return value to callers is not particularly useful.Returning an error code from
pm_runtime_put_autosuspend()merely means that it has not queued up a timer or work item to check whether or not the device can be suspended and there are many perfectly valid situations in which that can happen, like after writing "on" to the device's runtime PM "control" attribute in sysfs for one example.Accordingly, update the LPASS LPI pinctrl driver to simply discard the return value of
pm_runtime_put_autosuspend()and return 0 to the callers.
Quoted 6 lines
This will facilitate a planned change of the
pm_runtime_put_autosuspend()return type to void in the future, similar to commit3afd8df02433("PM: runtime: Changepm_runtime_put()return type to void").Signed-off-by Brian Norris <[email protected]>
Yours,
Linus Walleij
PATCH 6/13 spi: ti-qspi: Discard pm_runtime_put_autosuspend() return value
In reply to Brian NorrisFailing SPI setup due to pm_runtime_put_autosuspend() returning a negative value is not particularly useful.
Returning an error code from pm_runtime_put_autosuspend() merely means that it has not queued up a timer or work item to check whether or not the device can be suspended and there are many perfectly valid situations in which that can happen, like after writing "on" to the device's runtime PM "control" attribute in sysfs for one example.
Accordingly, update ti_qspi_setup() to simply discard the return value of pm_runtime_put_autosuspend().
This will facilitate a planned change of the pm_runtime_put_autosuspend() return type to void in the future, similar to commit 3afd8df02433 ("PM: runtime: Change pm_runtime_put() return type to void").
---
This patch is independent of the rest of the series, except for the end (changing the return type). I expect it can be applied by individual maintainers, and we pick up the end once the dust is settled.
drivers/spi/spi-ti-qspi.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)
drivers/spi/spi-ti-qspi.c +1 -5
@@ -158,11 +158,7 @@ static int ti_qspi_setup(struct spi_device *spi)return ret; } - ret =pm_runtime_put_autosuspend (qspi->dev); -if (ret <0 ) { -dev_err (qspi->dev,"pm_runtime_put_autosuspend() failed\n" ); -return ret; - } +pm_runtime_put_autosuspend (qspi->dev);return 0 ; }
Signature · 1 line
2.56.0.360.g66cac248cb-goog
PATCH 7/13 greybus: Discard pm_runtime_put_autosuspend() return value
In reply to Brian NorrisReturning an error code from pm_runtime_put_autosuspend() merely means that it has not queued up a timer or work item to check whether or not the device can be suspended and there are many perfectly valid situations in which that can happen, like after writing "on" to the device's runtime PM "control" attribute in sysfs for one example.
Modify gb_pm_runtime_put_autosuspend() to discard the pm_runtime_put_autosuspend() return value, change its return type to void, and update its caller in the Greybus power supply driver accordingly. Also drop the redundant pm_runtime_mark_last_busy() call from gb_pm_runtime_put_autosuspend() while we're here, as this is already part of pm_runtime_put_autosuspend().
Note that this resolves a bug in the power_supply driver: in tracking 'gbpsy->pm_acquired', it erroneously assumed that pm_runtime_put_autosuspend() would not release a refcount when it returned a non-zero value. That's a false assumption.
This will facilitate a planned change of the pm_runtime_put_autosuspend() return type to void in the future, similar to commit 3afd8df02433 ("PM: runtime: Change pm_runtime_put() return type to void").
---
This patch is independent of the rest of the series, except for the end (changing the return type). I expect it can be applied by individual maintainers, and we pick up the end once the dust is settled.
drivers/staging/greybus/power_supply.c | 8 ++------ include/linux/greybus/bundle.h | 12 +++--------- 2 files changed, 5 insertions(+), 15 deletions(-)
drivers/staging/greybus/power_supply.c +2 -6
@@ -377,12 +377,8 @@ static void gb_power_supply_state_change(struct gb_power_supply *gbpsy, gbpsy->pm_acquired = true; }else {if (gbpsy->pm_acquired) { - ret =gb_pm_runtime_put_autosuspend (connection->bundle); -if (ret) -dev_err (&connection->bundle->dev, -"Fail to set wake unlock for none charging\n" ); -else - gbpsy->pm_acquired = false; +gb_pm_runtime_put_autosuspend (connection->bundle); + gbpsy->pm_acquired = false; } }
include/linux/greybus/bundle.h +3 -9
@@ -59,14 +59,9 @@ static inline int gb_pm_runtime_get_sync(struct gb_bundle *bundle)return 0 ; } -static inline int gb_pm_runtime_put_autosuspend (struct gb_bundle *bundle) +static inline void gb_pm_runtime_put_autosuspend (struct gb_bundle *bundle) { -int retval; - -pm_runtime_mark_last_busy (&bundle->dev); - retval =pm_runtime_put_autosuspend (&bundle->dev); - -return retval; +pm_runtime_put_autosuspend (&bundle->dev); }static inline void gb_pm_runtime_get_noresume (struct gb_bundle *bundle)
@@ -82,8 +77,7 @@ static inline void gb_pm_runtime_put_noidle(struct gb_bundle *bundle)#else static inline int gb_pm_runtime_get_sync (struct gb_bundle *bundle) {return 0 ; } -static inline int gb_pm_runtime_put_autosuspend (struct gb_bundle *bundle) -{return 0 ; } +static inline void gb_pm_runtime_put_autosuspend (struct gb_bundle *bundle) {}static inline void gb_pm_runtime_get_noresume (struct gb_bundle *bundle) {}static inline void gb_pm_runtime_put_noidle (struct gb_bundle *bundle) {}
Signature · 1 line
2.56.0.360.g66cac248cb-goog
Re: [PATCH 07/13] greybus: Discard pm_runtime_put_autosuspend() return value
Quoted 5 lines · Brian Norris
Returning an error code from
pm_runtime_put_autosuspend()merely means that it has not queued up a timer or work item to check whether or not the device can be suspended and there are many perfectly valid situations in which that can happen, like after writing "on" to the device's runtime PM "control" attribute in sysfs for one example.
If I understand this right, "put autosuspend" is basically an unconditional "put" with the assumption that auto-suspend will ensure the eventual suspend occurs sometime after the reference count reaches zero.
Quoted 6 lines
Modify
gb_pm_runtime_put_autosuspend()to discard thepm_runtime_put_autosuspend()return value, change its return type to void, and update its caller in the Greybus power supply driver accordingly. Also drop the redundantpm_runtime_mark_last_busy()call fromgb_pm_runtime_put_autosuspend()while we're here, as this is already part ofpm_runtime_put_autosuspend().
I support including this fix in this patch.
Quoted 4 lines
Note that this resolves a bug in the
power_supplydriver: in tracking 'gbpsy->pm_acquired', it erroneously assumed thatpm_runtime_put_autosuspend()would not release a refcount when it returned a non-zero value. That's a false assumption.
So is it safe for this code to assume the reference count has been decremented in this case? The comments say we're trying to ensure there's exactly one get/put pair. (I don't know the reasoning behind that though.)
Quoted 6 lines
This will facilitate a planned change of the
pm_runtime_put_autosuspend()return type to void in the future, similar to commit3afd8df02433("PM: runtime: Changepm_runtime_put()return type to void").Signed-off-by Brian Norris <[email protected]>
Despite my questions/comments I think this looks good.
Quoted 59 lines
---
This patch is independent of the rest of the series, except for the end (changing the return type). I expect it can be applied by individual maintainers, and we pick up the end once the dust is settled.drivers/staging/greybus/power_supply.c | 8 ++------ include/linux/greybus/bundle.h | 12 +++--------- 2 files changed, 5 insertions(+), 15 deletions(-)drivers/staging/greybus/power_supply.c +2
-6@@ -377,12 +377,8 @@ static void gb_power_supply_state_change(struct gb_power_supply *gbpsy, gbpsy->pm_acquired = true; }else {if (gbpsy->pm_acquired) { - ret =gb_pm_runtime_put_autosuspend (connection->bundle); -if (ret) -dev_err (&connection->bundle->dev, -"Fail to set wake unlock for none charging\n" ); -else - gbpsy->pm_acquired = false; +gb_pm_runtime_put_autosuspend (connection->bundle); + gbpsy->pm_acquired = false; } }include/linux/greybus/bundle.h +3
-9@@ -59,14 +59,9 @@ static inline int gb_pm_runtime_get_sync(struct gb_bundle *bundle)return 0 ; } -static inline int gb_pm_runtime_put_autosuspend (struct gb_bundle *bundle) +static inline void gb_pm_runtime_put_autosuspend (struct gb_bundle *bundle) { -int retval; - -pm_runtime_mark_last_busy (&bundle->dev); - retval =pm_runtime_put_autosuspend (&bundle->dev); - -return retval; +pm_runtime_put_autosuspend (&bundle->dev); }static inline void gb_pm_runtime_get_noresume (struct gb_bundle *bundle)@@ -82,8 +77,7 @@ static inline void gb_pm_runtime_put_noidle(struct gb_bundle *bundle)#else static inline int gb_pm_runtime_get_sync (struct gb_bundle *bundle) {return 0 ; } -static inline int gb_pm_runtime_put_autosuspend (struct gb_bundle *bundle) -{return 0 ; } +static inline void gb_pm_runtime_put_autosuspend (struct gb_bundle *bundle) {}static inline void gb_pm_runtime_get_noresume (struct gb_bundle *bundle) {}static inline void gb_pm_runtime_put_noidle (struct gb_bundle *bundle) {}
Re: [PATCH 07/13] greybus: Discard pm_runtime_put_autosuspend() return value
Hi Alex,
Quoted 11 lines · Alex Elder
Quoted 5 lines · Brian Norris
Returning an error code from
pm_runtime_put_autosuspend()merely means that it has not queued up a timer or work item to check whether or not the device can be suspended and there are many perfectly valid situations in which that can happen, like after writing "on" to the device's runtime PM "control" attribute in sysfs for one example.If I understand this right, "put autosuspend" is basically an unconditional "put" with the assumption that auto-suspend will ensure the eventual suspend occurs sometime after the reference count reaches zero.
Yes. Similar pattern for any other pm_runtime_put... variant -- they always decrement the reference counter, even if they otherwise return non-zero/error.
It definitely can be confusing, and it's one reason for $subject series -- to remove confusing API surface when it doesn't help. (Similar story for pm_runtime_get_sync() -- it increments the reference count, even on error. That's why pm_runtime_resume_and_get() was invented.)
Quoted 9 lines
Quoted 6 lines
Modify
gb_pm_runtime_put_autosuspend()to discard thepm_runtime_put_autosuspend()return value, change its return type to void, and update its caller in the Greybus power supply driver accordingly. Also drop the redundantpm_runtime_mark_last_busy()call fromgb_pm_runtime_put_autosuspend()while we're here, as this is already part ofpm_runtime_put_autosuspend().I support including this fix in this patch.
Quoted 7 lines
Quoted 4 lines
Note that this resolves a bug in the
power_supplydriver: in tracking 'gbpsy->pm_acquired', it erroneously assumed thatpm_runtime_put_autosuspend()would not release a refcount when it returned a non-zero value. That's a false assumption.So is it safe for this code to assume the reference count has been decremented in this case?
Correct.
Quoted 3 lines
The comments say we're trying
to ensure there's exactly one get/put pair. (I don't know the reasoning behind that though.)
I didn't look too far into this, but on first glance, it looks like gb_power_supply_state_change() can receieve a number of different properties, several of which mean "off". We only want the first "off" to drop a reference count.
That seems like a fairly typical sort of pattern, and the right answer is to ignore the return code, because the reference count was decremented unconditionally.
Quoted 10 lines
Quoted 6 lines
This will facilitate a planned change of the
pm_runtime_put_autosuspend()return type to void in the future, similar to commit3afd8df02433("PM: runtime: Changepm_runtime_put()return type to void").Signed-off-by Brian Norris <[email protected]>Despite my questions/comments I think this looks good.
Reviewed-by Alex Elder <[email protected]>
Thanks,
Brian
Quoted 56 lines
Quoted 55 lines
---
This patch is independent of the rest of the series, except for the end (changing the return type). I expect it can be applied by individual maintainers, and we pick up the end once the dust is settled.drivers/staging/greybus/power_supply.c | 8 ++------ include/linux/greybus/bundle.h | 12 +++--------- 2 files changed, 5 insertions(+), 15 deletions(-)drivers/staging/greybus/power_supply.c +2
-6@@ -377,12 +377,8 @@ static void gb_power_supply_state_change(struct gb_power_supply *gbpsy, gbpsy->pm_acquired = true; }else {if (gbpsy->pm_acquired) { - ret =gb_pm_runtime_put_autosuspend (connection->bundle); -if (ret) -dev_err (&connection->bundle->dev, -"Fail to set wake unlock for none charging\n" ); -else - gbpsy->pm_acquired = false; +gb_pm_runtime_put_autosuspend (connection->bundle); + gbpsy->pm_acquired = false; } }include/linux/greybus/bundle.h +3
-9@@ -59,14 +59,9 @@ static inline int gb_pm_runtime_get_sync(struct gb_bundle *bundle)return 0 ; } -static inline int gb_pm_runtime_put_autosuspend (struct gb_bundle *bundle) +static inline void gb_pm_runtime_put_autosuspend (struct gb_bundle *bundle) { -int retval; - -pm_runtime_mark_last_busy (&bundle->dev); - retval =pm_runtime_put_autosuspend (&bundle->dev); - -return retval; +pm_runtime_put_autosuspend (&bundle->dev); }static inline void gb_pm_runtime_get_noresume (struct gb_bundle *bundle)@@ -82,8 +77,7 @@ static inline void gb_pm_runtime_put_noidle(struct gb_bundle *bundle)#else static inline int gb_pm_runtime_get_sync (struct gb_bundle *bundle) {return 0 ; } -static inline int gb_pm_runtime_put_autosuspend (struct gb_bundle *bundle) -{return 0 ; } +static inline void gb_pm_runtime_put_autosuspend (struct gb_bundle *bundle) {}static inline void gb_pm_runtime_get_noresume (struct gb_bundle *bundle) {}static inline void gb_pm_runtime_put_noidle (struct gb_bundle *bundle) {}
PATCH 8/13 ALSA: hda: Discard pm_runtime_put_autosuspend() return value
In reply to Brian Norrissnd_hdac_power_down() and snd_hdac_power_down_pm() are defined to return an int, but their return values are never used by any caller. snd_hdac_power_down() also passes the return value of pm_runtime_put_autosuspend() to the caller, which is not very useful.
Returning an error code from pm_runtime_put_autosuspend() merely means that it has not queued up a timer or work item to check whether or not the device can be suspended and there are many perfectly valid situations in which that can happen, like after writing "on" to the device's runtime PM "control" attribute in sysfs for one example.
Modify snd_hdac_power_down() to discard the pm_runtime_put_autosuspend() return value and change the return types of snd_hdac_power_down() and snd_hdac_power_down_pm() to void.
No intentional functional impact.
This will facilitate a planned change of the pm_runtime_put_autosuspend() return type to void in the future, similar to commit 3afd8df02433 ("PM: runtime: Change pm_runtime_put() return type to void").
---
This patch is independent of the rest of the series, except for the end (changing the return type). I expect it can be applied by individual maintainers, and we pick up the end once the dust is settled.
include/sound/hdaudio.h | 8 ++++---- sound/hda/core/device.c | 13 ++++--------- 2 files changed, 8 insertions(+), 13 deletions(-)
include/sound/hdaudio.h +4 -4
@@ -179,9 +179,9 @@ static inline int snd_hdac_read_parm(struct hdac_device *codec, hda_nid_t nid,#ifdef CONFIG_PM int snd_hdac_power_up (struct hdac_device *codec); -int snd_hdac_power_down (struct hdac_device *codec); +void snd_hdac_power_down (struct hdac_device *codec);int snd_hdac_power_up_pm (struct hdac_device *codec); -int snd_hdac_power_down_pm (struct hdac_device *codec); +void snd_hdac_power_down_pm (struct hdac_device *codec);int snd_hdac_keep_power_up (struct hdac_device *codec);/* call this at entering into suspend/resume callbacks in codec driver */
@@ -207,9 +207,9 @@ static inline bool snd_hdac_is_power_on(struct hdac_device *codec) }#else static inline int snd_hdac_power_up (struct hdac_device *codec) {return 0 ; } -static inline int snd_hdac_power_down (struct hdac_device *codec) {return 0 ; } +static inline void snd_hdac_power_down (struct hdac_device *codec) {}static inline int snd_hdac_power_up_pm (struct hdac_device *codec) {return 0 ; } -static inline int snd_hdac_power_down_pm (struct hdac_device *codec) {return 0 ; } +static inline void snd_hdac_power_down_pm (struct hdac_device *codec) {}static inline int snd_hdac_keep_power_up (struct hdac_device *codec) {return 0 ; }static inline void snd_hdac_enter_pm (struct hdac_device *codec) {}static inline void snd_hdac_leave_pm (struct hdac_device *codec) {}
sound/hda/core/device.c +4 -9
@@ -572,14 +572,12 @@ EXPORT_SYMBOL_GPL(snd_hdac_power_up);/** * snd_hdac_power_down - power down the codec * @codec: the codec object -* -* Returns zero if successful, or a negative error code. */ -int snd_hdac_power_down (struct hdac_device *codec) +void snd_hdac_power_down (struct hdac_device *codec) {struct device *dev = &codec->dev; -return pm_runtime_put_autosuspend (dev); +pm_runtime_put_autosuspend (dev); }EXPORT_SYMBOL_GPL (snd_hdac_power_down);
@@ -624,14 +622,11 @@ int snd_hdac_keep_power_up(struct hdac_device *codec)* * Like snd_hdac_power_up_pm(), this function is used in a recursive * code path like init code which may be called by PM suspend/resume again. -* -* Returns zero if successful, or a negative error code. */ -int snd_hdac_power_down_pm (struct hdac_device *codec) +void snd_hdac_power_down_pm (struct hdac_device *codec) {if (atomic_dec_if_positive (&codec->in_pm) <0 ) -return snd_hdac_power_down (codec); -return 0 ; +snd_hdac_power_down (codec); }EXPORT_SYMBOL_GPL (snd_hdac_power_down_pm);#endif
Signature · 1 line
2.56.0.360.g66cac248cb-goog
Re: [PATCH 08/13] ALSA: hda: Discard pm_runtime_put_autosuspend() return value
Quoted 23 lines · Brian Norris
snd_hdac_power_down()andsnd_hdac_power_down_pm()are defined to return an int, but their return values are never used by any caller.snd_hdac_power_down()also passes the return value ofpm_runtime_put_autosuspend()to the caller, which is not very useful.Returning an error code from
pm_runtime_put_autosuspend()merely means that it has not queued up a timer or work item to check whether or not the device can be suspended and there are many perfectly valid situations in which that can happen, like after writing "on" to the device's runtime PM "control" attribute in sysfs for one example.Modify
snd_hdac_power_down()to discard thepm_runtime_put_autosuspend()return value and change the return types ofsnd_hdac_power_down()andsnd_hdac_power_down_pm()to void.No intentional functional impact.
This will facilitate a planned change of the
pm_runtime_put_autosuspend()return type to void in the future, similar to commit3afd8df02433("PM: runtime: Changepm_runtime_put()return type to void").
Quoted 5 lines
Signed-off-by Brian Norris <[email protected]>---
This patch is independent of the rest of the series, except for the end (changing the return type). I expect it can be applied by individual maintainers, and we pick up the end once the dust is settled.
OK, I applied this one to sound.git tree for-next branch now.
thanks,
Takashi
PATCH 9/13 ASoC: codecs: lpass: Discard pm_runtime_put_autosuspend() return values
In reply to Brian NorrisPrinting warning messages on pm_runtime_put_autosuspend() returning negative values is not particularly useful.
Returning an error code from pm_runtime_put_autosuspend() merely means that it has not queued up a timer or work item to check whether or not the device can be suspended and there are many perfectly valid situations in which that can happen, like after writing "on" to the device's runtime PM "control" attribute in sysfs for one example.
Accordingly, update the LPASS macro codec drivers to simply discard the return value of pm_runtime_put_autosuspend().
This will facilitate a planned change of the pm_runtime_put_autosuspend() return type to void in the future, similar to commit 3afd8df02433 ("PM: runtime: Change pm_runtime_put() return type to void").
---
This patch is independent of the rest of the series, except for the end (changing the return type). I expect it can be applied by individual maintainers, and we pick up the end once the dust is settled.
sound/soc/codecs/lpass-rx-macro.c | 4 +--- sound/soc/codecs/lpass-tx-macro.c | 4 +--- sound/soc/codecs/lpass-va-macro.c | 17 ++++------------- sound/soc/codecs/lpass-wsa-macro.c | 8 ++------ 4 files changed, 8 insertions(+), 25 deletions(-)
sound/soc/codecs/lpass-rx-macro.c +1 -3
@@ -3917,9 +3917,7 @@ static int rx_macro_probe(struct platform_device *pdev)if (ret) goto err_rpm_put; - ret =pm_runtime_put_autosuspend (dev); -if (ret <0 ) -dev_warn (dev,"runtime PM put failed after probe: %d\n" , ret); +pm_runtime_put_autosuspend (dev);return 0 ;
sound/soc/codecs/lpass-tx-macro.c +1 -3
@@ -2370,9 +2370,7 @@ static int tx_macro_probe(struct platform_device *pdev)if (ret) goto err_rpm_put; - ret =pm_runtime_put_autosuspend (dev); -if (ret <0 ) -dev_warn (dev,"runtime PM put failed after probe: %d\n" , ret); +pm_runtime_put_autosuspend (dev);return 0 ;
sound/soc/codecs/lpass-va-macro.c +4 -13
@@ -1352,7 +1352,7 @@ static int fsgen_gate_enable(struct clk_hw *hw) {struct va_macro *va =to_va_macro (hw);struct regmap *regmap = va->regmap; -int ret, rpm_ret; +int ret; ret =pm_runtime_resume_and_get (va->dev);if (ret <0 )
@@ -1360,11 +1360,7 @@ static int fsgen_gate_enable(struct clk_hw *hw) ret =va_macro_mclk_enable (va, true);if (ret) { - rpm_ret =pm_runtime_put_autosuspend (va->dev); -if (rpm_ret <0 ) -dev_warn (va->dev, -"runtime PM put failed in fsgen enable unwind: %d\n" , - rpm_ret); +pm_runtime_put_autosuspend (va->dev);return ret; }if (va->has_swr_master)
@@ -1378,7 +1374,6 @@ static void fsgen_gate_disable(struct clk_hw *hw) {struct va_macro *va =to_va_macro (hw);struct regmap *regmap = va->regmap; -int ret;if (va->has_swr_master) regmap_update_bits(regmap, CDC_VA_CLK_RST_CTRL_SWR_CONTROL,
@@ -1386,9 +1381,7 @@ static void fsgen_gate_disable(struct clk_hw *hw)va_macro_mclk_enable (va, false); - ret =pm_runtime_put_autosuspend (va->dev); -if (ret <0 ) -dev_warn (va->dev,"runtime PM put failed in fsgen disable: %d\n" , ret); +pm_runtime_put_autosuspend (va->dev); }static int fsgen_gate_is_enabled (struct clk_hw *hw)
@@ -1693,9 +1686,7 @@ static int va_macro_probe(struct platform_device *pdev) goto err_rpm_put; } - rpm_ret =pm_runtime_put_autosuspend (dev); -if (rpm_ret <0 ) -dev_warn (dev,"runtime PM put failed after probe: %d\n" , rpm_ret); +pm_runtime_put_autosuspend (dev);return 0 ;
sound/soc/codecs/lpass-wsa-macro.c +2 -6
@@ -2548,9 +2548,7 @@ static int wsa_swrm_clock(struct wsa_macro *wsa, bool enable) CDC_WSA_SWR_CLK_EN_MASK, 0);wsa_macro_mclk_enable (wsa, false); - ret =pm_runtime_put_autosuspend (wsa->dev); -if (ret <0 ) -dev_warn (wsa->dev,"runtime PM put failed: %d\n" , ret); +pm_runtime_put_autosuspend (wsa->dev); }return 0 ;
@@ -2820,9 +2818,7 @@ static int wsa_macro_probe(struct platform_device *pdev)if (ret) goto err_rpm_put; - ret =pm_runtime_put_autosuspend (dev); -if (ret <0 ) -dev_warn (dev,"runtime PM put failed after probe: %d\n" , ret); +pm_runtime_put_autosuspend (dev);return 0 ; err_rpm_put:
Signature · 1 line
2.56.0.360.g66cac248cb-goog
PATCH 10/13 ASoC: intel: atom: Discard pm_runtime_put_autosuspend() return value
In reply to Brian NorrisPropagating the pm_runtime_put_autosuspend() return value to callers is not particularly useful.
Returning an error code from pm_runtime_put_autosuspend() merely means that it has not queued up a timer or work item to check whether or not the device can be suspended and there are many perfectly valid situations in which that can happen, like after writing "on" to the device's runtime PM "control" attribute in sysfs for one example.
Modify sst_pm_runtime_put() to discard the pm_runtime_put_autosuspend() return value, change its return type to void, and update its callers in sst_power_control() accordingly.
Note that in one error path for sst_power_control(), we would previously overwrite one error code (from "FW download fail") with the return code from pm_runtime_put_autosuspend(). This was probably an unexpected behavior, which is now changed to propagate the sst_load_fw() error code.
This will facilitate a planned change of the pm_runtime_put_autosuspend() return type to void in the future, similar to commit 3afd8df02433 ("PM: runtime: Change pm_runtime_put() return type to void").
---
This patch is independent of the rest of the series, except for the end (changing the return type). I expect it can be applied by individual maintainers, and we pick up the end once the dust is settled.
sound/soc/intel/atom/sst/sst.h | 2 +- sound/soc/intel/atom/sst/sst_drv_interface.c | 4 ++-- sound/soc/intel/atom/sst/sst_pvt.c | 9 ++------- 3 files changed, 5 insertions(+), 10 deletions(-)
sound/soc/intel/atom/sst/sst.h +1 -1
@@ -497,7 +497,7 @@ int get_stream_id_mrfld(struct intel_sst_drv *sst_drv_ctx,u32 relocate_imr_addr_mrfld (u32 base_addr);void sst_add_to_dispatch_list_and_post (struct intel_sst_drv *sst,struct ipc_post *msg); -int sst_pm_runtime_put (struct intel_sst_drv *sst_drv); +void sst_pm_runtime_put (struct intel_sst_drv *sst_drv);int sst_shim_write (void __iomem *addr,int offset,int value);u32 sst_shim_read (void __iomem *addr,int offset);u64 sst_reg_read64 (void __iomem *addr,int offset);
sound/soc/intel/atom/sst/sst_drv_interface.c +2 -2
@@ -135,13 +135,13 @@ static int sst_power_control(struct device *dev, bool state)if (ret) {dev_err (dev,"FW download fail %d\n" , ret);sst_set_fw_state_locked (ctx,SST_RESET ); - ret =sst_pm_runtime_put (ctx); +sst_pm_runtime_put (ctx); } } }else { usage_count =GET_USAGE_COUNT (dev);dev_dbg (ctx->dev,"Disable: pm usage count: %d\n" , usage_count); -return sst_pm_runtime_put (ctx); +sst_pm_runtime_put (ctx); }return ret; }
sound/soc/intel/atom/sst/sst_pvt.c +2 -7
@@ -255,14 +255,9 @@ int sst_prepare_and_post_msg(struct intel_sst_drv *sst,return ret; } -int sst_pm_runtime_put (struct intel_sst_drv *sst_drv) +void sst_pm_runtime_put (struct intel_sst_drv *sst_drv) { -int ret; - - ret =pm_runtime_put_autosuspend (sst_drv->dev); -if (ret <0 ) -return ret; -return 0 ; +pm_runtime_put_autosuspend (sst_drv->dev); } void sst_fill_header_mrfld(union ipc_header_mrfld *header,
Signature · 1 line
2.56.0.360.g66cac248cb-goog
Re: [PATCH 10/13] ASoC: intel: atom: Discard pm_runtime_put_autosuspend() return value
Quoted 24 lines · Brian Norris
Propagating the
pm_runtime_put_autosuspend()return value to callers is not particularly useful.Returning an error code from
pm_runtime_put_autosuspend()merely means that it has not queued up a timer or work item to check whether or not the device can be suspended and there are many perfectly valid situations in which that can happen, like after writing "on" to the device's runtime PM "control" attribute in sysfs for one example.Modify
sst_pm_runtime_put()to discard thepm_runtime_put_autosuspend()return value, change its return type to void, and update its callers insst_power_control()accordingly.Note that in one error path for
sst_power_control(), we would previously overwrite one error code (from "FW download fail") with the return code frompm_runtime_put_autosuspend(). This was probably an unexpected behavior, which is now changed to propagate thesst_load_fw()error code.This will facilitate a planned change of the
pm_runtime_put_autosuspend()return type to void in the future, similar to commit3afd8df02433("PM: runtime: Changepm_runtime_put()return type to void").
Quoted 5 lines
Signed-off-by Brian Norris <[email protected]>---
This patch is independent of the rest of the series, except for the end (changing the return type). I expect it can be applied by individual maintainers, and we pick up the end once the dust is settled.
PATCH 11/13 ASoC: SOF: Discard pm_runtime_put_autosuspend() return values
In reply to Brian NorrisPrinting error messages on pm_runtime_put_autosuspend() returning negative values is not particularly useful.
Returning an error code from pm_runtime_put_autosuspend() merely means that it has not queued up a timer or work item to check whether or not the device can be suspended and there are many perfectly valid situations in which that can happen, like after writing "on" to the device's runtime PM "control" attribute in sysfs for one example.
Accordingly, update the SOF driver to simply discard the return value of pm_runtime_put_autosuspend().
This will facilitate a planned change of the pm_runtime_put_autosuspend() return type to void in the future, similar to commit 3afd8df02433 ("PM: runtime: Change pm_runtime_put() return type to void").
---
This patch is independent of the rest of the series, except for the end (changing the return type). I expect it can be applied by individual maintainers, and we pick up the end once the dust is settled.
sound/soc/sof/control.c | 6 ++---- sound/soc/sof/sof-client-ipc-flood-test.c | 5 +---- sound/soc/sof/sof-client-ipc-kernel-injector.c | 4 +--- sound/soc/sof/sof-client-ipc-msg-injector.c | 6 ++---- sound/soc/sof/sof-client-probes.c | 18 ++++++------------ 5 files changed, 12 insertions(+), 27 deletions(-)
sound/soc/sof/control.c +2 -4
@@ -185,7 +185,7 @@ int snd_sof_bytes_ext_volatile_get(struct snd_kcontrol *kcontrol, unsigned int _struct snd_soc_component *scomp = scontrol->scomp;struct snd_sof_dev *sdev =snd_soc_component_get_drvdata (scomp);const struct sof_ipc_tplg_ops *tplg_ops =sof_ipc_get_ops (sdev, tplg); -int ret, err; +int ret;/* ignore the ext_volatile_get call if the callbacks are not provided */ if (!tplg_ops || !tplg_ops->control ||
@@ -205,9 +205,7 @@ int snd_sof_bytes_ext_volatile_get(struct snd_kcontrol *kcontrol, unsigned int _ binary_data, size); - err =pm_runtime_put_autosuspend (scomp->dev); -if (err <0 ) -dev_err_ratelimited (scomp->dev,"%s: failed to idle %d\n" , __func__, err); +pm_runtime_put_autosuspend (scomp->dev);return ret; }
sound/soc/sof/sof-client-ipc-flood-test.c +1 -4
@@ -157,7 +157,6 @@ static ssize_t sof_ipc_flood_dfs_write(struct file *file, const char __user *bufunsigned long ipc_duration_ms =0 ;bool flood_duration_test = false;unsigned long ipc_count =0 ; -int err;char *string;int ret;
@@ -223,9 +222,7 @@ static ssize_t sof_ipc_flood_dfs_write(struct file *file, const char __user *buf ret =sof_debug_ipc_flood_test (cdev, flood_duration_test, ipc_duration_ms, ipc_count); - err =pm_runtime_put_autosuspend (dev); -if (err <0 ) -dev_err_ratelimited (dev,"debugfs write failed to idle %d\n" , err); +pm_runtime_put_autosuspend (dev);/* return count if test is successful */ if (ret >=0 )
sound/soc/sof/sof-client-ipc-kernel-injector.c +1 -3
@@ -66,9 +66,7 @@ static ssize_t sof_kernel_msg_inject_dfs_write(struct file *file, const char __uif (!ret)sof_client_ipc_rx_message (cdev, hdr, priv->kernel_buffer); - ret =pm_runtime_put_autosuspend (dev); -if (ret <0 ) -dev_err_ratelimited (dev,"debugfs write failed to idle %d\n" , ret); +pm_runtime_put_autosuspend (dev);return count; };
sound/soc/sof/sof-client-ipc-msg-injector.c +2 -4
@@ -122,7 +122,7 @@ static int sof_msg_inject_send_message(struct sof_client_dev *cdev) {struct sof_msg_inject_priv *priv = cdev->data;struct device *dev = &cdev->auxdev.dev; -int ret, err; +int ret; ret =pm_runtime_resume_and_get (dev);if (ret <0 && ret != -EACCES ) {
@@ -140,9 +140,7 @@ static int sof_msg_inject_send_message(struct sof_client_dev *cdev)dev_err (dev,"IPC message send failed: %d\n" , ret); } - err =pm_runtime_put_autosuspend (dev); -if (err <0 ) -dev_err_ratelimited (dev,"debugfs write failed to idle %d\n" , err); +pm_runtime_put_autosuspend (dev);return ret; }
sound/soc/sof/sof-client-probes.c +6 -12
@@ -211,7 +211,7 @@ static ssize_t sof_probes_dfs_points_read(struct file *file, char __user *to,int remaining, offset;size_t num_desc;char *buf; -int i, ret, err; +int i, ret;if (priv->extractor_stream_tag ==SOF_PROBES_INVALID_NODE_ID ) {dev_warn (dev,"no extractor stream running\n" );
@@ -258,9 +258,7 @@ static ssize_t sof_probes_dfs_points_read(struct file *file, char __user *to,kfree (desc); pm_error: - err =pm_runtime_put_autosuspend (dev); -if (err <0 ) -dev_err_ratelimited (dev,"debugfs read failed to idle %d\n" , err); +pm_runtime_put_autosuspend (dev); exit:kfree (buf);
@@ -294,7 +292,7 @@ sof_probes_dfs_points_write(struct file *file, const char __user *from,struct sof_probe_point_desc *desc;u32 num_elems, *array;size_t bytes; -int ret, err; +int ret;if (priv->extractor_stream_tag ==SOF_PROBES_INVALID_NODE_ID ) {dev_warn (dev,"no extractor stream running\n" );
@@ -327,9 +325,7 @@ sof_probes_dfs_points_write(struct file *file, const char __user *from, ret = count; } - err =pm_runtime_put_autosuspend (dev); -if (err <0 ) -dev_err_ratelimited (dev,"debugfs write failed to idle %d\n" , err); +pm_runtime_put_autosuspend (dev); exit:kfree (array);return ret;
@@ -360,7 +356,7 @@ sof_probes_dfs_points_remove_write(struct file *file, const char __user *from,struct sof_probes_priv *priv = cdev->data;const struct sof_probes_ipc_ops *ipc = priv->ipc_ops;struct device *dev = &cdev->auxdev.dev; -int ret, err; +int ret;u32 *array;if (priv->extractor_stream_tag ==SOF_PROBES_INVALID_NODE_ID ) {
@@ -385,9 +381,7 @@ sof_probes_dfs_points_remove_write(struct file *file, const char __user *from, ret = count; } - err =pm_runtime_put_autosuspend (dev); -if (err <0 ) -dev_err_ratelimited (dev,"debugfs write failed to idle %d\n" , err); +pm_runtime_put_autosuspend (dev); exit:kfree (array);return ret;
Signature · 1 line
2.56.0.360.g66cac248cb-goog
PATCH 12/13 PM: runtime: Drop pm_runtime_put_autosuspend() return code
In reply to Brian NorrisThe primary role of pm_runtime_put_autosuspend() is to update the last access time and decrement the runtime PM usage counter of the given device. It always does that regardless of the value returned by it later.
In addition, if the runtime PM usage counter after decrementation turns out to be zero, a timer or work item is queued up to check whether or not the device can be suspended once its autosuspend delay has elapsed. This is not guaranteed to succeed though and even if it is successful, the device may still not be suspended going forward.
There are multiple valid reasons why pm_runtime_put_autosuspend() may not decide to queue up the timer or work item mentioned above, including, but not limited to, the case when user space has written "on" to the device's runtime PM "control" file in sysfs. In some of those cases, pm_runtime_put_autosuspend() returns a negative error code (even though the device's runtime PM usage counter has been successfully decremented by it) which is very confusing.
Accordingly, to avoid the confusion mentioned above, change the return types of pm_runtime_put_autosuspend() and __pm_runtime_put_autosuspend() to void, similar to commit 3afd8df02433 ("PM: runtime: Change pm_runtime_put() return type to void").
---
This patch must be applied after all the preceding patches. Presumably that can wait for other maintainers.
include/linux/pm_runtime.h | 32 ++++---------------------------- 1 file changed, 4 insertions(+), 28 deletions(-)
include/linux/pm_runtime.h +4 -28
@@ -593,22 +593,10 @@ static inline void pm_runtime_put(struct device *dev)* * Decrement the runtime PM usage counter of @dev and if it turns out to be * equal to 0, queue up a work item for @dev like in pm_request_autosuspend(). -* -* Return: -* * %1: Success. Usage counter dropped to zero, but device was already suspended. -* * %0: Success. -* * %-EINVAL: Runtime PM error. -* * %-EACCES: Runtime PM disabled. -* * %-EAGAIN: Runtime PM usage counter became non-zero or Runtime PM status -* change ongoing. -* * %-EBUSY: Runtime PM child_count non-zero. -* * %-EPERM: Device PM QoS resume latency 0. -* * %-EINPROGRESS: Suspend already in progress. -* * %-ENOSYS: %CONFIG_PM not enabled. */ -static inline int __pm_runtime_put_autosuspend (struct device *dev) +static inline void __pm_runtime_put_autosuspend (struct device *dev) { -return __pm_runtime_suspend (dev,RPM_GET_PUT |RPM_ASYNC |RPM_AUTO ); +__pm_runtime_suspend (dev,RPM_GET_PUT |RPM_ASYNC |RPM_AUTO ); }/**
@@ -619,23 +607,11 @@ static inline int __pm_runtime_put_autosuspend(struct device *dev)* Update the last access time of @dev and decrement its runtime PM usage * counter. If the counter drops to zero, asynchronously request the PM core to * suspend the device once its autosuspend delay has elapsed. -* -* Return: -* * %1: Success. Usage counter dropped to zero, but device was already suspended. -* * %0: Success. -* * %-EINVAL: Runtime PM error. -* * %-EACCES: Runtime PM disabled. -* * %-EAGAIN: Runtime PM usage counter became non-zero or Runtime PM status -* change ongoing. -* * %-EBUSY: Runtime PM child_count non-zero. -* * %-EPERM: Device PM QoS resume latency 0. -* * %-EINPROGRESS: Suspend already in progress. -* * %-ENOSYS: %CONFIG_PM not enabled. */ -static inline int pm_runtime_put_autosuspend (struct device *dev) +static inline void pm_runtime_put_autosuspend (struct device *dev) {pm_runtime_mark_last_busy (dev); -return __pm_runtime_put_autosuspend (dev); +__pm_runtime_put_autosuspend (dev); } DEFINE_GUARD(pm_runtime_noresume, struct device *,
Signature · 1 line
2.56.0.360.g66cac248cb-goog
PATCH 13/13 PM: runtime: Drop pm_request_{idle,autosuspend}() return codes
In reply to Brian NorrisNo in-tree callers (outside of KUnit tests) use the return values of pm_request_idle() or pm_request_autosuspend().
As with pm_runtime_put() and pm_runtime_put_autosuspend(), these functions asynchronously queue up a timer or work item to check whether or not the device can be suspended, which is not guaranteed to succeed and may return a negative error code in many valid situations (for example, when user space has written "on" to the device's runtime PM "control" file in sysfs, or when the device's usage counter is non-zero).
Accordingly, change the return types of pm_request_idle() and pm_request_autosuspend() to void, similar to commit 3afd8df02433 ("PM:
runtime: Change pm_runtime_put() return type to void").
---
This patch is independent of the rest and could be applied on its own. I put in this series because it has the same justification as the more noisy one, "PM: runtime: Drop pm_runtime_put_autosuspend() return code"
drivers/base/power/runtime-test.c | 8 -------- include/linux/pm_runtime.h | 31 ++++--------------------------- 2 files changed, 4 insertions(+), 35 deletions(-)
drivers/base/power/runtime-test.c +0 -8
@@ -43,7 +43,6 @@ static void pm_runtime_already_suspended_test(struct kunit *test)KUNIT_EXPECT_EQ (test,1 ,pm_runtime_suspend (dev));KUNIT_EXPECT_EQ (test,1 ,pm_runtime_autosuspend (dev)); -KUNIT_EXPECT_EQ (test,1 ,pm_request_autosuspend (dev));pm_runtime_get_noresume (dev);KUNIT_EXPECT_EQ (test,1 ,pm_runtime_put_sync_autosuspend (dev));
@@ -81,7 +80,6 @@ static void pm_runtime_idle_test(struct kunit *test)KUNIT_EXPECT_EQ (test,0 ,pm_runtime_idle (dev));KUNIT_EXPECT_TRUE (test,pm_runtime_suspended (dev));KUNIT_EXPECT_EQ (test, -EAGAIN ,pm_runtime_idle (dev)); -KUNIT_EXPECT_EQ (test, -EAGAIN ,pm_request_idle (dev)); }static void pm_runtime_disabled_test (struct kunit *test)
@@ -112,9 +110,7 @@ static void pm_runtime_disabled_test(struct kunit *test)KUNIT_EXPECT_EQ (test, -EACCES ,pm_runtime_resume_and_get (dev));KUNIT_EXPECT_EQ (test, -EACCES ,pm_runtime_idle (dev)); -KUNIT_EXPECT_EQ (test, -EACCES ,pm_request_idle (dev));KUNIT_EXPECT_EQ (test, -EACCES ,pm_request_resume (dev)); -KUNIT_EXPECT_EQ (test, -EACCES ,pm_request_autosuspend (dev));KUNIT_EXPECT_EQ (test, -EACCES ,pm_runtime_suspend (dev));KUNIT_EXPECT_EQ (test, -EACCES ,pm_runtime_resume (dev));KUNIT_EXPECT_EQ (test, -EACCES ,pm_runtime_autosuspend (dev));
@@ -154,9 +150,7 @@ static void pm_runtime_error_test(struct kunit *test)KUNIT_EXPECT_EQ (test, -EINVAL ,pm_runtime_resume_and_get (dev));KUNIT_EXPECT_EQ (test, -EINVAL ,pm_runtime_idle (dev)); -KUNIT_EXPECT_EQ (test, -EINVAL ,pm_request_idle (dev));KUNIT_EXPECT_EQ (test, -EINVAL ,pm_request_resume (dev)); -KUNIT_EXPECT_EQ (test, -EINVAL ,pm_request_autosuspend (dev));KUNIT_EXPECT_EQ (test, -EINVAL ,pm_runtime_suspend (dev));KUNIT_EXPECT_EQ (test, -EINVAL ,pm_runtime_resume (dev));KUNIT_EXPECT_EQ (test, -EINVAL ,pm_runtime_autosuspend (dev));
@@ -189,9 +183,7 @@ static void pm_runtime_error_test(struct kunit *test)* (already resumed). */ KUNIT_EXPECT_EQ (test, -EAGAIN ,pm_runtime_idle (dev)); -KUNIT_EXPECT_EQ (test, -EAGAIN ,pm_request_idle (dev));KUNIT_EXPECT_EQ (test,1 ,pm_request_resume (dev)); -KUNIT_EXPECT_EQ (test, -EAGAIN ,pm_request_autosuspend (dev));KUNIT_EXPECT_EQ (test, -EAGAIN ,pm_runtime_suspend (dev));KUNIT_EXPECT_EQ (test,1 ,pm_runtime_resume (dev));KUNIT_EXPECT_EQ (test, -EAGAIN ,pm_runtime_autosuspend (dev));
include/linux/pm_runtime.h +4 -27
@@ -447,21 +447,10 @@ static inline int pm_runtime_resume(struct device *dev)* * Asynchronously request the PM core to evaluate whether @dev can be idled * or suspended, invoking its ->runtime_idle() callback if provided. -* -* Return: -* * %0: Success. -* * %-EINVAL: Runtime PM error. -* * %-EACCES: Runtime PM disabled. -* * %-EAGAIN: Runtime PM usage counter non-zero, Runtime PM status change -* ongoing or device not in %RPM_ACTIVE state. -* * %-EBUSY: Runtime PM child_count non-zero. -* * %-EPERM: Device PM QoS resume latency 0. -* * %-EINPROGRESS: Suspend already in progress. -* * %-ENOSYS: %CONFIG_PM not enabled. */ -static inline int pm_request_idle (struct device *dev) +static inline void pm_request_idle (struct device *dev) { -return __pm_runtime_idle (dev,RPM_ASYNC ); +__pm_runtime_idle (dev,RPM_ASYNC ); }/**
@@ -487,23 +476,11 @@ static inline int pm_request_resume(struct device *dev)* * Update the last access time of @dev and asynchronously request the PM core * to suspend it after the autosuspend delay has elapsed. -* -* Return: -* * %1: Success; device was already suspended. -* * %0: Success. -* * %-EINVAL: Runtime PM error. -* * %-EACCES: Runtime PM disabled. -* * %-EAGAIN: Runtime PM usage counter non-zero or Runtime PM status change -* ongoing. -* * %-EBUSY: Runtime PM child_count non-zero. -* * %-EPERM: Device PM QoS resume latency 0. -* * %-EINPROGRESS: Suspend already in progress. -* * %-ENOSYS: %CONFIG_PM not enabled. */ -static inline int pm_request_autosuspend (struct device *dev) +static inline void pm_request_autosuspend (struct device *dev) {pm_runtime_mark_last_busy (dev); -return __pm_runtime_suspend (dev,RPM_ASYNC |RPM_AUTO ); +__pm_runtime_suspend (dev,RPM_ASYNC |RPM_AUTO ); }/**
Signature · 1 line
2.56.0.360.g66cac248cb-goog
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 7 lines · Patchwork Integration
Applying: hwrng: cctrng: Discard
pm_runtime_put_autosuspend()return value
Applying: drm: Discardpm_runtime_put_autosuspend()return values
Applying: iio: Discardpm_runtime_put_autosuspend()return values Patch failed at 0003 iio: Discardpm_runtime_put_autosuspend()return values
error: patch failed:drivers/iio/adc/rcar-gyroadc.c:199
error:drivers/iio/adc/rcar-gyroadc.c: patch does not apply
error: patch failed:drivers/iio/light/isl29028.c:395
Quoted 8 lines
error:
drivers/iio/light/isl29028.c: patch does not apply
error: patch failed:drivers/iio/light/tsl2583.c:640
error:drivers/iio/light/tsl2583.c: patch does not apply
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
My bad, I was based on the linux-pm -next tree, and not proper linux-next. On the bright side, some of these conflicts are from others removing the return-code checks during refactoring!
I've rebased, and I'll send v2 at some point, after giving other subsystems a bit to look (and possibly apply) other parts of the series.
Brian
Bartosz Golaszewski
AppliedRe: (subset) [PATCH 00/13] PM: runtime: Convert pm_runtime_put_autosuspend() and friends to a void function
In reply to Brian NorrisQuoted 9 lines · Brian Norris
Similar to this effort:
Subject: [PATCH v1 00/23] PM: runtime: Convert
pm_runtime_put()to a void function
https://lore.kernel.org/all/[email protected]/i.e., commit
3afd8df02433("PM: runtime: Changepm_runtime_put()return type to void") and friends.[...]
Applied, thanks!
[05/13] pinctrl: qcom: lpass-lpi: Discard pm_runtime_put_autosuspend() return values
https://git.kernel.org/brgl/c/9bcd1725b0cc7954525e3d0d02fecaec7c7aab33
Best regards,
Signature · 1 line
Bartosz Golaszewski <[email protected]>