linux-kernel - Re: [PATCH] media: staging: rkisp1: isp: check return value from phy

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20200617210855.GA81308@chromium.org>
Date:   Wed, 17 Jun 2020 21:08:55 +0000
From:   Tomasz Figa <tfiga@...omium.org>
To:     Helen Koike <helen.koike@...labora.com>
Cc:     linux-media@...r.kernel.org, mchehab@...nel.org,
        dafna.hirschfeld@...labora.com, linux-kernel@...r.kernel.org,
        tfiga@...gle.com, hans.verkuil@...co.com, kernel@...labora.com,
        Wojciech Zabolotny <wzab01@...il.com>
Subject: Re: [PATCH] media: staging: rkisp1: isp: check return value from
 phy_*

Hi Helen,

On Wed, Jun 17, 2020 at 03:22:29PM -0300, Helen Koike wrote:
> When starting streaming, do not ignore return value from phy_set_mode(),
> phy_configure() and phy_power_on().
> If it fails, return error to the user.
> 
> Fixes: d65dd85281fb ("media: staging: rkisp1: add Rockchip ISP1 base driver")
> 
> Reported-by: Wojciech Zabolotny <wzab01@...il.com>
> Signed-off-by: Helen Koike <helen.koike@...labora.com>
> 
> ---
> 
>  drivers/staging/media/rkisp1/rkisp1-isp.c | 22 +++++++++++++++++++---
>  1 file changed, 19 insertions(+), 3 deletions(-)
> 

Thank you for the patch. Please see my comments inline.

> diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c
> index dc2b59a0160a8..531047fc34a01 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-isp.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-isp.c
> @@ -892,6 +892,7 @@ static int rkisp1_mipi_csi2_start(struct rkisp1_isp *isp,
>  	union phy_configure_opts opts;
>  	struct phy_configure_opts_mipi_dphy *cfg = &opts.mipi_dphy;
>  	s64 pixel_clock;
> +	int ret;
>  
>  	if (!sensor->pixel_rate_ctrl) {
>  		dev_warn(sensor->sd->dev, "No pixel rate control in subdev\n");
> @@ -906,9 +907,24 @@ static int rkisp1_mipi_csi2_start(struct rkisp1_isp *isp,
>  
>  	phy_mipi_dphy_get_default_config(pixel_clock, isp->sink_fmt->bus_width,
>  					 sensor->lanes, cfg);
> -	phy_set_mode(sensor->dphy, PHY_MODE_MIPI_DPHY);
> -	phy_configure(sensor->dphy, &opts);
> -	phy_power_on(sensor->dphy);
> +
> +	ret = phy_set_mode(sensor->dphy, PHY_MODE_MIPI_DPHY);
> +	if (ret) {

nit: I don't seem to be able to find any documentation for this API and
it's not clear if it's guaranteed that the API doesn't return positive
values. It would probably be safer to check for ret < 0.

> +		dev_err(sensor->sd->dev, "Fail setting MIPI DPHY mode\n");
> +		return -EINVAL;

Should we just return ret?

> +	}
> +
> +	ret = phy_configure(sensor->dphy, &opts);
> +	if (ret && ret != -EOPNOTSUPP) {

Why are we okay with -EOPNOTSUPP?

> +		dev_err(sensor->sd->dev, "Fail configuring MIPI DPHY\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = phy_power_on(sensor->dphy);
> +	if (ret) {
> +		dev_err(sensor->sd->dev, "Fail powering on MIPI DPHY\n");
> +		return -EINVAL;

Ditto.

Best regards,
Tomasz