On Fri, Nov 02, 2018 at 08:30:08AM -0600, 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
> 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 <jcro...@codeaurora.org>
> ---
> 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);
This should be freed in the err label instead of relying on the caller to clean
it up on failure.
> 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;
> --
> 2.18.0
>
> _______________________________________________
> Freedreno mailing list
> freedr...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/freedreno
--
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
