[RFC 1/7] drm/omap: Use devm_kzalloc() to allocate omap_drm_private

[Peter Ujfalusi]


Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

On 2017-09-01 14:10, Laurent Pinchart wrote:
> Hi Peter,
> 
> Thank you for the patch.
> 
> On Tuesday, 29 August 2017 10:32:12 EEST Peter Ujfalusi wrote:
>> It makes the cleanup paths a bit cleaner.
> 
> But it also goes against a proper implementation of object lifetime management 
> :-( In DRM driver private structures are accessible from file operations, and 
> must thus be reference-counted and only freed when the last userspace user 
> goes away. This may be after the devm_* resources are released right after the 
> .remove() operation is called. Using devm_kzalloc() would thus make it 
> impossible to properly reference-count our data structures.

Hrm, not sure if I follow you on this.
Before this patch the 'priv' was freed up in pdev_remove(). With this
patch the 'priv' is going to be freed up after the pdev_remove() has
returned, so the 'priv' would be available longer than before as we
converted it as managed resource.

If what you are saying is true, then we already have the same issue and
I don't see how this could have been working or need to work. If you
remove the driver and you need to keep the private data available after
the driver has been unloaded, who is going to free that up?

> 
>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi at ti.com>
>> ---
>>  drivers/gpu/drm/omapdrm/omap_drv.c | 19 +++++++------------
>>  1 file changed, 7 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c
>> b/drivers/gpu/drm/omapdrm/omap_drv.c index 9b3c36b48356..903510d8054e
>> 100644
>> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
>> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
>> @@ -548,19 +548,17 @@ static int pdev_probe(struct platform_device *pdev)
>>  		return ret;
>>  	}
>>
>> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
>> +	if (!priv)
>> +		return -ENOMEM;
>> +
>>  	omap_crtc_pre_init();
>>
>>  	ret = omap_connect_dssdevs();
>>  	if (ret)
>>  		goto err_crtc_uninit;
>>
>> -	/* Allocate and initialize the driver private structure. */
>> -	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
>> -	if (!priv) {
>> -		ret = -ENOMEM;
>> -		goto err_disconnect_dssdevs;
>> -	}
>> -
>> +	/* Initialize the driver private structure. */
>>  	priv->dispc_ops = dispc_get_ops();
>>
>>  	soc = soc_device_match(omapdrm_soc_devices);
>> @@ -574,7 +572,7 @@ static int pdev_probe(struct platform_device *pdev)
>>  	ddev = drm_dev_alloc(&omap_drm_driver, &pdev->dev);
>>  	if (IS_ERR(ddev)) {
>>  		ret = PTR_ERR(ddev);
>> -		goto err_free_priv;
>> +		goto err_destroy_wq;
>>  	}
>>
>>  	ddev->dev_private = priv;
>> @@ -624,10 +622,8 @@ static int pdev_probe(struct platform_device *pdev)
>>  err_free_drm_dev:
>>  	omap_gem_deinit(ddev);
>>  	drm_dev_unref(ddev);
>> -err_free_priv:
>> +err_destroy_wq:
>>  	destroy_workqueue(priv->wq);
>> -	kfree(priv);
>> -err_disconnect_dssdevs:
>>  	omap_disconnect_dssdevs();
>>  err_crtc_uninit:
>>  	omap_crtc_pre_uninit();
>> @@ -659,7 +655,6 @@ static int pdev_remove(struct platform_device *pdev)
>>  	drm_dev_unref(ddev);
>>
>>  	destroy_workqueue(priv->wq);
>> -	kfree(priv);
>>
>>  	omap_disconnect_dssdevs();
>>  	omap_crtc_pre_uninit();
> 
> 

- Péter