Skip to contentSkip to conversation

drivers/video/fbcmap.c kmalloc audit

PATCH In review Leann Ogasawara 6 participants 14 messages Jan 27, 2004 – Feb 18, 2004

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 drivers/video/fbcmap.c kmalloc audit

Hi All,

Patch to audit kmalloc()'s and handle errors accordingly. Thanks,

Leann

diffed against 2.6.2-rc2

=== drivers/video/fbcmap.c 1.9 vs edited ==--- 1.9/drivers/video/fbcmap.c Mon Mar 31 13:51:12 2003
+++ edited/drivers/video/fbcmap.c Mon Jan 26 17:38:53 2004
@@ -98,14 +98,14 @@
if (!len)
return 0;
if (!(cmap->red = kmalloc(size, GFP_ATOMIC)))
- return -1;
+ goto err_red;
if (!(cmap->green = kmalloc(size, GFP_ATOMIC)))
- return -1;
+ goto err_green;
if (!(cmap->blue = kmalloc(size, GFP_ATOMIC)))
- return -1;
+ goto err_blue;
if (transp) {
if (!(cmap->transp = kmalloc(size, GFP_ATOMIC)))
- return -1;
+ goto err_transp;
} else

 	    cmap->transp = NULL;
     }
@@ -113,6 +113,17 @@
     cmap->len = len;
     fb_copy_cmap(fb_default_cmap(len), cmap, 0);
     return 0;

+
+ err_transp:
+ kfree(cmap->blue);
+ err_blue:
+ kfree(cmap->green);
+ err_green:
+ kfree(cmap->red);
+ err_red:

+    cmap->red = cmap->green = cmap->blue = cmap->transp = NULL;
+    cmap->len = 0;
+    return -1;

}

/**

_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors

#1 lore raw

PATCH 1/1 drivers/video/vga16fb.c ioremap() and

Hi All,

This is an updated patch of the previous ioremap/iounmap patch I submitted for vga16fb.c This not only includes the ioremap audit and error handling but also include fb_alloc_cmap() audit and error handling. Thanks,

Leann

diffed against 2.6.2-rc2

note: there are more cases of fb_alloc_cmap() needing to be audited that I'll get around to later.

=== drivers/video/vga16fb.c 1.33 vs edited ==--- 1.33/drivers/video/vga16fb.c Thu Apr 24 03:30:41 2003
+++ edited/drivers/video/vga16fb.c Mon Jan 26 17:33:26 2004
@@ -1341,6 +1341,7 @@
int __init vga16fb_init(void)
{
int i;
+ int ret;

printk(KERN_DEBUG "vga16fb: initializing\n");

@@ -1349,7 +1350,8 @@

         vga16fb.screen_base = ioremap(VGA_FB_PHYS, VGA_FB_PHYS_LEN);

if (!vga16fb.screen_base) {
printk(KERN_ERR "vga16fb: unable to map device\n");
- return -ENOMEM;
+ ret = -ENOMEM;
+ goto err_ioremap;
}
printk(KERN_INFO "vga16fb: mapped to 0x%p\n", vga16fb.screen_base);

@@ -1371,28 +1373,44 @@
vga16fb.flags = FBINFO_FLAG_DEFAULT;

i = (vga16fb_defined.bits_per_pixel = 8) ? 256 : 16;
- fb_alloc_cmap(&vga16fb.cmap, i, 0);
-
- if (vga16fb_check_var(&vga16fb.var, &vga16fb))
- return -EINVAL;
+ ret = fb_alloc_cmap(&vga16fb.cmap, i, 0);
+ if (ret) {
+ printk(KERN_ERR "vga16fb: unable to allocate colormap\n");
+ ret = -ENOMEM;
+ goto err_alloc_cmap;
+ }
+ if (vga16fb_check_var(&vga16fb.var, &vga16fb)) {
+ printk(KERN_ERR "vga16fb: unable to validate variable\n");
+ ret = -EINVAL;
+ goto err_check_var;
+ }

vga16fb_update_fix(&vga16fb);

if (register_framebuffer(&vga16fb) < 0) {
- iounmap(vga16fb.screen_base);
- return -EINVAL;
+ printk(KERN_ERR "vga16fb: unable to register framebuffer\n");
+ ret = -EINVAL;
+ goto err_check_var;
}

printk(KERN_INFO "fb%d: %s frame buffer device\n",
vga16fb.node, vga16fb.fix.id);

return 0;
+
+ err_check_var:
+ fb_dealloc_cmap(&vga16fb.cmap);
+ err_alloc_cmap:
+ iounmap(vga16fb.screen_base);
+ err_ioremap:
+ return ret;
}

static void __exit vga16fb_exit(void)

 {
     unregister_framebuffer(&vga16fb);
     iounmap(vga16fb.screen_base);
+    fb_dealloc_cmap(&vga16fb.cmap);
     /* XXX unshare VGA regions */

}

_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors

#2 lore raw
In reply to Leann Ogasawara

Em Tue, Jan 27, 2004 at 01:37:01PM -0800, Leann Ogasawara escreveu:

Quoted 34 lines

Hi All,

Patch to audit kmalloc()'s and handle errors accordingly. Thanks,

Leann

diffed against 2.6.2-rc2

=== drivers/video/fbcmap.c 1.9 vs edited ==> --- 1.9/drivers/video/fbcmap.c Mon Mar 31 13:51:12 2003
+++ edited/drivers/video/fbcmap.c Mon Jan 26 17:38:53 2004

(quoted diff) +5 -4
@@ -98,14 +98,14 @@
 	if (!len)
 	    return 0;
 	if (!(cmap->red = kmalloc(size, GFP_ATOMIC)))
-	    return -1;
+	    goto err_red;
 	if (!(cmap->green = kmalloc(size, GFP_ATOMIC)))
-	    return -1;
+	    goto err_green;
 	if (!(cmap->blue = kmalloc(size, GFP_ATOMIC)))
-	    return -1;
+	    goto err_blue;
 	if (transp) {
 	    if (!(cmap->transp = kmalloc(size, GFP_ATOMIC)))
-		return -1;
+		goto err_transp;
 	} else
 	    cmap->transp = NULL;
     }
@@ -113,6 +113,17 @@
     cmap->len = len;
     fb_copy_cmap(fb_default_cmap(len), cmap, 0);
     return 0;
+

Review line 126

Quoted 10 lines
(quoted diff) +10 -0
+ err_transp:
+    kfree(cmap->blue);
+ err_blue:
+    kfree(cmap->green);
+ err_green:
+    kfree(cmap->red);
+ err_red:
+    cmap->red = cmap->green = cmap->blue = cmap->transp = NULL;
+    cmap->len = 0;
+    return -1;

Looks good to me.

- Arnaldo
_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors

#3 lore raw
In reply to Leann Ogasawara
|+ err_transp:
|+    kfree(cmap->blue);
|+ err_blue:
|+    kfree(cmap->green);
|+ err_green:
|+    kfree(cmap->red);
|+ err_red:
|+    cmap->red = cmap->green = cmap->blue = cmap->transp = NULL;
|+    cmap->len = 0;
|+    return -1;  <--+
                     |

---------------------+
|
+-> IMHO "return -1" should be replaced with "return -ENOMEM"

Tnx.

Signature · 15 lines
Daniele.




"I could have made money this way, and perhaps amused myself writing code. 
But I knew that at the end of my career, I would look back on years of 
building walls to divide people, and feel I had spent my life making the 
world a worse place."                               
                                                          Richard Stallman

_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors
#4 lore raw
In reply to Leann Ogasawara

On Tue, 27 Jan 2004 22:54:19 +0100 Daniele Bellucci <[email protected]> wrote:

| 
| |+ err_transp:
| |+    kfree(cmap->blue);
| |+ err_blue:
| |+    kfree(cmap->green);
| |+ err_green:
| |+    kfree(cmap->red);
| |+ err_red:
| |+    cmap->red = cmap->green = cmap->blue = cmap->transp = NULL;
| |+    cmap->len = 0;
| |+    return -1;  <--+
|                      |
| ---------------------+
| |
| +-> IMHO "return -1" should be replaced with "return -ENOMEM"

Maybe in theory. In practice it is documented to return 0 for success or -1 on error. Changing that would require auditing all callers of it....

--
~Randy
kernel-janitors project: http://janitor.kernelnewbies.org/
_______________________________________________
Kernel-janitors mailing list [email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors

#5 lore raw
In reply to Leann Ogasawara

On Tue, Jan 27, 2004 at 02:05:18PM -0800, Randy.Dunlap wrote:

|On Tue, 27 Jan 2004 22:54:19 +0100 Daniele Bellucci <[email protected]> wrote:
|
|| 
|| |+ err_transp:
|| |+    kfree(cmap->blue);
|| |+ err_blue:
|| |+    kfree(cmap->green);
|| |+ err_green:
|| |+    kfree(cmap->red);
|| |+ err_red:
|| |+    cmap->red = cmap->green = cmap->blue = cmap->transp = NULL;
|| |+    cmap->len = 0;
|| |+    return -1;  <--+
||                      |
|| ---------------------+
|| |
|| +-> IMHO "return -1" should be replaced with "return -ENOMEM"
|
|
|Maybe in theory.  In practice it is documented to return 0 for
|success or -1 on error.  Changing that would require auditing all
|callers of it....

I would be very surprised to see " if (retval = -1) " in the code of any caller, BTW patch apparently looks good to me.

A quick find/grep shows that a few callers didn't audit the return code.

Signature · 15 lines
Daniele.




"I could have made money this way, and perhaps amused myself writing code. 
But I knew that at the end of my career, I would look back on years of 
building walls to divide people, and feel I had spent my life making the 
world a worse place."                               
                                                          Richard Stallman

_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors
#6 lore raw

PATCH 1/1 drivers/video/radeonfb.c fb_alloc_cmap()

In reply to Leann Ogasawara

Hi all,

Patch audits fb_alloc_cmap() and also incorporates an error path. Feedback appreciated. Thanks,

Leann

=== drivers/video/radeonfb.c 1.35 vs edited ==--- 1.35/drivers/video/radeonfb.c Wed Jan 21 09:48:54 2004
+++ edited/drivers/video/radeonfb.c Thu Feb 5 11:57:31 2004
@@ -1287,6 +1287,7 @@

 {
         struct fb_info *info = &rinfo->info;

struct fb_var_screeninfo var;
+ int ret;

         var = radeonfb_default_var;
         if ((radeon_init_disp_var(rinfo, &var)) < 0)

@@ -1296,7 +1297,9 @@
rinfo->bpp = var.bits_per_pixel;

info->var = var;
- fb_alloc_cmap(&info->cmap, 256, 0);
+ ret = fb_alloc_cmap(&info->cmap, 256, 0);
+ if (ret)
+ return ret;

 	var.activate = FB_ACTIVATE_NOW;
         return 0;

@@ -2806,7 +2809,9 @@
{
struct radeonfb_info *rinfo;
struct radeon_chip_info *rci = &radeon_chip_info[ent->driver_data];
+ struct fb_info *info;
u32 tmp;
+ int ret;

RTRACE("radeonfb_pci_register BEGIN\n");

@@ -2823,7 +2828,7 @@
}

memset (rinfo, 0, sizeof (struct radeonfb_info));
- //info = &rinfo->info;
+ info = &rinfo->info;
rinfo->pdev = pdev;
strcpy(rinfo->name, rci->name);
rinfo->arch = rci->arch;
@@ -2836,29 +2841,23 @@
if (!request_mem_region (rinfo->fb_base_phys,
pci_resource_len(pdev, 0), "radeonfb")) {
printk ("radeonfb: cannot reserve FB region\n");
- kfree (rinfo);
- return -ENODEV;
+ ret = -ENODEV;
+ goto err_mem_region_fb;
}

if (!request_mem_region (rinfo->mmio_base_phys,
pci_resource_len(pdev, 2), "radeonfb")) {
printk ("radeonfb: cannot reserve MMIO region\n");
- release_mem_region (rinfo->fb_base_phys,
- pci_resource_len(pdev, 0));
- kfree (rinfo);
- return -ENODEV;
+ ret = -ENODEV;
+ goto err_mem_region_mmio;
}

/* map the regions */
rinfo->mmio_base = (unsigned long) ioremap (rinfo->mmio_base_phys,
RADEON_REGSIZE);
if (!rinfo->mmio_base) {
printk ("radeonfb: cannot map MMIO\n");
- release_mem_region (rinfo->mmio_base_phys,
- pci_resource_len(pdev, 2));
- release_mem_region (rinfo->fb_base_phys,
- pci_resource_len(pdev, 0));
- kfree (rinfo);
- return -ENODEV;
+ ret = -ENODEV;
+ goto err_ioremap_mmio;
}

rinfo->chipset = pdev->device;
@@ -2977,26 +2976,16 @@
if ((rinfo->dviDisp_type = MT_DFP) || (rinfo->dviDisp_type = MT_LCD)
||
(rinfo->crtDisp_type = MT_DFP)) {
if (!radeon_get_dfpinfo(rinfo)) {
- iounmap ((void*)rinfo->mmio_base);
- release_mem_region (rinfo->mmio_base_phys,
- pci_resource_len(pdev, 2));
- release_mem_region (rinfo->fb_base_phys,
- pci_resource_len(pdev, 0));
- kfree (rinfo);
- return -ENODEV;
+ ret = -ENODEV;
+ goto err_ioremap_fb;
}
}

rinfo->fb_base = (unsigned long) ioremap (rinfo->fb_base_phys,
rinfo->video_ram);
if (!rinfo->fb_base) {
printk ("radeonfb: cannot map FB\n");
- iounmap ((void*)rinfo->mmio_base);
- release_mem_region (rinfo->mmio_base_phys,
- pci_resource_len(pdev, 2));
- release_mem_region (rinfo->fb_base_phys,
- pci_resource_len(pdev, 0));
- kfree (rinfo);
- return -ENODEV;
+ ret = -ENODEV;
+ goto err_ioremap_fb;
}

/* I SHOULD FIX THAT CRAP ! I should probably mimmic XFree DRI
@@ -3034,7 +3023,12 @@
radeon_save_state (rinfo, &rinfo->init_state);

/* set all the vital stuff */
- radeon_set_fbinfo (rinfo);
+ ret = radeon_set_fbinfo (rinfo);
+ if (ret) {
+ printk(KERN_ERR "fadeonfb: unable to set fb info\n");
+ ret = -ENOMEM;
+ goto err_set_fbinfo;
+ }

pci_set_drvdata(pdev, rinfo);
rinfo->next = board_list;
@@ -3042,14 +3036,8 @@

if (register_framebuffer ((struct fb_info *) rinfo) < 0) {
printk ("radeonfb: could not register framebuffer\n");
- iounmap ((void*)rinfo->fb_base);
- iounmap ((void*)rinfo->mmio_base);
- release_mem_region (rinfo->mmio_base_phys,
- pci_resource_len(pdev, 2));
- release_mem_region (rinfo->fb_base_phys,
- pci_resource_len(pdev, 0));
- kfree (rinfo);
- return -ENODEV;
+ ret = -ENODEV;
+ goto err_reg_fb;
}

#ifdef CONFIG_MTRR
@@ -3087,14 +3075,29 @@
RTRACE("radeonfb_pci_register END\n");

return 0;
-}
-

+ err_reg_fb:
+ fb_dealloc_cmap(&info->cmap);
+ err_set_fbinfo:
+ iounmap ((void*)rinfo->fb_base);
+ err_ioremap_fb:
+ iounmap ((void*)rinfo->mmio_base);
+ err_ioremap_mmio:
+ release_mem_region (rinfo->mmio_base_phys,
+ pci_resource_len(pdev, 2));
+ err_mem_region_mmio:
+ release_mem_region (rinfo->fb_base_phys,
+ pci_resource_len(pdev, 0));
+ err_mem_region_fb:
+ kfree (rinfo);
+ return ret;
+}

