[PATCH] drm: shmob_drm: Check clk_prepare_enable() return value

[PATCH] drm: shmob_drm: Check clk_prepare_enable() return value

[Laurent Pinchart]

The clk_prepare_enable() call can fail. Check it's return value. We
can't propagate it all the way to the user as the KMS operations in
which the clock is enabled return a void.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/gpu/drm/shmobile/shmob_drm_crtc.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/shmobile/shmob_drm_crtc.c b/drivers/gpu/drm/shmobile/shmob_drm_crtc.c
index 562f9a4..0428076 100644
--- a/drivers/gpu/drm/shmobile/shmob_drm_crtc.c
+++ b/drivers/gpu/drm/shmobile/shmob_drm_crtc.c
@@ -37,14 +37,21 @@
  * Clock management
  */
 
-static void shmob_drm_clk_on(struct shmob_drm_device *sdev)
+static int shmob_drm_clk_on(struct shmob_drm_device *sdev)
 {
-	if (sdev->clock)
-		clk_prepare_enable(sdev->clock);
+	int ret;
+
+	if (sdev->clock) {
+		ret = clk_prepare_enable(sdev->clock);
+		if (ret < 0)
+			return ret;
+	}
 #if 0
 	if (sdev->meram_dev && sdev->meram_dev->pdev)
 		pm_runtime_get_sync(&sdev->meram_dev->pdev->dev);
 #endif
+
+	return 0;
 }
 
 static void shmob_drm_clk_off(struct shmob_drm_device *sdev)
@@ -161,6 +168,7 @@ static void shmob_drm_crtc_start(struct shmob_drm_crtc *scrtc)
 	struct drm_device *dev = sdev->ddev;
 	struct drm_plane *plane;
 	u32 value;
+	int ret;
 
 	if (scrtc->started)
 		return;
@@ -170,7 +178,9 @@ static void shmob_drm_crtc_start(struct shmob_drm_crtc *scrtc)
 		return;
 
 	/* Enable clocks before accessing the hardware. */
-	shmob_drm_clk_on(sdev);
+	ret = shmob_drm_clk_on(sdev);
+	if (ret < 0)
+		return;
 
 	/* Reset and enable the LCDC. */
 	lcdc_write(sdev, LDCNT2R, lcdc_read(sdev, LDCNT2R) | LDCNT2R_BR);
-- 
1.8.3.2

Re: [PATCH] drm: shmob_drm: Check clk_prepare_enable() return value

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 585 bytes --]

On Mon, Dec 02, 2013 at 01:52:20AM +0100, Laurent Pinchart wrote:
[...]
> diff --git a/drivers/gpu/drm/shmobile/shmob_drm_crtc.c b/drivers/gpu/drm/shmobile/shmob_drm_crtc.c
[...]
> @@ -170,7 +178,9 @@ static void shmob_drm_crtc_start(struct shmob_drm_crtc *scrtc)
>  		return;
>  
>  	/* Enable clocks before accessing the hardware. */
> -	shmob_drm_clk_on(sdev);
> +	ret = shmob_drm_clk_on(sdev);
> +	if (ret < 0)
> +		return;

Perhaps this should be printing an error or using WARN_ON()? Otherwise
it might be very difficult to diagnose what's going wrong.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: shmob_drm: Check clk_prepare_enable() return value

[Laurent Pinchart]

