diff mbox series

iio: adc: qcom-pm8xxx-xoadc: Remove useless condition in pm8xxx_xoadc_parse_channel()

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

Commit Message

Kasumov Ruslan March 14, 2023, 7:37 p.m. UTC 
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(-)

Comments

Andi Shyti March 14, 2023, 8:42 p.m. UTC | #1 
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
>
Linus Walleij March 14, 2023, 9:12 p.m. UTC | #2 
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
Andi Shyti March 14, 2023, 9:28 p.m. UTC | #3 
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
Alexey Khoroshilov March 14, 2023, 10:03 p.m. UTC | #4 
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
Linus Walleij March 14, 2023, 10:04 p.m. UTC | #5 
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
Linus Walleij March 14, 2023, 10:07 p.m. UTC | #6 
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
Linus Walleij March 14, 2023, 10:16 p.m. UTC | #7 
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
Andi Shyti March 14, 2023, 10:41 p.m. UTC | #8 
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 mbox series

Patch

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;