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(); 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 > _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel