| Message ID | 13b31912c93b56426660106d673d3c1a5be63170.1619621413.git.mchehab+huawei@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | Address some issues with PM runtime at media subsystem | expand |
On Wed, 28 Apr 2021 16:51:35 +0200 Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote: > 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, ensure that the driver will check if PM runtime > resumed at vpfe_initialize_device(). > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> resume and suspend carrying regardless needs a comment I think. (see below) > --- > drivers/media/platform/am437x/am437x-vpfe.c | 22 +++++++++++++++------ > 1 file changed, 16 insertions(+), 6 deletions(-) > > diff --git a/drivers/media/platform/am437x/am437x-vpfe.c b/drivers/media/platform/am437x/am437x-vpfe.c > index 6cdc77dda0e4..bced526f30f2 100644 > --- a/drivers/media/platform/am437x/am437x-vpfe.c > +++ b/drivers/media/platform/am437x/am437x-vpfe.c > @@ -1021,7 +1021,9 @@ static int vpfe_initialize_device(struct vpfe_device *vpfe) > if (ret) > return ret; > > - pm_runtime_get_sync(vpfe->pdev); > + ret = pm_runtime_resume_and_get(vpfe->pdev); > + if (ret < 0) > + return ret; > > vpfe_config_enable(&vpfe->ccdc, 1); > > @@ -2443,7 +2445,11 @@ static int vpfe_probe(struct platform_device *pdev) > pm_runtime_enable(&pdev->dev); > > /* for now just enable it here instead of waiting for the open */ > - pm_runtime_get_sync(&pdev->dev); > + ret = pm_runtime_resume_and_get(&pdev->dev); > + if (ret < 0) { > + vpfe_err(vpfe, "Unable to resume device.\n"); > + goto probe_out_v4l2_unregister; > + } > > vpfe_ccdc_config_defaults(ccdc); > > @@ -2527,10 +2533,11 @@ static int vpfe_suspend(struct device *dev) > { > struct vpfe_device *vpfe = dev_get_drvdata(dev); > struct vpfe_ccdc *ccdc = &vpfe->ccdc; > + int ret; > > /* only do full suspend if streaming has started */ > if (vb2_start_streaming_called(&vpfe->buffer_queue)) { > - pm_runtime_get_sync(dev); > + ret = pm_runtime_resume_and_get(dev); Carrying on when you know the resume failed, seems interesting enough to deserve a comment in the code. Not sure you can usefully do anything but it seems likely a lot of the calls that follow will fail. > vpfe_config_enable(ccdc, 1); > > /* Save VPFE context */ > @@ -2541,7 +2548,8 @@ static int vpfe_suspend(struct device *dev) > vpfe_config_enable(ccdc, 0); > > /* Disable both master and slave clock */ > - pm_runtime_put_sync(dev); > + if (ret >= 0) > + pm_runtime_put_sync(dev); > } > > /* Select sleep pin state */ > @@ -2583,18 +2591,20 @@ static int vpfe_resume(struct device *dev) > { > struct vpfe_device *vpfe = dev_get_drvdata(dev); > struct vpfe_ccdc *ccdc = &vpfe->ccdc; > + int ret; > > /* only do full resume if streaming has started */ > if (vb2_start_streaming_called(&vpfe->buffer_queue)) { > /* Enable both master and slave clock */ > - pm_runtime_get_sync(dev); > + ret = pm_runtime_resume_and_get(dev); > vpfe_config_enable(ccdc, 1); > > /* Restore VPFE context */ > vpfe_restore_context(ccdc); > > vpfe_config_enable(ccdc, 0); > - pm_runtime_put_sync(dev); > + if (ret >= 0) > + pm_runtime_put_sync(dev); > } > > /* Select default pin state */
Em Fri, 30 Apr 2021 17:36:46 +0100 Jonathan Cameron <Jonathan.Cameron@Huawei.com> escreveu: > On Wed, 28 Apr 2021 16:51:35 +0200 > Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote: > > > 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, ensure that the driver will check if PM runtime > > resumed at vpfe_initialize_device(). > > > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> > > resume and suspend carrying regardless needs a comment I think. > (see below) > > --- > > drivers/media/platform/am437x/am437x-vpfe.c | 22 +++++++++++++++------ > > 1 file changed, 16 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/media/platform/am437x/am437x-vpfe.c b/drivers/media/platform/am437x/am437x-vpfe.c > > index 6cdc77dda0e4..bced526f30f2 100644 > > --- a/drivers/media/platform/am437x/am437x-vpfe.c > > +++ b/drivers/media/platform/am437x/am437x-vpfe.c > > @@ -1021,7 +1021,9 @@ static int vpfe_initialize_device(struct vpfe_device *vpfe) > > if (ret) > > return ret; > > > > - pm_runtime_get_sync(vpfe->pdev); > > + ret = pm_runtime_resume_and_get(vpfe->pdev); > > + if (ret < 0) > > + return ret; > > > > vpfe_config_enable(&vpfe->ccdc, 1); > > > > @@ -2443,7 +2445,11 @@ static int vpfe_probe(struct platform_device *pdev) > > pm_runtime_enable(&pdev->dev); > > > > /* for now just enable it here instead of waiting for the open */ > > - pm_runtime_get_sync(&pdev->dev); > > + ret = pm_runtime_resume_and_get(&pdev->dev); > > + if (ret < 0) { > > + vpfe_err(vpfe, "Unable to resume device.\n"); > > + goto probe_out_v4l2_unregister; > > + } > > > > vpfe_ccdc_config_defaults(ccdc); > > > > @@ -2527,10 +2533,11 @@ static int vpfe_suspend(struct device *dev) > > { > > struct vpfe_device *vpfe = dev_get_drvdata(dev); > > struct vpfe_ccdc *ccdc = &vpfe->ccdc; > > + int ret; > > > > /* only do full suspend if streaming has started */ > > if (vb2_start_streaming_called(&vpfe->buffer_queue)) { > > - pm_runtime_get_sync(dev); > > + ret = pm_runtime_resume_and_get(dev); > > Carrying on when you know the resume failed, seems interesting enough to > deserve a comment in the code. Not sure you can usefully do anything > but it seems likely a lot of the calls that follow will fail. This driver indeed has a different behavior. What most drivers do is to either resume RPM when a V4L2 devnode is opened, or when the device starts to stream. This one does, instead, at probing time. It even has a comment there which implies that this is something that may require changes in the future: static int vpfe_probe(struct platform_device *pdev) { ... /* for now just enable it here instead of waiting for the open */ ret = pm_runtime_resume_and_get(&pdev->dev); After probe, the driver just assumes that RPM is not suspended during its entire lifetime (except suspend/resuume). I can't even see a check at vpfe_open() or at vpfe_start_streaming() that would cause the functions to fail if, for whatever reason, RPM is suspended there[1], or if a command sent to the hardware failed. [1] The only place where there's a check is at v4l2_subdev_call(), asking sensors to start streaming. If those are on a different power domain, a valid sensor answer call won't ensure that am437x VPFE is operational. Yet, suspend/resume only checks if videobuf2 started its streaming logic. if streaming was started, suspend/resume logic tries ensure that the hardware will be ready to be suspended, restoring it to the previous state before at resume time, but neither one of those has a check to see if the commands were succeeded, just like the logic at vpfe_start_streaming(). - In summary, I'll add a comment there, but fixing it would require adding error checks on several places (open, start_streaming, resume and suspend). Thanks, Mauro
diff --git a/drivers/media/platform/am437x/am437x-vpfe.c b/drivers/media/platform/am437x/am437x-vpfe.c index 6cdc77dda0e4..bced526f30f2 100644 --- a/drivers/media/platform/am437x/am437x-vpfe.c +++ b/drivers/media/platform/am437x/am437x-vpfe.c @@ -1021,7 +1021,9 @@ static int vpfe_initialize_device(struct vpfe_device *vpfe) if (ret) return ret; - pm_runtime_get_sync(vpfe->pdev); + ret = pm_runtime_resume_and_get(vpfe->pdev); + if (ret < 0) + return ret; vpfe_config_enable(&vpfe->ccdc, 1); @@ -2443,7 +2445,11 @@ static int vpfe_probe(struct platform_device *pdev) pm_runtime_enable(&pdev->dev); /* for now just enable it here instead of waiting for the open */ - pm_runtime_get_sync(&pdev->dev); + ret = pm_runtime_resume_and_get(&pdev->dev); + if (ret < 0) { + vpfe_err(vpfe, "Unable to resume device.\n"); + goto probe_out_v4l2_unregister; + } vpfe_ccdc_config_defaults(ccdc); @@ -2527,10 +2533,11 @@ static int vpfe_suspend(struct device *dev) { struct vpfe_device *vpfe = dev_get_drvdata(dev); struct vpfe_ccdc *ccdc = &vpfe->ccdc; + int ret; /* only do full suspend if streaming has started */ if (vb2_start_streaming_called(&vpfe->buffer_queue)) { - pm_runtime_get_sync(dev); + ret = pm_runtime_resume_and_get(dev); vpfe_config_enable(ccdc, 1); /* Save VPFE context */ @@ -2541,7 +2548,8 @@ static int vpfe_suspend(struct device *dev) vpfe_config_enable(ccdc, 0); /* Disable both master and slave clock */ - pm_runtime_put_sync(dev); + if (ret >= 0) + pm_runtime_put_sync(dev); } /* Select sleep pin state */ @@ -2583,18 +2591,20 @@ static int vpfe_resume(struct device *dev) { struct vpfe_device *vpfe = dev_get_drvdata(dev); struct vpfe_ccdc *ccdc = &vpfe->ccdc; + int ret; /* only do full resume if streaming has started */ if (vb2_start_streaming_called(&vpfe->buffer_queue)) { /* Enable both master and slave clock */ - pm_runtime_get_sync(dev); + ret = pm_runtime_resume_and_get(dev); vpfe_config_enable(ccdc, 1); /* Restore VPFE context */ vpfe_restore_context(ccdc); vpfe_config_enable(ccdc, 0); - pm_runtime_put_sync(dev); + if (ret >= 0) + pm_runtime_put_sync(dev); } /* Select default pin state */
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, ensure that the driver will check if PM runtime resumed at vpfe_initialize_device(). Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> --- drivers/media/platform/am437x/am437x-vpfe.c | 22 +++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-)