| Message ID | 1470757001-4333-1-git-send-email-geert+renesas@glider.be (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
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);
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
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.
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
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 :-/
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
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);
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(-)