linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] iio: adc: handle unknow of_device_id data
@ 2017-02-03 17:01 Arnd Bergmann
  2017-02-04 21:11 ` Marek Vasut
  0 siblings, 1 reply; 6+ messages in thread
From: Arnd Bergmann @ 2017-02-03 17:01 UTC (permalink / raw)
  To: Jonathan Cameron, Arnd Bergmann, Marek Vasut
  Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
	linux-iio, linux-kernel

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

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH] iio: adc: handle unknow of_device_id data
  2017-02-03 17:01 [PATCH] iio: adc: handle unknow of_device_id data Arnd Bergmann
@ 2017-02-04 21:11 ` Marek Vasut
  2017-02-04 21:44   ` Arnd Bergmann
  0 siblings, 1 reply; 6+ messages in thread
From: Marek Vasut @ 2017-02-04 21:11 UTC (permalink / raw)
  To: Arnd Bergmann, Jonathan Cameron, Marek Vasut
  Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
	linux-iio, linux-kernel

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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] iio: adc: handle unknow of_device_id data
  2017-02-04 21:11 ` Marek Vasut
@ 2017-02-04 21:44   ` Arnd Bergmann
  2017-02-04 21:47     ` Marek Vasut
  0 siblings, 1 reply; 6+ messages in thread
From: Arnd Bergmann @ 2017-02-04 21:44 UTC (permalink / raw)
  To: Marek Vasut
  Cc: Jonathan Cameron, Marek Vasut, Hartmut Knaack,
	Lars-Peter Clausen, Peter Meerwald-Stadler, linux-iio,
	Linux Kernel Mailing List

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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] iio: adc: handle unknow of_device_id data
  2017-02-04 21:44   ` Arnd Bergmann
@ 2017-02-04 21:47     ` Marek Vasut
  2017-02-05  9:32       ` Jonathan Cameron
  0 siblings, 1 reply; 6+ messages in thread
From: Marek Vasut @ 2017-02-04 21:47 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Jonathan Cameron, Marek Vasut, Hartmut Knaack,
	Lars-Peter Clausen, Peter Meerwald-Stadler, linux-iio,
	Linux Kernel Mailing List

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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] iio: adc: handle unknow of_device_id data
  2017-02-04 21:47     ` Marek Vasut
@ 2017-02-05  9:32       ` Jonathan Cameron
  2017-02-08  1:13         ` Marek Vasut
  0 siblings, 1 reply; 6+ messages in thread
From: Jonathan Cameron @ 2017-02-05  9:32 UTC (permalink / raw)
  To: Marek Vasut, Arnd Bergmann
  Cc: Marek Vasut, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, linux-iio, Linux Kernel Mailing List

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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] iio: adc: handle unknow of_device_id data
  2017-02-05  9:32       ` Jonathan Cameron
@ 2017-02-08  1:13         ` Marek Vasut
  0 siblings, 0 replies; 6+ messages in thread
From: Marek Vasut @ 2017-02-08  1:13 UTC (permalink / raw)
  To: Jonathan Cameron, Arnd Bergmann
  Cc: Marek Vasut, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, linux-iio, Linux Kernel Mailing List

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

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-02-08  1:22 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-02-03 17:01 [PATCH] iio: adc: handle unknow of_device_id data Arnd Bergmann
2017-02-04 21:11 ` Marek Vasut
2017-02-04 21:44   ` Arnd Bergmann
2017-02-04 21:47     ` Marek Vasut
2017-02-05  9:32       ` Jonathan Cameron
2017-02-08  1:13         ` Marek Vasut

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).