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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Nov 02, 2018 at 10:57:46AM -0700, Jeykumar Sankaran wrote:
> On 2018-11-02 07:30, 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 component bind fails
> >due to an -EPROBE_DEFER. In this case the bind would try again
> 
> Isn't this the only case where using devm would be a problem? Even
> in this case
> do you expect any leaks if devm_kfree is called before DEFERing due
> to errors
> and in unbounds?

Yes, defer would be the only case where the memory would be leaked because you
would re-initialize the pointer when you attempted to bind again but then
again this is the only real case you need to worry about in a stable environment
so it is worth optimizing for.

You are right that using devm_kfree would solve the problem but the whole point
of devm is that it is supposed to be fire-and-forget. If you have to go to
the effort of explicitly bounding devm_kmalloc with devm_kfree then why are you
bothering to use devm in the first place?

Jordan

> >later and any devm managed memory allocated during the former
> >aborted attempt would be leaked until the device itself was
> >destroyed. Since all the memory allocated during a bind
> >should be freed during an unbind (or bind error case) there isn't
> >any reason to use devm for resources that have a explicit
> >teardown step.
> >
> >This doesn't remove devm for all resources - in particular
> >msm_ioremap() still uses devm_ioremap() but thats a generic
> >issue that can easily be addressed as a cleanup later and the
> >unbind code already does the requisite devm calls to unmap it.
> >
> >Signed-off-by: Jordan Crouse <jcrouse@xxxxxxxxxxxxxx>
> >---
> > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c |  4 ++--
> > drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.c |  4 +---
> > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     | 10 ++++++----
> > drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c    |  8 +++++---
> > 4 files changed, 14 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..90b53e9508f2 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;
> >
> >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..34ab489b1a5b 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;
> >
> >@@ -239,7 +241,7 @@ int dpu_mdss_init(struct drm_device *dev)
> > irq_domain_error:
> > 	msm_dss_put_clk(mp->clk_config, mp->num_clk);
> > clk_parse_err:
> >-	devm_kfree(&pdev->dev, mp->clk_config);
> >+	kfree(mp->clk_config);
> > 	if (dpu_mdss->mmio)
> > 		devm_iounmap(&pdev->dev, dpu_mdss->mmio);
> > 	dpu_mdss->mmio = NULL;
> 
> -- 
> Jeykumar S

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project



[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux