* Re: [patch] iio: adc: at91: signedness bug in at91_adc_get_trigger_value_by_name()
@ 2014-06-11 7:36 ` walter harms
0 siblings, 0 replies; 14+ messages in thread
From: walter harms @ 2014-06-11 7:36 UTC (permalink / raw)
To: Dan Carpenter
Cc: Jonathan Cameron, Grant Likely, Rob Herring, Josh Wu,
Alexandre Belloni, Nicolas Ferre, Maxime Ripard,
Lars-Peter Clausen, Thomas Meyer, Wei Yongjun, Sachin Kamat,
linux-iio, kernel-janitors
Am 11.06.2014 08:53, schrieb Dan Carpenter:
> The function returns a u8 so the -ENOMEM is truncated to a positive
> value. The caller tests for zero returns and treats them as an error
> so I have changed the -ENOMEM to 0.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> diff --git a/drivers/iio/adc/at91_adc.c b/drivers/iio/adc/at91_adc.c
> index 3b5bacd..5ebe33d 100644
> --- a/drivers/iio/adc/at91_adc.c
> +++ b/drivers/iio/adc/at91_adc.c
> @@ -525,7 +525,7 @@ static u8 at91_adc_get_trigger_value_by_name(struct iio_dev *idev,
> idev->id,
> triggers[i].name);
> if (!name)
> - return -ENOMEM;
> + return 0;
>
> if (strcmp(trigger_name, name) == 0) {
> value = triggers[i].value;
hi Dan,
LXR says there is only 1 caller (and he is checking for ENOMON)
perhaps it is better to change the return value to int.
re,
wh
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [patch] iio: adc: at91: signedness bug in at91_adc_get_trigger_value_by_name()
2014-06-11 7:36 ` walter harms
@ 2014-06-11 8:12 ` Dan Carpenter
-1 siblings, 0 replies; 14+ messages in thread
From: Dan Carpenter @ 2014-06-11 8:12 UTC (permalink / raw)
To: walter harms
Cc: Jonathan Cameron, Grant Likely, Rob Herring, Josh Wu,
Alexandre Belloni, Nicolas Ferre, Maxime Ripard,
Lars-Peter Clausen, Thomas Meyer, Wei Yongjun, Sachin Kamat,
linux-iio, kernel-janitors
On Wed, Jun 11, 2014 at 09:36:49AM +0200, walter harms wrote:
>
>
> Am 11.06.2014 08:53, schrieb Dan Carpenter:
> > The function returns a u8 so the -ENOMEM is truncated to a positive
> > value. The caller tests for zero returns and treats them as an error
> > so I have changed the -ENOMEM to 0.
> >
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> >
> > diff --git a/drivers/iio/adc/at91_adc.c b/drivers/iio/adc/at91_adc.c
> > index 3b5bacd..5ebe33d 100644
> > --- a/drivers/iio/adc/at91_adc.c
> > +++ b/drivers/iio/adc/at91_adc.c
> > @@ -525,7 +525,7 @@ static u8 at91_adc_get_trigger_value_by_name(struct iio_dev *idev,
> > idev->id,
> > triggers[i].name);
> > if (!name)
> > - return -ENOMEM;
> > + return 0;
> >
> > if (strcmp(trigger_name, name) = 0) {
> > value = triggers[i].value;
>
>
> hi Dan,
> LXR says there is only 1 caller (and he is checking for ENOMON)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
No, it's not? It's checking for zero.
> perhaps it is better to change the return value to int.
In theory, it's always better to preserve the error code but in real
life, this memory allocation is going to succeed. I'm not able to
compile this code, and that change is quite a bit more involved, but
I'll send it anyway.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [patch] iio: adc: at91: signedness bug in at91_adc_get_trigger_value_by_name()
@ 2014-06-11 8:12 ` Dan Carpenter
0 siblings, 0 replies; 14+ messages in thread
From: Dan Carpenter @ 2014-06-11 8:12 UTC (permalink / raw)
To: walter harms
Cc: Jonathan Cameron, Grant Likely, Rob Herring, Josh Wu,
Alexandre Belloni, Nicolas Ferre, Maxime Ripard,
Lars-Peter Clausen, Thomas Meyer, Wei Yongjun, Sachin Kamat,
linux-iio, kernel-janitors
On Wed, Jun 11, 2014 at 09:36:49AM +0200, walter harms wrote:
>
>
> Am 11.06.2014 08:53, schrieb Dan Carpenter:
> > The function returns a u8 so the -ENOMEM is truncated to a positive
> > value. The caller tests for zero returns and treats them as an error
> > so I have changed the -ENOMEM to 0.
> >
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> >
> > diff --git a/drivers/iio/adc/at91_adc.c b/drivers/iio/adc/at91_adc.c
> > index 3b5bacd..5ebe33d 100644
> > --- a/drivers/iio/adc/at91_adc.c
> > +++ b/drivers/iio/adc/at91_adc.c
> > @@ -525,7 +525,7 @@ static u8 at91_adc_get_trigger_value_by_name(struct iio_dev *idev,
> > idev->id,
> > triggers[i].name);
> > if (!name)
> > - return -ENOMEM;
> > + return 0;
> >
> > if (strcmp(trigger_name, name) == 0) {
> > value = triggers[i].value;
>
>
> hi Dan,
> LXR says there is only 1 caller (and he is checking for ENOMON)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
No, it's not? It's checking for zero.
> perhaps it is better to change the return value to int.
In theory, it's always better to preserve the error code but in real
life, this memory allocation is going to succeed. I'm not able to
compile this code, and that change is quite a bit more involved, but
I'll send it anyway.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [patch] iio: adc: at91: signedness bug in at91_adc_get_trigger_value_by_name()
2014-06-11 8:12 ` Dan Carpenter
@ 2014-06-11 8:27 ` walter harms
-1 siblings, 0 replies; 14+ messages in thread
From: walter harms @ 2014-06-11 8:27 UTC (permalink / raw)
To: Dan Carpenter
Cc: Jonathan Cameron, Grant Likely, Rob Herring, Josh Wu,
Alexandre Belloni, Nicolas Ferre, Maxime Ripard,
Lars-Peter Clausen, Thomas Meyer, Wei Yongjun, Sachin Kamat,
linux-iio, kernel-janitors
Am 11.06.2014 10:12, schrieb Dan Carpenter:
> On Wed, Jun 11, 2014 at 09:36:49AM +0200, walter harms wrote:
>>
>>
>> Am 11.06.2014 08:53, schrieb Dan Carpenter:
>>> The function returns a u8 so the -ENOMEM is truncated to a positive
>>> value. The caller tests for zero returns and treats them as an error
>>> so I have changed the -ENOMEM to 0.
>>>
>>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>>>
>>> diff --git a/drivers/iio/adc/at91_adc.c b/drivers/iio/adc/at91_adc.c
>>> index 3b5bacd..5ebe33d 100644
>>> --- a/drivers/iio/adc/at91_adc.c
>>> +++ b/drivers/iio/adc/at91_adc.c
>>> @@ -525,7 +525,7 @@ static u8 at91_adc_get_trigger_value_by_name(struct iio_dev *idev,
>>> idev->id,
>>> triggers[i].name);
>>> if (!name)
>>> - return -ENOMEM;
>>> + return 0;
>>>
>>> if (strcmp(trigger_name, name) = 0) {
>>> value = triggers[i].value;
>>
>>
>> hi Dan,
>> LXR says there is only 1 caller (and he is checking for ENOMON)
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> No, it's not? It's checking for zero.
>
i as looking at at91_adc_configure_trigger() may it has changed lately
363
364 value = at91_adc_get_trigger_value_by_name(idev,
365 st->trigger_list,
366 idev->trig->name);
367 if (value = 0)
368 return -EINVAL;
>> perhaps it is better to change the return value to int.
>
> In theory, it's always better to preserve the error code but in real
> life, this memory allocation is going to succeed. I'm not able to
> compile this code, and that change is quite a bit more involved, but
> I'll send it anyway.
no problem with me, it was only a hint, maybe the maintainer will pick it up.
In practice I found it is better to stay with int than to thing about
"optimisation" and using char or friends, it does not save anything most times
instead it has unintended consequences later.
re,
wh
> regards,
> dan carpenter
>
>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [patch] iio: adc: at91: signedness bug in at91_adc_get_trigger_value_by_name()
@ 2014-06-11 8:27 ` walter harms
0 siblings, 0 replies; 14+ messages in thread
From: walter harms @ 2014-06-11 8:27 UTC (permalink / raw)
To: Dan Carpenter
Cc: Jonathan Cameron, Grant Likely, Rob Herring, Josh Wu,
Alexandre Belloni, Nicolas Ferre, Maxime Ripard,
Lars-Peter Clausen, Thomas Meyer, Wei Yongjun, Sachin Kamat,
linux-iio, kernel-janitors
Am 11.06.2014 10:12, schrieb Dan Carpenter:
> On Wed, Jun 11, 2014 at 09:36:49AM +0200, walter harms wrote:
>>
>>
>> Am 11.06.2014 08:53, schrieb Dan Carpenter:
>>> The function returns a u8 so the -ENOMEM is truncated to a positive
>>> value. The caller tests for zero returns and treats them as an error
>>> so I have changed the -ENOMEM to 0.
>>>
>>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>>>
>>> diff --git a/drivers/iio/adc/at91_adc.c b/drivers/iio/adc/at91_adc.c
>>> index 3b5bacd..5ebe33d 100644
>>> --- a/drivers/iio/adc/at91_adc.c
>>> +++ b/drivers/iio/adc/at91_adc.c
>>> @@ -525,7 +525,7 @@ static u8 at91_adc_get_trigger_value_by_name(struct iio_dev *idev,
>>> idev->id,
>>> triggers[i].name);
>>> if (!name)
>>> - return -ENOMEM;
>>> + return 0;
>>>
>>> if (strcmp(trigger_name, name) == 0) {
>>> value = triggers[i].value;
>>
>>
>> hi Dan,
>> LXR says there is only 1 caller (and he is checking for ENOMON)
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> No, it's not? It's checking for zero.
>
i as looking at at91_adc_configure_trigger() may it has changed lately
363
364 value = at91_adc_get_trigger_value_by_name(idev,
365 st->trigger_list,
366 idev->trig->name);
367 if (value == 0)
368 return -EINVAL;
>> perhaps it is better to change the return value to int.
>
> In theory, it's always better to preserve the error code but in real
> life, this memory allocation is going to succeed. I'm not able to
> compile this code, and that change is quite a bit more involved, but
> I'll send it anyway.
no problem with me, it was only a hint, maybe the maintainer will pick it up.
In practice I found it is better to stay with int than to thing about
"optimisation" and using char or friends, it does not save anything most times
instead it has unintended consequences later.
re,
wh
> regards,
> dan carpenter
>
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* [patch v2] iio: adc: at91: signedness bug in at91_adc_get_trigger_value_by_name()
2014-06-11 7:36 ` walter harms
@ 2014-06-11 8:13 ` Dan Carpenter
-1 siblings, 0 replies; 14+ messages in thread
From: Dan Carpenter @ 2014-06-11 8:13 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Grant Likely, Rob Herring, Josh Wu, Alexandre Belloni,
Nicolas Ferre, Maxime Ripard, Lars-Peter Clausen, Thomas Meyer,
Wei Yongjun, Sachin Kamat, linux-iio, kernel-janitors
at91_adc_get_trigger_value_by_name() was returning -ENOMEM truncated to
a positive u8 and that doesn't work. I've changed it to int and
refactored it to preserve the error code.
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
I'm not able to compile this code.
v2: Preserve the -ENOMEM
diff --git a/drivers/iio/adc/at91_adc.c b/drivers/iio/adc/at91_adc.c
index 3b5bacd..2b6a9ce 100644
--- a/drivers/iio/adc/at91_adc.c
+++ b/drivers/iio/adc/at91_adc.c
@@ -510,12 +510,11 @@ static int at91_adc_channel_init(struct iio_dev *idev)
return idev->num_channels;
}
-static u8 at91_adc_get_trigger_value_by_name(struct iio_dev *idev,
+static int at91_adc_get_trigger_value_by_name(struct iio_dev *idev,
struct at91_adc_trigger *triggers,
const char *trigger_name)
{
struct at91_adc_state *st = iio_priv(idev);
- u8 value = 0;
int i;
for (i = 0; i < st->trigger_number; i++) {
@@ -528,15 +527,16 @@ static u8 at91_adc_get_trigger_value_by_name(struct iio_dev *idev,
return -ENOMEM;
if (strcmp(trigger_name, name) = 0) {
- value = triggers[i].value;
kfree(name);
- break;
+ if (triggers[i].value = 0)
+ return -EINVAL;
+ return triggers[i].value;
}
kfree(name);
}
- return value;
+ return -EINVAL;
}
static int at91_adc_configure_trigger(struct iio_trigger *trig, bool state)
@@ -546,14 +546,14 @@ static int at91_adc_configure_trigger(struct iio_trigger *trig, bool state)
struct iio_buffer *buffer = idev->buffer;
struct at91_adc_reg_desc *reg = st->registers;
u32 status = at91_adc_readl(st, reg->trigger_register);
- u8 value;
+ int value;
u8 bit;
value = at91_adc_get_trigger_value_by_name(idev,
st->trigger_list,
idev->trig->name);
- if (value = 0)
- return -EINVAL;
+ if (value < 0)
+ return value;
if (state) {
st->buffer = kmalloc(idev->scan_bytes, GFP_KERNEL);
^ permalink raw reply related [flat|nested] 14+ messages in thread* [patch v2] iio: adc: at91: signedness bug in at91_adc_get_trigger_value_by_name()
@ 2014-06-11 8:13 ` Dan Carpenter
0 siblings, 0 replies; 14+ messages in thread
From: Dan Carpenter @ 2014-06-11 8:13 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Grant Likely, Rob Herring, Josh Wu, Alexandre Belloni,
Nicolas Ferre, Maxime Ripard, Lars-Peter Clausen, Thomas Meyer,
Wei Yongjun, Sachin Kamat, linux-iio, kernel-janitors
at91_adc_get_trigger_value_by_name() was returning -ENOMEM truncated to
a positive u8 and that doesn't work. I've changed it to int and
refactored it to preserve the error code.
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
I'm not able to compile this code.
v2: Preserve the -ENOMEM
diff --git a/drivers/iio/adc/at91_adc.c b/drivers/iio/adc/at91_adc.c
index 3b5bacd..2b6a9ce 100644
--- a/drivers/iio/adc/at91_adc.c
+++ b/drivers/iio/adc/at91_adc.c
@@ -510,12 +510,11 @@ static int at91_adc_channel_init(struct iio_dev *idev)
return idev->num_channels;
}
-static u8 at91_adc_get_trigger_value_by_name(struct iio_dev *idev,
+static int at91_adc_get_trigger_value_by_name(struct iio_dev *idev,
struct at91_adc_trigger *triggers,
const char *trigger_name)
{
struct at91_adc_state *st = iio_priv(idev);
- u8 value = 0;
int i;
for (i = 0; i < st->trigger_number; i++) {
@@ -528,15 +527,16 @@ static u8 at91_adc_get_trigger_value_by_name(struct iio_dev *idev,
return -ENOMEM;
if (strcmp(trigger_name, name) == 0) {
- value = triggers[i].value;
kfree(name);
- break;
+ if (triggers[i].value == 0)
+ return -EINVAL;
+ return triggers[i].value;
}
kfree(name);
}
- return value;
+ return -EINVAL;
}
static int at91_adc_configure_trigger(struct iio_trigger *trig, bool state)
@@ -546,14 +546,14 @@ static int at91_adc_configure_trigger(struct iio_trigger *trig, bool state)
struct iio_buffer *buffer = idev->buffer;
struct at91_adc_reg_desc *reg = st->registers;
u32 status = at91_adc_readl(st, reg->trigger_register);
- u8 value;
+ int value;
u8 bit;
value = at91_adc_get_trigger_value_by_name(idev,
st->trigger_list,
idev->trig->name);
- if (value == 0)
- return -EINVAL;
+ if (value < 0)
+ return value;
if (state) {
st->buffer = kmalloc(idev->scan_bytes, GFP_KERNEL);
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [patch v2] iio: adc: at91: signedness bug in at91_adc_get_trigger_value_by_name()
2014-06-11 8:13 ` Dan Carpenter
@ 2014-06-11 14:41 ` Alexandre Belloni
-1 siblings, 0 replies; 14+ messages in thread
From: Alexandre Belloni @ 2014-06-11 14:41 UTC (permalink / raw)
To: Dan Carpenter
Cc: Jonathan Cameron, Grant Likely, Rob Herring, Josh Wu,
Nicolas Ferre, Maxime Ripard, Lars-Peter Clausen, Thomas Meyer,
Wei Yongjun, Sachin Kamat, linux-iio, kernel-janitors
On 11/06/2014 at 11:13:17 +0300, Dan Carpenter wrote :
> at91_adc_get_trigger_value_by_name() was returning -ENOMEM truncated to
> a positive u8 and that doesn't work. I've changed it to int and
> refactored it to preserve the error code.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Acked-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
Also, tested on an at91sam9g45ek.
> ---
> I'm not able to compile this code.
>
> v2: Preserve the -ENOMEM
>
> diff --git a/drivers/iio/adc/at91_adc.c b/drivers/iio/adc/at91_adc.c
> index 3b5bacd..2b6a9ce 100644
> --- a/drivers/iio/adc/at91_adc.c
> +++ b/drivers/iio/adc/at91_adc.c
> @@ -510,12 +510,11 @@ static int at91_adc_channel_init(struct iio_dev *idev)
> return idev->num_channels;
> }
>
> -static u8 at91_adc_get_trigger_value_by_name(struct iio_dev *idev,
> +static int at91_adc_get_trigger_value_by_name(struct iio_dev *idev,
> struct at91_adc_trigger *triggers,
> const char *trigger_name)
> {
> struct at91_adc_state *st = iio_priv(idev);
> - u8 value = 0;
> int i;
>
> for (i = 0; i < st->trigger_number; i++) {
> @@ -528,15 +527,16 @@ static u8 at91_adc_get_trigger_value_by_name(struct iio_dev *idev,
> return -ENOMEM;
>
> if (strcmp(trigger_name, name) = 0) {
> - value = triggers[i].value;
> kfree(name);
> - break;
> + if (triggers[i].value = 0)
> + return -EINVAL;
> + return triggers[i].value;
> }
>
> kfree(name);
> }
>
> - return value;
> + return -EINVAL;
> }
>
> static int at91_adc_configure_trigger(struct iio_trigger *trig, bool state)
> @@ -546,14 +546,14 @@ static int at91_adc_configure_trigger(struct iio_trigger *trig, bool state)
> struct iio_buffer *buffer = idev->buffer;
> struct at91_adc_reg_desc *reg = st->registers;
> u32 status = at91_adc_readl(st, reg->trigger_register);
> - u8 value;
> + int value;
> u8 bit;
>
> value = at91_adc_get_trigger_value_by_name(idev,
> st->trigger_list,
> idev->trig->name);
> - if (value = 0)
> - return -EINVAL;
> + if (value < 0)
> + return value;
>
> if (state) {
> st->buffer = kmalloc(idev->scan_bytes, GFP_KERNEL);
--
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [patch v2] iio: adc: at91: signedness bug in at91_adc_get_trigger_value_by_name()
@ 2014-06-11 14:41 ` Alexandre Belloni
0 siblings, 0 replies; 14+ messages in thread
From: Alexandre Belloni @ 2014-06-11 14:41 UTC (permalink / raw)
To: Dan Carpenter
Cc: Jonathan Cameron, Grant Likely, Rob Herring, Josh Wu,
Nicolas Ferre, Maxime Ripard, Lars-Peter Clausen, Thomas Meyer,
Wei Yongjun, Sachin Kamat, linux-iio, kernel-janitors
On 11/06/2014 at 11:13:17 +0300, Dan Carpenter wrote :
> at91_adc_get_trigger_value_by_name() was returning -ENOMEM truncated to
> a positive u8 and that doesn't work. I've changed it to int and
> refactored it to preserve the error code.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Acked-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
Also, tested on an at91sam9g45ek.
> ---
> I'm not able to compile this code.
>
> v2: Preserve the -ENOMEM
>
> diff --git a/drivers/iio/adc/at91_adc.c b/drivers/iio/adc/at91_adc.c
> index 3b5bacd..2b6a9ce 100644
> --- a/drivers/iio/adc/at91_adc.c
> +++ b/drivers/iio/adc/at91_adc.c
> @@ -510,12 +510,11 @@ static int at91_adc_channel_init(struct iio_dev *idev)
> return idev->num_channels;
> }
>
> -static u8 at91_adc_get_trigger_value_by_name(struct iio_dev *idev,
> +static int at91_adc_get_trigger_value_by_name(struct iio_dev *idev,
> struct at91_adc_trigger *triggers,
> const char *trigger_name)
> {
> struct at91_adc_state *st = iio_priv(idev);
> - u8 value = 0;
> int i;
>
> for (i = 0; i < st->trigger_number; i++) {
> @@ -528,15 +527,16 @@ static u8 at91_adc_get_trigger_value_by_name(struct iio_dev *idev,
> return -ENOMEM;
>
> if (strcmp(trigger_name, name) == 0) {
> - value = triggers[i].value;
> kfree(name);
> - break;
> + if (triggers[i].value == 0)
> + return -EINVAL;
> + return triggers[i].value;
> }
>
> kfree(name);
> }
>
> - return value;
> + return -EINVAL;
> }
>
> static int at91_adc_configure_trigger(struct iio_trigger *trig, bool state)
> @@ -546,14 +546,14 @@ static int at91_adc_configure_trigger(struct iio_trigger *trig, bool state)
> struct iio_buffer *buffer = idev->buffer;
> struct at91_adc_reg_desc *reg = st->registers;
> u32 status = at91_adc_readl(st, reg->trigger_register);
> - u8 value;
> + int value;
> u8 bit;
>
> value = at91_adc_get_trigger_value_by_name(idev,
> st->trigger_list,
> idev->trig->name);
> - if (value == 0)
> - return -EINVAL;
> + if (value < 0)
> + return value;
>
> if (state) {
> st->buffer = kmalloc(idev->scan_bytes, GFP_KERNEL);
--
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [patch v2] iio: adc: at91: signedness bug in at91_adc_get_trigger_value_by_name()
2014-06-11 14:41 ` Alexandre Belloni
@ 2014-06-14 14:38 ` Jonathan Cameron
-1 siblings, 0 replies; 14+ messages in thread
From: Jonathan Cameron @ 2014-06-14 14:38 UTC (permalink / raw)
To: Alexandre Belloni, Dan Carpenter
Cc: Grant Likely, Rob Herring, Josh Wu, Nicolas Ferre, Maxime Ripard,
Lars-Peter Clausen, Thomas Meyer, Wei Yongjun, Sachin Kamat,
linux-iio, kernel-janitors
On 11/06/14 15:41, Alexandre Belloni wrote:
> On 11/06/2014 at 11:13:17 +0300, Dan Carpenter wrote :
>> at91_adc_get_trigger_value_by_name() was returning -ENOMEM truncated to
>> a positive u8 and that doesn't work. I've changed it to int and
>> refactored it to preserve the error code.
>>
>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> Acked-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
>
> Also, tested on an at91sam9g45ek.
I've added a tested-by as in many ways that has more weight than an
Ack (and can sit alongside it just fine)
Dan, thanks as ever!
Applied to the fixes-togreg branch of iio.git and cc'd to stable.
Jonathan
>
>> ---
>> I'm not able to compile this code.
Me neither - which makes that tested by rather handy ;)
>>
>> v2: Preserve the -ENOMEM
>>
>> diff --git a/drivers/iio/adc/at91_adc.c b/drivers/iio/adc/at91_adc.c
>> index 3b5bacd..2b6a9ce 100644
>> --- a/drivers/iio/adc/at91_adc.c
>> +++ b/drivers/iio/adc/at91_adc.c
>> @@ -510,12 +510,11 @@ static int at91_adc_channel_init(struct iio_dev *idev)
>> return idev->num_channels;
>> }
>>
>> -static u8 at91_adc_get_trigger_value_by_name(struct iio_dev *idev,
>> +static int at91_adc_get_trigger_value_by_name(struct iio_dev *idev,
>> struct at91_adc_trigger *triggers,
>> const char *trigger_name)
>> {
>> struct at91_adc_state *st = iio_priv(idev);
>> - u8 value = 0;
>> int i;
>>
>> for (i = 0; i < st->trigger_number; i++) {
>> @@ -528,15 +527,16 @@ static u8 at91_adc_get_trigger_value_by_name(struct iio_dev *idev,
>> return -ENOMEM;
>>
>> if (strcmp(trigger_name, name) = 0) {
>> - value = triggers[i].value;
>> kfree(name);
>> - break;
>> + if (triggers[i].value = 0)
>> + return -EINVAL;
>> + return triggers[i].value;
>> }
>>
>> kfree(name);
>> }
>>
>> - return value;
>> + return -EINVAL;
>> }
>>
>> static int at91_adc_configure_trigger(struct iio_trigger *trig, bool state)
>> @@ -546,14 +546,14 @@ static int at91_adc_configure_trigger(struct iio_trigger *trig, bool state)
>> struct iio_buffer *buffer = idev->buffer;
>> struct at91_adc_reg_desc *reg = st->registers;
>> u32 status = at91_adc_readl(st, reg->trigger_register);
>> - u8 value;
>> + int value;
>> u8 bit;
>>
>> value = at91_adc_get_trigger_value_by_name(idev,
>> st->trigger_list,
>> idev->trig->name);
>> - if (value = 0)
>> - return -EINVAL;
>> + if (value < 0)
>> + return value;
>>
>> if (state) {
>> st->buffer = kmalloc(idev->scan_bytes, GFP_KERNEL);
>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [patch v2] iio: adc: at91: signedness bug in at91_adc_get_trigger_value_by_name()
@ 2014-06-14 14:38 ` Jonathan Cameron
0 siblings, 0 replies; 14+ messages in thread
From: Jonathan Cameron @ 2014-06-14 14:38 UTC (permalink / raw)
To: Alexandre Belloni, Dan Carpenter
Cc: Grant Likely, Rob Herring, Josh Wu, Nicolas Ferre, Maxime Ripard,
Lars-Peter Clausen, Thomas Meyer, Wei Yongjun, Sachin Kamat,
linux-iio, kernel-janitors
On 11/06/14 15:41, Alexandre Belloni wrote:
> On 11/06/2014 at 11:13:17 +0300, Dan Carpenter wrote :
>> at91_adc_get_trigger_value_by_name() was returning -ENOMEM truncated to
>> a positive u8 and that doesn't work. I've changed it to int and
>> refactored it to preserve the error code.
>>
>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> Acked-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
>
> Also, tested on an at91sam9g45ek.
I've added a tested-by as in many ways that has more weight than an
Ack (and can sit alongside it just fine)
Dan, thanks as ever!
Applied to the fixes-togreg branch of iio.git and cc'd to stable.
Jonathan
>
>> ---
>> I'm not able to compile this code.
Me neither - which makes that tested by rather handy ;)
>>
>> v2: Preserve the -ENOMEM
>>
>> diff --git a/drivers/iio/adc/at91_adc.c b/drivers/iio/adc/at91_adc.c
>> index 3b5bacd..2b6a9ce 100644
>> --- a/drivers/iio/adc/at91_adc.c
>> +++ b/drivers/iio/adc/at91_adc.c
>> @@ -510,12 +510,11 @@ static int at91_adc_channel_init(struct iio_dev *idev)
>> return idev->num_channels;
>> }
>>
>> -static u8 at91_adc_get_trigger_value_by_name(struct iio_dev *idev,
>> +static int at91_adc_get_trigger_value_by_name(struct iio_dev *idev,
>> struct at91_adc_trigger *triggers,
>> const char *trigger_name)
>> {
>> struct at91_adc_state *st = iio_priv(idev);
>> - u8 value = 0;
>> int i;
>>
>> for (i = 0; i < st->trigger_number; i++) {
>> @@ -528,15 +527,16 @@ static u8 at91_adc_get_trigger_value_by_name(struct iio_dev *idev,
>> return -ENOMEM;
>>
>> if (strcmp(trigger_name, name) == 0) {
>> - value = triggers[i].value;
>> kfree(name);
>> - break;
>> + if (triggers[i].value == 0)
>> + return -EINVAL;
>> + return triggers[i].value;
>> }
>>
>> kfree(name);
>> }
>>
>> - return value;
>> + return -EINVAL;
>> }
>>
>> static int at91_adc_configure_trigger(struct iio_trigger *trig, bool state)
>> @@ -546,14 +546,14 @@ static int at91_adc_configure_trigger(struct iio_trigger *trig, bool state)
>> struct iio_buffer *buffer = idev->buffer;
>> struct at91_adc_reg_desc *reg = st->registers;
>> u32 status = at91_adc_readl(st, reg->trigger_register);
>> - u8 value;
>> + int value;
>> u8 bit;
>>
>> value = at91_adc_get_trigger_value_by_name(idev,
>> st->trigger_list,
>> idev->trig->name);
>> - if (value == 0)
>> - return -EINVAL;
>> + if (value < 0)
>> + return value;
>>
>> if (state) {
>> st->buffer = kmalloc(idev->scan_bytes, GFP_KERNEL);
>
^ permalink raw reply [flat|nested] 14+ messages in thread