Re: [PATCH] fbdev: colormap fixes

Re: [PATCH] fbdev: colormap fixes

[Geert Uytterhoeven]

On Wed, 27 Jul 2005, Linux Kernel Mailing List wrote:
> tree 17014af0ea8b19dae7848736d324499715b7a1a3
> parent 3ca34fcbfbf8a7cbe99d54ae81c4e28fdc6f4ac6
> author Jon Smirl <jonsmirl@gmail.com> Thu, 28 Jul 2005 01:46:05 -0700
> committer Linus Torvalds <torvalds@g5.osdl.org> Thu, 28 Jul 2005 06:26:18 -0700
> 
> [PATCH] fbdev: colormap fixes
> 
> Color maps have up to 256 entries.  4096/256 allows for 16 characters per
             ^^^^^^^^^^^^^^^^^^^^^^
Most (all?) current drivers have this limitation. But there exists hardware
with more entries.

> line.  The format for a cmap entry is "%02x%c%4x%4x%4x\n" %02x entry %c
> transp %4x red %4x blue %4x green
> 
> You can read the color_map with cat fb0/color_map.
> 
> Signed-off-by: Jon Smirl <jonsmirl@gmail.com>
> Signed-off-by: Andrew Morton <akpm@osdl.org>
> Signed-off-by: Linus Torvalds <torvalds@osdl.org>
> 
>  drivers/video/fbsysfs.c |   82 ++++++++++++++++++++++++++++++++++++++++++------
>  1 files changed, 72 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/video/fbsysfs.c b/drivers/video/fbsysfs.c
> --- a/drivers/video/fbsysfs.c
> +++ b/drivers/video/fbsysfs.c
> @@ -242,10 +242,68 @@ static ssize_t show_virtual(struct class
>  			fb_info->var.yres_virtual);
>  }
>  
> -static ssize_t store_cmap(struct class_device *class_device, const char * buf,
> +/* Format for cmap is "%02x%c%4x%4x%4x\n" */
> +/* %02x entry %c transp %4x red %4x blue %4x green \n */
> +/* 255 rows at 16 chars equals 4096 */
      ^^^
256?

> +/* PAGE_SIZE can be 4096 or larger */
> +static ssize_t store_cmap(struct class_device *class_device, const char *buf,
>  			  size_t count)
>  {
> -//	struct fb_info *fb_info = (struct fb_info *)class_get_devdata(class_device);
> +	struct fb_info *fb_info = (struct fb_info *)class_get_devdata(class_device);
> +	int rc, i, start, length, transp = 0;
> +
> +	if ((count > 4096) || ((count % 16) != 0) || (PAGE_SIZE < 4096))
> +		return -EINVAL;
> +
> +	if (!fb_info->fbops->fb_setcolreg && !fb_info->fbops->fb_setcmap)
> +		return -EINVAL;
> +
> +	sscanf(buf, "%02x", &start);
> +	length = count / 16;
> +
> +	for (i = 0; i < length; i++)
> +		if (buf[i * 16 + 2] != ' ')
> +			transp = 1;
> +
> +	/* If we can batch, do it */
> +	if (fb_info->fbops->fb_setcmap && length > 1) {
> +		struct fb_cmap umap;
> +
> +		memset(&umap, 0, sizeof(umap));
> +		if ((rc = fb_alloc_cmap(&umap, length, transp)))
> +			return rc;
> +
> +		umap.start = start;
> +		for (i = 0; i < length; i++) {
> +			sscanf(&buf[i * 16 +  3], "%4hx", &umap.red[i]);
> +			sscanf(&buf[i * 16 +  7], "%4hx", &umap.blue[i]);
> +			sscanf(&buf[i * 16 + 11], "%4hx", &umap.green[i]);
> +			if (transp)
> +				umap.transp[i] = (buf[i * 16 +  2] != ' ');
> +		}
> +		rc = fb_info->fbops->fb_setcmap(&umap, fb_info);
> +		fb_copy_cmap(&umap, &fb_info->cmap);
> +		fb_dealloc_cmap(&umap);
> +
> +		return rc;
> +	}
> +	for (i = 0; i < length; i++) {
> +		u16 red, blue, green, tsp;
> +
> +		sscanf(&buf[i * 16 +  3], "%4hx", &red);
> +		sscanf(&buf[i * 16 +  7], "%4hx", &blue);
> +		sscanf(&buf[i * 16 + 11], "%4hx", &green);
> +		tsp = (buf[i * 16 +  2] != ' ');
> +		if ((rc = fb_info->fbops->fb_setcolreg(start++,
> +				      red, green, blue, tsp, fb_info)))
> +			return rc;
> +
> +		fb_info->cmap.red[i] = red;
> +		fb_info->cmap.blue[i] = blue;
> +		fb_info->cmap.green[i] = green;
> +		if (transp)
> +			fb_info->cmap.transp[i] = tsp;
> +	}
>  	return 0;
>  }
>  
> @@ -253,20 +311,24 @@ static ssize_t show_cmap(struct class_de
>  {
>  	struct fb_info *fb_info =
>  		(struct fb_info *)class_get_devdata(class_device);
> -	unsigned int offset = 0, i;
> +	unsigned int i;
>  
>  	if (!fb_info->cmap.red || !fb_info->cmap.blue ||
> -	    !fb_info->cmap.green || !fb_info->cmap.transp)
> +	   !fb_info->cmap.green)
> +		return -EINVAL;
> +
> +	if (PAGE_SIZE < 4096)
>  		return -EINVAL;
>  
> +	/* don't mess with the format, the buffer is PAGE_SIZE */
> +	/* 255 entries at 16 chars per line equals 4096 = PAGE_SIZE */
           ^^^
256?

>  	for (i = 0; i < fb_info->cmap.len; i++) {
> -		offset += snprintf(buf, PAGE_SIZE - offset,
> -				   "%d,%d,%d,%d,%d\n", i + fb_info->cmap.start,
> -				   fb_info->cmap.red[i], fb_info->cmap.blue[i],
> -				   fb_info->cmap.green[i],
> -				   fb_info->cmap.transp[i]);
> +		sprintf(&buf[ i * 16], "%02x%c%4x%4x%4x\n", i + fb_info->cmap.start,

Woops, nice buffer overflow if fb_info->cmap.len > 256...

> +			((fb_info->cmap.transp && fb_info->cmap.transp[i]) ? '*' : ' '),
> +			fb_info->cmap.red[i], fb_info->cmap.blue[i],
> +			fb_info->cmap.green[i]);
>  	}
> -	return offset;
> +	return 4096;
               ^^^^

What if fb_info->cmap.len is smaller than 256?

>  }

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds


-------------------------------------------------------
SF.Net email is Sponsored by the Better Software Conference & EXPO September
19-22, 2005 * San Francisco, CA * Development Lifecycle Practices
Agile & Plan-Driven Development * Managing Projects & Teams * Testing & QA
Security * Process Improvement & Measurement * http://www.sqe.com/bsce5sf

Re: [PATCH] fbdev: colormap fixes

[Jon Smirl]

On 7/28/05, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Wed, 27 Jul 2005, Linux Kernel Mailing List wrote:

There are a couple of ways to fix this. 

1) Add a check to limit use of the sysfs attributes to 256 entries. If
you want more you have to use /dev/fb0 and the ioctl. More is an
uncommon case.
2) Switch this to a binary parameter. Now you have to use tools like
hexdump instead of cat to work with the data. It was nice to be able
to use cat to see the current map.

Does anyone have preferences for which way to fix it?

Thanks for catching the problems. I'm posting these patches to fbdev
for review first so it is best to catch bugs there.

> > tree 17014af0ea8b19dae7848736d324499715b7a1a3
> > parent 3ca34fcbfbf8a7cbe99d54ae81c4e28fdc6f4ac6
> > author Jon Smirl <jonsmirl@gmail.com> Thu, 28 Jul 2005 01:46:05 -0700
> > committer Linus Torvalds <torvalds@g5.osdl.org> Thu, 28 Jul 2005 06:26:18 -0700
> >
> > [PATCH] fbdev: colormap fixes
> >
> > Color maps have up to 256 entries.  4096/256 allows for 16 characters per
>              ^^^^^^^^^^^^^^^^^^^^^^
> Most (all?) current drivers have this limitation. But there exists hardware
> with more entries.
> 
> > line.  The format for a cmap entry is "%02x%c%4x%4x%4x\n" %02x entry %c
> > transp %4x red %4x blue %4x green
> >
> > You can read the color_map with cat fb0/color_map.
> >
> > Signed-off-by: Jon Smirl <jonsmirl@gmail.com>
> > Signed-off-by: Andrew Morton <akpm@osdl.org>
> > Signed-off-by: Linus Torvalds <torvalds@osdl.org>
> >
> >  drivers/video/fbsysfs.c |   82 ++++++++++++++++++++++++++++++++++++++++++------
> >  1 files changed, 72 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/video/fbsysfs.c b/drivers/video/fbsysfs.c
> > --- a/drivers/video/fbsysfs.c
> > +++ b/drivers/video/fbsysfs.c
> > @@ -242,10 +242,68 @@ static ssize_t show_virtual(struct class
> >                       fb_info->var.yres_virtual);
> >  }
> >
> > -static ssize_t store_cmap(struct class_device *class_device, const char * buf,
> > +/* Format for cmap is "%02x%c%4x%4x%4x\n" */
> > +/* %02x entry %c transp %4x red %4x blue %4x green \n */
> > +/* 255 rows at 16 chars equals 4096 */
>       ^^^
> 256?
> 
> > +/* PAGE_SIZE can be 4096 or larger */
> > +static ssize_t store_cmap(struct class_device *class_device, const char *buf,
> >                         size_t count)
> >  {
> > -//   struct fb_info *fb_info = (struct fb_info *)class_get_devdata(class_device);
> > +     struct fb_info *fb_info = (struct fb_info *)class_get_devdata(class_device);
> > +     int rc, i, start, length, transp = 0;
> > +
> > +     if ((count > 4096) || ((count % 16) != 0) || (PAGE_SIZE < 4096))
> > +             return -EINVAL;
> > +
> > +     if (!fb_info->fbops->fb_setcolreg && !fb_info->fbops->fb_setcmap)
> > +             return -EINVAL;
> > +
> > +     sscanf(buf, "%02x", &start);
> > +     length = count / 16;
> > +
> > +     for (i = 0; i < length; i++)
> > +             if (buf[i * 16 + 2] != ' ')
> > +                     transp = 1;
> > +
> > +     /* If we can batch, do it */
> > +     if (fb_info->fbops->fb_setcmap && length > 1) {
> > +             struct fb_cmap umap;
> > +
> > +             memset(&umap, 0, sizeof(umap));
> > +             if ((rc = fb_alloc_cmap(&umap, length, transp)))
> > +                     return rc;
> > +
> > +             umap.start = start;
> > +             for (i = 0; i < length; i++) {
> > +                     sscanf(&buf[i * 16 +  3], "%4hx", &umap.red[i]);
> > +                     sscanf(&buf[i * 16 +  7], "%4hx", &umap.blue[i]);
> > +                     sscanf(&buf[i * 16 + 11], "%4hx", &umap.green[i]);
> > +                     if (transp)
> > +                             umap.transp[i] = (buf[i * 16 +  2] != ' ');
> > +             }
> > +             rc = fb_info->fbops->fb_setcmap(&umap, fb_info);
> > +             fb_copy_cmap(&umap, &fb_info->cmap);
> > +             fb_dealloc_cmap(&umap);
> > +
> > +             return rc;
> > +     }
> > +     for (i = 0; i < length; i++) {
> > +             u16 red, blue, green, tsp;
> > +
> > +             sscanf(&buf[i * 16 +  3], "%4hx", &red);
> > +             sscanf(&buf[i * 16 +  7], "%4hx", &blue);
> > +             sscanf(&buf[i * 16 + 11], "%4hx", &green);
> > +             tsp = (buf[i * 16 +  2] != ' ');
> > +             if ((rc = fb_info->fbops->fb_setcolreg(start++,
> > +                                   red, green, blue, tsp, fb_info)))
> > +                     return rc;
> > +
> > +             fb_info->cmap.red[i] = red;
> > +             fb_info->cmap.blue[i] = blue;
> > +             fb_info->cmap.green[i] = green;
> > +             if (transp)
> > +                     fb_info->cmap.transp[i] = tsp;
> > +     }
> >       return 0;
> >  }
> >
> > @@ -253,20 +311,24 @@ static ssize_t show_cmap(struct class_de
> >  {
> >       struct fb_info *fb_info =
> >               (struct fb_info *)class_get_devdata(class_device);
> > -     unsigned int offset = 0, i;
> > +     unsigned int i;
> >
> >       if (!fb_info->cmap.red || !fb_info->cmap.blue ||
> > -         !fb_info->cmap.green || !fb_info->cmap.transp)
> > +        !fb_info->cmap.green)
> > +             return -EINVAL;
> > +
> > +     if (PAGE_SIZE < 4096)
> >               return -EINVAL;
> >
> > +     /* don't mess with the format, the buffer is PAGE_SIZE */
> > +     /* 255 entries at 16 chars per line equals 4096 = PAGE_SIZE */
>            ^^^
> 256?
> 
> >       for (i = 0; i < fb_info->cmap.len; i++) {
> > -             offset += snprintf(buf, PAGE_SIZE - offset,
> > -                                "%d,%d,%d,%d,%d\n", i + fb_info->cmap.start,
> > -                                fb_info->cmap.red[i], fb_info->cmap.blue[i],
> > -                                fb_info->cmap.green[i],
> > -                                fb_info->cmap.transp[i]);
> > +             sprintf(&buf[ i * 16], "%02x%c%4x%4x%4x\n", i + fb_info->cmap.start,
> 
> Woops, nice buffer overflow if fb_info->cmap.len > 256...
> 
> > +                     ((fb_info->cmap.transp && fb_info->cmap.transp[i]) ? '*' : ' '),
> > +                     fb_info->cmap.red[i], fb_info->cmap.blue[i],
> > +                     fb_info->cmap.green[i]);
> >       }
> > -     return offset;
> > +     return 4096;
>                ^^^^
> 
> What if fb_info->cmap.len is smaller than 256?
> 
> >  }
> 
> Gr{oetje,eeting}s,
> 
>                                                 Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                                             -- Linus Torvalds
> 


-- 
Jon Smirl
jonsmirl@gmail.com


-------------------------------------------------------
SF.Net email is Sponsored by the Better Software Conference & EXPO September
19-22, 2005 * San Francisco, CA * Development Lifecycle Practices
Agile & Plan-Driven Development * Managing Projects & Teams * Testing & QA
Security * Process Improvement & Measurement * http://www.sqe.com/bsce5sf

Re: [PATCH] fbdev: colormap fixes

[Geert Uytterhoeven]

On Thu, 28 Jul 2005, Jon Smirl wrote:
> On 7/28/05, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Wed, 27 Jul 2005, Linux Kernel Mailing List wrote:
> 
> There are a couple of ways to fix this. 
> 
> 1) Add a check to limit use of the sysfs attributes to 256 entries. If
> you want more you have to use /dev/fb0 and the ioctl. More is an
> uncommon case.
> 2) Switch this to a binary parameter. Now you have to use tools like
> hexdump instead of cat to work with the data. It was nice to be able
> to use cat to see the current map.
> 
> Does anyone have preferences for which way to fix it?

I prefer the first way.

> Thanks for catching the problems. I'm posting these patches to fbdev
> for review first so it is best to catch bugs there.

Sorry, sometimes I miss a few things...

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds


-------------------------------------------------------
SF.Net email is Sponsored by the Better Software Conference & EXPO September
19-22, 2005 * San Francisco, CA * Development Lifecycle Practices
Agile & Plan-Driven Development * Managing Projects & Teams * Testing & QA
Security * Process Improvement & Measurement * http://www.sqe.com/bsce5sf

Re: [PATCH] fbdev: colormap fixes

[Jon Smirl]

Can you review this fix for the issues below? I fixed things to
automatically adjust the number of entries to whatever fits in
PAGE_SIZE.

