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

This is a discussion on [PATCH] fb: Remove use of lock_kernel / unlock_kernel in fbmem - Kernel ; Hello This patch removes lock_kernel(), unlock_kernel() usage in fbmem.c and replaces it with a mutex Signed-off-by: Thiago Galesi --- Index: linux-2.6.23.1/drivers/video/fbmem.c ================================================== ================= --- linux-2.6.23.1.orig/drivers/video/fbmem.c 2007-10-12 13:43:44.000000000 -0300 +++ linux-2.6.23.1/drivers/video/fbmem.c 2008-04-12 15:08:04.000000000 -0300 @@ -1208,7 +1208,7 @@ struct fb_ops *fb ...

+ Reply to Thread
Results 1 to 5 of 5

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

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

    Hello

    This patch removes lock_kernel(), unlock_kernel() usage in fbmem.c and replaces it with a mutex

    Signed-off-by: Thiago Galesi

    ---

    Index: linux-2.6.23.1/drivers/video/fbmem.c
    ================================================== =================
    --- linux-2.6.23.1.orig/drivers/video/fbmem.c 2007-10-12 13:43:44.000000000 -0300
    +++ linux-2.6.23.1/drivers/video/fbmem.c 2008-04-12 15:08:04.000000000 -0300
    @@ -1208,7 +1208,7 @@
    struct fb_ops *fb = info->fbops;
    long ret = -ENOIOCTLCMD;

    - lock_kernel();
    + mutex_lock(&info->hwlock);
    switch(cmd) {
    case FBIOGET_VSCREENINFO:
    case FBIOPUT_VSCREENINFO:
    @@ -1234,7 +1234,7 @@
    ret = fb->fb_compat_ioctl(info, cmd, arg);
    break;
    }
    - unlock_kernel();
    + mutex_unlock(&info->hwlock);
    return ret;
    }
    #endif
    @@ -1256,13 +1256,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 +1271,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,11 +1323,11 @@
    {
    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;
    }

    @@ -1413,6 +1413,8 @@

    event.info = fb_info;
    fb_notifier_call_chain(FB_EVENT_FB_REGISTERED, &event);
    +
    + mutex_init(&fb_info->hwlock);
    return 0;
    }

    @@ -1464,6 +1466,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 2007-10-12 13:43:44.000000000 -0300
    +++ linux-2.6.23.1/include/linux/fb.h 2008-04-12 15:06:41.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,

    --
    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/

  2. Re: [Linux-fbdev-devel] [PATCH] fb: Remove use of lock_kernel / unlock_kernel in fbmem

    On Sun, 13 Apr 2008 11:50:07 -0300 Thiago Galesi wrote:

    > This patch removes lock_kernel(), unlock_kernel() usage in fbmem.c and replaces it with a mutex


    It isn't that simple, alas.

    vfs_ioctl() runs lock_kernel() prior to calling fb_ioctl(), so the
    lock_kernel()s in fb_compat_ioctl() are actually providing exclusion against
    fb_ioctl(). Your patch would break that.

    A suitable fix might be to do

    __fb_ioctl(...)
    {

    }

    fb_ioctl(...)
    {
    mutex_lock(&info->hwlock);
    __fb_ioctl(...);
    mutex_unlock(&info->hwlock);
    }

    and then change fb_compat_ioctl() to call __fb_ioctl(). All the other
    callers of fb_ioctl() would need to be reviewed - see if they need to take
    the mutex then call __fb_ioctl(), or they might be OK as they are, calling
    fb_ioctl().

    Then we can switch fb_fops over to

    .ioctl = NULL,
    .unlocked_ioctl = fb_ioctl,


    --
    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/

  3. Re: [Linux-fbdev-devel] [PATCH] fb: Remove use of lock_kernel / unlock_kernel in fbmem





    This patch removes lock_kernel(), unlock_kernel() usage in fbmem.c and replaces it with a mutex

    Signed-off-by: Thiago Galesi

    ---

    Ok, 2nd try

    The only thing I don't like is the name of fb_ioctl, but I don't have a better idea, so...


    ---

    Index: linux-2.6.23.1/drivers/video/fbmem.c
    ================================================== =================
    --- linux-2.6.23.1.orig/drivers/video/fbmem.c
    +++ linux-2.6.23.1/drivers/video/fbmem.c
    @@ -987,7 +987,7 @@ fb_blank(struct fb_info *info, int blank
    }

    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 @@ fb_ioctl(struct inode *inode, struct fil
    }
    }

    +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 @@ static int fb_getput_cmap(struct inode *
    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 @@ static int fb_get_fscreeninfo(struct ino

    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 @@ fb_compat_ioctl(struct file *file, unsig
    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 @@ fb_compat_ioctl(struct file *file, unsig
    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 @@ fb_compat_ioctl(struct file *file, unsig
    ret = fb->fb_compat_ioctl(info, cmd, arg);
    break;
    }
    - unlock_kernel();
    + mutex_unlock(&info->hwlock);
    return ret;
    }
    #endif
    @@ -1256,13 +1278,13 @@ fb_mmap(struct file *file, struct vm_are
    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 @@ fb_mmap(struct file *file, struct vm_are
    /* 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 @@ fb_release(struct inode *inode, struct f
    {
    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
    @@ -1413,6 +1435,8 @@ register_framebuffer(struct fb_info *fb_

    event.info = fb_info;
    fb_notifier_call_chain(FB_EVENT_FB_REGISTERED, &event);
    +
    + mutex_init(&fb_info->hwlock);
    return 0;
    }

    @@ -1464,6 +1488,7 @@ unregister_framebuffer(struct fb_info *f
    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
    +++ linux-2.6.23.1/include/linux/fb.h
    @@ -802,6 +802,7 @@ struct fb_info {
    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,






    --
    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/

  4. Re: [Linux-fbdev-devel] [PATCH] fb: Remove use of lock_kernel / unlock_kernel in fbmem

    On Wed, 16 Apr 2008 20:00:59 -0300
    Thiago Galesi wrote:

    >
    > This patch removes lock_kernel(), unlock_kernel() usage in fbmem.c and replaces it with a mutex
    >
    > Signed-off-by: Thiago Galesi
    >

    [...]
    > @@ -1413,6 +1435,8 @@ register_framebuffer(struct fb_info *fb_
    >
    > event.info = fb_info;
    > fb_notifier_call_chain(FB_EVENT_FB_REGISTERED, &event);
    > +
    > + mutex_init(&fb_info->hwlock);
    > return 0;
    > }
    >


    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.



    --
    Need a kernel or Debian developer? Contact me, I'm looking for contracts.
    --
    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/

  5. Re: [Linux-fbdev-devel] [PATCH] fb: Remove use of lock_kernel / unlock_kernel in fbmem

    On Fri, 18 Apr 2008 21:54:43 -0300
    Thiago Galesi 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
    >
    > ---
    >
    >
    > 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.
    --
    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/

+ Reply to Thread