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@xxxxxxxxxxxxxx> > > --- > > 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
