in vc4_dsi_encoder_enable, the call to pm_runtime_get_sync increments
the counter even in case of failure, leading to incorrect
ref count. In case of failure, decrement the ref count before returning.
Signed-off-by: Navid Emamdoost <[email protected]>
---
drivers/gpu/drm/vc4/vc4_dsi.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c
index eaf276978ee7..e651de9d1c7d 100644
--- a/drivers/gpu/drm/vc4/vc4_dsi.c
+++ b/drivers/gpu/drm/vc4/vc4_dsi.c
@@ -838,7 +838,7 @@ static void vc4_dsi_encoder_enable(struct drm_encoder *encoder)
ret = pm_runtime_get_sync(dev);
if (ret) {
DRM_ERROR("Failed to runtime PM enable on DSI%d\n", dsi->port);
- return;
+ goto out;
}
if (debug_dump_regs) {
@@ -916,13 +916,13 @@ static void vc4_dsi_encoder_enable(struct drm_encoder *encoder)
ret = clk_prepare_enable(dsi->escape_clock);
if (ret) {
DRM_ERROR("Failed to turn on DSI escape clock: %d\n", ret);
- return;
+ goto out;
}
ret = clk_prepare_enable(dsi->pll_phy_clock);
if (ret) {
DRM_ERROR("Failed to turn on DSI PLL: %d\n", ret);
- return;
+ goto out;
}
hs_clock = clk_get_rate(dsi->pll_phy_clock);
@@ -944,7 +944,7 @@ static void vc4_dsi_encoder_enable(struct drm_encoder *encoder)
ret = clk_prepare_enable(dsi->pixel_clock);
if (ret) {
DRM_ERROR("Failed to turn on DSI pixel clock: %d\n", ret);
- return;
+ goto out;
}
/* How many ns one DSI unit interval is. Note that the clock
@@ -1088,6 +1088,8 @@ static void vc4_dsi_encoder_enable(struct drm_encoder *encoder)
dev_info(&dsi->pdev->dev, "DSI regs after:\n");
drm_print_regset32(&p, &dsi->regset);
}
+out:
+ pm_runtime_put(dev);
}
static ssize_t vc4_dsi_host_transfer(struct mipi_dsi_host *host,
--
2.17.1
> in vc4_dsi_encoder_enable, the call to pm_runtime_get_sync increments
> the counter even in case of failure, leading to incorrect
> ref count. In case of failure, decrement the ref count before returning.
* Can the term “reference count” become relevant also for this commit message
besides other possible adjustments?
* Can it be nicer to combine proposed updates for this software module
as a patch series (with a cover letter)?
* Would you like to add the tag “Fixes”?
…
> +++ b/drivers/gpu/drm/vc4/vc4_dsi.c
…
> @@ -1088,6 +1088,8 @@ static void vc4_dsi_encoder_enable(struct drm_encoder *encoder)
> dev_info(&dsi->pdev->dev, "DSI regs after:\n");
> drm_print_regset32(&p, &dsi->regset);
> }
> +out:
> + pm_runtime_put(dev);
> }
…
* Perhaps use the label “put_runtime” instead?
* Do you propose to perform an additional function call always
(and not only according to failure cases)?
* How do you think about calling the function “pm_runtime_put_noidle”?
Regards,
Markus
On Sun, Jun 14, 2020 at 9:55 AM Navid Emamdoost
<[email protected]> wrote:
>
> in vc4_dsi_encoder_enable, the call to pm_runtime_get_sync increments
> the counter even in case of failure, leading to incorrect
> ref count. In case of failure, decrement the ref count before returning.
...
> +out:
> + pm_runtime_put(dev);
Better to use pm_runtime_put_noidle() for error case.
And here is a change of semantics, i.e. before your patch there was no
put at all. How did you test this?
--
With Best Regards,
Andy Shevchenko
On Sun, Jun 14, 2020 at 7:32 AM Andy Shevchenko
<[email protected]> wrote:
>
> On Sun, Jun 14, 2020 at 9:55 AM Navid Emamdoost
> <[email protected]> wrote:
> >
> > in vc4_dsi_encoder_enable, the call to pm_runtime_get_sync increments
> > the counter even in case of failure, leading to incorrect
> > ref count. In case of failure, decrement the ref count before returning.
>
> ...
>
> > +out:
> > + pm_runtime_put(dev);
>
> Better to use pm_runtime_put_noidle() for error case.
> And here is a change of semantics, i.e. before your patch there was no
> put at all. How did you test this?
I had no way to test this but looked to me like a miscalculation of
ref count when there is a get and an error happens then the ref count
should be restored. Does that look incorrect?
>
> --
> With Best Regards,
> Andy Shevchenko
--
Navid.