Message ID | 20230314193709.15208-1-xhxgldhlpfy@gmail.com (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | iio: adc: qcom-pm8xxx-xoadc: Remove useless condition in pm8xxx_xoadc_parse_channel() | expand |
On Tue, Mar 14, 2023 at 10:37:09PM +0300, Kasumov Ruslan wrote: > The left side of the loop condition never becomes false. > hwchan cannot be NULL, because it points to elements of the > hw_channels array that takes one of 4 predefined values: > pm8018_xoadc_channels, pm8038_xoadc_channels, > pm8058_xoadc_channels, pm8921_xoadc_channels. > > Found by Linux Verification Center (linuxtesting.org) with SVACE. > > Fixes: 63c3ecd946d4 ("iio: adc: add a driver for Qualcomm PM8xxx HK/XOADC") It wasn't broken before, it wasn't causing any harm. It's not a fix, it's a cleanup. Please, remove remove the "Fixes:" tag. > Signed-off-by: Kasumov Ruslan <s02210418@gse.cs.msu.ru> > --- > drivers/iio/adc/qcom-pm8xxx-xoadc.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/iio/adc/qcom-pm8xxx-xoadc.c b/drivers/iio/adc/qcom-pm8xxx-xoadc.c > index eb424496ee1d..64a3aeb6261c 100644 > --- a/drivers/iio/adc/qcom-pm8xxx-xoadc.c > +++ b/drivers/iio/adc/qcom-pm8xxx-xoadc.c > @@ -758,7 +758,7 @@ static int pm8xxx_xoadc_parse_channel(struct device *dev, > /* Find the right channel setting */ > chid = 0; > hwchan = &hw_channels[0]; > - while (hwchan && hwchan->datasheet_name) { > + while (hwchan->datasheet_name) { With the fixes tag removed: Reviewed-by: Andi Shyti <andi.shyti@kernel.org> Thanks, Andi > if (hwchan->pre_scale_mux == pre_scale_mux && > hwchan->amux_channel == amux_channel) > break; > -- > 2.34.1 >
On Tue, Mar 14, 2023 at 8:37 PM Kasumov Ruslan <xhxgldhlpfy@gmail.com> wrote: > The left side of the loop condition never becomes false. > hwchan cannot be NULL, because it points to elements of the > hw_channels array that takes one of 4 predefined values: > pm8018_xoadc_channels, pm8038_xoadc_channels, > pm8058_xoadc_channels, pm8921_xoadc_channels. > > Found by Linux Verification Center (linuxtesting.org) with SVACE. I am not impressed with that tool. See below: > Fixes: 63c3ecd946d4 ("iio: adc: add a driver for Qualcomm PM8xxx HK/XOADC") > Signed-off-by: Kasumov Ruslan <s02210418@gse.cs.msu.ru> (...) > hwchan = &hw_channels[0]; > - while (hwchan && hwchan->datasheet_name) { > + while (hwchan->datasheet_name) { > if (hwchan->pre_scale_mux == pre_scale_mux && > hwchan->amux_channel == amux_channel) > break; NAK have you tested this on a real system? Here is the complete loop: hwchan = &hw_channels[0]; while (hwchan && hwchan->datasheet_name) { if (hwchan->pre_scale_mux == pre_scale_mux && hwchan->amux_channel == amux_channel) break; hwchan++; chid++; } Notice how hwchan is used as iterator in hwchan++. What you are doing will cause a zero-pointer dereference. Whoever is developing "SVACE" needs to fix the tool to understand this kind of usage. Yours, Linus Walleij
Hi, > > The left side of the loop condition never becomes false. > > hwchan cannot be NULL, because it points to elements of the > > hw_channels array that takes one of 4 predefined values: > > pm8018_xoadc_channels, pm8038_xoadc_channels, > > pm8058_xoadc_channels, pm8921_xoadc_channels. > > > > Found by Linux Verification Center (linuxtesting.org) with SVACE. > > I am not impressed with that tool. See below: > > > Fixes: 63c3ecd946d4 ("iio: adc: add a driver for Qualcomm PM8xxx HK/XOADC") > > Signed-off-by: Kasumov Ruslan <s02210418@gse.cs.msu.ru> > > (...) > > hwchan = &hw_channels[0]; > > - while (hwchan && hwchan->datasheet_name) { > > + while (hwchan->datasheet_name) { > > if (hwchan->pre_scale_mux == pre_scale_mux && > > hwchan->amux_channel == amux_channel) > > break; > > NAK have you tested this on a real system? > > Here is the complete loop: > > hwchan = &hw_channels[0]; > while (hwchan && hwchan->datasheet_name) { > if (hwchan->pre_scale_mux == pre_scale_mux && > hwchan->amux_channel == amux_channel) > break; > hwchan++; ops, yes, sorry, I overlooked at this... This becomes NULL at some point as there is a sentinel in the _variant structures. Thanks, Andi > chid++; > } > > Notice how hwchan is used as iterator in hwchan++. > > What you are doing will cause a zero-pointer dereference. > > Whoever is developing "SVACE" needs to fix the tool to understand > this kind of usage. > > Yours, > Linus Walleij
On 15.03.2023 00:28, Andi Shyti wrote: > Hi, > >>> The left side of the loop condition never becomes false. >>> hwchan cannot be NULL, because it points to elements of the >>> hw_channels array that takes one of 4 predefined values: >>> pm8018_xoadc_channels, pm8038_xoadc_channels, >>> pm8058_xoadc_channels, pm8921_xoadc_channels. >>> >>> Found by Linux Verification Center (linuxtesting.org) with SVACE. >> >> I am not impressed with that tool. See below: >> >>> Fixes: 63c3ecd946d4 ("iio: adc: add a driver for Qualcomm PM8xxx HK/XOADC") >>> Signed-off-by: Kasumov Ruslan <s02210418@gse.cs.msu.ru> >> >> (...) >>> hwchan = &hw_channels[0]; >>> - while (hwchan && hwchan->datasheet_name) { >>> + while (hwchan->datasheet_name) { >>> if (hwchan->pre_scale_mux == pre_scale_mux && >>> hwchan->amux_channel == amux_channel) >>> break; >> >> NAK have you tested this on a real system? >> >> Here is the complete loop: >> >> hwchan = &hw_channels[0]; >> while (hwchan && hwchan->datasheet_name) { >> if (hwchan->pre_scale_mux == pre_scale_mux && >> hwchan->amux_channel == amux_channel) >> break; >> hwchan++; > > ops, yes, sorry, I overlooked at this... This becomes NULL at > some point as there is a sentinel in the _variant structures. > Could you please clarify what do you mean. As far as I can see sentinel is an "empty" element of xoadc_channel in the array, i.e. hwchan->datasheet_name works as a sentinel while hwchan is always non NULL. -- Alexey
On Tue, Mar 14, 2023 at 10:12 PM Linus Walleij <linus.walleij@linaro.org> wrote: > On Tue, Mar 14, 2023 at 8:37 PM Kasumov Ruslan <xhxgldhlpfy@gmail.com> wrote: > > > The left side of the loop condition never becomes false. > > hwchan cannot be NULL, because it points to elements of the > > hw_channels array that takes one of 4 predefined values: > > pm8018_xoadc_channels, pm8038_xoadc_channels, > > pm8058_xoadc_channels, pm8921_xoadc_channels. > > > > Found by Linux Verification Center (linuxtesting.org) with SVACE. > > I am not impressed with that tool. See below: > > > Fixes: 63c3ecd946d4 ("iio: adc: add a driver for Qualcomm PM8xxx HK/XOADC") > > Signed-off-by: Kasumov Ruslan <s02210418@gse.cs.msu.ru> > > (...) > > hwchan = &hw_channels[0]; > > - while (hwchan && hwchan->datasheet_name) { > > + while (hwchan->datasheet_name) { > > if (hwchan->pre_scale_mux == pre_scale_mux && > > hwchan->amux_channel == amux_channel) > > break; > > NAK have you tested this on a real system? > > Here is the complete loop: > > hwchan = &hw_channels[0]; > while (hwchan && hwchan->datasheet_name) { > if (hwchan->pre_scale_mux == pre_scale_mux && > hwchan->amux_channel == amux_channel) > break; > hwchan++; > chid++; > } > > Notice how hwchan is used as iterator in hwchan++. > > What you are doing will cause a zero-pointer dereference. Nah the AI is smarter than me this time, I'm wrong, I think :( hwchan is indeed never NULL here, and the code immediately after unconditionally dereferences hwchan->datasheet_name. Who wrote this convoluted code again: 63c3ecd946d4a (Linus Walleij 2017-04-04 14:08:19 +0200 759) chid = 0; 63c3ecd946d4a (Linus Walleij 2017-04-04 14:08:19 +0200 760) hwchan = &hw_channels[0]; 63c3ecd946d4a (Linus Walleij 2017-04-04 14:08:19 +0200 761) while (hwchan && hwchan->datasheet_name) { 63c3ecd946d4a (Linus Walleij 2017-04-04 14:08:19 +0200 762) if (hwchan->pre_scale_mux == pre_scale_mux && 63c3ecd946d4a (Linus Walleij 2017-04-04 14:08:19 +0200 763) hwchan->amux_channel == amux_channel) 63c3ecd946d4a (Linus Walleij 2017-04-04 14:08:19 +0200 764) break; 63c3ecd946d4a (Linus Walleij 2017-04-04 14:08:19 +0200 765) hwchan++; 63c3ecd946d4a (Linus Walleij 2017-04-04 14:08:19 +0200 766) chid++; 63c3ecd946d4a (Linus Walleij 2017-04-04 14:08:19 +0200 767) } 63c3ecd946d4a (Linus Walleij 2017-04-04 14:08:19 +0200 768) /* The sentinel does not have a name assigned */ 63c3ecd946d4a (Linus Walleij 2017-04-04 14:08:19 +0200 769) if (!hwchan->datasheet_name) { Oh that guy ... I wonder if we can make it look better and less unintuitive. Yours, Linus Walleij
On Tue, Mar 14, 2023 at 11:03 PM Alexey Khoroshilov <khoroshilov@ispras.ru> wrote: > As far as I can see sentinel is an "empty" element of xoadc_channel in > the array, i.e. hwchan->datasheet_name works as a sentinel while hwchan > is always non NULL. You're right, I was unable to understand my own code :( Yours, Linus Walleij
On Tue, Mar 14, 2023 at 8:37 PM Kasumov Ruslan <xhxgldhlpfy@gmail.com> wrote: > The left side of the loop condition never becomes false. > hwchan cannot be NULL, because it points to elements of the > hw_channels array that takes one of 4 predefined values: > pm8018_xoadc_channels, pm8038_xoadc_channels, > pm8058_xoadc_channels, pm8921_xoadc_channels. > > Found by Linux Verification Center (linuxtesting.org) with SVACE. > > Fixes: 63c3ecd946d4 ("iio: adc: add a driver for Qualcomm PM8xxx HK/XOADC") > Signed-off-by: Kasumov Ruslan <s02210418@gse.cs.msu.ru> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
Hi Alexey and Ruslan, On Tue, Mar 14, 2023 at 11:07:19PM +0100, Linus Walleij wrote: > On Tue, Mar 14, 2023 at 11:03 PM Alexey Khoroshilov > <khoroshilov@ispras.ru> wrote: > > > As far as I can see sentinel is an "empty" element of xoadc_channel in > > the array, i.e. hwchan->datasheet_name works as a sentinel while hwchan > > is always non NULL. > > You're right, I was unable to understand my own code :( At this time of the day I got alarmed too. Happens :) Please ignore my previous comment but still no need for the Fixes: tag from the commit log as it's a cleanup and not a bug fix. Thanks, Andi
diff --git a/drivers/iio/adc/qcom-pm8xxx-xoadc.c b/drivers/iio/adc/qcom-pm8xxx-xoadc.c index eb424496ee1d..64a3aeb6261c 100644 --- a/drivers/iio/adc/qcom-pm8xxx-xoadc.c +++ b/drivers/iio/adc/qcom-pm8xxx-xoadc.c @@ -758,7 +758,7 @@ static int pm8xxx_xoadc_parse_channel(struct device *dev, /* Find the right channel setting */ chid = 0; hwchan = &hw_channels[0]; - while (hwchan && hwchan->datasheet_name) { + while (hwchan->datasheet_name) { if (hwchan->pre_scale_mux == pre_scale_mux && hwchan->amux_channel == amux_channel) break;
The left side of the loop condition never becomes false. hwchan cannot be NULL, because it points to elements of the hw_channels array that takes one of 4 predefined values: pm8018_xoadc_channels, pm8038_xoadc_channels, pm8058_xoadc_channels, pm8921_xoadc_channels. Found by Linux Verification Center (linuxtesting.org) with SVACE. Fixes: 63c3ecd946d4 ("iio: adc: add a driver for Qualcomm PM8xxx HK/XOADC") Signed-off-by: Kasumov Ruslan <s02210418@gse.cs.msu.ru> --- drivers/iio/adc/qcom-pm8xxx-xoadc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)