[PATCH] add mutex to fbdev for fb_mmap locking

[PATCH] add mutex to fbdev for fb_mmap locking

[Krzysztof Helt]

From: Krzysztof Helt <krzysztof.h1@wp.pl>

Add a mutex to avoid a circular locking problem between the mm layer semaphore 
and fbdev ioctl mutex through the fb_mmap() call.

Also, add mutex to all places where smem_start and smem_len fields change
so the mutex inside the fb_mmap() is actually used. Changing of these fields
before calling the framebuffer_register() are not mutexed.

Signed-off-by: Krzysztof Helt <krzysztof.h1@wp.pl>
---
This is the promised patch.

Please decide if this is 2.6.30 material or not. It removes one lockdep (fb_mmap()
and register_framebuffer()) but there is still another one (fb_release() and register_framebuffer()).
It also cleans up handling of the smem_start and smem_len fields used by mutexed 
section of the fb_mmap().

Kind regards,
Krzysztof

diff --git a/drivers/video/atafb.c b/drivers/video/atafb.c
index 018850c..c6f43c3 100644
--- a/drivers/video/atafb.c
+++ b/drivers/video/atafb.c
@@ -2406,6 +2406,8 @@ static int do_fb_set_var(struct fb_var_screeninfo *var, int isactive)
 }
 
 static int atafb_get_fix(struct fb_fix_screeninfo *fix, struct fb_info *info)
+__acquires(&info->mm_lock)
+__releases(&info->mm_lock)
 {
 	struct atafb_par par;
 	int err;
@@ -2414,7 +2416,10 @@ static int atafb_get_fix(struct fb_fix_screeninfo *fix, struct fb_info *info)
 	if (err)
 		return err;
 	memset(fix, 0, sizeof(struct fb_fix_screeninfo));
-	return fbhw->encode_fix(fix, &par);
+	mutex_lock(&info->mm_lock);
+	err = fbhw->encode_fix(fix, &par);
+	mutex_unlock(&info->mm_lock);
+	return err;
 }
 
 static int atafb_get_var(struct fb_var_screeninfo *var, struct fb_info *info)
@@ -2738,12 +2743,16 @@ static int atafb_check_var(struct fb_var_screeninfo *var, struct fb_info *info)
 	/* actually set hw par by decoding var, then setting hardware from
 	 * hw par just decoded */
 static int atafb_set_par(struct fb_info *info)
+__acquires(&info->mm_lock)
+__releases(&info->mm_lock)
 {
 	struct atafb_par *par = (struct atafb_par *)info->par;
 
 	/* Decode wanted screen parameters */
 	fbhw->decode_var(&info->var, par);
+	mutex_lock(&info->mm_lock);
 	fbhw->encode_fix(&info->fix, par);
+	mutex_unlock(&info->mm_lock);
 
 	/* Set new videomode */
 	ata_set_par(par);
diff --git a/drivers/video/atmel_lcdfb.c b/drivers/video/atmel_lcdfb.c
index 2fb63f6..40eb1b4 100644
--- a/drivers/video/atmel_lcdfb.c
+++ b/drivers/video/atmel_lcdfb.c
@@ -263,6 +263,8 @@ static inline void atmel_lcdfb_free_video_memory(struct atmel_lcdfb_info *sinfo)
  *	@sinfo: the frame buffer to allocate memory for
  */
 static int atmel_lcdfb_alloc_video_memory(struct atmel_lcdfb_info *sinfo)
+__acquires(&info->mm_lock)
+__releases(&info->mm_lock)
 {
 	struct fb_info *info = sinfo->info;
 	struct fb_var_screeninfo *var = &info->var;
@@ -270,7 +272,9 @@ static int atmel_lcdfb_alloc_video_memory(struct atmel_lcdfb_info *sinfo)
 
 	smem_len = (var->xres_virtual * var->yres_virtual
 		    * ((var->bits_per_pixel + 7) / 8));
+	mutex_lock(&info->mm_lock);
 	info->fix.smem_len = max(smem_len, sinfo->smem_len);
+	mutex_unlock(&info->mm_lock);
 
 	info->screen_base = dma_alloc_writecombine(info->device, info->fix.smem_len,
 					(dma_addr_t *)&info->fix.smem_start, GFP_KERNEL);
diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
index d412a1d..c173683 100644
--- a/drivers/video/fbmem.c
+++ b/drivers/video/fbmem.c
@@ -1310,8 +1310,8 @@ static long fb_compat_ioctl(struct file *file, unsigned int cmd,
 
 static int
 fb_mmap(struct file *file, struct vm_area_struct * vma)
-__acquires(&info->lock)
-__releases(&info->lock)
+__acquires(&info->mm_lock)
+__releases(&info->mm_lock)
 {
 	int fbidx = iminor(file->f_path.dentry->d_inode);
 	struct fb_info *info = registered_fb[fbidx];
@@ -1325,16 +1325,14 @@ __releases(&info->lock)
 	off = vma->vm_pgoff << PAGE_SHIFT;
 	if (!fb)
 		return -ENODEV;
+	mutex_lock(&info->mm_lock);
 	if (fb->fb_mmap) {
 		int res;
-		mutex_lock(&info->lock);
 		res = fb->fb_mmap(info, vma);
-		mutex_unlock(&info->lock);
+		mutex_unlock(&info->mm_lock);
 		return res;
 	}
 
-	mutex_lock(&info->lock);
-
 	/* frame buffer memory */
 	start = info->fix.smem_start;
 	len = PAGE_ALIGN((start & ~PAGE_MASK) + info->fix.smem_len);
@@ -1342,13 +1340,13 @@ __releases(&info->lock)
 		/* memory mapped io */
 		off -= len;
 		if (info->var.accel_flags) {
-			mutex_unlock(&info->lock);
+			mutex_unlock(&info->mm_lock);
 			return -EINVAL;
 		}
 		start = info->fix.mmio_start;
 		len = PAGE_ALIGN((start & ~PAGE_MASK) + info->fix.mmio_len);
 	}
-	mutex_unlock(&info->lock);
+	mutex_unlock(&info->mm_lock);
 	start &= PAGE_MASK;
 	if ((vma->vm_end - vma->vm_start + off) > len)
 		return -EINVAL;
@@ -1491,6 +1489,7 @@ register_framebuffer(struct fb_info *fb_info)
 			break;
 	fb_info->node = i;
 	mutex_init(&fb_info->lock);
+	mutex_init(&fb_info->mm_lock);
 
 	fb_info->dev = device_create(fb_class, fb_info->device,
 				     MKDEV(FB_MAJOR, i), NULL, "fb%d", i);
diff --git a/drivers/video/fsl-diu-fb.c b/drivers/video/fsl-diu-fb.c
index f153c58..ca664f5 100644
--- a/drivers/video/fsl-diu-fb.c
+++ b/drivers/video/fsl-diu-fb.c
@@ -748,37 +748,45 @@ static void update_lcdc(struct fb_info *info)
 }
 
 static int map_video_memory(struct fb_info *info)
+__acquires(&info->mm_lock)
+__releases(&info->mm_lock)
 {
 	phys_addr_t phys;
+	u32 smem_len = info->fix.line_length * info->var.yres_virtual;
 
 	pr_debug("info->var.xres_virtual = %d\n", info->var.xres_virtual);
 	pr_debug("info->var.yres_virtual = %d\n", info->var.yres_virtual);
 	pr_debug("info->fix.line_length  = %d\n", info->fix.line_length);
+	pr_debug("MAP_VIDEO_MEMORY: smem_len = %u\n", smem_len);
 
-	info->fix.smem_len = info->fix.line_length * info->var.yres_virtual;
-	pr_debug("MAP_VIDEO_MEMORY: smem_len = %d\n", info->fix.smem_len);
-	info->screen_base = fsl_diu_alloc(info->fix.smem_len, &phys);
+	info->screen_base = fsl_diu_alloc(smem_len, &phys);
 	if (info->screen_base == NULL) {
 		printk(KERN_ERR "Unable to allocate fb memory\n");
 		return -ENOMEM;
 	}
+	mutex_lock(&info->mm_lock);
 	info->fix.smem_start = (unsigned long) phys;
+	info->fix.smem_len = smem_len;
+	mutex_unlock(&info->mm_lock);
 	info->screen_size = info->fix.smem_len;
 
 	pr_debug("Allocated fb @ paddr=0x%08lx, size=%d.\n",
-				info->fix.smem_start,
-		info->fix.smem_len);
+		 info->fix.smem_start, info->fix.smem_len);
 	pr_debug("screen base %p\n", info->screen_base);
 
 	return 0;
 }
 
 static void unmap_video_memory(struct fb_info *info)
+__acquires(&info->mm_lock)
+__releases(&info->mm_lock)
 {
 	fsl_diu_free(info->screen_base, info->fix.smem_len);
+	mutex_lock(&info->mm_lock);
 	info->screen_base = NULL;
 	info->fix.smem_start = 0;
 	info->fix.smem_len = 0;
+	mutex_unlock(&info->mm_lock);
 }
 
 /*
diff --git a/drivers/video/i810/i810_main.c b/drivers/video/i810/i810_main.c
index 2e94019..fac172d 100644
--- a/drivers/video/i810/i810_main.c
+++ b/drivers/video/i810/i810_main.c
@@ -1084,14 +1084,18 @@ static int i810_check_params(struct fb_var_screeninfo *var,
  * This will set up parameters that are unmodifiable by the user.
  */
 static int encode_fix(struct fb_fix_screeninfo *fix, struct fb_info *info)
+__acquires(&info->mm_lock)
+__releases(&info->mm_lock)
 {
 	struct i810fb_par *par = info->par;
 
     	memset(fix, 0, sizeof(struct fb_fix_screeninfo));
 
     	strcpy(fix->id, "I810");
+	mutex_lock(&info->mm_lock);
     	fix->smem_start = par->fb.physical;
     	fix->smem_len = par->fb.size;
+	mutex_unlock(&info->mm_lock);
     	fix->type = FB_TYPE_PACKED_PIXELS;
     	fix->type_aux = 0;
 	fix->xpanstep = 8;
diff --git a/drivers/video/matrox/matroxfb_base.c b/drivers/video/matrox/matroxfb_base.c
index 8e7a275..b6bf28f 100644
--- a/drivers/video/matrox/matroxfb_base.c
+++ b/drivers/video/matrox/matroxfb_base.c
@@ -720,12 +720,16 @@ static void matroxfb_init_fix(WPMINFO2)
 }
 
 static void matroxfb_update_fix(WPMINFO2)
+__acquires(&info->mm_lock)
+__releases(&info->mm_lock)
 {
 	struct fb_fix_screeninfo *fix = &ACCESS_FBINFO(fbcon).fix;
 	DBG(__func__)
 
+	mutex_lock(&ACCESS_FBINFO(fbcon).mm_lock);
 	fix->smem_start = ACCESS_FBINFO(video.base) + ACCESS_FBINFO(curr.ydstorg.bytes);
 	fix->smem_len = ACCESS_FBINFO(video.len_usable) - ACCESS_FBINFO(curr.ydstorg.bytes);
+	mutex_unlock(&ACCESS_FBINFO(fbcon).mm_lock);
 }
 
 static int matroxfb_check_var(struct fb_var_screeninfo *var, struct fb_info *info)
@@ -2081,6 +2085,7 @@ static int matroxfb_probe(struct pci_dev* pdev, const struct pci_device_id* dumm
 	spin_lock_init(&ACCESS_FBINFO(lock.accel));
 	init_rwsem(&ACCESS_FBINFO(crtc2.lock));
 	init_rwsem(&ACCESS_FBINFO(altout.lock));
+	mutex_init(&ACCESS_FBINFO(fbcon).mm_lock);
 	ACCESS_FBINFO(irq_flags) = 0;
 	init_waitqueue_head(&ACCESS_FBINFO(crtc1.vsync.wait));
 	init_waitqueue_head(&ACCESS_FBINFO(crtc2.vsync.wait));
diff --git a/drivers/video/matrox/matroxfb_crtc2.c b/drivers/video/matrox/matroxfb_crtc2.c
index 7ac4c5f..7ad14d0 100644
--- a/drivers/video/matrox/matroxfb_crtc2.c
+++ b/drivers/video/matrox/matroxfb_crtc2.c
@@ -289,13 +289,18 @@ static int matroxfb_dh_release(struct fb_info* info, int user) {
 #undef m2info
 }
 
-static void matroxfb_dh_init_fix(struct matroxfb_dh_fb_info *m2info) {
+static void matroxfb_dh_init_fix(struct matroxfb_dh_fb_info *m2info)
+__acquires(&info->mm_lock)
+__releases(&info->mm_lock)
+{
 	struct fb_fix_screeninfo *fix = &m2info->fbcon.fix;
 
 	strcpy(fix->id, "MATROX DH");
 
+	mutex_lock(&m2info->fbcon.mm_lock);
 	fix->smem_start = m2info->video.base;
 	fix->smem_len = m2info->video.len_usable;
+	mutex_unlock(&m2info->fbcon.mm_lock);
 	fix->ypanstep = 1;
 	fix->ywrapstep = 0;
 	fix->xpanstep = 8;	/* TBD */
diff --git a/drivers/video/mx3fb.c b/drivers/video/mx3fb.c
index 9894de1..fb57795 100644
--- a/drivers/video/mx3fb.c
+++ b/drivers/video/mx3fb.c
@@ -669,7 +669,7 @@ static uint32_t bpp_to_pixfmt(int bpp)
 }
 
 static int mx3fb_blank(int blank, struct fb_info *fbi);
-static int mx3fb_map_video_memory(struct fb_info *fbi);
+static int mx3fb_map_video_memory(struct fb_info *fbi, unsigned int mem_len);
 static int mx3fb_unmap_video_memory(struct fb_info *fbi);
 
 /**
@@ -742,8 +742,7 @@ static int mx3fb_set_par(struct fb_info *fbi)
 		if (fbi->fix.smem_start)
 			mx3fb_unmap_video_memory(fbi);
 
-		fbi->fix.smem_len = mem_len;
-		if (mx3fb_map_video_memory(fbi) < 0) {
+		if (mx3fb_map_video_memory(fbi, mem_len) < 0) {
 			mutex_unlock(&mx3_fbi->mutex);
 			return -ENOMEM;
 		}
@@ -1198,6 +1197,7 @@ static int mx3fb_resume(struct platform_device *pdev)
 /**
  * mx3fb_map_video_memory() - allocates the DRAM memory for the frame buffer.
  * @fbi:	framebuffer information pointer
+ * @mem_len:	length of mapped memory
  * @return:	Error code indicating success or failure
  *
  * This buffer is remapped into a non-cached, non-buffered, memory region to
@@ -1205,23 +1205,28 @@ static int mx3fb_resume(struct platform_device *pdev)
  * area is remapped, all virtual memory access to the video memory should occur
  * at the new region.
  */
-static int mx3fb_map_video_memory(struct fb_info *fbi)
+static int mx3fb_map_video_memory(struct fb_info *fbi, unsigned int mem_len)
+__acquires(&info->mm_lock)
+__releases(&info->mm_lock)
 {
 	int retval = 0;
 	dma_addr_t addr;
 
 	fbi->screen_base = dma_alloc_writecombine(fbi->device,
-						  fbi->fix.smem_len,
+						  mem_len,
 						  &addr, GFP_DMA);
 
 	if (!fbi->screen_base) {
 		dev_err(fbi->device, "Cannot allocate %u bytes framebuffer memory\n",
-			fbi->fix.smem_len);
+			mem_len);
 		retval = -EBUSY;
 		goto err0;
 	}
 
+	mutex_lock(&fbi->mm_lock);
 	fbi->fix.smem_start = addr;
+	fbi->fix.smem_len = mem_len;
+	mutex_unlock(&fbi->mm_lock);
 
 	dev_dbg(fbi->device, "allocated fb @ p=0x%08x, v=0x%p, size=%d.\n",
 		(uint32_t) fbi->fix.smem_start, fbi->screen_base, fbi->fix.smem_len);
@@ -1246,13 +1251,17 @@ err0:
  * @return:	error code indicating success or failure
  */
 static int mx3fb_unmap_video_memory(struct fb_info *fbi)
+__acquires(&info->mm_lock)
+__releases(&info->mm_lock)
 {
 	dma_free_writecombine(fbi->device, fbi->fix.smem_len,
 			      fbi->screen_base, fbi->fix.smem_start);
 
 	fbi->screen_base = 0;
+	mutex_lock(&fbi->mm_lock);
 	fbi->fix.smem_start = 0;
 	fbi->fix.smem_len = 0;
+	mutex_unlock(&fbi->mm_lock);
 	return 0;
 }
 
diff --git a/drivers/video/omap/omapfb_main.c b/drivers/video/omap/omapfb_main.c
index 060d72f..eccf4bd 100644
--- a/drivers/video/omap/omapfb_main.c
+++ b/drivers/video/omap/omapfb_main.c
@@ -384,6 +384,8 @@ static void omapfb_sync(struct fb_info *fbi)
  * When calling this fb_info.var must be set up already.
  */
 static void set_fb_fix(struct fb_info *fbi)
+__acquires(&info->mm_lock)
+__releases(&info->mm_lock)
 {
 	struct fb_fix_screeninfo *fix = &fbi->fix;
 	struct fb_var_screeninfo *var = &fbi->var;
@@ -393,8 +395,10 @@ static void set_fb_fix(struct fb_info *fbi)
 
 	rg = &plane->fbdev->mem_desc.region[plane->idx];
 	fbi->screen_base	= rg->vaddr;
+	mutex_lock(&fbi->mm_lock);
 	fix->smem_start		= rg->paddr;
 	fix->smem_len		= rg->size;
+	mutex_unlock(&fbi->mm_lock);
 
 	fix->type = FB_TYPE_PACKED_PIXELS;
 	bpp = var->bits_per_pixel;
@@ -824,6 +828,8 @@ static int omapfb_query_plane(struct fb_info *fbi, struct omapfb_plane_info *pi)
 }
 
 static int omapfb_setup_mem(struct fb_info *fbi, struct omapfb_mem_info *mi)
+__acquires(&info->mm_lock)
+__releases(&info->mm_lock)
 {
 	struct omapfb_plane_struct *plane = fbi->par;
 	struct omapfb_device *fbdev = plane->fbdev;
@@ -886,8 +892,10 @@ static int omapfb_setup_mem(struct fb_info *fbi, struct omapfb_mem_info *mi)
 				 * plane memory is dealloce'd, the other
 				 * screen parameters in var / fix are invalid.
 				 */
+				mutex_lock(&fbi->mm_lock);
 				fbi->fix.smem_start = 0;
 				fbi->fix.smem_len = 0;
+				mutex_unlock(&fbi->mm_lock);
 			}
 		}
 	}
diff --git a/drivers/video/platinumfb.c b/drivers/video/platinumfb.c
index 03b3670..1600ee7 100644
--- a/drivers/video/platinumfb.c
+++ b/drivers/video/platinumfb.c
@@ -122,6 +122,8 @@ static int platinumfb_check_var (struct fb_var_screeninfo *var, struct fb_info *
  * Applies current var to display
  */
 static int platinumfb_set_par (struct fb_info *info)
+__acquires(&info->mm_lock)
+__releases(&info->mm_lock)
 {
 	struct fb_info_platinum *pinfo = info->par;
 	struct platinum_regvals *init;
@@ -141,7 +143,9 @@ static int platinumfb_set_par (struct fb_info *info)
   		offset = 0x10;
 
 	info->screen_base = pinfo->frame_buffer + init->fb_offset + offset;
+	mutex_lock(&info->mm_lock);
 	info->fix.smem_start = (pinfo->frame_buffer_phys) + init->fb_offset + offset;
+	mutex_unlock(&info->mm_lock);
 	info->fix.visual = (pinfo->cmode == CMODE_8) ?
 		FB_VISUAL_PSEUDOCOLOR : FB_VISUAL_DIRECTCOLOR;
  	info->fix.line_length = vmode_attrs[pinfo->vmode-1].hres * (1<<pinfo->cmode)
diff --git a/drivers/video/pxafb.c b/drivers/video/pxafb.c
index 0889d50..140c692 100644
--- a/drivers/video/pxafb.c
+++ b/drivers/video/pxafb.c
@@ -783,6 +783,8 @@ static int overlayfb_check_var(struct fb_var_screeninfo *var,
 }
 
 static int overlayfb_map_video_memory(struct pxafb_layer *ofb)
+__acquires(&info->mm_lock)
+__releases(&info->mm_lock)
 {
 	struct fb_var_screeninfo *var = &ofb->fb.var;
 	int pfor = NONSTD_TO_PFOR(var->nonstd);
@@ -815,8 +817,10 @@ static int overlayfb_map_video_memory(struct pxafb_layer *ofb)
 	ofb->video_mem_phys = virt_to_phys(ofb->video_mem);
 	ofb->video_mem_size = size;
 
+	mutex_lock(&ofb->fb.mm_lock);
 	ofb->fb.fix.smem_start	= ofb->video_mem_phys;
 	ofb->fb.fix.smem_len	= ofb->fb.fix.line_length * var->yres_virtual;
+	mutex_unlock(&ofb->fb.mm_lock);
 	ofb->fb.screen_base	= ofb->video_mem;
 	return 0;
 }
diff --git a/drivers/video/sh7760fb.c b/drivers/video/sh7760fb.c
index 653bdfe..9f6d6e6 100644
--- a/drivers/video/sh7760fb.c
+++ b/drivers/video/sh7760fb.c
@@ -120,18 +120,6 @@ static int sh7760_setcolreg (u_int regno,
 	return 0;
 }
 
-static void encode_fix(struct fb_fix_screeninfo *fix, struct fb_info *info,
-		       unsigned long stride)
-{
-	memset(fix, 0, sizeof(struct fb_fix_screeninfo));
-	strcpy(fix->id, "sh7760-lcdc");
-
-	fix->smem_start = (unsigned long)info->screen_base;
-	fix->smem_len = info->screen_size;
-
-	fix->line_length = stride;
-}
-
 static int sh7760fb_get_color_info(struct device *dev,
 				   u16 lddfr, int *bpp, int *gray)
 {
@@ -334,7 +322,8 @@ static int sh7760fb_set_par(struct fb_info *info)
 
 	iowrite32(ldsarl, par->base + LDSARL);	/* mem for lower half of DSTN */
 
-	encode_fix(&info->fix, info, stride);
+	info->fix.line_length = stride;
+
 	sh7760fb_check_var(&info->var, info);
 
 	sh7760fb_blank(FB_BLANK_UNBLANK, info);	/* panel on! */
@@ -435,6 +424,8 @@ static int sh7760fb_alloc_mem(struct fb_info *info)
 
 	info->screen_base = fbmem;
 	info->screen_size = vram;
+	info->fix.smem_start = (unsigned long)info->screen_base;
+	info->fix.smem_len = info->screen_size;
 
 	return 0;
 }
@@ -520,6 +511,8 @@ static int __devinit sh7760fb_probe(struct platform_device *pdev)
 	info->var.transp.length = 0;
 	info->var.transp.msb_right = 0;
 
+	strcpy(info->fix.id, "sh7760-lcdc");
+
 	/* set the DON2 bit now, before cmap allocation, as it will randomize
 	 * palette memory.
 	 */
diff --git a/drivers/video/sis/sis_main.c b/drivers/video/sis/sis_main.c
index 7e17ee9..59f5faf 100644
--- a/drivers/video/sis/sis_main.c
+++ b/drivers/video/sis/sis_main.c
@@ -1840,6 +1840,8 @@ static int	sisfb_ioctl(struct fb_info *info, unsigned int cmd,
 
 static int
 sisfb_get_fix(struct fb_fix_screeninfo *fix, int con, struct fb_info *info)
+__acquires(&info->mm_lock)
+__releases(&info->mm_lock)
 {
 	struct sis_video_info *ivideo = (struct sis_video_info *)info->par;
 
@@ -1847,8 +1849,10 @@ sisfb_get_fix(struct fb_fix_screeninfo *fix, int con, struct fb_info *info)
 
 	strcpy(fix->id, ivideo->myid);
 
+	mutex_lock(&info->mm_lock);
 	fix->smem_start  = ivideo->video_base + ivideo->video_offset;
 	fix->smem_len    = ivideo->sisfb_mem;
+	mutex_unlock(&info->mm_lock);
 	fix->type        = FB_TYPE_PACKED_PIXELS;
 	fix->type_aux    = 0;
 	fix->visual      = (ivideo->video_bpp == 8) ? FB_VISUAL_PSEUDOCOLOR : FB_VISUAL_TRUECOLOR;
diff --git a/drivers/video/sm501fb.c b/drivers/video/sm501fb.c
index eb5d73a..1285b70 100644
--- a/drivers/video/sm501fb.c
+++ b/drivers/video/sm501fb.c
@@ -145,7 +145,7 @@ static inline void sm501fb_sync_regs(struct sm501fb_info *info)
 #define SM501_MEMF_ACCEL		(8)
 
 static int sm501_alloc_mem(struct sm501fb_info *inf, struct sm501_mem *mem,
-			   unsigned int why, size_t size)
+			   unsigned int why, size_t size, u32 smem_len)
 {
 	struct sm501fb_par *par;
 	struct fb_info *fbi;
@@ -172,7 +172,7 @@ static int sm501_alloc_mem(struct sm501fb_info *inf, struct sm501_mem *mem,
 		if (ptr > 0)
 			ptr &= ~(PAGE_SIZE - 1);
 
-		if (fbi && ptr < fbi->fix.smem_len)
+		if (fbi && ptr < smem_len)
 			return -ENOMEM;
 
 		break;
@@ -197,7 +197,7 @@ static int sm501_alloc_mem(struct sm501fb_info *inf, struct sm501_mem *mem,
 
 	case SM501_MEMF_ACCEL:
 		fbi = inf->fb[HEAD_CRT];
-		ptr = fbi ? fbi->fix.smem_len : 0;
+		ptr = fbi ? smem_len : 0;
 
 		fbi = inf->fb[HEAD_PANEL];
 		if (fbi) {
@@ -405,6 +405,8 @@ static int sm501fb_check_var_pnl(struct fb_var_screeninfo *var,
 
 static int sm501fb_set_par_common(struct fb_info *info,
 				  struct fb_var_screeninfo *var)
+__acquires(&info->mm_lock)
+__releases(&info->mm_lock)
 {
 	struct sm501fb_par  *par = info->par;
 	struct sm501fb_info *fbi = par->info;
@@ -413,6 +415,7 @@ static int sm501fb_set_par_common(struct fb_info *info,
 	unsigned int mem_type;
 	unsigned int clock_type;
 	unsigned int head_addr;
+	unsigned int smem_len;
 
 	dev_dbg(fbi->dev, "%s: %dx%d, bpp = %d, virtual %dx%d\n",
 		__func__, var->xres, var->yres, var->bits_per_pixel,
@@ -453,18 +456,20 @@ static int sm501fb_set_par_common(struct fb_info *info,
 
 	/* allocate fb memory within 501 */
 	info->fix.line_length = (var->xres_virtual * var->bits_per_pixel)/8;
-	info->fix.smem_len    = info->fix.line_length * var->yres_virtual;
+	smem_len = info->fix.line_length * var->yres_virtual;
 
 	dev_dbg(fbi->dev, "%s: line length = %u\n", __func__,
 		info->fix.line_length);
 
-	if (sm501_alloc_mem(fbi, &par->screen, mem_type,
-			    info->fix.smem_len)) {
+	if (sm501_alloc_mem(fbi, &par->screen, mem_type, smem_len, smem_len)) {
 		dev_err(fbi->dev, "no memory available\n");
 		return -ENOMEM;
 	}
 
+	mutex_lock(&info->mm_lock);
 	info->fix.smem_start = fbi->fbmem_res->start + par->screen.sm_addr;
+	info->fix.smem_len   = smem_len;
+	mutex_unlock(&info->mm_lock);
 
 	info->screen_base = fbi->fbmem + par->screen.sm_addr;
 	info->screen_size = info->fix.smem_len;
@@ -637,7 +642,8 @@ static int sm501fb_set_par_crt(struct fb_info *info)
 	if ((control & SM501_DC_CRT_CONTROL_SEL) == 0) {
 		/* the head is displaying panel data... */
 
-		sm501_alloc_mem(fbi, &par->screen, SM501_MEMF_CRT, 0);
+		sm501_alloc_mem(fbi, &par->screen, SM501_MEMF_CRT, 0,
+				info->fix.smem_len);
 		goto out_update;
 	}
 
@@ -1289,7 +1295,8 @@ static int sm501_init_cursor(struct fb_info *fbi, unsigned int reg_base)
 
 	par->cursor_regs = info->regs + reg_base;
 
-	ret = sm501_alloc_mem(info, &par->cursor, SM501_MEMF_CURSOR, 1024);
+	ret = sm501_alloc_mem(info, &par->cursor, SM501_MEMF_CURSOR, 1024,
+			      fbi->fix.smem_len);
 	if (ret < 0)
 		return ret;
 
diff --git a/drivers/video/w100fb.c b/drivers/video/w100fb.c
index d0674f1..05356d3 100644
--- a/drivers/video/w100fb.c
+++ b/drivers/video/w100fb.c
@@ -510,6 +510,8 @@ static int w100fb_check_var(struct fb_var_screeninfo *var, struct fb_info *info)
  *  by looking at the values in info.var
  */
 static int w100fb_set_par(struct fb_info *info)
+__acquires(&info->mm_lock)
+__releases(&info->mm_lock)
 {
 	struct w100fb_par *par=info->par;
 
@@ -523,6 +525,7 @@ static int w100fb_set_par(struct fb_info *info)
 		info->fix.ywrapstep = 0;
 		info->fix.line_length = par->xres * BITS_PER_PIXEL / 8;
 
+		mutex_lock(&info->mm_lock);
 		if ((par->xres*par->yres*BITS_PER_PIXEL/8) > (MEM_INT_SIZE+1)) {
 			par->extmem_active = 1;
 			info->fix.smem_len = par->mach->mem->size+1;
@@ -530,6 +533,7 @@ static int w100fb_set_par(struct fb_info *info)
 			par->extmem_active = 0;
 			info->fix.smem_len = MEM_INT_SIZE+1;
 		}
+		mutex_unlock(&info->mm_lock);
 
 		w100fb_activate_var(par);
 	}
diff --git a/include/linux/fb.h b/include/linux/fb.h
index 330c4b1..89a4d91 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -814,6 +814,7 @@ struct fb_info {
 	int node;
 	int flags;
 	struct mutex lock;		/* Lock for open/release/ioctl funcs */
+	struct mutex mm_lock;		/* Lock for fb_mmap and smem_* fields */
 	struct fb_var_screeninfo var;	/* Current var */
 	struct fb_fix_screeninfo fix;	/* Current fix */
 	struct fb_monspecs monspecs;	/* Current Monitor specs */

----------------------------------------------------------------------
Kup wlasne mieszkanie za 33 tys. zl.
Sprawdz >>> http://link.interia.pl/f21a3


------------------------------------------------------------------------------
Register Now for Creativity and Technology (CaT), June 3rd, NYC. CaT 
is a gathering of tech-side developers & brand creativity professionals. Meet
the minds behind Google Creative Lab, Visual Complexity, Processing, & 
iPhoneDevCamp as they present alongside digital heavyweights like Barbarian 
Group, R/GA, & Big Spaceship. http://p.sf.net/sfu/creativitycat-com 

Re: [PATCH] add mutex to fbdev for fb_mmap locking

[Linus Torvalds]

On Sun, 31 May 2009, Krzysztof Helt wrote:
>  
>  static int atafb_get_fix(struct fb_fix_screeninfo *fix, struct fb_info *info)
> +__acquires(&info->mm_lock)
> +__releases(&info->mm_lock)

Btw, you shouldn't do this when things nest "correctly" - sparse will see 
the fact that the function acquires and releases a lock and is perfectly 
happy.

The extra annotations are helpful only if you have a function that does 
something _unexpected_ to a lock, like first releasing it and then 
re-aqcuiring it, or releasing it without acquiring it at all. Then the 
extra annotations (a) tell sparse not to complain about odd locking, and 
(b) act as visual notification to users that this function drops a lock in 
the middle.

I haven't actually looked at the patch yet, I just reacted to this part of 
it.

		Linus

------------------------------------------------------------------------------
Register Now for Creativity and Technology (CaT), June 3rd, NYC. CaT 
is a gathering of tech-side developers & brand creativity professionals. Meet
the minds behind Google Creative Lab, Visual Complexity, Processing, & 
iPhoneDevCamp as they present alongside digital heavyweights like Barbarian 
Group, R/GA, & Big Spaceship. http://p.sf.net/sfu/creativitycat-com 

Re: [PATCH] add mutex to fbdev for fb_mmap locking

[Krzysztof Helt]

On Sun, 31 May 2009 10:09:01 -0700 (PDT)
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> 
> 
> On Sun, 31 May 2009, Krzysztof Helt wrote:
> >  
> >  static int atafb_get_fix(struct fb_fix_screeninfo *fix, struct fb_info *info)
> > +__acquires(&info->mm_lock)
> > +__releases(&info->mm_lock)
> 
> Btw, you shouldn't do this when things nest "correctly" - sparse will see 
> the fact that the function acquires and releases a lock and is perfectly 
> happy.
> 

I will fix it. 
If the revert to BKL is rejected this patch may wait till 2.6.31.

Regards,
Krzysztof

------------------------------------------------------------------------------
OpenSolaris 2009.06 is a cutting edge operating system for enterprises 
looking to deploy the next generation of Solaris that includes the latest 
innovations from Sun and the OpenSource community. Download a copy and 
enjoy capabilities such as Networking, Storage and Virtualization. 
Go to: http://p.sf.net/sfu/opensolaris-get

Re: [PATCH] add mutex to fbdev for fb_mmap locking

[Linus Torvalds]

On Mon, 1 Jun 2009, Krzysztof Helt wrote:
> 
> I will fix it. 

Thanks.

> If the revert to BKL is rejected this patch may wait till 2.6.31.

Ahh, so this one helps clean up locking, but doesn't fix any actual 
regressions? I was going to ask you about that.

Btw - one thing you could try on the whole lockdep front - and I realize 
that this is a _total_ hack - is to try the patch below.

The _only_ thing it does is to hide the sysfs_mutex -> mm_lock chain from 
lockdep, by using the (incorrect) __copy_to_user_inatomic() instead of the 
(correct) copy_to_user(). But I'd like to hear if that sysfs_mutex in 
readdir is the only way you can get a chain.

So this is in _no_ way meant to be a serious patch, and is literally just 
a test to see if "ok, readdir and sysfs_mutex is really the only real 
deadlock case" is true. Or are there possibly other cases that trigger 
this?

Because if it's really just this chain through sysfs_mutex, then the 
deadlock scenario should boil down to just the "two different fbcon's and 
really unlucky activity just as you register the second of them". Which is 
still a bug, of course - it's just not ever going to bite anybody in 
practice.

		Linus

---
 fs/readdir.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/readdir.c b/fs/readdir.c
index 7723401..431f647 100644
--- a/fs/readdir.c
+++ b/fs/readdir.c
@@ -172,7 +172,7 @@ static int filldir(void * __buf, const char * name, int namlen, loff_t offset,
 		goto efault;
 	if (__put_user(reclen, &dirent->d_reclen))
 		goto efault;
-	if (copy_to_user(dirent->d_name, name, namlen))
+	if (__copy_to_user_inatomic(dirent->d_name, name, namlen))
 		goto efault;
 	if (__put_user(0, dirent->d_name + namlen))
 		goto efault;
@@ -256,7 +256,7 @@ static int filldir64(void * __buf, const char * name, int namlen, loff_t offset,
 		goto efault;
 	if (__put_user(d_type, &dirent->d_type))
 		goto efault;
-	if (copy_to_user(dirent->d_name, name, namlen))
+	if (__copy_to_user_inatomic(dirent->d_name, name, namlen))
 		goto efault;
 	if (__put_user(0, dirent->d_name + namlen))
 		goto efault;

------------------------------------------------------------------------------
OpenSolaris 2009.06 is a cutting edge operating system for enterprises 
looking to deploy the next generation of Solaris that includes the latest 
innovations from Sun and the OpenSource community. Download a copy and 
enjoy capabilities such as Networking, Storage and Virtualization. 
Go to: http://p.sf.net/sfu/opensolaris-get

Re: [PATCH] add mutex to fbdev for fb_mmap locking

[Ingo Molnar]

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Mon, 1 Jun 2009, Krzysztof Helt wrote:
> > 
> > I will fix it.
> 
> Thanks.
> 
> > If the revert to BKL is rejected this patch may wait till 
> > 2.6.31.
> 
> Ahh, so this one helps clean up locking, but doesn't fix any 
> actual regressions? I was going to ask you about that.
> 
> Btw - one thing you could try on the whole lockdep front - and I 
> realize that this is a _total_ hack - is to try the patch below.

One thing that might be less obscure is to add:

	lockdep_off();
	...
	lockdep_on();

to around those user-copies.

Those cause lockdep to totally ignore those dependencies. It's an 
ugly hack - but at least a readable hack IMO. With big red warning 
signs, a promise to fix this ASAP, etc.

It's still much better than a big ugly revert back to the BKL (of 
course - and regardless of how late we are in the cycle) - and 
probably a bit better than the atomic-copy hack.

Or am i missing something?

	Ingo

------------------------------------------------------------------------------
OpenSolaris 2009.06 is a cutting edge operating system for enterprises 
looking to deploy the next generation of Solaris that includes the latest 
innovations from Sun and the OpenSource community. Download a copy and 
enjoy capabilities such as Networking, Storage and Virtualization. 
Go to: http://p.sf.net/sfu/opensolaris-get

Re: [PATCH] add mutex to fbdev for fb_mmap locking

[Krzysztof Helt]

On Mon, 1 Jun 2009 13:38:40 -0700 (PDT)
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> 
> 
> On Mon, 1 Jun 2009, Krzysztof Helt wrote:
> > If the revert to BKL is rejected this patch may wait till 2.6.31.
> 
> Ahh, so this one helps clean up locking, but doesn't fix any actual 
> regressions? I was going to ask you about that.
> 

There is a problem with multihead configuration with and without the patch.
I realize that it is late stage of the 2.6.30 so IMO it is not worth pushing it at 
this moment .
The patch is quite an improvement for smem_start/smem_len handling
so it should find its way into the tree.

> Btw - one thing you could try on the whole lockdep front - and I realize 
> that this is a _total_ hack - is to try the patch below.
> 
> The _only_ thing it does is to hide the sysfs_mutex -> mm_lock chain from 
> lockdep, by using the (incorrect) __copy_to_user_inatomic() instead of the 
> (correct) copy_to_user(). But I'd like to hear if that sysfs_mutex in 
> readdir is the only way you can get a chain.
> 

I'll test this.

Best regards,
Krzysztof

----------------------------------------------------------------------
Chcesz miec nawigacje GPS ?
Zamow lub przedluz umowe na neostrade, a nawigacja bedzie Twoja.
Kliknij na link po szczegoly! http://link.interia.pl/f219a



------------------------------------------------------------------------------
OpenSolaris 2009.06 is a cutting edge operating system for enterprises 
looking to deploy the next generation of Solaris that includes the latest 
innovations from Sun and the OpenSource community. Download a copy and 
enjoy capabilities such as Networking, Storage and Virtualization. 
Go to: http://p.sf.net/sfu/opensolaris-get

Re: [PATCH] add mutex to fbdev for fb_mmap locking

[Krzysztof Helt]

On Mon, 1 Jun 2009 13:38:40 -0700 (PDT)
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> 
> Btw - one thing you could try on the whole lockdep front - and I realize 
> that this is a _total_ hack - is to try the patch below.
> 
> The _only_ thing it does is to hide the sysfs_mutex -> mm_lock chain from 
> lockdep, by using the (incorrect) __copy_to_user_inatomic() instead of the 
> (correct) copy_to_user(). But I'd like to hear if that sysfs_mutex in 
> readdir is the only way you can get a chain.
> 

Not only. The patch uncovers another lockdep. My fb_mmap patch was applied
during this test.

I will investigate the fb_notifier_list.rwsem issue and how to solve this 
but not for the 2.6.30 (I don't enough time).

Regards,
Krzysztof

=======================================================
[ INFO: possible circular locking dependency detected ]
2.6.30-rc7 #5
-------------------------------------------------------
mplayer/1267 is trying to acquire lock:
 (&fb_info->lock){+.+.+.}, at: [<c032c71f>] fb_release+0x1f/0x60

but task is already holding lock:
 (&mm->mmap_sem){++++++}, at: [<c026e072>] sys_munmap+0x22/0x50

which lock already depends on the new lock.


the existing dependency chain (in reverse order) is:

-> #3 (&mm->mmap_sem){++++++}:
       [<c0243b3d>] validate_chain+0xa8d/0xfd0
       [<c0244318>] __lock_acquire+0x298/0x9e0
       [<c0244ad4>] lock_acquire+0x74/0xa0
       [<c026a967>] might_fault+0x77/0xa0
       [<c031aa93>] copy_to_user+0x33/0x60
       [<c0351a38>] tty_mode_ioctl+0x98/0x4c0
       [<c0351e9a>] n_tty_ioctl_helper+0x3a/0x180
       [<c034ed67>] n_tty_ioctl+0x27/0xd0
       [<c034d96e>] tty_ioctl+0xae/0x850
       [<c0288610>] vfs_ioctl+0x20/0x70
       [<c0288a34>] do_vfs_ioctl+0x2c4/0x4c0
       [<c0288c69>] sys_ioctl+0x39/0x60
       [<c0202c65>] syscall_call+0x7/0xb
       [<ffffffff>] 0xffffffff

-> #2 (&tty->termios_mutex){+.+...}:
       [<c0243b3d>] validate_chain+0xa8d/0xfd0
       [<c0244318>] __lock_acquire+0x298/0x9e0
       [<c0244ad4>] lock_acquire+0x74/0xa0
       [<c044d4c3>] mutex_lock_nested+0x53/0x280
       [<c034c5c2>] tty_do_resize+0x22/0xe0
       [<c035f06d>] vc_do_resize+0x2fd/0x380
       [<c035f15b>] vc_resize+0x1b/0x30
       [<c03382eb>] fbcon_init+0x19b/0x400
       [<c035aec0>] visual_init+0x80/0xc0
       [<c035ea28>] bind_con_driver+0x158/0x2e0
       [<c035ebe3>] take_over_console+0x33/0x50
       [<c03385b2>] fbcon_takeover+0x62/0xb0
       [<c0339175>] fbcon_event_notify+0x815/0x8f0
       [<c0237f1e>] notifier_call_chain+0x4e/0x90
       [<c0238124>] __blocking_notifier_call_chain+0x44/0x60
       [<c023815a>] blocking_notifier_call_chain+0x1a/0x20
       [<c032c291>] fb_notifier_call_chain+0x11/0x20
       [<c032e3d7>] register_framebuffer+0x177/0x240
       [<ccacc5a6>] radeonfb_pci_register+0x9d6/0xd70 [radeonfb]
       [<c032804e>] local_pci_probe+0xe/0x10
       [<c0328c8c>] pci_device_probe+0x5c/0x80
       [<c0371548>] driver_probe_device+0x68/0x140
       [<c0371695>] __driver_attach+0x75/0x80
       [<c0370b23>] bus_for_each_dev+0x43/0x70
       [<c03713e9>] driver_attach+0x19/0x20
       [<c03711a3>] bus_add_driver+0x1b3/0x250
       [<c03718ea>] driver_register+0x5a/0x120
       [<c032902e>] __pci_register_driver+0x4e/0xb0
       [<ccad5017>] 0xccad5017
       [<c020102b>] _stext+0x2b/0x150
       [<c024ce05>] sys_init_module+0x85/0x1c0
       [<c0202c65>] syscall_call+0x7/0xb
       [<ffffffff>] 0xffffffff

-> #1 ((fb_notifier_list).rwsem){.+.+.+}:
       [<c0243b3d>] validate_chain+0xa8d/0xfd0
       [<c0244318>] __lock_acquire+0x298/0x9e0
       [<c0244ad4>] lock_acquire+0x74/0xa0
       [<c044dc47>] down_read+0x47/0x60
       [<c023810a>] __blocking_notifier_call_chain+0x2a/0x60
       [<c023815a>] blocking_notifier_call_chain+0x1a/0x20
       [<c032c291>] fb_notifier_call_chain+0x11/0x20
       [<c032e3d7>] register_framebuffer+0x177/0x240
       [<ccacc5a6>] radeonfb_pci_register+0x9d6/0xd70 [radeonfb]
       [<c032804e>] local_pci_probe+0xe/0x10
       [<c0328c8c>] pci_device_probe+0x5c/0x80
       [<c0371548>] driver_probe_device+0x68/0x140
       [<c0371695>] __driver_attach+0x75/0x80
       [<c0370b23>] bus_for_each_dev+0x43/0x70
       [<c03713e9>] driver_attach+0x19/0x20
       [<c03711a3>] bus_add_driver+0x1b3/0x250
       [<c03718ea>] driver_register+0x5a/0x120
       [<c032902e>] __pci_register_driver+0x4e/0xb0
       [<ccad5017>] 0xccad5017
       [<c020102b>] _stext+0x2b/0x150
       [<c024ce05>] sys_init_module+0x85/0x1c0
       [<c0202c65>] syscall_call+0x7/0xb
       [<ffffffff>] 0xffffffff

-> #0 (&fb_info->lock){+.+.+.}:
       [<c024362b>] validate_chain+0x57b/0xfd0
       [<c0244318>] __lock_acquire+0x298/0x9e0
       [<c0244ad4>] lock_acquire+0x74/0xa0
       [<c044d4c3>] mutex_lock_nested+0x53/0x280
       [<c032c71f>] fb_release+0x1f/0x60
       [<c027d9a6>] __fput+0xc6/0x1c0
       [<c027dcf8>] fput+0x18/0x20
       [<c026c53c>] remove_vma+0x3c/0x60
       [<c026d015>] do_munmap+0x1f5/0x260
       [<c026e07f>] sys_munmap+0x2f/0x50
       [<c0202c65>] syscall_call+0x7/0xb
       [<ffffffff>] 0xffffffff

other info that might help us debug this:

1 lock held by mplayer/1267:
 #0:  (&mm->mmap_sem){++++++}, at: [<c026e072>] sys_munmap+0x22/0x50

stack backtrace:
Pid: 1267, comm: mplayer Tainted: G        W  2.6.30-rc7 #5
Call Trace:
 [<c0242f48>] print_circular_bug_tail+0x78/0xc0
 [<c0240de3>] ? print_circular_bug_entry+0x43/0x50
 [<c024362b>] validate_chain+0x57b/0xfd0
 [<c0244318>] __lock_acquire+0x298/0x9e0
 [<c0241e6b>] ? trace_hardirqs_on+0xb/0x10
 [<c0244ad4>] lock_acquire+0x74/0xa0
 [<c032c71f>] ? fb_release+0x1f/0x60
 [<c044d4c3>] mutex_lock_nested+0x53/0x280
 [<c032c71f>] ? fb_release+0x1f/0x60
 [<c026a708>] ? free_pgd_range+0x128/0x170
 [<c032c71f>] fb_release+0x1f/0x60
 [<c027d9a6>] __fput+0xc6/0x1c0
 [<c027dcf8>] fput+0x18/0x20
 [<c026c53c>] remove_vma+0x3c/0x60
 [<c026d015>] do_munmap+0x1f5/0x260
 [<c026e07f>] sys_munmap+0x2f/0x50
 [<c0202c65>] syscall_call+0x7/0xb

----------------------------------------------------------------------
Audi kilka tysiecy zlotych taniej? Przebieraj wsrod tysiecy ogloszen!
Sprawdz >>> http://link.interia.pl/f21b7


------------------------------------------------------------------------------
OpenSolaris 2009.06 is a cutting edge operating system for enterprises 
looking to deploy the next generation of Solaris that includes the latest 
innovations from Sun and the OpenSource community. Download a copy and 
enjoy capabilities such as Networking, Storage and Virtualization. 
Go to: http://p.sf.net/sfu/opensolaris-get

Re: [PATCH] add mutex to fbdev for fb_mmap locking

[Linus Torvalds]

Alan cc'd due to the terminal layer locking thing.

On Tue, 2 Jun 2009, Krzysztof Helt wrote:
> 
> Not only. The patch uncovers another lockdep. My fb_mmap patch was applied
> during this test.
> 
> I will investigate the fb_notifier_list.rwsem issue and how to solve this 
> but not for the 2.6.30 (I don't enough time).

Sure.  Thanks for testing. This one still seems to be all about the fb 
registration phase (ie no runtime deadlocks), an I guess a 

        lockdep_off();
        ...
        lockdep_on();

around just the registration part would have been the much better thing to 
do (ie snip the lockdep chains at the actual point we don't care about).

That said, this particular lockdep chain does point to an interesting 
chain in the loop:

> -> #3 (&mm->mmap_sem){++++++}:
>        [<c0243b3d>] validate_chain+0xa8d/0xfd0
>        [<c0244318>] __lock_acquire+0x298/0x9e0
>        [<c0244ad4>] lock_acquire+0x74/0xa0
>        [<c026a967>] might_fault+0x77/0xa0
>        [<c031aa93>] copy_to_user+0x33/0x60
>        [<c0351a38>] tty_mode_ioctl+0x98/0x4c0
>        [<c0351e9a>] n_tty_ioctl_helper+0x3a/0x180
>        [<c034ed67>] n_tty_ioctl+0x27/0xd0
>        [<c034d96e>] tty_ioctl+0xae/0x850
>        [<c0288610>] vfs_ioctl+0x20/0x70
>        [<c0288a34>] do_vfs_ioctl+0x2c4/0x4c0
>        [<c0288c69>] sys_ioctl+0x39/0x60
>        [<c0202c65>] syscall_call+0x7/0xb
>        [<ffffffff>] 0xffffffff

ie the fact that the TTY layer does user-mode copies while holding some 
tty lock. So now the tty layer introduces that chain from some random lock 
to the mmap_sem.

(Which is not a deadlock in itself, but is now a very strong link in a 
chain of locking for any device driver that does both mmap() and acts as a 
tty. Admittedly fbcon is perhaps fairly unique in that).

I thought we had gotten rid of all of those. And we probably did - on the 
read/write side. But apparently not termios_lock.

> -> #2 (&tty->termios_mutex){+.+...}:
>        [<c0243b3d>] validate_chain+0xa8d/0xfd0
>        [<c0244318>] __lock_acquire+0x298/0x9e0
>        [<c0244ad4>] lock_acquire+0x74/0xa0
>        [<c044d4c3>] mutex_lock_nested+0x53/0x280
>        [<c034c5c2>] tty_do_resize+0x22/0xe0
>        [<c035f06d>] vc_do_resize+0x2fd/0x380
>        [<c035f15b>] vc_resize+0x1b/0x30
>        [<c03382eb>] fbcon_init+0x19b/0x400
>        [<c035aec0>] visual_init+0x80/0xc0
>        [<c035ea28>] bind_con_driver+0x158/0x2e0
>        [<c035ebe3>] take_over_console+0x33/0x50
>        [<c03385b2>] fbcon_takeover+0x62/0xb0
>        [<c0339175>] fbcon_event_notify+0x815/0x8f0
>        [<c0237f1e>] notifier_call_chain+0x4e/0x90
>        [<c0238124>] __blocking_notifier_call_chain+0x44/0x60
>        [<c023815a>] blocking_notifier_call_chain+0x1a/0x20
>        [<c032c291>] fb_notifier_call_chain+0x11/0x20
>        [<c032e3d7>] register_framebuffer+0x177/0x240

And here we have the other link in the chain. Again, it's to that fairly 
uninteresting "register_framebuffer()", so in practice none of this will 
ever deadlock, but it shows how easy it is to get subtle chains like that.

			Linus

------------------------------------------------------------------------------
OpenSolaris 2009.06 is a cutting edge operating system for enterprises 
looking to deploy the next generation of Solaris that includes the latest 
innovations from Sun and the OpenSource community. Download a copy and 
enjoy capabilities such as Networking, Storage and Virtualization. 
Go to: http://p.sf.net/sfu/opensolaris-get

Re: [PATCH] add mutex to fbdev for fb_mmap locking

[Alan Cox]

> ie the fact that the TTY layer does user-mode copies while holding some 
> tty lock. So now the tty layer introduces that chain from some random lock 
> to the mmap_sem.

Its basically holding the termios lock to copy from the struct termios to
user space which means its trivial to do copy to a stack buffer first. I
can fix that pretty easily if you want.

Alan

------------------------------------------------------------------------------
OpenSolaris 2009.06 is a cutting edge operating system for enterprises 
looking to deploy the next generation of Solaris that includes the latest 
innovations from Sun and the OpenSource community. Download a copy and 
enjoy capabilities such as Networking, Storage and Virtualization. 
Go to: http://p.sf.net/sfu/opensolaris-get

Re: [PATCH] add mutex to fbdev for fb_mmap locking

[Linus Torvalds]

On Tue, 2 Jun 2009, Alan Cox wrote:
>
> > ie the fact that the TTY layer does user-mode copies while holding some 
> > tty lock. So now the tty layer introduces that chain from some random lock 
> > to the mmap_sem.
> 
> Its basically holding the termios lock to copy from the struct termios to
> user space which means its trivial to do copy to a stack buffer first. I
> can fix that pretty easily if you want.

It would be good. I don't know if it matters for any other path, but 
mmap_sem has always been a total _bitch_ to work around for deadlocks, so 
it's always good to try to avoid holding another lock while doing copying 
to/from user. 

I thought we already always copied things to a buffer (for conversion 
reasons, ie doing the whole "ktermios<->random-user-termios-of-the-day" 
thing), but I guess I was wrong.

		Linus

------------------------------------------------------------------------------
OpenSolaris 2009.06 is a cutting edge operating system for enterprises 
looking to deploy the next generation of Solaris that includes the latest 
innovations from Sun and the OpenSource community. Download a copy and 
enjoy capabilities such as Networking, Storage and Virtualization. 
Go to: http://p.sf.net/sfu/opensolaris-get

Re: [PATCH] add mutex to fbdev for fb_mmap locking

[Linus Torvalds]

On Tue, 2 Jun 2009, Linus Torvalds wrote:
> 
> I thought we already always copied things to a buffer (for conversion 
> reasons, ie doing the whole "ktermios<->random-user-termios-of-the-day" 
> thing), but I guess I was wrong.

Ahh. We do it in the other direction (ie set_termios), and for some 
limited form of to-user (get_sgttyb, get_tchars etc) but apparently not 
for TCGETS*.

There's a few other odd corners there too. Look at TCGETA - it doesn't get 
the lock at all. Why are TCGETS* and TCGETA so different?

I wonder if we even really need that lock for TCGETS*. We clearly don't do 
it for "struct termio" (TCGETA).

The same imbalance seems to exist for get_termiox vs set_termiox. The 
"set" part does the nice "copy outside the lock", while the "get" part 
copies to user space inside the lock.

And then there is TIOCGSOFTCAR, which is just insane, and apparently gets 
the lock in order to just test _one_ bit (C_CLOCAL). Never mind that if 
something is changing it, we really don't care _which_ case we return, so 
the lock is likely pointless to begin with (can "termios" actually change 
as a pointer?). But then does the user space access with the lock held.

		Linus

------------------------------------------------------------------------------
OpenSolaris 2009.06 is a cutting edge operating system for enterprises 
looking to deploy the next generation of Solaris that includes the latest 
innovations from Sun and the OpenSource community. Download a copy and 
enjoy capabilities such as Networking, Storage and Virtualization. 
Go to: http://p.sf.net/sfu/opensolaris-get

Re: [PATCH] add mutex to fbdev for fb_mmap locking

[Alan Cox]

> And then there is TIOCGSOFTCAR, which is just insane, and apparently gets 
> the lock in order to just test _one_ bit (C_CLOCAL). Never mind that if 

If you don't take it there are cases where it gets cleared by drivers
momentarily and put back so you would give the wrong answer.

> something is changing it, we really don't care _which_ case we return, so 
> the lock is likely pointless to begin with (can "termios" actually change 
> as a pointer?). But then does the user space access with the lock held.

I'll fix those over the next few days to copy first, its pretty trivial
stuff

------------------------------------------------------------------------------
OpenSolaris 2009.06 is a cutting edge operating system for enterprises 
looking to deploy the next generation of Solaris that includes the latest 
innovations from Sun and the OpenSource community. Download a copy and 
enjoy capabilities such as Networking, Storage and Virtualization. 
Go to: http://p.sf.net/sfu/opensolaris-get

Re: [PATCH] add mutex to fbdev for fb_mmap locking

[Linus Torvalds]

On Tue, 2 Jun 2009, Alan Cox wrote:
> 
> I'll fix those over the next few days to copy first, its pretty trivial
> stuff

Btw, it's probably best to put them in functions of their own, and make 
sure they are marked "noinline". Because otherwise gcc will easily inline 
them and because they then presumably have these "struct termios" etc as 
local variables, if that happens gcc will blow your stack to kingdom come.

			Linus

------------------------------------------------------------------------------
OpenSolaris 2009.06 is a cutting edge operating system for enterprises 
looking to deploy the next generation of Solaris that includes the latest 
innovations from Sun and the OpenSource community. Download a copy and 
enjoy capabilities such as Networking, Storage and Virtualization. 
Go to: http://p.sf.net/sfu/opensolaris-get