media: use pm_runtime_resume_and_get() instead of pm_runtime_get_sync()

The pm_runtime_get_sync() internally increments the
dev->power.usage_count without decrementing it, even on errors.
replace it by the new pm_runtime_resume_and_get(), introduced by:
commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter") in order to properly decrement the usage counter and avoid memory
leaks.

While here, check if the PM runtime was caught during chipset probing time.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

---
 drivers/media/platform/rockchip/rga/rga.c | 4 +++-

 1 file changed, 3 insertions(+), 1 deletion(-)

The pm_runtime_get_sync() internally increments the dev->power.usage_count without decrementing it, even on errors. replace it by the new pm_runtime_resume_and_get(), introduced by:

commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter") in order to properly decrement the usage counter and avoid memory
leaks.

While here, check if the PM runtime was caught at open time.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

---
 drivers/media/platform/renesas-ceu.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

drivers/media/platform/renesas-ceu.c +0 -0

index cd137101d41e..965a7259e707 100644

drivers/media/platform/renesas-ceu.c +1 -1

+
 	mutex_unlock(&ceudev->mlock);
 
 	return 0;
--

2.30.2

The pm_runtime_get_sync() internally increments the
dev->power.usage_count without decrementing it, even on errors.
replace it by the new pm_runtime_resume_and_get(), introduced by:
commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter") in order to properly decrement the usage counter and avoid memory
leaks.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

Commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter") added pm_runtime_resume_and_get() in order to automatically handle dev->power.usage_count decrement on errors.

Use the new API, in order to cleanup the error check logic.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

---
 drivers/staging/media/hantro/hantro_drv.c | 2 +-

 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/staging/media/hantro/hantro_drv.c +1 -1

index 595e82a82728..3147dcbebeb9 100644
@@ -155,7 +155,7 @@ static void device_run(void *priv)
         ret = clk_bulk_enable(ctx->dev->variant->num_clocks, ctx->dev->clocks);
         if (ret)
                 goto err_cancel_job;
-       ret = pm_runtime_get_sync(ctx->dev->dev);
+       ret = pm_runtime_resume_and_get(ctx->dev->dev);
         if (ret < 0)
                 goto err_cancel_job;

Hi Mauro,

Quoted 11 lines · Mauro Carvalho Chehab

Commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter") added pm_runtime_resume_and_get() in order to automatically handle dev->power.usage_count decrement on errors.

Use the new API, in order to cleanup the error check logic.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

---
 drivers/staging/media/hantro/hantro_drv.c | 2 +-

 1 file changed, 1 insertion(+), 1 deletion(-)

Quoted 12 lines

drivers/staging/media/hantro/hantro_drv.c +1 -1

index 595e82a82728..3147dcbebeb9 100644
@@ -155,7 +155,7 @@ static void device_run(void *priv)
         ret = clk_bulk_enable(ctx->dev->variant->num_clocks, ctx->dev->clocks);
         if (ret)
                 goto err_cancel_job;
-       ret = pm_runtime_get_sync(ctx->dev->dev);
+       ret = pm_runtime_resume_and_get(ctx->dev->dev);
         if (ret < 0)
                 goto err_cancel_job;

Quoted 1 line

Seems this one needs a different fix: err_cancel_job will call hantro_job_finish which has a pm_runtime put.

Em Sat, 24 Apr 2021 20:23:53 -0300 Ezequiel Garcia <[email protected]> escreveu:

Quoted 14 lines

Hi Mauro,

Quoted 11 lines · Mauro Carvalho Chehab

Commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter") added pm_runtime_resume_and_get() in order to automatically handle dev->power.usage_count decrement on errors.

Use the new API, in order to cleanup the error check logic.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

---
 drivers/staging/media/hantro/hantro_drv.c | 2 +-

 1 file changed, 1 insertion(+), 1 deletion(-)

Quoted 16 lines

Quoted 12 lines

drivers/staging/media/hantro/hantro_drv.c +1 -1

index 595e82a82728..3147dcbebeb9 100644
@@ -155,7 +155,7 @@ static void device_run(void *priv)
         ret = clk_bulk_enable(ctx->dev->variant->num_clocks, ctx->dev->clocks);
         if (ret)
                 goto err_cancel_job;
-       ret = pm_runtime_get_sync(ctx->dev->dev);
+       ret = pm_runtime_resume_and_get(ctx->dev->dev);
         if (ret < 0)
                 goto err_cancel_job;

Quoted 1 line

Seems this one needs a different fix: err_cancel_job will call hantro_job_finish which has a pm_runtime put.

Good point. Thanks for reviewing it!

It sounds that this is a place where the best seems to keep using pm_runtime_get_sync(), but let's at least add a comment explaining why it should be kept here. This should help to avoid people to copy-and-paste the code on situations where pm_runtime_resume_and_get() should be used instead.

See enclosed patch.

Thanks,
Mauro

[PATCH] media: hantro: document the usage of pm_runtime_get_sync()

Despite other *_get()/*_put() functions, where usage count is incremented only if not errors, the pm_runtime_get_sync() has a different behavior, incrementing the counter *even* on errors.

That's an error prone behavior, as people often forget to decrement the usage counter.

However, the hantro driver depends on this behavior, as it will decrement the usage_count unconditionally at the m2m job finish time, which makes sense.

So, intead of using the pm_runtime_resume_and_get() that would decrement the counter on error, keep the current API, but add a documentation explaining the rationale for keep using pm_runtime_get_sync().

Signed-off-by Mauro Carvalho Chehab <[email protected]>

drivers/staging/media/hantro/hantro_drv.c +0 -0

index 595e82a82728..96f940c1c85c 100644
@@ -155,6 +155,13 @@ static void device_run(void *priv)
 	ret = clk_bulk_enable(ctx->dev->variant->num_clocks, ctx->dev->clocks);
 	if (ret)
 		goto err_cancel_job;

Commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter") added pm_runtime_resume_and_get() in order to automatically handle dev->power.usage_count decrement on errors.

Use the new API, in order to cleanup the error check logic.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

---
 drivers/staging/media/imx/imx7-mipi-csis.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

drivers/staging/media/imx/imx7-mipi-csis.c +3 -4

index 025fdc488bd6..1dc680d94a46 100644
@@ -695,11 +695,10 @@ static int mipi_csis_s_stream(struct v4l2_subdev *mipi_sd, int enable)
 
 		mipi_csis_clear_counters(state);
 
-		ret = pm_runtime_get_sync(&state->pdev->dev);
-		if (ret < 0) {
-			pm_runtime_put_noidle(&state->pdev->dev);
+		ret = pm_runtime_resume_and_get(&state->pdev->dev);
+		if (ret < 0)
 			return ret;
-		}
+
 		ret = v4l2_subdev_call(state->src_sd, core, s_power, 1);
 		if (ret < 0 && ret != -ENOIOCTLCMD)
 			goto done;

Signature · 1 line

2.30.2

Commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter") added pm_runtime_resume_and_get() in order to automatically handle dev->power.usage_count decrement on errors.

Use the new API, in order to cleanup the error check logic.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

---
 drivers/staging/media/tegra-vde/vde.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

drivers/staging/media/tegra-vde/vde.c +8 -5

index 28845b5bafaf..8936f140a246 100644
@@ -775,9 +775,9 @@ static int tegra_vde_ioctl_decode_h264(struct tegra_vde *vde,
 	if (ret)
 		goto release_dpb_frames;
 
-	ret = pm_runtime_get_sync(dev);
+	ret = pm_runtime_resume_and_get(dev);
 	if (ret < 0)
-		goto put_runtime_pm;
+		goto unlock;
 
 	/*
 	 * We rely on the VDE registers reset value, otherwise VDE

@@ -843,6 +843,8 @@ static int tegra_vde_ioctl_decode_h264(struct tegra_vde *vde,
 put_runtime_pm:
 	pm_runtime_mark_last_busy(dev);
 	pm_runtime_put_autosuspend(dev);
+
+unlock:
 	mutex_unlock(&vde->lock);
 
 release_dpb_frames:

@@ -1069,8 +1071,8 @@ static int tegra_vde_probe(struct platform_device *pdev)
 	 * power-cycle it in order to put hardware into a predictable lower
 	 * power state.
 	 */
-	pm_runtime_get_sync(dev);
-	pm_runtime_put(dev);
+	if (pm_runtime_resume_and_get(dev) >= 0)
+		pm_runtime_put(dev);
 
 	return 0;
 

@@ -1088,8 +1090,9 @@ static int tegra_vde_remove(struct platform_device *pdev)
 {
 	struct tegra_vde *vde = platform_get_drvdata(pdev);
 	struct device *dev = &pdev->dev;
+	int ret;
 
-	pm_runtime_get_sync(dev);
+	ret = pm_runtime_resume_and_get(dev);
 	pm_runtime_dont_use_autosuspend(dev);
 	pm_runtime_disable(dev);
 

24.04.2021 09:44, Mauro Carvalho Chehab пишет:

Quoted 67 lines

Commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter") added pm_runtime_resume_and_get() in order to automatically handle dev->power.usage_count decrement on errors.

Use the new API, in order to cleanup the error check logic.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

---
 drivers/staging/media/tegra-vde/vde.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

drivers/staging/media/tegra-vde/vde.c +10 -6

index 28845b5bafaf..8936f140a246 100644
@@ -775,9 +775,9 @@ static int tegra_vde_ioctl_decode_h264(struct tegra_vde *vde,
 	if (ret)
 		goto release_dpb_frames;
 
-	ret = pm_runtime_get_sync(dev);
+	ret = pm_runtime_resume_and_get(dev);
 	if (ret < 0)
-		goto put_runtime_pm;
+		goto unlock;
 
 	/*
 	 * We rely on the VDE registers reset value, otherwise VDE

@@ -843,6 +843,8 @@ static int tegra_vde_ioctl_decode_h264(struct tegra_vde *vde,
 put_runtime_pm:
 	pm_runtime_mark_last_busy(dev);
 	pm_runtime_put_autosuspend(dev);
+
+unlock:
 	mutex_unlock(&vde->lock);
 
 release_dpb_frames:

@@ -1069,8 +1071,8 @@ static int tegra_vde_probe(struct platform_device *pdev)
 	 * power-cycle it in order to put hardware into a predictable lower
 	 * power state.
 	 */
-	pm_runtime_get_sync(dev);
-	pm_runtime_put(dev);
+	if (pm_runtime_resume_and_get(dev) >= 0)
+		pm_runtime_put(dev);
 
 	return 0;
 

@@ -1088,8 +1090,9 @@ static int tegra_vde_remove(struct platform_device *pdev)
 {
 	struct tegra_vde *vde = platform_get_drvdata(pdev);
 	struct device *dev = &pdev->dev;
+	int ret;
 
-	pm_runtime_get_sync(dev);
+	ret = pm_runtime_resume_and_get(dev);
 	pm_runtime_dont_use_autosuspend(dev);
 	pm_runtime_disable(dev);
 

@@ -1097,7 +1100,8 @@ static int tegra_vde_remove(struct platform_device *pdev)
 	 * Balance RPM state, the VDE power domain is left ON and hardware
 	 * is clock-gated. It's safe to reboot machine now.
 	 */
-	pm_runtime_put_noidle(dev);
+	if (ret >= 0)
+		pm_runtime_put_noidle(dev);
 	clk_disable_unprepare(vde->clk);
 

drivers/staging/media/tegra-vde/vde.c static int tegra_vde_remove(struct platform_device *pdev) · line 1107

Quoted 2 lines

drivers/staging/media/tegra-vde/vde.c +0 -0

 	misc_deregister(&vde->miscdev);

Hello Mauro,

Thank you very much for the patch. It looks to me that the original variant was a bit simpler, this patch adds more code lines without changing the previous behaviour. Or am I missing something?

Hi Dmitry,

Em Sat, 24 Apr 2021 10:35:22 +0300
Dmitry Osipenko <[email protected]> escreveu:

Quoted 76 lines

24.04.2021 09:44, Mauro Carvalho Chehab пишет:

Quoted 60 lines

Commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter") added pm_runtime_resume_and_get() in order to automatically handle dev->power.usage_count decrement on errors.

Use the new API, in order to cleanup the error check logic.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

---
 drivers/staging/media/tegra-vde/vde.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

drivers/staging/media/tegra-vde/vde.c +8 -5

index 28845b5bafaf..8936f140a246 100644
@@ -775,9 +775,9 @@ static int tegra_vde_ioctl_decode_h264(struct tegra_vde *vde,
 	if (ret)
 		goto release_dpb_frames;
 
-	ret = pm_runtime_get_sync(dev);
+	ret = pm_runtime_resume_and_get(dev);
 	if (ret < 0)
-		goto put_runtime_pm;
+		goto unlock;
 
 	/*
 	 * We rely on the VDE registers reset value, otherwise VDE

@@ -843,6 +843,8 @@ static int tegra_vde_ioctl_decode_h264(struct tegra_vde *vde,
 put_runtime_pm:
 	pm_runtime_mark_last_busy(dev);
 	pm_runtime_put_autosuspend(dev);
+
+unlock:
 	mutex_unlock(&vde->lock);
 
 release_dpb_frames:

@@ -1069,8 +1071,8 @@ static int tegra_vde_probe(struct platform_device *pdev)
 	 * power-cycle it in order to put hardware into a predictable lower
 	 * power state.
 	 */
-	pm_runtime_get_sync(dev);
-	pm_runtime_put(dev);
+	if (pm_runtime_resume_and_get(dev) >= 0)
+		pm_runtime_put(dev);
 
 	return 0;
 

@@ -1088,8 +1090,9 @@ static int tegra_vde_remove(struct platform_device *pdev)
 {
 	struct tegra_vde *vde = platform_get_drvdata(pdev);
 	struct device *dev = &pdev->dev;
+	int ret;
 
-	pm_runtime_get_sync(dev);
+	ret = pm_runtime_resume_and_get(dev);
 	pm_runtime_dont_use_autosuspend(dev);
 	pm_runtime_disable(dev);
 

drivers/staging/media/tegra-vde/vde.c static int tegra_vde_remove(struct platform_device *pdev) · line 1107

Quoted 9 lines

drivers/staging/media/tegra-vde/vde.c +2 -1

@@ -1097,7 +1100,8 @@ static int tegra_vde_remove(struct platform_device *pdev)
 	 * Balance RPM state, the VDE power domain is left ON and hardware
 	 * is clock-gated. It's safe to reboot machine now.
 	 */
-	pm_runtime_put_noidle(dev);
+	if (ret >= 0)
+		pm_runtime_put_noidle(dev);
 	clk_disable_unprepare(vde->clk);
 
 	misc_deregister(&vde->miscdev);

Hello Mauro,

Thank you very much for the patch. It looks to me that the original variant was a bit simpler, this patch adds more code lines without changing the previous behaviour. Or am I missing something?

While on several places the newer code is simpler, the end goal here is to replace all occurrences of pm_runtime_get_sync() from the media subsystem, due to the number of problems we're having with this:

1. despite its name, this is actually a PM runtime resume call, but some developers didn't seem to realize that, as I got this pattern on some drivers:

        pm_runtime_get_sync(&client->dev);
        pm_runtime_disable(&client->dev);
        pm_runtime_set_suspended(&client->dev);
        pm_runtime_put_noidle(&client->dev);

It makes no sense to resume PM just to suspend it again ;-)

The name of the new variant is a lot clearer:
pm_runtime_resume_and_get()

2. Usual *_get() methods only increment their use count on success, but pm_runtime_get_sync() increments it unconditionally. Due to that, several drivers were mistakenly not calling pm_runtime_put_noidle() when it fails;

3. Consistency: we did similar changes subsystem wide with for instance strlcpy() and strcpy() that got replaced by strscpy(). Having all drivers using the same known-to-be-safe methods is a good thing;

4. Prevent newer drivers to copy-and-paste a code that it would be easier to break if they don't truly understand what's behind the scenes.

The pm_runtime_get_sync() internally increments the dev->power.usage_count without decrementing it, even on errors. replace it by the new pm_runtime_resume_and_get(), introduced by:

commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter") in order to properly decrement the usage counter and avoid memory
leaks.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

Commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter") added pm_runtime_resume_and_get() in order to automatically handle dev->power.usage_count decrement on errors.

Use the new API, in order to cleanup the error check logic.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

---
 drivers/media/i2c/ccs/ccs-core.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

drivers/media/i2c/ccs/ccs-core.c +4 -5

index 9dc3f45da3dc..1441ddcc9b35 100644
@@ -1880,12 +1880,11 @@ static int ccs_pm_get_init(struct ccs_sensor *sensor)
 	struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
 	int rval;
 
-	rval = pm_runtime_get_sync(&client->dev);
-	if (rval < 0) {
-		pm_runtime_put_noidle(&client->dev);
-
+	rval = pm_runtime_resume_and_get(&client->dev);
+	if (rval < 0)
 		return rval;
-	} else if (!rval) {
+
+	if (!rval) {
 		rval = v4l2_ctrl_handler_setup(&sensor->pixel_array->
 					       ctrl_handler);
 		if (rval)

Hi Mauro,

Thanks for the patch.

Quoted 36 lines · Mauro Carvalho Chehab

Commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter") added pm_runtime_resume_and_get() in order to automatically handle dev->power.usage_count decrement on errors.

Use the new API, in order to cleanup the error check logic.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

---
 drivers/media/i2c/ccs/ccs-core.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

drivers/media/i2c/ccs/ccs-core.c +4 -5

index 9dc3f45da3dc..1441ddcc9b35 100644
@@ -1880,12 +1880,11 @@ static int ccs_pm_get_init(struct ccs_sensor *sensor)
 	struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
 	int rval;
 
-	rval = pm_runtime_get_sync(&client->dev);
-	if (rval < 0) {
-		pm_runtime_put_noidle(&client->dev);
-
+	rval = pm_runtime_resume_and_get(&client->dev);
+	if (rval < 0)
 		return rval;
-	} else if (!rval) {
+
+	if (!rval) {
 		rval = v4l2_ctrl_handler_setup(&sensor->pixel_array->
 					       ctrl_handler);
 		if (rval)

@@ -3089,7 +3088,7 @@ static int __maybe_unused ccs_suspend(struct device *dev)
 	bool streaming = sensor->streaming;
 	int rval;
 

drivers/media/i2c/ccs/ccs-core.c static int __maybe_unused ccs_suspend(struct device *dev) · line 3093

Quoted 4 lines

drivers/media/i2c/ccs/ccs-core.c +1 -1

-	rval = pm_runtime_get_sync(dev);
+	rval = pm_runtime_resume_and_get(dev);
 	if (rval < 0) {
 		pm_runtime_put_noidle(dev);  

You'll need to drop pm_runtime_put_noidle() here.

Em Sun, 25 Apr 2021 21:55:25 +0300
Sakari Ailus <[email protected]> escreveu:

Quoted 41 lines

Hi Mauro,

Thanks for the patch.

Quoted 36 lines · Mauro Carvalho Chehab

Commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter") added pm_runtime_resume_and_get() in order to automatically handle dev->power.usage_count decrement on errors.

Use the new API, in order to cleanup the error check logic.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

---
 drivers/media/i2c/ccs/ccs-core.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

drivers/media/i2c/ccs/ccs-core.c +4 -5

index 9dc3f45da3dc..1441ddcc9b35 100644
@@ -1880,12 +1880,11 @@ static int ccs_pm_get_init(struct ccs_sensor *sensor)
 	struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
 	int rval;
 
-	rval = pm_runtime_get_sync(&client->dev);
-	if (rval < 0) {
-		pm_runtime_put_noidle(&client->dev);
-
+	rval = pm_runtime_resume_and_get(&client->dev);
+	if (rval < 0)
 		return rval;
-	} else if (!rval) {
+
+	if (!rval) {
 		rval = v4l2_ctrl_handler_setup(&sensor->pixel_array->
 					       ctrl_handler);
 		if (rval)

@@ -3089,7 +3088,7 @@ static int __maybe_unused ccs_suspend(struct device *dev)
 	bool streaming = sensor->streaming;
 	int rval;
 

Quoted 6 lines

drivers/media/i2c/ccs/ccs-core.c static int __maybe_unused ccs_suspend(struct device *dev) · line 3093

Quoted 4 lines

drivers/media/i2c/ccs/ccs-core.c +1 -1

-	rval = pm_runtime_get_sync(dev);
+	rval = pm_runtime_resume_and_get(dev);
 	if (rval < 0) {
 		pm_runtime_put_noidle(dev);  

You'll need to drop pm_runtime_put_noidle() here.

OK!

---

On a non-related issue at the same code, after the change, the suspend function will be:

  static int __maybe_unused ccs_suspend(struct device *dev)
  {
        struct i2c_client *client = to_i2c_client(dev);
        struct v4l2_subdev *subdev = i2c_get_clientdata(client);
        struct ccs_sensor *sensor = to_ccs_sensor(subdev);
        bool streaming = sensor->streaming;
        int rval;

        rval = pm_runtime_resume_and_get(dev);
        if (rval < 0) 
                return -EAGAIN;

        if (sensor->streaming)
                ccs_stop_streaming(sensor);

        /* save state for resume */
        sensor->streaming = streaming;

        return 0;

}

Not sure if "return -EAGAIN" is the right thing here. I mean, the PM runtime core has two error conditions that are independent on whatever the PM callback would be doing[1]:

	        if (dev->power.runtime_error)
                retval = -EINVAL;
        else if (dev->power.disable_depth > 0)
                retval = -EACCES;

It would be very unlikely that trying to suspend again would solve those conditions.

So, I guess that the right thing to do is to change the code to do, instead:

  static int __maybe_unused ccs_suspend(struct device *dev)
  {
        struct i2c_client *client = to_i2c_client(dev);
        struct v4l2_subdev *subdev = i2c_get_clientdata(client);
        struct ccs_sensor *sensor = to_ccs_sensor(subdev);
        bool streaming = sensor->streaming;
        int rval;

        rval = pm_runtime_resume_and_get(dev);
        if (rval < 0) 
                return rval;
	...

}

[1] see rpm_resume() code at drivers/base/power/runtime.c.

Hi Mauro,

Quoted 102 lines · Mauro Carvalho Chehab

Em Sun, 25 Apr 2021 21:55:25 +0300
Sakari Ailus <[email protected]> escreveu:

Quoted 41 lines

Hi Mauro,

Thanks for the patch.

Quoted 36 lines · Mauro Carvalho Chehab

Commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter") added pm_runtime_resume_and_get() in order to automatically handle dev->power.usage_count decrement on errors.

Use the new API, in order to cleanup the error check logic.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

---
 drivers/media/i2c/ccs/ccs-core.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

drivers/media/i2c/ccs/ccs-core.c +4 -5

index 9dc3f45da3dc..1441ddcc9b35 100644
@@ -1880,12 +1880,11 @@ static int ccs_pm_get_init(struct ccs_sensor *sensor)
 	struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
 	int rval;
 
-	rval = pm_runtime_get_sync(&client->dev);
-	if (rval < 0) {
-		pm_runtime_put_noidle(&client->dev);
-
+	rval = pm_runtime_resume_and_get(&client->dev);
+	if (rval < 0)
 		return rval;
-	} else if (!rval) {
+
+	if (!rval) {
 		rval = v4l2_ctrl_handler_setup(&sensor->pixel_array->
 					       ctrl_handler);
 		if (rval)

@@ -3089,7 +3088,7 @@ static int __maybe_unused ccs_suspend(struct device *dev)
 	bool streaming = sensor->streaming;
 	int rval;
 

Quoted 6 lines

drivers/media/i2c/ccs/ccs-core.c static int __maybe_unused ccs_suspend(struct device *dev) · line 3093

Quoted 4 lines

drivers/media/i2c/ccs/ccs-core.c +1 -1

-	rval = pm_runtime_get_sync(dev);
+	rval = pm_runtime_resume_and_get(dev);
 	if (rval < 0) {
 		pm_runtime_put_noidle(dev);    

You'll need to drop pm_runtime_put_noidle() here.

OK!

---

On a non-related issue at the same code, after the change, the suspend function will be:

  static int __maybe_unused ccs_suspend(struct device *dev)
  {
        struct i2c_client *client = to_i2c_client(dev);
        struct v4l2_subdev *subdev = i2c_get_clientdata(client);
        struct ccs_sensor *sensor = to_ccs_sensor(subdev);
        bool streaming = sensor->streaming;
        int rval;

        rval = pm_runtime_resume_and_get(dev);
        if (rval < 0) 
                return -EAGAIN;

        if (sensor->streaming)
                ccs_stop_streaming(sensor);

        /* save state for resume */
        sensor->streaming = streaming;

        return 0;

}

Not sure if "return -EAGAIN" is the right thing here. I mean, the PM runtime core has two error conditions that are independent on whatever the PM callback would be doing[1]:

	        if (dev->power.runtime_error)
                retval = -EINVAL;
        else if (dev->power.disable_depth > 0)
                retval = -EACCES;

It would be very unlikely that trying to suspend again would solve those conditions.

So, I guess that the right thing to do is to change the code to do, instead:

  static int __maybe_unused ccs_suspend(struct device *dev)
  {
        struct i2c_client *client = to_i2c_client(dev);
        struct v4l2_subdev *subdev = i2c_get_clientdata(client);
        struct ccs_sensor *sensor = to_ccs_sensor(subdev);
        bool streaming = sensor->streaming;
        int rval;

Quoted 8 lines

        rval = pm_runtime_resume_and_get(dev);
        if (rval < 0) 
                return rval;
	...

}

[1] see rpm_resume() code at drivers/base/power/runtime.c.

Yeah, I agree. This code is one of the older parts the driver.

Please add:

Acked-by Sakari Ailus <[email protected]>

The same goes for the other sensor driver patches in the set you cc'd me, i.e. patches 12, 15, 26, 28,32, 40, 45, 51, 53 and 55.

Em Mon, 26 Apr 2021 17:09:00 +0300
Sakari Ailus <[email protected]> escreveu:

Quoted 114 lines

Hi Mauro,

Quoted 102 lines · Mauro Carvalho Chehab

Em Sun, 25 Apr 2021 21:55:25 +0300
Sakari Ailus <[email protected]> escreveu:

Quoted 41 lines

Hi Mauro,

Thanks for the patch.

Quoted 36 lines · Mauro Carvalho Chehab

Commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter") added pm_runtime_resume_and_get() in order to automatically handle dev->power.usage_count decrement on errors.

Use the new API, in order to cleanup the error check logic.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

---
 drivers/media/i2c/ccs/ccs-core.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

drivers/media/i2c/ccs/ccs-core.c +4 -5

index 9dc3f45da3dc..1441ddcc9b35 100644
@@ -1880,12 +1880,11 @@ static int ccs_pm_get_init(struct ccs_sensor *sensor)
 	struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
 	int rval;
 
-	rval = pm_runtime_get_sync(&client->dev);
-	if (rval < 0) {
-		pm_runtime_put_noidle(&client->dev);
-
+	rval = pm_runtime_resume_and_get(&client->dev);
+	if (rval < 0)
 		return rval;
-	} else if (!rval) {
+
+	if (!rval) {
 		rval = v4l2_ctrl_handler_setup(&sensor->pixel_array->
 					       ctrl_handler);
 		if (rval)

@@ -3089,7 +3088,7 @@ static int __maybe_unused ccs_suspend(struct device *dev)
 	bool streaming = sensor->streaming;
 	int rval;
 

Quoted 6 lines

drivers/media/i2c/ccs/ccs-core.c static int __maybe_unused ccs_suspend(struct device *dev) · line 3093

Quoted 4 lines

drivers/media/i2c/ccs/ccs-core.c +1 -1

-	rval = pm_runtime_get_sync(dev);
+	rval = pm_runtime_resume_and_get(dev);
 	if (rval < 0) {
 		pm_runtime_put_noidle(dev);    

You'll need to drop pm_runtime_put_noidle() here.

OK!

---

On a non-related issue at the same code, after the change, the suspend function will be:

  static int __maybe_unused ccs_suspend(struct device *dev)
  {
        struct i2c_client *client = to_i2c_client(dev);
        struct v4l2_subdev *subdev = i2c_get_clientdata(client);
        struct ccs_sensor *sensor = to_ccs_sensor(subdev);
        bool streaming = sensor->streaming;
        int rval;

        rval = pm_runtime_resume_and_get(dev);
        if (rval < 0) 
                return -EAGAIN;

        if (sensor->streaming)
                ccs_stop_streaming(sensor);

        /* save state for resume */
        sensor->streaming = streaming;

        return 0;

}

Not sure if "return -EAGAIN" is the right thing here. I mean, the PM runtime core has two error conditions that are independent on whatever the PM callback would be doing[1]:

	        if (dev->power.runtime_error)
                retval = -EINVAL;
        else if (dev->power.disable_depth > 0)
                retval = -EACCES;

It would be very unlikely that trying to suspend again would solve those conditions.

So, I guess that the right thing to do is to change the code to do, instead:

  static int __maybe_unused ccs_suspend(struct device *dev)
  {
        struct i2c_client *client = to_i2c_client(dev);
        struct v4l2_subdev *subdev = i2c_get_clientdata(client);
        struct ccs_sensor *sensor = to_ccs_sensor(subdev);
        bool streaming = sensor->streaming;
        int rval;

Quoted 8 lines

        rval = pm_runtime_resume_and_get(dev);
        if (rval < 0) 
                return rval;
	...

}

[1] see rpm_resume() code at drivers/base/power/runtime.c.

Quoted 8 lines

Yeah, I agree. This code is one of the older parts the driver.

Please add:

Acked-by Sakari Ailus <[email protected]>

The same goes for the other sensor driver patches in the set you cc'd me, i.e. patches 12, 15, 26, 28,32, 40, 45, 51, 53 and 55.

It probably makes sense to address the suspend/resume -EAGAIN return code on a separate patch series, before this one, as:

1. this is unrelated to this change;
2. it is something that should be c/c to fixes. So, having it before this series makes easier to apply there.

Quoted 126 lines

Em Mon, 26 Apr 2021 17:09:00 +0300
Sakari Ailus <[email protected]> escreveu:

Quoted 122 lines

Hi Mauro,

Quoted 102 lines · Mauro Carvalho Chehab

Em Sun, 25 Apr 2021 21:55:25 +0300
Sakari Ailus <[email protected]> escreveu:

Quoted 41 lines

Hi Mauro,

Thanks for the patch.

Quoted 36 lines · Mauro Carvalho Chehab

Commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter") added pm_runtime_resume_and_get() in order to automatically handle dev->power.usage_count decrement on errors.

Use the new API, in order to cleanup the error check logic.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

---
 drivers/media/i2c/ccs/ccs-core.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

drivers/media/i2c/ccs/ccs-core.c +4 -5

index 9dc3f45da3dc..1441ddcc9b35 100644
@@ -1880,12 +1880,11 @@ static int ccs_pm_get_init(struct ccs_sensor *sensor)
 	struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
 	int rval;
 
-	rval = pm_runtime_get_sync(&client->dev);
-	if (rval < 0) {
-		pm_runtime_put_noidle(&client->dev);
-
+	rval = pm_runtime_resume_and_get(&client->dev);
+	if (rval < 0)
 		return rval;
-	} else if (!rval) {
+
+	if (!rval) {
 		rval = v4l2_ctrl_handler_setup(&sensor->pixel_array->
 					       ctrl_handler);
 		if (rval)

@@ -3089,7 +3088,7 @@ static int __maybe_unused ccs_suspend(struct device *dev)
 	bool streaming = sensor->streaming;
 	int rval;
 

Quoted 6 lines

drivers/media/i2c/ccs/ccs-core.c static int __maybe_unused ccs_suspend(struct device *dev) · line 3093

Quoted 4 lines

drivers/media/i2c/ccs/ccs-core.c +1 -1

-	rval = pm_runtime_get_sync(dev);
+	rval = pm_runtime_resume_and_get(dev);
 	if (rval < 0) {
 		pm_runtime_put_noidle(dev);      

You'll need to drop pm_runtime_put_noidle() here.

OK!

---

On a non-related issue at the same code, after the change, the suspend function will be:

  static int __maybe_unused ccs_suspend(struct device *dev)
  {
        struct i2c_client *client = to_i2c_client(dev);
        struct v4l2_subdev *subdev = i2c_get_clientdata(client);
        struct ccs_sensor *sensor = to_ccs_sensor(subdev);
        bool streaming = sensor->streaming;
        int rval;

        rval = pm_runtime_resume_and_get(dev);
        if (rval < 0) 
                return -EAGAIN;

        if (sensor->streaming)
                ccs_stop_streaming(sensor);

        /* save state for resume */
        sensor->streaming = streaming;

        return 0;

}

Not sure if "return -EAGAIN" is the right thing here. I mean, the PM runtime core has two error conditions that are independent on whatever the PM callback would be doing[1]:

	        if (dev->power.runtime_error)
                retval = -EINVAL;
        else if (dev->power.disable_depth > 0)
                retval = -EACCES;

It would be very unlikely that trying to suspend again would solve those conditions.

So, I guess that the right thing to do is to change the code to do, instead:

  static int __maybe_unused ccs_suspend(struct device *dev)
  {
        struct i2c_client *client = to_i2c_client(dev);
        struct v4l2_subdev *subdev = i2c_get_clientdata(client);
        struct ccs_sensor *sensor = to_ccs_sensor(subdev);
        bool streaming = sensor->streaming;
        int rval;

Quoted 8 lines

        rval = pm_runtime_resume_and_get(dev);
        if (rval < 0) 
                return rval;
	...

}

[1] see rpm_resume() code at drivers/base/power/runtime.c.

Yeah, I agree. This code is one of the older parts the driver.

Please add:

Acked-by Sakari Ailus <[email protected]>

The same goes for the other sensor driver patches in the set you cc'd me, i.e. patches 12, 15, 26, 28,32, 40, 45, 51, 53 and 55.

Quoted 6 lines

It probably makes sense to address the suspend/resume -EAGAIN return code on a separate patch series, before this one, as:

1. this is unrelated to this change;
2. it is something that should be c/c to fixes. So, having it before this series makes easier to apply there.

Sounds good to me. If you can submit a patch, please add my ack. :-)

Em Mon, 26 Apr 2021 17:29:02 +0300
Sakari Ailus <[email protected]> escreveu:

Quoted 10 lines · Mauro Carvalho Chehab

Quoted 9 lines

Em Mon, 26 Apr 2021 17:09:00 +0300
Sakari Ailus <[email protected]> escreveu:

Quoted 6 lines

Hi Mauro,

Quoted 3 lines · Mauro Carvalho Chehab

Em Sun, 25 Apr 2021 21:55:25 +0300
Sakari Ailus <[email protected]> escreveu:

Quoted 68 lines

Quoted 68 lines

Quoted 56 lines

Quoted 47 lines

On a non-related issue at the same code, after the change, the suspend function will be:

  static int __maybe_unused ccs_suspend(struct device *dev)
  {
        struct i2c_client *client = to_i2c_client(dev);
        struct v4l2_subdev *subdev = i2c_get_clientdata(client);
        struct ccs_sensor *sensor = to_ccs_sensor(subdev);
        bool streaming = sensor->streaming;
        int rval;

        rval = pm_runtime_resume_and_get(dev);
        if (rval < 0) 
                return -EAGAIN;

        if (sensor->streaming)
                ccs_stop_streaming(sensor);

        /* save state for resume */
        sensor->streaming = streaming;

        return 0;

}

Not sure if "return -EAGAIN" is the right thing here. I mean, the PM runtime core has two error conditions that are independent on whatever the PM callback would be doing[1]:

	        if (dev->power.runtime_error)
                retval = -EINVAL;
        else if (dev->power.disable_depth > 0)
                retval = -EACCES;

It would be very unlikely that trying to suspend again would solve those conditions.

So, I guess that the right thing to do is to change the code to do, instead:

  static int __maybe_unused ccs_suspend(struct device *dev)
  {
        struct i2c_client *client = to_i2c_client(dev);
        struct v4l2_subdev *subdev = i2c_get_clientdata(client);
        struct ccs_sensor *sensor = to_ccs_sensor(subdev);
        bool streaming = sensor->streaming;
        int rval;

Quoted 8 lines

        rval = pm_runtime_resume_and_get(dev);
        if (rval < 0) 
                return rval;
	...

}

[1] see rpm_resume() code at drivers/base/power/runtime.c.

Quoted 8 lines

Yeah, I agree. This code is one of the older parts the driver.

Please add:

Acked-by Sakari Ailus <[email protected]>

The same goes for the other sensor driver patches in the set you cc'd me, i.e. patches 12, 15, 26, 28,32, 40, 45, 51, 53 and 55.

It probably makes sense to address the suspend/resume -EAGAIN return code on a separate patch series, before this one, as:

Quoted 5 lines

Quoted 3 lines

1. this is unrelated to this change;
2. it is something that should be c/c to fixes. So, having it before this series makes easier to apply there.

Sounds good to me. If you can submit a patch, please add my ack. :-)

Sure. I'll work on such patch series.

Commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter")
added pm_runtime_resume_and_get() in order to automatically handle dev->power.usage_count decrement on errors.

Use the new API, in order to cleanup the error check logic.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

---
 drivers/media/i2c/imx334.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

drivers/media/i2c/imx334.c +2 -2

index 047aa7658d21..a5a03bb4a6ae 100644
@@ -717,9 +717,9 @@ static int imx334_set_stream(struct v4l2_subdev
 *sd, int enable)
 	}
 
 	if (enable) {
-		ret = pm_runtime_get_sync(imx334->dev);
+		ret = pm_runtime_resume_and_get(imx334->dev);
 		if (ret)
-			goto error_power_off;
+			goto error_unlock;
 
 		ret = imx334_start_streaming(imx334);

drivers/media/i2c/imx334.c +1 -0

 		if (ret)

@@ -737,6 +737,7 @@ static int imx334_set_stream(struct v4l2_subdev
 *sd, int enable)
 
 error_power_off:
 	pm_runtime_put(imx334->dev);
+error_unlock:
 	mutex_unlock(&imx334->mutex);
 

return ret;

Commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter") added pm_runtime_resume_and_get() in order to automatically handle dev->power.usage_count decrement on errors.

Use the new API, in order to cleanup the error check logic.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

---
 drivers/media/i2c/mt9m001.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

drivers/media/i2c/mt9m001.c +4 -4

index 3b0ba8ed5233..57e15a291ebd 100644
@@ -217,9 +217,9 @@ static int mt9m001_s_stream(struct v4l2_subdev *sd, int enable)
 		goto done;
 
 	if (enable) {
-		ret = pm_runtime_get_sync(&client->dev);
+		ret = pm_runtime_resume_and_get(&client->dev);
 		if (ret < 0)
-			goto put_unlock;
+			goto unlock;
 
 		ret = mt9m001_apply_selection(sd);
 		if (ret)

@@ -247,6 +247,7 @@ static int mt9m001_s_stream(struct v4l2_subdev *sd, int enable)
 
 put_unlock:
 	pm_runtime_put(&client->dev);
+unlock:
 	mutex_unlock(&mt9m001->mutex);
 
 	return ret;

@@ -834,7 +835,7 @@ static int mt9m001_remove(struct i2c_client *client)
 {
 	struct mt9m001 *mt9m001 = to_mt9m001(client);
 
-	pm_runtime_get_sync(&client->dev);
+	pm_runtime_resume_and_get(&client->dev);
 
 	v4l2_async_unregister_subdev(&mt9m001->subdev);
 	media_entity_cleanup(&mt9m001->subdev.entity);
--

2.30.2

Hi Mauro,

Quoted 7 lines · Mauro Carvalho Chehab

Commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter") added pm_runtime_resume_and_get() in order to automatically handle dev->power.usage_count decrement on errors.

Use the new API, in order to cleanup the error check logic.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

Thanks

Acked-by Jacopo Mondi <[email protected]>

I should re-work the error handling sequence there on top of this patch as right now it's not the best, that 'done' label bothers me... anyway, for later.

Quoted 32 lines

---
 drivers/media/i2c/mt9m001.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

drivers/media/i2c/mt9m001.c +3 -2

index 3b0ba8ed5233..57e15a291ebd 100644
@@ -217,9 +217,9 @@ static int mt9m001_s_stream(struct v4l2_subdev *sd, int enable)
 		goto done;
 
 	if (enable) {
-		ret = pm_runtime_get_sync(&client->dev);
+		ret = pm_runtime_resume_and_get(&client->dev);
 		if (ret < 0)
-			goto put_unlock;
+			goto unlock;
 
 		ret = mt9m001_apply_selection(sd);
 		if (ret)

@@ -247,6 +247,7 @@ static int mt9m001_s_stream(struct v4l2_subdev *sd, int enable)
 
 put_unlock:
 	pm_runtime_put(&client->dev);
+unlock:
 	mutex_unlock(&mt9m001->mutex);
 
 	return ret;

@@ -834,7 +835,7 @@ static int mt9m001_remove(struct i2c_client *client)
 {
 	struct mt9m001 *mt9m001 = to_mt9m001(client);
 

drivers/media/i2c/mt9m001.c static int mt9m001_remove(struct i2c_client *client) · line 841

Quoted 5 lines

drivers/media/i2c/mt9m001.c +1 -1

-	pm_runtime_get_sync(&client->dev);
+	pm_runtime_resume_and_get(&client->dev);
 
 	v4l2_async_unregister_subdev(&mt9m001->subdev);
 	media_entity_cleanup(&mt9m001->subdev.entity);

Quoted 50 lines

Hi Mauro,

Quoted 7 lines · Mauro Carvalho Chehab

Commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter") added pm_runtime_resume_and_get() in order to automatically handle dev->power.usage_count decrement on errors.

Use the new API, in order to cleanup the error check logic.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

Thanks

Acked-by Jacopo Mondi <[email protected]>

I should re-work the error handling sequence there on top of this patch as right now it's not the best, that 'done' label bothers me... anyway, for later.

Quoted 32 lines

---
 drivers/media/i2c/mt9m001.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

drivers/media/i2c/mt9m001.c +3 -2

index 3b0ba8ed5233..57e15a291ebd 100644
@@ -217,9 +217,9 @@ static int mt9m001_s_stream(struct v4l2_subdev *sd, int enable)
 		goto done;
 
 	if (enable) {
-		ret = pm_runtime_get_sync(&client->dev);
+		ret = pm_runtime_resume_and_get(&client->dev);
 		if (ret < 0)
-			goto put_unlock;
+			goto unlock;
 
 		ret = mt9m001_apply_selection(sd);
 		if (ret)

@@ -247,6 +247,7 @@ static int mt9m001_s_stream(struct v4l2_subdev *sd, int enable)
 
 put_unlock:
 	pm_runtime_put(&client->dev);
+unlock:
 	mutex_unlock(&mt9m001->mutex);
 
 	return ret;

@@ -834,7 +835,7 @@ static int mt9m001_remove(struct i2c_client *client)
 {
 	struct mt9m001 *mt9m001 = to_mt9m001(client);
 

Quoted 5 lines

drivers/media/i2c/mt9m001.c static int mt9m001_remove(struct i2c_client *client) · line 841

Quoted 5 lines

drivers/media/i2c/mt9m001.c +1 -1

-	pm_runtime_get_sync(&client->dev);
+	pm_runtime_resume_and_get(&client->dev);
 
 	v4l2_async_unregister_subdev(&mt9m001->subdev);
 	media_entity_cleanup(&mt9m001->subdev.entity);  

I couldn't help looking at one more now that you got feedback on this one.

Here you have the same problem as the one I reported earlier, in that the usage count could end up negative on resume failure due to the later put_noidle() call in remove().

Also note that you're adding more lines than you're removing.

I'd say this kind of mass-conversion is of questionable worth as pm_runtime_resume_and_get() isn't necessarily an improvement (even if it may have its use in some places).

Em Sat, 24 Apr 2021 12:00:46 +0200
Johan Hovold <[email protected]> escreveu:

Quoted 8 lines · Jacopo Mondi

Quoted 7 lines

Hi Mauro,

Quoted 4 lines · Mauro Carvalho Chehab

Commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter") added pm_runtime_resume_and_get() in order to automatically handle dev->power.usage_count decrement on errors.

Quoted 7 lines

Quoted 3 lines

Quoted 3 lines

Use the new API, in order to cleanup the error check logic.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

I'd say this kind of mass-conversion is of questionable worth as pm_runtime_resume_and_get() isn't necessarily an improvement (even if it may have its use in some places).

The main problem is that other parts of the driver's core APIs assume that get object methods will only increment the usage counter if no errors. The pm_runtime_get_sync() is an exception.

Its name doesn't help at all: A function like that should, IMHO, be called, instead:

pm_runtime_inc_usage_count_and_try_to_resume().

Or something similar, in order to make clearer that it always increment the usage count, no matter what. If possible, all drivers should get rid of it too (or alternatively add comments warning people that keeping the usage_count incremented is desired on the very specific places where it is really needed), as it is risky to use something that has a different usage_count increement behavior than other more usual *_get() functions.

With regards to mass-fixing it, I've seen several patches seen to media fixing bugs due to the bad usage_count decrement logic. So, the best is to solve them all at once, and stop using pm_runtime_get_sync() inside the subsystem.

Quoted 28 lines

Em Sat, 24 Apr 2021 12:00:46 +0200
Johan Hovold <[email protected]> escreveu:

Quoted 8 lines · Jacopo Mondi

Quoted 7 lines

Hi Mauro,

Quoted 4 lines · Mauro Carvalho Chehab

Commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter") added pm_runtime_resume_and_get() in order to automatically handle dev->power.usage_count decrement on errors.

Quoted 7 lines

Quoted 3 lines

Quoted 3 lines

Use the new API, in order to cleanup the error check logic.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

I'd say this kind of mass-conversion is of questionable worth as pm_runtime_resume_and_get() isn't necessarily an improvement (even if it may have its use in some places).

The main problem is that other parts of the driver's core APIs assume that get object methods will only increment the usage counter if no errors. The pm_runtime_get_sync() is an exception.

Its name doesn't help at all: A function like that should, IMHO, be called, instead:

pm_runtime_inc_usage_count_and_try_to_resume().

Quoted 7 lines

Or something similar, in order to make clearer that it always increment the usage count, no matter what. If possible, all drivers should get rid of it too (or alternatively add comments warning people that keeping the usage_count incremented is desired on the very specific places where it is really needed), as it is risky to use something that has a different usage_count increement behavior than other more usual *_get() functions.

pm_runtime_get_sync() has worked this way since it was merged 12 years ago, and for someone who's used to this interface this is not such a big deal as you seem to think. Sure, you need to remember to put the usage counter on errors, but that's it (and the other side of that is that you don't need to worry about error handling where it doesn't matter).

Also note all the pm_runtime_get functions *always* increment the usage count even if an async resume may later fail so there is consistency here.

And regarding naming, the new pm_resume_and_get() looks completely out of place to me since it uses a different naming scheme than the other helpers (including the ones that are used to balance the new call).

Quoted 4 lines

With regards to mass-fixing it, I've seen several patches seen to media fixing bugs due to the bad usage_count decrement logic. So, the best is to solve them all at once, and stop using pm_runtime_get_sync() inside the subsystem.

Sure, having the script kiddies patch drivers without understanding what they're are really doing is bound to introduce bugs unless it can be caught in review.

You're call, but converting functioning drivers where the authors knew what they were doing just because you're not used to the API and risk introducing new bugs in the process isn't necessarily a good idea.

Especially since the pm_runtime_get_sync() will continue to be used elsewhere, and possibly even in media in cases where you don't need to check for errors (e.g. remove()).

Em Tue, 27 Apr 2021 14:23:20 +0200
Johan Hovold <[email protected]> escreveu:

Quoted 5 lines

pm_runtime_get_sync() has worked this way since it was merged 12 years ago, and for someone who's used to this interface this is not such a big deal as you seem to think. Sure, you need to remember to put the usage counter on errors, but that's it (and the other side of that is that you don't need to worry about error handling where it doesn't matter).

Before we have those at PM subsystem, the media had its own way to set/disable power for their sub-devices. The PCI and USB drivers still use it, instead of pm_runtime, mostly due to historic reasons.

So, basically, its usage at the media subsystem is restricted to drivers for embedded systems. The vast majority of drivers supporting PM runtime are the I2C camera drivers. The camera drivers can be used interchangeable. So, in practice, the same bridge driver can work with a lot of different camera models, depending on the hardware vendors' personal preferences and the desired max resolution.

So, in thesis, all such drivers should behave exactly the same with regards to PM.

However, on most existing drivers, the pm_runtime was added a couple of years ago, and by people that are not too familiar with the PM subsystem.

That probably explains why there were/are several places that do things like this[1]:

	ret = pm_runtime_get_sync(dev);
	if (ret < 0)
		return ret;

without taking care of calling a pm_runtime_put*() function.

[1] besides the 13 patches made by UCN addressing it on existing code, I discovered the same pattern on a couple of other drivers with current upstream code.

That shows a pattern: several media developers are not familiar with the usage_count behavior for pm_runtime_get functions.

So, doing this work is not only helping to make the PM support more uniform, but it is also helping to solve existing issues.

Quoted 3 lines

You're call, but converting functioning drivers where the authors knew what they were doing just because you're not used to the API and risk introducing new bugs in the process isn't necessarily a good idea.

The problem is that the above assumption is not necessarily true:
based on the number of drivers that pm_runtime_get_sync() weren't decrementing usage_count on errors, several driver authors were not familiar enough with the PM runtime behavior by the time the drivers were written or converted to use the PM runtime, instead of the media .s_power()/.s_stream() callbacks.

Quoted 3 lines

Especially since the pm_runtime_get_sync() will continue to be used elsewhere, and possibly even in media in cases where you don't need to check for errors (e.g. remove()).

Talking about the remove(), I'm not sure if just ignoring errors there would do the right thing. I mean, if pm_runtime_get_sync() fails, probably any attempts to disable clocks and other things that depend on PM runtime will also (silently) fail.

This may put the device on an unknown PM and keep clock lines enabled after its removal.

Quoted 8 lines

Em Tue, 27 Apr 2021 14:23:20 +0200
Johan Hovold <[email protected]> escreveu:

Quoted 3 lines

You're call, but converting functioning drivers where the authors knew what they were doing just because you're not used to the API and risk introducing new bugs in the process isn't necessarily a good idea.

Quoted 6 lines

The problem is that the above assumption is not necessarily true:
based on the number of drivers that pm_runtime_get_sync() weren't decrementing usage_count on errors, several driver authors were not familiar enough with the PM runtime behavior by the time the drivers were written or converted to use the PM runtime, instead of the media .s_power()/.s_stream() callbacks.

That may very well be the case. My point is just that this work needs to be done carefully and by people familiar with the code (and runtime pm) or you risk introducing new issues.

I really don't want the bot-warning-suppression crew to start with this for example.

Quoted 11 lines

Quoted 3 lines

Especially since the pm_runtime_get_sync() will continue to be used elsewhere, and possibly even in media in cases where you don't need to check for errors (e.g. remove()).

Talking about the remove(), I'm not sure if just ignoring errors there would do the right thing. I mean, if pm_runtime_get_sync() fails, probably any attempts to disable clocks and other things that depend on PM runtime will also (silently) fail.

This may put the device on an unknown PM and keep clock lines enabled after its removal.

Right, a resume failure is a pretty big issue and it's not really clear how to to even handle that generally. But at remove() time you don't have much choice but to go on and release resource anyway.

So unless actually implementing some error handling too, using pm_runtime_sync_get() without checking for errors is still preferred over pm_runtime_resume_and_get(). That is

	pm_runtime_get_sync();
	/* cleanup */
	pm_runtime_disable()
	pm_runtime_put_noidle();

is better than:

	ret = pm_runtime_resume_and_get();
	/* cleanup */
	pm_runtime_disable();
	if (ret == 0)
		pm_runtime_put_noidle();

unless you also start doing something ret.

Johan

Em Wed, 28 Apr 2021 12:05:26 +0200
Johan Hovold <[email protected]> escreveu:

Quoted 15 lines

Right, a resume failure is a pretty big issue and it's not really clear how to to even handle that generally. But at remove() time you don't have much choice but to go on and release resource anyway.

So unless actually implementing some error handling too, using pm_runtime_sync_get() without checking for errors is still preferred over pm_runtime_resume_and_get(). That is

	pm_runtime_get_sync();
	/* cleanup */
	pm_runtime_disable()
	pm_runtime_put_noidle();

is better than:

Quoted 7 lines

	ret = pm_runtime_resume_and_get();
	/* cleanup */
	pm_runtime_disable();
	if (ret == 0)
		pm_runtime_put_noidle();

unless you also start doing something ret.

Perhaps the best would be to use, instead:

	pm_runtime_get_noresume();

(quoted diff) +0 -0

 	/* cleanup */
 	pm_runtime_disable()
 	pm_runtime_put_noidle();

	pm_runtime_set_suspended();

I mean, at least for my eyes, it doesn't make sense to do a PM resume during driver's removal/unbind time.

Commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter") added pm_runtime_resume_and_get() in order to automatically handle dev->power.usage_count decrement on errors.

Use the new API, in order to cleanup the error check logic.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

---
 drivers/media/i2c/ov5647.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

drivers/media/i2c/ov5647.c +5 -5

index 1cefa15729ce..38faa74755e3 100644
@@ -882,20 +882,20 @@ static int ov5647_s_stream(struct v4l2_subdev *sd, int enable)
 	}
 
 	if (enable) {
-		ret = pm_runtime_get_sync(&client->dev);
+		ret = pm_runtime_resume_and_get(&client->dev);
 		if (ret < 0)
 			goto error_unlock;
 
 		ret = ov5647_stream_on(sd);
 		if (ret < 0) {
 			dev_err(&client->dev, "stream start failed: %d\n", ret);
-			goto error_unlock;
+			goto error_pm;
 		}
 	} else {
 		ret = ov5647_stream_off(sd);
 		if (ret < 0) {
 			dev_err(&client->dev, "stream stop failed: %d\n", ret);
-			goto error_unlock;
+			goto error_pm;
 		}
 		pm_runtime_put(&client->dev);
 	}

@@ -905,8 +905,9 @@ static int ov5647_s_stream(struct v4l2_subdev *sd, int enable)
 
 	return 0;
 
+error_pm:
+	pm_runtime_put(&client->dev);
 error_unlock:
-	pm_runtime_put(&client->dev);
 	mutex_unlock(&sensor->lock);
 
 	return ret;
--

2.30.2