fbdev: au1100fb: Add missing check for clk_enable

[PATCH] fbdev: au1100fb: Add missing check for clk_enable

Posted by Chen Ni 1 week, 2 days ago

Add check for the return value of clk_enable() andreturn the error
if it fails in order to catch the error.

Signed-off-by: Chen Ni <nichen@iscas.ac.cn>
---
 drivers/video/fbdev/au1100fb.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/video/fbdev/au1100fb.c b/drivers/video/fbdev/au1100fb.c
index 6251a6b07b3a..5e7a8f018b7b 100644
--- a/drivers/video/fbdev/au1100fb.c
+++ b/drivers/video/fbdev/au1100fb.c
@@ -567,13 +567,16 @@ int au1100fb_drv_suspend(struct platform_device *dev, pm_message_t state)
 int au1100fb_drv_resume(struct platform_device *dev)
 {
 	struct au1100fb_device *fbdev = platform_get_drvdata(dev);
+	int  ret;
 
 	if (!fbdev)
 		return 0;
 
 	memcpy(fbdev->regs, &fbregs, sizeof(struct au1100fb_regs));
 
-	clk_enable(fbdev->lcdclk);
+	ret = clk_enable(fbdev->lcdclk);
+	if (ret)
+		return ret;
 
 	/* Unblank the LCD */
 	au1100fb_fb_blank(VESA_NO_BLANKING, &fbdev->info);
-- 
2.25.1

Re: [PATCH] fbdev: au1100fb: Add missing check for clk_enable

Posted by Helge Deller 1 week, 2 days ago

On 1/28/26 10:10, Chen Ni wrote:
> Add check for the return value of clk_enable() andreturn the error
> if it fails in order to catch the error.

Why?
If it really fails, the screen and everything stay blank,
so it's even worse....
It seems nobody is actually checking the return value, most likely
because if it fails, the system will sleep forever anyway.
Or am I missing something?

For now I reject this patch.

Helge


> Signed-off-by: Chen Ni <nichen@iscas.ac.cn>
> ---
>   drivers/video/fbdev/au1100fb.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/video/fbdev/au1100fb.c b/drivers/video/fbdev/au1100fb.c
> index 6251a6b07b3a..5e7a8f018b7b 100644
> --- a/drivers/video/fbdev/au1100fb.c
> +++ b/drivers/video/fbdev/au1100fb.c
> @@ -567,13 +567,16 @@ int au1100fb_drv_suspend(struct platform_device *dev, pm_message_t state)
>   int au1100fb_drv_resume(struct platform_device *dev)
>   {
>   	struct au1100fb_device *fbdev = platform_get_drvdata(dev);
> +	int  ret;
>   
>   	if (!fbdev)
>   		return 0;
>   
>   	memcpy(fbdev->regs, &fbregs, sizeof(struct au1100fb_regs));
>   
> -	clk_enable(fbdev->lcdclk);
> +	ret = clk_enable(fbdev->lcdclk);
> +	if (ret)
> +		return ret;
>   
>   	/* Unblank the LCD */
>   	au1100fb_fb_blank(VESA_NO_BLANKING, &fbdev->info);

Re: [PATCH] fbdev: au1100fb: Add missing check for clk_enable

Posted by Uwe Kleine-König 1 week, 2 days ago

On Wed, Jan 28, 2026 at 02:29:37PM +0100, Helge Deller wrote:
> On 1/28/26 10:10, Chen Ni wrote:
> > Add check for the return value of clk_enable() andreturn the error
> > if it fails in order to catch the error.
> 
> Why?
> If it really fails, the screen and everything stay blank,
> so it's even worse....
> It seems nobody is actually checking the return value, most likely
> because if it fails, the system will sleep forever anyway.
> Or am I missing something?
> 
> For now I reject this patch.

I think it's the right approach to check the return value. If
au1100fb_drv_resume() fails, the system as a whole will still be
up and it will know that the fb failed to resume.

With the status quo it will assume it's properly resumed despite it
didn't.

Still there are some things so criticise:

	- s/Add missing check for clk_enable/Check return value of clk_enable() in .resume().
	- s/  / /

While looking at the driver I spotted another issue: There is a single

	static struct au1100fb_regs fbregs;

to store the register values. This only works if there are <= 1 such
devices (and wastes memory if there is no such device).

Usually there is at most one such device, but it's cheap to move the
struct into struct au1100fb_device, and if it's only to make the driver
a better template for new drivers.

Best regards
Uwe

[PATCH v2] fbdev: au1100fb: Check return value of clk_enable() in .resume()

Posted by Chen Ni 1 week, 2 days ago

Check the return value of clk_enable() in au1100fb_drv_resume() and
return the error on failure.
This ensures the system is aware of the resume failure and can track
its state accurately.

Signed-off-by: Chen Ni <nichen@iscas.ac.cn>
---
Changes in v2:
- Update commit message
- Clean up extraneous whitespace in the code
---
 drivers/video/fbdev/au1100fb.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/video/fbdev/au1100fb.c b/drivers/video/fbdev/au1100fb.c
index 6251a6b07b3a..feaa1061c436 100644
--- a/drivers/video/fbdev/au1100fb.c
+++ b/drivers/video/fbdev/au1100fb.c
@@ -567,13 +567,16 @@ int au1100fb_drv_suspend(struct platform_device *dev, pm_message_t state)
 int au1100fb_drv_resume(struct platform_device *dev)
 {
 	struct au1100fb_device *fbdev = platform_get_drvdata(dev);
+	int ret;
 
 	if (!fbdev)
 		return 0;
 
 	memcpy(fbdev->regs, &fbregs, sizeof(struct au1100fb_regs));
 
-	clk_enable(fbdev->lcdclk);
+	ret = clk_enable(fbdev->lcdclk);
+	if (ret)
+		return ret;
 
 	/* Unblank the LCD */
 	au1100fb_fb_blank(VESA_NO_BLANKING, &fbdev->info);
-- 
2.25.1

Re: [PATCH v2] fbdev: au1100fb: Check return value of clk_enable() in .resume()

Posted by Markus Elfring 1 week, 1 day ago

> Check the return value of clk_enable() in au1100fb_drv_resume() and
> return the error on failure.

Were any source code analysis tools involved here?


> This ensures the system is aware of the resume failure and can track

Ensure?


> its state accurately.

Did anything hinder to add any tags (like “Fixes” and “Cc”) accordingly?

Regards,
Markus

Re: [PATCH v2] fbdev: au1100fb: Check return value of clk_enable() in .resume()

Posted by Uwe Kleine-König 1 week, 1 day ago

On Thu, Jan 29, 2026 at 12:07:14PM +0800, Chen Ni wrote:
> Check the return value of clk_enable() in au1100fb_drv_resume() and
> return the error on failure.
> This ensures the system is aware of the resume failure and can track
> its state accurately.
> 
> Signed-off-by: Chen Ni <nichen@iscas.ac.cn>

Acked-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>

Thanks
Uwe

Re: [PATCH v2] fbdev: au1100fb: Check return value of clk_enable() in .resume()

Posted by Helge Deller 1 week, 1 day ago

On 1/29/26 08:13, Uwe Kleine-König wrote:
> On Thu, Jan 29, 2026 at 12:07:14PM +0800, Chen Ni wrote:
>> Check the return value of clk_enable() in au1100fb_drv_resume() and
>> return the error on failure.
>> This ensures the system is aware of the resume failure and can track
>> its state accurately.
>>
>> Signed-off-by: Chen Ni <nichen@iscas.ac.cn>
> 
> Acked-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>

Applied to fbdev git tree.

Uwe, thanks for feedback & double-checking!
Helge

Re: [PATCH] fbdev: au1100fb: Add missing check for clk_enable

Posted by Markus Elfring 1 week, 2 days ago

> Add check for the return value of clk_enable() andreturn the error

                                                 and return?


> if it fails in order to catch the error.

How do you think about to add any tags (like “Fixes” and “Cc”) accordingly?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.19-rc7#n145

Regards,
Markus