| Message ID | 20190709095952.5666-1-geert+renesas@glider.be (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: fdp1: Reduce FCP not found message level to debug | expand |
Hi Geert, On 09/07/2019 10:59, Geert Uytterhoeven wrote: > When support for the IPMMU is not enabled, the FDP driver may be > probe-deferred multiple times, causing several messages to be printed > like: > > rcar_fdp1 fe940000.fdp1: FCP not found (-517) > rcar_fdp1 fe944000.fdp1: FCP not found (-517) > > Fix this by reducing the message level to debug level, as is done in the > VSP1 driver. Does the lack of IPMMU prevent the FDP1 being loaded entirely? If so is that a problem for us? (I thought we were able to run without the IPMMU) > Fixes: 4710b752e029f3f8 ("[media] v4l: Add Renesas R-Car FDP1 Driver") > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- > The alternative would be to add an explicit check for -EPROBE_DEFER. > --- > drivers/media/platform/rcar_fdp1.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/media/platform/rcar_fdp1.c b/drivers/media/platform/rcar_fdp1.c > index 43aae9b6bb20e3e8..c23ec127c2776f51 100644 > --- a/drivers/media/platform/rcar_fdp1.c > +++ b/drivers/media/platform/rcar_fdp1.c > @@ -2306,7 +2306,7 @@ static int fdp1_probe(struct platform_device *pdev) > fdp1->fcp = rcar_fcp_get(fcp_node); > of_node_put(fcp_node); > if (IS_ERR(fdp1->fcp)) { > - dev_err(&pdev->dev, "FCP not found (%ld)\n", > + dev_dbg(&pdev->dev, "FCP not found (%ld)\n", > PTR_ERR(fdp1->fcp)); Should we be doing something differently here in the event of no IPMMU then? Otherwise, if the IPMMU has become a hard dependency, should we add it to the Kconfig dependencies? > return PTR_ERR(fdp1->fcp); > } > -- Regards Kieran
Hi Kieran, On Tue, Jul 9, 2019 at 12:07 PM Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> wrote: > On 09/07/2019 10:59, Geert Uytterhoeven wrote: > > When support for the IPMMU is not enabled, the FDP driver may be > > probe-deferred multiple times, causing several messages to be printed > > like: > > > > rcar_fdp1 fe940000.fdp1: FCP not found (-517) > > rcar_fdp1 fe944000.fdp1: FCP not found (-517) > > > > Fix this by reducing the message level to debug level, as is done in the > > VSP1 driver. > > Does the lack of IPMMU prevent the FDP1 being loaded entirely? No it doesn't. If CONFIG_IPMMU_VMSA=n, rcar_fdp1 fe940000.fdp1: FCP not found (-517) rcar_fdp1 fe944000.fdp1: FCP not found (-517) rcar_fdp1 fe940000.fdp1: FCP not found (-517) rcar_fdp1 fe944000.fdp1: FCP not found (-517) rcar_fdp1 fe940000.fdp1: FCP not found (-517) rcar_fdp1 fe944000.fdp1: FCP not found (-517) rcar_fdp1 fe940000.fdp1: Device registered as /dev/video8 rcar_fdp1 fe944000.fdp1: Device registered as /dev/video9 So the driver succeeds, eventually. If CONFIG_IPMMU_VMSA=y, it succeeds sooner: rcar_fdp1 fe940000.fdp1: Device registered as /dev/video0 rcar_fdp1 fe944000.fdp1: Device registered as /dev/video1 Always be prepared to handle -EPROBE_DEFER. Gr{oetje,eeting}s, Geert
Hi Geert, On 09/07/2019 11:13, Geert Uytterhoeven wrote: > Hi Kieran, > > On Tue, Jul 9, 2019 at 12:07 PM Kieran Bingham > <kieran.bingham+renesas@ideasonboard.com> wrote: >> On 09/07/2019 10:59, Geert Uytterhoeven wrote: >>> When support for the IPMMU is not enabled, the FDP driver may be >>> probe-deferred multiple times, causing several messages to be printed >>> like: >>> >>> rcar_fdp1 fe940000.fdp1: FCP not found (-517) >>> rcar_fdp1 fe944000.fdp1: FCP not found (-517) >>> >>> Fix this by reducing the message level to debug level, as is done in the >>> VSP1 driver. >> >> Does the lack of IPMMU prevent the FDP1 being loaded entirely? > > No it doesn't. > If CONFIG_IPMMU_VMSA=n, > > rcar_fdp1 fe940000.fdp1: FCP not found (-517) > rcar_fdp1 fe944000.fdp1: FCP not found (-517) > rcar_fdp1 fe940000.fdp1: FCP not found (-517) > rcar_fdp1 fe944000.fdp1: FCP not found (-517) > rcar_fdp1 fe940000.fdp1: FCP not found (-517) > rcar_fdp1 fe944000.fdp1: FCP not found (-517) > rcar_fdp1 fe940000.fdp1: Device registered as /dev/video8 > rcar_fdp1 fe944000.fdp1: Device registered as /dev/video9 > > So the driver succeeds, eventually. > > If CONFIG_IPMMU_VMSA=y, it succeeds sooner: > > rcar_fdp1 fe940000.fdp1: Device registered as /dev/video0 > rcar_fdp1 fe944000.fdp1: Device registered as /dev/video1 > > Always be prepared to handle -EPROBE_DEFER. > > Gr{oetje,eeting}s, On the basis that the driver is not prevented from loading, then the message does indeed become more of a debug print. I wonder if it's better to print something different in the event of EPROBE_DEFER vs another actual error preventing the loading of the driver, but in either case if someone hits an issue they're likely to start adding/enabling debug. So, with that and a precedent for this change in VSP1, I'm happy here. Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > > Geert >
Hi Kieran, On Tue, Jul 9, 2019 at 12:18 PM Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> wrote: > On 09/07/2019 11:13, Geert Uytterhoeven wrote: > > On Tue, Jul 9, 2019 at 12:07 PM Kieran Bingham > > <kieran.bingham+renesas@ideasonboard.com> wrote: > >> On 09/07/2019 10:59, Geert Uytterhoeven wrote: > >>> When support for the IPMMU is not enabled, the FDP driver may be > >>> probe-deferred multiple times, causing several messages to be printed > >>> like: > >>> > >>> rcar_fdp1 fe940000.fdp1: FCP not found (-517) > >>> rcar_fdp1 fe944000.fdp1: FCP not found (-517) > >>> > >>> Fix this by reducing the message level to debug level, as is done in the > >>> VSP1 driver. > >> > >> Does the lack of IPMMU prevent the FDP1 being loaded entirely? > > > > No it doesn't. > > If CONFIG_IPMMU_VMSA=n, > > > > rcar_fdp1 fe940000.fdp1: FCP not found (-517) > > rcar_fdp1 fe944000.fdp1: FCP not found (-517) > > rcar_fdp1 fe940000.fdp1: FCP not found (-517) > > rcar_fdp1 fe944000.fdp1: FCP not found (-517) > > rcar_fdp1 fe940000.fdp1: FCP not found (-517) > > rcar_fdp1 fe944000.fdp1: FCP not found (-517) > > rcar_fdp1 fe940000.fdp1: Device registered as /dev/video8 > > rcar_fdp1 fe944000.fdp1: Device registered as /dev/video9 > > > > So the driver succeeds, eventually. > > > > If CONFIG_IPMMU_VMSA=y, it succeeds sooner: > > > > rcar_fdp1 fe940000.fdp1: Device registered as /dev/video0 > > rcar_fdp1 fe944000.fdp1: Device registered as /dev/video1 > > > > Always be prepared to handle -EPROBE_DEFER. > > On the basis that the driver is not prevented from loading, then the > message does indeed become more of a debug print. > > I wonder if it's better to print something different in the event of > EPROBE_DEFER vs another actual error preventing the loading of the > driver, but in either case if someone hits an issue they're likely to > start adding/enabling debug. That's why some driver suppress printing errors for -EPROBE_DEFER. > So, with that and a precedent for this change in VSP1, I'm happy here. > > Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> Thanks! Gr{oetje,eeting}s, Geert
diff --git a/drivers/media/platform/rcar_fdp1.c b/drivers/media/platform/rcar_fdp1.c index 43aae9b6bb20e3e8..c23ec127c2776f51 100644 --- a/drivers/media/platform/rcar_fdp1.c +++ b/drivers/media/platform/rcar_fdp1.c @@ -2306,7 +2306,7 @@ static int fdp1_probe(struct platform_device *pdev) fdp1->fcp = rcar_fcp_get(fcp_node); of_node_put(fcp_node); if (IS_ERR(fdp1->fcp)) { - dev_err(&pdev->dev, "FCP not found (%ld)\n", + dev_dbg(&pdev->dev, "FCP not found (%ld)\n", PTR_ERR(fdp1->fcp)); return PTR_ERR(fdp1->fcp); }
When support for the IPMMU is not enabled, the FDP driver may be probe-deferred multiple times, causing several messages to be printed like: rcar_fdp1 fe940000.fdp1: FCP not found (-517) rcar_fdp1 fe944000.fdp1: FCP not found (-517) Fix this by reducing the message level to debug level, as is done in the VSP1 driver. Fixes: 4710b752e029f3f8 ("[media] v4l: Add Renesas R-Car FDP1 Driver") Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> --- The alternative would be to add an explicit check for -EPROBE_DEFER. --- drivers/media/platform/rcar_fdp1.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)