* [PATCH v4 01/79] media: venus: fix PM runtime logic at venus_sys_error_handler()
2021-04-28 14:51 [PATCH v4 00/79] Address some issues with PM runtime at media subsystem Mauro Carvalho Chehab
@ 2021-04-28 14:51 ` Mauro Carvalho Chehab
2021-04-30 15:21 ` Jonathan Cameron
2021-04-28 14:52 ` [PATCH v4 65/79] media: camss: use pm_runtime_resume_and_get() Mauro Carvalho Chehab
` (4 subsequent siblings)
5 siblings, 1 reply; 13+ messages in thread
From: Mauro Carvalho Chehab @ 2021-04-28 14:51 UTC (permalink / raw)
Cc: linuxarm, mauro.chehab, Mauro Carvalho Chehab, Andy Gross,
Bjorn Andersson, Hans Verkuil, Mauro Carvalho Chehab,
Stanimir Varbanov, linux-arm-msm, linux-kernel, linux-media
The venus_sys_error_handler() assumes that pm_runtime was
able to resume, as it does things like:
while (pm_runtime_active(core->dev_dec) || pm_runtime_active(core->dev_enc))
msleep(10);
Well, if, for whatever reason, this won't happen, the routine
won't do what's expected. So, check for the returned error
condition, warning if it returns an error.
Fixes: af2c3834c8ca ("[media] media: venus: adding core part and helper functions")
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
drivers/media/platform/qcom/venus/core.c | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
index 54bac7ec14c5..c80c27c87ccc 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -84,7 +84,11 @@ static void venus_sys_error_handler(struct work_struct *work)
container_of(work, struct venus_core, work.work);
int ret = 0;
- pm_runtime_get_sync(core->dev);
+ ret = pm_runtime_get_sync(core->dev);
+ if (WARN_ON(ret < 0)) {
+ pm_runtime_put_noidle(core->dev);
+ return;
+ }
hfi_core_deinit(core, true);
@@ -106,9 +110,13 @@ static void venus_sys_error_handler(struct work_struct *work)
hfi_reinit(core);
- pm_runtime_get_sync(core->dev);
+ ret = pm_runtime_get_sync(core->dev);
+ if (WARN_ON(ret < 0)) {
+ pm_runtime_put_noidle(core->dev);
+ return;
+ }
- ret |= venus_boot(core);
+ ret = venus_boot(core);
ret |= hfi_core_resume(core, true);
enable_irq(core->irq);
--
2.30.2
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v4 01/79] media: venus: fix PM runtime logic at venus_sys_error_handler()
2021-04-28 14:51 ` [PATCH v4 01/79] media: venus: fix PM runtime logic at venus_sys_error_handler() Mauro Carvalho Chehab
@ 2021-04-30 15:21 ` Jonathan Cameron
2021-05-03 9:00 ` Mauro Carvalho Chehab
0 siblings, 1 reply; 13+ messages in thread
From: Jonathan Cameron @ 2021-04-30 15:21 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: linuxarm, mauro.chehab, Andy Gross, Bjorn Andersson,
Hans Verkuil, Mauro Carvalho Chehab, Stanimir Varbanov,
linux-arm-msm, linux-kernel, linux-media
On Wed, 28 Apr 2021 16:51:22 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:
> The venus_sys_error_handler() assumes that pm_runtime was
> able to resume, as it does things like:
>
> while (pm_runtime_active(core->dev_dec) || pm_runtime_active(core->dev_enc))
> msleep(10);
>
> Well, if, for whatever reason, this won't happen, the routine
> won't do what's expected. So, check for the returned error
> condition, warning if it returns an error.
>
> Fixes: af2c3834c8ca ("[media] media: venus: adding core part and helper functions")
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> ---
> drivers/media/platform/qcom/venus/core.c | 14 +++++++++++---
> 1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
> index 54bac7ec14c5..c80c27c87ccc 100644
> --- a/drivers/media/platform/qcom/venus/core.c
> +++ b/drivers/media/platform/qcom/venus/core.c
> @@ -84,7 +84,11 @@ static void venus_sys_error_handler(struct work_struct *work)
> container_of(work, struct venus_core, work.work);
> int ret = 0;
>
> - pm_runtime_get_sync(core->dev);
> + ret = pm_runtime_get_sync(core->dev);
> + if (WARN_ON(ret < 0)) {
> + pm_runtime_put_noidle(core->dev);
> + return;
> + }
>
> hfi_core_deinit(core, true);
>
> @@ -106,9 +110,13 @@ static void venus_sys_error_handler(struct work_struct *work)
>
> hfi_reinit(core);
>
> - pm_runtime_get_sync(core->dev);
> + ret = pm_runtime_get_sync(core->dev);
> + if (WARN_ON(ret < 0)) {
> + pm_runtime_put_noidle(core->dev);
mutex_unlock(&core->lock);
(the unlock is currently just below the enable_irq() in 5.12)
> + return;
> + }
>
> - ret |= venus_boot(core);
> + ret = venus_boot(core);
> ret |= hfi_core_resume(core, true);
>
> enable_irq(core->irq);
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v4 01/79] media: venus: fix PM runtime logic at venus_sys_error_handler()
2021-04-30 15:21 ` Jonathan Cameron
@ 2021-05-03 9:00 ` Mauro Carvalho Chehab
0 siblings, 0 replies; 13+ messages in thread
From: Mauro Carvalho Chehab @ 2021-05-03 9:00 UTC (permalink / raw)
To: Jonathan Cameron
Cc: linuxarm, mauro.chehab, Andy Gross, Bjorn Andersson,
Hans Verkuil, Mauro Carvalho Chehab, Stanimir Varbanov,
linux-arm-msm, linux-kernel, linux-media
Em Fri, 30 Apr 2021 16:21:10 +0100
Jonathan Cameron <Jonathan.Cameron@Huawei.com> escreveu:
> On Wed, 28 Apr 2021 16:51:22 +0200
> Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:
>
> > The venus_sys_error_handler() assumes that pm_runtime was
> > able to resume, as it does things like:
> >
> > while (pm_runtime_active(core->dev_dec) || pm_runtime_active(core->dev_enc))
> > msleep(10);
> >
> > Well, if, for whatever reason, this won't happen, the routine
> > won't do what's expected. So, check for the returned error
> > condition, warning if it returns an error.
> >
> > Fixes: af2c3834c8ca ("[media] media: venus: adding core part and helper functions")
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > ---
> > drivers/media/platform/qcom/venus/core.c | 14 +++++++++++---
> > 1 file changed, 11 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
> > index 54bac7ec14c5..c80c27c87ccc 100644
> > --- a/drivers/media/platform/qcom/venus/core.c
> > +++ b/drivers/media/platform/qcom/venus/core.c
> > @@ -84,7 +84,11 @@ static void venus_sys_error_handler(struct work_struct *work)
> > container_of(work, struct venus_core, work.work);
> > int ret = 0;
> >
> > - pm_runtime_get_sync(core->dev);
> > + ret = pm_runtime_get_sync(core->dev);
> > + if (WARN_ON(ret < 0)) {
> > + pm_runtime_put_noidle(core->dev);
> > + return;
> > + }
> >
> > hfi_core_deinit(core, true);
> >
> > @@ -106,9 +110,13 @@ static void venus_sys_error_handler(struct work_struct *work)
> >
> > hfi_reinit(core);
> >
> > - pm_runtime_get_sync(core->dev);
> > + ret = pm_runtime_get_sync(core->dev);
> > + if (WARN_ON(ret < 0)) {
> > + pm_runtime_put_noidle(core->dev);
Thanks for review!
> mutex_unlock(&core->lock);
> (the unlock is currently just below the enable_irq() in 5.12)
Basically, this function assumes that the core->lock is not locked
and that IRQs are disabled, as can be seen at the function which
starts such work:
static void venus_event_notify(struct venus_core *core, u32 event)
{
struct venus_inst *inst;
switch (event) {
case EVT_SYS_WATCHDOG_TIMEOUT:
case EVT_SYS_ERROR:
break;
default:
return;
}
mutex_lock(&core->lock);
core->sys_error = true;
list_for_each_entry(inst, &core->instances, list)
inst->ops->event_notify(inst, EVT_SESSION_ERROR, NULL);
mutex_unlock(&core->lock);
disable_irq_nosync(core->irq);
schedule_delayed_work(&core->work, msecs_to_jiffies(10));
}
The code inside it actually locks/unlocks two times the core->lock.
See, this is the original code:
static void venus_sys_error_handler(struct work_struct *work)
{
<not locked>
pm_runtime_get_sync(core->dev);
hfi_core_deinit(core, true);
dev_warn(core->dev, "system error has occurred, starting recovery!\n");
mutex_lock(&core->lock);
while (pm_runtime_active(core->dev_dec) || pm_runtime_active(core->dev_enc))
msleep(10);
...
enable_irq(core->irq);
mutex_unlock(&core->lock);
...
if (ret) {
disable_irq_nosync(core->irq);
dev_warn(core->dev, "recovery failed (%d)\n", ret);
schedule_delayed_work(&core->work, msecs_to_jiffies(10));
return;
}
mutex_lock(&core->lock);
core->sys_error = false;
mutex_unlock(&core->lock);
}
It should be noticed that, once started, this delayed work re-starts
itself, with IRQs disabled, trying to reboot the Venus IP hardware,
until it stops failing [1].
[1] IMHO, it seems a very bad idea to keep running the work forever,
flooding syslog with error messages on every 10ms or so.
That's said, my patch doesn't seem to fix all potential issues that
could happen there.
I'll propose a separate fix, outside this patch series, as the issues
here are not only due to RPM, but the main issue is that both
while loops inside this code can run forever with the core->lock
hold.
Thanks,
Mauro
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v4 65/79] media: camss: use pm_runtime_resume_and_get()
2021-04-28 14:51 [PATCH v4 00/79] Address some issues with PM runtime at media subsystem Mauro Carvalho Chehab
2021-04-28 14:51 ` [PATCH v4 01/79] media: venus: fix PM runtime logic at venus_sys_error_handler() Mauro Carvalho Chehab
@ 2021-04-28 14:52 ` Mauro Carvalho Chehab
2021-04-28 14:52 ` [PATCH v4 66/79] media: venus: " Mauro Carvalho Chehab
` (3 subsequent siblings)
5 siblings, 0 replies; 13+ messages in thread
From: Mauro Carvalho Chehab @ 2021-04-28 14:52 UTC (permalink / raw)
Cc: linuxarm, mauro.chehab, Mauro Carvalho Chehab, Andy Gross,
Bjorn Andersson, Mauro Carvalho Chehab, Robert Foss, Todor Tomov,
linux-arm-msm, linux-kernel, linux-media
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.
Reviewed-by: Robert Foss <robert.foss@linaro.org>
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
drivers/media/platform/qcom/camss/camss-csid.c | 6 ++----
drivers/media/platform/qcom/camss/camss-csiphy.c | 6 ++----
drivers/media/platform/qcom/camss/camss-ispif.c | 6 ++----
drivers/media/platform/qcom/camss/camss-vfe.c | 5 +++--
4 files changed, 9 insertions(+), 14 deletions(-)
diff --git a/drivers/media/platform/qcom/camss/camss-csid.c b/drivers/media/platform/qcom/camss/camss-csid.c
index cc11fbfdae13..d2a7f2a64f26 100644
--- a/drivers/media/platform/qcom/camss/camss-csid.c
+++ b/drivers/media/platform/qcom/camss/camss-csid.c
@@ -156,11 +156,9 @@ static int csid_set_power(struct v4l2_subdev *sd, int on)
int ret;
if (on) {
- ret = pm_runtime_get_sync(dev);
- if (ret < 0) {
- pm_runtime_put_sync(dev);
+ ret = pm_runtime_resume_and_get(dev);
+ if (ret < 0)
return ret;
- }
ret = regulator_enable(csid->vdda);
if (ret < 0) {
diff --git a/drivers/media/platform/qcom/camss/camss-csiphy.c b/drivers/media/platform/qcom/camss/camss-csiphy.c
index b3c3bf19e522..8e18b8e668cf 100644
--- a/drivers/media/platform/qcom/camss/camss-csiphy.c
+++ b/drivers/media/platform/qcom/camss/camss-csiphy.c
@@ -197,11 +197,9 @@ static int csiphy_set_power(struct v4l2_subdev *sd, int on)
if (on) {
int ret;
- ret = pm_runtime_get_sync(dev);
- if (ret < 0) {
- pm_runtime_put_sync(dev);
+ ret = pm_runtime_resume_and_get(dev);
+ if (ret < 0)
return ret;
- }
ret = csiphy_set_clock_rates(csiphy);
if (ret < 0) {
diff --git a/drivers/media/platform/qcom/camss/camss-ispif.c b/drivers/media/platform/qcom/camss/camss-ispif.c
index 37611c8861da..d9907742ba79 100644
--- a/drivers/media/platform/qcom/camss/camss-ispif.c
+++ b/drivers/media/platform/qcom/camss/camss-ispif.c
@@ -372,11 +372,9 @@ static int ispif_set_power(struct v4l2_subdev *sd, int on)
goto exit;
}
- ret = pm_runtime_get_sync(dev);
- if (ret < 0) {
- pm_runtime_put_sync(dev);
+ ret = pm_runtime_resume_and_get(dev);
+ if (ret < 0)
goto exit;
- }
ret = camss_enable_clocks(ispif->nclocks, ispif->clock, dev);
if (ret < 0) {
diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c b/drivers/media/platform/qcom/camss/camss-vfe.c
index 15695fd466c4..cf743e61f798 100644
--- a/drivers/media/platform/qcom/camss/camss-vfe.c
+++ b/drivers/media/platform/qcom/camss/camss-vfe.c
@@ -584,9 +584,9 @@ static int vfe_get(struct vfe_device *vfe)
if (ret < 0)
goto error_pm_domain;
- ret = pm_runtime_get_sync(vfe->camss->dev);
+ ret = pm_runtime_resume_and_get(vfe->camss->dev);
if (ret < 0)
- goto error_pm_runtime_get;
+ goto error_domain_off;
ret = vfe_set_clock_rates(vfe);
if (ret < 0)
@@ -620,6 +620,7 @@ static int vfe_get(struct vfe_device *vfe)
error_pm_runtime_get:
pm_runtime_put_sync(vfe->camss->dev);
+error_domain_off:
vfe->ops->pm_domain_off(vfe);
error_pm_domain:
--
2.30.2
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH v4 66/79] media: venus: use pm_runtime_resume_and_get()
2021-04-28 14:51 [PATCH v4 00/79] Address some issues with PM runtime at media subsystem Mauro Carvalho Chehab
2021-04-28 14:51 ` [PATCH v4 01/79] media: venus: fix PM runtime logic at venus_sys_error_handler() Mauro Carvalho Chehab
2021-04-28 14:52 ` [PATCH v4 65/79] media: camss: use pm_runtime_resume_and_get() Mauro Carvalho Chehab
@ 2021-04-28 14:52 ` Mauro Carvalho Chehab
2021-04-28 14:52 ` [PATCH v4 67/79] media: venus: vdec: " Mauro Carvalho Chehab
` (2 subsequent siblings)
5 siblings, 0 replies; 13+ messages in thread
From: Mauro Carvalho Chehab @ 2021-04-28 14:52 UTC (permalink / raw)
Cc: linuxarm, mauro.chehab, Mauro Carvalho Chehab, Andy Gross,
Bjorn Andersson, Mauro Carvalho Chehab, Stanimir Varbanov,
linux-arm-msm, linux-kernel, linux-media
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 <mchehab+huawei@kernel.org>
---
drivers/media/platform/qcom/venus/core.c | 30 +++++++++----------
.../media/platform/qcom/venus/pm_helpers.c | 10 +++----
2 files changed, 18 insertions(+), 22 deletions(-)
diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
index c80c27c87ccc..aa359f8e82c5 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -84,11 +84,9 @@ static void venus_sys_error_handler(struct work_struct *work)
container_of(work, struct venus_core, work.work);
int ret = 0;
- ret = pm_runtime_get_sync(core->dev);
- if (WARN_ON(ret < 0)) {
- pm_runtime_put_noidle(core->dev);
+ ret = pm_runtime_resume_and_get(core->dev);
+ if (WARN_ON(ret < 0))
return;
- }
hfi_core_deinit(core, true);
@@ -110,11 +108,9 @@ static void venus_sys_error_handler(struct work_struct *work)
hfi_reinit(core);
- ret = pm_runtime_get_sync(core->dev);
- if (WARN_ON(ret < 0)) {
- pm_runtime_put_noidle(core->dev);
+ ret = pm_runtime_resume_and_get(core->dev);
+ if (WARN_ON(ret < 0))
return;
- }
ret = venus_boot(core);
ret |= hfi_core_resume(core, true);
@@ -313,21 +309,21 @@ static int venus_probe(struct platform_device *pdev)
pm_runtime_enable(dev);
- ret = pm_runtime_get_sync(dev);
+ ret = pm_runtime_resume_and_get(dev);
if (ret < 0)
goto err_runtime_disable;
ret = of_platform_populate(dev->of_node, NULL, NULL, dev);
if (ret)
- goto err_runtime_disable;
+ goto err_pm;
ret = venus_firmware_init(core);
if (ret)
- goto err_runtime_disable;
+ goto err_pm;
ret = venus_boot(core);
if (ret)
- goto err_runtime_disable;
+ goto err_pm;
ret = hfi_core_resume(core, true);
if (ret)
@@ -359,8 +355,9 @@ static int venus_probe(struct platform_device *pdev)
v4l2_device_unregister(&core->v4l2_dev);
err_venus_shutdown:
venus_shutdown(core);
-err_runtime_disable:
+err_pm:
pm_runtime_put_noidle(dev);
+err_runtime_disable:
pm_runtime_set_suspended(dev);
pm_runtime_disable(dev);
hfi_destroy(core);
@@ -379,7 +376,7 @@ static int venus_remove(struct platform_device *pdev)
struct device *dev = core->dev;
int ret;
- ret = pm_runtime_get_sync(dev);
+ ret = pm_runtime_resume_and_get(dev);
WARN_ON(ret < 0);
ret = hfi_core_deinit(core, true);
@@ -390,7 +387,8 @@ static int venus_remove(struct platform_device *pdev)
venus_firmware_deinit(core);
- pm_runtime_put_sync(dev);
+ if (ret >= 0)
+ pm_runtime_put_sync(dev);
pm_runtime_disable(dev);
if (pm_ops->core_put)
@@ -411,7 +409,7 @@ static void venus_core_shutdown(struct platform_device *pdev)
{
struct venus_core *core = platform_get_drvdata(pdev);
- pm_runtime_get_sync(core->dev);
+ pm_runtime_resume_and_get(core->dev);
venus_shutdown(core);
venus_firmware_deinit(core);
pm_runtime_put_sync(core->dev);
diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
index c7e1ebec47ee..9e32ec866af7 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.c
+++ b/drivers/media/platform/qcom/venus/pm_helpers.c
@@ -486,7 +486,7 @@ static int poweron_coreid(struct venus_core *core, unsigned int coreid_mask)
int ret;
if (coreid_mask & VIDC_CORE_ID_1) {
- ret = pm_runtime_get_sync(core->pmdomains[1]);
+ ret = pm_runtime_resume_and_get(core->pmdomains[1]);
if (ret < 0)
return ret;
@@ -504,7 +504,7 @@ static int poweron_coreid(struct venus_core *core, unsigned int coreid_mask)
}
if (coreid_mask & VIDC_CORE_ID_2) {
- ret = pm_runtime_get_sync(core->pmdomains[2]);
+ ret = pm_runtime_resume_and_get(core->pmdomains[2]);
if (ret < 0)
return ret;
@@ -990,11 +990,9 @@ static int core_power_v4(struct venus_core *core, int on)
if (on == POWER_ON) {
if (pmctrl) {
- ret = pm_runtime_get_sync(pmctrl);
- if (ret < 0) {
- pm_runtime_put_noidle(pmctrl);
+ ret = pm_runtime_resume_and_get(pmctrl);
+ if (ret < 0)
return ret;
- }
}
ret = core_resets_reset(core);
--
2.30.2
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH v4 67/79] media: venus: vdec: use pm_runtime_resume_and_get()
2021-04-28 14:51 [PATCH v4 00/79] Address some issues with PM runtime at media subsystem Mauro Carvalho Chehab
` (2 preceding siblings ...)
2021-04-28 14:52 ` [PATCH v4 66/79] media: venus: " Mauro Carvalho Chehab
@ 2021-04-28 14:52 ` Mauro Carvalho Chehab
2021-04-30 17:53 ` Jonathan Cameron
2021-04-28 14:52 ` [PATCH v4 68/79] media: venus: venc: " Mauro Carvalho Chehab
2021-04-28 15:50 ` [PATCH v4 00/79] Address some issues with PM runtime at media subsystem Johan Hovold
5 siblings, 1 reply; 13+ messages in thread
From: Mauro Carvalho Chehab @ 2021-04-28 14:52 UTC (permalink / raw)
Cc: linuxarm, mauro.chehab, Mauro Carvalho Chehab, Andy Gross,
Bjorn Andersson, Mauro Carvalho Chehab, Stanimir Varbanov,
linux-arm-msm, linux-kernel, linux-media
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 <mchehab+huawei@kernel.org>
---
drivers/media/platform/qcom/venus/vdec.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
index ddb7cd39424e..347e533ea673 100644
--- a/drivers/media/platform/qcom/venus/vdec.c
+++ b/drivers/media/platform/qcom/venus/vdec.c
@@ -568,7 +568,7 @@ static int vdec_pm_get(struct venus_inst *inst)
int ret;
mutex_lock(&core->pm_lock);
- ret = pm_runtime_get_sync(dev);
+ ret = pm_runtime_resume_and_get(dev);
mutex_unlock(&core->pm_lock);
return ret < 0 ? ret : 0;
@@ -601,7 +601,7 @@ static int vdec_pm_get_put(struct venus_inst *inst)
mutex_lock(&core->pm_lock);
if (pm_runtime_suspended(dev)) {
- ret = pm_runtime_get_sync(dev);
+ ret = pm_runtime_resume_and_get(dev);
if (ret < 0)
goto error;
--
2.30.2
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v4 67/79] media: venus: vdec: use pm_runtime_resume_and_get()
2021-04-28 14:52 ` [PATCH v4 67/79] media: venus: vdec: " Mauro Carvalho Chehab
@ 2021-04-30 17:53 ` Jonathan Cameron
0 siblings, 0 replies; 13+ messages in thread
From: Jonathan Cameron @ 2021-04-30 17:53 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: linuxarm, mauro.chehab, Andy Gross, Bjorn Andersson,
Mauro Carvalho Chehab, Stanimir Varbanov, linux-arm-msm,
linux-kernel, linux-media
On Wed, 28 Apr 2021 16:52:28 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:
> 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 <mchehab+huawei@kernel.org>
> ---
> drivers/media/platform/qcom/venus/vdec.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
> index ddb7cd39424e..347e533ea673 100644
> --- a/drivers/media/platform/qcom/venus/vdec.c
> +++ b/drivers/media/platform/qcom/venus/vdec.c
> @@ -568,7 +568,7 @@ static int vdec_pm_get(struct venus_inst *inst)
> int ret;
>
> mutex_lock(&core->pm_lock);
> - ret = pm_runtime_get_sync(dev);
> + ret = pm_runtime_resume_and_get(dev);
> mutex_unlock(&core->pm_lock);
>
> return ret < 0 ? ret : 0;
Don't need this dance any more due to " Return 0 if the runtime PM usage counter of @dev has been
* incremented or a negative error code otherwise."
return ret;
> @@ -601,7 +601,7 @@ static int vdec_pm_get_put(struct venus_inst *inst)
> mutex_lock(&core->pm_lock);
>
> if (pm_runtime_suspended(dev)) {
> - ret = pm_runtime_get_sync(dev);
> + ret = pm_runtime_resume_and_get(dev);
> if (ret < 0)
> goto error;
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v4 68/79] media: venus: venc: use pm_runtime_resume_and_get()
2021-04-28 14:51 [PATCH v4 00/79] Address some issues with PM runtime at media subsystem Mauro Carvalho Chehab
` (3 preceding siblings ...)
2021-04-28 14:52 ` [PATCH v4 67/79] media: venus: vdec: " Mauro Carvalho Chehab
@ 2021-04-28 14:52 ` Mauro Carvalho Chehab
2021-04-28 15:50 ` [PATCH v4 00/79] Address some issues with PM runtime at media subsystem Johan Hovold
5 siblings, 0 replies; 13+ messages in thread
From: Mauro Carvalho Chehab @ 2021-04-28 14:52 UTC (permalink / raw)
Cc: linuxarm, mauro.chehab, Mauro Carvalho Chehab, Andy Gross,
Bjorn Andersson, Mauro Carvalho Chehab, Stanimir Varbanov,
linux-arm-msm, linux-kernel, linux-media
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 <mchehab+huawei@kernel.org>
---
drivers/media/platform/qcom/venus/venc.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/qcom/venus/venc.c
index 4a7291f934b6..8dd49d4f124c 100644
--- a/drivers/media/platform/qcom/venus/venc.c
+++ b/drivers/media/platform/qcom/venus/venc.c
@@ -1205,9 +1205,9 @@ static int venc_open(struct file *file)
venus_helper_init_instance(inst);
- ret = pm_runtime_get_sync(core->dev_enc);
+ ret = pm_runtime_resume_and_get(core->dev_enc);
if (ret < 0)
- goto err_put_sync;
+ goto err_free;
ret = venc_ctrl_init(inst);
if (ret)
@@ -1252,6 +1252,7 @@ static int venc_open(struct file *file)
venc_ctrl_deinit(inst);
err_put_sync:
pm_runtime_put_sync(core->dev_enc);
+err_free:
kfree(inst);
return ret;
}
--
2.30.2
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v4 00/79] Address some issues with PM runtime at media subsystem
2021-04-28 14:51 [PATCH v4 00/79] Address some issues with PM runtime at media subsystem Mauro Carvalho Chehab
` (4 preceding siblings ...)
2021-04-28 14:52 ` [PATCH v4 68/79] media: venus: venc: " Mauro Carvalho Chehab
@ 2021-04-28 15:50 ` Johan Hovold
2021-04-29 10:18 ` Mauro Carvalho Chehab
5 siblings, 1 reply; 13+ messages in thread
From: Johan Hovold @ 2021-04-28 15:50 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Shawn Tu, Ricardo Ribalda, Dafna Hirschfeld, Heiko Stuebner,
linuxarm, Todor Tomov, Bjorn Andersson, Andrzej Hajda, Lad,
Prabhakar, Thierry Reding, Pengutronix Kernel Team,
Dmitry Osipenko, linux-stm32, Andrzej Pietrasiewicz, Leon Luo,
Paul Kocialkowski, Mauro Carvalho Chehab, Dave Stevenson,
Matt Ranostay, Krzysztof Kozlowski, Jonathan Hunter,
linux-rockchip, Chen-Yu Tsai, Andy Gross, Matthias Brugger,
Dongchun Zhu, Sakari Ailus, Bingbu Cao, Marek Szyprowski,
Shunqian Zheng, Tianshu Qiu, NXP Linux Team, Philipp Zabel,
devel, Jacopo Mondi, Sylwester Nawrocki, linux-tegra,
Alexandre Torgue, Wenyou Yang, Manivannan Sadhasivam,
linux-arm-msm, Sascha Hauer, Steve Longerbeam, linux-media,
Maxime Ripard, Stanimir Varbanov, Benoit Parrot, Helen Koike,
linux-samsung-soc, linux-mediatek, Jacek Anaszewski,
mauro.chehab, Sylwester Nawrocki, Paul J. Murphy,
Ezequiel Garcia, Daniele Alessandrelli, Chiranjeevi Rapolu,
linux-arm-kernel, Jacob Chen, Jernej Skrabec, Hyungwoo Yang,
linux-kernel, Robert Foss, Dan Scally, Sowjanya Komatineni,
Maxime Coquelin, linux-renesas-soc, Yong Zhi, Shawn Guo
On Wed, Apr 28, 2021 at 04:51:21PM +0200, Mauro Carvalho Chehab wrote:
> During the review of the patches from unm.edu, one of the patterns
> I noticed is the amount of patches trying to fix pm_runtime_get_sync()
> calls.
>
> After analyzing the feedback from version 1 of this series, I noticed
> a few other weird behaviors at the PM runtime resume code. So, this
> series start addressing some bugs and issues at the current code.
> Then, it gets rid of pm_runtime_get_sync() at the media subsystem
> (with 2 exceptions).
>
> It should be noticed that
> Commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter")
> added a new method to does a pm_runtime get, which increments
> the usage count only on success.
>
> The rationale of getting rid of pm_runtime_get_sync() is:
>
> 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 ;-)
This is perfectly alright. Take a look at ov7740_remove() for example:
pm_runtime_get_sync(&client->dev);
pm_runtime_disable(&client->dev);
pm_runtime_set_suspended(&client->dev);
pm_runtime_put_noidle(&client->dev);
ov7740_set_power(ov7740, 0);
There's an explicit power-off after balancing the PM count and this will
work regardless of the power state when entering this function.
So this has nothing to do with pm_runtime_get_sync() per se.
> 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;
Sure, but pm_runtime_get_async() also works this way. You just won't be
notified if the async resume fails.
> 3. The name of the new variant is a lot clearer:
> pm_runtime_resume_and_get()
> As its same clearly says that this is a PM runtime resume function,
> that also increments the usage counter on success;
It also introduced an inconsistency in the API and does not pair as well
with the pm_runtime_put variants.
> 4. 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;
It's not known to be safe; there are ways to get also this interface
wrong as for example this series has shown.
> 5. 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.
Cargo-cult programming always runs that risk.
> This series replace places pm_runtime_get_sync(), by calling
> pm_runtime_resume_and_get() instead.
>
> This should help to avoid future mistakes like that, as people
> tend to use the existing drivers as examples for newer ones.
The only valid point about and use for pm_runtime_resume_and_get() is to
avoid leaking a PM usage count reference in the unlikely case that
resume fails (something which hardly any driver implements recovery
from anyway).
It's a convenience wrapper that saves you from writing one extra line in
some cases (depending on how you implement runtime-pm support) and not a
silver bullet against bugs.
> compile-tested only.
>
> Patches 1 to 7 fix some issues that already exists at the current
> PM runtime code;
>
> patches 8 to 20 fix some usage_count problems that still exists
> at the media subsystem;
>
> patches 21 to 78 repaces pm_runtime_get_sync() by
> pm_runtime_resume_and_get();
>
> Patch 79 (and a hunk on patch 78) documents the two exceptions
> where pm_runtime_get_sync() will still be used for now.
>
> ---
>
> v4:
> - Added a couple of additional fixes at existing PM runtime code;
> - Some patches are now more conservative in order to avoid causing
> regressions.
> v3:
> - fix a compilation error;
> v2:
> - addressed pointed issues and fixed a few other PM issues.
This really doesn't say much more than "changed stuff" so kinda hard to
track if review feedback has been taken into account for example.
Johan
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v4 00/79] Address some issues with PM runtime at media subsystem
2021-04-28 15:50 ` [PATCH v4 00/79] Address some issues with PM runtime at media subsystem Johan Hovold
@ 2021-04-29 10:18 ` Mauro Carvalho Chehab
2021-04-29 12:33 ` Dmitry Osipenko
2021-04-29 15:17 ` Johan Hovold
0 siblings, 2 replies; 13+ messages in thread
From: Mauro Carvalho Chehab @ 2021-04-29 10:18 UTC (permalink / raw)
To: Johan Hovold
Cc: Shawn Tu, Ricardo Ribalda, Dafna Hirschfeld, Heiko Stuebner,
linuxarm, Todor Tomov, Bjorn Andersson, Andrzej Hajda, Lad,
Prabhakar, Thierry Reding, Pengutronix Kernel Team,
Dmitry Osipenko, linux-stm32, Andrzej Pietrasiewicz, Leon Luo,
Paul Kocialkowski, Mauro Carvalho Chehab, Dave Stevenson,
Matt Ranostay, Krzysztof Kozlowski, Jonathan Hunter,
linux-rockchip, Chen-Yu Tsai, Andy Gross, Matthias Brugger,
Dongchun Zhu, Sakari Ailus, Bingbu Cao, Marek Szyprowski,
Shunqian Zheng, Tianshu Qiu, NXP Linux Team, Philipp Zabel,
devel, Jacopo Mondi, Sylwester Nawrocki, linux-tegra,
Alexandre Torgue, Wenyou Yang, Manivannan Sadhasivam,
linux-arm-msm, Sascha Hauer, Steve Longerbeam, linux-media,
Maxime Ripard, Stanimir Varbanov, Benoit Parrot, Helen Koike,
linux-samsung-soc, linux-mediatek, Jacek Anaszewski,
mauro.chehab, Sylwester Nawrocki, Paul J. Murphy,
Ezequiel Garcia, Daniele Alessandrelli, Chiranjeevi Rapolu,
linux-arm-kernel, Jacob Chen, Jernej Skrabec, Hyungwoo Yang,
linux-kernel, Robert Foss, Dan Scally, Sowjanya Komatineni,
Maxime Coquelin, linux-renesas-soc, Yong Zhi, Shawn Guo
Em Wed, 28 Apr 2021 17:50:08 +0200
Johan Hovold <johan@kernel.org> escreveu:
> On Wed, Apr 28, 2021 at 04:51:21PM +0200, Mauro Carvalho Chehab wrote:
> > 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 ;-)
>
> This is perfectly alright. Take a look at ov7740_remove() for example:
>
> pm_runtime_get_sync(&client->dev);
> pm_runtime_disable(&client->dev);
> pm_runtime_set_suspended(&client->dev);
> pm_runtime_put_noidle(&client->dev);
>
> ov7740_set_power(ov7740, 0);
>
> There's an explicit power-off after balancing the PM count and this will
> work regardless of the power state when entering this function.
Ok, but this should equally work:
pm_runtime_disable(&client->dev);
pm_runtime_set_suspended(&client->dev);
ov7740_set_power(ov7740, 0);
as there's no additional cleanup made on this particular driver
between pm_runtime_get_sync() and pm_runtime_put_noidle().
> So this has nothing to do with pm_runtime_get_sync() per se.
Yes, but some patches on this series are cleaning up the driver release
logic.
>
> > 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;
>
> Sure, but pm_runtime_get_async() also works this way. You just won't be
> notified if the async resume fails.
Granted, it makes sense along the pm_runtime kAPI.
It is inconsistent with the behavior of kobject_get*() and other
*_get*() methods that are based or inspired on it, as, on those, the
operations are atomic: either everything succeeds and it doesn't return
an error, or the usage counter is not incremented and the object
state doesn't change after the call.
> > 3. The name of the new variant is a lot clearer:
> > pm_runtime_resume_and_get()
> > As its same clearly says that this is a PM runtime resume function,
> > that also increments the usage counter on success;
>
> It also introduced an inconsistency in the API and does not pair as well
> with the pm_runtime_put variants.
Agreed. A name that would be more consistent with PM runtime would
probably be:
pm_runtime_resume_if_get()
as there are already:
pm_runtime_get_if_in_use()
pm_runtime_get_if_active()
But any such discussions are out of the scope of this patchset ;-)
>
> > 4. 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;
>
> It's not known to be safe; there are ways to get also this interface
> wrong as for example this series has shown.
Very true. Yet, it is a lot simpler to use functions that won't change
the state of the objects when returning an error, as this is by far
the most common pattern within the Kernel.
Human brains are trained to identify certain patterns. When there's
something using a similar pattern, but with a different behavior,
our brains are more subject to fail identifying problems.
> > 5. 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.
>
> Cargo-cult programming always runs that risk.
True.
> > This series replace places pm_runtime_get_sync(), by calling
> > pm_runtime_resume_and_get() instead.
> >
> > This should help to avoid future mistakes like that, as people
> > tend to use the existing drivers as examples for newer ones.
>
> The only valid point about and use for pm_runtime_resume_and_get() is to
> avoid leaking a PM usage count reference in the unlikely case that
> resume fails (something which hardly any driver implements recovery
> from anyway).
>
> It's a convenience wrapper that saves you from writing one extra line in
> some cases (depending on how you implement runtime-pm support) and not a
> silver bullet against bugs.
>
> > compile-tested only.
> >
> > Patches 1 to 7 fix some issues that already exists at the current
> > PM runtime code;
> >
> > patches 8 to 20 fix some usage_count problems that still exists
> > at the media subsystem;
> >
> > patches 21 to 78 repaces pm_runtime_get_sync() by
> > pm_runtime_resume_and_get();
> >
> > Patch 79 (and a hunk on patch 78) documents the two exceptions
> > where pm_runtime_get_sync() will still be used for now.
> >
> > ---
> >
> > v4:
> > - Added a couple of additional fixes at existing PM runtime code;
> > - Some patches are now more conservative in order to avoid causing
> > regressions.
> > v3:
> > - fix a compilation error;
> > v2:
> > - addressed pointed issues and fixed a few other PM issues.
>
> This really doesn't say much more than "changed stuff" so kinda hard to
> track if review feedback has been taken into account for example.
I addressed all review feedback I got (as far as I'm aware), and added
all received reviewed-by/acked-by.
Yeah, I could have written a more comprehensive changes description
there.
Thanks,
Mauro
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 00/79] Address some issues with PM runtime at media subsystem
2021-04-29 10:18 ` Mauro Carvalho Chehab
@ 2021-04-29 12:33 ` Dmitry Osipenko
2021-04-29 15:17 ` Johan Hovold
1 sibling, 0 replies; 13+ messages in thread
From: Dmitry Osipenko @ 2021-04-29 12:33 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Johan Hovold
Cc: Shawn Tu, Ricardo Ribalda, Dafna Hirschfeld, Heiko Stuebner,
linuxarm, Todor Tomov, Bjorn Andersson, Andrzej Hajda, Lad,
Prabhakar, Thierry Reding, Pengutronix Kernel Team, linux-stm32,
Andrzej Pietrasiewicz, Leon Luo, Paul Kocialkowski,
Mauro Carvalho Chehab, Dave Stevenson, Matt Ranostay,
Krzysztof Kozlowski, Jonathan Hunter, linux-rockchip,
Chen-Yu Tsai, Andy Gross, Matthias Brugger, Dongchun Zhu,
Sakari Ailus, Bingbu Cao, Marek Szyprowski, Shunqian Zheng,
Tianshu Qiu, NXP Linux Team, Philipp Zabel, devel, Jacopo Mondi,
Sylwester Nawrocki, linux-tegra, Alexandre Torgue, Wenyou Yang,
Manivannan Sadhasivam, linux-arm-msm, Sascha Hauer,
Steve Longerbeam, linux-media, Maxime Ripard, Stanimir Varbanov,
Benoit Parrot, Helen Koike, linux-samsung-soc, linux-mediatek,
Jacek Anaszewski, mauro.chehab, Sylwester Nawrocki,
Paul J. Murphy, Ezequiel Garcia, Daniele Alessandrelli,
Chiranjeevi Rapolu, linux-arm-kernel, Jacob Chen, Jernej Skrabec,
Hyungwoo Yang, linux-kernel, Robert Foss, Dan Scally,
Sowjanya Komatineni, Maxime Coquelin, linux-renesas-soc,
Yong Zhi, Shawn Guo
29.04.2021 13:18, Mauro Carvalho Chehab пишет:
>> This is perfectly alright. Take a look at ov7740_remove() for example:
>>
>> pm_runtime_get_sync(&client->dev);
>> pm_runtime_disable(&client->dev);
>> pm_runtime_set_suspended(&client->dev);
>> pm_runtime_put_noidle(&client->dev);
>>
>> ov7740_set_power(ov7740, 0);
>>
>> There's an explicit power-off after balancing the PM count and this will
>> work regardless of the power state when entering this function.
> Ok, but this should equally work:
>
> pm_runtime_disable(&client->dev);
> pm_runtime_set_suspended(&client->dev);
>
> ov7740_set_power(ov7740, 0);
>
> as there's no additional cleanup made on this particular driver
> between pm_runtime_get_sync() and pm_runtime_put_noidle().
>
The pm_runtime_get_sync() turns hardware ON by invoking
ov7740_set_power(ov7740, 1), and thus, the ON->OFF is kept balanced in
both RPM-available and RPM-unavailable cases. The RPM state of device
should be reset after driver removal.
It doesn't look like any additional cleanups are needed by that ov7740
driver. The driver removal is opposite to the probe, hence it should be
correct as-is.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 00/79] Address some issues with PM runtime at media subsystem
2021-04-29 10:18 ` Mauro Carvalho Chehab
2021-04-29 12:33 ` Dmitry Osipenko
@ 2021-04-29 15:17 ` Johan Hovold
1 sibling, 0 replies; 13+ messages in thread
From: Johan Hovold @ 2021-04-29 15:17 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Shawn Tu, Ricardo Ribalda, Dafna Hirschfeld, Heiko Stuebner,
linuxarm, Todor Tomov, Bjorn Andersson, Andrzej Hajda, Lad,
Prabhakar, Thierry Reding, Pengutronix Kernel Team,
Dmitry Osipenko, linux-stm32, Andrzej Pietrasiewicz, Leon Luo,
Paul Kocialkowski, Mauro Carvalho Chehab, Dave Stevenson,
Matt Ranostay, Krzysztof Kozlowski, Jonathan Hunter,
linux-rockchip, Chen-Yu Tsai, Andy Gross, Matthias Brugger,
Dongchun Zhu, Sakari Ailus, Bingbu Cao, Marek Szyprowski,
Shunqian Zheng, Tianshu Qiu, NXP Linux Team, Philipp Zabel,
devel, Jacopo Mondi, Sylwester Nawrocki, linux-tegra,
Alexandre Torgue, Wenyou Yang, Manivannan Sadhasivam,
linux-arm-msm, Sascha Hauer, Steve Longerbeam, linux-media,
Maxime Ripard, Stanimir Varbanov, Benoit Parrot, Helen Koike,
linux-samsung-soc, linux-mediatek, Jacek Anaszewski,
mauro.chehab, Sylwester Nawrocki, Paul J. Murphy,
Ezequiel Garcia, Daniele Alessandrelli, Chiranjeevi Rapolu,
linux-arm-kernel, Jacob Chen, Jernej Skrabec, Hyungwoo Yang,
linux-kernel, Robert Foss, Dan Scally, Sowjanya Komatineni,
Maxime Coquelin, linux-renesas-soc, Yong Zhi, Shawn Guo
On Thu, Apr 29, 2021 at 12:18:16PM +0200, Mauro Carvalho Chehab wrote:
> Em Wed, 28 Apr 2021 17:50:08 +0200
> Johan Hovold <johan@kernel.org> escreveu:
>
> > On Wed, Apr 28, 2021 at 04:51:21PM +0200, Mauro Carvalho Chehab wrote:
>
> > > 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 ;-)
> >
> > This is perfectly alright. Take a look at ov7740_remove() for example:
> >
> > pm_runtime_get_sync(&client->dev);
> > pm_runtime_disable(&client->dev);
> > pm_runtime_set_suspended(&client->dev);
> > pm_runtime_put_noidle(&client->dev);
> >
> > ov7740_set_power(ov7740, 0);
> >
> > There's an explicit power-off after balancing the PM count and this will
> > work regardless of the power state when entering this function.
>
> Ok, but this should equally work:
>
> pm_runtime_disable(&client->dev);
> pm_runtime_set_suspended(&client->dev);
>
> ov7740_set_power(ov7740, 0);
>
> as there's no additional cleanup made on this particular driver
> between pm_runtime_get_sync() and pm_runtime_put_noidle().
No, that would break the driver as I pointed out to you yesterday:
https://lore.kernel.org/r/YImG1klSPkFSaS3a@hovoldconsulting.com
If the device is already suspended when remove is called then you'll
end up with an unbalanced call to ov7740_set_power() that will try to
disable an already disabled clock.
> > So this has nothing to do with pm_runtime_get_sync() per se.
>
> Yes, but some patches on this series are cleaning up the driver release
> logic.
You mentioned this example as an argument against using
pm_runtime_get_sync(), which I don't think makes sense.
> > > 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;
> >
> > Sure, but pm_runtime_get_async() also works this way. You just won't be
> > notified if the async resume fails.
>
> Granted, it makes sense along the pm_runtime kAPI.
>
> It is inconsistent with the behavior of kobject_get*() and other
> *_get*() methods that are based or inspired on it, as, on those, the
> operations are atomic: either everything succeeds and it doesn't return
> an error, or the usage counter is not incremented and the object
> state doesn't change after the call.
Right, and I'm aware that some people have overlooked this. But its not
the end of the world since hardly any driver can handle resume failures
properly anyway.
This is mostly just an exercise to shut up static checkers.
> > > 3. The name of the new variant is a lot clearer:
> > > pm_runtime_resume_and_get()
> > > As its same clearly says that this is a PM runtime resume function,
> > > that also increments the usage counter on success;
> >
> > It also introduced an inconsistency in the API and does not pair as well
> > with the pm_runtime_put variants.
>
> Agreed. A name that would be more consistent with PM runtime would
> probably be:
>
> pm_runtime_resume_if_get()
Naw, since the get part always succeeds.
It should start with pm_runtime_get, but pm_runtime_get_sync() is
unfortunately taken.
> as there are already:
>
> pm_runtime_get_if_in_use()
> pm_runtime_get_if_active()
>
> But any such discussions are out of the scope of this patchset ;-)
Right.
> > > 4. 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;
> >
> > It's not known to be safe; there are ways to get also this interface
> > wrong as for example this series has shown.
>
> Very true. Yet, it is a lot simpler to use functions that won't change
> the state of the objects when returning an error, as this is by far
> the most common pattern within the Kernel.
A resume failure does change the state (and needs to be recovered from),
but I get what you're saying.
> Human brains are trained to identify certain patterns. When there's
> something using a similar pattern, but with a different behavior,
> our brains are more subject to fail identifying problems.
Sure. But I'm not sure that having two interfaces with different
semantics to do the job is doing us any favours here. But again, that
discussion has already been had.
And I realise that this is partly also your motive here (even if the old
interface isn't going to go away).
> > > compile-tested only.
> > > Patches 1 to 7 fix some issues that already exists at the current
> > > PM runtime code;
> > >
> > > patches 8 to 20 fix some usage_count problems that still exists
> > > at the media subsystem;
> > >
> > > patches 21 to 78 repaces pm_runtime_get_sync() by
> > > pm_runtime_resume_and_get();
> > >
> > > Patch 79 (and a hunk on patch 78) documents the two exceptions
> > > where pm_runtime_get_sync() will still be used for now.
80 patches in one series (posted to lkml) is a bit excessive. Perhaps
you can break it up in a fixes part and one or more cleanups parts?
Johan
^ permalink raw reply [flat|nested] 13+ messages in thread