* [PATCH v8 0/2] drm: rockchip: Fix rockchip drm unbind crash error @ 2017-04-12 2:55 Jeffy Chen 2017-04-12 2:55 ` [PATCH v8 1/2] drm: Unplug drm device when unregistering it Jeffy Chen 2017-04-12 2:55 ` [PATCH v8 2/2] drm: Prevent release fb after cleanup drm_mode_config Jeffy Chen 0 siblings, 2 replies; 5+ messages in thread From: Jeffy Chen @ 2017-04-12 2:55 UTC (permalink / raw) To: linux-kernel Cc: briannorris, dianders, tfiga, seanpaul, zyw, marcheu, mark.yao, hshi, Jeffy Chen, Daniel Vetter, Jani Nikula, dri-devel, Chris Wilson, David Airlie, Tom Gundersen, Patrik Jakobsson, Dave Airlie Verified on rk3399 chromebook kevin(with cros 4.4 kernel), no more crashes during unbind/bind drm with/out ui service running. Changes in v8: Fix hang when unregistering drm dev with open_count 0 Changes in v7: Address Sean Paul <seanpaul@chromium.org>'s comments. Update commit message. Changes in v6: Address Daniel Vetter <daniel@ffwll.ch>'s comments. Changes in v5: Fix wrong git account. Changes in v2: Fix some commit messages. Jeffy Chen (2): drm: Unplug drm device when unregistering it drm: Prevent release fb after cleanup drm_mode_config drivers/gpu/drm/drm_drv.c | 19 +++---------------- drivers/gpu/drm/drm_framebuffer.c | 5 +++++ drivers/gpu/drm/udl/udl_drv.c | 2 +- include/drm/drmP.h | 5 +++-- include/drm/drm_drv.h | 1 - 5 files changed, 12 insertions(+), 20 deletions(-) -- 2.1.4 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v8 1/2] drm: Unplug drm device when unregistering it 2017-04-12 2:55 [PATCH v8 0/2] drm: rockchip: Fix rockchip drm unbind crash error Jeffy Chen @ 2017-04-12 2:55 ` Jeffy Chen 2017-04-26 19:43 ` Ville Syrjälä 2017-04-12 2:55 ` [PATCH v8 2/2] drm: Prevent release fb after cleanup drm_mode_config Jeffy Chen 1 sibling, 1 reply; 5+ messages in thread From: Jeffy Chen @ 2017-04-12 2:55 UTC (permalink / raw) To: linux-kernel Cc: briannorris, dianders, tfiga, seanpaul, zyw, marcheu, mark.yao, hshi, Jeffy Chen, Daniel Vetter, Jani Nikula, dri-devel, Chris Wilson, David Airlie, Tom Gundersen, Patrik Jakobsson, Dave Airlie After unbinding drm, the user space may still owns the drm dev fd, and may still be able to call drm ioctl. We're using an unplugged state to prevent something like that, so let's reuse it here. Also drop drm_unplug_dev, because it would be unused after other changes. Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com> Reviewed-by: Sean Paul <seanpaul@chromium.org> --- Changes in v8: Fix hang when unregistering drm dev with open_count 0 Changes in v7: Address Sean Paul <seanpaul@chromium.org>'s comments. Changes in v6: Address Daniel Vetter <daniel@ffwll.ch>'s comments. Changes in v5: Fix wrong git account. Changes in v2: Fix some commit messages. drivers/gpu/drm/drm_drv.c | 19 +++---------------- drivers/gpu/drm/udl/udl_drv.c | 2 +- include/drm/drmP.h | 5 +++-- include/drm/drm_drv.h | 1 - 4 files changed, 7 insertions(+), 20 deletions(-) diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index b5c6bb4..cc2d018 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -355,22 +355,6 @@ void drm_put_dev(struct drm_device *dev) } EXPORT_SYMBOL(drm_put_dev); -void drm_unplug_dev(struct drm_device *dev) -{ - /* for a USB device */ - drm_dev_unregister(dev); - - mutex_lock(&drm_global_mutex); - - drm_device_set_unplugged(dev); - - if (dev->open_count == 0) { - drm_put_dev(dev); - } - mutex_unlock(&drm_global_mutex); -} -EXPORT_SYMBOL(drm_unplug_dev); - /* * DRM internal mount * We want to be able to allocate our own "struct address_space" to control @@ -787,6 +771,8 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags) if (drm_core_check_feature(dev, DRIVER_MODESET)) drm_modeset_register_all(dev); + drm_device_set_plug_state(dev, true); + ret = 0; DRM_INFO("Initialized %s %d.%d.%d %s for %s on minor %d\n", @@ -826,6 +812,7 @@ void drm_dev_unregister(struct drm_device *dev) drm_lastclose(dev); dev->registered = false; + drm_device_set_plug_state(dev, false); if (drm_core_check_feature(dev, DRIVER_MODESET)) drm_modeset_unregister_all(dev); diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c index cd8b017..5dbd916 100644 --- a/drivers/gpu/drm/udl/udl_drv.c +++ b/drivers/gpu/drm/udl/udl_drv.c @@ -108,7 +108,7 @@ static void udl_usb_disconnect(struct usb_interface *interface) drm_kms_helper_poll_disable(dev); udl_fbdev_unplug(dev); udl_drop_usb(dev); - drm_unplug_dev(dev); + drm_dev_unregister(dev); } /* diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 3bfafcd..a9a5a64 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -488,10 +488,11 @@ static __inline__ int drm_core_check_feature(struct drm_device *dev, return ((dev->driver->driver_features & feature) ? 1 : 0); } -static inline void drm_device_set_unplugged(struct drm_device *dev) +static inline void drm_device_set_plug_state(struct drm_device *dev, + bool plugged) { smp_wmb(); - atomic_set(&dev->unplugged, 1); + atomic_set(&dev->unplugged, !plugged); } static inline int drm_device_is_unplugged(struct drm_device *dev) diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h index 0fefc3f..eb63078 100644 --- a/include/drm/drm_drv.h +++ b/include/drm/drm_drv.h @@ -544,7 +544,6 @@ void drm_dev_unregister(struct drm_device *dev); void drm_dev_ref(struct drm_device *dev); void drm_dev_unref(struct drm_device *dev); void drm_put_dev(struct drm_device *dev); -void drm_unplug_dev(struct drm_device *dev); int drm_dev_set_unique(struct drm_device *dev, const char *name); -- 2.1.4 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v8 1/2] drm: Unplug drm device when unregistering it 2017-04-12 2:55 ` [PATCH v8 1/2] drm: Unplug drm device when unregistering it Jeffy Chen @ 2017-04-26 19:43 ` Ville Syrjälä 2017-04-26 20:36 ` Sean Paul 0 siblings, 1 reply; 5+ messages in thread From: Ville Syrjälä @ 2017-04-26 19:43 UTC (permalink / raw) To: Jeffy Chen Cc: linux-kernel, Dave Airlie, briannorris, dianders, tfiga, dri-devel, Daniel Vetter, zyw, marcheu, hshi On Wed, Apr 12, 2017 at 10:55:29AM +0800, Jeffy Chen wrote: > After unbinding drm, the user space may still owns the drm dev fd, and > may still be able to call drm ioctl. > > We're using an unplugged state to prevent something like that, so let's > reuse it here. > > Also drop drm_unplug_dev, because it would be unused after other changes. > > Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com> > Reviewed-by: Sean Paul <seanpaul@chromium.org> > > --- > > Changes in v8: > Fix hang when unregistering drm dev with open_count 0 > > Changes in v7: > Address Sean Paul <seanpaul@chromium.org>'s comments. > > Changes in v6: > Address Daniel Vetter <daniel@ffwll.ch>'s comments. > > Changes in v5: > Fix wrong git account. > > Changes in v2: > Fix some commit messages. > > drivers/gpu/drm/drm_drv.c | 19 +++---------------- > drivers/gpu/drm/udl/udl_drv.c | 2 +- > include/drm/drmP.h | 5 +++-- > include/drm/drm_drv.h | 1 - > 4 files changed, 7 insertions(+), 20 deletions(-) > > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c > index b5c6bb4..cc2d018 100644 > --- a/drivers/gpu/drm/drm_drv.c > +++ b/drivers/gpu/drm/drm_drv.c > @@ -355,22 +355,6 @@ void drm_put_dev(struct drm_device *dev) > } > EXPORT_SYMBOL(drm_put_dev); > > -void drm_unplug_dev(struct drm_device *dev) > -{ > - /* for a USB device */ > - drm_dev_unregister(dev); > - > - mutex_lock(&drm_global_mutex); > - > - drm_device_set_unplugged(dev); > - > - if (dev->open_count == 0) { > - drm_put_dev(dev); > - } > - mutex_unlock(&drm_global_mutex); > -} > -EXPORT_SYMBOL(drm_unplug_dev); > - > /* > * DRM internal mount > * We want to be able to allocate our own "struct address_space" to control > @@ -787,6 +771,8 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags) > if (drm_core_check_feature(dev, DRIVER_MODESET)) > drm_modeset_register_all(dev); > > + drm_device_set_plug_state(dev, true); This makes me think this has something to do with actual plugs, be they the bath tub kind or some *ahem* other kind. /methinks this should at least be called set_plugged_state or something like that. Or maybe there's an even better name that could be used? > + > ret = 0; > > DRM_INFO("Initialized %s %d.%d.%d %s for %s on minor %d\n", > @@ -826,6 +812,7 @@ void drm_dev_unregister(struct drm_device *dev) > drm_lastclose(dev); > > dev->registered = false; > + drm_device_set_plug_state(dev, false); > > if (drm_core_check_feature(dev, DRIVER_MODESET)) > drm_modeset_unregister_all(dev); > diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c > index cd8b017..5dbd916 100644 > --- a/drivers/gpu/drm/udl/udl_drv.c > +++ b/drivers/gpu/drm/udl/udl_drv.c > @@ -108,7 +108,7 @@ static void udl_usb_disconnect(struct usb_interface *interface) > drm_kms_helper_poll_disable(dev); > udl_fbdev_unplug(dev); > udl_drop_usb(dev); > - drm_unplug_dev(dev); > + drm_dev_unregister(dev); > } > > /* > diff --git a/include/drm/drmP.h b/include/drm/drmP.h > index 3bfafcd..a9a5a64 100644 > --- a/include/drm/drmP.h > +++ b/include/drm/drmP.h > @@ -488,10 +488,11 @@ static __inline__ int drm_core_check_feature(struct drm_device *dev, > return ((dev->driver->driver_features & feature) ? 1 : 0); > } > > -static inline void drm_device_set_unplugged(struct drm_device *dev) > +static inline void drm_device_set_plug_state(struct drm_device *dev, > + bool plugged) > { > smp_wmb(); > - atomic_set(&dev->unplugged, 1); > + atomic_set(&dev->unplugged, !plugged); > } > > static inline int drm_device_is_unplugged(struct drm_device *dev) > diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h > index 0fefc3f..eb63078 100644 > --- a/include/drm/drm_drv.h > +++ b/include/drm/drm_drv.h > @@ -544,7 +544,6 @@ void drm_dev_unregister(struct drm_device *dev); > void drm_dev_ref(struct drm_device *dev); > void drm_dev_unref(struct drm_device *dev); > void drm_put_dev(struct drm_device *dev); > -void drm_unplug_dev(struct drm_device *dev); > > int drm_dev_set_unique(struct drm_device *dev, const char *name); > > -- > 2.1.4 > > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Ville Syrjälä Intel OTC ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v8 1/2] drm: Unplug drm device when unregistering it 2017-04-26 19:43 ` Ville Syrjälä @ 2017-04-26 20:36 ` Sean Paul 0 siblings, 0 replies; 5+ messages in thread From: Sean Paul @ 2017-04-26 20:36 UTC (permalink / raw) To: Ville Syrjälä Cc: Jeffy Chen, linux-kernel, Dave Airlie, briannorris, dianders, tfiga, dri-devel, Daniel Vetter, zyw, marcheu, hshi On Wed, Apr 26, 2017 at 10:43:31PM +0300, Ville Syrjälä wrote: > On Wed, Apr 12, 2017 at 10:55:29AM +0800, Jeffy Chen wrote: > > After unbinding drm, the user space may still owns the drm dev fd, and > > may still be able to call drm ioctl. > > > > We're using an unplugged state to prevent something like that, so let's > > reuse it here. > > > > Also drop drm_unplug_dev, because it would be unused after other changes. > > > > Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com> > > Reviewed-by: Sean Paul <seanpaul@chromium.org> > > > > --- > > > > Changes in v8: > > Fix hang when unregistering drm dev with open_count 0 > > > > Changes in v7: > > Address Sean Paul <seanpaul@chromium.org>'s comments. > > > > Changes in v6: > > Address Daniel Vetter <daniel@ffwll.ch>'s comments. > > > > Changes in v5: > > Fix wrong git account. > > > > Changes in v2: > > Fix some commit messages. > > > > drivers/gpu/drm/drm_drv.c | 19 +++---------------- > > drivers/gpu/drm/udl/udl_drv.c | 2 +- > > include/drm/drmP.h | 5 +++-- > > include/drm/drm_drv.h | 1 - > > 4 files changed, 7 insertions(+), 20 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c > > index b5c6bb4..cc2d018 100644 > > --- a/drivers/gpu/drm/drm_drv.c > > +++ b/drivers/gpu/drm/drm_drv.c > > @@ -355,22 +355,6 @@ void drm_put_dev(struct drm_device *dev) > > } > > EXPORT_SYMBOL(drm_put_dev); > > > > -void drm_unplug_dev(struct drm_device *dev) > > -{ > > - /* for a USB device */ > > - drm_dev_unregister(dev); > > - > > - mutex_lock(&drm_global_mutex); > > - > > - drm_device_set_unplugged(dev); > > - > > - if (dev->open_count == 0) { > > - drm_put_dev(dev); > > - } > > - mutex_unlock(&drm_global_mutex); > > -} > > -EXPORT_SYMBOL(drm_unplug_dev); > > - > > /* > > * DRM internal mount > > * We want to be able to allocate our own "struct address_space" to control > > @@ -787,6 +771,8 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags) > > if (drm_core_check_feature(dev, DRIVER_MODESET)) > > drm_modeset_register_all(dev); > > > > + drm_device_set_plug_state(dev, true); > > This makes me think this has something to do with actual plugs, be > they the bath tub kind or some *ahem* other kind. > > /methinks this should at least be called set_plugged_state or > something like that. Or maybe there's an even better name that > could be used? thanks for reviewing this, Ville. fwiw, we decided this patch wasn't worth carrying upstream (see my response to v11 in <20170414151503.lmpp3udfuycavfki@art_vandelay>). Sean > > > + > > ret = 0; > > > > DRM_INFO("Initialized %s %d.%d.%d %s for %s on minor %d\n", > > @@ -826,6 +812,7 @@ void drm_dev_unregister(struct drm_device *dev) > > drm_lastclose(dev); > > > > dev->registered = false; > > + drm_device_set_plug_state(dev, false); > > > > if (drm_core_check_feature(dev, DRIVER_MODESET)) > > drm_modeset_unregister_all(dev); > > diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c > > index cd8b017..5dbd916 100644 > > --- a/drivers/gpu/drm/udl/udl_drv.c > > +++ b/drivers/gpu/drm/udl/udl_drv.c > > @@ -108,7 +108,7 @@ static void udl_usb_disconnect(struct usb_interface *interface) > > drm_kms_helper_poll_disable(dev); > > udl_fbdev_unplug(dev); > > udl_drop_usb(dev); > > - drm_unplug_dev(dev); > > + drm_dev_unregister(dev); > > } > > > > /* > > diff --git a/include/drm/drmP.h b/include/drm/drmP.h > > index 3bfafcd..a9a5a64 100644 > > --- a/include/drm/drmP.h > > +++ b/include/drm/drmP.h > > @@ -488,10 +488,11 @@ static __inline__ int drm_core_check_feature(struct drm_device *dev, > > return ((dev->driver->driver_features & feature) ? 1 : 0); > > } > > > > -static inline void drm_device_set_unplugged(struct drm_device *dev) > > +static inline void drm_device_set_plug_state(struct drm_device *dev, > > + bool plugged) > > { > > smp_wmb(); > > - atomic_set(&dev->unplugged, 1); > > + atomic_set(&dev->unplugged, !plugged); > > } > > > > static inline int drm_device_is_unplugged(struct drm_device *dev) > > diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h > > index 0fefc3f..eb63078 100644 > > --- a/include/drm/drm_drv.h > > +++ b/include/drm/drm_drv.h > > @@ -544,7 +544,6 @@ void drm_dev_unregister(struct drm_device *dev); > > void drm_dev_ref(struct drm_device *dev); > > void drm_dev_unref(struct drm_device *dev); > > void drm_put_dev(struct drm_device *dev); > > -void drm_unplug_dev(struct drm_device *dev); > > > > int drm_dev_set_unique(struct drm_device *dev, const char *name); > > > > -- > > 2.1.4 > > > > > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > -- > Ville Syrjälä > Intel OTC -- Sean Paul, Software Engineer, Google / Chromium OS ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v8 2/2] drm: Prevent release fb after cleanup drm_mode_config 2017-04-12 2:55 [PATCH v8 0/2] drm: rockchip: Fix rockchip drm unbind crash error Jeffy Chen 2017-04-12 2:55 ` [PATCH v8 1/2] drm: Unplug drm device when unregistering it Jeffy Chen @ 2017-04-12 2:55 ` Jeffy Chen 1 sibling, 0 replies; 5+ messages in thread From: Jeffy Chen @ 2017-04-12 2:55 UTC (permalink / raw) To: linux-kernel Cc: briannorris, dianders, tfiga, seanpaul, zyw, marcheu, mark.yao, hshi, Jeffy Chen, Daniel Vetter, Jani Nikula, dri-devel, David Airlie We are freeing all framebuffers in drm_mode_config_cleanup without sync the drm_file's fbs list. So if someone try to unbind drm before release drm dev fd, the fbs list would remain some invalid fb references. And that would cause crash later in drm_fb_release. Add a sanity check to prevent that. Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com> --- Changes in v8: None Changes in v7: Update commit message. Changes in v6: None Changes in v5: None Changes in v2: None drivers/gpu/drm/drm_framebuffer.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c index e8f9c13..03c1632 100644 --- a/drivers/gpu/drm/drm_framebuffer.c +++ b/drivers/gpu/drm/drm_framebuffer.c @@ -583,6 +583,11 @@ void drm_fb_release(struct drm_file *priv) { struct drm_framebuffer *fb, *tfb; struct drm_mode_rmfb_work arg; + struct drm_minor *minor = priv->minor; + struct drm_device *dev = minor->dev; + + if (WARN_ON(!dev->mode_config.num_fb && !list_empty(&priv->fbs))) + return; INIT_LIST_HEAD(&arg.fbs); -- 2.1.4 ^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-04-26 20:36 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-04-12 2:55 [PATCH v8 0/2] drm: rockchip: Fix rockchip drm unbind crash error Jeffy Chen 2017-04-12 2:55 ` [PATCH v8 1/2] drm: Unplug drm device when unregistering it Jeffy Chen 2017-04-26 19:43 ` Ville Syrjälä 2017-04-26 20:36 ` Sean Paul 2017-04-12 2:55 ` [PATCH v8 2/2] drm: Prevent release fb after cleanup drm_mode_config Jeffy Chen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).