[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 at chris-wilson.co.uk>
>>> Cc: Marco Diego Aurélio Mesquita <marcodiegomesquita at gmail.com>
>>> Reported-by: Marco Diego Aurélio Mesquita <marcodiegomesquita at gmail.com>
>>> Signed-off-by: Hans de Goede <hdegoede at 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 at lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>