| Message ID | Y+utEvjfjQRQo2QB@kili (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | iio: adc: imx93: fix a signedness bug in imx93_adc_read_raw() | expand |
> -----Original Message----- > From: Dan Carpenter <error27@gmail.com> > Sent: 2023年2月14日 23:48 > To: Bough Chen <haibo.chen@nxp.com> > Cc: Jonathan Cameron <jic23@kernel.org>; Lars-Peter Clausen > <lars@metafoo.de>; Shawn Guo <shawnguo@kernel.org>; Sascha Hauer > <s.hauer@pengutronix.de>; Pengutronix Kernel Team > <kernel@pengutronix.de>; Fabio Estevam <festevam@gmail.com>; dl-linux-imx > <linux-imx@nxp.com>; linux-iio@vger.kernel.org; > kernel-janitors@vger.kernel.org > Subject: [PATCH] iio: adc: imx93: fix a signedness bug in imx93_adc_read_raw() > > The problem is these lines: > > ret = vref_uv = regulator_get_voltage(adc->vref); > if (ret < 0) > > The "ret" variable is type long and "vref_uv" is u32 so that means the condition > can never be true on a 64bit system. A negative error code from > regulator_get_voltage() would be cast to a high positive > u32 value and then remain a high positive value when cast to a long. > > The "ret" variable only ever stores ints so it should be declared as an int. We > can delete the "vref_uv" variable and use "ret" directly. Thanks for the catching! Reviewed-by: Haibo Chen <haibo.chen@nxp.com> Best Regards Haibo Chen > > Fixes: 7d02296ac8b8 ("iio: adc: add imx93 adc support") > Signed-off-by: Dan Carpenter <error27@gmail.com> > --- > drivers/iio/adc/imx93_adc.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/drivers/iio/adc/imx93_adc.c b/drivers/iio/adc/imx93_adc.c index > d8de8284e13d..8c68f0cd48f2 100644 > --- a/drivers/iio/adc/imx93_adc.c > +++ b/drivers/iio/adc/imx93_adc.c > @@ -236,8 +236,7 @@ static int imx93_adc_read_raw(struct iio_dev > *indio_dev, { > struct imx93_adc *adc = iio_priv(indio_dev); > struct device *dev = adc->dev; > - long ret; > - u32 vref_uv; > + int ret; > > switch (mask) { > case IIO_CHAN_INFO_RAW: > @@ -253,10 +252,10 @@ static int imx93_adc_read_raw(struct iio_dev > *indio_dev, > return IIO_VAL_INT; > > case IIO_CHAN_INFO_SCALE: > - ret = vref_uv = regulator_get_voltage(adc->vref); > + ret = regulator_get_voltage(adc->vref); > if (ret < 0) > return ret; > - *val = vref_uv / 1000; > + *val = ret / 1000; > *val2 = 12; > return IIO_VAL_FRACTIONAL_LOG2; > > -- > 2.35.1
On Wed, 15 Feb 2023 02:16:40 +0000 Bough Chen <haibo.chen@nxp.com> wrote: > > -----Original Message----- > > From: Dan Carpenter <error27@gmail.com> > > Sent: 2023年2月14日 23:48 > > To: Bough Chen <haibo.chen@nxp.com> > > Cc: Jonathan Cameron <jic23@kernel.org>; Lars-Peter Clausen > > <lars@metafoo.de>; Shawn Guo <shawnguo@kernel.org>; Sascha Hauer > > <s.hauer@pengutronix.de>; Pengutronix Kernel Team > > <kernel@pengutronix.de>; Fabio Estevam <festevam@gmail.com>; dl-linux-imx > > <linux-imx@nxp.com>; linux-iio@vger.kernel.org; > > kernel-janitors@vger.kernel.org > > Subject: [PATCH] iio: adc: imx93: fix a signedness bug in imx93_adc_read_raw() > > > > The problem is these lines: > > > > ret = vref_uv = regulator_get_voltage(adc->vref); > > if (ret < 0) > > > > The "ret" variable is type long and "vref_uv" is u32 so that means the condition > > can never be true on a 64bit system. A negative error code from > > regulator_get_voltage() would be cast to a high positive > > u32 value and then remain a high positive value when cast to a long. > > > > The "ret" variable only ever stores ints so it should be declared as an int. We > > can delete the "vref_uv" variable and use "ret" directly. > > Thanks for the catching! > > Reviewed-by: Haibo Chen <haibo.chen@nxp.com> Applied to the fixes-togreg branch of iio.git. Thanks Jonathan > > Best Regards > Haibo Chen > > > > Fixes: 7d02296ac8b8 ("iio: adc: add imx93 adc support") > > Signed-off-by: Dan Carpenter <error27@gmail.com> > > --- > > drivers/iio/adc/imx93_adc.c | 7 +++---- > > 1 file changed, 3 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/iio/adc/imx93_adc.c b/drivers/iio/adc/imx93_adc.c index > > d8de8284e13d..8c68f0cd48f2 100644 > > --- a/drivers/iio/adc/imx93_adc.c > > +++ b/drivers/iio/adc/imx93_adc.c > > @@ -236,8 +236,7 @@ static int imx93_adc_read_raw(struct iio_dev > > *indio_dev, { > > struct imx93_adc *adc = iio_priv(indio_dev); > > struct device *dev = adc->dev; > > - long ret; > > - u32 vref_uv; > > + int ret; > > > > switch (mask) { > > case IIO_CHAN_INFO_RAW: > > @@ -253,10 +252,10 @@ static int imx93_adc_read_raw(struct iio_dev > > *indio_dev, > > return IIO_VAL_INT; > > > > case IIO_CHAN_INFO_SCALE: > > - ret = vref_uv = regulator_get_voltage(adc->vref); > > + ret = regulator_get_voltage(adc->vref); > > if (ret < 0) > > return ret; > > - *val = vref_uv / 1000; > > + *val = ret / 1000; > > *val2 = 12; > > return IIO_VAL_FRACTIONAL_LOG2; > > > > -- > > 2.35.1 >
diff --git a/drivers/iio/adc/imx93_adc.c b/drivers/iio/adc/imx93_adc.c index d8de8284e13d..8c68f0cd48f2 100644 --- a/drivers/iio/adc/imx93_adc.c +++ b/drivers/iio/adc/imx93_adc.c @@ -236,8 +236,7 @@ static int imx93_adc_read_raw(struct iio_dev *indio_dev, { struct imx93_adc *adc = iio_priv(indio_dev); struct device *dev = adc->dev; - long ret; - u32 vref_uv; + int ret; switch (mask) { case IIO_CHAN_INFO_RAW: @@ -253,10 +252,10 @@ static int imx93_adc_read_raw(struct iio_dev *indio_dev, return IIO_VAL_INT; case IIO_CHAN_INFO_SCALE: - ret = vref_uv = regulator_get_voltage(adc->vref); + ret = regulator_get_voltage(adc->vref); if (ret < 0) return ret; - *val = vref_uv / 1000; + *val = ret / 1000; *val2 = 12; return IIO_VAL_FRACTIONAL_LOG2;
The problem is these lines: ret = vref_uv = regulator_get_voltage(adc->vref); if (ret < 0) The "ret" variable is type long and "vref_uv" is u32 so that means the condition can never be true on a 64bit system. A negative error code from regulator_get_voltage() would be cast to a high positive u32 value and then remain a high positive value when cast to a long. The "ret" variable only ever stores ints so it should be declared as an int. We can delete the "vref_uv" variable and use "ret" directly. Fixes: 7d02296ac8b8 ("iio: adc: add imx93 adc support") Signed-off-by: Dan Carpenter <error27@gmail.com> --- drivers/iio/adc/imx93_adc.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)