Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1573391 > unrolled thread

[PATCH] iio: adc: handle unknow of_device_id data

Back to article view | Back to linux.kernel

Contents

  [PATCH] iio: adc: handle unknow of_device_id data Arnd Bergmann <arnd@arndb.de> - 2017-02-03 20:40 +0100
    Re: [PATCH] iio: adc: handle unknow of_device_id data Marek Vasut <marek.vasut@gmail.com> - 2017-02-04 22:20 +0100
      Re: [PATCH] iio: adc: handle unknow of_device_id data Marek Vasut <marek.vasut@gmail.com> - 2017-02-04 22:50 +0100
        Re: [PATCH] iio: adc: handle unknow of_device_id data Jonathan Cameron <jic23@kernel.org> - 2017-02-05 10:40 +0100
          Re: [PATCH] iio: adc: handle unknow of_device_id data Marek Vasut <marek.vasut@gmail.com> - 2017-02-08 02:30 +0100
      Re: [PATCH] iio: adc: handle unknow of_device_id data Arnd Bergmann <arnd@arndb.de> - 2017-02-04 22:50 +0100

#1573391 — [PATCH] iio: adc: handle unknow of_device_id data

If we get an unknown 'childmode' value, a number of variables are not
initialized properly:

drivers/iio/adc/rcar-gyroadc.c: In function 'rcar_gyroadc_probe':
drivers/iio/adc/rcar-gyroadc.c:390:5: error: 'num_channels' may be used uninitialized in this function [-Werror=maybe-uninitialized]
drivers/iio/adc/rcar-gyroadc.c:426:22: error: 'sample_width' may be used uninitialized in this function [-Werror=maybe-uninitialized]
drivers/iio/adc/rcar-gyroadc.c:428:23: error: 'channels' may be used uninitialized in this function [-Werror=maybe-uninitialized]

The driver is currently correct, but handling this properly is more robust
for possible modifications.

There is also a false-positive warning about adcmode being possibly uninitialized,
but that cannot happen as we also check the 'first' flag:

drivers/iio/adc/rcar-gyroadc.c:398:26: error: 'adcmode' may be used uninitialized in this function [-Werror=maybe-uninitialized]

This adds an initialization for 'adcmode' and bails out for any unknown childmode.

Fixes: 059c53b32329 ("iio: adc: Add Renesas GyroADC driver")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/iio/adc/rcar-gyroadc.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/adc/rcar-gyroadc.c b/drivers/iio/adc/rcar-gyroadc.c
index 0c44f72c32a8..331ff9a673be 100644
--- a/drivers/iio/adc/rcar-gyroadc.c
+++ b/drivers/iio/adc/rcar-gyroadc.c
@@ -336,7 +336,7 @@ static int rcar_gyroadc_parse_subdevs(struct iio_dev *indio_dev)
 	struct device_node *child;
 	struct regulator *vref;
 	unsigned int reg;
-	unsigned int adcmode, childmode;
+	unsigned int adcmode = -1, childmode;
 	unsigned int sample_width;
 	unsigned int num_channels;
 	int ret, first = 1;
@@ -366,6 +366,9 @@ static int rcar_gyroadc_parse_subdevs(struct iio_dev *indio_dev)
 			channels = rcar_gyroadc_iio_channels_3;
 			num_channels = ARRAY_SIZE(rcar_gyroadc_iio_channels_3);
 			break;
+		default:
+			dev_err(dev, "unknown device type");
+			return -EINVAL;
 		}
 
 		/*
-- 
2.9.0

[toc] | [next] | [standalone]

#1573757

On 02/03/2017 06:01 PM, Arnd Bergmann wrote:
> If we get an unknown 'childmode' value, a number of variables are not
> initialized properly:
> 
> drivers/iio/adc/rcar-gyroadc.c: In function 'rcar_gyroadc_probe':
> drivers/iio/adc/rcar-gyroadc.c:390:5: error: 'num_channels' may be used uninitialized in this function [-Werror=maybe-uninitialized]
> drivers/iio/adc/rcar-gyroadc.c:426:22: error: 'sample_width' may be used uninitialized in this function [-Werror=maybe-uninitialized]
> drivers/iio/adc/rcar-gyroadc.c:428:23: error: 'channels' may be used uninitialized in this function [-Werror=maybe-uninitialized]
> 
> The driver is currently correct, but handling this properly is more robust
> for possible modifications.

Yeah, the OF match will make sure this case is never triggered.
But then again, it triggers kernel ci, so it should be fixed.
Thanks for the patch :)

> There is also a false-positive warning about adcmode being possibly uninitialized,
> but that cannot happen as we also check the 'first' flag:
> 
> drivers/iio/adc/rcar-gyroadc.c:398:26: error: 'adcmode' may be used uninitialized in this function [-Werror=maybe-uninitialized]
> 
> This adds an initialization for 'adcmode' and bails out for any unknown childmode.
> 
> Fixes: 059c53b32329 ("iio: adc: Add Renesas GyroADC driver")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/iio/adc/rcar-gyroadc.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/rcar-gyroadc.c b/drivers/iio/adc/rcar-gyroadc.c
> index 0c44f72c32a8..331ff9a673be 100644
> --- a/drivers/iio/adc/rcar-gyroadc.c
> +++ b/drivers/iio/adc/rcar-gyroadc.c
> @@ -336,7 +336,7 @@ static int rcar_gyroadc_parse_subdevs(struct iio_dev *indio_dev)
>  	struct device_node *child;
>  	struct regulator *vref;
>  	unsigned int reg;
> -	unsigned int adcmode, childmode;
> +	unsigned int adcmode = -1, childmode;
>  	unsigned int sample_width;
>  	unsigned int num_channels;
>  	int ret, first = 1;
> @@ -366,6 +366,9 @@ static int rcar_gyroadc_parse_subdevs(struct iio_dev *indio_dev)
>  			channels = rcar_gyroadc_iio_channels_3;
>  			num_channels = ARRAY_SIZE(rcar_gyroadc_iio_channels_3);
>  			break;
> +		default:
> +			dev_err(dev, "unknown device type");

Is this verbose output really needed ?

> +			return -EINVAL;
>  		}
>  
>  		/*
> 


-- 
Best regards,
Marek Vasut

[toc] | [prev] | [next] | [standalone]

#1573760

On 02/04/2017 10:44 PM, Arnd Bergmann wrote:
> On Sat, Feb 4, 2017 at 10:11 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
>> On 02/03/2017 06:01 PM, Arnd Bergmann wrote:
>>> diff --git a/drivers/iio/adc/rcar-gyroadc.c b/drivers/iio/adc/rcar-gyroadc.c
>>> index 0c44f72c32a8..331ff9a673be 100644
>>> --- a/drivers/iio/adc/rcar-gyroadc.c
>>> +++ b/drivers/iio/adc/rcar-gyroadc.c
>>> @@ -336,7 +336,7 @@ static int rcar_gyroadc_parse_subdevs(struct iio_dev *indio_dev)
>>>       struct device_node *child;
>>>       struct regulator *vref;
>>>       unsigned int reg;
>>> -     unsigned int adcmode, childmode;
>>> +     unsigned int adcmode = -1, childmode;
>>>       unsigned int sample_width;
>>>       unsigned int num_channels;
>>>       int ret, first = 1;
>>> @@ -366,6 +366,9 @@ static int rcar_gyroadc_parse_subdevs(struct iio_dev *indio_dev)
>>>                       channels = rcar_gyroadc_iio_channels_3;
>>>                       num_channels = ARRAY_SIZE(rcar_gyroadc_iio_channels_3);
>>>                       break;
>>> +             default:
>>> +                     dev_err(dev, "unknown device type");
>>
>> Is this verbose output really needed ?
> 
> I don't care much either way, I was just trying to follow what the function does
> for the other failure cases. We can probably drop it though, as this
> would indicate
> a bug in the driver implementation, while the others are about
> incorrect DT data.

I don't really want to pester you too much with minor details, let's
wait for JIC's opinion. If you're resending, please add my Ack,
otherwise consider this patch:

Acked-by: Marek Vasut <marek.vasut@gmail.com>

-- 
Best regards,
Marek Vasut

[toc] | [prev] | [next] | [standalone]

#1573848

On 04/02/17 21:47, Marek Vasut wrote:
> On 02/04/2017 10:44 PM, Arnd Bergmann wrote:
>> On Sat, Feb 4, 2017 at 10:11 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
>>> On 02/03/2017 06:01 PM, Arnd Bergmann wrote:
>>>> diff --git a/drivers/iio/adc/rcar-gyroadc.c b/drivers/iio/adc/rcar-gyroadc.c
>>>> index 0c44f72c32a8..331ff9a673be 100644
>>>> --- a/drivers/iio/adc/rcar-gyroadc.c
>>>> +++ b/drivers/iio/adc/rcar-gyroadc.c
>>>> @@ -336,7 +336,7 @@ static int rcar_gyroadc_parse_subdevs(struct iio_dev *indio_dev)
>>>>       struct device_node *child;
>>>>       struct regulator *vref;
>>>>       unsigned int reg;
>>>> -     unsigned int adcmode, childmode;
>>>> +     unsigned int adcmode = -1, childmode;
>>>>       unsigned int sample_width;
>>>>       unsigned int num_channels;
>>>>       int ret, first = 1;
>>>> @@ -366,6 +366,9 @@ static int rcar_gyroadc_parse_subdevs(struct iio_dev *indio_dev)
>>>>                       channels = rcar_gyroadc_iio_channels_3;
>>>>                       num_channels = ARRAY_SIZE(rcar_gyroadc_iio_channels_3);
>>>>                       break;
>>>> +             default:
>>>> +                     dev_err(dev, "unknown device type");
>>>
>>> Is this verbose output really needed ?
>>
>> I don't care much either way, I was just trying to follow what the function does
>> for the other failure cases. We can probably drop it though, as this
>> would indicate
>> a bug in the driver implementation, while the others are about
>> incorrect DT data.
> 
> I don't really want to pester you too much with minor details, let's
> wait for JIC's opinion. If you're resending, please add my Ack,
> otherwise consider this patch:
> 
> Acked-by: Marek Vasut <marek.vasut@gmail.com>
> 
I've applied with the dev_err dropped.  Didn't care that much either
way :)

Anyhow, applied to the fixes-togreg-post-rc1 branch of iio.git as
I might not get another pull request out pre the merge window.
(if I do I'll apply it with that).

Thanks,

Jonathan

[toc] | [prev] | [next] | [standalone]

#1576211

On 02/05/2017 10:32 AM, Jonathan Cameron wrote:
> On 04/02/17 21:47, Marek Vasut wrote:
>> On 02/04/2017 10:44 PM, Arnd Bergmann wrote:
>>> On Sat, Feb 4, 2017 at 10:11 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
>>>> On 02/03/2017 06:01 PM, Arnd Bergmann wrote:
>>>>> diff --git a/drivers/iio/adc/rcar-gyroadc.c b/drivers/iio/adc/rcar-gyroadc.c
>>>>> index 0c44f72c32a8..331ff9a673be 100644
>>>>> --- a/drivers/iio/adc/rcar-gyroadc.c
>>>>> +++ b/drivers/iio/adc/rcar-gyroadc.c
>>>>> @@ -336,7 +336,7 @@ static int rcar_gyroadc_parse_subdevs(struct iio_dev *indio_dev)
>>>>>       struct device_node *child;
>>>>>       struct regulator *vref;
>>>>>       unsigned int reg;
>>>>> -     unsigned int adcmode, childmode;
>>>>> +     unsigned int adcmode = -1, childmode;
>>>>>       unsigned int sample_width;
>>>>>       unsigned int num_channels;
>>>>>       int ret, first = 1;
>>>>> @@ -366,6 +366,9 @@ static int rcar_gyroadc_parse_subdevs(struct iio_dev *indio_dev)
>>>>>                       channels = rcar_gyroadc_iio_channels_3;
>>>>>                       num_channels = ARRAY_SIZE(rcar_gyroadc_iio_channels_3);
>>>>>                       break;
>>>>> +             default:
>>>>> +                     dev_err(dev, "unknown device type");
>>>>
>>>> Is this verbose output really needed ?
>>>
>>> I don't care much either way, I was just trying to follow what the function does
>>> for the other failure cases. We can probably drop it though, as this
>>> would indicate
>>> a bug in the driver implementation, while the others are about
>>> incorrect DT data.
>>
>> I don't really want to pester you too much with minor details, let's
>> wait for JIC's opinion. If you're resending, please add my Ack,
>> otherwise consider this patch:
>>
>> Acked-by: Marek Vasut <marek.vasut@gmail.com>
>>
> I've applied with the dev_err dropped.  Didn't care that much either
> way :)

Noone seems to care much, but everyone is slightly opinionated, hehe :)

> Anyhow, applied to the fixes-togreg-post-rc1 branch of iio.git as
> I might not get another pull request out pre the merge window.
> (if I do I'll apply it with that).
> 
> Thanks,
> 
> Jonathan
> 
Thanks

-- 
Best regards,
Marek Vasut

[toc] | [prev] | [next] | [standalone]

#1573761

On Sat, Feb 4, 2017 at 10:11 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
> On 02/03/2017 06:01 PM, Arnd Bergmann wrote:
>> diff --git a/drivers/iio/adc/rcar-gyroadc.c b/drivers/iio/adc/rcar-gyroadc.c
>> index 0c44f72c32a8..331ff9a673be 100644
>> --- a/drivers/iio/adc/rcar-gyroadc.c
>> +++ b/drivers/iio/adc/rcar-gyroadc.c
>> @@ -336,7 +336,7 @@ static int rcar_gyroadc_parse_subdevs(struct iio_dev *indio_dev)
>>       struct device_node *child;
>>       struct regulator *vref;
>>       unsigned int reg;
>> -     unsigned int adcmode, childmode;
>> +     unsigned int adcmode = -1, childmode;
>>       unsigned int sample_width;
>>       unsigned int num_channels;
>>       int ret, first = 1;
>> @@ -366,6 +366,9 @@ static int rcar_gyroadc_parse_subdevs(struct iio_dev *indio_dev)
>>                       channels = rcar_gyroadc_iio_channels_3;
>>                       num_channels = ARRAY_SIZE(rcar_gyroadc_iio_channels_3);
>>                       break;
>> +             default:
>> +                     dev_err(dev, "unknown device type");
>
> Is this verbose output really needed ?

I don't care much either way, I was just trying to follow what the function does
for the other failure cases. We can probably drop it though, as this
would indicate
a bug in the driver implementation, while the others are about
incorrect DT data.

    Arnd

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web