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

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

[Geert Uytterhoeven]

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;
+
 	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);
 
-- 
1.9.1

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

[Laurent Pinchart]

Hi Geert,

Thank you for the patch.

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.

>  	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);

-- 
Regards,

Laurent Pinchart

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

[Geert Uytterhoeven]

Hi Laurent,

On Wed, Aug 17, 2016 at 2:55 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> 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?

>>       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,
-110 = ETIMEDOUT, returned by vsp1_device_init() due to the failure
to reset wpf.0.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Laurent Pinchart]

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

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

[Geert Uytterhoeven]

Hi Laurent,

On Mon, Sep 5, 2016 at 10:17 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>> 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 ? :-)

Unfortuately not.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Laurent Pinchart]

Hi Geert,

On Monday 05 Sep 2016 10:20:52 Geert Uytterhoeven wrote:
> On Mon, Sep 5, 2016 at 10:17 AM, Laurent Pinchart wrote:
> >> 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 ? :-)
> 
> Unfortuately not.

Is this caused by the fact that pm_runtime_get_sync() is called on the FCP 
device before the FCP gets system-resumed ? Lovely PM order dependency :-/

-- 
Regards,

Laurent Pinchart

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

[Geert Uytterhoeven]

Hi Laurent,

On Mon, Sep 5, 2016 at 10:25 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Monday 05 Sep 2016 10:20:52 Geert Uytterhoeven wrote:
>> On Mon, Sep 5, 2016 at 10:17 AM, Laurent Pinchart wrote:
>> >> 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 ? :-)
>>
>> Unfortuately not.
>
> Is this caused by the fact that pm_runtime_get_sync() is called on the FCP
> device before the FCP gets system-resumed ? Lovely PM order dependency :-/

It's called from resume_noirq. IIRC, it's called a second time from resume.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds