Re: [PATCH 36/44] drm/komeda: use devm_drm_dev_alloc
On Wed, Apr 8, 2020 at 10:41 AM james qian wang (Arm Technology China)
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
>
> 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 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
> > Cc: "James (Qian) Wang"
> > Cc: Liviu Dudau
> > Cc: Mihail Atanassov
> > ---
> > 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, _kms_driver,
> > + struct komeda_kms_dev, base);
> > + if (IS_ERR(kms))
> > + return kms;
> >
> > drm = >base;
> > - err = drm_dev_init(drm, _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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 36/44] drm/komeda: use devm_drm_dev_alloc
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
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.
Thanks
James
> Signed-off-by: Daniel Vetter
> Cc: "James (Qian) Wang"
> Cc: Liviu Dudau
> Cc: Mihail Atanassov
> ---
> 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, _kms_driver,
> + struct komeda_kms_dev, base);
> + if (IS_ERR(kms))
> + return kms;
>
> drm = >base;
> - err = drm_dev_init(drm, _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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 36/44] drm/komeda: use devm_drm_dev_alloc
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.
Signed-off-by: Daniel Vetter
Cc: "James (Qian) Wang"
Cc: Liviu Dudau
Cc: Mihail Atanassov
---
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, _kms_driver,
+struct komeda_kms_dev, base);
+ if (IS_ERR(kms))
+ return kms;
drm = >base;
- err = drm_dev_init(drm, _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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
