| Message ID | 20210204145712.1531203-3-kraxel@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/qxl: fix driver shutdown issues. | expand |
Hi Am 04.02.21 um 15:57 schrieb Gerd Hoffmann: > This reverts commit b91907a6241193465ca92e357adf16822242296d. This should be in the correct format, as given by 'dim cite'. dim cite b91907a6241193465ca92e357adf16822242296d b91907a62411 ("drm/qxl: do not run release if qxl failed to init") > > Patch is broken, it effectively makes qxl_drm_release() a nop > because on normal driver shutdown qxl_drm_release() is called > *after* drm_dev_unregister(). > > Cc: Tong Zhang <ztong0001@gmail.com> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > --- > drivers/gpu/drm/qxl/qxl_drv.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c > index 34c8b25b5780..fb5f6a5e81d7 100644 > --- a/drivers/gpu/drm/qxl/qxl_drv.c > +++ b/drivers/gpu/drm/qxl/qxl_drv.c > @@ -144,8 +144,6 @@ static void qxl_drm_release(struct drm_device *dev) > * reodering qxl_modeset_fini() + qxl_device_fini() calls is > * non-trivial though. > */ > - if (!dev->registered) > - return; I'm not sure what the original problem was, but I'm sure that this isn't the fix for it. If there's a problem with shutdown, the operations rather have to be reordered correctly. With the citation style address: Acked-by: Thomas Zimmermann <tzimmermann@suse.de> > qxl_modeset_fini(qdev); > qxl_device_fini(qdev); > } >
Hi Thomas, The original problem was qxl_device_init() can fail, when it fails there is no need to call qxl_modeset_fini(qdev); qxl_device_fini(qdev); But those two functions are otherwise called in the qxl_drm_release() - I have posted an updated patch. The new patch use the following logic + if (!qdev->ddev.mode_config.funcs) + return; qxl_modeset_fini(qdev); qxl_device_fini(qdev); Thanks, - Tong > On Feb 4, 2021, at 1:34 PM, Thomas Zimmermann <tzimmermann@suse.de> wrote: > > Hi > > Am 04.02.21 um 15:57 schrieb Gerd Hoffmann: >> This reverts commit b91907a6241193465ca92e357adf16822242296d. > > This should be in the correct format, as given by 'dim cite'. > > dim cite b91907a6241193465ca92e357adf16822242296d > b91907a62411 ("drm/qxl: do not run release if qxl failed to init") > >> Patch is broken, it effectively makes qxl_drm_release() a nop >> because on normal driver shutdown qxl_drm_release() is called >> *after* drm_dev_unregister(). >> Cc: Tong Zhang <ztong0001@gmail.com> >> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> >> --- >> drivers/gpu/drm/qxl/qxl_drv.c | 2 -- >> 1 file changed, 2 deletions(-) >> diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c >> index 34c8b25b5780..fb5f6a5e81d7 100644 >> --- a/drivers/gpu/drm/qxl/qxl_drv.c >> +++ b/drivers/gpu/drm/qxl/qxl_drv.c >> @@ -144,8 +144,6 @@ static void qxl_drm_release(struct drm_device *dev) >> * reodering qxl_modeset_fini() + qxl_device_fini() calls is >> * non-trivial though. >> */ >> - if (!dev->registered) >> - return; > > I'm not sure what the original problem was, but I'm sure that this isn't the fix for it. If there's a problem with shutdown, the operations rather have to be reordered correctly. > > With the citation style address: > > Acked-by: Thomas Zimmermann <tzimmermann@suse.de> > >> qxl_modeset_fini(qdev); >> qxl_device_fini(qdev); >> } > > -- > Thomas Zimmermann > Graphics Driver Developer > SUSE Software Solutions Germany GmbH > Maxfeldstr. 5, 90409 Nürnberg, Germany > (HRB 36809, AG Nürnberg) > Geschäftsführer: Felix Imendörffer >
Hi Tong Am 04.02.21 um 19:52 schrieb Tong Zhang: > Hi Thomas, > > The original problem was qxl_device_init() can fail, > when it fails there is no need to call > qxl_modeset_fini(qdev); > qxl_device_fini(qdev); > But those two functions are otherwise called in the qxl_drm_release() - OK, makes sense. Thanks for the explanation. > > I have posted an updated patch. > The new patch use the following logic > > + if (!qdev->ddev.mode_config.funcs) > + return; This is again just papering over the issue. Better don't call qxl_drm_release() in the error path if qxl_device_init() fails. I see two solutions: either roll-back manually, or use our new managed DRM interfaces. This is what the other drivers do. Best regards Thomas > qxl_modeset_fini(qdev); > qxl_device_fini(qdev); > > Thanks, > - Tong > > >> On Feb 4, 2021, at 1:34 PM, Thomas Zimmermann <tzimmermann@suse.de> wrote: >> >> Hi >> >> Am 04.02.21 um 15:57 schrieb Gerd Hoffmann: >>> This reverts commit b91907a6241193465ca92e357adf16822242296d. >> >> This should be in the correct format, as given by 'dim cite'. >> >> dim cite b91907a6241193465ca92e357adf16822242296d >> b91907a62411 ("drm/qxl: do not run release if qxl failed to init") >> >>> Patch is broken, it effectively makes qxl_drm_release() a nop >>> because on normal driver shutdown qxl_drm_release() is called >>> *after* drm_dev_unregister(). >>> Cc: Tong Zhang <ztong0001@gmail.com> >>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> >>> --- >>> drivers/gpu/drm/qxl/qxl_drv.c | 2 -- >>> 1 file changed, 2 deletions(-) >>> diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c >>> index 34c8b25b5780..fb5f6a5e81d7 100644 >>> --- a/drivers/gpu/drm/qxl/qxl_drv.c >>> +++ b/drivers/gpu/drm/qxl/qxl_drv.c >>> @@ -144,8 +144,6 @@ static void qxl_drm_release(struct drm_device *dev) >>> * reodering qxl_modeset_fini() + qxl_device_fini() calls is >>> * non-trivial though. >>> */ >>> - if (!dev->registered) >>> - return; >> >> I'm not sure what the original problem was, but I'm sure that this isn't the fix for it. If there's a problem with shutdown, the operations rather have to be reordered correctly. >> >> With the citation style address: >> >> Acked-by: Thomas Zimmermann <tzimmermann@suse.de> >> >>> qxl_modeset_fini(qdev); >>> qxl_device_fini(qdev); >>> } >> >> -- >> Thomas Zimmermann >> Graphics Driver Developer >> SUSE Software Solutions Germany GmbH >> Maxfeldstr. 5, 90409 Nürnberg, Germany >> (HRB 36809, AG Nürnberg) >> Geschäftsführer: Felix Imendörffer >> >
> On Feb 4, 2021, at 2:06 PM, Thomas Zimmermann <tzimmermann@suse.de> wrote: > > Hi Tong > >> I have posted an updated patch. >> The new patch use the following logic >> + if (!qdev->ddev.mode_config.funcs) >> + return; > > This is again just papering over the issue. Better don't call qxl_drm_release() in the error path if qxl_device_init() fails. > > I see two solutions: either roll-back manually, or use our new managed DRM interfaces. This is what the other drivers do. > > Best regards > Thomas IMHO - qdev->ddev.mode_config.funcs is set only if the initialization is successful, so using this as an indicator of error case looks ok to me. The other two options you suggested would require some serious significant amount of work to be done, which I don’t think I currently have such ability to do. Thanks, - Tong
diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c index 34c8b25b5780..fb5f6a5e81d7 100644 --- a/drivers/gpu/drm/qxl/qxl_drv.c +++ b/drivers/gpu/drm/qxl/qxl_drv.c @@ -144,8 +144,6 @@ static void qxl_drm_release(struct drm_device *dev) * reodering qxl_modeset_fini() + qxl_device_fini() calls is * non-trivial though. */ - if (!dev->registered) - return; qxl_modeset_fini(qdev); qxl_device_fini(qdev); }
This reverts commit b91907a6241193465ca92e357adf16822242296d. Patch is broken, it effectively makes qxl_drm_release() a nop because on normal driver shutdown qxl_drm_release() is called *after* drm_dev_unregister(). Cc: Tong Zhang <ztong0001@gmail.com> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- drivers/gpu/drm/qxl/qxl_drv.c | 2 -- 1 file changed, 2 deletions(-)