Re: [PATCH 0/1] [RFC] DRM locking issues during early open

[Dave Airlie <airlied@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3830 bytes --]

>>
>>> On Thu, Apr 19, 2012 at 5:47 PM, Dave Airlie <airlied@gmail.com> wrote:
>>> > On Thu, Apr 19, 2012 at 5:41 PM, Andy Whitcroft <apw@canonical.com> wrote:
>>> >> On Thu, Apr 19, 2012 at 05:30:03PM +0100, Dave Airlie wrote:
>>> >>> On Thu, Apr 19, 2012 at 5:22 PM, Andy Whitcroft <apw@canonical.com> wrote:
>>> >>> > We have been carrying a (rather poor) patch for an issue we identified in
>>> >>> > the DRM driver.  This issue is triggered when a DRM device is initialising
>>> >>> > and userspace attempts to open it, typically in response to the sysfs
>>> >>> > device added event.  Basically we allocate the minor numbers making
>>> >>> > the device available, and then call the drm load callback.  Until this
>>> >>> > completes the device is really not ready and these early opens typically
>>> >>> > lead to oopses.
>>> >>> >
>>> >>> > We have been using the following patch to avoid this by marking the minors
>>> >>> > as in error until the load method has completed.  This avoids the early
>>> >>> > open by simply erroring out the opens with EAGAIN.  Obviously we should
>>> >>> > be delaying the open until the load method complete.
>>> >>> >
>>> >>> > I include the existing patch for completness (it is not really ready for
>>> >>> > merging) to illustrate the issue.  I think it is logical that the wait
>>> >>> > should simply be delayed until the load has completed.  I am proposing
>>> >>> > to include a wait queue associated with the idr cache for the drm minors
>>> >>> > which we can use to allow open callers to wait_event_interruptible() on.
>>> >>> > I'll be putting together a prototype shortly and will follow up with it.
>>> >>> >
>>> >>> > Thoughts?
>>> >>>
>>> >>> Couldn't we just delay registering things until the driver is ready to
>>> >>> accept an open?
>>> >>>
>>> >>> Granted the midlayer of drm doesn't make that easy,
>>> >>
>>> >> It seems that we need the dri minor allocated before we hit the load
>>> >> function as things are done right now.
>>> >>
>>> >>> thanks for sending this out, it keeps falling off my radar, I don't
>>> >>> think I've ever seen this reported on RHEL/Fedora, which makes me
>>> >>> wonder what we are doing that makes us lucky.
>>> >>
>>> >> We never hit it until we started doing things earlier and quicker.  I first
>>> >> found it in the prettification of boot so we were keen to get plymouth
>>> >> running as soon as possible.  That lead to random panics and me finding
>>> >> this bug.  The window is tiny as far as I know and it tends to be specific
>>> >> machines and specific package combinations which trigger it reliably.
>>> >>
>>> >> I suspect that a proper fix would allow delaying the registration as you
>>> >> suggest but in the interim a wait would at least avoid the issues we are
>>> >> seeing.  I will see how awful it looks.
>>> >
>>> > Just to confirm its the drm_sysfs_device_add that causes the race we care about.
>>> >
>>> > it needs to happen after the driver is happy. Since it calls
>>> > device_register and that is what triggers udev magic to load the
>>> > userspace.
>>> >
>>> > If you have a userspace app banging on a static device node that might
>>> > need another set of fun fixes.
>>>
>>> Okay the sysfs add and the idr_replace are the things we need to delay.
>>
>> Since you can still get at things with a static node, it seems like
>> locking is the real issue here?  Is there no mutex we can take across
>> init to block any openers until we're done?
>
> well the idr replace should be the thing that matters, since before
> that openers get -ENODEV, after it they end up success.
> we may need a lock around that once we fix the logic.\

Here's my predinner hack, contains random rtl change as well, plz ignore.

now for dinner.

Dave.

[-- Attachment #2: myhack --]
[-- Type: application/octet-stream, Size: 2821 bytes --]

diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c
index 13f3d93..23b472b 100644
--- a/drivers/gpu/drm/drm_pci.c
+++ b/drivers/gpu/drm/drm_pci.c
@@ -367,6 +367,11 @@ int drm_get_pci_dev(struct pci_dev *pdev, const struct pci_device_id *ent,
 
 	list_add_tail(&dev->driver_item, &driver->device_list);
 
+	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
+		drm_activate_minor(&dev->control);
+	}
+	drm_activate_minor(&dev->primary);
+
 	DRM_INFO("Initialized %s %d.%d.%d %s for %s on minor %d\n",
 		 driver->name, driver->major, driver->minor, driver->patchlevel,
 		 driver->date, pci_name(pdev), dev->primary->index);
diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
index aa454f8..703c05a 100644
--- a/drivers/gpu/drm/drm_stub.c
+++ b/drivers/gpu/drm/drm_stub.c
@@ -357,7 +357,7 @@ int drm_get_minor(struct drm_device *dev, struct drm_minor **minor, int type)
 	new_minor->index = minor_id;
 	INIT_LIST_HEAD(&new_minor->master_list);
 
-	idr_replace(&drm_minors_idr, new_minor, minor_id);
+	//idr_replace(&drm_minors_idr, new_minor, minor_id);
 
 	if (type == DRM_MINOR_LEGACY) {
 		ret = drm_proc_init(new_minor, minor_id, drm_proc_root);
@@ -375,13 +375,14 @@ int drm_get_minor(struct drm_device *dev, struct drm_minor **minor, int type)
 		goto err_g2;
 	}
 #endif
-
+#if 0
 	ret = drm_sysfs_device_add(new_minor);
 	if (ret) {
 		printk(KERN_ERR
 		       "DRM: Error sysfs_device_add.\n");
 		goto err_g2;
 	}
+#endif
 	*minor = new_minor;
 
 	DRM_DEBUG("new minor assigned %d\n", minor_id);
@@ -400,6 +401,18 @@ err_idr:
 }
 EXPORT_SYMBOL(drm_get_minor);
 
+int drm_activate_minor(struct drm_minor *minor)
+{
+	int ret;
+	idr_replace(&drm_minors_idr, minor, minor->index);
+	ret = drm_sysfs_device_add(minor);
+	if (ret) {
+		printk(KERN_ERR "DRM: Error sysfs_device_add.\n");
+	}
+	return ret;
+}
+EXPORT_SYMBOL(drm_activate_minor);
+
 /**
  * Put a secondary minor number.
  *
diff --git a/drivers/net/wireless/rtlwifi/pci.c b/drivers/net/wireless/rtlwifi/pci.c
index 288b035..cc15fdb 100644
--- a/drivers/net/wireless/rtlwifi/pci.c
+++ b/drivers/net/wireless/rtlwifi/pci.c
@@ -1941,6 +1941,7 @@ void rtl_pci_disconnect(struct pci_dev *pdev)
 		rtl_deinit_deferred_work(hw);
 		rtlpriv->intf_ops->adapter_stop(hw);
 	}
+	rtlpriv->cfg->ops->disable_interrupt(hw);
 
 	/*deinit rfkill */
 	rtl_deinit_rfkill(hw);
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index dd73104..3a5606f 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1506,6 +1506,7 @@ extern void drm_master_put(struct drm_master **master);
 
 extern void drm_put_dev(struct drm_device *dev);
 extern int drm_put_minor(struct drm_minor **minor);
+extern int drm_activate_minor(struct drm_minor *minor);
 extern void drm_unplug_dev(struct drm_device *dev);
 extern unsigned int drm_debug;