Hi Peter,
On Monday, 4 September 2017 12:13:40 EEST Peter Ujfalusi wrote:
> On 2017-09-01 14:10, Laurent Pinchart wrote:
> > 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?
At the moment the memory is freed at .remove() time, which can lead to memory
corruption if a user has a handle on the device (for instance an open file
handle that is then close()d). Fixing this requires moving memory free to the
drm_driver::release() handler. devm_kzalloc() goes in the wrong direction.
> >> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@xxxxxx>
> >> ---
> >>
> >> 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();
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