static void __devexit radeonfb_pci_unregister (struct pci_dev *pdev)

 {
         struct radeonfb_info *rinfo = pci_get_drvdata(pdev);
- 
+        struct fb_info *info = &rinfo->info;
+
         if (!rinfo)
                 return;

@@ -3111,7 +3114,7 @@
#endif

         unregister_framebuffer ((struct fb_info *) rinfo);
-                
+	fb_dealloc_cmap(&info->cmap);
         iounmap ((void*)rinfo->mmio_base);
         iounmap ((void*)rinfo->fb_base);

_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors

#7 lore raw

PATCH 1/1 drivers/video/cyber2000fb.c

In reply to Leann Ogasawara

Hi All,

Patch audits fb_alloc_cmap() and uses fb_dealloc_cmap() to free colormap. Thanks,

Leann

diffed against 2.6.2

=== drivers/video/cyber2000fb.c 1.31 vs edited ==--- 1.31/drivers/video/cyber2000fb.c Thu Jul 31 08:58:45 2003
+++ edited/drivers/video/cyber2000fb.c Fri Feb 6 14:33:45 2004
@@ -1223,6 +1223,7 @@
cyberpro_alloc_fb_info(unsigned int id, char *name) {
struct cfb_info *cfb;
+ int ret;

cfb = kmalloc(sizeof(struct cfb_info) +
sizeof(u32) * 16, GFP_KERNEL);
@@ -1284,7 +1285,11 @@
cfb->fb.flags = FBINFO_FLAG_DEFAULT;
cfb->fb.pseudo_palette = (void *)(cfb + 1);

- fb_alloc_cmap(&cfb->fb.cmap, NR_PALETTE, 0);
+ ret = fb_alloc_cmap(&cfb->fb.cmap, NR_PALETTE, 0);
+ if (ret) {
+ kfree(cfb);
+ return NULL;
+ }

return cfb;
}
@@ -1296,7 +1301,7 @@
/*
* Free the colourmap
*/
- fb_alloc_cmap(&cfb->fb.cmap, 0, 0);
+ fb_dealloc_cmap(&cfb->fb.cmap);

kfree(cfb);
}

