[Kernel-janitors] [PATCH] drivers/video/fbcmap.c kmalloc audit

[Kernel-janitors] [PATCH] drivers/video/fbcmap.c kmalloc audit

[Leann Ogasawara]

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
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

[Kernel-janitors] [PATCH] drivers/video/vga16fb.c ioremap() and

[Leann Ogasawara]

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
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [Kernel-janitors] [PATCH] drivers/video/fbcmap.c kmalloc audit

[Arnaldo Carvalho de Melo]

Em Tue, Jan 27, 2004 at 01:37:01PM -0800, Leann Ogasawara escreveu:
> 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;

Looks good to me.

- Arnaldo
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [Kernel-janitors] [PATCH] drivers/video/fbcmap.c kmalloc audit

[Daniele Bellucci]

|+ 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.




-- 

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
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [Kernel-janitors] [PATCH] drivers/video/fbcmap.c kmalloc audit

[Randy.Dunlap]

On Tue, 27 Jan 2004 22:54:19 +0100 Daniele Bellucci <bellucda@tiscali.it> 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
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [Kernel-janitors] [PATCH] drivers/video/fbcmap.c kmalloc audit

[Daniele Bellucci]

On Tue, Jan 27, 2004 at 02:05:18PM -0800, Randy.Dunlap wrote:
|On Tue, 27 Jan 2004 22:54:19 +0100 Daniele Bellucci <bellucda@tiscali.it> 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.


-- 

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
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

[Kernel-janitors] [PATCH] drivers/video/radeonfb.c fb_alloc_cmap()

[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
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

[Kernel-janitors] [PATCH] drivers/video/cyber2000fb.c

[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
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

[Kernel-janitors] [PATCH] drivers/video/vfb.c fb_alloc_cmap() audit

[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
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

[Kernel-janitors] [PATCH] drivers/video/vesafb.c fb_alloc_cmap()

[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
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

[Kernel-janitors] [PATCH] drivers/video/neofb.c fb_alloc_cmap()

[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
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

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

[Domen Puncer]

On Friday 06 of February 2004 23:31, Leann Ogasawara wrote:
> Hi all,
>
> Patch audits fb_alloc_cmap() and also incorporates an error path.
> Feedback appreciated.  Thanks,
>
...
> @@ -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.

...

>  	/* map the regions */
>  	rinfo->mmio_base = (unsigned long) ioremap (rinfo->mmio_base_phys,
> RADEON_REGSIZE);

Turn off wordwrap.


	Domen

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

[Kernel-janitors] [PATCH] drivers/video/pm2fb.c MIN/MAX removal

[Michael Veeck]

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

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!

[-- Attachment #2: minmax_drivers_video.patch --]
[-- Type: text/plain, Size: 858 bytes --]

--- linux-2.6.2.org/drivers/video/pm2fb.c	2004-02-04 04:45:07.000000000 +0100
+++ linux-2.6.2.new/drivers/video/pm2fb.c	2004-02-06 14:16:50.000000000 +0100
@@ -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);
 	}
 }

[-- Attachment #3: Type: text/plain, Size: 163 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

[Kernel-janitors] [PATCH] drivers/video/neofb.patch update

[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
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors