* [patch] iio: adc: at91: signedness bug in at91_adc_get_trigger_value_by_name() @ 2014-06-11 6:53 Dan Carpenter 2014-06-11 7:36 ` walter harms 0 siblings, 1 reply; 7+ messages in thread From: Dan Carpenter @ 2014-06-11 6:53 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 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; ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [patch] iio: adc: at91: signedness bug in at91_adc_get_trigger_value_by_name() 2014-06-11 6:53 [patch] iio: adc: at91: signedness bug in at91_adc_get_trigger_value_by_name() Dan Carpenter @ 2014-06-11 7:36 ` walter harms 2014-06-11 8:12 ` Dan Carpenter 2014-06-11 8:13 ` [patch v2] " Dan Carpenter 0 siblings, 2 replies; 7+ 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] 7+ 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 2014-06-11 8:27 ` walter harms 2014-06-11 8:13 ` [patch v2] " Dan Carpenter 1 sibling, 1 reply; 7+ 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] 7+ 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 0 siblings, 0 replies; 7+ 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] 7+ 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:12 ` Dan Carpenter @ 2014-06-11 8:13 ` Dan Carpenter 2014-06-11 14:41 ` Alexandre Belloni 1 sibling, 1 reply; 7+ 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] 7+ messages in thread
* Re: [patch v2] iio: adc: at91: signedness bug in at91_adc_get_trigger_value_by_name() 2014-06-11 8:13 ` [patch v2] " Dan Carpenter @ 2014-06-11 14:41 ` Alexandre Belloni 2014-06-14 14:38 ` Jonathan Cameron 0 siblings, 1 reply; 7+ 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] 7+ 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 0 siblings, 0 replies; 7+ 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] 7+ messages in thread
end of thread, other threads:[~2014-06-14 14:36 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-06-11 6:53 [patch] iio: adc: at91: signedness bug in at91_adc_get_trigger_value_by_name() Dan Carpenter 2014-06-11 7:36 ` walter harms 2014-06-11 8:12 ` Dan Carpenter 2014-06-11 8:27 ` walter harms 2014-06-11 8:13 ` [patch v2] " Dan Carpenter 2014-06-11 14:41 ` Alexandre Belloni 2014-06-14 14:38 ` 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).