Hi

I'm submitting this upstream patch for the 4.19 stable branch.

Mikulas

commit 68a958a915ca912b8ce71b9eea7445996f6e681e Author: Mikulas Patocka mpatocka@redhat.com Date: Mon Oct 8 12:57:34 2018 +0200

udlfb: handle unplug properly

The udlfb driver maintained an open count and cleaned up itself when the count reached zero. But the console is also counted in the reference count - so, if the user unplugged the device, the open count would not drop to zero and the driver stayed loaded with console attached. If the user re-plugged the adapter, it would create a device /dev/fb1, show green screen and the access to the console would be lost.

The framebuffer subsystem has reference counting on its own - in order to fix the unplug bug, we rely the framebuffer reference counting. When the user unplugs the adapter, we call unregister_framebuffer unconditionally. unregister_framebuffer will unbind the console, wait until all users stop using the framebuffer and then call the fb_destroy method. The fb_destroy cleans up the USB driver.

This patch makes the following changes: * Drop dlfb->kref and rely on implicit framebuffer reference counting instead. * dlfb_usb_disconnect calls unregister_framebuffer, the rest of driver cleanup is done in the function dlfb_ops_destroy. dlfb_ops_destroy will be called by the framebuffer subsystem when no processes have the framebuffer open or mapped. * We don't use workqueue during initialization, but initialize directly from dlfb_usb_probe. The workqueue could race with dlfb_usb_disconnect and this racing would produce various kinds of memory corruption. * We use usb_get_dev and usb_put_dev to make sure that the USB subsystem doesn't free the device under us.

Signed-off-by: Mikulas Patocka mpatocka@redhat.com cc: Dave Airlie airlied@redhat.com Cc: Bernie Thompson bernie@plugable.com, Cc: Ladislav Michl ladis@linux-mips.org Signed-off-by: Bartlomiej Zolnierkiewicz b.zolnierkie@samsung.com

diff --git a/drivers/video/fbdev/udlfb.c b/drivers/video/fbdev/udlfb.c index afbd6101c78e..070026a7e55a 100644 --- a/drivers/video/fbdev/udlfb.c +++ b/drivers/video/fbdev/udlfb.c @@ -916,8 +916,6 @@ static int dlfb_ops_open(struct fb_info *info, int user)

dlfb->fb_count++;

- kref_get(&dlfb->kref); - if (fb_defio && (info->fbdefio == NULL)) { /* enable defio at last moment if not disabled by client */

@@ -940,14 +938,17 @@ static int dlfb_ops_open(struct fb_info *info, int user) return 0; }

-/* - * Called when all client interfaces to start transactions have been disabled, - * and all references to our device instance (dlfb_data) are released. - * Every transaction must have a reference, so we know are fully spun down - */ -static void dlfb_free(struct kref *kref) +static void dlfb_ops_destroy(struct fb_info *info) { - struct dlfb_data *dlfb = container_of(kref, struct dlfb_data, kref); + struct dlfb_data *dlfb = info->par; + + if (info->cmap.len != 0) + fb_dealloc_cmap(&info->cmap); + if (info->monspecs.modedb) + fb_destroy_modedb(info->monspecs.modedb); + vfree(info->screen_base); + + fb_destroy_modelist(&info->modelist);

while (!list_empty(&dlfb->deferred_free)) { struct dlfb_deferred_free *d = list_entry(dlfb->deferred_free.next, struct dlfb_deferred_free, list); @@ -957,40 +958,13 @@ static void dlfb_free(struct kref *kref) } vfree(dlfb->backing_buffer); kfree(dlfb->edid); + usb_put_dev(dlfb->udev); kfree(dlfb); -} - -static void dlfb_free_framebuffer(struct dlfb_data *dlfb) -{ - struct fb_info *info = dlfb->info; - - if (info) { - unregister_framebuffer(info); - - if (info->cmap.len != 0) - fb_dealloc_cmap(&info->cmap); - if (info->monspecs.modedb) - fb_destroy_modedb(info->monspecs.modedb); - vfree(info->screen_base); - - fb_destroy_modelist(&info->modelist); - - dlfb->info = NULL; - - /* Assume info structure is freed after this point */ - framebuffer_release(info); - }

- /* ref taken in probe() as part of registering framebfufer */ - kref_put(&dlfb->kref, dlfb_free); + /* Assume info structure is freed after this point */ + framebuffer_release(info); }

-static void dlfb_free_framebuffer_work(struct work_struct *work) -{ - struct dlfb_data *dlfb = container_of(work, struct dlfb_data, - free_framebuffer_work.work); - dlfb_free_framebuffer(dlfb); -} /* * Assumes caller is holding info->lock mutex (for open and release at least) */ @@ -1000,10 +974,6 @@ static int dlfb_ops_release(struct fb_info *info, int user)

dlfb->fb_count--;

- /* We can't free fb_info here - fbmem will touch it when we return */ - if (dlfb->virtualized && (dlfb->fb_count == 0)) - schedule_delayed_work(&dlfb->free_framebuffer_work, HZ); - if ((dlfb->fb_count == 0) && (info->fbdefio)) { fb_deferred_io_cleanup(info); kfree(info->fbdefio); @@ -1013,8 +983,6 @@ static int dlfb_ops_release(struct fb_info *info, int user)

dev_dbg(info->dev, "release, user=%d count=%d\n", user, dlfb->fb_count);

- kref_put(&dlfb->kref, dlfb_free); - return 0; }

@@ -1172,6 +1140,7 @@ static struct fb_ops dlfb_ops = { .fb_blank = dlfb_ops_blank, .fb_check_var = dlfb_ops_check_var, .fb_set_par = dlfb_ops_set_par, + .fb_destroy = dlfb_ops_destroy, };

@@ -1615,12 +1584,13 @@ static int dlfb_parse_vendor_descriptor(struct dlfb_data *dlfb, return true; }

-static void dlfb_init_framebuffer_work(struct work_struct *work); - static int dlfb_usb_probe(struct usb_interface *intf, const struct usb_device_id *id) { + int i; + const struct device_attribute *attr; struct dlfb_data *dlfb; + struct fb_info *info; int retval = -ENOMEM; struct usb_device *usbdev = interface_to_usbdev(intf);

@@ -1631,10 +1601,9 @@ static int dlfb_usb_probe(struct usb_interface *intf, goto error; }

- kref_init(&dlfb->kref); /* matching kref_put in usb .disconnect fn */ INIT_LIST_HEAD(&dlfb->deferred_free);

- dlfb->udev = usbdev; + dlfb->udev = usb_get_dev(usbdev); usb_set_intfdata(intf, dlfb);

dev_dbg(&intf->dev, "console enable=%d\n", console); @@ -1657,42 +1626,6 @@ static int dlfb_usb_probe(struct usb_interface *intf, }

- if (!dlfb_alloc_urb_list(dlfb, WRITES_IN_FLIGHT, MAX_TRANSFER)) { - retval = -ENOMEM; - dev_err(&intf->dev, "unable to allocate urb list\n"); - goto error; - } - - kref_get(&dlfb->kref); /* matching kref_put in free_framebuffer_work */ - - /* We don't register a new USB class. Our client interface is dlfbev */ - - /* Workitem keep things fast & simple during USB enumeration */ - INIT_DELAYED_WORK(&dlfb->init_framebuffer_work, - dlfb_init_framebuffer_work); - schedule_delayed_work(&dlfb->init_framebuffer_work, 0); - - return 0; - -error: - if (dlfb) { - - kref_put(&dlfb->kref, dlfb_free); /* last ref from kref_init */ - - /* dev has been deallocated. Do not dereference */ - } - - return retval; -} - -static void dlfb_init_framebuffer_work(struct work_struct *work) -{ - int i, retval; - struct fb_info *info; - const struct device_attribute *attr; - struct dlfb_data *dlfb = container_of(work, struct dlfb_data, - init_framebuffer_work.work); - /* allocates framebuffer driver structure, not framebuffer memory */ info = framebuffer_alloc(0, &dlfb->udev->dev); if (!info) { @@ -1706,17 +1639,22 @@ static void dlfb_init_framebuffer_work(struct work_struct *work) dlfb->ops = dlfb_ops; info->fbops = &dlfb->ops;

+ INIT_LIST_HEAD(&info->modelist); + + if (!dlfb_alloc_urb_list(dlfb, WRITES_IN_FLIGHT, MAX_TRANSFER)) { + retval = -ENOMEM; + dev_err(&intf->dev, "unable to allocate urb list\n"); + goto error; + } + + /* We don't register a new USB class. Our client interface is dlfbev */ + retval = fb_alloc_cmap(&info->cmap, 256, 0); if (retval < 0) { dev_err(info->device, "cmap allocation failed: %d\n", retval); goto error; }

- INIT_DELAYED_WORK(&dlfb->free_framebuffer_work, - dlfb_free_framebuffer_work); - - INIT_LIST_HEAD(&info->modelist); - retval = dlfb_setup_modes(dlfb, info, NULL, 0); if (retval != 0) { dev_err(info->device, @@ -1760,10 +1698,16 @@ static void dlfb_init_framebuffer_work(struct work_struct *work) dev_name(info->dev), info->var.xres, info->var.yres, ((dlfb->backing_buffer) ? info->fix.smem_len * 2 : info->fix.smem_len) >> 10); - return; + return 0;

error: - dlfb_free_framebuffer(dlfb); + if (dlfb->info) { + dlfb_ops_destroy(dlfb->info); + } else if (dlfb) { + usb_put_dev(dlfb->udev); + kfree(dlfb); + } + return retval; }

static void dlfb_usb_disconnect(struct usb_interface *intf) @@ -1791,20 +1735,9 @@ static void dlfb_usb_disconnect(struct usb_interface *intf) for (i = 0; i < ARRAY_SIZE(fb_device_attrs); i++) device_remove_file(info->dev, &fb_device_attrs[i]); device_remove_bin_file(info->dev, &edid_attr); - unlink_framebuffer(info); }

- usb_set_intfdata(intf, NULL); - dlfb->udev = NULL; - - /* if clients still have us open, will be freed on last close */ - if (dlfb->fb_count == 0) - schedule_delayed_work(&dlfb->free_framebuffer_work, 0); - - /* release reference taken by kref_init in probe() */ - kref_put(&dlfb->kref, dlfb_free); - - /* consider dlfb_data freed */ + unregister_framebuffer(info); }

static struct usb_driver dlfb_driver = { diff --git a/include/video/udlfb.h b/include/video/udlfb.h index 3abd327bada6..7d09e54ae54e 100644 --- a/include/video/udlfb.h +++ b/include/video/udlfb.h @@ -36,12 +36,9 @@ struct dlfb_data { struct usb_device *udev; struct fb_info *info; struct urb_list urbs; - struct kref kref; char *backing_buffer; int fb_count; bool virtualized; /* true when physical usb device not present */ - struct delayed_work init_framebuffer_work; - struct delayed_work free_framebuffer_work; atomic_t usb_active; /* 0 = update virtual buffer, but no usb traffic */ atomic_t lost_pixels; /* 1 = a render op failed. Need screen refresh */ char *edid; /* null until we read edid from hw or get from sysfs */