[-- Attachment #1.1: Type: text/plain, Size: 963 bytes --]

Hi Thierry,

On Tuesday 03 December 2013 10:59:24 Thierry Reding wrote:
> On Mon, Dec 02, 2013 at 01:52:20AM +0100, Laurent Pinchart wrote:
> [...]
> 
> > diff --git a/drivers/gpu/drm/shmobile/shmob_drm_crtc.c
> > b/drivers/gpu/drm/shmobile/shmob_drm_crtc.c
> [...]
> 
> > @@ -170,7 +178,9 @@ static void shmob_drm_crtc_start(struct shmob_drm_crtc
> > *scrtc)
> >  		return;
> >  	
> >  	/* Enable clocks before accessing the hardware. */
> > -	shmob_drm_clk_on(sdev);
> > +	ret = shmob_drm_clk_on(sdev);
> > +	if (ret < 0)
> > +		return;
> 
> Perhaps this should be printing an error or using WARN_ON()? Otherwise
> it might be very difficult to diagnose what's going wrong.

That's a good point. In practice, as the driver always uses a Renesas SoC MSTP 
clock, the clock code will print a message to the kernel log though, so I'm 
not sure whether we should duplicate that here. If you think we should I'll 
submit a new patch.

-- 
Regards,

Laurent Pinchart

[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: shmob_drm: Check clk_prepare_enable() return value

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 1229 bytes --]

On Wed, Dec 04, 2013 at 02:38:21AM +0100, Laurent Pinchart wrote:
> Hi Thierry,
> 
> On Tuesday 03 December 2013 10:59:24 Thierry Reding wrote:
> > On Mon, Dec 02, 2013 at 01:52:20AM +0100, Laurent Pinchart wrote:
> > [...]
> > 
> > > diff --git a/drivers/gpu/drm/shmobile/shmob_drm_crtc.c
> > > b/drivers/gpu/drm/shmobile/shmob_drm_crtc.c
> > [...]
> > 
> > > @@ -170,7 +178,9 @@ static void shmob_drm_crtc_start(struct shmob_drm_crtc
> > > *scrtc)
> > >  		return;
> > >  	
> > >  	/* Enable clocks before accessing the hardware. */
> > > -	shmob_drm_clk_on(sdev);
> > > +	ret = shmob_drm_clk_on(sdev);
> > > +	if (ret < 0)
> > > +		return;
> > 
> > Perhaps this should be printing an error or using WARN_ON()? Otherwise
> > it might be very difficult to diagnose what's going wrong.
> 
> That's a good point. In practice, as the driver always uses a Renesas SoC MSTP 
> clock, the clock code will print a message to the kernel log though, so I'm 
> not sure whether we should duplicate that here. If you think we should I'll 
> submit a new patch.

Is that one of the clocks implemented in drivers/sh/clk? There don't
seem to be any error messages in the clk_enable() implementation.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: shmob_drm: Check clk_prepare_enable() return value

[Laurent Pinchart]

[-- Attachment #1.1: Type: text/plain, Size: 1681 bytes --]

Hi Thierry,

On Wednesday 04 December 2013 11:01:07 Thierry Reding wrote:
> On Wed, Dec 04, 2013 at 02:38:21AM +0100, Laurent Pinchart wrote:
> > On Tuesday 03 December 2013 10:59:24 Thierry Reding wrote:
> > > On Mon, Dec 02, 2013 at 01:52:20AM +0100, Laurent Pinchart wrote:
> > > [...]
> > > 
> > > > diff --git a/drivers/gpu/drm/shmobile/shmob_drm_crtc.c
> > > > b/drivers/gpu/drm/shmobile/shmob_drm_crtc.c
> > > 
> > > [...]
> > > 
> > > > @@ -170,7 +178,9 @@ static void shmob_drm_crtc_start(struct
> > > > shmob_drm_crtc
> > > > *scrtc)
> > > > 
> > > >  		return;
> > > >  	
> > > >  	/* Enable clocks before accessing the hardware. */
> > > > 
> > > > -	shmob_drm_clk_on(sdev);
> > > > +	ret = shmob_drm_clk_on(sdev);
> > > > +	if (ret < 0)
> > > > +		return;
> > > 
> > > Perhaps this should be printing an error or using WARN_ON()? Otherwise
> > > it might be very difficult to diagnose what's going wrong.
> > 
> > That's a good point. In practice, as the driver always uses a Renesas SoC
> > MSTP clock, the clock code will print a message to the kernel log though,
> > so I'm not sure whether we should duplicate that here. If you think we
> > should I'll submit a new patch.
> 
> Is that one of the clocks implemented in drivers/sh/clk?

Correct, and in drivers/clk/shmobile/clk-mstp.c when 
http://www.spinics.net/lists/devicetree/msg12696.html will be merged.

> There don't seem to be any error messages in the clk_enable()
> implementation.

There will be one when http://www.spinics.net/lists/linux-sh/msg25650.html 
will get merged :-) Before that enabling the clock never failed, with the 
patch applied the failure is logged.

-- 
Regards,

Laurent Pinchart

[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: shmob_drm: Check clk_prepare_enable() return value

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 1915 bytes --]

On Wed, Dec 04, 2013 at 03:37:48PM +0100, Laurent Pinchart wrote:
> Hi Thierry,
> 
> On Wednesday 04 December 2013 11:01:07 Thierry Reding wrote:
> > On Wed, Dec 04, 2013 at 02:38:21AM +0100, Laurent Pinchart wrote:
> > > On Tuesday 03 December 2013 10:59:24 Thierry Reding wrote:
> > > > On Mon, Dec 02, 2013 at 01:52:20AM +0100, Laurent Pinchart wrote:
> > > > [...]
> > > > 
> > > > > diff --git a/drivers/gpu/drm/shmobile/shmob_drm_crtc.c
> > > > > b/drivers/gpu/drm/shmobile/shmob_drm_crtc.c
> > > > 
> > > > [...]
> > > > 
> > > > > @@ -170,7 +178,9 @@ static void shmob_drm_crtc_start(struct
> > > > > shmob_drm_crtc
> > > > > *scrtc)
> > > > > 
> > > > >  		return;
> > > > >  	
> > > > >  	/* Enable clocks before accessing the hardware. */
> > > > > 
> > > > > -	shmob_drm_clk_on(sdev);
> > > > > +	ret = shmob_drm_clk_on(sdev);
> > > > > +	if (ret < 0)
> > > > > +		return;
> > > > 
> > > > Perhaps this should be printing an error or using WARN_ON()? Otherwise
> > > > it might be very difficult to diagnose what's going wrong.
> > > 
> > > That's a good point. In practice, as the driver always uses a Renesas SoC
> > > MSTP clock, the clock code will print a message to the kernel log though,
> > > so I'm not sure whether we should duplicate that here. If you think we
> > > should I'll submit a new patch.
> > 
> > Is that one of the clocks implemented in drivers/sh/clk?
> 
> Correct, and in drivers/clk/shmobile/clk-mstp.c when 
> http://www.spinics.net/lists/devicetree/msg12696.html will be merged.
> 
> > There don't seem to be any error messages in the clk_enable()
> > implementation.
> 
> There will be one when http://www.spinics.net/lists/linux-sh/msg25650.html 
> will get merged :-) Before that enabling the clock never failed, with the 
> patch applied the failure is logged.

Alright,

Reviewed-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: shmob_drm: Check clk_prepare_enable() return value

[Laurent Pinchart]

[-- Attachment #1.1: Type: text/plain, Size: 2071 bytes --]

Hello,

On Wednesday 04 December 2013 16:59:22 Thierry Reding wrote:
> On Wed, Dec 04, 2013 at 03:37:48PM +0100, Laurent Pinchart wrote:
> > On Wednesday 04 December 2013 11:01:07 Thierry Reding wrote:
> > > On Wed, Dec 04, 2013 at 02:38:21AM +0100, Laurent Pinchart wrote:
> > > > On Tuesday 03 December 2013 10:59:24 Thierry Reding wrote:
> > > > > On Mon, Dec 02, 2013 at 01:52:20AM +0100, Laurent Pinchart wrote:
> > > > > [...]
> > > > > 
> > > > > > diff --git a/drivers/gpu/drm/shmobile/shmob_drm_crtc.c
> > > > > > b/drivers/gpu/drm/shmobile/shmob_drm_crtc.c
> > > > > 
> > > > > [...]
> > > > > 
> > > > > > @@ -170,7 +178,9 @@ static void shmob_drm_crtc_start(struct
> > > > > > shmob_drm_crtc *scrtc)
> > > > > >  		return;
> > > > > >  	
> > > > > >  	/* Enable clocks before accessing the hardware. */
> > > > > > -	shmob_drm_clk_on(sdev);
> > > > > > +	ret = shmob_drm_clk_on(sdev);
> > > > > > +	if (ret < 0)
> > > > > > +		return;
> > > > > 
> > > > > Perhaps this should be printing an error or using WARN_ON()?
> > > > > Otherwise it might be very difficult to diagnose what's going wrong.
> > > > 
> > > > That's a good point. In practice, as the driver always uses a Renesas
> > > > SoC MSTP clock, the clock code will print a message to the kernel log
> > > > though, so I'm not sure whether we should duplicate that here. If you
> > > > think we should I'll submit a new patch.
> > > 
> > > Is that one of the clocks implemented in drivers/sh/clk?
> > 
> > Correct, and in drivers/clk/shmobile/clk-mstp.c when
> > http://www.spinics.net/lists/devicetree/msg12696.html will be merged.
> > 
> > > There don't seem to be any error messages in the clk_enable()
> > > implementation.
> > 
> > There will be one when http://www.spinics.net/lists/linux-sh/msg25650.html
> > will get merged :-) Before that enabling the clock never failed, with the
> > patch applied the failure is logged.
> 
> Alright,
> 
> Reviewed-by: Thierry Reding <treding@nvidia.com>

Thank you.

Dave, could you please pick this patch up for v3.14 ?

-- 
Regards,

Laurent Pinchart

[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel