[Intel-gfx] [PATCH 36/44] drm/komeda: use devm_drm_dev_alloc

[Daniel Vetter]

On Wed, Apr 8, 2020 at 10:41 AM james qian wang (Arm Technology China)
<james.qian.wang at arm.com> wrote:
>
> On Fri, Apr 03, 2020 at 09:58:20PM +0800, Daniel Vetter wrote:
> > Komeda uses the component framework, which does open/close a new
> > devres group around all the bind callbacks. Which means we can use
> > devm_ functions for managing the drm_device cleanup, with leaking
> > stuff in case of deferred probes or other reasons to unbind
> > components, or the component_master.
> >
> > Also note that this fixes a double-free in the probe unroll code, bot
> > drm_dev_put and kfree(kms) result in the kms allocation getting freed.
> >
> > Aside: komeda_bind could be cleaned up a lot, devm_kfree is a bit
> > redundant. Plus I'm not clear on why there's suballocations for
> > mdrv->mdev and mdrv->kms. Plus I'm not sure the lifetimes are correct
> > with all that devm_kzalloc usage ... That structure layout is also the
> > reason why komeda still uses drm_device->dev_private and can't easily
> > be replaced with a proper container_of upcasting. I'm pretty sure that
> > there's endless amounts of hotunplug/hotremove bugs in there with all
> > the unprotected dereferencing of drm_device->dev_private.
>
> Hi Daniel:
>
> Thank you for the patch.
>
> Reviewed-by: James Qian Wang <james.qian.wang at arm.com>
>
> For why komeda has two devices komeda_dev and komeda_kms_dev.
> That because komeda treats drm_crtc/plane as virtual, which pick the real
> komeda resources to satify the user's requirement. In the initial driver
> design we want to clear the difference of these two class structures
> so we defined two devices:
>
> - komeda_dev:
>   managing the real komeda device and resources.
>
> - komeda_kms_dev
>   the virtual device managing the drm related stuff like
>   komeda_crtc/plane.
>
> And yes, even for that we don't need two sub-allocations, we are planing
> to move the komeda_dev into the komeda_kms_dev or just merge two devices
> together.

Yeah I think the logical split makes a lot of sense, e.g. amdgpu has a
similar split between the drm front-end structures, and the DC
back-end stuff that deals with the hardware. Same as other drivers.
The issue I think I'm seeing with komeda is all the pointer chasing
(not a problem, just a bit of indirection that's not strictly needed),
and that when you unload the back-end disappears right away, while the
front-end might still be used by userspace. And then all the pointers
you have lead to oopses.

I admit that for built-in hw hotunplug isn't really a major use-case.
But we do have some usb drivers nowadays in drm, so I'm trying to
clean this all up a bit and make sure that as many drivers as possible
have clean&correct code in this area.

Anyway if you're already planning to look into this then awesome!

Cheers, Daniel

> Thanks
> James
>
> > Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> > Cc: "James (Qian) Wang" <james.qian.wang at arm.com>
> > Cc: Liviu Dudau <liviu.dudau at arm.com>
> > Cc: Mihail Atanassov <mihail.atanassov at arm.com>
> > ---
> >  drivers/gpu/drm/arm/display/komeda/komeda_kms.c | 16 +++++-----------
> >  1 file changed, 5 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> > index 16dfd5cdb66c..6b85d5f4caa8 100644
> > --- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> > @@ -261,18 +261,16 @@ static void komeda_kms_mode_config_init(struct komeda_kms_dev *kms,
> >
> >  struct komeda_kms_dev *komeda_kms_attach(struct komeda_dev *mdev)
> >  {
> > -     struct komeda_kms_dev *kms = kzalloc(sizeof(*kms), GFP_KERNEL);
> > +     struct komeda_kms_dev *kms;
> >       struct drm_device *drm;
> >       int err;
> >
> > -     if (!kms)
> > -             return ERR_PTR(-ENOMEM);
> > +     kms = devm_drm_dev_alloc(mdev->dev, &komeda_kms_driver,
> > +                              struct komeda_kms_dev, base);
> > +     if (IS_ERR(kms))
> > +             return kms;
> >
> >       drm = &kms->base;
> > -     err = drm_dev_init(drm, &komeda_kms_driver, mdev->dev);
> > -     if (err)
> > -             goto free_kms;
> > -     drmm_add_final_kfree(drm, kms);
> >
> >       drm->dev_private = mdev;
> >
> > @@ -329,9 +327,6 @@ struct komeda_kms_dev *komeda_kms_attach(struct komeda_dev *mdev)
> >       drm_mode_config_cleanup(drm);
> >       komeda_kms_cleanup_private_objs(kms);
> >       drm->dev_private = NULL;
> > -     drm_dev_put(drm);
> > -free_kms:
> > -     kfree(kms);
> >       return ERR_PTR(err);
> >  }
> >
> > @@ -348,5 +343,4 @@ void komeda_kms_detach(struct komeda_kms_dev *kms)
> >       drm_mode_config_cleanup(drm);
> >       komeda_kms_cleanup_private_objs(kms);
> >       drm->dev_private = NULL;
> > -     drm_dev_put(drm);
> >  }
> > --
> > 2.25.1



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch