[PATCH v2] drm: Do not call drm_dev_unregister twice on drm_unplug_dev

[PATCH v2] drm: Do not call drm_dev_unregister twice on drm_unplug_dev

[Hans de Goede]

Since commit a39be606f99d ("drm: Do a full device unregister when
unplugging") drm_unplug_dev has been calling drm_dev_unregister followed
by a drm_put_dev when open_count reaches 0. This drm_put_dev calls
drm_dev_unregister again. Since drm_dev_unregister is not protected
against being called multiple times this leads to havoc.

This commit fixes this by calling drm_dev_unref instead of drm_put_dev.

Fixes: a39be606f99d ("drm: Do a full device unregister when unplugging")
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Marco Diego Aurélio Mesquita <marcodiegomesquita@gmail.com>
Reported-by: Marco Diego Aurélio Mesquita <marcodiegomesquita@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Note I don't have any USB display devices at hand for testing atm so
this patch has only been compile tested.
---
Changes in v2:
-Remove unnecessary mutex changes
---
 drivers/gpu/drm/drm_drv.c  | 6 +++---
 drivers/gpu/drm/drm_file.c | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index b5c6bb46a425..30b5382bf877 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -364,9 +364,9 @@ void drm_unplug_dev(struct drm_device *dev)
 
 	drm_device_set_unplugged(dev);
 
-	if (dev->open_count == 0) {
-		drm_put_dev(dev);
-	}
+	if (dev->open_count == 0)
+		drm_dev_unref(dev);
+
 	mutex_unlock(&drm_global_mutex);
 }
 EXPORT_SYMBOL(drm_unplug_dev);
diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
index 3783b659cd38..edba71c8ccc3 100644
--- a/drivers/gpu/drm/drm_file.c
+++ b/drivers/gpu/drm/drm_file.c
@@ -424,7 +424,7 @@ int drm_release(struct inode *inode, struct file *filp)
 	if (!--dev->open_count) {
 		drm_lastclose(dev);
 		if (drm_device_is_unplugged(dev))
-			drm_put_dev(dev);
+			drm_dev_unref(dev);
 	}
 	mutex_unlock(&drm_global_mutex);
 
-- 
2.13.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm: Do not call drm_dev_unregister twice on drm_unplug_dev

[Daniel Vetter]

On Sun, May 28, 2017 at 07:16:55PM +0200, Hans de Goede wrote:
> Since commit a39be606f99d ("drm: Do a full device unregister when
> unplugging") drm_unplug_dev has been calling drm_dev_unregister followed
> by a drm_put_dev when open_count reaches 0. This drm_put_dev calls
> drm_dev_unregister again. Since drm_dev_unregister is not protected
> against being called multiple times this leads to havoc.
> 
> This commit fixes this by calling drm_dev_unref instead of drm_put_dev.
> 
> Fixes: a39be606f99d ("drm: Do a full device unregister when unplugging")
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Marco Diego Aurélio Mesquita <marcodiegomesquita@gmail.com>
> Reported-by: Marco Diego Aurélio Mesquita <marcodiegomesquita@gmail.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Note I don't have any USB display devices at hand for testing atm so
> this patch has only been compile tested.
> ---
> Changes in v2:
> -Remove unnecessary mutex changes

Offending patch is in 4.8 ... do we need cc: stable? Does this need a
bugreport link?
-Daniel

> ---
>  drivers/gpu/drm/drm_drv.c  | 6 +++---
>  drivers/gpu/drm/drm_file.c | 2 +-
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index b5c6bb46a425..30b5382bf877 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -364,9 +364,9 @@ void drm_unplug_dev(struct drm_device *dev)
>  
>  	drm_device_set_unplugged(dev);
>  
> -	if (dev->open_count == 0) {
> -		drm_put_dev(dev);
> -	}
> +	if (dev->open_count == 0)
> +		drm_dev_unref(dev);
> +
>  	mutex_unlock(&drm_global_mutex);
>  }
>  EXPORT_SYMBOL(drm_unplug_dev);
> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> index 3783b659cd38..edba71c8ccc3 100644
> --- a/drivers/gpu/drm/drm_file.c
> +++ b/drivers/gpu/drm/drm_file.c
> @@ -424,7 +424,7 @@ int drm_release(struct inode *inode, struct file *filp)
>  	if (!--dev->open_count) {
>  		drm_lastclose(dev);
>  		if (drm_device_is_unplugged(dev))
> -			drm_put_dev(dev);
> +			drm_dev_unref(dev);
>  	}
>  	mutex_unlock(&drm_global_mutex);
>  
> -- 
> 2.13.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm: Do not call drm_dev_unregister twice on drm_unplug_dev

