Re: [PATCH v2] drm/msm/dpu: Don't use devm for component devices

[Jordan Crouse <jcrouse-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>]

On Mon, Nov 05, 2018 at 07:54:52AM +0100, Andrzej Hajda wrote:
> On 02.11.2018 19:25, Jordan Crouse wrote:
> > Devices that are bound as components should not use devm since
> > device managed memory is not freed when the component is
> > unbound.
> >
> > In particular this is an issue if the compoent bind fails
> > due to an -EPROBE_DEFER. In this case the bind would try again
> > later and any devm managed meory allocated during the former
> > aborted attempt would be leaked until the device itself was
> > destroyed.  Therefore, all the memory allocated during a bind
> > should be freed during an unbind (or bind error case) and there
> > isn't any reason to use devm for resources that have a explicit
> > teardown step.
> 
> It does not look correct, component framework should properly free devm
> resources, ie if devm resource was allocated in bind callback, it should
> be released after unbind or failed bind.
> See comments inside component_bind[1]. If it does not work it should be
> fixed in components.

I looked again - you are right.  There is a devres_release_group() in the
failure path. I'll rework this patch to remove the devm release calls instead
and rely on that.

Thanks,
Jordan

> [1]:
> https://elixir.bootlin.com/linux/latest/source/drivers/base/component.c#L464
> 
> Regards
> Andrzej
> 
> 
> 
> >
> > This doesn't remove devm for all resources - in particular
> > msm_ioremap() still uses devm_ioremap() but thats a more
> > generic condition that can easily be addressed as a cleanup
> > later and the unbind code already does the requisite devm
> > calls to unmap it.
> >
> > v2: free mp->clk_config on failure
> >
> > Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org>
> > ---
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c |  4 ++--
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.c |  6 +++---
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     | 10 ++++++----
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c    |  8 +++++---
> >  4 files changed, 16 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > index 82c55efb500f..287d4c3e58c3 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > @@ -2220,14 +2220,14 @@ struct drm_encoder *dpu_encoder_init(struct drm_device *dev,
> >  	struct dpu_encoder_virt *dpu_enc = NULL;
> >  	int rc = 0;
> >  
> > -	dpu_enc = devm_kzalloc(dev->dev, sizeof(*dpu_enc), GFP_KERNEL);
> > +	dpu_enc = kzalloc(sizeof(*dpu_enc), GFP_KERNEL);
> >  	if (!dpu_enc)
> >  		return ERR_PTR(ENOMEM);
> >  
> >  	rc = drm_encoder_init(dev, &dpu_enc->base, &dpu_encoder_funcs,
> >  			drm_enc_mode, NULL);
> >  	if (rc) {
> > -		devm_kfree(dev->dev, dpu_enc);
> > +		kfree(dpu_enc);
> >  		return ERR_PTR(rc);
> >  	}
> >  
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.c
> > index 89ee4b36beff..14fecf00e032 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.c
> > @@ -155,9 +155,7 @@ int msm_dss_parse_clock(struct platform_device *pdev,
> >  		return 0;
> >  	}
> >  
> > -	mp->clk_config = devm_kzalloc(&pdev->dev,
> > -				      sizeof(struct dss_clk) * num_clk,
> > -				      GFP_KERNEL);
> > +	mp->clk_config = kcalloc(num_clk, sizeof(struct dss_clk), GFP_KERNEL);
> >  	if (!mp->clk_config)
> >  		return -ENOMEM;
> >  
> > @@ -201,5 +199,7 @@ int msm_dss_parse_clock(struct platform_device *pdev,
> >  
> >  err:
> >  	msm_dss_put_clk(mp->clk_config, num_clk);
> > +	kfree(mp->clk_config);
> > +	mp->clk_config = NULL;
> >  	return rc;
> >  }
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > index 985c855796ae..5ac3c3f3b08d 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > @@ -1086,13 +1086,14 @@ static int dpu_bind(struct device *dev, struct device *master, void *data)
> >  	struct dss_module_power *mp;
> >  	int ret = 0;
> >  
> > -	dpu_kms = devm_kzalloc(&pdev->dev, sizeof(*dpu_kms), GFP_KERNEL);
> > +	dpu_kms = kzalloc(sizeof(*dpu_kms), GFP_KERNEL);
> >  	if (!dpu_kms)
> >  		return -ENOMEM;
> >  
> >  	mp = &dpu_kms->mp;
> >  	ret = msm_dss_parse_clock(pdev, mp);
> >  	if (ret) {
> > +		kfree(dpu_kms);
> >  		DPU_ERROR("failed to parse clocks, ret=%d\n", ret);
> >  		return ret;
> >  	}
> > @@ -1109,7 +1110,7 @@ static int dpu_bind(struct device *dev, struct device *master, void *data)
> >  	dpu_kms->rpm_enabled = true;
> >  
> >  	priv->kms = &dpu_kms->base;
> > -	return ret;
> > +	return 0;
> >  }
> >  
> >  static void dpu_unbind(struct device *dev, struct device *master, void *data)
> > @@ -1120,11 +1121,12 @@ static void dpu_unbind(struct device *dev, struct device *master, void *data)
> >  
> >  	dpu_power_resource_deinit(pdev, &dpu_kms->phandle);
> >  	msm_dss_put_clk(mp->clk_config, mp->num_clk);
> > -	devm_kfree(&pdev->dev, mp->clk_config);
> > -	mp->num_clk = 0;
> > +	kfree(mp->clk_config);
> >  
> >  	if (dpu_kms->rpm_enabled)
> >  		pm_runtime_disable(&pdev->dev);
> > +
> > +	kfree(dpu_kms);
> >  }
> >  
> >  static const struct component_ops dpu_ops = {
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c
> > index 2235ef8129f4..c82347afc967 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c
> > @@ -161,7 +161,7 @@ static void dpu_mdss_destroy(struct drm_device *dev)
> >  	free_irq(platform_get_irq(pdev, 0), dpu_mdss);
> >  
> >  	msm_dss_put_clk(mp->clk_config, mp->num_clk);
> > -	devm_kfree(&pdev->dev, mp->clk_config);
> > +	kfree(mp->clk_config);
> >  
> >  	if (dpu_mdss->mmio)
> >  		devm_iounmap(&pdev->dev, dpu_mdss->mmio);
> > @@ -169,6 +169,8 @@ static void dpu_mdss_destroy(struct drm_device *dev)
> >  
> >  	pm_runtime_disable(dev->dev);
> >  	priv->mdss = NULL;
> > +
> > +	kfree(dpu_mdss);
> >  }
> >  
> >  static const struct msm_mdss_funcs mdss_funcs = {
> > @@ -186,7 +188,7 @@ int dpu_mdss_init(struct drm_device *dev)
> >  	struct dss_module_power *mp;
> >  	int ret = 0;
> >  
> > -	dpu_mdss = devm_kzalloc(dev->dev, sizeof(*dpu_mdss), GFP_KERNEL);
> > +	dpu_mdss = kzalloc(sizeof(*dpu_mdss), GFP_KERNEL);
> >  	if (!dpu_mdss)
> >  		return -ENOMEM;
> >  
> > @@ -238,8 +240,8 @@ int dpu_mdss_init(struct drm_device *dev)
> >  	_dpu_mdss_irq_domain_fini(dpu_mdss);
> >  irq_domain_error:
> >  	msm_dss_put_clk(mp->clk_config, mp->num_clk);
> > +	kfree(mp->clk_config);
> >  clk_parse_err:
> > -	devm_kfree(&pdev->dev, mp->clk_config);
> >  	if (dpu_mdss->mmio)
> >  		devm_iounmap(&pdev->dev, dpu_mdss->mmio);
> >  	dpu_mdss->mmio = NULL;
> 
> 

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno