omapfb-main.c: check result of simple_strtoul

omapfb-main.c: check result of simple_strtoul

[Hein Tibosch]

Tomi,


In drivers/video/omap2/omapfb/omapfb-main.c:

 static int omapfb_parse_vram_param(const char *param, int max_entries,
 		unsigned long *sizes, unsigned long *paddrs)
 {
 	int fbnum;
 	unsigned long size;
 	unsigned long paddr = 0;
 	char *p, *start;
  	start = (char *)param;
 	while (1) {
 		p = start;
  		fbnum = simple_strtoul(p, &p, 10);
-		if (p == param)
+		if (p == start)

correct?

Hein

Re: omapfb-main.c: check result of simple_strtoul

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 638 bytes --]

On Wed, 2012-05-09 at 22:47 +0800, Hein Tibosch wrote:
> Tomi,
> 
> 
> In drivers/video/omap2/omapfb/omapfb-main.c:
> 
>  static int omapfb_parse_vram_param(const char *param, int max_entries,
>  		unsigned long *sizes, unsigned long *paddrs)
>  {
>  	int fbnum;
>  	unsigned long size;
>  	unsigned long paddr = 0;
>  	char *p, *start;
>   	start = (char *)param;
>  	while (1) {
>  		p = start;
>   		fbnum = simple_strtoul(p, &p, 10);
> -		if (p == param)
> +		if (p == start)
> 
> correct?

Yes, looks like a correct fix. I'll cook up a patch.

How did you encounter the bug? What was the outcome?

 Tomi


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

Re: omapfb-main.c: check result of simple_strtoul

[Hein Tibosch]

On 5/10/2012 4:23 PM, Tomi Valkeinen wrote:
> On Wed, 2012-05-09 at 22:47 +0800, Hein Tibosch wrote:
>>   		fbnum = simple_strtoul(p, &p, 10);
>> -		if (p == param)
>> +		if (p == start)
>>
>> correct?
> Yes, looks like a correct fix. I'll cook up a patch.
>
> How did you encounter the bug? What was the outcome?
I was reading the source, adding some printk's to get more debugging.

It would fail to detect a syntax error like:

     "omapfb.vram=0:2M,:5M"


If I may ask: why aren't "mode" and "modes" filled in:

    /sys/devices/platform/omapfb/graphics/fb0 ?

Only "modes" will get an entry when a new mode becomes active.

I thought of adding something like this in hdmi.c:

+static void omap_to_fbmode (struct fb_videomode *fb_mode, const struct hdmi_config *cfg)
+{
+	char name[64];
+	unsigned hsync;
+	unsigned vsync;
+	unsigned refresh;
+	snprintf (name, sizeof name, "%d_%s_%d.%d",
+		cfg->cm.code,
+		cfg->cm.mode == HDMI_HDMI ? "hdmi" : "dvi",
+		cfg->timings.x_res,
+		cfg->timings.y_res);
+
+	hsync = cfg->timings.hfp + cfg->timings.hsw + cfg->timings.hbp + cfg->timings.x_res;
+	vsync = cfg->timings.vfp + cfg->timings.vsw + cfg->timings.vbp + cfg->timings.y_res;
+	refresh = (unsigned) ((1000U * cfg->timings.pixel_clock + (hsync * vsync-1) ) / (hsync * vsync));
+	
+	fb_mode->name = kstrdup(name, GFP_KERNEL);	/* optional */
+	fb_mode->refresh = refresh;		/* optional */
+	fb_mode->xres = cfg->timings.x_res;
+	fb_mode->yres = cfg->timings.y_res;
+	fb_mode->pixclock = DIV_ROUND_UP (1000000000UL, cfg->timings.pixel_clock);
+	fb_mode->left_margin = cfg->timings.hfp;
+	fb_mode->right_margin = cfg->timings.hbp;
+	fb_mode->upper_margin = cfg->timings.vfp;
+	fb_mode->lower_margin = cfg->timings.vbp;
+	fb_mode->hsync_len = cfg->timings.hsw;
+	fb_mode->vsync_len = cfg->timings.vsw;
+	fb_mode->sync =
+		(cfg->timings.vsync_pol ? FB_SYNC_VERT_HIGH_ACT : 0) |
+		(cfg->timings.hsync_pol ? FB_SYNC_HOR_HIGH_ACT : 0);
+	fb_mode->vmode = 0;
+	fb_mode->flag = 0;
+}
+
+int omap2_add_video_modes (struct fb_info *fbi)
+{
+	struct fb_videomode fb_mode;
+	int i;
+	int count = 0;
+
+	INIT_LIST_HEAD(&fbi->modelist);
+
+	for (i = 0; i < ARRAY_SIZE(cea_timings); i++) {
+		omap_to_fbmode (&fb_mode, &cea_timings[i]);
+		fb_add_videomode(&fb_mode, &fbi->modelist);
+		count++;
+	}
+	for (i = 0; i < ARRAY_SIZE(vesa_timings); i++) {
+		omap_to_fbmode (&fb_mode, &vesa_timings[i]);
+		fb_add_videomode(&fb_mode, &fbi->modelist);
+		count++;
+	}
+	return count;
+}


and call it from omapfb-main.c:

static int omapfb_create_framebuffers(struct omapfb2_device *fbdev)
{
	...
 		clear_fb_info(fbi);

 		fbdev->fbs[i] = fbi;
+		omap2_add_video_modes (fbi);

With this patch I get 34 working entries from "cat modes":

U:1024x768p-59
U:1280x800p-67
U:1680x1050p-59
U:1400x1050p-59
U:1280x768p-59
U:1920x1080p-60
U:1366x768p-59
...

which makes testing a bit easier.

But maybe I'm reinventing some wheel?


Hein

Re: omapfb-main.c: check result of simple_strtoul

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 1167 bytes --]

On Thu, 2012-05-10 at 18:51 +0800, Hein Tibosch wrote:
> On 5/10/2012 4:23 PM, Tomi Valkeinen wrote:
> > On Wed, 2012-05-09 at 22:47 +0800, Hein Tibosch wrote:
> >>   		fbnum = simple_strtoul(p, &p, 10);
> >> -		if (p == param)
> >> +		if (p == start)
> >>
> >> correct?
> > Yes, looks like a correct fix. I'll cook up a patch.
> >
> > How did you encounter the bug? What was the outcome?
> I was reading the source, adding some printk's to get more debugging.
> 
> It would fail to detect a syntax error like:
> 
>      "omapfb.vram=0:2M,:5M"
> 

Ok, thanks.

> If I may ask: why aren't "mode" and "modes" filled in:
> 
>     /sys/devices/platform/omapfb/graphics/fb0 ?
> 
> Only "modes" will get an entry when a new mode becomes active.

No particular reason, nobody just has implemented it.

> I thought of adding something like this in hdmi.c:
> 
> +static void omap_to_fbmode (struct fb_videomode *fb_mode, const struct hdmi_config *cfg)
> +{

You can't add framebuffer code to omapdss. omapdss is the lowlevel
driver that cannot depend on the fb driver. omapdss is also used by
other display drivers like omapdrm.

 Tomi


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