* [PATCH] iio: as3935: improve error reporting in as3935_event_work
@ 2016-05-30 14:52 Arnd Bergmann
2016-05-30 17:33 ` Matt Ranostay
2016-05-31 14:53 ` Andrew F. Davis
0 siblings, 2 replies; 6+ messages in thread
From: Arnd Bergmann @ 2016-05-30 14:52 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Arnd Bergmann, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, Mark Brown, Andrew F. Davis,
Javier Martinez Canillas, linux-iio, linux-kernel
gcc warns about a potentially uninitialized variable use
in as3935_event_work:
drivers/iio/proximity/as3935.c: In function ‘as3935_event_work’:
drivers/iio/proximity/as3935.c:231:6: error: ‘val’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
This case specifically happens when spi_w8r8() fails with a
negative return code. We check all other users of this function
except this one.
As the error is rather unlikely to happen after the device
has already been initialized, this just adds a dev_warn().
Another warning already existst in the same function, but is
missing a trailing '\n' character, so I'm fixing that too.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
drivers/iio/proximity/as3935.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/iio/proximity/as3935.c b/drivers/iio/proximity/as3935.c
index f4d29d5dbd5f..b49e3ab5730a 100644
--- a/drivers/iio/proximity/as3935.c
+++ b/drivers/iio/proximity/as3935.c
@@ -224,10 +224,16 @@ static void as3935_event_work(struct work_struct *work)
{
struct as3935_state *st;
int val;
+ int ret;
st = container_of(work, struct as3935_state, work.work);
- as3935_read(st, AS3935_INT, &val);
+ ret = as3935_read(st, AS3935_INT, &val);
+ if (ret) {
+ dev_warn(&st->spi->dev, "read error\n");
+ return;
+ }
+
val &= AS3935_INT_MASK;
switch (val) {
@@ -235,7 +241,7 @@ static void as3935_event_work(struct work_struct *work)
iio_trigger_poll(st->trig);
break;
case AS3935_NOISE_INT:
- dev_warn(&st->spi->dev, "noise level is too high");
+ dev_warn(&st->spi->dev, "noise level is too high\n");
break;
}
}
--
2.7.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] iio: as3935: improve error reporting in as3935_event_work
2016-05-30 14:52 [PATCH] iio: as3935: improve error reporting in as3935_event_work Arnd Bergmann
@ 2016-05-30 17:33 ` Matt Ranostay
2016-05-31 14:53 ` Andrew F. Davis
1 sibling, 0 replies; 6+ messages in thread
From: Matt Ranostay @ 2016-05-30 17:33 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, Mark Brown, Andrew F. Davis,
Javier Martinez Canillas, linux-iio, linux-kernel
Reviewed-by: Matt Ranostay <mranostay@gmail.com>
On Mon, May 30, 2016 at 7:52 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> gcc warns about a potentially uninitialized variable use
> in as3935_event_work:
>
> drivers/iio/proximity/as3935.c: In function ‘as3935_event_work’:
> drivers/iio/proximity/as3935.c:231:6: error: ‘val’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>
> This case specifically happens when spi_w8r8() fails with a
> negative return code. We check all other users of this function
> except this one.
>
> As the error is rather unlikely to happen after the device
> has already been initialized, this just adds a dev_warn().
> Another warning already existst in the same function, but is
> missing a trailing '\n' character, so I'm fixing that too.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> drivers/iio/proximity/as3935.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iio/proximity/as3935.c b/drivers/iio/proximity/as3935.c
> index f4d29d5dbd5f..b49e3ab5730a 100644
> --- a/drivers/iio/proximity/as3935.c
> +++ b/drivers/iio/proximity/as3935.c
> @@ -224,10 +224,16 @@ static void as3935_event_work(struct work_struct *work)
> {
> struct as3935_state *st;
> int val;
> + int ret;
>
> st = container_of(work, struct as3935_state, work.work);
>
> - as3935_read(st, AS3935_INT, &val);
> + ret = as3935_read(st, AS3935_INT, &val);
> + if (ret) {
> + dev_warn(&st->spi->dev, "read error\n");
> + return;
> + }
> +
> val &= AS3935_INT_MASK;
>
> switch (val) {
> @@ -235,7 +241,7 @@ static void as3935_event_work(struct work_struct *work)
> iio_trigger_poll(st->trig);
> break;
> case AS3935_NOISE_INT:
> - dev_warn(&st->spi->dev, "noise level is too high");
> + dev_warn(&st->spi->dev, "noise level is too high\n");
> break;
> }
> }
> --
> 2.7.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] iio: as3935: improve error reporting in as3935_event_work
2016-05-30 14:52 [PATCH] iio: as3935: improve error reporting in as3935_event_work Arnd Bergmann
2016-05-30 17:33 ` Matt Ranostay
@ 2016-05-31 14:53 ` Andrew F. Davis
2016-06-11 16:32 ` Jonathan Cameron
1 sibling, 1 reply; 6+ messages in thread
From: Andrew F. Davis @ 2016-05-31 14:53 UTC (permalink / raw)
To: Arnd Bergmann, Jonathan Cameron
Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
Mark Brown, Javier Martinez Canillas, linux-iio, linux-kernel
On 05/30/2016 09:52 AM, Arnd Bergmann wrote:
> gcc warns about a potentially uninitialized variable use
> in as3935_event_work:
>
> drivers/iio/proximity/as3935.c: In function ‘as3935_event_work’:
> drivers/iio/proximity/as3935.c:231:6: error: ‘val’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>
> This case specifically happens when spi_w8r8() fails with a
> negative return code. We check all other users of this function
> except this one.
>
> As the error is rather unlikely to happen after the device
> has already been initialized, this just adds a dev_warn().
> Another warning already existst in the same function, but is
^^ typo
> missing a trailing '\n' character, so I'm fixing that too.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> drivers/iio/proximity/as3935.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iio/proximity/as3935.c b/drivers/iio/proximity/as3935.c
> index f4d29d5dbd5f..b49e3ab5730a 100644
> --- a/drivers/iio/proximity/as3935.c
> +++ b/drivers/iio/proximity/as3935.c
> @@ -224,10 +224,16 @@ static void as3935_event_work(struct work_struct *work)
> {
> struct as3935_state *st;
> int val;
> + int ret;
>
> st = container_of(work, struct as3935_state, work.work);
>
> - as3935_read(st, AS3935_INT, &val);
> + ret = as3935_read(st, AS3935_INT, &val);
> + if (ret) {
> + dev_warn(&st->spi->dev, "read error\n");
Maybe I'm misunderstanding the commit message, why does this error not
use dev_err()? A read error here would be rather serious, it might even
be worth it to return a code and fail through the caller too.
> + return;
> + }
> +
> val &= AS3935_INT_MASK;
>
> switch (val) {
> @@ -235,7 +241,7 @@ static void as3935_event_work(struct work_struct *work)
> iio_trigger_poll(st->trig);
> break;
> case AS3935_NOISE_INT:
> - dev_warn(&st->spi->dev, "noise level is too high");
> + dev_warn(&st->spi->dev, "noise level is too high\n");
> break;
> }
> }
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] iio: as3935: improve error reporting in as3935_event_work
2016-05-31 14:53 ` Andrew F. Davis
@ 2016-06-11 16:32 ` Jonathan Cameron
2016-06-12 6:55 ` Matt Ranostay
0 siblings, 1 reply; 6+ messages in thread
From: Jonathan Cameron @ 2016-06-11 16:32 UTC (permalink / raw)
To: Andrew F. Davis, Arnd Bergmann
Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
Mark Brown, Javier Martinez Canillas, linux-iio, linux-kernel
On 31/05/16 15:53, Andrew F. Davis wrote:
> On 05/30/2016 09:52 AM, Arnd Bergmann wrote:
>> gcc warns about a potentially uninitialized variable use
>> in as3935_event_work:
>>
>> drivers/iio/proximity/as3935.c: In function ‘as3935_event_work’:
>> drivers/iio/proximity/as3935.c:231:6: error: ‘val’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>>
>> This case specifically happens when spi_w8r8() fails with a
>> negative return code. We check all other users of this function
>> except this one.
>>
>> As the error is rather unlikely to happen after the device
>> has already been initialized, this just adds a dev_warn().
>> Another warning already existst in the same function, but is
>
> ^^ typo
>
>> missing a trailing '\n' character, so I'm fixing that too.
>>
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>> ---
>> drivers/iio/proximity/as3935.c | 10 ++++++++--
>> 1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/iio/proximity/as3935.c b/drivers/iio/proximity/as3935.c
>> index f4d29d5dbd5f..b49e3ab5730a 100644
>> --- a/drivers/iio/proximity/as3935.c
>> +++ b/drivers/iio/proximity/as3935.c
>> @@ -224,10 +224,16 @@ static void as3935_event_work(struct work_struct *work)
>> {
>> struct as3935_state *st;
>> int val;
>> + int ret;
>>
>> st = container_of(work, struct as3935_state, work.work);
>>
>> - as3935_read(st, AS3935_INT, &val);
>> + ret = as3935_read(st, AS3935_INT, &val);
>> + if (ret) {
>> + dev_warn(&st->spi->dev, "read error\n");
>
> Maybe I'm misunderstanding the commit message, why does this error not
> use dev_err()? A read error here would be rather serious, it might even
> be worth it to return a code and fail through the caller too.
They are unusual and typically result in momentary corruption. Hmm.
As this is in a work function, there is no easy way of actually
passing the error upstream.. dev_err is a little brutal so perhaps
this is the best option...
>
>> + return;
>> + }
>> +
>> val &= AS3935_INT_MASK;
>>
>> switch (val) {
>> @@ -235,7 +241,7 @@ static void as3935_event_work(struct work_struct *work)
>> iio_trigger_poll(st->trig);
>> break;
>> case AS3935_NOISE_INT:
>> - dev_warn(&st->spi->dev, "noise level is too high");
>> + dev_warn(&st->spi->dev, "noise level is too high\n");
>> break;
>> }
>> }
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] iio: as3935: improve error reporting in as3935_event_work
2016-06-11 16:32 ` Jonathan Cameron
@ 2016-06-12 6:55 ` Matt Ranostay
2016-06-26 16:05 ` Jonathan Cameron
0 siblings, 1 reply; 6+ messages in thread
From: Matt Ranostay @ 2016-06-12 6:55 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Andrew F. Davis, Arnd Bergmann, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald-Stadler, Mark Brown,
Javier Martinez Canillas, linux-iio, linux-kernel
Also important to note these warnings are environment related (e.g. room with lot of EMI noise) and unlikely a chip misconfiguration. Unless the tuning capacitor setting is wrong of course
> On Jun 11, 2016, at 09:32, Jonathan Cameron <jic23@kernel.org> wrote:
>
>> On 31/05/16 15:53, Andrew F. Davis wrote:
>>> On 05/30/2016 09:52 AM, Arnd Bergmann wrote:
>>> gcc warns about a potentially uninitialized variable use
>>> in as3935_event_work:
>>>
>>> drivers/iio/proximity/as3935.c: In function ‘as3935_event_work’:
>>> drivers/iio/proximity/as3935.c:231:6: error: ‘val’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>>>
>>> This case specifically happens when spi_w8r8() fails with a
>>> negative return code. We check all other users of this function
>>> except this one.
>>>
>>> As the error is rather unlikely to happen after the device
>>> has already been initialized, this just adds a dev_warn().
>>> Another warning already existst in the same function, but is
>>
>> ^^ typo
>>
>>> missing a trailing '\n' character, so I'm fixing that too.
>>>
>>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>>> ---
>>> drivers/iio/proximity/as3935.c | 10 ++++++++--
>>> 1 file changed, 8 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/iio/proximity/as3935.c b/drivers/iio/proximity/as3935.c
>>> index f4d29d5dbd5f..b49e3ab5730a 100644
>>> --- a/drivers/iio/proximity/as3935.c
>>> +++ b/drivers/iio/proximity/as3935.c
>>> @@ -224,10 +224,16 @@ static void as3935_event_work(struct work_struct *work)
>>> {
>>> struct as3935_state *st;
>>> int val;
>>> + int ret;
>>>
>>> st = container_of(work, struct as3935_state, work.work);
>>>
>>> - as3935_read(st, AS3935_INT, &val);
>>> + ret = as3935_read(st, AS3935_INT, &val);
>>> + if (ret) {
>>> + dev_warn(&st->spi->dev, "read error\n");
>>
>> Maybe I'm misunderstanding the commit message, why does this error not
>> use dev_err()? A read error here would be rather serious, it might even
>> be worth it to return a code and fail through the caller too.
> They are unusual and typically result in momentary corruption. Hmm.
> As this is in a work function, there is no easy way of actually
> passing the error upstream.. dev_err is a little brutal so perhaps
> this is the best option...
>>
>>> + return;
>>> + }
>>> +
>>> val &= AS3935_INT_MASK;
>>>
>>> switch (val) {
>>> @@ -235,7 +241,7 @@ static void as3935_event_work(struct work_struct *work)
>>> iio_trigger_poll(st->trig);
>>> break;
>>> case AS3935_NOISE_INT:
>>> - dev_warn(&st->spi->dev, "noise level is too high");
>>> + dev_warn(&st->spi->dev, "noise level is too high\n");
>>> break;
>>> }
>>> }
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] iio: as3935: improve error reporting in as3935_event_work
2016-06-12 6:55 ` Matt Ranostay
@ 2016-06-26 16:05 ` Jonathan Cameron
0 siblings, 0 replies; 6+ messages in thread
From: Jonathan Cameron @ 2016-06-26 16:05 UTC (permalink / raw)
To: Matt Ranostay
Cc: Andrew F. Davis, Arnd Bergmann, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald-Stadler, Mark Brown,
Javier Martinez Canillas, linux-iio, linux-kernel
On 12/06/16 07:55, Matt Ranostay wrote:
>
> Also important to note these warnings are environment related (e.g. room with lot of EMI noise) and unlikely a chip misconfiguration. Unless the tuning capacitor setting is wrong of course
Applied to the togreg branch of iio.git. Initially pushed out as testing
for the autobuilders to play with it.
Thanks,
Jonathan
>
>> On Jun 11, 2016, at 09:32, Jonathan Cameron <jic23@kernel.org> wrote:
>>
>>> On 31/05/16 15:53, Andrew F. Davis wrote:
>>>> On 05/30/2016 09:52 AM, Arnd Bergmann wrote:
>>>> gcc warns about a potentially uninitialized variable use
>>>> in as3935_event_work:
>>>>
>>>> drivers/iio/proximity/as3935.c: In function ‘as3935_event_work’:
>>>> drivers/iio/proximity/as3935.c:231:6: error: ‘val’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>>>>
>>>> This case specifically happens when spi_w8r8() fails with a
>>>> negative return code. We check all other users of this function
>>>> except this one.
>>>>
>>>> As the error is rather unlikely to happen after the device
>>>> has already been initialized, this just adds a dev_warn().
>>>> Another warning already existst in the same function, but is
>>>
>>> ^^ typo
>>>
>>>> missing a trailing '\n' character, so I'm fixing that too.
>>>>
>>>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>>>> ---
>>>> drivers/iio/proximity/as3935.c | 10 ++++++++--
>>>> 1 file changed, 8 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/iio/proximity/as3935.c b/drivers/iio/proximity/as3935.c
>>>> index f4d29d5dbd5f..b49e3ab5730a 100644
>>>> --- a/drivers/iio/proximity/as3935.c
>>>> +++ b/drivers/iio/proximity/as3935.c
>>>> @@ -224,10 +224,16 @@ static void as3935_event_work(struct work_struct *work)
>>>> {
>>>> struct as3935_state *st;
>>>> int val;
>>>> + int ret;
>>>>
>>>> st = container_of(work, struct as3935_state, work.work);
>>>>
>>>> - as3935_read(st, AS3935_INT, &val);
>>>> + ret = as3935_read(st, AS3935_INT, &val);
>>>> + if (ret) {
>>>> + dev_warn(&st->spi->dev, "read error\n");
>>>
>>> Maybe I'm misunderstanding the commit message, why does this error not
>>> use dev_err()? A read error here would be rather serious, it might even
>>> be worth it to return a code and fail through the caller too.
>> They are unusual and typically result in momentary corruption. Hmm.
>> As this is in a work function, there is no easy way of actually
>> passing the error upstream.. dev_err is a little brutal so perhaps
>> this is the best option...
>>>
>>>> + return;
>>>> + }
>>>> +
>>>> val &= AS3935_INT_MASK;
>>>>
>>>> switch (val) {
>>>> @@ -235,7 +241,7 @@ static void as3935_event_work(struct work_struct *work)
>>>> iio_trigger_poll(st->trig);
>>>> break;
>>>> case AS3935_NOISE_INT:
>>>> - dev_warn(&st->spi->dev, "noise level is too high");
>>>> + dev_warn(&st->spi->dev, "noise level is too high\n");
>>>> break;
>>>> }
>>>> }
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-06-26 16:05 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-05-30 14:52 [PATCH] iio: as3935: improve error reporting in as3935_event_work Arnd Bergmann
2016-05-30 17:33 ` Matt Ranostay
2016-05-31 14:53 ` Andrew F. Davis
2016-06-11 16:32 ` Jonathan Cameron
2016-06-12 6:55 ` Matt Ranostay
2016-06-26 16:05 ` Jonathan Cameron
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).