_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors

#8 lore raw

PATCH 1/1 drivers/video/vfb.c fb_alloc_cmap() audit

In reply to Leann Ogasawara

Hi All,

Patch audits fb_alloc_cmap() and adds error path. Feedback welcome. Thanks,

Leann

diffed against 2.6.2

=== drivers/video/vfb.c 1.27 vs edited ==--- 1.27/drivers/video/vfb.c Thu Apr 24 03:30:41 2003
+++ edited/drivers/video/vfb.c Thu Feb 5 11:04:16 2004
@@ -437,17 +437,28 @@
fb_info.pseudo_palette = &vfb_pseudo_palette; fb_info.flags = FBINFO_FLAG_DEFAULT;

- fb_alloc_cmap(&fb_info.cmap, 256, 0);
+ retval = fb_alloc_cmap(&fb_info.cmap, 256, 0);
+ if (retval) {
+ retval = -ENOMEM;
+ goto err_alloc_cmap;
+ }

if (register_framebuffer(&fb_info) < 0) {
- vfree(videomemory);
- return -EINVAL;
+ retval = -EINVAL;
+ goto err_reg_fb;
}

printk(KERN_INFO
"fb%d: Virtual frame buffer device, using %ldK of video
memory\n",
fb_info.node, videomemorysize >> 10);
return 0;
+
+ err_reg_fb:
+ fb_dealloc_cmap(&fb_info.cmap);
+
+ err_alloc_cmap:
+ vfree(videomemory);
+ return retval;
}

#ifdef MODULE
@@ -455,6 +466,7 @@
static void __exit vfb_cleanup(void)
{
unregister_framebuffer(&fb_info);
+ fb_dealloc_cmap(&fb_info.cmap);
vfree(videomemory);
}

_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors

#9 lore raw

PATCH 1/1 drivers/video/vesafb.c fb_alloc_cmap()

In reply to Leann Ogasawara

Hi All,

Patch audits fb_aloc_cmap() and incorporates an error path. Feedback appreciated. Thanks,

Leann

diffed against 2.6.2

=== drivers/video/vesafb.c 1.33 vs edited ==--- 1.33/drivers/video/vesafb.c Thu Jul 17 22:30:53 2003
+++ edited/drivers/video/vesafb.c Thu Feb 5 08:20:05 2004
@@ -218,6 +218,8 @@
{
int video_cmap_len;
int i;
+ int ret;
+ int failed = 0;

if (screen_info.orig_video_isVGA != VIDEO_TYPE_VLFB)
return -ENXIO;
@@ -249,15 +251,16 @@
vesafb_fix.smem_start);
/* We cannot make this fatal. Sometimes this comes from magic
spaces our resource handlers simply don't know about */
+ failed = 1;
}

