On Thu, Feb 07, 2019 at 04:07:33PM -0500, Sean Paul wrote:
> 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 <nor...@tronnes.org>
> > ---
> >
> > Maybe s/unplugged/unregistered/ ?
> >
> > I looked at drm_device->registered, but using that would mean that
> > drm_dev_is_unplugged() would return before drm_device is registered.
> > And given that its current purpose is to prevent race against connector
> > registration, I stayed away from it.
>
> Makes sense. I think having unregistered along with registered might cause
> some
> confusion unless it's really clearly documented. Perhaps
> s/unplugged/unregistered/ and s/registered/initialized/ but init has its own
> meaning...
>
> I'd hate to hold up the patchset over naming bikeshed, so maybe this would be
> better off as a follow-on series where everyone can pile on opinions. So,
>
> Reviewed-by: Sean Paul <s...@poorly.run>
Looks like I was dropped from Cc, so I didn't see the replies after Oleksandr's
review. So go ahead and disregard this :-)
Sean
>
>
> >
> > Noralf.
> >
> >
> > 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
> > + * 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.
> > *
> > - * 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
> >
>
> --
> Sean Paul, Software Engineer, Google / Chromium OS
--
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
