Re: [PATCH] [RFC] drivers/staging/fbtft: fix sparse warnings
2015-02-23 21:27 GMT+02:00 Noralf Trønnes : > Yes, it's best to leave this alone for now. > I'm working on a proposal to provide better layering and minimal coupling > to fbdev. This will hopefully lead to screen_base eventually being used > only twice in the fbtft module and nowhere else. Ok, staying away and looking for sparse warnings for other staging drivers. Thanks to all for comments. -- Andrey Utkin -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [RFC] drivers/staging/fbtft: fix sparse warnings
diff --git a/drivers/staging/fbtft/fb_agm1264k-fl.c
b/drivers/staging/fbtft/fb_agm1264k-fl.c
index 9cc7d25..9114239 100644
--- a/drivers/staging/fbtft/fb_agm1264k-fl.c
+++ b/drivers/staging/fbtft/fb_agm1264k-fl.c
@@ -273,7 +273,7 @@ construct_line_bitmap(struct fbtft_par *par, u8 *dest,
signed short *src,
static int write_vmem(struct fbtft_par *par, size_t offset, size_t len)
{
- u16 *vmem16 = (u16 *)par->info->screen_base;
+ u16 __iomem *vmem16 = (u16 __iomem *)par->info->screen_base;
I haven't looked. What is the type for ->screen_base and why can't it
be declared as __iomem type?
http://lxr.free-electrons.com/source/include/linux/fb.h#L486
screen_base is component of struct fb_info, defined as "char __iomem *".
In drivers/staging/fbtft/fbtft-core.c, it looks to be actually set to
a pointer resulting from vzalloc().
Hm, you're right. Normally, it's an __iomem * but this time it's not
an __iomem pointer. Adding anotations to mark it as __iomem is wrong
and adding calls to ioread16() is buggy.
There are a couple ways to make these warnings go away. The simplest
is just to silence the warning with __force:
u16 *vmem16 = (u16 __force *)par->info->screen_base;
This is how some fbdev drivers with vmalloc'ed memory does this:
video/fbdev/{metronomefb.c,hecubafb.c}:
unsigned char *buf = (unsigned char __force *)par->info->screen_base;
info->screen_base = (char __force __iomem *)videomemory;
drivers/video/fbdev/ssd1307fb.c (this one is quite new: 3.15):
u8 __iomem *dst; dst = (void __force *) (info->screen_base + p);
info->screen_base = (u8 __force __iomem *)vmem;
We have to use screen_base because of vmalloc'ed memory and deferred io
(fb_deferred_io_page).
I'm not terribly familiar with this code. I don't know that this is the
cleanest approach. We could also just leave the code alone for now and
ignore the warning.
Yes, it's best to leave this alone for now.
I'm working on a proposal to provide better layering and minimal coupling
to fbdev. This will hopefully lead to screen_base eventually being used
only twice in the fbtft module and nowhere else.
Regards,
Noralf Trønnes
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [RFC] drivers/staging/fbtft: fix sparse warnings
On Mon, Feb 23, 2015 at 07:06:44PM +0200, Andrey Utkin wrote:
> 2015-02-21 20:58 GMT+02:00 Dan Carpenter :
> > Send two separate patches. You can't "fix" sparse warnings. You can
> > only "fix" bugs. The rest is add annotation, doing cleanups or possibly
> > silencing warnings.
>
> My first email wasn't a patch supposed for accepting, but rather a
> request for comments, so I didn't bother with commit granularity,
> separation of commit description and the description of my situation
> with scissors marker etc. Sorry if this is rude or confusing.
At least *try* to send proper patches. It is a waste of time to send
half finished patches with lazy butt changelogs, yes.
>
>
> >> diff --git a/drivers/staging/fbtft/fb_agm1264k-fl.c
> >> b/drivers/staging/fbtft/fb_agm1264k-fl.c
> >> index 9cc7d25..9114239 100644
> >> --- a/drivers/staging/fbtft/fb_agm1264k-fl.c
> >> +++ b/drivers/staging/fbtft/fb_agm1264k-fl.c
> >> @@ -273,7 +273,7 @@ construct_line_bitmap(struct fbtft_par *par, u8 *dest,
> >> signed short *src,
> >>
> >> static int write_vmem(struct fbtft_par *par, size_t offset, size_t len)
> >> {
> >> - u16 *vmem16 = (u16 *)par->info->screen_base;
> >> + u16 __iomem *vmem16 = (u16 __iomem *)par->info->screen_base;
> >
> > I haven't looked. What is the type for ->screen_base and why can't it
> > be declared as __iomem type?
>
> http://lxr.free-electrons.com/source/include/linux/fb.h#L486
> screen_base is component of struct fb_info, defined as "char __iomem *".
> In drivers/staging/fbtft/fbtft-core.c, it looks to be actually set to
> a pointer resulting from vzalloc().
Hm, you're right. Normally, it's an __iomem * but this time it's not
an __iomem pointer. Adding anotations to mark it as __iomem is wrong
and adding calls to ioread16() is buggy.
There are a couple ways to make these warnings go away. The simplest
is just to silence the warning with __force:
u16 *vmem16 = (u16 __force *)par->info->screen_base;
I'm not terribly familiar with this code. I don't know that this is the
cleanest approach. We could also just leave the code alone for now and
ignore the warning.
regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [RFC] drivers/staging/fbtft: fix sparse warnings
2015-02-21 20:58 GMT+02:00 Dan Carpenter :
> Send two separate patches. You can't "fix" sparse warnings. You can
> only "fix" bugs. The rest is add annotation, doing cleanups or possibly
> silencing warnings.
My first email wasn't a patch supposed for accepting, but rather a
request for comments, so I didn't bother with commit granularity,
separation of commit description and the description of my situation
with scissors marker etc. Sorry if this is rude or confusing.
>> diff --git a/drivers/staging/fbtft/fb_agm1264k-fl.c
>> b/drivers/staging/fbtft/fb_agm1264k-fl.c
>> index 9cc7d25..9114239 100644
>> --- a/drivers/staging/fbtft/fb_agm1264k-fl.c
>> +++ b/drivers/staging/fbtft/fb_agm1264k-fl.c
>> @@ -273,7 +273,7 @@ construct_line_bitmap(struct fbtft_par *par, u8 *dest,
>> signed short *src,
>>
>> static int write_vmem(struct fbtft_par *par, size_t offset, size_t len)
>> {
>> - u16 *vmem16 = (u16 *)par->info->screen_base;
>> + u16 __iomem *vmem16 = (u16 __iomem *)par->info->screen_base;
>
> I haven't looked. What is the type for ->screen_base and why can't it
> be declared as __iomem type?
http://lxr.free-electrons.com/source/include/linux/fb.h#L486
screen_base is component of struct fb_info, defined as "char __iomem *".
In drivers/staging/fbtft/fbtft-core.c, it looks to be actually set to
a pointer resulting from vzalloc().
At https://www.kernel.org/doc/htmldocs/kernel-api/API-vzalloc.html ,
there's no mention of the result being of __iomem nature. So at this
point I'm lost: this looks like inconsistence in driver "by design",
but I don't know enough about this driver. Maybe fbtft driver should
use some another variable in some driver-private structre, and not
screen_base from struct fb_info? Or maybe it should not implicitly
assume that memory allocated by vzalloc() behaves the same way that
properly __iomem-allocated memory? Sorry if my phrases are way too
wrong and sound stupid - please don't let me to die being a fool, give
a comment :)
>> u8 *buf = par->txbuf.buf;
>> int x, y;
>> int ret = 0;
>> @@ -287,7 +287,7 @@ static int write_vmem(struct fbtft_par *par, size_t
>> offset, size_t len)
>> /* converting to grayscale16 */
>> for (x = 0; x < par->info->var.xres; ++x)
>> for (y = 0; y < par->info->var.yres; ++y) {
>> - u16 pixel = vmem16[y * par->info->var.xres + x];
>> + u16 pixel = ioread16(vmem16 + y * par->info->var.xres
>> + x);
>
> You're saying this is a bug in the original code. Are you positive?
> The changelog should have explained your thinking here. Same for all
> the iomem changes.
vmem16 is set to a pointer from screen_base, which is _iomem, which
implicates the prohibition of dereferencing. Afrer some brief
searching, I've found that __iomem pointers are supposed to be read
and written with special functions like ioread16(). Also I've read the
fact that at some architectures, simple dereferencing works, but on
others it doesn't.
Of course I'm not sure that exactly this is the correct way to make
sparse happy and to improve correctness of the code (I'm avoiding a
word "to fix" :) ). See above my explanation of condtradiction in this
driver.
--
Andrey Utkin
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [RFC] drivers/staging/fbtft: fix sparse warnings
2015-02-21 20:58 GMT+02:00 Dan Carpenter dan.carpen...@oracle.com:
Send two separate patches. You can't fix sparse warnings. You can
only fix bugs. The rest is add annotation, doing cleanups or possibly
silencing warnings.
My first email wasn't a patch supposed for accepting, but rather a
request for comments, so I didn't bother with commit granularity,
separation of commit description and the description of my situation
with scissors marker etc. Sorry if this is rude or confusing.
diff --git a/drivers/staging/fbtft/fb_agm1264k-fl.c
b/drivers/staging/fbtft/fb_agm1264k-fl.c
index 9cc7d25..9114239 100644
--- a/drivers/staging/fbtft/fb_agm1264k-fl.c
+++ b/drivers/staging/fbtft/fb_agm1264k-fl.c
@@ -273,7 +273,7 @@ construct_line_bitmap(struct fbtft_par *par, u8 *dest,
signed short *src,
static int write_vmem(struct fbtft_par *par, size_t offset, size_t len)
{
- u16 *vmem16 = (u16 *)par-info-screen_base;
+ u16 __iomem *vmem16 = (u16 __iomem *)par-info-screen_base;
I haven't looked. What is the type for -screen_base and why can't it
be declared as __iomem type?
http://lxr.free-electrons.com/source/include/linux/fb.h#L486
screen_base is component of struct fb_info, defined as char __iomem *.
In drivers/staging/fbtft/fbtft-core.c, it looks to be actually set to
a pointer resulting from vzalloc().
At https://www.kernel.org/doc/htmldocs/kernel-api/API-vzalloc.html ,
there's no mention of the result being of __iomem nature. So at this
point I'm lost: this looks like inconsistence in driver by design,
but I don't know enough about this driver. Maybe fbtft driver should
use some another variable in some driver-private structre, and not
screen_base from struct fb_info? Or maybe it should not implicitly
assume that memory allocated by vzalloc() behaves the same way that
properly __iomem-allocated memory? Sorry if my phrases are way too
wrong and sound stupid - please don't let me to die being a fool, give
a comment :)
u8 *buf = par-txbuf.buf;
int x, y;
int ret = 0;
@@ -287,7 +287,7 @@ static int write_vmem(struct fbtft_par *par, size_t
offset, size_t len)
/* converting to grayscale16 */
for (x = 0; x par-info-var.xres; ++x)
for (y = 0; y par-info-var.yres; ++y) {
- u16 pixel = vmem16[y * par-info-var.xres + x];
+ u16 pixel = ioread16(vmem16 + y * par-info-var.xres
+ x);
You're saying this is a bug in the original code. Are you positive?
The changelog should have explained your thinking here. Same for all
the iomem changes.
vmem16 is set to a pointer from screen_base, which is _iomem, which
implicates the prohibition of dereferencing. Afrer some brief
searching, I've found that __iomem pointers are supposed to be read
and written with special functions like ioread16(). Also I've read the
fact that at some architectures, simple dereferencing works, but on
others it doesn't.
Of course I'm not sure that exactly this is the correct way to make
sparse happy and to improve correctness of the code (I'm avoiding a
word to fix :) ). See above my explanation of condtradiction in this
driver.
--
Andrey Utkin
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [RFC] drivers/staging/fbtft: fix sparse warnings
diff --git a/drivers/staging/fbtft/fb_agm1264k-fl.c
b/drivers/staging/fbtft/fb_agm1264k-fl.c
index 9cc7d25..9114239 100644
--- a/drivers/staging/fbtft/fb_agm1264k-fl.c
+++ b/drivers/staging/fbtft/fb_agm1264k-fl.c
@@ -273,7 +273,7 @@ construct_line_bitmap(struct fbtft_par *par, u8 *dest,
signed short *src,
static int write_vmem(struct fbtft_par *par, size_t offset, size_t len)
{
- u16 *vmem16 = (u16 *)par-info-screen_base;
+ u16 __iomem *vmem16 = (u16 __iomem *)par-info-screen_base;
I haven't looked. What is the type for -screen_base and why can't it
be declared as __iomem type?
http://lxr.free-electrons.com/source/include/linux/fb.h#L486
screen_base is component of struct fb_info, defined as char __iomem *.
In drivers/staging/fbtft/fbtft-core.c, it looks to be actually set to
a pointer resulting from vzalloc().
Hm, you're right. Normally, it's an __iomem * but this time it's not
an __iomem pointer. Adding anotations to mark it as __iomem is wrong
and adding calls to ioread16() is buggy.
There are a couple ways to make these warnings go away. The simplest
is just to silence the warning with __force:
u16 *vmem16 = (u16 __force *)par-info-screen_base;
This is how some fbdev drivers with vmalloc'ed memory does this:
video/fbdev/{metronomefb.c,hecubafb.c}:
unsigned char *buf = (unsigned char __force *)par-info-screen_base;
info-screen_base = (char __force __iomem *)videomemory;
drivers/video/fbdev/ssd1307fb.c (this one is quite new: 3.15):
u8 __iomem *dst; dst = (void __force *) (info-screen_base + p);
info-screen_base = (u8 __force __iomem *)vmem;
We have to use screen_base because of vmalloc'ed memory and deferred io
(fb_deferred_io_page).
I'm not terribly familiar with this code. I don't know that this is the
cleanest approach. We could also just leave the code alone for now and
ignore the warning.
Yes, it's best to leave this alone for now.
I'm working on a proposal to provide better layering and minimal coupling
to fbdev. This will hopefully lead to screen_base eventually being used
only twice in the fbtft module and nowhere else.
Regards,
Noralf Trønnes
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [RFC] drivers/staging/fbtft: fix sparse warnings
On Mon, Feb 23, 2015 at 07:06:44PM +0200, Andrey Utkin wrote:
2015-02-21 20:58 GMT+02:00 Dan Carpenter dan.carpen...@oracle.com:
Send two separate patches. You can't fix sparse warnings. You can
only fix bugs. The rest is add annotation, doing cleanups or possibly
silencing warnings.
My first email wasn't a patch supposed for accepting, but rather a
request for comments, so I didn't bother with commit granularity,
separation of commit description and the description of my situation
with scissors marker etc. Sorry if this is rude or confusing.
At least *try* to send proper patches. It is a waste of time to send
half finished patches with lazy butt changelogs, yes.
diff --git a/drivers/staging/fbtft/fb_agm1264k-fl.c
b/drivers/staging/fbtft/fb_agm1264k-fl.c
index 9cc7d25..9114239 100644
--- a/drivers/staging/fbtft/fb_agm1264k-fl.c
+++ b/drivers/staging/fbtft/fb_agm1264k-fl.c
@@ -273,7 +273,7 @@ construct_line_bitmap(struct fbtft_par *par, u8 *dest,
signed short *src,
static int write_vmem(struct fbtft_par *par, size_t offset, size_t len)
{
- u16 *vmem16 = (u16 *)par-info-screen_base;
+ u16 __iomem *vmem16 = (u16 __iomem *)par-info-screen_base;
I haven't looked. What is the type for -screen_base and why can't it
be declared as __iomem type?
http://lxr.free-electrons.com/source/include/linux/fb.h#L486
screen_base is component of struct fb_info, defined as char __iomem *.
In drivers/staging/fbtft/fbtft-core.c, it looks to be actually set to
a pointer resulting from vzalloc().
Hm, you're right. Normally, it's an __iomem * but this time it's not
an __iomem pointer. Adding anotations to mark it as __iomem is wrong
and adding calls to ioread16() is buggy.
There are a couple ways to make these warnings go away. The simplest
is just to silence the warning with __force:
u16 *vmem16 = (u16 __force *)par-info-screen_base;
I'm not terribly familiar with this code. I don't know that this is the
cleanest approach. We could also just leave the code alone for now and
ignore the warning.
regards,
dan carpenter
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [RFC] drivers/staging/fbtft: fix sparse warnings
2015-02-23 21:27 GMT+02:00 Noralf Trønnes nor...@tronnes.org: Yes, it's best to leave this alone for now. I'm working on a proposal to provide better layering and minimal coupling to fbdev. This will hopefully lead to screen_base eventually being used only twice in the fbtft module and nowhere else. Ok, staying away and looking for sparse warnings for other staging drivers. Thanks to all for comments. -- Andrey Utkin -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [RFC] drivers/staging/fbtft: fix sparse warnings
On Fri, Feb 20, 2015 at 11:34:09PM +0200, Andrey Utkin wrote:
> See below how sparse output changed with these changes.
> In few words:
> - fixed printf specifiers for size_t;
> - trying to fix address space specifiers issues, not sure what's correct
> approach, ASKING FOR COMMENTS AND HELP;
Send two separate patches. You can't "fix" sparse warnings. You can
only "fix" bugs. The rest is add annotation, doing cleanups or possibly
silencing warnings.
> - didn't touch "was not declared. Should it be static?" yet.
>
> -drivers/staging/fbtft/fbtft-core.c: In function ‘fbtft_register_framebuffer’:
[ millions of lines of warnings snipped. ]
> drivers/staging/fbtft/fbtft_device.c:32:19: warning: symbol 'spi_device' was
> not declared. Should it be static?
> drivers/staging/fbtft/fbtft_device.c:33:24: warning: symbol 'p_device' was
> not declared. Should it be static?
This changelog is a bit rubbish because it's just copy and pasted
warnings for things that didn't change.
>
> This is for Eudyptulla challenge. If you want me to help with any other
> staging driver, I am open.
Don't put this in the changelog.
> diff --git a/drivers/staging/fbtft/fb_agm1264k-fl.c
> b/drivers/staging/fbtft/fb_agm1264k-fl.c
> index 9cc7d25..9114239 100644
> --- a/drivers/staging/fbtft/fb_agm1264k-fl.c
> +++ b/drivers/staging/fbtft/fb_agm1264k-fl.c
> @@ -273,7 +273,7 @@ construct_line_bitmap(struct fbtft_par *par, u8 *dest,
> signed short *src,
>
> static int write_vmem(struct fbtft_par *par, size_t offset, size_t len)
> {
> - u16 *vmem16 = (u16 *)par->info->screen_base;
> + u16 __iomem *vmem16 = (u16 __iomem *)par->info->screen_base;
I haven't looked. What is the type for ->screen_base and why can't it
be declared as __iomem type?
> u8 *buf = par->txbuf.buf;
> int x, y;
> int ret = 0;
> @@ -287,7 +287,7 @@ static int write_vmem(struct fbtft_par *par, size_t
> offset, size_t len)
> /* converting to grayscale16 */
> for (x = 0; x < par->info->var.xres; ++x)
> for (y = 0; y < par->info->var.yres; ++y) {
> - u16 pixel = vmem16[y * par->info->var.xres + x];
> + u16 pixel = ioread16(vmem16 + y * par->info->var.xres
> + x);
You're saying this is a bug in the original code. Are you positive?
The changelog should have explained your thinking here. Same for all
the iomem changes.
regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [RFC] drivers/staging/fbtft: fix sparse warnings
On Fri, Feb 20, 2015 at 11:34:09PM +0200, Andrey Utkin wrote:
See below how sparse output changed with these changes.
In few words:
- fixed printf specifiers for size_t;
- trying to fix address space specifiers issues, not sure what's correct
approach, ASKING FOR COMMENTS AND HELP;
Send two separate patches. You can't fix sparse warnings. You can
only fix bugs. The rest is add annotation, doing cleanups or possibly
silencing warnings.
- didn't touch was not declared. Should it be static? yet.
-drivers/staging/fbtft/fbtft-core.c: In function ‘fbtft_register_framebuffer’:
[ millions of lines of warnings snipped. ]
drivers/staging/fbtft/fbtft_device.c:32:19: warning: symbol 'spi_device' was
not declared. Should it be static?
drivers/staging/fbtft/fbtft_device.c:33:24: warning: symbol 'p_device' was
not declared. Should it be static?
This changelog is a bit rubbish because it's just copy and pasted
warnings for things that didn't change.
This is for Eudyptulla challenge. If you want me to help with any other
staging driver, I am open.
Don't put this in the changelog.
diff --git a/drivers/staging/fbtft/fb_agm1264k-fl.c
b/drivers/staging/fbtft/fb_agm1264k-fl.c
index 9cc7d25..9114239 100644
--- a/drivers/staging/fbtft/fb_agm1264k-fl.c
+++ b/drivers/staging/fbtft/fb_agm1264k-fl.c
@@ -273,7 +273,7 @@ construct_line_bitmap(struct fbtft_par *par, u8 *dest,
signed short *src,
static int write_vmem(struct fbtft_par *par, size_t offset, size_t len)
{
- u16 *vmem16 = (u16 *)par-info-screen_base;
+ u16 __iomem *vmem16 = (u16 __iomem *)par-info-screen_base;
I haven't looked. What is the type for -screen_base and why can't it
be declared as __iomem type?
u8 *buf = par-txbuf.buf;
int x, y;
int ret = 0;
@@ -287,7 +287,7 @@ static int write_vmem(struct fbtft_par *par, size_t
offset, size_t len)
/* converting to grayscale16 */
for (x = 0; x par-info-var.xres; ++x)
for (y = 0; y par-info-var.yres; ++y) {
- u16 pixel = vmem16[y * par-info-var.xres + x];
+ u16 pixel = ioread16(vmem16 + y * par-info-var.xres
+ x);
You're saying this is a bug in the original code. Are you positive?
The changelog should have explained your thinking here. Same for all
the iomem changes.
regards,
dan carpenter
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
