Re: [PATCH] fb: Remove use of lock_kernel / unlock_kernel in fbmem

[Andres Salomon <dilinger@queued.net>]

On Fri, 18 Apr 2008 21:54:43 -0300
Thiago Galesi <thiagogalesi@gmail.com> wrote:

> Hello
> 
> 
> >Just a minor nit; in general, I'd think you would want to completely
> >initialize the structure (including calling mutex_init) before
> >registering the structure with the rest of the system (in this case,
> >with registered_fb and *_call_notifier_chain).  Initializing after
> >registration is just asking for a race conditions.
> 
> This is better?? 

That does look better, yes.  Someone else who is familiar w/ the locking
semantics there would have to comment on the rest of it.



> 
> 
> ---
> This patch removes lock_kernel(), unlock_kernel() usage in fbmem.c and replaces it with a mutex
> 
> Signed-off-by: Thiago Galesi <thiagogalesi@gmail.com>
> 
> ---
> 
> 
> Index: linux-2.6.23.1/drivers/video/fbmem.c
> ===================================================================
> --- linux-2.6.23.1.orig/drivers/video/fbmem.c	2008-04-16 18:52:32.000000000 -0300
> +++ linux-2.6.23.1/drivers/video/fbmem.c	2008-04-18 19:25:53.000000000 -0300
> @@ -987,7 +987,7 @@
>  }
>  
>  static int 
> -fb_ioctl(struct inode *inode, struct file *file, unsigned int cmd,
> +__fb_ioctl(struct inode *inode, struct file *file, unsigned int cmd,
>  	 unsigned long arg)
>  {
>  	int fbidx = iminor(inode);
> @@ -1085,6 +1085,28 @@
>  	}
>  }
>  
> +static int
> +fb_ioctl(struct inode *inode, struct file *file, unsigned int cmd,
> +	 unsigned long arg)
> +{
> +	int fbidx = iminor(inode);
> +	struct fb_info *info = registered_fb[fbidx];
> +	int ret;
> +
> +	mutex_lock(&info->hwlock);
> +	ret = __fb_ioctl(inode, file, cmd, arg);
> +	mutex_unlock(&info->hwlock);
> +	return ret;
> +}
> +
> +static long
> +fb_unlocked_ioctl(struct file *file, unsigned int cmd,
> +	 unsigned long arg)
> +{
> +	struct inode *inode = file->f_path.dentry->d_inode;
> +	return fb_ioctl(inode, file, cmd, arg);
> +}
> +
>  #ifdef CONFIG_COMPAT
>  struct fb_fix_screeninfo32 {
>  	char			id[16];
> @@ -1136,7 +1158,7 @@
>  	    put_user(compat_ptr(data), &cmap->transp))
>  		return -EFAULT;
>  
> -	err = fb_ioctl(inode, file, cmd, (unsigned long) cmap);
> +	err = __fb_ioctl(inode, file, cmd, (unsigned long) cmap);
>  
>  	if (!err) {
>  		if (copy_in_user(&cmap32->start,
> @@ -1190,7 +1212,7 @@
>  
>  	old_fs = get_fs();
>  	set_fs(KERNEL_DS);
> -	err = fb_ioctl(inode, file, cmd, (unsigned long) &fix);
> +	err = __fb_ioctl(inode, file, cmd, (unsigned long) &fix);
>  	set_fs(old_fs);
>  
>  	if (!err)
> @@ -1208,7 +1230,7 @@
>  	struct fb_ops *fb = info->fbops;
>  	long ret = -ENOIOCTLCMD;
>  
> -	lock_kernel();
> +	mutex_lock(&info->hwlock);
>  	switch(cmd) {
>  	case FBIOGET_VSCREENINFO:
>  	case FBIOPUT_VSCREENINFO:
> @@ -1217,7 +1239,7 @@
>  	case FBIOPUT_CON2FBMAP:
>  		arg = (unsigned long) compat_ptr(arg);
>  	case FBIOBLANK:
> -		ret = fb_ioctl(inode, file, cmd, arg);
> +		ret = __fb_ioctl(inode, file, cmd, arg);
>  		break;
>  
>  	case FBIOGET_FSCREENINFO:
> @@ -1234,7 +1256,7 @@
>  			ret = fb->fb_compat_ioctl(info, cmd, arg);
>  		break;
>  	}
> -	unlock_kernel();
> +	mutex_unlock(&info->hwlock);
>  	return ret;
>  }
>  #endif
> @@ -1256,13 +1278,13 @@
>  		return -ENODEV;
>  	if (fb->fb_mmap) {
>  		int res;
> -		lock_kernel();
> +		mutex_lock(&info->hwlock);
>  		res = fb->fb_mmap(info, vma);
> -		unlock_kernel();
> +		mutex_unlock(&info->hwlock);
>  		return res;
>  	}
>  
> -	lock_kernel();
> +	mutex_lock(&info->hwlock);
>  
>  	/* frame buffer memory */
>  	start = info->fix.smem_start;
> @@ -1271,13 +1293,13 @@
>  		/* memory mapped io */
>  		off -= len;
>  		if (info->var.accel_flags) {
> -			unlock_kernel();
> +			mutex_unlock(&info->hwlock);
>  			return -EINVAL;
>  		}
>  		start = info->fix.mmio_start;
>  		len = PAGE_ALIGN((start & ~PAGE_MASK) + info->fix.mmio_len);
>  	}
> -	unlock_kernel();
> +	mutex_unlock(&info->hwlock);
>  	start &= PAGE_MASK;
>  	if ((vma->vm_end - vma->vm_start + off) > len)
>  		return -EINVAL;
> @@ -1323,25 +1345,25 @@
>  {
>  	struct fb_info * const info = file->private_data;
>  
> -	lock_kernel();
> +	mutex_lock(&info->hwlock);
>  	if (info->fbops->fb_release)
>  		info->fbops->fb_release(info,1);
>  	module_put(info->fbops->owner);
> -	unlock_kernel();
> +	mutex_unlock(&info->hwlock);
>  	return 0;
>  }
>  
>  static const struct file_operations fb_fops = {
> -	.owner =	THIS_MODULE,
> -	.read =		fb_read,
> -	.write =	fb_write,
> -	.ioctl =	fb_ioctl,
> +	.owner =		THIS_MODULE,
> +	.read =			fb_read,
> +	.write =		fb_write,
> +	.unlocked_ioctl =	fb_unlocked_ioctl,
>  #ifdef CONFIG_COMPAT
> -	.compat_ioctl = fb_compat_ioctl,
> +	.compat_ioctl = 	fb_compat_ioctl,
>  #endif
> -	.mmap =		fb_mmap,
> -	.open =		fb_open,
> -	.release =	fb_release,
> +	.mmap =			fb_mmap,
> +	.open =			fb_open,
> +	.release =		fb_release,
>  #ifdef HAVE_ARCH_FB_UNMAPPED_AREA
>  	.get_unmapped_area = get_fb_unmapped_area,
>  #endif
> @@ -1377,6 +1399,7 @@
>  			break;
>  	fb_info->node = i;
>  
> +	mutex_init(&fb_info->hwlock);
>  	fb_info->dev = device_create(fb_class, fb_info->device,
>  				     MKDEV(FB_MAJOR, i), "fb%d", i);
>  	if (IS_ERR(fb_info->dev)) {
> @@ -1413,6 +1436,7 @@
>  
>  	event.info = fb_info;
>  	fb_notifier_call_chain(FB_EVENT_FB_REGISTERED, &event);
> +
>  	return 0;
>  }
>  
> @@ -1464,6 +1488,7 @@
>  	device_destroy(fb_class, MKDEV(FB_MAJOR, i));
>  	event.info = fb_info;
>  	fb_notifier_call_chain(FB_EVENT_FB_UNREGISTERED, &event);
> +	mutex_destroy(&fb_info.hwlock);
>  done:
>  	return ret;
>  }
> Index: linux-2.6.23.1/include/linux/fb.h
> ===================================================================
> --- linux-2.6.23.1.orig/include/linux/fb.h	2008-04-16 18:52:32.000000000 -0300
> +++ linux-2.6.23.1/include/linux/fb.h	2008-04-16 18:53:22.000000000 -0300
> @@ -802,6 +802,7 @@
>  	struct list_head modelist;      /* mode list */
>  	struct fb_videomode *mode;	/* current mode */
>  
> +	struct mutex hwlock;		/* mutex for protecting hw ops */
>  #ifdef CONFIG_FB_BACKLIGHT
>  	/* assigned backlight device */
>  	/* set before framebuffer registration, 
> 
> 
> 


-- 
Need a kernel or Debian developer?  Contact me, I'm looking for contracts.

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone