All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jonathan Cameron <jic23@kernel.org>
To: Yongzhi Liu <lyz_cs@pku.edu.cn>
Cc: agross@kernel.org, bjorn.andersson@linaro.org, lars@metafoo.de,
	svarbanov@mm-sol.com, iivanov@mm-sol.com,
	jonathan.cameron@huawei.com, linux-arm-msm@vger.kernel.org,
	linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org,
	fuyq@stu.pku.edu.cn
Subject: Re: [PATCH v4] iio: vadc: Fix potential dereference of NULL pointer
Date: Fri, 3 Jun 2022 16:02:22 +0100	[thread overview]
Message-ID: <20220603160222.1ad6ef49@jic23-huawei> (raw)
In-Reply-To: <1653238427-73587-1-git-send-email-lyz_cs@pku.edu.cn>

On Sun, 22 May 2022 09:53:47 -0700
Yongzhi Liu <lyz_cs@pku.edu.cn> wrote:

> The return value of vadc_get_channel() needs to be checked to
> avoid use of NULL pointer. vadc_do_conversion() already provides
> error prints in at least some of it's error paths. Thus it is
> reasonable to add the null pointer check on prop and drop the
> extra reporting in vadc_measure_ref_points().
> 
> Signed-off-by: Yongzhi Liu <lyz_cs@pku.edu.cn>

Hi

Biggest remaining thing is squashing
ret = -ENODEV;
return ret;

into the shorter
return -ENODEV;


> ---
>  drivers/iio/adc/qcom-spmi-vadc.c | 38 ++++++++++++++++++++++++++++----------
>  1 file changed, 28 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/iio/adc/qcom-spmi-vadc.c b/drivers/iio/adc/qcom-spmi-vadc.c
> index 34202ba..43a52b1 100644
> --- a/drivers/iio/adc/qcom-spmi-vadc.c
> +++ b/drivers/iio/adc/qcom-spmi-vadc.c
> @@ -358,22 +358,33 @@ static int vadc_measure_ref_points(struct vadc_priv *vadc)
>  	vadc->graph[VADC_CALIB_ABSOLUTE].dx = VADC_ABSOLUTE_RANGE_UV;
>  
>  	prop = vadc_get_channel(vadc, VADC_REF_1250MV);
> +	if (!prop) {
> +		dev_err(vadc->dev, "Please define 1.25V channel\n");
Probably makes more sense to have the error as 
"No 1.25V channel found\n");

It's not obvious to anyone getting this error what 'define' might mean
without them looking at the code, so I'd rather we just said what had
gone wrong rather offering incomplete advice.

> +		ret = -ENODEV;

Don't bother assigning a variable just to return it in the next line.

return -ENODEV;

> +		return ret;
> +	}
>  	ret = vadc_do_conversion(vadc, prop, &read_1);
>  	if (ret)
> -		goto err;
> +		return ret;
>  
>  	/* Try with buffered 625mV channel first */
>  	prop = vadc_get_channel(vadc, VADC_SPARE1);
> -	if (!prop)
> +	if (!prop) {
>  		prop = vadc_get_channel(vadc, VADC_REF_625MV);
> +		if (!prop) {
> +			dev_err(vadc->dev, "Please define 0.625V channel\n");
"No 0.625V channel found\n"
> +			ret = -ENODEV;

return -ENODEV;

> +			return ret;
> +		}
> +	}
>  
>  	ret = vadc_do_conversion(vadc, prop, &read_2);
>  	if (ret)
> -		goto err;
> +		return ret;
>  
>  	if (read_1 == read_2) {
>  		ret = -EINVAL;
> -		goto err;
> +		return ret;
>  	}
>  
>  	vadc->graph[VADC_CALIB_ABSOLUTE].dy = read_1 - read_2;
> @@ -381,25 +392,32 @@ static int vadc_measure_ref_points(struct vadc_priv *vadc)
>  
>  	/* Ratiometric calibration */
>  	prop = vadc_get_channel(vadc, VADC_VDD_VADC);
> +	if (!prop) {
> +		dev_err(vadc->dev, "Please define VDD channel\n");

"No VDD channel found\n"

> +		ret = -ENODEV;
> +		return ret;
> +	}
>  	ret = vadc_do_conversion(vadc, prop, &read_1);
>  	if (ret)
> -		goto err;
> +		return ret;
>  
>  	prop = vadc_get_channel(vadc, VADC_GND_REF);
> +	if (!prop) {
> +		dev_err(vadc->dev, "Please define GND channel\n");

"No GND channel found\n"

> +		ret = -ENODEV;
> +		return ret;

return -ENODEV;

> +	}
>  	ret = vadc_do_conversion(vadc, prop, &read_2);
>  	if (ret)
> -		goto err;
> +		return ret;
>  
>  	if (read_1 == read_2) {
>  		ret = -EINVAL;
> -		goto err;
> +		return ret;

return -ENODEV;

>  	}
>  
>  	vadc->graph[VADC_CALIB_RATIOMETRIC].dy = read_1 - read_2;
>  	vadc->graph[VADC_CALIB_RATIOMETRIC].gnd = read_2;
> -err:
> -	if (ret)
> -		dev_err(vadc->dev, "measure reference points failed\n");
>  
>  	return ret;

Can't get here with anything other than ret == 0 so
	return 0;
to make that explicit.


>  }


  reply	other threads:[~2022-06-03 14:53 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-05-18  6:43 [PATCH] " Yongzhi Liu
2022-05-18 17:31 ` Matthias Kaehlcke
2022-05-19  5:50   ` [PATCH v2] " Yongzhi Liu
2022-05-20 17:13     ` Jonathan Cameron
2022-05-21  3:31       ` [PATCH] hv_netvsc: " Yongzhi Liu
2022-05-21  3:34         ` 刘永志
2022-05-23 15:21         ` Andy Shevchenko
2022-05-23 15:45           ` 刘永志
2022-05-21  3:35       ` [PATCH v3] iio: vadc: " Yongzhi Liu
2022-05-22 11:01         ` Jonathan Cameron
2022-05-22 16:53           ` [PATCH v4] " Yongzhi Liu
2022-06-03 15:02             ` Jonathan Cameron [this message]
2022-06-03 15:20               ` Jonathan Cameron

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20220603160222.1ad6ef49@jic23-huawei \
    --to=jic23@kernel.org \
    --cc=agross@kernel.org \
    --cc=bjorn.andersson@linaro.org \
    --cc=fuyq@stu.pku.edu.cn \
    --cc=iivanov@mm-sol.com \
    --cc=jonathan.cameron@huawei.com \
    --cc=lars@metafoo.de \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-iio@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=lyz_cs@pku.edu.cn \
    --cc=svarbanov@mm-sol.com \
    --subject='Re: [PATCH v4] iio: vadc: Fix potential dereference of NULL pointer' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.