fb_info.screen_base = ioremap(vesafb_fix.smem_start,
vesafb_fix.smem_len);
if (!fb_info.screen_base) {
- release_mem_region(vesafb_fix.smem_start, vesafb_fix.smem_len);
printk(KERN_ERR
"vesafb: abort, cannot ioremap video memory 0x%x @ 0x%lx\n",
vesafb_fix.smem_len, vesafb_fix.smem_start);
- return -EIO;
+ ret = -EIO;
+ goto err_ioremap;
}

printk(KERN_INFO "vesafb: framebuffer at 0x%lx, mapped to 0x%p, size
%dk\n",
@@ -364,14 +367,33 @@
fb_info.pseudo_palette = pseudo_palette;
fb_info.flags = FBINFO_FLAG_DEFAULT;

- fb_alloc_cmap(&fb_info.cmap, video_cmap_len, 0);
-
- if (register_framebuffer(&fb_info)<0)
- return -EINVAL;
-
+ ret = fb_alloc_cmap(&fb_info.cmap, video_cmap_len, 0);
+ if (ret) {
+ printk(KERN_ERR "vesafb: unable to allocate colormap\n");
+ ret = -ENOMEM;
+ goto err_alloc_cmap;
+ }
+ if (register_framebuffer(&fb_info)<0) {
+ printk (KERN_ERR "vesafb: unable to register framebuffer\n");
+ ret = -EINVAL;
+ goto err_reg_fb;
+ }
printk(KERN_INFO "fb%d: %s frame buffer device\n",
fb_info.node, fb_info.fix.id);
return 0;
+
+
+ err_reg_fb:
+ fb_dealloc_cmap(&fb_info.cmap);
+
+ err_alloc_cmap:
+ iounmap(fb_info.screen_base);
+
+ err_ioremap:
+ if (!failed)
+ release_mem_region(vesafb_fix.smem_start, vesafb_fix.smem_len);
+
+ return ret;
}

/*

_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors

#10 lore raw

PATCH 1/1 drivers/video/neofb.c fb_alloc_cmap()

In reply to Leann Ogasawara

Hi All,

Patch audits fb_alloc_cmap() and fixes up error handling. Feedback appreciated. Thanks,

Leann

diffed against 2.6.2

=== drivers/video/neofb.c 1.31 vs edited ==--- 1.31/drivers/video/neofb.c Tue Feb 3 21:39:34 2004
+++ edited/drivers/video/neofb.c Fri Feb 6 08:27:13 2004
@@ -1637,13 +1637,12 @@

DBG("neo_unmap_mmio");

- if (par->mmio_vbase) {
- iounmap(par->mmio_vbase);
- par->mmio_vbase = NULL;
+ iounmap(par->mmio_vbase);
+ par->mmio_vbase = NULL;
+
+ release_mem_region(info->fix.mmio_start,
+ info->fix.mmio_len);

- release_mem_region(info->fix.mmio_start,
- info->fix.mmio_len);
- }
}

static int __devinit neo_map_video(struct fb_info *info,
@@ -1687,19 +1686,17 @@

DBG("neo_unmap_video");

- if (info->screen_base) {
#ifdef CONFIG_MTRR
- struct neofb_par *par = (struct neofb_par *) info->par;
- mtrr_del(par->mtrr, info->fix.smem_start,
- info->fix.smem_len);
+ struct neofb_par *par = (struct neofb_par *) info->par;
+ mtrr_del(par->mtrr, info->fix.smem_start,
+ info->fix.smem_len);
#endif

- iounmap(info->screen_base);
- info->screen_base = NULL;
+ iounmap(info->screen_base);
+ info->screen_base = NULL;

- release_mem_region(info->fix.smem_start,
- info->fix.smem_len);
- }
+ release_mem_region(info->fix.smem_start,
+ info->fix.smem_len);
}

static int __devinit neo_init_hw(struct fb_info *info)
@@ -1880,6 +1877,7 @@
{
struct fb_info *info;
struct neofb_par *par;
+ int ret;

info = kmalloc(sizeof(struct fb_info) + sizeof(struct neofb_par) +
sizeof(u32) * 17, GFP_KERNEL);
@@ -1942,7 +1940,11 @@
info->par = par;
info->pseudo_palette = (void *) (par + 1);

- fb_alloc_cmap(&info->cmap, NR_PALETTE, 0);
+ ret = fb_alloc_cmap(&info->cmap, NR_PALETTE, 0);
+ if (ret) {
+ kfree(info);
+ info = NULL;
+ }

return info;
}
@@ -1953,7 +1955,7 @@
/*
* Free the colourmap
*/
- fb_alloc_cmap(&info->cmap, 0, 0);
+ fb_dealloc_cmap(&info->cmap);

kfree(info);
}
@@ -1978,21 +1980,21 @@
err = -ENOMEM;
info = neo_alloc_fb_info(dev, id);
if (!info)
- goto failed;
+ return err;

err = neo_map_mmio(info, dev);
if (err)
- goto failed;
+ goto err_map_mmio;

video_len = neo_init_hw(info);
if (video_len < 0) {
err = video_len;
- goto failed;
+ goto err_init_hw;
}

err = neo_map_video(info, dev, video_len); if (err)
- goto failed;
+ goto err_init_hw;

/*
* Calculate the hsync and vsync frequencies. Note that
@@ -2016,7 +2018,7 @@

err = register_framebuffer(info);
if (err < 0)
- goto failed;
+ goto err_reg_fb;

printk(KERN_INFO "fb%d: %s frame buffer device\n",
info->node, info->fix.id);
@@ -2027,9 +2029,11 @@
pci_set_drvdata(dev, info);
return 0;

- failed:
+ err_reg_fb:
neo_unmap_video(info);
+ err_init_hw:
neo_unmap_mmio(info);
+ err_map_mmio:
neo_free_fb_info(info);

return err;

_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors

#11 lore raw

Re: [Kernel-janitors] [PATCH] drivers/video/radeonfb.c

In reply to Leann Ogasawara
Quoted 5 lines · Leann Ogasawara

Hi all,

Patch audits fb_alloc_cmap() and also incorporates an error path. Feedback appreciated. Thanks,

...

Review line 2812

Quoted 5 lines
(quoted diff) +1 -0
@@ -2806,7 +2809,9 @@
 {
 	struct radeonfb_info *rinfo;
 	struct radeon_chip_info *rci = &radeon_chip_info[ent->driver_data];
+        struct fb_info *info;

Spaces instead of tabs.
Do we really need this? It's only used once.

...

Review line 2815

Quoted 3 lines
(quoted diff) +0 -0
 	/* map the regions */
 	rinfo->mmio_base = (unsigned long) ioremap (rinfo->mmio_base_phys,
 RADEON_REGSIZE);

Turn off wordwrap.

Domen

_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors

#12 lore raw

PATCH 1/1 drivers/video/pm2fb.c MIN/MAX removal

In reply to Leann Ogasawara

Hi!
Patch (against 2.6.3-rc1) removes unnecessary min/max macros and changes calls to use kernel.h macros instead. Feedback always welcome
Michael

ps: file didnt compile before, maybe thats my next task ;-)
But there are still more min/max macros lyings around, so feedback on those first patches is really welcome!

drivers/video/pm2fb.c +2 -10
@@ -100,14 +100,6 @@
 #define DEFRW()		mb()
 #endif
 
-#ifndef MIN
-#define MIN(a,b) ((a)<(b)?(a):(b))
-#endif
-
-#ifndef MAX
-#define MAX(a,b) ((a)>(b)?(a):(b))
-#endif
-
 #define VIDEO_MASK	0x00011e7f	/* r/w values for VIDEO_CONTROL */
 
 #define PM2FF_ACCEL		(1L<<0)
@@ -1847,8 +1839,8 @@
 	if (pm2fb_options.flags & OPTF_YPAN) {
 		i->current_par.height=i->regions.fb_size/
 			(i->current_par.width*i->current_par.depth/8);
-		i->current_par.height=MIN(i->current_par.height,2047);
-		i->current_par.height=MAX(i->current_par.height,
+		i->current_par.height=min(i->current_par.height,(u32)2047);
+		i->current_par.height=max(i->current_par.height,
 					  pm2fb_options.user_mode.height);
 	}
 }

_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors

#13 lore raw

PATCH 1/1 drivers/video/neofb.patch update

In reply to Leann Ogasawara

Hi All,

I had previously sent this patch to audit fb_alloc_cmap() and fix up some error handling. This patch now also includes the removal of some __devinit markings from functions that are called from a __devexit function. Thanks,

Leann

diffed against linux-2.6.3

=== drivers/video/neofb.c 1.31 vs edited ==--- 1.31/drivers/video/neofb.c Tue Feb 3 21:39:34 2004
+++ edited/drivers/video/neofb.c Wed Feb 18 10:26:09 2004
@@ -1631,19 +1631,18 @@
return 0;
}

-static void __devinit neo_unmap_mmio(struct fb_info *info) +static void neo_unmap_mmio(struct fb_info *info)
{
struct neofb_par *par = (struct neofb_par *) info->par;

DBG("neo_unmap_mmio");

- if (par->mmio_vbase) {
- iounmap(par->mmio_vbase);
- par->mmio_vbase = NULL;
+ iounmap(par->mmio_vbase);
+ par->mmio_vbase = NULL;
+
+ release_mem_region(info->fix.mmio_start,
+ info->fix.mmio_len);

- release_mem_region(info->fix.mmio_start,
- info->fix.mmio_len);
- }
}

static int __devinit neo_map_video(struct fb_info *info,
@@ -1682,24 +1681,22 @@
return 0;
}

-static void __devinit neo_unmap_video(struct fb_info *info) +static void neo_unmap_video(struct fb_info *info)
{

DBG("neo_unmap_video");

- if (info->screen_base) {
#ifdef CONFIG_MTRR
- struct neofb_par *par = (struct neofb_par *) info->par;
- mtrr_del(par->mtrr, info->fix.smem_start,
- info->fix.smem_len);
+ struct neofb_par *par = (struct neofb_par *) info->par;
+ mtrr_del(par->mtrr, info->fix.smem_start,
+ info->fix.smem_len);
#endif

- iounmap(info->screen_base);
- info->screen_base = NULL;
+ iounmap(info->screen_base);
+ info->screen_base = NULL;

- release_mem_region(info->fix.smem_start,
- info->fix.smem_len);
- }
+ release_mem_region(info->fix.smem_start,
+ info->fix.smem_len);
}

static int __devinit neo_init_hw(struct fb_info *info)
@@ -1880,6 +1877,7 @@
{
struct fb_info *info;
struct neofb_par *par;
+ int ret;

info = kmalloc(sizeof(struct fb_info) + sizeof(struct neofb_par) +
sizeof(u32) * 17, GFP_KERNEL);
@@ -1942,18 +1940,22 @@
info->par = par;
info->pseudo_palette = (void *) (par + 1);

- fb_alloc_cmap(&info->cmap, NR_PALETTE, 0);
+ ret = fb_alloc_cmap(&info->cmap, NR_PALETTE, 0);
+ if (ret) {
+ kfree(info);
+ info = NULL;
+ }

return info;
}

-static void __devinit neo_free_fb_info(struct fb_info *info) +static void neo_free_fb_info(struct fb_info *info)
{
if (info) {
/*
* Free the colourmap
*/
- fb_alloc_cmap(&info->cmap, 0, 0);
+ fb_dealloc_cmap(&info->cmap);

kfree(info);
}
@@ -1978,21 +1980,21 @@
err = -ENOMEM;
info = neo_alloc_fb_info(dev, id);
if (!info)
- goto failed;
+ return err;

err = neo_map_mmio(info, dev);
if (err)
- goto failed;
+ goto err_map_mmio;

video_len = neo_init_hw(info);
if (video_len < 0) {
err = video_len;
- goto failed;
+ goto err_init_hw;
}

err = neo_map_video(info, dev, video_len); if (err)
- goto failed;
+ goto err_init_hw;

/*
* Calculate the hsync and vsync frequencies. Note that
@@ -2016,7 +2018,7 @@

err = register_framebuffer(info);
if (err < 0)
- goto failed;
+ goto err_reg_fb;

printk(KERN_INFO "fb%d: %s frame buffer device\n",
info->node, info->fix.id);
@@ -2027,9 +2029,11 @@
pci_set_drvdata(dev, info);
return 0;

- failed:
+ err_reg_fb:
neo_unmap_video(info);
+ err_init_hw:
neo_unmap_mmio(info);
+ err_map_mmio:
neo_free_fb_info(info);

return err;

_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors