drivers/iio/dac/ltc2664.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-)
In the current implementation of the ltc2664_channel_config function,
a variable named span is declared and initialized to 0, intended to
capture the return value of the ltc2664_set_span function. However,
the output of ltc2664_set_span is directly assigned to chan->span,
leaving span unchanged. As a result, when the function later checks
if (span < 0), this condition will never trigger an error since
span remains 0, this flaw leads to ineffective error handling. The
current patch resolves this issue by using the ret variable for
getting the return value, later assigning if successful and also
effectively removing span variable.
Signed-off-by: Mohammed Anees <pvmohammedanees2003@gmail.com>
Fixes: 4cc2fc445d2e4e63ed6bd5d310752d88d365f8e4
---
v2:
- Using the ret variable to store the result from ltc2664_set_span
---
drivers/iio/dac/ltc2664.c | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/drivers/iio/dac/ltc2664.c b/drivers/iio/dac/ltc2664.c
index 5be5345ac5c8..7dafcba7ece7 100644
--- a/drivers/iio/dac/ltc2664.c
+++ b/drivers/iio/dac/ltc2664.c
@@ -516,7 +516,7 @@ static int ltc2664_channel_config(struct ltc2664_state *st)
const struct ltc2664_chip_info *chip_info = st->chip_info;
struct device *dev = &st->spi->dev;
u32 reg, tmp[2], mspan;
- int ret, span = 0;
+ int ret;
mspan = LTC2664_MSPAN_SOFTSPAN;
ret = device_property_read_u32(dev, "adi,manual-span-operation-config",
@@ -579,20 +579,24 @@ static int ltc2664_channel_config(struct ltc2664_state *st)
ret = fwnode_property_read_u32_array(child, "output-range-microvolt",
tmp, ARRAY_SIZE(tmp));
if (!ret && mspan == LTC2664_MSPAN_SOFTSPAN) {
- chan->span = ltc2664_set_span(st, tmp[0] / 1000,
+ ret = ltc2664_set_span(st, tmp[0] / 1000,
tmp[1] / 1000, reg);
- if (span < 0)
- return dev_err_probe(dev, span,
+ if (ret < 0)
+ return dev_err_probe(dev, ret,
"Failed to set span\n");
+ else
+ chan->span = ret;
}
ret = fwnode_property_read_u32_array(child, "output-range-microamp",
tmp, ARRAY_SIZE(tmp));
if (!ret) {
- chan->span = ltc2664_set_span(st, 0, tmp[1] / 1000, reg);
- if (span < 0)
- return dev_err_probe(dev, span,
+ ret = ltc2664_set_span(st, 0, tmp[1] / 1000, reg);
+ if (ret < 0)
+ return dev_err_probe(dev, ret,
"Failed to set span\n");
+ else
+ chan->span = ret;
}
}
--
2.46.0On Sat, 5 Oct 2024 22:37:22 +0530
Mohammed Anees <pvmohammedanees2003@gmail.com> wrote:
> In the current implementation of the ltc2664_channel_config function,
> a variable named span is declared and initialized to 0, intended to
> capture the return value of the ltc2664_set_span function. However,
> the output of ltc2664_set_span is directly assigned to chan->span,
> leaving span unchanged. As a result, when the function later checks
> if (span < 0), this condition will never trigger an error since
> span remains 0, this flaw leads to ineffective error handling. The
> current patch resolves this issue by using the ret variable for
> getting the return value, later assigning if successful and also
> effectively removing span variable.
>
> Signed-off-by: Mohammed Anees <pvmohammedanees2003@gmail.com>
> Fixes: 4cc2fc445d2e4e63ed6bd5d310752d88d365f8e4
> ---
> v2:
> - Using the ret variable to store the result from ltc2664_set_span
> ---
> drivers/iio/dac/ltc2664.c | 18 +++++++++++-------
> 1 file changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/iio/dac/ltc2664.c b/drivers/iio/dac/ltc2664.c
> index 5be5345ac5c8..7dafcba7ece7 100644
> --- a/drivers/iio/dac/ltc2664.c
> +++ b/drivers/iio/dac/ltc2664.c
> @@ -516,7 +516,7 @@ static int ltc2664_channel_config(struct ltc2664_state *st)
> const struct ltc2664_chip_info *chip_info = st->chip_info;
> struct device *dev = &st->spi->dev;
> u32 reg, tmp[2], mspan;
> - int ret, span = 0;
> + int ret;
>
> mspan = LTC2664_MSPAN_SOFTSPAN;
> ret = device_property_read_u32(dev, "adi,manual-span-operation-config",
> @@ -579,20 +579,24 @@ static int ltc2664_channel_config(struct ltc2664_state *st)
> ret = fwnode_property_read_u32_array(child, "output-range-microvolt",
> tmp, ARRAY_SIZE(tmp));
> if (!ret && mspan == LTC2664_MSPAN_SOFTSPAN) {
> - chan->span = ltc2664_set_span(st, tmp[0] / 1000,
> + ret = ltc2664_set_span(st, tmp[0] / 1000,
> tmp[1] / 1000, reg);
> - if (span < 0)
> - return dev_err_probe(dev, span,
> + if (ret < 0)
> + return dev_err_probe(dev, ret,
> "Failed to set span\n");
> + else
else is unnecessary here as we have the standard check and error and return
if set pattern.
> + chan->span = ret;
> }
>
> ret = fwnode_property_read_u32_array(child, "output-range-microamp",
> tmp, ARRAY_SIZE(tmp));
> if (!ret) {
> - chan->span = ltc2664_set_span(st, 0, tmp[1] / 1000, reg);
> - if (span < 0)
> - return dev_err_probe(dev, span,
> + ret = ltc2664_set_span(st, 0, tmp[1] / 1000, reg);
> + if (ret < 0)
> + return dev_err_probe(dev, ret,
> "Failed to set span\n");
> + else
and here.
> + chan->span = ret;
> }
> }
>
On Sun, 6 Oct 2024 15:25:33 +0100
Jonathan Cameron <jic23@kernel.org> wrote:
> On Sat, 5 Oct 2024 22:37:22 +0530
> Mohammed Anees <pvmohammedanees2003@gmail.com> wrote:
>
> > In the current implementation of the ltc2664_channel_config function,
> > a variable named span is declared and initialized to 0, intended to
> > capture the return value of the ltc2664_set_span function. However,
> > the output of ltc2664_set_span is directly assigned to chan->span,
> > leaving span unchanged. As a result, when the function later checks
> > if (span < 0), this condition will never trigger an error since
> > span remains 0, this flaw leads to ineffective error handling. The
> > current patch resolves this issue by using the ret variable for
> > getting the return value, later assigning if successful and also
> > effectively removing span variable.
> >
> > Signed-off-by: Mohammed Anees <pvmohammedanees2003@gmail.com>
> > Fixes: 4cc2fc445d2e4e63ed6bd5d310752d88d365f8e4
Hmm. I see you had a v3. For some reason that hasn't reached my inbox.
Also note the fixes tag was wrong and you've fixed that.
I've picked up v3 and applied it to the fixes-togreg branch of iio.git
and marked it for stable inclusion.
Thanks,
J
> > ---
> > v2:
> > - Using the ret variable to store the result from ltc2664_set_span
> > ---
> > drivers/iio/dac/ltc2664.c | 18 +++++++++++-------
> > 1 file changed, 11 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/iio/dac/ltc2664.c b/drivers/iio/dac/ltc2664.c
> > index 5be5345ac5c8..7dafcba7ece7 100644
> > --- a/drivers/iio/dac/ltc2664.c
> > +++ b/drivers/iio/dac/ltc2664.c
> > @@ -516,7 +516,7 @@ static int ltc2664_channel_config(struct ltc2664_state *st)
> > const struct ltc2664_chip_info *chip_info = st->chip_info;
> > struct device *dev = &st->spi->dev;
> > u32 reg, tmp[2], mspan;
> > - int ret, span = 0;
> > + int ret;
> >
> > mspan = LTC2664_MSPAN_SOFTSPAN;
> > ret = device_property_read_u32(dev, "adi,manual-span-operation-config",
> > @@ -579,20 +579,24 @@ static int ltc2664_channel_config(struct ltc2664_state *st)
> > ret = fwnode_property_read_u32_array(child, "output-range-microvolt",
> > tmp, ARRAY_SIZE(tmp));
> > if (!ret && mspan == LTC2664_MSPAN_SOFTSPAN) {
> > - chan->span = ltc2664_set_span(st, tmp[0] / 1000,
> > + ret = ltc2664_set_span(st, tmp[0] / 1000,
> > tmp[1] / 1000, reg);
> > - if (span < 0)
> > - return dev_err_probe(dev, span,
> > + if (ret < 0)
> > + return dev_err_probe(dev, ret,
> > "Failed to set span\n");
> > + else
> else is unnecessary here as we have the standard check and error and return
> if set pattern.
>
>
> > + chan->span = ret;
> > }
> >
> > ret = fwnode_property_read_u32_array(child, "output-range-microamp",
> > tmp, ARRAY_SIZE(tmp));
> > if (!ret) {
> > - chan->span = ltc2664_set_span(st, 0, tmp[1] / 1000, reg);
> > - if (span < 0)
> > - return dev_err_probe(dev, span,
> > + ret = ltc2664_set_span(st, 0, tmp[1] / 1000, reg);
> > + if (ret < 0)
> > + return dev_err_probe(dev, ret,
> > "Failed to set span\n");
> > + else
> and here.
> > + chan->span = ret;
> > }
> > }
> >
>
>
Hey thanks Jonathan! Christophe helped me rectify the previous patch. Thanks for all your help!
Le 05/10/2024 à 19:07, Mohammed Anees a écrit :
> In the current implementation of the ltc2664_channel_config function,
Hi,
Usually () are added after function name.
In the current implementation of ltc2664_channel_config()
Same for other function in the text below.
> a variable named span is declared and initialized to 0, intended to
> capture the return value of the ltc2664_set_span function. However,
> the output of ltc2664_set_span is directly assigned to chan->span,
> leaving span unchanged. As a result, when the function later checks
> if (span < 0), this condition will never trigger an error since
> span remains 0, this flaw leads to ineffective error handling. The
> current patch resolves this issue by using the ret variable for
Usually imperative is preferred in patch description. This could be:
Resolve this issue by using the ret variable for...
(without "The current patch")
> getting the return value, later assigning if successful and also
> effectively removing span variable.
>
> Signed-off-by: Mohammed Anees <pvmohammedanees2003-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Fixes: 4cc2fc445d2e4e63ed6bd5d310752d88d365f8e4
Fixes should be before S-o-b.
Fixes should be Fixes: <12 chars of sha1> ("<title line>")
So here: Fixes: 4cc2fc445d2e ("iio: dac: ltc2664: Add driver for LTC2664
and LTC2672")
I think that running "./scripts/checkpatch.pl" on your patch should warn
about it.
> ---
> v2:
> - Using the ret variable to store the result from ltc2664_set_span
> ---
> drivers/iio/dac/ltc2664.c | 18 +++++++++++-------
> 1 file changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/iio/dac/ltc2664.c b/drivers/iio/dac/ltc2664.c
> index 5be5345ac5c8..7dafcba7ece7 100644
> --- a/drivers/iio/dac/ltc2664.c
> +++ b/drivers/iio/dac/ltc2664.c
> @@ -516,7 +516,7 @@ static int ltc2664_channel_config(struct ltc2664_state *st)
> const struct ltc2664_chip_info *chip_info = st->chip_info;
> struct device *dev = &st->spi->dev;
> u32 reg, tmp[2], mspan;
> - int ret, span = 0;
> + int ret;
>
> mspan = LTC2664_MSPAN_SOFTSPAN;
> ret = device_property_read_u32(dev, "adi,manual-span-operation-config",
> @@ -579,20 +579,24 @@ static int ltc2664_channel_config(struct ltc2664_state *st)
> ret = fwnode_property_read_u32_array(child, "output-range-microvolt",
> tmp, ARRAY_SIZE(tmp));
> if (!ret && mspan == LTC2664_MSPAN_SOFTSPAN) {
> - chan->span = ltc2664_set_span(st, tmp[0] / 1000,
> + ret = ltc2664_set_span(st, tmp[0] / 1000,
> tmp[1] / 1000, reg);
Parameters are not aligned anymore with the opening ( of the previous line.
"./scripts/checkpatch.pl --strict" would also warn about it.
> - if (span < 0)
> - return dev_err_probe(dev, span,
> + if (ret < 0)
> + return dev_err_probe(dev, ret,
> "Failed to set span\n");
> + else
No need for 'else'.
> + chan->span = ret;
> }
>
> ret = fwnode_property_read_u32_array(child, "output-range-microamp",
> tmp, ARRAY_SIZE(tmp));
> if (!ret) {
> - chan->span = ltc2664_set_span(st, 0, tmp[1] / 1000, reg);
> - if (span < 0)
> - return dev_err_probe(dev, span,
> + ret = ltc2664_set_span(st, 0, tmp[1] / 1000, reg);
> + if (ret < 0)
> + return dev_err_probe(dev, ret,
> "Failed to set span\n");
> + else
No need for 'else'.
CJ
> + chan->span = ret;
> }
> }
>
Hi! Thank you for your feedback. I’ve made the necessary adjustments based on your suggestions and checked with checkpatch.pl --strict, resolving the warnings. I'm sending the v3 patch right away. Thanks again for your help, learned a lot!
© 2016 - 2024 Red Hat, Inc.