diff --git a/drivers/video/fbsysfs.c b/drivers/video/fbsysfs.c
--- a/drivers/video/fbsysfs.c
+++ b/drivers/video/fbsysfs.c
@@ -244,15 +244,15 @@ static ssize_t show_virtual(struct class
 
 /* Format for cmap is "%02x%c%4x%4x%4x\n" */
 /* %02x entry %c transp %4x red %4x blue %4x green \n */
-/* 255 rows at 16 chars equals 4096 */
-/* PAGE_SIZE can be 4096 or larger */
+/* 256 rows at 16 chars equals 4096, the normal page size */
+/* the code will automatically adjust for different page sizes */
 static ssize_t store_cmap(struct class_device *class_device, const char *buf,
 			  size_t count)
 {
 	struct fb_info *fb_info = (struct fb_info *)class_get_devdata(class_device);
 	int rc, i, start, length, transp = 0;
 
-	if ((count > 4096) || ((count % 16) != 0) || (PAGE_SIZE < 4096))
+	if ((count > PAGE_SIZE) || ((count % 16) != 0))
 		return -EINVAL;
 
 	if (!fb_info->fbops->fb_setcolreg && !fb_info->fbops->fb_setcmap)
@@ -317,18 +317,18 @@ static ssize_t show_cmap(struct class_de
 	   !fb_info->cmap.green)
 		return -EINVAL;
 
-	if (PAGE_SIZE < 4096)
+	if (fb_info->cmap.len > PAGE_SIZE / 16)
 		return -EINVAL;
 
 	/* don't mess with the format, the buffer is PAGE_SIZE */
-	/* 255 entries at 16 chars per line equals 4096 = PAGE_SIZE */
+	/* 256 entries at 16 chars per line equals 4096 = PAGE_SIZE */
 	for (i = 0; i < fb_info->cmap.len; i++) {
-		sprintf(&buf[ i * 16], "%02x%c%4x%4x%4x\n", i + fb_info->cmap.start,
+		snprintf(&buf[ i * 16], PAGE_SIZE - i * 16, "%02x%c%4x%4x%4x\n", i
+ fb_info->cmap.start,
 			((fb_info->cmap.transp && fb_info->cmap.transp[i]) ? '*' : ' '),
 			fb_info->cmap.red[i], fb_info->cmap.blue[i],
 			fb_info->cmap.green[i]);
 	}
-	return 4096;
+	return 16 * fb_info->cmap.len;
 }
 
 static ssize_t store_blank(struct class_device *class_device, const char * buf,



On 7/28/05, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Wed, 27 Jul 2005, Linux Kernel Mailing List wrote:
> > tree 17014af0ea8b19dae7848736d324499715b7a1a3
> > parent 3ca34fcbfbf8a7cbe99d54ae81c4e28fdc6f4ac6
> > author Jon Smirl <jonsmirl@gmail.com> Thu, 28 Jul 2005 01:46:05 -0700
> > committer Linus Torvalds <torvalds@g5.osdl.org> Thu, 28 Jul 2005 06:26:18 -0700
> >
> > [PATCH] fbdev: colormap fixes
> >
> > Color maps have up to 256 entries.  4096/256 allows for 16 characters per
>              ^^^^^^^^^^^^^^^^^^^^^^
> Most (all?) current drivers have this limitation. But there exists hardware
> with more entries.
> 
> > line.  The format for a cmap entry is "%02x%c%4x%4x%4x\n" %02x entry %c
> > transp %4x red %4x blue %4x green
> >
> > You can read the color_map with cat fb0/color_map.
> >
> > Signed-off-by: Jon Smirl <jonsmirl@gmail.com>
> > Signed-off-by: Andrew Morton <akpm@osdl.org>
> > Signed-off-by: Linus Torvalds <torvalds@osdl.org>
> >
> >  drivers/video/fbsysfs.c |   82 ++++++++++++++++++++++++++++++++++++++++++------
> >  1 files changed, 72 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/video/fbsysfs.c b/drivers/video/fbsysfs.c
> > --- a/drivers/video/fbsysfs.c
> > +++ b/drivers/video/fbsysfs.c
> > @@ -242,10 +242,68 @@ static ssize_t show_virtual(struct class
> >                       fb_info->var.yres_virtual);
> >  }
> >
> > -static ssize_t store_cmap(struct class_device *class_device, const char * buf,
> > +/* Format for cmap is "%02x%c%4x%4x%4x\n" */
> > +/* %02x entry %c transp %4x red %4x blue %4x green \n */
> > +/* 255 rows at 16 chars equals 4096 */
>       ^^^
> 256?
> 
> > +/* PAGE_SIZE can be 4096 or larger */
> > +static ssize_t store_cmap(struct class_device *class_device, const char *buf,
> >                         size_t count)
> >  {
> > -//   struct fb_info *fb_info = (struct fb_info *)class_get_devdata(class_device);
> > +     struct fb_info *fb_info = (struct fb_info *)class_get_devdata(class_device);
> > +     int rc, i, start, length, transp = 0;
> > +
> > +     if ((count > 4096) || ((count % 16) != 0) || (PAGE_SIZE < 4096))
> > +             return -EINVAL;
> > +
> > +     if (!fb_info->fbops->fb_setcolreg && !fb_info->fbops->fb_setcmap)
> > +             return -EINVAL;
> > +
> > +     sscanf(buf, "%02x", &start);
> > +     length = count / 16;
> > +
> > +     for (i = 0; i < length; i++)
> > +             if (buf[i * 16 + 2] != ' ')
> > +                     transp = 1;
> > +
> > +     /* If we can batch, do it */
> > +     if (fb_info->fbops->fb_setcmap && length > 1) {
> > +             struct fb_cmap umap;
> > +
> > +             memset(&umap, 0, sizeof(umap));
> > +             if ((rc = fb_alloc_cmap(&umap, length, transp)))
> > +                     return rc;
> > +
> > +             umap.start = start;
> > +             for (i = 0; i < length; i++) {
> > +                     sscanf(&buf[i * 16 +  3], "%4hx", &umap.red[i]);
> > +                     sscanf(&buf[i * 16 +  7], "%4hx", &umap.blue[i]);
> > +                     sscanf(&buf[i * 16 + 11], "%4hx", &umap.green[i]);
> > +                     if (transp)
> > +                             umap.transp[i] = (buf[i * 16 +  2] != ' ');
> > +             }
> > +             rc = fb_info->fbops->fb_setcmap(&umap, fb_info);
> > +             fb_copy_cmap(&umap, &fb_info->cmap);
> > +             fb_dealloc_cmap(&umap);
> > +
> > +             return rc;
> > +     }
> > +     for (i = 0; i < length; i++) {
> > +             u16 red, blue, green, tsp;
> > +
> > +             sscanf(&buf[i * 16 +  3], "%4hx", &red);
> > +             sscanf(&buf[i * 16 +  7], "%4hx", &blue);
> > +             sscanf(&buf[i * 16 + 11], "%4hx", &green);
> > +             tsp = (buf[i * 16 +  2] != ' ');
> > +             if ((rc = fb_info->fbops->fb_setcolreg(start++,
> > +                                   red, green, blue, tsp, fb_info)))
> > +                     return rc;
> > +
> > +             fb_info->cmap.red[i] = red;
> > +             fb_info->cmap.blue[i] = blue;
> > +             fb_info->cmap.green[i] = green;
> > +             if (transp)
> > +                     fb_info->cmap.transp[i] = tsp;
> > +     }
> >       return 0;
> >  }
> >
> > @@ -253,20 +311,24 @@ static ssize_t show_cmap(struct class_de
> >  {
> >       struct fb_info *fb_info =
> >               (struct fb_info *)class_get_devdata(class_device);
> > -     unsigned int offset = 0, i;
> > +     unsigned int i;
> >
> >       if (!fb_info->cmap.red || !fb_info->cmap.blue ||
> > -         !fb_info->cmap.green || !fb_info->cmap.transp)
> > +        !fb_info->cmap.green)
> > +             return -EINVAL;
> > +
> > +     if (PAGE_SIZE < 4096)
> >               return -EINVAL;
> >
> > +     /* don't mess with the format, the buffer is PAGE_SIZE */
> > +     /* 255 entries at 16 chars per line equals 4096 = PAGE_SIZE */
>            ^^^
> 256?
> 
> >       for (i = 0; i < fb_info->cmap.len; i++) {
> > -             offset += snprintf(buf, PAGE_SIZE - offset,
> > -                                "%d,%d,%d,%d,%d\n", i + fb_info->cmap.start,
> > -                                fb_info->cmap.red[i], fb_info->cmap.blue[i],
> > -                                fb_info->cmap.green[i],
> > -                                fb_info->cmap.transp[i]);
> > +             sprintf(&buf[ i * 16], "%02x%c%4x%4x%4x\n", i + fb_info->cmap.start,
> 
> Woops, nice buffer overflow if fb_info->cmap.len > 256...
> 
> > +                     ((fb_info->cmap.transp && fb_info->cmap.transp[i]) ? '*' : ' '),
> > +                     fb_info->cmap.red[i], fb_info->cmap.blue[i],
> > +                     fb_info->cmap.green[i]);
> >       }
> > -     return offset;
> > +     return 4096;
>                ^^^^
> 
> What if fb_info->cmap.len is smaller than 256?
> 
> >  }
> 
> Gr{oetje,eeting}s,
> 
>                                                 Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                                             -- Linus Torvalds
> 


-- 
Jon Smirl
jonsmirl@gmail.com


-------------------------------------------------------
SF.Net email is Sponsored by the Better Software Conference & EXPO September
19-22, 2005 * San Francisco, CA * Development Lifecycle Practices
Agile & Plan-Driven Development * Managing Projects & Teams * Testing & QA
Security * Process Improvement & Measurement * http://www.sqe.com/bsce5sf

Re: Re: [PATCH] fbdev: colormap fixes

[Antonino A. Daplas]

Jon Smirl wrote:
> On 7/28/05, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>> On Wed, 27 Jul 2005, Linux Kernel Mailing List wrote:
> 
> There are a couple of ways to fix this. 
> 
> 1) Add a check to limit use of the sysfs attributes to 256 entries. If
> you want more you have to use /dev/fb0 and the ioctl. More is an
> uncommon case.
> 2) Switch this to a binary parameter. Now you have to use tools like
> hexdump instead of cat to work with the data. It was nice to be able
> to use cat to see the current map.
> 
> Does anyone have preferences for which way to fix it?

Or...

  3) Add another file in sysfs which specifies at what index and how many
entries will be read or written from or to the cmap. With this additional
sysfs file, it should be able to handle any reasonable cmap length, but
it will take more than one reading of the color_map file. Another
advantage is that the entire color map need not be read or written if
only one field needs to be changed.

I've attached a test patch.  Let me know what you think.

