HID: picoLCD: rework hid-fbdev interaction

Split out all FB related data out of struct picolcd_data into a struct
picolcd_fb_data that is allocated with fb_info. This way fb_info may
cleanly outlive struct picolcd_data for as long as needed for its last
user to drop his reference.

Access to struct picolcd_data is now protected with struct
picolcd_fb_data's lock and tile  update reports are only generated
while picolcd_fbdata->picolcd is not NULL and is not marked as failed
(which indicates unplug in progress).
Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>

drivers/hid/hid-picolcd.h

@@ -90,11 +90,6 @@ struct picolcd_data {
 
 #ifdef CONFIG_HID_PICOLCD_FB
 	/* Framebuffer stuff */
-	u8 fb_update_rate;
-	u8 fb_bpp;
-	u8 fb_force;
-	u8 *fb_vbitmap;		/* local copy of what was sent to PicoLCD */
-	u8 *fb_bitmap;		/* framebuffer */
 	struct fb_info *fb_info;
 #endif /* CONFIG_HID_PICOLCD_FB */
 #ifdef CONFIG_HID_PICOLCD_LCD
@@ -119,9 +114,21 @@ struct picolcd_data {
 	int status;
 #define PICOLCD_BOOTLOADER 1
 #define PICOLCD_FAILED 2
-#define PICOLCD_READY_FB 4
 };
 
+#ifdef CONFIG_HID_PICOLCD_FB
+struct picolcd_fb_data {
+	/* Framebuffer stuff */
+	spinlock_t lock;
+	struct picolcd_data *picolcd;
+	u8 update_rate;
+	u8 bpp;
+	u8 force;
+	u8 ready;
+	u8 *vbitmap;		/* local copy of what was sent to PicoLCD */
+	u8 *bitmap;		/* framebuffer */
+};
+#endif /* CONFIG_HID_PICOLCD_FB */
 
 /* Find a given report */
 #define picolcd_in_report(id, dev) picolcd_report(id, dev, HID_INPUT_REPORT)

drivers/hid/hid-picolcd_fb.c

@@ -99,19 +99,26 @@ static const struct fb_var_screeninfo picolcdfb_var = {
 };
 
 /* Send a given tile to PicoLCD */
-static int picolcd_fb_send_tile(struct hid_device *hdev, int chip, int tile)
+static int picolcd_fb_send_tile(struct picolcd_data *data, u8 *vbitmap,
+		int chip, int tile)
 {
-	struct picolcd_data *data = hid_get_drvdata(hdev);
-	struct hid_report *report1 = picolcd_out_report(REPORT_LCD_CMD_DATA, hdev);
-	struct hid_report *report2 = picolcd_out_report(REPORT_LCD_DATA, hdev);
+	struct hid_report *report1, *report2;
 	unsigned long flags;
 	u8 *tdata;
 	int i;
 
-	if (!report1 || report1->maxfield != 1 || !report2 || report2->maxfield != 1)
+	report1 = picolcd_out_report(REPORT_LCD_CMD_DATA, data->hdev);
+	if (!report1 || report1->maxfield != 1)
+		return -ENODEV;
+	report2 = picolcd_out_report(REPORT_LCD_DATA, data->hdev);
+	if (!report2 || report2->maxfield != 1)
 		return -ENODEV;
 
 	spin_lock_irqsave(&data->lock, flags);
+	if ((data->status & PICOLCD_FAILED)) {
+		spin_unlock_irqrestore(&data->lock, flags);
+		return -ENODEV;
+	}
 	hid_set_field(report1->field[0],  0, chip << 2);
 	hid_set_field(report1->field[0],  1, 0x02);
 	hid_set_field(report1->field[0],  2, 0x00);
@@ -129,7 +136,7 @@ static int picolcd_fb_send_tile(struct hid_device *hdev, int chip, int tile)
 	hid_set_field(report2->field[0],  2, 0x00);
 	hid_set_field(report2->field[0],  3,   32);
 
-	tdata = data->fb_vbitmap + (tile * 4 + chip) * 64;
+	tdata = vbitmap + (tile * 4 + chip) * 64;
 	for (i = 0; i < 64; i++)
 		if (i < 32)
 			hid_set_field(report1->field[0], 11 + i, tdata[i]);
@@ -190,6 +197,7 @@ void picolcd_fb_refresh(struct picolcd_data *data)
 int picolcd_fb_reset(struct picolcd_data *data, int clear)
 {
 	struct hid_report *report = picolcd_out_report(REPORT_LCD_CMD, data->hdev);
+	struct picolcd_fb_data *fbdata = data->fb_info->par;
 	int i, j;
 	unsigned long flags;
 	static const u8 mapcmd[8] = { 0x00, 0x02, 0x00, 0x64, 0x3f, 0x00, 0x64, 0xc0 };
@@ -208,20 +216,19 @@ int picolcd_fb_reset(struct picolcd_data *data, int clear)
 				hid_set_field(report->field[0], j, 0);
 		usbhid_submit_report(data->hdev, report, USB_DIR_OUT);
 	}
-
-	data->status |= PICOLCD_READY_FB;
 	spin_unlock_irqrestore(&data->lock, flags);
 
-	if (data->fb_bitmap) {
-		if (clear) {
-			memset(data->fb_vbitmap, 0, PICOLCDFB_SIZE);
-			memset(data->fb_bitmap, 0, PICOLCDFB_SIZE*data->fb_bpp);
-		}
-		data->fb_force = 1;
+	if (clear) {
+		memset(fbdata->vbitmap, 0, PICOLCDFB_SIZE);
+		memset(fbdata->bitmap, 0, PICOLCDFB_SIZE*fbdata->bpp);
 	}
+	fbdata->force = 1;
 
 	/* schedule first output of framebuffer */
-	picolcd_fb_refresh(data);
+	if (fbdata->ready)
+		schedule_delayed_work(&data->fb_info->deferred_work, 0);
+	else
+		fbdata->ready = 1;
 
 	return 0;
 }
@@ -231,20 +238,15 @@ static void picolcd_fb_update(struct fb_info *info)
 {
 	int chip, tile, n;
 	unsigned long flags;
+	struct picolcd_fb_data *fbdata = info->par;
 	struct picolcd_data *data;
 
 	mutex_lock(&info->lock);
-	data = info->par;
-	if (!data)
-		goto out;
 
-	spin_lock_irqsave(&data->lock, flags);
-	if (!(data->status & PICOLCD_READY_FB)) {
-		spin_unlock_irqrestore(&data->lock, flags);
-		picolcd_fb_reset(data, 0);
-	} else {
-		spin_unlock_irqrestore(&data->lock, flags);
-	}
+	spin_lock_irqsave(&fbdata->lock, flags);
+	if (!fbdata->ready && fbdata->picolcd)
+		picolcd_fb_reset(fbdata->picolcd, 0);
+	spin_unlock_irqrestore(&fbdata->lock, flags);
 
 	/*
 	 * Translate the framebuffer into the format needed by the PicoLCD.
@@ -255,32 +257,38 @@ static void picolcd_fb_update(struct fb_info *info)
 	 */
 	n = 0;
 	for (chip = 0; chip < 4; chip++)
-		for (tile = 0; tile < 8; tile++)
-			if (picolcd_fb_update_tile(data->fb_vbitmap,
-					data->fb_bitmap, data->fb_bpp, chip, tile) ||
-				data->fb_force) {
-				n += 2;
-				if (n >= HID_OUTPUT_FIFO_SIZE / 2) {
-					mutex_unlock(&info->lock);
-					usbhid_wait_io(data->hdev);
-					mutex_lock(&info->lock);
-					data = info->par;
-					if (!data)
-						goto out;
-					spin_lock_irqsave(&data->lock, flags);
-					if (data->status & PICOLCD_FAILED) {
-						spin_unlock_irqrestore(&data->lock, flags);
-						goto out;
-					}
-					spin_unlock_irqrestore(&data->lock, flags);
-					n = 0;
-				}
-				picolcd_fb_send_tile(data->hdev, chip, tile);
+		for (tile = 0; tile < 8; tile++) {
+			if (!fbdata->force && !picolcd_fb_update_tile(
+					fbdata->vbitmap, fbdata->bitmap,
+					fbdata->bpp, chip, tile))
+				continue;
+			n += 2;
+			if (n >= HID_OUTPUT_FIFO_SIZE / 2) {
+				spin_lock_irqsave(&fbdata->lock, flags);
+				data = fbdata->picolcd;
+				spin_unlock_irqrestore(&fbdata->lock, flags);
+				mutex_unlock(&info->lock);
+				if (!data)
+					return;
+				usbhid_wait_io(data->hdev);
+				mutex_lock(&info->lock);
+				n = 0;
 			}
-	data->fb_force = false;
+			spin_lock_irqsave(&fbdata->lock, flags);
+			data = fbdata->picolcd;
+			spin_unlock_irqrestore(&fbdata->lock, flags);
+			if (!data || picolcd_fb_send_tile(data,
+					fbdata->vbitmap, chip, tile))
+				goto out;
+		}
+	fbdata->force = false;
 	if (n) {
+		spin_lock_irqsave(&fbdata->lock, flags);
+		data = fbdata->picolcd;
+		spin_unlock_irqrestore(&fbdata->lock, flags);
 		mutex_unlock(&info->lock);
-		usbhid_wait_io(data->hdev);
+		if (data)
+			usbhid_wait_io(data->hdev);
 		return;
 	}
 out:
@@ -337,20 +345,22 @@ static ssize_t picolcd_fb_write(struct fb_info *info, const char __user *buf,
 
 static int picolcd_fb_blank(int blank, struct fb_info *info)
 {
-	if (!info->par)
-		return -ENODEV;
 	/* We let fb notification do this for us via lcd/backlight device */
 	return 0;
 }
 
 static void picolcd_fb_destroy(struct fb_info *info)
 {
+	struct picolcd_fb_data *fbdata = info->par;
+
 	/* make sure no work is deferred */
 	fb_deferred_io_cleanup(info);
 
+	/* No thridparty should ever unregister our framebuffer! */
+	WARN_ON(fbdata->picolcd != NULL);
+
 	vfree((u8 *)info->fix.smem_start);
 	framebuffer_release(info);
-	printk(KERN_DEBUG "picolcd_fb_destroy(%p)\n", info);
 }
 
 static int picolcd_fb_check_var(struct fb_var_screeninfo *var, struct fb_info *info)
@@ -377,17 +387,15 @@ static int picolcd_fb_check_var(struct fb_var_screeninfo *var, struct fb_info *i
 
 static int picolcd_set_par(struct fb_info *info)
 {
-	struct picolcd_data *data = info->par;
+	struct picolcd_fb_data *fbdata = info->par;
 	u8 *tmp_fb, *o_fb;
-	if (!data)
-		return -ENODEV;
-	if (info->var.bits_per_pixel == data->fb_bpp)
+	if (info->var.bits_per_pixel == fbdata->bpp)
 		return 0;
 	/* switch between 1/8 bit depths */
 	if (info->var.bits_per_pixel != 1 && info->var.bits_per_pixel != 8)
 		return -EINVAL;
 
-	o_fb   = data->fb_bitmap;
+	o_fb   = fbdata->bitmap;
 	tmp_fb = kmalloc(PICOLCDFB_SIZE*info->var.bits_per_pixel, GFP_KERNEL);
 	if (!tmp_fb)
 		return -ENOMEM;
@@ -416,7 +424,7 @@ static int picolcd_set_par(struct fb_info *info)
 	}
 
 	kfree(tmp_fb);
-	data->fb_bpp      = info->var.bits_per_pixel;
+	fbdata->bpp = info->var.bits_per_pixel;
 	return 0;
 }
 
@@ -454,7 +462,8 @@ static ssize_t picolcd_fb_update_rate_show(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
 	struct picolcd_data *data = dev_get_drvdata(dev);
-	unsigned i, fb_update_rate = data->fb_update_rate;
+	struct picolcd_fb_data *fbdata = data->fb_info->par;
+	unsigned i, fb_update_rate = fbdata->update_rate;
 	size_t ret = 0;
 
 	for (i = 1; i <= PICOLCDFB_UPDATE_RATE_LIMIT; i++)
@@ -473,6 +482,7 @@ static ssize_t picolcd_fb_update_rate_store(struct device *dev,
 		struct device_attribute *attr, const char *buf, size_t count)
 {
 	struct picolcd_data *data = dev_get_drvdata(dev);
+	struct picolcd_fb_data *fbdata = data->fb_info->par;
 	int i;
 	unsigned u;
 
@@ -488,8 +498,8 @@ static ssize_t picolcd_fb_update_rate_store(struct device *dev,
 	else if (u == 0)
 		u = PICOLCDFB_UPDATE_RATE_DEFAULT;
 
-	data->fb_update_rate = u;
-	data->fb_info->fbdefio->delay = HZ / data->fb_update_rate;
+	fbdata->update_rate = u;
+	data->fb_info->fbdefio->delay = HZ / fbdata->update_rate;
 	return count;
 }
 
@@ -501,30 +511,18 @@ int picolcd_init_framebuffer(struct picolcd_data *data)
 {
 	struct device *dev = &data->hdev->dev;
 	struct fb_info *info = NULL;
+	struct picolcd_fb_data *fbdata = NULL;
 	int i, error = -ENOMEM;
-	u8 *fb_vbitmap = NULL;
-	u8 *fb_bitmap  = NULL;
 	u32 *palette;
 
-	fb_bitmap = vmalloc(PICOLCDFB_SIZE*8);
-	if (fb_bitmap == NULL) {
-		dev_err(dev, "can't get a free page for framebuffer\n");
-		goto err_nomem;
-	}
-
-	fb_vbitmap = kmalloc(PICOLCDFB_SIZE, GFP_KERNEL);
-	if (fb_vbitmap == NULL) {
-		dev_err(dev, "can't alloc vbitmap image buffer\n");
-		goto err_nomem;
-	}
-
-	data->fb_update_rate = PICOLCDFB_UPDATE_RATE_DEFAULT;
 	/* The extra memory is:
 	 * - 256*u32 for pseudo_palette
 	 * - struct fb_deferred_io
 	 */
 	info = framebuffer_alloc(256 * sizeof(u32) +
-			sizeof(struct fb_deferred_io), dev);
+			sizeof(struct fb_deferred_io) +
+			sizeof(struct picolcd_fb_data) +
+			PICOLCDFB_SIZE, dev);
 	if (info == NULL) {
 		dev_err(dev, "failed to allocate a framebuffer\n");
 		goto err_nomem;
@@ -532,74 +530,86 @@ int picolcd_init_framebuffer(struct picolcd_data *data)
 
 	info->fbdefio = info->par;
 	*info->fbdefio = picolcd_fb_defio;
-	palette  = info->par + sizeof(struct fb_deferred_io);
+	info->par += sizeof(struct fb_deferred_io);
+	palette = info->par;
+	info->par += 256 * sizeof(u32);
 	for (i = 0; i < 256; i++)
 		palette[i] = i > 0 && i < 16 ? 0xff : 0;
 	info->pseudo_palette = palette;
-	info->screen_base = (char __force __iomem *)fb_bitmap;
 	info->fbops = &picolcdfb_ops;
 	info->var = picolcdfb_var;
 	info->fix = picolcdfb_fix;
 	info->fix.smem_len   = PICOLCDFB_SIZE*8;
-	info->fix.smem_start = (unsigned long)fb_bitmap;
-	info->par = data;
 	info->flags = FBINFO_FLAG_DEFAULT;
 
-	data->fb_vbitmap = fb_vbitmap;
-	data->fb_bitmap  = fb_bitmap;
-	data->fb_bpp     = picolcdfb_var.bits_per_pixel;
+	fbdata = info->par;
+	spin_lock_init(&fbdata->lock);
+	fbdata->picolcd = data;
+	fbdata->update_rate = PICOLCDFB_UPDATE_RATE_DEFAULT;
+	fbdata->bpp     = picolcdfb_var.bits_per_pixel;
+	fbdata->force   = 1;
+	fbdata->vbitmap = info->par + sizeof(struct picolcd_fb_data);
+	fbdata->bitmap  = vmalloc(PICOLCDFB_SIZE*8);
+	if (fbdata->bitmap == NULL) {
+		dev_err(dev, "can't get a free page for framebuffer\n");
+		goto err_nomem;
+	}
+	info->screen_base = (char __force __iomem *)fbdata->bitmap;
+	info->fix.smem_start = (unsigned long)fbdata->bitmap;
+	memset(fbdata->vbitmap, 0xff, PICOLCDFB_SIZE);
+	data->fb_info = info;
+
 	error = picolcd_fb_reset(data, 1);
 	if (error) {
 		dev_err(dev, "failed to configure display\n");
 		goto err_cleanup;
 	}
+
 	error = device_create_file(dev, &dev_attr_fb_update_rate);
 	if (error) {
 		dev_err(dev, "failed to create sysfs attributes\n");
 		goto err_cleanup;
 	}
+
 	fb_deferred_io_init(info);
-	data->fb_info    = info;
 	error = register_framebuffer(info);
 	if (error) {
 		dev_err(dev, "failed to register framebuffer\n");
 		goto err_sysfs;
 	}
-	/* schedule first output of framebuffer */
-	data->fb_force = 1;
-	schedule_delayed_work(&info->deferred_work, 0);
 	return 0;
 
 err_sysfs:
-	fb_deferred_io_cleanup(info);
 	device_remove_file(dev, &dev_attr_fb_update_rate);
+	fb_deferred_io_cleanup(info);
 err_cleanup:
-	data->fb_vbitmap = NULL;
-	data->fb_bitmap  = NULL;
-	data->fb_bpp     = 0;
 	data->fb_info    = NULL;
 
 err_nomem:
+	if (fbdata)
+		vfree(fbdata->bitmap);
 	framebuffer_release(info);
-	vfree(fb_bitmap);
-	kfree(fb_vbitmap);
 	return error;
 }
 
 void picolcd_exit_framebuffer(struct picolcd_data *data)
 {
 	struct fb_info *info = data->fb_info;
-	u8 *fb_vbitmap = data->fb_vbitmap;
-
-	if (!info)
-		return;
+	struct picolcd_fb_data *fbdata = info->par;
+	unsigned long flags;
 
 	device_remove_file(&data->hdev->dev, &dev_attr_fb_update_rate);
-	info->par = NULL;
+
+	/* disconnect framebuffer from HID dev */
+	spin_lock_irqsave(&fbdata->lock, flags);
+	fbdata->picolcd = NULL;
+	spin_unlock_irqrestore(&fbdata->lock, flags);
+
+	/* make sure there is no running update - thus that fbdata->picolcd
+	 * once obtained under lock is guaranteed not to get free() under
+	 * the feet of the deferred work */
+	flush_delayed_work_sync(&info->deferred_work);
+
+	data->fb_info = NULL;
 	unregister_framebuffer(info);
-	data->fb_vbitmap = NULL;
-	data->fb_bitmap  = NULL;
-	data->fb_bpp     = 0;
-	data->fb_info    = NULL;
-	kfree(fb_vbitmap);
 }