[Daniel Vetter]

On Mon, May 29, 2017 at 09:02:38PM +0200, Daniel Vetter wrote:
> On Sun, May 28, 2017 at 07:16:55PM +0200, Hans de Goede wrote:
> > Since commit a39be606f99d ("drm: Do a full device unregister when
> > unplugging") drm_unplug_dev has been calling drm_dev_unregister followed
> > by a drm_put_dev when open_count reaches 0. This drm_put_dev calls
> > drm_dev_unregister again. Since drm_dev_unregister is not protected
> > against being called multiple times this leads to havoc.
> > 
> > This commit fixes this by calling drm_dev_unref instead of drm_put_dev.
> > 
> > Fixes: a39be606f99d ("drm: Do a full device unregister when unplugging")
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Marco Diego Aurélio Mesquita <marcodiegomesquita@gmail.com>
> > Reported-by: Marco Diego Aurélio Mesquita <marcodiegomesquita@gmail.com>
> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > ---
> > Note I don't have any USB display devices at hand for testing atm so
> > this patch has only been compile tested.
> > ---
> > Changes in v2:
> > -Remove unnecessary mutex changes
> 
> Offending patch is in 4.8 ... do we need cc: stable? Does this need a
> bugreport link?

One more: When fixing a regression pls cc author/reviewer of the offending
patch, for the learning. Adding Chris.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm: Do not call drm_dev_unregister twice on drm_unplug_dev

[Chris Wilson]

On Sun, May 28, 2017 at 07:16:55PM +0200, Hans de Goede wrote:
> Since commit a39be606f99d ("drm: Do a full device unregister when
> unplugging") drm_unplug_dev has been calling drm_dev_unregister followed
> by a drm_put_dev when open_count reaches 0. This drm_put_dev calls
> drm_dev_unregister again. Since drm_dev_unregister is not protected
> against being called multiple times this leads to havoc.
> 
> This commit fixes this by calling drm_dev_unref instead of drm_put_dev.
> 
> Fixes: a39be606f99d ("drm: Do a full device unregister when unplugging")
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Marco Diego Aurélio Mesquita <marcodiegomesquita@gmail.com>
> Reported-by: Marco Diego Aurélio Mesquita <marcodiegomesquita@gmail.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Note I don't have any USB display devices at hand for testing atm so
> this patch has only been compile tested.

Not tested it either, but yes we do end up calling
drm_device_unregister() twice and indeed that looks bad.

This morning I was thinking about kselftests to simulate the different
conditions and avoid us having to get our hands dirty with real hw. It
also reminded me of https://patchwork.kernel.org/patch/9678823/ which
has the tantalising prospect of removing drm_unplug_dev() entirely.

Another benefit is the removal of one more call to drm_put_dev() which
has been deprecated for a while.

The patch definitely fixes a bug at first glance, but I wonder if it is
really telling me that the call to drm_device_unregister() here is the
fundamental issue. But I am short of actual answers. 
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm: Do not call drm_dev_unregister twice on drm_unplug_dev

[Chris Wilson]

On Mon, May 29, 2017 at 09:04:07PM +0200, Daniel Vetter wrote:
> On Mon, May 29, 2017 at 09:02:38PM +0200, Daniel Vetter wrote:
> > On Sun, May 28, 2017 at 07:16:55PM +0200, Hans de Goede wrote:
> > > Since commit a39be606f99d ("drm: Do a full device unregister when
> > > unplugging") drm_unplug_dev has been calling drm_dev_unregister followed
> > > by a drm_put_dev when open_count reaches 0. This drm_put_dev calls
> > > drm_dev_unregister again. Since drm_dev_unregister is not protected
> > > against being called multiple times this leads to havoc.
> > > 
> > > This commit fixes this by calling drm_dev_unref instead of drm_put_dev.
> > > 
> > > Fixes: a39be606f99d ("drm: Do a full device unregister when unplugging")
> > > Cc: Chris Wilson <chris@chris-wilson.co.uk>

> One more: When fixing a regression pls cc author/reviewer of the offending
> patch, for the learning. Adding Chris.

I was CC'ed, the ml stripping headers again?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm: Do not call drm_dev_unregister twice on drm_unplug_dev

[Michel Dänzer]

On 30/05/17 05:13 AM, Chris Wilson wrote:
> On Mon, May 29, 2017 at 09:04:07PM +0200, Daniel Vetter wrote:
>> On Mon, May 29, 2017 at 09:02:38PM +0200, Daniel Vetter wrote:
>>> On Sun, May 28, 2017 at 07:16:55PM +0200, Hans de Goede wrote:
>>>> Since commit a39be606f99d ("drm: Do a full device unregister when
>>>> unplugging") drm_unplug_dev has been calling drm_dev_unregister followed
>>>> by a drm_put_dev when open_count reaches 0. This drm_put_dev calls
>>>> drm_dev_unregister again. Since drm_dev_unregister is not protected
>>>> against being called multiple times this leads to havoc.
>>>>
>>>> This commit fixes this by calling drm_dev_unref instead of drm_put_dev.
>>>>
>>>> Fixes: a39be606f99d ("drm: Do a full device unregister when unplugging")
>>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> 
>> One more: When fixing a regression pls cc author/reviewer of the offending
>> patch, for the learning. Adding Chris.
> 
> I was CC'ed, the ml stripping headers again?

If you have the "Avoid duplicate copies of messages?" option enabled in
your dri-devel mailing list membership configuration, mailman will
remove you from Cc in the posts it distributes to subscribers. Consider
disabling that option.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm: Do not call drm_dev_unregister twice on drm_unplug_dev

[Hans de Goede]

Hi,

On 29-05-17 21:02, Daniel Vetter wrote:
> On Sun, May 28, 2017 at 07:16:55PM +0200, Hans de Goede wrote:
>> Since commit a39be606f99d ("drm: Do a full device unregister when
>> unplugging") drm_unplug_dev has been calling drm_dev_unregister followed
>> by a drm_put_dev when open_count reaches 0. This drm_put_dev calls
>> drm_dev_unregister again. Since drm_dev_unregister is not protected
>> against being called multiple times this leads to havoc.
>>
>> This commit fixes this by calling drm_dev_unref instead of drm_put_dev.
>>
>> Fixes: a39be606f99d ("drm: Do a full device unregister when unplugging")
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Marco Diego Aurélio Mesquita <marcodiegomesquita@gmail.com>
>> Reported-by: Marco Diego Aurélio Mesquita <marcodiegomesquita@gmail.com>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Note I don't have any USB display devices at hand for testing atm so
>> this patch has only been compile tested.
>> ---
>> Changes in v2:
>> -Remove unnecessary mutex changes
> 
> Offending patch is in 4.8 ... do we need cc: stable?

Yes I think a Cc: stable would be good.

> Does this need a bugreport link?

There is no bug-report that I know of (Marco reported the issues
to me by email). This likely fixes some crashes when unplugging
USB display link devices.

Note I plan to actually test this patch with an USB display device
tomorrow, so you may want to hold on merging this till I've got
around to testing this.

Regards,

Hans


>> ---
>>   drivers/gpu/drm/drm_drv.c  | 6 +++---
>>   drivers/gpu/drm/drm_file.c | 2 +-
>>   2 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
>> index b5c6bb46a425..30b5382bf877 100644
>> --- a/drivers/gpu/drm/drm_drv.c
>> +++ b/drivers/gpu/drm/drm_drv.c
>> @@ -364,9 +364,9 @@ void drm_unplug_dev(struct drm_device *dev)
>>   
>>   	drm_device_set_unplugged(dev);
>>   
>> -	if (dev->open_count == 0) {
>> -		drm_put_dev(dev);
>> -	}
>> +	if (dev->open_count == 0)
>> +		drm_dev_unref(dev);
>> +
>>   	mutex_unlock(&drm_global_mutex);
>>   }
>>   EXPORT_SYMBOL(drm_unplug_dev);
>> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
>> index 3783b659cd38..edba71c8ccc3 100644
>> --- a/drivers/gpu/drm/drm_file.c
>> +++ b/drivers/gpu/drm/drm_file.c
>> @@ -424,7 +424,7 @@ int drm_release(struct inode *inode, struct file *filp)
>>   	if (!--dev->open_count) {
>>   		drm_lastclose(dev);
>>   		if (drm_device_is_unplugged(dev))
>> -			drm_put_dev(dev);
>> +			drm_dev_unref(dev);
>>   	}
>>   	mutex_unlock(&drm_global_mutex);
>>   
>> -- 
>> 2.13.0
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm: Do not call drm_dev_unregister twice on drm_unplug_dev

[Hans de Goede]

Hi,

On 30-05-17 14:02, Hans de Goede wrote:
> Hi,
> 
> On 29-05-17 21:02, Daniel Vetter wrote:
>> On Sun, May 28, 2017 at 07:16:55PM +0200, Hans de Goede wrote:
>>> Since commit a39be606f99d ("drm: Do a full device unregister when
>>> unplugging") drm_unplug_dev has been calling drm_dev_unregister followed
>>> by a drm_put_dev when open_count reaches 0. This drm_put_dev calls
>>> drm_dev_unregister again. Since drm_dev_unregister is not protected
>>> against being called multiple times this leads to havoc.
>>>
>>> This commit fixes this by calling drm_dev_unref instead of drm_put_dev.
>>>
>>> Fixes: a39be606f99d ("drm: Do a full device unregister when unplugging")
>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Marco Diego Aurélio Mesquita <marcodiegomesquita@gmail.com>
>>> Reported-by: Marco Diego Aurélio Mesquita <marcodiegomesquita@gmail.com>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> ---
>>> Note I don't have any USB display devices at hand for testing atm so
>>> this patch has only been compile tested.
>>> ---
>>> Changes in v2:
>>> -Remove unnecessary mutex changes
>>
>> Offending patch is in 4.8 ... do we need cc: stable?
> 
> Yes I think a Cc: stable would be good.
> 
>> Does this need a bugreport link?
> 
> There is no bug-report that I know of (Marco reported the issues
> to me by email). This likely fixes some crashes when unplugging
> USB display link devices.
> 
> Note I plan to actually test this patch with an USB display device
> tomorrow, so you may want to hold on merging this till I've got
> around to testing this.

So as promised I've been testing (and then debugging) this today,
this patch is no good.

Chris was right when he said maybe we should not call drm_dev_unregister()
at all, that indeed is bad and causes oopses and an unusable system on
unplug. Reverting Chris' original commit replaces those oopses with
different oopses and still results in an unusable system. I believe
that I know how to fix this now, but it is almost 10pm over here
and I think it is better if I continue this tomorrow.

In the mean time in case it was not clear: self-NACK for this patch.

Regards,

Hans



> 
> Regards,
> 
> Hans
> 
> 
>>> ---
>>>   drivers/gpu/drm/drm_drv.c  | 6 +++---
>>>   drivers/gpu/drm/drm_file.c | 2 +-
>>>   2 files changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
>>> index b5c6bb46a425..30b5382bf877 100644
>>> --- a/drivers/gpu/drm/drm_drv.c
>>> +++ b/drivers/gpu/drm/drm_drv.c
>>> @@ -364,9 +364,9 @@ void drm_unplug_dev(struct drm_device *dev)
>>>       drm_device_set_unplugged(dev);
>>> -    if (dev->open_count == 0) {
>>> -        drm_put_dev(dev);
>>> -    }
>>> +    if (dev->open_count == 0)
>>> +        drm_dev_unref(dev);
>>> +
>>>       mutex_unlock(&drm_global_mutex);
>>>   }
>>>   EXPORT_SYMBOL(drm_unplug_dev);
>>> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
>>> index 3783b659cd38..edba71c8ccc3 100644
>>> --- a/drivers/gpu/drm/drm_file.c
>>> +++ b/drivers/gpu/drm/drm_file.c
>>> @@ -424,7 +424,7 @@ int drm_release(struct inode *inode, struct file *filp)
>>>       if (!--dev->open_count) {
>>>           drm_lastclose(dev);
>>>           if (drm_device_is_unplugged(dev))
>>> -            drm_put_dev(dev);
>>> +            drm_dev_unref(dev);
>>>       }
>>>       mutex_unlock(&drm_global_mutex);
>>> -- 
>>> 2.13.0
>>>
>>> _______________________________________________
>>> dri-devel mailing list
>>> dri-devel@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel