Re: [PATCH] [media] rcar-fcp: Make sure rcar_fcp_enable() returns 0 on success

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Geert,

On Tuesday 23 Aug 2016 15:11:59 Geert Uytterhoeven wrote:
> On Wed, Aug 17, 2016 at 2:55 PM, Laurent Pinchart wrote:
> > On Tuesday 09 Aug 2016 17:36:41 Geert Uytterhoeven wrote:
> >> When resuming from suspend-to-RAM on r8a7795/salvator-x:
> >>     dpm_run_callback(): pm_genpd_resume_noirq+0x0/0x90 returns 1
> >>     PM: Device fe940000.fdp1 failed to resume noirq: error 1
> >>     dpm_run_callback(): pm_genpd_resume_noirq+0x0/0x90 returns 1
> >>     PM: Device fe944000.fdp1 failed to resume noirq: error 1
> >>     dpm_run_callback(): pm_genpd_resume_noirq+0x0/0x90 returns 1
> >>     PM: Device fe948000.fdp1 failed to resume noirq: error 1
> >> 
> >> According to its documentation, rcar_fcp_enable() returns 0 on success
> >> or a negative error code if an error occurs.  Hence
> >> fdp1_pm_runtime_resume() and vsp1_pm_runtime_resume() forward its return
> >> value to their callers.
> >> 
> >> However, rcar_fcp_enable() forwards the return value of
> >> pm_runtime_get_sync(), which can actually be 1 on success, leading to
> >> the resume failure above.
> >> 
> >> To fix this, consider only negative values returned by
> >> pm_runtime_get_sync() to be failures.
> >> 
> >> Fixes: 7b49235e83b2347c ("[media] v4l: Add Renesas R-Car FCP driver")
> >> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> >> ---
> >> 
> >>  drivers/media/platform/rcar-fcp.c | 8 +++++++-
> >>  1 file changed, 7 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/drivers/media/platform/rcar-fcp.c
> >> b/drivers/media/platform/rcar-fcp.c index
> >> 0ff6b1edf1dbf677..7e944479205d4059 100644
> >> --- a/drivers/media/platform/rcar-fcp.c
> >> +++ b/drivers/media/platform/rcar-fcp.c
> >> @@ -99,10 +99,16 @@ EXPORT_SYMBOL_GPL(rcar_fcp_put);
> >> 
> >>   */
> >>  
> >>  int rcar_fcp_enable(struct rcar_fcp_device *fcp)
> >>  {
> >> 
> >> +     int error;
> > 
> > I was going to write that the driver uses "ret" instead of "error" for
> > integer status return values, but it doesn't as there no such value
> > stored in a variable at all. I will thus argue that it will use that
> > style later, so let's keep the style consistent with the to-be-written
> > code if you don't mind :-)
> > 
> > Apart from that,
> > 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> > and applied to my tree.
> 
> Thanks!
> 
> Where exactly has this been applied?

git://linuxtv.org/pinchartl/media.git vsp1/next

I see now that Mauro has applied it already. Mauro, could you please avoid 
merging patches that I take through my tree, especially when I request changes 
?

> >>       if (!fcp)
> >>       
> >>               return 0;
> >> 
> >> -     return pm_runtime_get_sync(fcp->dev);
> >> +     error = pm_runtime_get_sync(fcp->dev);
> >> +     if (error < 0)
> >> +             return error;
> >> +
> >> +     return 0;
> >> 
> >>  }
> >>  EXPORT_SYMBOL_GPL(rcar_fcp_enable);
> 
> BTW, it seems I missed a few more s2ram resume errors:
> 
>     dpm_run_callback(): pm_genpd_resume_noirq+0x0/0x90 returns -13
>     PM: Device fe920000.vsp failed to resume noirq: error -13
>     dpm_run_callback(): pm_genpd_resume_noirq+0x0/0x90 returns -13
>     PM: Device fe960000.vsp failed to resume noirq: error -13
>     dpm_run_callback(): pm_genpd_resume_noirq+0x0/0x90 returns -13
>     PM: Device fe9a0000.vsp failed to resume noirq: error -13
>     dpm_run_callback(): pm_genpd_resume_noirq+0x0/0x90 returns -13
>     PM: Device fe9b0000.vsp failed to resume noirq: error -13
>     dpm_run_callback(): pm_genpd_resume_noirq+0x0/0x90 returns -13
>     PM: Device fe9c0000.vsp failed to resume noirq: error -13
>     dpm_run_callback(): pm_genpd_resume_noirq+0x0/0x90 returns -13
>     PM: Device fea20000.vsp failed to resume noirq: error -13
>     dpm_run_callback(): pm_genpd_resume_noirq+0x0/0x90 returns -13
>     PM: Device fea28000.vsp failed to resume noirq: error -13
>     dpm_run_callback(): pm_genpd_resume_noirq+0x0/0x90 returns -13
>     PM: Device fea30000.vsp failed to resume noirq: error -13
>     vsp1 fea38000.vsp: failed to reset wpf.0
>     dpm_run_callback(): pm_genpd_resume_noirq+0x0/0x90 returns -110
>     PM: Device fea38000.vsp failed to resume noirq: error -110
> 
> -13 == -EACCES, returned by rcar_fcp_enable() as pm_runtime_get_sync()
> is called too early during system resume,

Do you have a fix for this ? :-)

> -110 = ETIMEDOUT, returned by vsp1_device_init() due to the failure
> to reset wpf.0.

This one needs to be investigated.

-- 
Regards,

Laurent Pinchart