On Fri, 20 May 2022 20:35:35 -0700
Yongzhi Liu <[email protected]> wrote:
> The return value of vadc_get_channel() needs to be checked to
> avoid use of NULL pointer. Fix this by adding the null pointer
> check on prop and dropping general error prints.
>
> Fixes: 0917de94c02f ("iio: vadc: Qualcomm SPMI PMIC voltage ADC driver")
> Signed-off-by: Yongzhi Liu <[email protected]>
Hi,
Heading in the right direction. A few comments inline.
Thanks,
Jonathan
> ---
> 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");
> + ret = -ENODEV;
> + return ret;
return -ENODEV;
Same for the other case below.
> + }
> ret = vadc_do_conversion(vadc, prop, &read_1);
> if (ret)
> - goto err;
Good to add a note to the patch description that
vadc_do_conversion() already provides error prints in at least
some of it's error paths. Thus it is reasonable to drop the
extra reporting in this function.
> + 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");
> + ret = -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");
> + 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");
> + ret = -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_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;
> }
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 <[email protected]>
---
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");
+ ret = -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");
+ ret = -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");
+ 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");
+ ret = -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_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;
}
--
2.7.4
On Fri, 3 Jun 2022 16:02:22 +0100
Jonathan Cameron <[email protected]> wrote:
> On Sun, 22 May 2022 09:53:47 -0700
> Yongzhi Liu <[email protected]> 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 <[email protected]>
>
> Hi
>
> Biggest remaining thing is squashing
> ret = -ENODEV;
> return ret;
>
> into the shorter
> return -ENODEV;
>
One additional process thing I didn't mention before now as this is a
single patch.
Generally for IIO at least, don't send new versions in reply to old threads.
The threads can get very deep and confusing, so I'd much rather a new thread
for each version.
Thanks,
Jonathan
>
> > ---
> > 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.
>
>
> > }
>
On Sun, 22 May 2022 09:53:47 -0700
Yongzhi Liu <[email protected]> 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 <[email protected]>
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.
> }