Tony

    The functions store_cmap and show_cmap assume that cmap entries will never
    exceed 256.  However, even on the i386 graphics cards exist with 10-bit
    DAC's. The current functions are insufficient on these hardware.

    In order to provide support > 256 entries, a new sysfs entry is introduced -
    cmap_range.  It consists of two fields separated by a comma. The first field
    sets the starting index, and the second field, the number of entries. Thus,
    to grab a 256 color map in a machine with pagesize = 4096, one might do
    the ff:

        echo 0,128 > cmap_entry
        cat color_map
        echo 128,128 > cmap_entry
        cat color_map

    It also makes the function more efficient since one can alter a single entry
    without reading/writing the entire cmap.

    The output of color_map is also changed to accomodate the transparency field,
    and also to increase readability.

    iiii. tttt rrrr gggg bbbb
    iiii. tttt rrrr gggg bbbb

    where i = index;
          t = transparency;
          r = red;
          g = green;
          b = blue;

    From: Antonino Daplas <adaplas@pol.net>
    Signed-off-by: Antonino Daplas <adaplas@pol.net>
---
 drivers/video/fbsysfs.c |  172 ++++++++++++++++++++++++++++++++----------------
 include/linux/fb.h      |    2
 2 files changed, 117 insertions(+), 57 deletions(-)

--- a/drivers/video/fbsysfs.c
+++ b/drivers/video/fbsysfs.c
@@ -242,95 +242,152 @@ static ssize_t show_virtual(struct class
 			fb_info->var.yres_virtual);
 }
 
-/* Format for cmap is "%02x%c%4x%4x%4x\n" */
-/* %02x entry %c transp %4x red %4x blue %4x green \n */
-/* 255 rows at 16 chars equals 4096 */
-/* PAGE_SIZE can be 4096 or larger */
+/* Format for cmap is "%4x. %4x %4x %4x %4x\n" */
+/* %4x. index %4x transp %4x red %4x green %4x blue \n */
 static ssize_t store_cmap(struct class_device *class_device, const char *buf,
 			  size_t count)
 {
-	struct fb_info *fb_info = (struct fb_info *)class_get_devdata(class_device);
-	int rc, i, start, length, transp = 0;
-
-	if ((count > 4096) || ((count % 16) != 0) || (PAGE_SIZE < 4096))
-		return -EINVAL;
+	struct fb_info *fb_info = class_get_devdata(class_device);
+	int rc = 0, i, length, start = fb_info->cmap_start;
 
 	if (!fb_info->fbops->fb_setcolreg && !fb_info->fbops->fb_setcmap)
 		return -EINVAL;
 
-	sscanf(buf, "%02x", &start);
-	length = count / 16;
+	length = count/26;
 
-	for (i = 0; i < length; i++)
-		if (buf[i * 16 + 2] != ' ')
-			transp = 1;
+	if (start + length > fb_info->cmap.len || length > fb_info->cmap_len)
+		return -EINVAL;
 
+	acquire_console_sem();
+	
 	/* If we can batch, do it */
 	if (fb_info->fbops->fb_setcmap && length > 1) {
 		struct fb_cmap umap;
 
 		memset(&umap, 0, sizeof(umap));
-		if ((rc = fb_alloc_cmap(&umap, length, transp)))
-			return rc;
 
-		umap.start = start;
-		for (i = 0; i < length; i++) {
-			sscanf(&buf[i * 16 +  3], "%4hx", &umap.red[i]);
-			sscanf(&buf[i * 16 +  7], "%4hx", &umap.blue[i]);
-			sscanf(&buf[i * 16 + 11], "%4hx", &umap.green[i]);
-			if (transp)
-				umap.transp[i] = (buf[i * 16 +  2] != ' ');
+		rc = fb_alloc_cmap(&umap, length, 1);
+		
+		if (!rc) {
+			umap.start = start;
+
+			for (i = 0; i < length; i++) {
+				sscanf(&buf[i*26+06], "%4hx", &umap.transp[i]);
+				sscanf(&buf[i*26+11], "%4hx", &umap.red[i]);
+				sscanf(&buf[i*26+16], "%4hx", &umap.green[i]);
+				sscanf(&buf[i*26+21], "%4hx", &umap.blue[i]);
+			}
+			
+			rc = fb_info->fbops->fb_setcmap(&umap, fb_info);
+			fb_copy_cmap(&umap, &fb_info->cmap);
+			fb_dealloc_cmap(&umap);
 		}
-		rc = fb_info->fbops->fb_setcmap(&umap, fb_info);
-		fb_copy_cmap(&umap, &fb_info->cmap);
-		fb_dealloc_cmap(&umap);
-
-		return rc;
-	}
-	for (i = 0; i < length; i++) {
+			
+	} else {
 		u16 red, blue, green, tsp;
 
-		sscanf(&buf[i * 16 +  3], "%4hx", &red);
-		sscanf(&buf[i * 16 +  7], "%4hx", &blue);
-		sscanf(&buf[i * 16 + 11], "%4hx", &green);
-		tsp = (buf[i * 16 +  2] != ' ');
-		if ((rc = fb_info->fbops->fb_setcolreg(start++,
-				      red, green, blue, tsp, fb_info)))
-			return rc;
-
-		fb_info->cmap.red[i] = red;
-		fb_info->cmap.blue[i] = blue;
-		fb_info->cmap.green[i] = green;
-		if (transp)
-			fb_info->cmap.transp[i] = tsp;
+		for (i = 0; i < length; i++) {
+			sscanf(&buf[i * 26 +  6], "%4hx", &tsp);
+			sscanf(&buf[i * 26 + 11], "%4hx", &red);
+			sscanf(&buf[i * 26 + 16], "%4hx", &green);
+			sscanf(&buf[i * 26 + 21], "%4hx", &blue);
+			
+			rc = fb_info->fbops->fb_setcolreg(start++, red, green,
+							  blue, tsp, fb_info);
+			if (rc)
+				break;
+
+			fb_info->cmap.red[i] = red;
+			fb_info->cmap.blue[i] = blue;
+			fb_info->cmap.green[i] = green;
+			if (fb_info->cmap.transp)
+				fb_info->cmap.transp[i] = tsp;
+		}
 	}
-	return 0;
+
+	release_console_sem();
+	return rc;
 }
 
 static ssize_t show_cmap(struct class_device *class_device, char *buf)
 {
-	struct fb_info *fb_info =
-		(struct fb_info *)class_get_devdata(class_device);
-	unsigned int i;
+	struct fb_info *fb_info = class_get_devdata(class_device);
+	unsigned int i, start;
+	u16 transp = 1;
 
 	if (!fb_info->cmap.red || !fb_info->cmap.blue ||
 	   !fb_info->cmap.green)
 		return -EINVAL;
 
-	if (PAGE_SIZE < 4096)
-		return -EINVAL;
+	if (!fb_info->cmap.transp)
+		transp = 0;
+	
+	acquire_console_sem();
+	start = fb_info->cmap_start;
 
-	/* don't mess with the format, the buffer is PAGE_SIZE */
-	/* 255 entries at 16 chars per line equals 4096 = PAGE_SIZE */
-	for (i = 0; i < fb_info->cmap.len; i++) {
-		sprintf(&buf[ i * 16], "%02x%c%4x%4x%4x\n", i + fb_info->cmap.start,
-			((fb_info->cmap.transp && fb_info->cmap.transp[i]) ? '*' : ' '),
-			fb_info->cmap.red[i], fb_info->cmap.blue[i],
-			fb_info->cmap.green[i]);
+	if (start + fb_info->cmap_len <= fb_info->cmap.len) {
+		/* each row is 26 bytes */
+		for (i = 0; i < fb_info->cmap_len; i++) {
+			sprintf(&buf[i * 26], "%04x. %04x %04x %04x %04x\n",
+				i + fb_info->cmap_start,
+				(!transp) ? 0 : fb_info->cmap.transp[i+start],
+				fb_info->cmap.red[i+start],
+				fb_info->cmap.green[i+start],
+				fb_info->cmap.blue[i+start]);
+		}
 	}
-	return 4096;
+
+	i = fb_info->cmap_len * 26;
+	release_console_sem();
+
+	return i;
 }
 
+static ssize_t show_cmap_range(struct class_device *class_device, char *buf)
+{
+	struct fb_info *fb_info = class_get_devdata(class_device);
+	
+	return snprintf(buf, PAGE_SIZE, "%d,%d\n", fb_info->cmap_start,
+			fb_info->cmap_len);
+}
+
+static ssize_t store_cmap_range(struct class_device *class_device,
+				const char *buf, size_t count)
+{
+	struct fb_info *fb_info = class_get_devdata(class_device);
+	char *last = NULL;
+	int start, len, err = 1;
+
+	start = simple_strtoul(buf, &last, 0);
+	last++;
+
+	if (last - buf < count) {
+		len = simple_strtoul(last, &last, 0);
+		err = 0;
+	}
+
+	if (!err) {
+		if (len > 0x10000)
+			len = 0x10000;
+
+		if (len > PAGE_SIZE/26)
+			len = PAGE_SIZE/26;
+
+		if (start > fb_info->cmap.len - 1)
+			start = 0;
+
+		if (len > fb_info->cmap.len - start) 
+			len = fb_info->cmap.len - start;
+
+		acquire_console_sem();
+		fb_info->cmap_start = start;
+		fb_info->cmap_len = len;
+		release_console_sem();
+	}
+
+	return count;
+}
+	
 static ssize_t store_blank(struct class_device *class_device, const char * buf,
 			   size_t count)
 {
@@ -424,6 +481,7 @@ static struct class_device_attribute cla
 	__ATTR(modes, S_IRUGO|S_IWUSR, show_modes, store_modes),
 	__ATTR(pan, S_IRUGO|S_IWUSR, show_pan, store_pan),
 	__ATTR(virtual_size, S_IRUGO|S_IWUSR, show_virtual, store_virtual),
+	__ATTR(cmap_range, S_IRUGO|S_IWUSR, show_cmap_range, store_cmap_range),
 };
 
 int fb_init_class_device(struct fb_info *fb_info)
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -720,6 +720,8 @@ struct fb_info {
 	struct fb_pixmap pixmap;	/* Image hardware mapper */
 	struct fb_pixmap sprite;	/* Cursor hardware mapper */
 	struct fb_cmap cmap;		/* Current cmap */
+	int    cmap_start;              /* Start of cmap entries to access in sysfs */
+	int    cmap_len;                /* Num of entries to to access in sysfs */
 	struct list_head modelist;      /* mode list */
 	struct fb_videomode *mode;	/* current mode */
 	struct fb_ops *fbops;



-------------------------------------------------------
SF.Net email is Sponsored by the Better Software Conference & EXPO September
19-22, 2005 * San Francisco, CA * Development Lifecycle Practices
Agile & Plan-Driven Development * Managing Projects & Teams * Testing & QA
Security * Process Improvement & Measurement * http://www.sqe.com/bsce5sf

Re: [PATCH] fbdev: colormap fixes

[Geert Uytterhoeven]

On Thu, 28 Jul 2005, Jon Smirl wrote:
> Can you review this fix for the issues below? I fixed things to
> automatically adjust the number of entries to whatever fits in
> PAGE_SIZE.

Looks OK, but...

> @@ -317,18 +317,18 @@ static ssize_t show_cmap(struct class_de
>  	   !fb_info->cmap.green)
>  		return -EINVAL;
>  
> -	if (PAGE_SIZE < 4096)
> +	if (fb_info->cmap.len > PAGE_SIZE / 16)
>  		return -EINVAL;

... perhaps you want to just return the first PAGE_SIZE/16 entries, instead of
failing?

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds


-------------------------------------------------------
SF.Net email is Sponsored by the Better Software Conference & EXPO September
19-22, 2005 * San Francisco, CA * Development Lifecycle Practices
Agile & Plan-Driven Development * Managing Projects & Teams * Testing & QA
Security * Process Improvement & Measurement * http://www.sqe.com/bsce5sf

Re: Re: [PATCH] fbdev: colormap fixes

[Geert Uytterhoeven]

On Thu, 28 Jul 2005, Antonino A. Daplas wrote:
> Jon Smirl wrote:
> > On 7/28/05, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > On Wed, 27 Jul 2005, Linux Kernel Mailing List wrote:
> > 
> > There are a couple of ways to fix this. 
> > 1) Add a check to limit use of the sysfs attributes to 256 entries. If
> > you want more you have to use /dev/fb0 and the ioctl. More is an
> > uncommon case.
> > 2) Switch this to a binary parameter. Now you have to use tools like
> > hexdump instead of cat to work with the data. It was nice to be able
> > to use cat to see the current map.
> > 
> > Does anyone have preferences for which way to fix it?
> 
> Or...
> 
>  3) Add another file in sysfs which specifies at what index and how many
> entries will be read or written from or to the cmap. With this additional
> sysfs file, it should be able to handle any reasonable cmap length, but
> it will take more than one reading of the color_map file. Another
> advantage is that the entire color map need not be read or written if
> only one field needs to be changed.
> 
> I've attached a test patch.  Let me know what you think.

