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

[Peter Ujfalusi]

Hi Laurent,


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

On 2017-09-04 17:19, Laurent Pinchart wrote:
>> Ah, OK, so the current way is buggy as well.
>>
>> How do you plan to fix that?
>> I think this should work:
>>
>> struct omap_drm_private {
>> 	/* First member in the private struct! */
>> +	struct drm_device ddev;
>> ...
>> };
>>
>> Use drm_dev_init(&priv->ddev, ...); to initialize the drm_device instead
>> of drm_dev_alloc()
>>
>> then priv->ddev.dev_private = priv;
>>
>> in this case the drm_dev_unref() would free up our omap_drm_private, right?
> 
> That's the idea, yes. I got a local patch for that in my tree.

Hrm, so what is the difference between what I do in this patch and the
described fix?
To be precise what is the difference between:

struct drm_device *ddev = platform_get_drvdata(pdev);
struct omap_drm_private *priv = ddev->dev_private;
...
drm_dev_unref(ddev);
...
devm would free priv

compared to

struct omap_drm_private {
	/* First member in the private struct! */
	struct drm_device ddev;
	....
};

struct drm_device *ddev = platform_get_drvdata(pdev);
struct omap_drm_private *priv = ddev->dev_private;
...
/* Here we would free priv since &priv->ddev == ddev */
drm_dev_unref(ddev);


The drm_dev_unregister() is provided as a protection for the issue you
are describing, the comment suggest that at least:
 * This should be called first in the device teardown code to make sure
 * userspace can't access the device instance any more.

and we do call it. As the first step.


> 
>> I think this is what other DRM drivers are doing, not all, but i915 does
>> this at least.
>>
>> But by the description most of the DRM drivers are doing this wrong, right?
> 
> Correct, most drivers get it wrong. We'll have to fix it, but given that we 
> have race conditions in the core that prevent proper hot-unplug support at the 
> moment, I didn't want to start pushing for fixing drivers. Once we get the 
> core sorted out, it will be time to address the other side of the issue.
> 

- Péter