linux-kernel - Re: [PATCH v4 11/79] media: rcar_fdp1: fix pm_runtime_get

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20210430172652.00003c45@Huawei.com>
Date:   Fri, 30 Apr 2021 17:26:52 +0100
From:   Jonathan Cameron <Jonathan.Cameron@...wei.com>
To:     Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
CC:     <linuxarm@...wei.com>, <mauro.chehab@...wei.com>,
        Kieran Bingham <kieran.bingham+renesas@...asonboard.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        <linux-kernel@...r.kernel.org>, <linux-media@...r.kernel.org>,
        <linux-renesas-soc@...r.kernel.org>
Subject: Re: [PATCH v4 11/79] media: rcar_fdp1: fix pm_runtime_get_sync()
 usage count

On Wed, 28 Apr 2021 16:51:32 +0200
Mauro Carvalho Chehab <mchehab+huawei@...nel.org> wrote:

> The pm_runtime_get_sync() internally increments the
> dev->power.usage_count without decrementing it, even on errors.
> Replace it by the new pm_runtime_resume_and_get(), introduced by:
> commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter")
> in order to properly decrement the usage counter and avoid memory
> leaks.
> 
> Also, right now, the driver is ignoring any troubles when
> trying to do PM resume. So, add the proper error handling
> for the code.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@...nel.org>

Refactor to move the error handling block to one place perhaps?

Also, I thin more cleanup is needed int he 
> ---
>  drivers/media/platform/rcar_fdp1.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar_fdp1.c b/drivers/media/platform/rcar_fdp1.c
> index 01c1fbb97bf6..c32d237af618 100644
> --- a/drivers/media/platform/rcar_fdp1.c
> +++ b/drivers/media/platform/rcar_fdp1.c
> @@ -2140,7 +2140,13 @@ static int fdp1_open(struct file *file)
>  	}
>  
>  	/* Perform any power management required */
> -	pm_runtime_get_sync(fdp1->dev);
> +	ret = pm_runtime_resume_and_get(fdp1->dev);
> +	if (ret < 0) {
> +		v4l2_m2m_ctx_release(ctx->fh.m2m_ctx);

Next few lines repeated enough times that I'd suggest just pulling error handling out
to it's own path.

> +		v4l2_ctrl_handler_free(&ctx->hdl);
> +		kfree(ctx);
> +		goto done;
> +	}
>  
>  	v4l2_fh_add(&ctx->fh);
>  
> @@ -2351,7 +2357,9 @@ static int fdp1_probe(struct platform_device *pdev)
>  
>  	/* Power up the cells to read HW */
>  	pm_runtime_enable(&pdev->dev);
> -	pm_runtime_get_sync(fdp1->dev);
> +	ret = pm_runtime_resume_and_get(fdp1->dev);

>From a balance point of view, do you want to all pm_runtime_disable()
if this happens and also I'd guess you need to unregister the device
as done in other error paths in probe

The lack of balance between ordering in probe and remove before
your patch bothers me but I don't know the code well enough to tell
if there are any actual bugs there.


> +	if (ret < 0)
> +		return ret;
>  
>  	hw_version = fdp1_read(fdp1, FD1_IP_INTDATA);
>  	switch (hw_version) {