Hi,
On Mon, Dec 5, 2022 at 11:55 PM Miaoqian Lin <linmq006@xxxxxxxxx> wrote:
>
> of_icc_get() alloc resources for path1, we should release it when not
> need anymore. Early return when IS_ERR_OR_NULL(path0) may leak path1.
> Add icc_put(path1) in the error path to fix this.
>
> Fixes: b9364eed9232 ("drm/msm/dpu: Move min BW request and full BW disable back to mdss")
> Signed-off-by: Miaoqian Lin <linmq006@xxxxxxxxx>
> ---
> drivers/gpu/drm/msm/msm_mdss.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
> index e13c5c12b775..a38fa9a9a3d6 100644
> --- a/drivers/gpu/drm/msm/msm_mdss.c
> +++ b/drivers/gpu/drm/msm/msm_mdss.c
> @@ -49,8 +49,10 @@ static int msm_mdss_parse_data_bus_icc_path(struct device *dev,
> struct icc_path *path0 = of_icc_get(dev, "mdp0-mem");
> struct icc_path *path1 = of_icc_get(dev, "mdp1-mem");
>
> - if (IS_ERR_OR_NULL(path0))
> + if (IS_ERR_OR_NULL(path0)) {
> + icc_put(path1);
> return PTR_ERR_OR_ZERO(path0);
> + }
>
> msm_mdss->path[0] = path0;
> msm_mdss->num_paths = 1;
Hmmm. I guess the original author of the code (which wasn't me--I just
restored the code that was deleted by a previous change) was assuming
that if mdp0-mem had a problem that mdp1-mem would also have a
problem. That would mean that you wouldn't need to call icc_put() on
it.
...and, in fact, your patch doesn't handle that case, does it? If
_both_ of the two are error or NULL then you'll be calling icc_put()
on something invalid. I guess icc_put() handles those cases without
crashing but it will give a WARN_ON() splat if it happens to be an
error...
Really, there's a better solution anyway. Instead, you should do:
path0 = of_icc_get(dev, "mdp0-mem");
if (IS_ERR_OR_NULL(path0))
return PTR_ERR_OR_ZERO(path0);
msm_mdss->path[0] = path0;
msm_mdss->num_paths = 1;
path1 = of_icc_get(dev, "mdp1-mem");
if (!IS_ERR_OR_NULL(path1)) {
...
}
In other words just defer getting path1 until after you've checked
path0 for an error.
-Doug