I like it! ... But, a disadvantages is that it needs to store state between two
non-atomic operations. E.g. imagine two processes doing this at the same time.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds


-------------------------------------------------------
SF.Net email is Sponsored by the Better Software Conference & EXPO September
19-22, 2005 * San Francisco, CA * Development Lifecycle Practices
Agile & Plan-Driven Development * Managing Projects & Teams * Testing & QA
Security * Process Improvement & Measurement * http://www.sqe.com/bsce5sf

Re: Re: [PATCH] fbdev: colormap fixes

[Jon Smirl]

On 7/28/05, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Thu, 28 Jul 2005, Antonino A. Daplas wrote:
> > Jon Smirl wrote:
> > > On 7/28/05, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > On Wed, 27 Jul 2005, Linux Kernel Mailing List wrote:
> > >
> > > There are a couple of ways to fix this.
> > > 1) Add a check to limit use of the sysfs attributes to 256 entries. If
> > > you want more you have to use /dev/fb0 and the ioctl. More is an
> > > uncommon case.
> > > 2) Switch this to a binary parameter. Now you have to use tools like
> > > hexdump instead of cat to work with the data. It was nice to be able
> > > to use cat to see the current map.
> > >
> > > Does anyone have preferences for which way to fix it?
> >
> > Or...
> >
> >  3) Add another file in sysfs which specifies at what index and how many
> > entries will be read or written from or to the cmap. With this additional
> > sysfs file, it should be able to handle any reasonable cmap length, but
> > it will take more than one reading of the color_map file. Another
> > advantage is that the entire color map need not be read or written if
> > only one field needs to be changed.
> >
> > I've attached a test patch.  Let me know what you think.
> 
> I like it! ... But, a disadvantages is that it needs to store state between two
> non-atomic operations. E.g. imagine two processes doing this at the same time.

Two attributes is a big problem with atomicity. If you want access to
the full cmap I would switch to a binary attribute or use the existing
IOCTL.

Note that you can set the entire map with the new patch, you just
can't read it. You can store 1-N entries at any base you want.

> 
> Gr{oetje,eeting}s,
> 
>                                                 Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                                             -- Linus Torvalds
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 


-- 
Jon Smirl
jonsmirl@gmail.com


-------------------------------------------------------
SF.Net email is Sponsored by the Better Software Conference & EXPO September
19-22, 2005 * San Francisco, CA * Development Lifecycle Practices
Agile & Plan-Driven Development * Managing Projects & Teams * Testing & QA
Security * Process Improvement & Measurement * http://www.sqe.com/bsce5sf

Re: Re: [PATCH] fbdev: colormap fixes

[Jon Smirl]

I can't see a way to query how long of cmap the device supports using
the current fbdev ioctls.

I wouldn't even be messing with cmap except for the true/direct color
support and gamma ramps. Don't I need to know how long the cmap is in
order to set the right gamma ramp?

If I set a 256 entry gamma ramp on a card with 1024 entries, will it
still work right? Or do I need to set all 1024 entries?

On 7/28/05, Jon Smirl <jonsmirl@gmail.com> wrote:
> On 7/28/05, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Thu, 28 Jul 2005, Antonino A. Daplas wrote:
> > > Jon Smirl wrote:
> > > > On 7/28/05, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > > On Wed, 27 Jul 2005, Linux Kernel Mailing List wrote:
> > > >
> > > > There are a couple of ways to fix this.
> > > > 1) Add a check to limit use of the sysfs attributes to 256 entries. If
> > > > you want more you have to use /dev/fb0 and the ioctl. More is an
> > > > uncommon case.
> > > > 2) Switch this to a binary parameter. Now you have to use tools like
> > > > hexdump instead of cat to work with the data. It was nice to be able
> > > > to use cat to see the current map.
> > > >
> > > > Does anyone have preferences for which way to fix it?
> > >
> > > Or...
> > >
> > >  3) Add another file in sysfs which specifies at what index and how many
> > > entries will be read or written from or to the cmap. With this additional
> > > sysfs file, it should be able to handle any reasonable cmap length, but
> > > it will take more than one reading of the color_map file. Another
> > > advantage is that the entire color map need not be read or written if
> > > only one field needs to be changed.
> > >
> > > I've attached a test patch.  Let me know what you think.
> >
> > I like it! ... But, a disadvantages is that it needs to store state between two
> > non-atomic operations. E.g. imagine two processes doing this at the same time.
> 
> Two attributes is a big problem with atomicity. If you want access to
> the full cmap I would switch to a binary attribute or use the existing
> IOCTL.
> 
> Note that you can set the entire map with the new patch, you just
> can't read it. You can store 1-N entries at any base you want.
> 
> >
> > Gr{oetje,eeting}s,
> >
> >                                                 Geert
> >
> > --
> > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> >
> > In personal conversations with technical people, I call myself a hacker. But
> > when I'm talking to journalists I just say "programmer" or something like that.
> >                                                             -- Linus Torvalds
> > -
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/
> >
> 
> 
> --
> Jon Smirl
> jonsmirl@gmail.com
> 


-- 
Jon Smirl
jonsmirl@gmail.com


-------------------------------------------------------
SF.Net email is Sponsored by the Better Software Conference & EXPO September
19-22, 2005 * San Francisco, CA * Development Lifecycle Practices
Agile & Plan-Driven Development * Managing Projects & Teams * Testing & QA
Security * Process Improvement & Measurement * http://www.sqe.com/bsce5sf

Re: [PATCH] fbdev: colormap fixes

[Jon Smirl]

Do we want to apply this patch now to get rid of the buffer overflow hole?
Then we can take our time and work out a better solution.

-- 
Jon Smirl
jonsmirl@gmail.com

Fix a buffer overflow vunerabilty in previous cmap patch
signed-off-by: Jon Smirl <jonsmirl@gmail.com>

diff --git a/drivers/video/fbsysfs.c b/drivers/video/fbsysfs.c
--- a/drivers/video/fbsysfs.c
+++ b/drivers/video/fbsysfs.c
@@ -244,15 +244,15 @@ static ssize_t show_virtual(struct class
 
 /* Format for cmap is "%02x%c%4x%4x%4x\n" */
 /* %02x entry %c transp %4x red %4x blue %4x green \n */
-/* 255 rows at 16 chars equals 4096 */
-/* PAGE_SIZE can be 4096 or larger */
+/* 256 rows at 16 chars equals 4096, the normal page size */
+/* the code will automatically adjust for different page sizes */
 static ssize_t store_cmap(struct class_device *class_device, const char *buf,
 			  size_t count)
 {
 	struct fb_info *fb_info = (struct fb_info *)class_get_devdata(class_device);
 	int rc, i, start, length, transp = 0;
 
-	if ((count > 4096) || ((count % 16) != 0) || (PAGE_SIZE < 4096))
+	if ((count > PAGE_SIZE) || ((count % 16) != 0))
 		return -EINVAL;
 
 	if (!fb_info->fbops->fb_setcolreg && !fb_info->fbops->fb_setcmap)
@@ -317,18 +317,18 @@ static ssize_t show_cmap(struct class_de
 	   !fb_info->cmap.green)
 		return -EINVAL;
 
-	if (PAGE_SIZE < 4096)
+	if (fb_info->cmap.len > PAGE_SIZE / 16)
 		return -EINVAL;
 
 	/* don't mess with the format, the buffer is PAGE_SIZE */
-	/* 255 entries at 16 chars per line equals 4096 = PAGE_SIZE */
+	/* 256 entries at 16 chars per line equals 4096 = PAGE_SIZE */
 	for (i = 0; i < fb_info->cmap.len; i++) {
-		sprintf(&buf[ i * 16], "%02x%c%4x%4x%4x\n", i + fb_info->cmap.start,
+		snprintf(&buf[ i * 16], PAGE_SIZE - i * 16, "%02x%c%4x%4x%4x\n", i
+ fb_info->cmap.start,
 			((fb_info->cmap.transp && fb_info->cmap.transp[i]) ? '*' : ' '),
 			fb_info->cmap.red[i], fb_info->cmap.blue[i],
 			fb_info->cmap.green[i]);
 	}
-	return 4096;
+	return 16 * fb_info->cmap.len;
 }
 
 static ssize_t store_blank(struct class_device *class_device, const char * buf,


-------------------------------------------------------
SF.Net email is Sponsored by the Better Software Conference & EXPO September
19-22, 2005 * San Francisco, CA * Development Lifecycle Practices
Agile & Plan-Driven Development * Managing Projects & Teams * Testing & QA
Security * Process Improvement & Measurement * http://www.sqe.com/bsce5sf

Re: Re: [PATCH] fbdev: colormap fixes

[Geert Uytterhoeven]

On Thu, 28 Jul 2005, Jon Smirl wrote:
> I can't see a way to query how long of cmap the device supports using
> the current fbdev ioctls.

Look at the lengths of the color bitfields?

> I wouldn't even be messing with cmap except for the true/direct color
> support and gamma ramps. Don't I need to know how long the cmap is in
> order to set the right gamma ramp?

Of course.

> If I set a 256 entry gamma ramp on a card with 1024 entries, will it
> still work right? Or do I need to set all 1024 entries?

No, you have to set all 1024 entries. And you will notice, because the lengths
of the color bitfields will be 10, not 8.

> On 7/28/05, Jon Smirl <jonsmirl@gmail.com> wrote:
> > On 7/28/05, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > On Thu, 28 Jul 2005, Antonino A. Daplas wrote:
> > > > Jon Smirl wrote:
> > > > > On 7/28/05, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > > > On Wed, 27 Jul 2005, Linux Kernel Mailing List wrote:
> > > > >
> > > > > There are a couple of ways to fix this.
> > > > > 1) Add a check to limit use of the sysfs attributes to 256 entries. If
> > > > > you want more you have to use /dev/fb0 and the ioctl. More is an
> > > > > uncommon case.
> > > > > 2) Switch this to a binary parameter. Now you have to use tools like
> > > > > hexdump instead of cat to work with the data. It was nice to be able
> > > > > to use cat to see the current map.
> > > > >
> > > > > Does anyone have preferences for which way to fix it?
> > > >
> > > > Or...
> > > >
> > > >  3) Add another file in sysfs which specifies at what index and how many
> > > > entries will be read or written from or to the cmap. With this additional
> > > > sysfs file, it should be able to handle any reasonable cmap length, but
> > > > it will take more than one reading of the color_map file. Another
> > > > advantage is that the entire color map need not be read or written if
> > > > only one field needs to be changed.
> > > >
> > > > I've attached a test patch.  Let me know what you think.
> > >
> > > I like it! ... But, a disadvantages is that it needs to store state between two
> > > non-atomic operations. E.g. imagine two processes doing this at the same time.
> > 
> > Two attributes is a big problem with atomicity. If you want access to
> > the full cmap I would switch to a binary attribute or use the existing
> > IOCTL.
> > 
> > Note that you can set the entire map with the new patch, you just
> > can't read it. You can store 1-N entries at any base you want.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds


-------------------------------------------------------
SF.Net email is Sponsored by the Better Software Conference & EXPO September
19-22, 2005 * San Francisco, CA * Development Lifecycle Practices
Agile & Plan-Driven Development * Managing Projects & Teams * Testing & QA
Security * Process Improvement & Measurement * http://www.sqe.com/bsce5sf

Re: [PATCH] fbdev: colormap fixes

[Geert Uytterhoeven]

On Thu, 28 Jul 2005, Jon Smirl wrote:
> Do we want to apply this patch now to get rid of the buffer overflow hole?

IMHO, yes please.

> Then we can take our time and work out a better solution.

Indeed.

> Fix a buffer overflow vunerabilty in previous cmap patch
> signed-off-by: Jon Smirl <jonsmirl@gmail.com>

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds


-------------------------------------------------------
SF.Net email is Sponsored by the Better Software Conference & EXPO September
19-22, 2005 * San Francisco, CA * Development Lifecycle Practices
Agile & Plan-Driven Development * Managing Projects & Teams * Testing & QA
Security * Process Improvement & Measurement * http://www.sqe.com/bsce5sf

Re: Re: [PATCH] fbdev: colormap fixes

[Jon Smirl]

On 7/28/05, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Thu, 28 Jul 2005, Jon Smirl wrote:
> > I can't see a way to query how long of cmap the device supports using
> > the current fbdev ioctls.
> 
> Look at the lengths of the color bitfields?

Which color bitfields? Does hardware that supports 10bit cmap also
support a 10:10:10 framebuffer? If you can't do 10:10:10 how do the
10bit cmaps work?  Does alpha matter in a 10:10:10 scanout buffer?

-- 
Jon Smirl
jonsmirl@gmail.com


-------------------------------------------------------
SF.Net email is Sponsored by the Better Software Conference & EXPO September
19-22, 2005 * San Francisco, CA * Development Lifecycle Practices
Agile & Plan-Driven Development * Managing Projects & Teams * Testing & QA
Security * Process Improvement & Measurement * http://www.sqe.com/bsce5sf

Re: Re: [PATCH] fbdev: colormap fixes

[Jon Smirl]

On 7/28/05, Jon Smirl <jonsmirl@gmail.com> wrote:
> On 7/28/05, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Thu, 28 Jul 2005, Jon Smirl wrote:
> > > I can't see a way to query how long of cmap the device supports using
> > > the current fbdev ioctls.
> >
> > Look at the lengths of the color bitfields?
> 
> Which color bitfields? Does hardware that supports 10bit cmap also
> support a 10:10:10 framebuffer? If you can't do 10:10:10 how do the
> 10bit cmaps work?  Does alpha matter in a 10:10:10 scanout buffer?

From the OpenGL headers I can see the answer to my questions...
#define GL_RGB10                                0x8052
#define GL_RGB10_A2                          0x8059

OpenGL supports all of these:
#define GL_RGB10                                0x8052
#define GL_RGB12                                0x8053
#define GL_RGB16                                0x8054
#define GL_RGB10_A2                          0x8059
#define GL_RGBA12                               0x805A
#define GL_RGBA16                               0x805B

Are there 12 and 16 bit cmaps too?

-- 
Jon Smirl
jonsmirl@gmail.com


-------------------------------------------------------
SF.Net email is Sponsored by the Better Software Conference & EXPO September
19-22, 2005 * San Francisco, CA * Development Lifecycle Practices
Agile & Plan-Driven Development * Managing Projects & Teams * Testing & QA
Security * Process Improvement & Measurement * http://www.sqe.com/bsce5sf

Re: Re: [PATCH] fbdev: colormap fixes

[Jon Smirl]

I've verified now that all ATI R300+ chips have 10bit cmaps. These are
pretty common so I'd be in favor of making this into a binary
attribute where I can get/set the whole table at once. Given that
OpenGL is already supporting 12 and 16 bits these tables are only
going to get much larger.

1024 entries * 5 fields * 2 bytes = 10KB -- too big for a text attribute.

65536 entries * 5 fields * 2 bytes = 655KB -- way too big for a text attribute.

The bits_per_pixel sysfs attribute is an easy way to tell how many
entries you need. You can just set it at 4, 8, 10, etc until you get
an error. Now you know the max. 2^n and you know how many entries.

-- 
Jon Smirl
jonsmirl@gmail.com


-------------------------------------------------------
SF.Net email is Sponsored by the Better Software Conference & EXPO September
19-22, 2005 * San Francisco, CA * Development Lifecycle Practices
Agile & Plan-Driven Development * Managing Projects & Teams * Testing & QA
Security * Process Improvement & Measurement * http://www.sqe.com/bsce5sf

Re: Re: [PATCH] fbdev: colormap fixes

[Geert Uytterhoeven]

On Thu, 28 Jul 2005, Jon Smirl wrote:
> I've verified now that all ATI R300+ chips have 10bit cmaps. These are
> pretty common so I'd be in favor of making this into a binary
> attribute where I can get/set the whole table at once. Given that
> OpenGL is already supporting 12 and 16 bits these tables are only
> going to get much larger.
> 
> 1024 entries * 5 fields * 2 bytes = 10KB -- too big for a text attribute.
> 
> 65536 entries * 5 fields * 2 bytes = 655KB -- way too big for a text attribute.
> 
> The bits_per_pixel sysfs attribute is an easy way to tell how many
> entries you need. You can just set it at 4, 8, 10, etc until you get
> an error. Now you know the max. 2^n and you know how many entries.

No, bits_per_pixel can be (much) larger than the color map size. E.g. a simple
ARGB8888 directcolor mode has bits_per_pixel = 32 and color map size = 256.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds


-------------------------------------------------------
SF.Net email is Sponsored by the Better Software Conference & EXPO September
19-22, 2005 * San Francisco, CA * Development Lifecycle Practices
Agile & Plan-Driven Development * Managing Projects & Teams * Testing & QA
Security * Process Improvement & Measurement * http://www.sqe.com/bsce5sf

Re: Re: [PATCH] fbdev: colormap fixes

[Jon Smirl]

On 7/28/05, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Thu, 28 Jul 2005, Jon Smirl wrote:
> > I've verified now that all ATI R300+ chips have 10bit cmaps. These are
> > pretty common so I'd be in favor of making this into a binary
> > attribute where I can get/set the whole table at once. Given that
> > OpenGL is already supporting 12 and 16 bits these tables are only
> > going to get much larger.
> >
> > 1024 entries * 5 fields * 2 bytes = 10KB -- too big for a text attribute.
> >
> > 65536 entries * 5 fields * 2 bytes = 655KB -- way too big for a text attribute.
> >
> > The bits_per_pixel sysfs attribute is an easy way to tell how many
> > entries you need. You can just set it at 4, 8, 10, etc until you get
> > an error. Now you know the max. 2^n and you know how many entries.
> 
> No, bits_per_pixel can be (much) larger than the color map size. E.g. a simple
> ARGB8888 directcolor mode has bits_per_pixel = 32 and color map size = 256.

So I have the bits_per_pixel attribute wrong in sysfs. It needs to be
bits_per_color and then let the driver sort it out.  Otherwise there
is no way to set ARGB8888 versus ARGB2101010. With bits per color you
would set 8 or 10.

If that isn't good enough I can switch the attribute to take strings
like ARGB8888.

What do you think, should I just switch to fbconfig names and a binary
cmap attribute?

-- 
Jon Smirl
jonsmirl@gmail.com


-------------------------------------------------------
SF.Net email is Sponsored by the Better Software Conference & EXPO September
19-22, 2005 * San Francisco, CA * Development Lifecycle Practices
Agile & Plan-Driven Development * Managing Projects & Teams * Testing & QA
Security * Process Improvement & Measurement * http://www.sqe.com/bsce5sf

Re: Re: [PATCH] fbdev: colormap fixes

[Antonino A. Daplas]

Jon Smirl wrote:
> Can you review this fix for the issues below? I fixed things to
> automatically adjust the number of entries to whatever fits in
> PAGE_SIZE.
>
> diff --git a/drivers/video/fbsysfs.c b/drivers/video/fbsysfs.c
> --- a/drivers/video/fbsysfs.c
> +++ b/drivers/video/fbsysfs.c
> @@ -244,15 +244,15 @@ static ssize_t show_virtual(struct class
>  
>  /* Format for cmap is "%02x%c%4x%4x%4x\n" */
>   
Why is the alpha channel not given the same weight as the RGB? Wouldn't
it be correct to also give it 4 bytes (16 bits)  Also, what would happen if
you exceed 256 entries, you've only alloted a maximum of 256 for the
index?  This will also be a problem because you cannot access cmap entries
past 255.

So, 4 bytes per channel + 4 bytes for the index will be needed per entry,

Tony



-------------------------------------------------------
SF.Net email is Sponsored by the Better Software Conference & EXPO September
19-22, 2005 * San Francisco, CA * Development Lifecycle Practices
Agile & Plan-Driven Development * Managing Projects & Teams * Testing & QA
Security * Process Improvement & Measurement * http://www.sqe.com/bsce5sf

Re: Re: [PATCH] fbdev: colormap fixes

[Antonino A. Daplas]

Jon Smirl wrote:
> On 7/28/05, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>> On Thu, 28 Jul 2005, Jon Smirl wrote:
>>> I've verified now that all ATI R300+ chips have 10bit cmaps. These are
>>> pretty common so I'd be in favor of making this into a binary
>>> attribute where I can get/set the whole table at once. Given that
>>> OpenGL is already supporting 12 and 16 bits these tables are only
>>> going to get much larger.
>>>
>>> 1024 entries * 5 fields * 2 bytes = 10KB -- too big for a text attribute.
>>>
>>> 65536 entries * 5 fields * 2 bytes = 655KB -- way too big for a text attribute.
>>>
>>> The bits_per_pixel sysfs attribute is an easy way to tell how many
>>> entries you need. You can just set it at 4, 8, 10, etc until you get
>>> an error. Now you know the max. 2^n and you know how many entries.
>> No, bits_per_pixel can be (much) larger than the color map size. E.g. a simple
>> ARGB8888 directcolor mode has bits_per_pixel = 32 and color map size = 256.
> 
> So I have the bits_per_pixel attribute wrong in sysfs. It needs to be
> bits_per_color and then let the driver sort it out.  Otherwise there
> is no way to set ARGB8888 versus ARGB2101010. With bits per color you
> would set 8 or 10.
> 

No, you have to add another attribute for {transp|red|green|blue}.{len,offset}
and another attribute for the pixelformat. Then using those, one can
easily deduce the cmap size.
 
> If that isn't good enough I can switch the attribute to take strings
> like ARGB8888.
> 

Please no.

> What do you think, should I just switch to fbconfig names and a binary
> cmap attribute?
> 

Does a binary attribute not have the same buffer size limitation as
the text attribute?  I really don't know, just asking.

Tony


-------------------------------------------------------
SF.Net email is Sponsored by the Better Software Conference & EXPO September
19-22, 2005 * San Francisco, CA * Development Lifecycle Practices
Agile & Plan-Driven Development * Managing Projects & Teams * Testing & QA
Security * Process Improvement & Measurement * http://www.sqe.com/bsce5sf

Re: Re: [PATCH] fbdev: colormap fixes

[Antonino A. Daplas]

Geert Uytterhoeven wrote:
> On Thu, 28 Jul 2005, Antonino A. Daplas wrote:
>> Jon Smirl wrote:
>>> On 7/28/05, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>>> On Wed, 27 Jul 2005, Linux Kernel Mailing List wrote:
>>> There are a couple of ways to fix this. 
>>> 1) Add a check to limit use of the sysfs attributes to 256 entries. If
>>> you want more you have to use /dev/fb0 and the ioctl. More is an
>>> uncommon case.
>>> 2) Switch this to a binary parameter. Now you have to use tools like
>>> hexdump instead of cat to work with the data. It was nice to be able
>>> to use cat to see the current map.
>>>
>>> Does anyone have preferences for which way to fix it?
>> Or...
>>
>>  3) Add another file in sysfs which specifies at what index and how many
>> entries will be read or written from or to the cmap. With this additional
>> sysfs file, it should be able to handle any reasonable cmap length, but
>> it will take more than one reading of the color_map file. Another
>> advantage is that the entire color map need not be read or written if
>> only one field needs to be changed.
>>
>> I've attached a test patch.  Let me know what you think.
> 
> I like it! ... But, a disadvantages is that it needs to store state between two
> non-atomic operations. E.g. imagine two processes doing this at the same time.

We can add a check that if the incoming buffer index start and length does
not match the current start and len (as set by cmap_range), then exit with
and empty buffer.

Conversely, users  can always check if the output read from color_map matches
the value it entered in cmap_range.

As a side note:

The rest of the fbsysfs attributes suffer from the same thing, especially since the
value of each attribute depends on each other.  What if one process reads the
virtual_size, while another one changes the bits_per_pixel?  I'm pretty sure that
setting bits_per_pixel to a higher value will almost always change the virtual_size.

Currently, the only way to guarantee atomicity is to use the ioctls. The main
reason why we change the video mode, framebuffer format, color format, etc in one
go with fb_set_var() is precisely this.

Tony


 


-------------------------------------------------------
SF.Net email is Sponsored by the Better Software Conference & EXPO September
19-22, 2005 * San Francisco, CA * Development Lifecycle Practices
Agile & Plan-Driven Development * Managing Projects & Teams * Testing & QA
Security * Process Improvement & Measurement * http://www.sqe.com/bsce5sf

Re: Re: [PATCH] fbdev: colormap fixes

[Geert Uytterhoeven]

On Fri, 29 Jul 2005, Antonino A. Daplas wrote:
> Jon Smirl wrote:
> > On 7/28/05, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > On Thu, 28 Jul 2005, Jon Smirl wrote:
> > > > I've verified now that all ATI R300+ chips have 10bit cmaps. These are
> > > > pretty common so I'd be in favor of making this into a binary
> > > > attribute where I can get/set the whole table at once. Given that
> > > > OpenGL is already supporting 12 and 16 bits these tables are only
> > > > going to get much larger.
> > > > 
> > > > 1024 entries * 5 fields * 2 bytes = 10KB -- too big for a text
> > > > attribute.
> > > > 
> > > > 65536 entries * 5 fields * 2 bytes = 655KB -- way too big for a text
> > > > attribute.
> > > > 
> > > > The bits_per_pixel sysfs attribute is an easy way to tell how many
> > > > entries you need. You can just set it at 4, 8, 10, etc until you get
> > > > an error. Now you know the max. 2^n and you know how many entries.
> > > No, bits_per_pixel can be (much) larger than the color map size. E.g. a
> > > simple
> > > ARGB8888 directcolor mode has bits_per_pixel = 32 and color map size =
> > > 256.
> > 
> > So I have the bits_per_pixel attribute wrong in sysfs. It needs to be
> > bits_per_color and then let the driver sort it out.  Otherwise there
> > is no way to set ARGB8888 versus ARGB2101010. With bits per color you
> > would set 8 or 10.
> 
> No, you have to add another attribute for {transp|red|green|blue}.{len,offset}
> and another attribute for the pixelformat. Then using those, one can
> easily deduce the cmap size.

Indeed. One bits_per_color cannot handle e.g. RGB565 (or RGBA{10,10,10,2} :-).

> > If that isn't good enough I can switch the attribute to take strings
> > like ARGB8888.
> > 
> 
> Please no.

Ack.

> > What do you think, should I just switch to fbconfig names and a binary
> > cmap attribute?
> 
> Does a binary attribute not have the same buffer size limitation as
> the text attribute?  I really don't know, just asking.

Yes it has, but since binary data is more compact, you can fit more data in
PAGE_SIZE.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds


-------------------------------------------------------
SF.Net email is Sponsored by the Better Software Conference & EXPO September
19-22, 2005 * San Francisco, CA * Development Lifecycle Practices
Agile & Plan-Driven Development * Managing Projects & Teams * Testing & QA
Security * Process Improvement & Measurement * http://www.sqe.com/bsce5sf

Re: Re: [PATCH] fbdev: colormap fixes

[Jon Smirl]

On 7/29/05, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Fri, 29 Jul 2005, Antonino A. Daplas wrote:
> > Jon Smirl wrote:
> > > On 7/28/05, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > On Thu, 28 Jul 2005, Jon Smirl wrote:
> > > > > I've verified now that all ATI R300+ chips have 10bit cmaps. These are
> > > > > pretty common so I'd be in favor of making this into a binary
> > > > > attribute where I can get/set the whole table at once. Given that
> > > > > OpenGL is already supporting 12 and 16 bits these tables are only
> > > > > going to get much larger.
> > > > >
> > > > > 1024 entries * 5 fields * 2 bytes = 10KB -- too big for a text
> > > > > attribute.
> > > > >
> > > > > 65536 entries * 5 fields * 2 bytes = 655KB -- way too big for a text
> > > > > attribute.
> > > > >
> > > > > The bits_per_pixel sysfs attribute is an easy way to tell how many
> > > > > entries you need. You can just set it at 4, 8, 10, etc until you get
> > > > > an error. Now you know the max. 2^n and you know how many entries.
> > > > No, bits_per_pixel can be (much) larger than the color map size. E.g. a
> > > > simple
> > > > ARGB8888 directcolor mode has bits_per_pixel = 32 and color map size =
> > > > 256.
> > >
> > > So I have the bits_per_pixel attribute wrong in sysfs. It needs to be
> > > bits_per_color and then let the driver sort it out.  Otherwise there
> > > is no way to set ARGB8888 versus ARGB2101010. With bits per color you
> > > would set 8 or 10.
> >
> > No, you have to add another attribute for {transp|red|green|blue}.{len,offset}
> > and another attribute for the pixelformat. Then using those, one can
> > easily deduce the cmap size.
> 
> Indeed. One bits_per_color cannot handle e.g. RGB565 (or RGBA{10,10,10,2} :-).
> 
> > > If that isn't good enough I can switch the attribute to take strings
> > > like ARGB8888.
> > >
> >
> > Please no.
> 
> Ack.

> 
> > > What do you think, should I just switch to fbconfig names and a binary
> > > cmap attribute?
> >
> > Does a binary attribute not have the same buffer size limitation as
> > the text attribute?  I really don't know, just asking.
> 
> Yes it has, but since binary data is more compact, you can fit more data in
> PAGE_SIZE.

Binary attributes are not limited to page size.

-- 
Jon Smirl
jonsmirl@gmail.com


-------------------------------------------------------
SF.Net email is Sponsored by the Better Software Conference & EXPO September
19-22, 2005 * San Francisco, CA * Development Lifecycle Practices
Agile & Plan-Driven Development * Managing Projects & Teams * Testing & QA
Security * Process Improvement & Measurement * http://www.sqe.com/bsce5sf

Re: Re: [PATCH] fbdev: colormap fixes

[James Simmons]

> >  3) Add another file in sysfs which specifies at what index and how many
> > entries will be read or written from or to the cmap. With this additional
> > sysfs file, it should be able to handle any reasonable cmap length, but
> > it will take more than one reading of the color_map file. Another
> > advantage is that the entire color map need not be read or written if
> > only one field needs to be changed.
> > 
> > I've attached a test patch.  Let me know what you think.
> 
> I like it! ... But, a disadvantages is that it needs to store state between two
> non-atomic operations. E.g. imagine two processes doing this at the same time.

   That is really nice. Setting the values in the color map don't have to 
be atomic. Programming the hardware color registers do!! You could do a 
loop filling the color map and then do a sync operation that would flush 
the values to the hardware. It would be really nice if sysfs files had 
hooks for syncing so we could do atomic operations :-) 
     I realized for the cursor it is the same things. We have several 
fields to set but we don't want to program the cursor over and over 
again for every slight change. Instead we shoudl cache the changes
until we want to send those changes to the hardware.





-------------------------------------------------------
SF.Net email is sponsored by: Discover Easy Linux Migration Strategies
from IBM. Find simple to follow Roadmaps, straightforward articles,
informative Webcasts and more! Get everything you need to get up to
speed, fast. http://ads.osdn.com/?ad_id=7477&alloc_id=16492&op=click

Re: Re: [PATCH] fbdev: colormap fixes

[James Simmons]

> > No, bits_per_pixel can be (much) larger than the color map size. E.g. a simple
> > ARGB8888 directcolor mode has bits_per_pixel = 32 and color map size = 256.
> 
> So I have the bits_per_pixel attribute wrong in sysfs. It needs to be
> bits_per_color and then let the driver sort it out.  Otherwise there
> is no way to set ARGB8888 versus ARGB2101010. With bits per color you
> would set 8 or 10.

You would need bits_per_read, bit_per_green. This doesn't event count the 
other color spaces avaliable like YUV.



-------------------------------------------------------
SF.Net email is sponsored by: Discover Easy Linux Migration Strategies
from IBM. Find simple to follow Roadmaps, straightforward articles,
informative Webcasts and more! Get everything you need to get up to
speed, fast. http://ads.osdn.com/?ad_id=7477&alloc_id=16492&op=click