Re: [PATCH 2/6] drm/drv: Prepare to remove drm_dev_unplug()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




Den 05.02.2019 10.11, skrev Daniel Vetter:
> On Mon, Feb 04, 2019 at 06:35:28PM +0100, Noralf Trønnes wrote:
>>
>>
>> Den 04.02.2019 16.41, skrev Daniel Vetter:
>>> On Sun, Feb 03, 2019 at 04:41:56PM +0100, Noralf Trønnes wrote:
>>>> The only thing now that makes drm_dev_unplug() special is that it sets
>>>> drm_device->unplugged. Move this code to drm_dev_unregister() so that we
>>>> can remove drm_dev_unplug().
>>>>
>>>> Signed-off-by: Noralf Trønnes <noralf@xxxxxxxxxxx>
>>>> ---
>>
>> [...]
>>
>>>>  drivers/gpu/drm/drm_drv.c | 27 +++++++++++++++------------
>>>>  include/drm/drm_drv.h     | 10 ++++------
>>>>  2 files changed, 19 insertions(+), 18 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
>>>> index 05bbc2b622fc..e0941200edc6 100644
>>>> --- a/drivers/gpu/drm/drm_drv.c
>>>> +++ b/drivers/gpu/drm/drm_drv.c
>>>> @@ -366,15 +366,6 @@ EXPORT_SYMBOL(drm_dev_exit);
>>>>   */
>>>>  void drm_dev_unplug(struct drm_device *dev)
>>>>  {
>>>> -	/*
>>>> -	 * After synchronizing any critical read section is guaranteed to see
>>>> -	 * the new value of ->unplugged, and any critical section which might
>>>> -	 * still have seen the old value of ->unplugged is guaranteed to have
>>>> -	 * finished.
>>>> -	 */
>>>> -	dev->unplugged = true;
>>>> -	synchronize_srcu(&drm_unplug_srcu);
>>>> -
>>>>  	drm_dev_unregister(dev);
>>>>  	drm_dev_put(dev);
>>>>  }
>>>> @@ -832,11 +823,14 @@ EXPORT_SYMBOL(drm_dev_register);
>>>>   * drm_dev_register() but does not deallocate the device. The caller must call
>>>>   * drm_dev_put() to drop their final reference.
>>>>   *
>>>> - * A special form of unregistering for hotpluggable devices is drm_dev_unplug(),
>>>> - * which can be called while there are still open users of @dev.
>>>> + * This function can be called while there are still open users of @dev as long
>>>> + * as the driver protects its device resources using drm_dev_enter() and
>>>> + * drm_dev_exit().
>>>>   *
>>>>   * This should be called first in the device teardown code to make sure
>>>> - * userspace can't access the device instance any more.
>>>> + * userspace can't access the device instance any more. Drivers that support
>>>> + * device unplug will probably want to call drm_atomic_helper_shutdown() first
>>>
>>> Read once more with a bit more coffee, spotted this:
>>>
>>> s/first/afterwards/ - shutting down the hw before we've taken it away from
>>> userspace is kinda the wrong way round. It should be the inverse of driver
>>> load, which is 1) allocate structures 2) prep hw 3) register driver with
>>> the world (simplified ofc).
>>>
>>
>> The problem is that drm_dev_unregister() sets the device as unplugged
>> and if drm_atomic_helper_shutdown() is called afterwards it's not
>> allowed to touch hardware.
>>
>> I know it's the wrong order, but the only way to do it in the right
>> order is to have a separate function that sets unplugged:
>>
>> 	drm_dev_unregister();
>> 	drm_atomic_helper_shutdown();
>> 	drm_dev_set_unplugged();
> 
> Annoying ... but yeah calling _shutdown() before we stopped userspace is
> also not going to work. Because userspace could quickly re-enable
> something, and then the refcounts would be all wrong again and leaking
> objects.
> 

What happens with a USB device that is unplugged with open userspace,
will that leak objects?

> I get a bit the feeling we're over-optimizing here with trying to devm-ize
> drm_dev_register. Just getting drm_device correctly devm-ized is a big
> step forward already, and will open up a lot of TODO items across a lot of
> drivers. E.g. we could add a drm_dev_kzalloc, for allocating all the drm_*
> structs, which gets released together with drm_device. I think that's a
> much clearer path forward, I think we all agree that getting the kfree out
> of the driver codes is a good thing, and it would allow us to do this
> correctly.
> 
> Then once we have that and rolled out to a few drivers we can reconsider
> the entire unregister/shutdown gordian knot here. Atm I just have no idea
> how to do this properly :-/
> 
> Thoughts, other ideas?
> 

Yeah, I've come to the conclusion that devm_drm_dev_register() doesn't
make much sense if we need a driver remove callback anyways.

I think devm_drm_dev_init() makes sense because it yields a cleaner
probe() function. An additional benefit is that it requires a
drm_driver->release function which is a step in the right direction to
get the drm_device lifetime right.

Do we agree that a drm_dev_set_unplugged() function is necessary to get
the remove/disconnect order right?

What about drm_dev_unplug() maybe I should just leave it be?

- amd uses drm_driver->unload, so that one takes some work to get right
  to support unplug. It doesn't check the unplugged state, so really
  doesn't need drm_dev_unplug() I guess. Do they have cards that can be
  hotplugged?
- udl uses drm_driver->unload, doesn't use drm_atomic_helper_shutdown().
  It has only one drm_dev_is_unplugged() check and that is in its
  fbdev->open callback.
- xen calls drm_atomic_helper_shutdown() in its drm_driver->release
  callback which I guess is not correct.

This is what it will look like with a set unplugged function:

void drm_dev_unplug(struct drm_device *dev)
{
	drm_dev_set_unplugged(dev);
	drm_dev_unregister(dev);
	drm_dev_put(dev);
}

Hm, I should probably remove it to avoid further use of it since it's
not correct for a modern driver.

Noralf.

> Cheers, Daniel
> 
>> Noralf.
>>
>>>> + * in order to disable the hardware on regular driver module unload.
>>>>   */
>>>>  void drm_dev_unregister(struct drm_device *dev)
>>>>  {
>>>> @@ -845,6 +839,15 @@ void drm_dev_unregister(struct drm_device *dev)
>>>>  	if (drm_core_check_feature(dev, DRIVER_LEGACY))
>>>>  		drm_lastclose(dev);
>>>>  
>>>> +	/*
>>>> +	 * After synchronizing any critical read section is guaranteed to see
>>>> +	 * the new value of ->unplugged, and any critical section which might
>>>> +	 * still have seen the old value of ->unplugged is guaranteed to have
>>>> +	 * finished.
>>>> +	 */
>>>> +	dev->unplugged = true;
>>>> +	synchronize_srcu(&drm_unplug_srcu);
>>>> +
>>>>  	dev->registered = false;
>>>>  
>>>>  	drm_client_dev_unregister(dev);
>>>> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
>>>> index ca46a45a9cce..c50696c82a42 100644
>>>> --- a/include/drm/drm_drv.h
>>>> +++ b/include/drm/drm_drv.h
>>>> @@ -736,13 +736,11 @@ void drm_dev_unplug(struct drm_device *dev);
>>>>   * drm_dev_is_unplugged - is a DRM device unplugged
>>>>   * @dev: DRM device
>>>>   *
>>>> - * This function can be called to check whether a hotpluggable is unplugged.
>>>> - * Unplugging itself is singalled through drm_dev_unplug(). If a device is
>>>> - * unplugged, these two functions guarantee that any store before calling
>>>> - * drm_dev_unplug() is visible to callers of this function after it completes
>>>> + * This function can be called to check whether @dev is unregistered. This can
>>>> + * be used to detect that the underlying parent device is gone.
>>>
>>> I think it'd be good to keep the first part, and just update the reference
>>> to drm_dev_unregister. So:
>>>
>>>  * This function can be called to check whether a hotpluggable is unplugged.
>>>  * Unplugging itself is singalled through drm_dev_unregister(). If a device is
>>>  * unplugged, these two functions guarantee that any store before calling
>>>  * drm_dev_unregister() is visible to callers of this function after it
>>>  * completes.
>>>
>>> I think your version shrugs a few important details under the rug. With
>>> those nits addressed:
>>>
>>> Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
>>>
>>> Cheers, Daniel
>>>
>>>>   *
>>>> - * WARNING: This function fundamentally races against drm_dev_unplug(). It is
>>>> - * recommended that drivers instead use the underlying drm_dev_enter() and
>>>> + * WARNING: This function fundamentally races against drm_dev_unregister(). It
>>>> + * is recommended that drivers instead use the underlying drm_dev_enter() and
>>>>   * drm_dev_exit() function pairs.
>>>>   */
>>>>  static inline bool drm_dev_is_unplugged(struct drm_device *dev)
>>>> -- 
>>>> 2.20.1
>>>>
>>>> _______________________________________________
>>>> Intel-gfx mailing list
>>>> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
>>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>>
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux