| Message ID | 20170528171655.25424-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@chris-wilson.co.uk> > Cc: Marco Diego Aurélio Mesquita <marcodiegomesquita@gmail.com> > Reported-by: Marco Diego Aurélio Mesquita <marcodiegomesquita@gmail.com> > Signed-off-by: Hans de Goede <hdegoede@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? Does this need a bugreport link? -Daniel > --- > 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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Mon, May 29, 2017 at 09:02:38PM +0200, 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@chris-wilson.co.uk> > > Cc: Marco Diego Aurélio Mesquita <marcodiegomesquita@gmail.com> > > Reported-by: Marco Diego Aurélio Mesquita <marcodiegomesquita@gmail.com> > > Signed-off-by: Hans de Goede <hdegoede@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? Does this need a > bugreport link? One more: When fixing a regression pls cc author/reviewer of the offending patch, for the learning. Adding Chris. -Daniel
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@chris-wilson.co.uk> > Cc: Marco Diego Aurélio Mesquita <marcodiegomesquita@gmail.com> > Reported-by: Marco Diego Aurélio Mesquita <marcodiegomesquita@gmail.com> > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > Note I don't have any USB display devices at hand for testing atm so > this patch has only been compile tested. Not tested it either, but yes we do end up calling drm_device_unregister() twice and indeed that looks bad. This morning I was thinking about kselftests to simulate the different conditions and avoid us having to get our hands dirty with real hw. It also reminded me of https://patchwork.kernel.org/patch/9678823/ which has the tantalising prospect of removing drm_unplug_dev() entirely. Another benefit is the removal of one more call to drm_put_dev() which has been deprecated for a while. The patch definitely fixes a bug at first glance, but I wonder if it is really telling me that the call to drm_device_unregister() here is the fundamental issue. But I am short of actual answers. -Chris
On Mon, May 29, 2017 at 09:04:07PM +0200, Daniel Vetter wrote: > On Mon, May 29, 2017 at 09:02:38PM +0200, 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@chris-wilson.co.uk> > One more: When fixing a regression pls cc author/reviewer of the offending > patch, for the learning. Adding Chris. I was CC'ed, the ml stripping headers again? -Chris
On 30/05/17 05:13 AM, Chris Wilson wrote: > On Mon, May 29, 2017 at 09:04:07PM +0200, Daniel Vetter wrote: >> On Mon, May 29, 2017 at 09:02:38PM +0200, 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@chris-wilson.co.uk> > >> One more: When fixing a regression pls cc author/reviewer of the offending >> patch, for the learning. Adding Chris. > > I was CC'ed, the ml stripping headers again? If you have the "Avoid duplicate copies of messages?" option enabled in your dri-devel mailing list membership configuration, mailman will remove you from Cc in the posts it distributes to subscribers. Consider disabling that option.
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@chris-wilson.co.uk> >> Cc: Marco Diego Aurélio Mesquita <marcodiegomesquita@gmail.com> >> Reported-by: Marco Diego Aurélio Mesquita <marcodiegomesquita@gmail.com> >> Signed-off-by: Hans de Goede <hdegoede@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. 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@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel >
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@chris-wilson.co.uk> >>> Cc: Marco Diego Aurélio Mesquita <marcodiegomesquita@gmail.com> >>> Reported-by: Marco Diego Aurélio Mesquita <marcodiegomesquita@gmail.com> >>> Signed-off-by: Hans de Goede <hdegoede@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@lists.freedesktop.org >>> https://lists.freedesktop.org/mailman/listinfo/dri-devel >>
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);
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@chris-wilson.co.uk> Cc: Marco Diego Aurélio Mesquita <marcodiegomesquita@gmail.com> Reported-by: Marco Diego Aurélio Mesquita <marcodiegomesquita@gmail.com> Signed-off-by: Hans de Goede <hdegoede@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 --- drivers/gpu/drm/drm_drv.c | 6 +++--- drivers/gpu/drm/drm_file.c | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-)