Thomas Zimmermann
2021-Feb-04 19:06 UTC
[PATCH v6 02/10] Revert "drm/qxl: do not run release if qxl failed to init"
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 at 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 at gmail.com> >>> Signed-off-by: Gerd Hoffmann <kraxel at 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 at 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 >> >-- 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 -------------- next part -------------- A non-text attachment was scrubbed... Name: OpenPGP_signature Type: application/pgp-signature Size: 840 bytes Desc: OpenPGP digital signature URL: <http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20210204/6b4ebcfe/attachment.sig>