Re: [PATCH v6 02/10] Revert "drm/qxl: do not run release if qxl failed to init"

2021-02-04 Thread Tong Zhang 


> On Feb 4, 2021, at 2:06 PM, Thomas Zimmermann  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

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v6 02/10] Revert "drm/qxl: do not run release if qxl failed to init"

2021-02-04 Thread 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() -

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  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 
>> Signed-off-by: Gerd Hoffmann 
>> ---
>>  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 
> 
>>  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
> 

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v6 02/10] Revert "drm/qxl: do not run release if qxl failed to init"

2021-02-04 Thread Thomas Zimmermann 

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  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 
Signed-off-by: Gerd Hoffmann 
---
  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 


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



OpenPGP_signature
Description: OpenPGP digital signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v6 02/10] Revert "drm/qxl: do not run release if qxl failed to init"

2021-02-04 Thread Thomas Zimmermann 

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 
Signed-off-by: Gerd Hoffmann 
---
  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 


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



OpenPGP_signature
Description: OpenPGP digital signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel