[PATCH] staging: fbtft: core: set smem_len before fb_deferred_io_init call

[PATCH] staging: fbtft: core: set smem_len before fb_deferred_io_init call

[Peter Suti]

fb_deferred_io_init depends on smem_len being filled
to be able to initialize the virtual page lists since
commit 856082f021a2 ("fbdev: defio: fix the pagelist corruption")

Signed-off-by: Peter Suti <peter.suti@streamunlimited.com>
---
 drivers/staging/fbtft/fbtft-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/fbtft/fbtft-core.c b/drivers/staging/fbtft/fbtft-core.c
index 9c4d797e7ae4..4137c1a51e1b 100644
--- a/drivers/staging/fbtft/fbtft-core.c
+++ b/drivers/staging/fbtft/fbtft-core.c
@@ -656,7 +656,6 @@ struct fb_info *fbtft_framebuffer_alloc(struct fbtft_display *display,
 	fbdefio->delay =           HZ / fps;
 	fbdefio->sort_pagelist =   true;
 	fbdefio->deferred_io =     fbtft_deferred_io;
-	fb_deferred_io_init(info);
 
 	snprintf(info->fix.id, sizeof(info->fix.id), "%s", dev->driver->name);
 	info->fix.type =           FB_TYPE_PACKED_PIXELS;
@@ -667,6 +666,7 @@ struct fb_info *fbtft_framebuffer_alloc(struct fbtft_display *display,
 	info->fix.line_length =    width * bpp / 8;
 	info->fix.accel =          FB_ACCEL_NONE;
 	info->fix.smem_len =       vmem_size;
+	fb_deferred_io_init(info);
 
 	info->var.rotate =         pdata->rotate;
 	info->var.xres =           width;
-- 
2.25.1

Re: [PATCH] staging: fbtft: core: set smem_len before fb_deferred_io_init call

[Dan Carpenter]

Thanks for the patch.

On Tue, Jul 26, 2022 at 10:21:13AM +0200, Peter Suti wrote:
> fb_deferred_io_init depends on smem_len being filled
> to be able to initialize the virtual page lists since
> commit 856082f021a2 ("fbdev: defio: fix the pagelist corruption")
> 

This code has changed since then so the patch needs to be updated.
The patch is still necessary but the bug will look different now
because there was a WARN_ON() added.

Currently the commit message does not say how this bug looks like to the
user.  Also the use a Fixes tag.  Something like this:

The fbtft_framebuffer_alloc() calls fb_deferred_io_init() before
initializing info->fix.smem_len.  It is set to zero by the
framebuffer_alloc() function.  It will trigger a WARN_ON() at the
start of fb_deferred_io_init() and the function will not do anything.

Fixes: 856082f021a2 ("fbdev: defio: fix the pagelist corruption")
Signed-off-by:

Make sure you CC the original author (Chuansheng Liu) so they can review
the bug fix.

Google used to give good guides for how to send a v2 patch but now the
first page is just useless.  :/

regards,
dan carpenter








> Signed-off-by: Peter Suti <peter.suti@streamunlimited.com>
> ---
>  drivers/staging/fbtft/fbtft-core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/fbtft/fbtft-core.c b/drivers/staging/fbtft/fbtft-core.c
> index 9c4d797e7ae4..4137c1a51e1b 100644
> --- a/drivers/staging/fbtft/fbtft-core.c
> +++ b/drivers/staging/fbtft/fbtft-core.c
> @@ -656,7 +656,6 @@ struct fb_info *fbtft_framebuffer_alloc(struct fbtft_display *display,
>  	fbdefio->delay =           HZ / fps;
>  	fbdefio->sort_pagelist =   true;
>  	fbdefio->deferred_io =     fbtft_deferred_io;
> -	fb_deferred_io_init(info);
>  
>  	snprintf(info->fix.id, sizeof(info->fix.id), "%s", dev->driver->name);
>  	info->fix.type =           FB_TYPE_PACKED_PIXELS;
> @@ -667,6 +666,7 @@ struct fb_info *fbtft_framebuffer_alloc(struct fbtft_display *display,
>  	info->fix.line_length =    width * bpp / 8;
>  	info->fix.accel =          FB_ACCEL_NONE;
>  	info->fix.smem_len =       vmem_size;
> +	fb_deferred_io_init(info);
>  
>  	info->var.rotate =         pdata->rotate;
>  	info->var.xres =           width;
> -- 
> 2.25.1
> 

[PATCH v2] staging: fbtft: core: set smem_len before fb_deferred_io_init call

[Peter Suti]

The fbtft_framebuffer_alloc() calls fb_deferred_io_init() before
initializing info->fix.smem_len.  It is set to zero by the
framebuffer_alloc() function.  It will trigger a WARN_ON() at the
start of fb_deferred_io_init() and the function will not do anything.

Fixes: 856082f021a2 ("fbdev: defio: fix the pagelist corruption")

Signed-off-by: Peter Suti <peter.suti@streamunlimited.com>
---
 drivers/staging/fbtft/fbtft-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/fbtft/fbtft-core.c b/drivers/staging/fbtft/fbtft-core.c
index 9b3eaed80cdd..afaba94d1d1c 100644
--- a/drivers/staging/fbtft/fbtft-core.c
+++ b/drivers/staging/fbtft/fbtft-core.c
@@ -654,7 +654,6 @@ struct fb_info *fbtft_framebuffer_alloc(struct fbtft_display *display,
 	fbdefio->delay =            HZ / fps;
 	fbdefio->sort_pagereflist = true;
 	fbdefio->deferred_io =      fbtft_deferred_io;
-	fb_deferred_io_init(info);
 
 	snprintf(info->fix.id, sizeof(info->fix.id), "%s", dev->driver->name);
 	info->fix.type =           FB_TYPE_PACKED_PIXELS;
@@ -665,6 +664,7 @@ struct fb_info *fbtft_framebuffer_alloc(struct fbtft_display *display,
 	info->fix.line_length =    width * bpp / 8;
 	info->fix.accel =          FB_ACCEL_NONE;
 	info->fix.smem_len =       vmem_size;
+	fb_deferred_io_init(info);
 
 	info->var.rotate =         pdata->rotate;
 	info->var.xres =           width;
-- 
2.25.1

Re: [PATCH v2] staging: fbtft: core: set smem_len before fb_deferred_io_init call

[Greg Kroah-Hartman]

On Wed, Jul 27, 2022 at 09:07:23AM +0200, Peter Suti wrote:
> The fbtft_framebuffer_alloc() calls fb_deferred_io_init() before
> initializing info->fix.smem_len.  It is set to zero by the
> framebuffer_alloc() function.  It will trigger a WARN_ON() at the
> start of fb_deferred_io_init() and the function will not do anything.
> 
> Fixes: 856082f021a2 ("fbdev: defio: fix the pagelist corruption")
> 
> Signed-off-by: Peter Suti <peter.suti@streamunlimited.com>
> ---
>  drivers/staging/fbtft/fbtft-core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/fbtft/fbtft-core.c b/drivers/staging/fbtft/fbtft-core.c
> index 9b3eaed80cdd..afaba94d1d1c 100644
> --- a/drivers/staging/fbtft/fbtft-core.c
> +++ b/drivers/staging/fbtft/fbtft-core.c
> @@ -654,7 +654,6 @@ struct fb_info *fbtft_framebuffer_alloc(struct fbtft_display *display,
>  	fbdefio->delay =            HZ / fps;
>  	fbdefio->sort_pagereflist = true;
>  	fbdefio->deferred_io =      fbtft_deferred_io;
> -	fb_deferred_io_init(info);
>  
>  	snprintf(info->fix.id, sizeof(info->fix.id), "%s", dev->driver->name);
>  	info->fix.type =           FB_TYPE_PACKED_PIXELS;
> @@ -665,6 +664,7 @@ struct fb_info *fbtft_framebuffer_alloc(struct fbtft_display *display,
>  	info->fix.line_length =    width * bpp / 8;
>  	info->fix.accel =          FB_ACCEL_NONE;
>  	info->fix.smem_len =       vmem_size;
> +	fb_deferred_io_init(info);
>  
>  	info->var.rotate =         pdata->rotate;
>  	info->var.xres =           width;
> -- 
> 2.25.1
> 
> 

Hi,

This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
a patch that has triggered this response.  He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created.  Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.

You are receiving this message because of the following common error(s)
as indicated below:

- This looks like a new version of a previously submitted patch, but you
  did not list below the --- line any changes from the previous version.
  Please read the section entitled "The canonical patch format" in the
  kernel file, Documentation/SubmittingPatches for what needs to be done
  here to properly describe this.

If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.

thanks,

greg k-h's patch email bot

[PATCH v3] staging: fbtft: core: set smem_len before fb_deferred_io_init call

[Peter Suti]

The fbtft_framebuffer_alloc() calls fb_deferred_io_init() before
initializing info->fix.smem_len.  It is set to zero by the
framebuffer_alloc() function.  It will trigger a WARN_ON() at the
start of fb_deferred_io_init() and the function will not do anything.

Fixes: 856082f021a2 ("fbdev: defio: fix the pagelist corruption")

Signed-off-by: Peter Suti <peter.suti@streamunlimited.com>
---
 V2 -> V3: Add patch changelog 
 V1 -> V2: Change commit message and base on top of linux-next

 drivers/staging/fbtft/fbtft-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/fbtft/fbtft-core.c b/drivers/staging/fbtft/fbtft-core.c
index 9b3eaed80cdd..afaba94d1d1c 100644
--- a/drivers/staging/fbtft/fbtft-core.c
+++ b/drivers/staging/fbtft/fbtft-core.c
@@ -654,7 +654,6 @@ struct fb_info *fbtft_framebuffer_alloc(struct fbtft_display *display,
 	fbdefio->delay =            HZ / fps;
 	fbdefio->sort_pagereflist = true;
 	fbdefio->deferred_io =      fbtft_deferred_io;
-	fb_deferred_io_init(info);
 
 	snprintf(info->fix.id, sizeof(info->fix.id), "%s", dev->driver->name);
 	info->fix.type =           FB_TYPE_PACKED_PIXELS;
@@ -665,6 +664,7 @@ struct fb_info *fbtft_framebuffer_alloc(struct fbtft_display *display,
 	info->fix.line_length =    width * bpp / 8;
 	info->fix.accel =          FB_ACCEL_NONE;
 	info->fix.smem_len =       vmem_size;
+	fb_deferred_io_init(info);
 
 	info->var.rotate =         pdata->rotate;
 	info->var.xres =           width;
-- 
2.25.1

RE: [PATCH v3] staging: fbtft: core: set smem_len before fb_deferred_io_init call

[Liu, Chuansheng]

Hi Peter,

> -----Original Message-----
> From: Peter Suti <peter.suti@streamunlimited.com>
> Sent: Wednesday, July 27, 2022 3:36 PM
> To: Liu, Chuansheng <chuansheng.liu@intel.com>; dan.carpenter@oracle.com;
> Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Thomas Zimmermann
> <tzimmermann@suse.de>; Javier Martinez Canillas <javierm@redhat.com>
> Cc: Peter Suti <peter.suti@streamunlimited.com>; dri-
> devel@lists.freedesktop.org; linux-fbdev@vger.kernel.org; linux-
> staging@lists.linux.dev; linux-kernel@vger.kernel.org
> Subject: [PATCH v3] staging: fbtft: core: set smem_len before
> fb_deferred_io_init call
> 
> The fbtft_framebuffer_alloc() calls fb_deferred_io_init() before
> initializing info->fix.smem_len.  It is set to zero by the
> framebuffer_alloc() function.  It will trigger a WARN_ON() at the
> start of fb_deferred_io_init() and the function will not do anything.

Please show us the WARN_ON() call stack, then everything is clear. It is the driver itself issue to be fixed.
I saw Thomas made such WARN_ON().

> 
> Fixes: 856082f021a2 ("fbdev: defio: fix the pagelist corruption")

Don't agree with such description.

Best Regards
Chuansheng

Re: [PATCH v3] staging: fbtft: core: set smem_len before fb_deferred_io_init call

[Dan Carpenter]

On Thu, Jul 28, 2022 at 12:49:15PM +0000, Liu, Chuansheng wrote:
> Hi Peter,
> 
> > 
> > The fbtft_framebuffer_alloc() calls fb_deferred_io_init() before
> > initializing info->fix.smem_len.  It is set to zero by the
> > framebuffer_alloc() function.  It will trigger a WARN_ON() at the
> > start of fb_deferred_io_init() and the function will not do anything.
> 
> Please show us the WARN_ON() call stack, then everything is clear.
> It is the driver itself issue to be fixed. I saw Thomas made such
> WARN_ON().

I don't understand the confusion here.  The code is very simple and the
description seems very clear.  No need to redo the patch.  I think
Peter tested it before the WARN_ON() was added so he probably didn't
see the WARN_ON().  I told him to add that because it's pretty obvious
what will happen just from reading the code.

> 
> > 
> > Fixes: 856082f021a2 ("fbdev: defio: fix the pagelist corruption")
> 
> Don't agree with such description.

I don't see how you can disagree?  Before your patch the
fb_deferred_io_init() did not use info->fix.smem_len and now it does.

regards,
dan carpenter