Re: [PATCH 36/44] drm/komeda: use devm_drm_dev_alloc

["james qian wang (Arm Technology China)" <james.qian.wang@arm.com>]

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.

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel