Re: [PATCH 2/5] drm/sysfs: protect sysfs removal code against being run twice.

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Feb 20, 2012 at 04:13:46PM +0000, Dave Airlie wrote:
> From: Dave Airlie <airlied@redhat.com>
> 
> a step towards correct hot unplug for USB devices, we need to
> remove the userspace facing bits at the unplug time for correct
> udev operation.
> 
> Signed-off-by: Dave Airlie <airlied@redhat.com>

The lack of any WARN_ON(mutex_is_locked) here and the funny comment about
the sysfs vs. hotunplug deadlock in the next patch makes me a bit uneasy.

I think the only issue race we need to care about here is a hotremove vs.
module unload race. Everything else should stay around for long enough
(although this might not quite be true with the current code ...). Simply
add a dev->hotremove_mutex and grabbing it in these two functions should
fix this.

It's a bit a funky locking scheme, but imo it should keep on working even
when properly fix up all our setup/teardown issues:
- setup just needs to properly order ctors, i.e. publish the userspace
  interfaces only after everything is properly set up - we don't need any
  locking to achieve that (well, currently we still rely massively on the
  global_mutex dutcttape).
- concurrent removal of the userspace interfaces (like here) would be
  protected by the new hotremove_mutex lock.
- protection against the underlying device disappearing would be handled
  by dev->unplugged (I have some more comments about that)
- and the actual drm_device teardown controlled by reference-counting.
  Currently we get away with abusing the fd open count, but I think sooner
  or later we want a real reference count (e.g. when exporting buffers
  with dma_buf we can't just reap them when the last user disappears).


Cheers, Daniel
> ---
>  drivers/gpu/drm/drm_sysfs.c |    7 ++++++-
>  1 files changed, 6 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
> index 62c3675..5a7bd51 100644
> --- a/drivers/gpu/drm/drm_sysfs.c
> +++ b/drivers/gpu/drm/drm_sysfs.c
> @@ -454,6 +454,8 @@ void drm_sysfs_connector_remove(struct drm_connector *connector)
>  {
>  	int i;
>  
> +	if (!connector->kdev.parent)
> +		return;
>  	DRM_DEBUG("removing \"%s\" from sysfs\n",
>  		  drm_get_connector_name(connector));
>  
> @@ -461,6 +463,7 @@ void drm_sysfs_connector_remove(struct drm_connector *connector)
>  		device_remove_file(&connector->kdev, &connector_attrs[i]);
>  	sysfs_remove_bin_file(&connector->kdev.kobj, &edid_attr);
>  	device_unregister(&connector->kdev);
> +	connector->kdev.parent = NULL;
>  }
>  EXPORT_SYMBOL(drm_sysfs_connector_remove);
>  
> @@ -533,7 +536,9 @@ err_out:
>   */
>  void drm_sysfs_device_remove(struct drm_minor *minor)
>  {
> -	device_unregister(&minor->kdev);
> +	if (minor->kdev.parent)
> +		device_unregister(&minor->kdev);
> +	minor->kdev.parent = NULL;
>  }
>  
>  
> -- 
> 1.7.6
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48