On Wed, Apr 8, 2020 at 10:41 AM james qian wang (Arm Technology China) james.qian.wang@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@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@intel.com Cc: "James (Qian) Wang" james.qian.wang@arm.com Cc: Liviu Dudau liviu.dudau@arm.com Cc: Mihail Atanassov mihail.atanassov@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