[PATCH] iio: mxs-lradc: Remove useless check in read_raw

[PATCH] iio: mxs-lradc: Remove useless check in read_raw

[Marek Vasut]

The removed check in the read_raw implementation was always true,
therefore remove it.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Jonathan Cameron <jic23@kernel.org>
Cc: Fabio Estevam <fabio.estevam@freescale.com>
---
 drivers/staging/iio/adc/mxs-lradc.c |    7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/staging/iio/adc/mxs-lradc.c b/drivers/staging/iio/adc/mxs-lradc.c
index d92c97a..c318eb6 100644
--- a/drivers/staging/iio/adc/mxs-lradc.c
+++ b/drivers/staging/iio/adc/mxs-lradc.c
@@ -234,7 +234,6 @@ static int mxs_lradc_read_raw(struct iio_dev *iio_dev,
 {
 	struct mxs_lradc *lradc = iio_priv(iio_dev);
 	int ret;
-	unsigned long mask;
 
 	if (m != IIO_CHAN_INFO_RAW)
 		return -EINVAL;
@@ -243,12 +242,6 @@ static int mxs_lradc_read_raw(struct iio_dev *iio_dev,
 	if (chan->channel > LRADC_MAX_TOTAL_CHANS)
 		return -EINVAL;
 
-	/* Validate the channel if it doesn't intersect with reserved chans. */
-	bitmap_set(&mask, chan->channel, 1);
-	ret = iio_validate_scan_mask_onehot(iio_dev, &mask);
-	if (ret)
-		return -EINVAL;
-
 	/*
 	 * See if there is no buffered operation in progess. If there is, simply
 	 * bail out. This can be improved to support both buffered and raw IO at
-- 
1.7.10.4

Re: [PATCH] iio: mxs-lradc: Remove useless check in read_raw

[Otavio Salvador]

On Mon, Jul 1, 2013 at 9:08 PM, Marek Vasut <marex@denx.de> wrote:
> The removed check in the read_raw implementation was always true,
> therefore remove it.
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Jonathan Cameron <jic23@kernel.org>
> Cc: Fabio Estevam <fabio.estevam@freescale.com>

This fixes the driver for MX23 but why this code were add at first?

--
Otavio Salvador                             O.S. Systems
http://www.ossystems.com.br        http://projetos.ossystems.com.br
Mobile: +55 (53) 9981-7854            Mobile: +1 (347) 903-9750

Re: [PATCH] iio: mxs-lradc: Remove useless check in read_raw

[Marek Vasut]

Dear Otavio Salvador,

> On Mon, Jul 1, 2013 at 9:08 PM, Marek Vasut <marex@denx.de> wrote:
> > The removed check in the read_raw implementation was always true,
> > therefore remove it.
> > 
> > Signed-off-by: Marek Vasut <marex@denx.de>
> > Cc: Jonathan Cameron <jic23@kernel.org>
> > Cc: Fabio Estevam <fabio.estevam@freescale.com>
> 
> This fixes the driver for MX23 but why this code were add at first?

Obviously because I wrote the code incorrectly. I appologize for the trouble.

Jonathan, can we get this one into 3.10.1 maybe ?

Best regards,
Marek Vasut

Re: [PATCH] iio: mxs-lradc: Remove useless check in read_raw

[Hector Palacios]

Dear Marek,

On 07/02/2013 02:08 AM, Marek Vasut wrote:
> The removed check in the read_raw implementation was always true,
> therefore remove it.
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Jonathan Cameron <jic23@kernel.org>
> Cc: Fabio Estevam <fabio.estevam@freescale.com>
> ---
>   drivers/staging/iio/adc/mxs-lradc.c |    7 -------
>   1 file changed, 7 deletions(-)
>
> diff --git a/drivers/staging/iio/adc/mxs-lradc.c b/drivers/staging/iio/adc/mxs-lradc.c
> index d92c97a..c318eb6 100644
> --- a/drivers/staging/iio/adc/mxs-lradc.c
> +++ b/drivers/staging/iio/adc/mxs-lradc.c
> @@ -234,7 +234,6 @@ static int mxs_lradc_read_raw(struct iio_dev *iio_dev,
>   {
>   	struct mxs_lradc *lradc = iio_priv(iio_dev);
>   	int ret;
> -	unsigned long mask;
>
>   	if (m != IIO_CHAN_INFO_RAW)
>   		return -EINVAL;
> @@ -243,12 +242,6 @@ static int mxs_lradc_read_raw(struct iio_dev *iio_dev,
>   	if (chan->channel > LRADC_MAX_TOTAL_CHANS)
>   		return -EINVAL;
>
> -	/* Validate the channel if it doesn't intersect with reserved chans. */
> -	bitmap_set(&mask, chan->channel, 1);
> -	ret = iio_validate_scan_mask_onehot(iio_dev, &mask);
> -	if (ret)
> -		return -EINVAL;
> -
>   	/*
>   	 * See if there is no buffered operation in progess. If there is, simply
>   	 * bail out. This can be improved to support both buffered and raw IO at
>

By "always true" you mean that it always entered the if and exited with -EINVAL, right?
I just started to look at this driver on my MX28 platform and this check was always 
returning -EINVAL. With this patch I can now read the channels.
Was this what was happening to you as well?

Thank you,
-- 
Héctor Palacios

Re: [PATCH] iio: mxs-lradc: Remove useless check in read_raw

[Alexandre Belloni]

Dear Marek,

I don't seem to be hitting that issue. I'm using 3.10rc7. Do you know
how to reproduce it ?

Regards,

On 02/07/2013 02:08, Marek Vasut wrote:
> The removed check in the read_raw implementation was always true,
> therefore remove it.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Jonathan Cameron <jic23@kernel.org>
> Cc: Fabio Estevam <fabio.estevam@freescale.com>
> ---
>  drivers/staging/iio/adc/mxs-lradc.c |    7 -------
>  1 file changed, 7 deletions(-)
> 
> diff --git a/drivers/staging/iio/adc/mxs-lradc.c b/drivers/staging/iio/adc/mxs-lradc.c
> index d92c97a..c318eb6 100644
> --- a/drivers/staging/iio/adc/mxs-lradc.c
> +++ b/drivers/staging/iio/adc/mxs-lradc.c
> @@ -234,7 +234,6 @@ static int mxs_lradc_read_raw(struct iio_dev *iio_dev,
>  {
>  	struct mxs_lradc *lradc = iio_priv(iio_dev);
>  	int ret;
> -	unsigned long mask;
>  
>  	if (m != IIO_CHAN_INFO_RAW)
>  		return -EINVAL;
> @@ -243,12 +242,6 @@ static int mxs_lradc_read_raw(struct iio_dev *iio_dev,
>  	if (chan->channel > LRADC_MAX_TOTAL_CHANS)
>  		return -EINVAL;
>  
> -	/* Validate the channel if it doesn't intersect with reserved chans. */
> -	bitmap_set(&mask, chan->channel, 1);
> -	ret = iio_validate_scan_mask_onehot(iio_dev, &mask);
> -	if (ret)
> -		return -EINVAL;
> -
>  	/*
>  	 * See if there is no buffered operation in progess. If there is, simply
>  	 * bail out. This can be improved to support both buffered and raw IO at
> 


-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH] iio: mxs-lradc: Remove useless check in read_raw

[Otavio Salvador]

On Tue, Jul 2, 2013 at 8:43 AM, Alexandre Belloni
<alexandre.belloni@free-electrons.com> wrote:
> I don't seem to be hitting that issue. I'm using 3.10rc7. Do you know
> how to reproduce it ?

I had this, but in MX23.

It seems like a race as if I put some prints in this code area it
works sometimes.

--
Otavio Salvador                             O.S. Systems
http://www.ossystems.com.br        http://projetos.ossystems.com.br
Mobile: +55 (53) 9981-7854            Mobile: +1 (347) 903-9750

Re: [PATCH] iio: mxs-lradc: Remove useless check in read_raw

[Marek Vasut]

Dear Alexandre Belloni,

> Dear Marek,
> 
> I don't seem to be hitting that issue. I'm using 3.10rc7. Do you know
> how to reproduce it ?

The check is just redundant, it's not a bug.

> Regards,
> 
> On 02/07/2013 02:08, Marek Vasut wrote:
> > The removed check in the read_raw implementation was always true,
> > therefore remove it.
> > 
> > Signed-off-by: Marek Vasut <marex@denx.de>
> > Cc: Jonathan Cameron <jic23@kernel.org>
> > Cc: Fabio Estevam <fabio.estevam@freescale.com>
> > ---
> > 
> >  drivers/staging/iio/adc/mxs-lradc.c |    7 -------
> >  1 file changed, 7 deletions(-)
> > 
> > diff --git a/drivers/staging/iio/adc/mxs-lradc.c
> > b/drivers/staging/iio/adc/mxs-lradc.c index d92c97a..c318eb6 100644
> > --- a/drivers/staging/iio/adc/mxs-lradc.c
> > +++ b/drivers/staging/iio/adc/mxs-lradc.c
> > @@ -234,7 +234,6 @@ static int mxs_lradc_read_raw(struct iio_dev
> > *iio_dev,
> > 
> >  {
> >  
> >  	struct mxs_lradc *lradc = iio_priv(iio_dev);
> >  	int ret;
> > 
> > -	unsigned long mask;
> > 
> >  	if (m != IIO_CHAN_INFO_RAW)
> >  	
> >  		return -EINVAL;
> > 
> > @@ -243,12 +242,6 @@ static int mxs_lradc_read_raw(struct iio_dev
> > *iio_dev,
> > 
> >  	if (chan->channel > LRADC_MAX_TOTAL_CHANS)
> >  	
> >  		return -EINVAL;
> > 
> > -	/* Validate the channel if it doesn't intersect with reserved chans. */
> > -	bitmap_set(&mask, chan->channel, 1);
> > -	ret = iio_validate_scan_mask_onehot(iio_dev, &mask);
> > -	if (ret)
> > -		return -EINVAL;
> > -
> > 
> >  	/*
> >  	
> >  	 * See if there is no buffered operation in progess. If there is,
> >  	 simply * bail out. This can be improved to support both buffered and
> >  	 raw IO at

Best regards,
Marek Vasut

Re: [PATCH] iio: mxs-lradc: Remove useless check in read_raw

[Alexandre Belloni]

On 02/07/2013 14:03, Marek Vasut wrote:
> Dear Alexandre Belloni,
> 
>> Dear Marek,
>>
>> I don't seem to be hitting that issue. I'm using 3.10rc7. Do you know
>> how to reproduce it ?
> 
> The check is just redundant, it's not a bug.
> 

Ok, that's what I understood first but then got confused by reports of
it solving a bug.

Thanks !


-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH] iio: mxs-lradc: Remove useless check in read_raw

[Marek Vasut]

Dear Alexandre Belloni,

> On 02/07/2013 14:03, Marek Vasut wrote:
> > Dear Alexandre Belloni,
> > 
> >> Dear Marek,
> >> 
> >> I don't seem to be hitting that issue. I'm using 3.10rc7. Do you know
> >> how to reproduce it ?
> > 
> > The check is just redundant, it's not a bug.
> 
> Ok, that's what I understood first but then got confused by reports of
> it solving a bug.

It cannot solve a thing. If it does, then we have a problem.

What kind of bug do you see ? How can I replicate it ? Can you send me a 
testcase?

Best regards,
Marek Vasut

Re: [PATCH] iio: mxs-lradc: Remove useless check in read_raw

[Otavio Salvador]

On Tue, Jul 2, 2013 at 1:13 PM, Marek Vasut <marex@denx.de> wrote:
> Dear Alexandre Belloni,
>
>> On 02/07/2013 14:03, Marek Vasut wrote:
>> > Dear Alexandre Belloni,
>> >
>> >> Dear Marek,
>> >>
>> >> I don't seem to be hitting that issue. I'm using 3.10rc7. Do you know
>> >> how to reproduce it ?
>> >
>> > The check is just redundant, it's not a bug.
>>
>> Ok, that's what I understood first but then got confused by reports of
>> it solving a bug.
>
> It cannot solve a thing. If it does, then we have a problem.
>
> What kind of bug do you see ? How can I replicate it ? Can you send me a
> testcase?

As I said this code sometimes work. If you put a printf before this
call it sometimes work. So I think we have a race somewhere.

When I were debugging this I found that when it works we have 10
active channels, it seems.

--
Otavio Salvador                             O.S. Systems
http://www.ossystems.com.br        http://projetos.ossystems.com.br
Mobile: +55 (53) 9981-7854            Mobile: +1 (347) 903-9750

Re: [PATCH] iio: mxs-lradc: Remove useless check in read_raw

[Marek Vasut]

Dear Otavio Salvador,

> On Tue, Jul 2, 2013 at 1:13 PM, Marek Vasut <marex@denx.de> wrote:
> > Dear Alexandre Belloni,
> > 
> >> On 02/07/2013 14:03, Marek Vasut wrote:
> >> > Dear Alexandre Belloni,
> >> > 
> >> >> Dear Marek,
> >> >> 
> >> >> I don't seem to be hitting that issue. I'm using 3.10rc7. Do you know
> >> >> how to reproduce it ?
> >> > 
> >> > The check is just redundant, it's not a bug.
> >> 
> >> Ok, that's what I understood first but then got confused by reports of
> >> it solving a bug.
> > 
> > It cannot solve a thing. If it does, then we have a problem.
> > 
> > What kind of bug do you see ? How can I replicate it ? Can you send me a
> > testcase?
> 
> As I said this code sometimes work. If you put a printf before this
> call it sometimes work. So I think we have a race somewhere.
> 
> When I were debugging this I found that when it works we have 10
> active channels, it seems.

Uh, the read_raw() should exit with -EBUSY, since the mutex_tryload() will fail 
iff buffered operation is in progress. Or what do you mean by having "10 active 
channels"?

Best regards,
Marek Vasut

Re: [PATCH] iio: mxs-lradc: Remove useless check in read_raw

[Otavio Salvador]

On Tue, Jul 2, 2013 at 2:36 PM, Marek Vasut <marex@denx.de> wrote:
> Dear Otavio Salvador,
>
>> On Tue, Jul 2, 2013 at 1:13 PM, Marek Vasut <marex@denx.de> wrote:
>> > Dear Alexandre Belloni,
>> >
>> >> On 02/07/2013 14:03, Marek Vasut wrote:
>> >> > Dear Alexandre Belloni,
>> >> >
>> >> >> Dear Marek,
>> >> >>
>> >> >> I don't seem to be hitting that issue. I'm using 3.10rc7. Do you know
>> >> >> how to reproduce it ?
>> >> >
>> >> > The check is just redundant, it's not a bug.
>> >>
>> >> Ok, that's what I understood first but then got confused by reports of
>> >> it solving a bug.
>> >
>> > It cannot solve a thing. If it does, then we have a problem.
>> >
>> > What kind of bug do you see ? How can I replicate it ? Can you send me a
>> > testcase?
>>
>> As I said this code sometimes work. If you put a printf before this
>> call it sometimes work. So I think we have a race somewhere.
>>
>> When I were debugging this I found that when it works we have 10
>> active channels, it seems.
>
> Uh, the read_raw() should exit with -EBUSY, since the mutex_tryload() will fail
> iff buffered operation is in progress. Or what do you mean by having "10 active
> channels"?

The read_raw returned invalid. The onehot always returns false and I
have 10 as mask comparing return.

--
Otavio Salvador                             O.S. Systems
http://www.ossystems.com.br        http://projetos.ossystems.com.br
Mobile: +55 (53) 9981-7854            Mobile: +1 (347) 903-9750

Re: [PATCH] iio: mxs-lradc: Remove useless check in read_raw

[Marek Vasut]

Dear Otavio Salvador,

> On Tue, Jul 2, 2013 at 2:36 PM, Marek Vasut <marex@denx.de> wrote:
> > Dear Otavio Salvador,
> > 
> >> On Tue, Jul 2, 2013 at 1:13 PM, Marek Vasut <marex@denx.de> wrote:
> >> > Dear Alexandre Belloni,
> >> > 
> >> >> On 02/07/2013 14:03, Marek Vasut wrote:
> >> >> > Dear Alexandre Belloni,
> >> >> > 
> >> >> >> Dear Marek,
> >> >> >> 
> >> >> >> I don't seem to be hitting that issue. I'm using 3.10rc7. Do you
> >> >> >> know how to reproduce it ?
> >> >> > 
> >> >> > The check is just redundant, it's not a bug.
> >> >> 
> >> >> Ok, that's what I understood first but then got confused by reports
> >> >> of it solving a bug.
> >> > 
> >> > It cannot solve a thing. If it does, then we have a problem.
> >> > 
> >> > What kind of bug do you see ? How can I replicate it ? Can you send me
> >> > a testcase?
> >> 
> >> As I said this code sometimes work. If you put a printf before this
> >> call it sometimes work. So I think we have a race somewhere.
> >> 
> >> When I were debugging this I found that when it works we have 10
> >> active channels, it seems.
> > 
> > Uh, the read_raw() should exit with -EBUSY, since the mutex_tryload()
> > will fail iff buffered operation is in progress. Or what do you mean by
> > having "10 active channels"?
> 
> The read_raw returned invalid. The onehot always returns false and I
> have 10 as mask comparing return.

Looking at the code.

the bitmap_set() will set (1 << chan->channel) into "mask" var.

The iio_validate_scan_mask_onehot is implemented like this:

688 bool iio_validate_scan_mask_onehot(struct iio_dev *indio_dev,
689         const unsigned long *mask)
690 {
691         return bitmap_weight(mask, indio_dev->masklength) == 1;
692 }

So if mask _always_ has exactly one bit set, the iio_validate_scan_mask_onehot 
will always return 1. Aaaand ... now I see why it fixes a bug for you.

if (ret)
 return -EINVAL;

This stuff above will always be if (1) so will always return -EINVAL. Dang!

Best regards,
Marek Vasut

Re: [PATCH] iio: mxs-lradc: Remove useless check in read_raw

[Hector Palacios]

Dear Marek,

On 07/02/2013 07:55 PM, Marek Vasut wrote:
> Dear Otavio Salvador,
>
>> On Tue, Jul 2, 2013 at 2:36 PM, Marek Vasut <marex-ynQEQJNshbs@public.gmane.org> wrote:
>>> Dear Otavio Salvador,
>>>
>>>> On Tue, Jul 2, 2013 at 1:13 PM, Marek Vasut <marex-ynQEQJNshbs@public.gmane.org> wrote:
>>>>> Dear Alexandre Belloni,
>>>>>
>>>>>> On 02/07/2013 14:03, Marek Vasut wrote:
>>>>>>> Dear Alexandre Belloni,
>>>>>>>
>>>>>>>> Dear Marek,
>>>>>>>>
>>>>>>>> I don't seem to be hitting that issue. I'm using 3.10rc7. Do you
>>>>>>>> know how to reproduce it ?
>>>>>>>
>>>>>>> The check is just redundant, it's not a bug.
>>>>>>
>>>>>> Ok, that's what I understood first but then got confused by reports
>>>>>> of it solving a bug.
>>>>>
>>>>> It cannot solve a thing. If it does, then we have a problem.
>>>>>
>>>>> What kind of bug do you see ? How can I replicate it ? Can you send me
>>>>> a testcase?
>>>>
>>>> As I said this code sometimes work. If you put a printf before this
>>>> call it sometimes work. So I think we have a race somewhere.
>>>>
>>>> When I were debugging this I found that when it works we have 10
>>>> active channels, it seems.
>>>
>>> Uh, the read_raw() should exit with -EBUSY, since the mutex_tryload()
>>> will fail iff buffered operation is in progress. Or what do you mean by
>>> having "10 active channels"?
>>
>> The read_raw returned invalid. The onehot always returns false and I
>> have 10 as mask comparing return.
>
> Looking at the code.
>
> the bitmap_set() will set (1 << chan->channel) into "mask" var.
>
> The iio_validate_scan_mask_onehot is implemented like this:
>
> 688 bool iio_validate_scan_mask_onehot(struct iio_dev *indio_dev,
> 689         const unsigned long *mask)
> 690 {
> 691         return bitmap_weight(mask, indio_dev->masklength) == 1;
> 692 }
>
> So if mask _always_ has exactly one bit set, the iio_validate_scan_mask_onehot
> will always return 1. Aaaand ... now I see why it fixes a bug for you.
>
> if (ret)
>   return -EINVAL;
>
> This stuff above will always be if (1) so will always return -EINVAL. Dang!

Yes, this was exactly what was happening on my MX28 when trying to read one channel.
Now, after the different comments I'm confused, was this a bug that your patch solved 
or was I doing something wrong?

Best regards,
--
Hector Palacios

Re: [PATCH] iio: mxs-lradc: Remove useless check in read_raw

[Marek Vasut]

Dear Hector Palacios,

> Dear Marek,
> 
> On 07/02/2013 07:55 PM, Marek Vasut wrote:
> > Dear Otavio Salvador,
> > 
> >> On Tue, Jul 2, 2013 at 2:36 PM, Marek Vasut <marex-
ynQEQJNshbs@public.gmane.org> wrote:
> >>> Dear Otavio Salvador,
> >>> 
> >>>> On Tue, Jul 2, 2013 at 1:13 PM, Marek Vasut <marex-
ynQEQJNshbs@public.gmane.org> wrote:
> >>>>> Dear Alexandre Belloni,
> >>>>> 
> >>>>>> On 02/07/2013 14:03, Marek Vasut wrote:
> >>>>>>> Dear Alexandre Belloni,
> >>>>>>> 
> >>>>>>>> Dear Marek,
> >>>>>>>> 
> >>>>>>>> I don't seem to be hitting that issue. I'm using 3.10rc7. Do you
> >>>>>>>> know how to reproduce it ?
> >>>>>>> 
> >>>>>>> The check is just redundant, it's not a bug.
> >>>>>> 
> >>>>>> Ok, that's what I understood first but then got confused by reports
> >>>>>> of it solving a bug.
> >>>>> 
> >>>>> It cannot solve a thing. If it does, then we have a problem.
> >>>>> 
> >>>>> What kind of bug do you see ? How can I replicate it ? Can you send
> >>>>> me a testcase?
> >>>> 
> >>>> As I said this code sometimes work. If you put a printf before this
> >>>> call it sometimes work. So I think we have a race somewhere.
> >>>> 
> >>>> When I were debugging this I found that when it works we have 10
> >>>> active channels, it seems.
> >>> 
> >>> Uh, the read_raw() should exit with -EBUSY, since the mutex_tryload()
> >>> will fail iff buffered operation is in progress. Or what do you mean by
> >>> having "10 active channels"?
> >> 
> >> The read_raw returned invalid. The onehot always returns false and I
> >> have 10 as mask comparing return.
> > 
> > Looking at the code.
> > 
> > the bitmap_set() will set (1 << chan->channel) into "mask" var.
> > 
> > The iio_validate_scan_mask_onehot is implemented like this:
> > 
> > 688 bool iio_validate_scan_mask_onehot(struct iio_dev *indio_dev,
> > 689         const unsigned long *mask)
> > 690 {
> > 691         return bitmap_weight(mask, indio_dev->masklength) == 1;
> > 692 }
> > 
> > So if mask _always_ has exactly one bit set, the
> > iio_validate_scan_mask_onehot will always return 1. Aaaand ... now I see
> > why it fixes a bug for you.
> > 
> > if (ret)
> > 
> >   return -EINVAL;
> > 
> > This stuff above will always be if (1) so will always return -EINVAL.
> > Dang!
> 
> Yes, this was exactly what was happening on my MX28 when trying to read one
> channel. Now, after the different comments I'm confused, was this a bug
> that your patch solved or was I doing something wrong?

This patch accidentally (in completely unintended way) solved a bug, yes. But 
the main idea was to just remove redundant code.

Best regards,
Marek Vasut

Re: [PATCH] iio: mxs-lradc: Remove useless check in read_raw

[Hector Palacios]

Dear Marek,

On 07/03/2013 01:38 PM, Marek Vasut wrote:
> Dear Hector Palacios,
>
>> Dear Marek,
>>
>> On 07/02/2013 07:55 PM, Marek Vasut wrote:
>>> Dear Otavio Salvador,
>>>
>>>> On Tue, Jul 2, 2013 at 2:36 PM, Marek Vasut <marex-
> ynQEQJNshbs@public.gmane.org> wrote:
>>>>> Dear Otavio Salvador,
>>>>>
>>>>>> On Tue, Jul 2, 2013 at 1:13 PM, Marek Vasut <marex-
> ynQEQJNshbs@public.gmane.org> wrote:
>>>>>>> Dear Alexandre Belloni,
>>>>>>>
>>>>>>>> On 02/07/2013 14:03, Marek Vasut wrote:
>>>>>>>>> Dear Alexandre Belloni,
>>>>>>>>>
>>>>>>>>>> Dear Marek,
>>>>>>>>>>
>>>>>>>>>> I don't seem to be hitting that issue. I'm using 3.10rc7. Do you
>>>>>>>>>> know how to reproduce it ?
>>>>>>>>>
>>>>>>>>> The check is just redundant, it's not a bug.
>>>>>>>>
>>>>>>>> Ok, that's what I understood first but then got confused by reports
>>>>>>>> of it solving a bug.
>>>>>>>
>>>>>>> It cannot solve a thing. If it does, then we have a problem.
>>>>>>>
>>>>>>> What kind of bug do you see ? How can I replicate it ? Can you send
>>>>>>> me a testcase?
>>>>>>
>>>>>> As I said this code sometimes work. If you put a printf before this
>>>>>> call it sometimes work. So I think we have a race somewhere.
>>>>>>
>>>>>> When I were debugging this I found that when it works we have 10
>>>>>> active channels, it seems.
>>>>>
>>>>> Uh, the read_raw() should exit with -EBUSY, since the mutex_tryload()
>>>>> will fail iff buffered operation is in progress. Or what do you mean by
>>>>> having "10 active channels"?
>>>>
>>>> The read_raw returned invalid. The onehot always returns false and I
>>>> have 10 as mask comparing return.
>>>
>>> Looking at the code.
>>>
>>> the bitmap_set() will set (1 << chan->channel) into "mask" var.
>>>
>>> The iio_validate_scan_mask_onehot is implemented like this:
>>>
>>> 688 bool iio_validate_scan_mask_onehot(struct iio_dev *indio_dev,
>>> 689         const unsigned long *mask)
>>> 690 {
>>> 691         return bitmap_weight(mask, indio_dev->masklength) == 1;
>>> 692 }
>>>
>>> So if mask _always_ has exactly one bit set, the
>>> iio_validate_scan_mask_onehot will always return 1. Aaaand ... now I see
>>> why it fixes a bug for you.
>>>
>>> if (ret)
>>>
>>>    return -EINVAL;
>>>
>>> This stuff above will always be if (1) so will always return -EINVAL.
>>> Dang!
>>
>> Yes, this was exactly what was happening on my MX28 when trying to read one
>> channel. Now, after the different comments I'm confused, was this a bug
>> that your patch solved or was I doing something wrong?
>
> This patch accidentally (in completely unintended way) solved a bug, yes. But
> the main idea was to just remove redundant code.

Maybe you should then amend the commit message to make clear that it solves a bug.

Best regards,
--
Hector Palacios

Re: [PATCH] iio: mxs-lradc: Remove useless check in read_raw

[Jonathan Cameron]

On 07/03/2013 02:58 PM, Hector Palacios wrote:
> Dear Marek,
> 
> On 07/03/2013 01:38 PM, Marek Vasut wrote:
>> Dear Hector Palacios,
>>
>>> Dear Marek,
>>>
>>> On 07/02/2013 07:55 PM, Marek Vasut wrote:
>>>> Dear Otavio Salvador,
>>>>
>>>>> On Tue, Jul 2, 2013 at 2:36 PM, Marek Vasut <marex-
>> ynQEQJNshbs@public.gmane.org> wrote:
>>>>>> Dear Otavio Salvador,
>>>>>>
>>>>>>> On Tue, Jul 2, 2013 at 1:13 PM, Marek Vasut <marex-
>> ynQEQJNshbs@public.gmane.org> wrote:
>>>>>>>> Dear Alexandre Belloni,
>>>>>>>>
>>>>>>>>> On 02/07/2013 14:03, Marek Vasut wrote:
>>>>>>>>>> Dear Alexandre Belloni,
>>>>>>>>>>
>>>>>>>>>>> Dear Marek,
>>>>>>>>>>>
>>>>>>>>>>> I don't seem to be hitting that issue. I'm using 3.10rc7. Do you
>>>>>>>>>>> know how to reproduce it ?
>>>>>>>>>>
>>>>>>>>>> The check is just redundant, it's not a bug.
>>>>>>>>>
>>>>>>>>> Ok, that's what I understood first but then got confused by reports
>>>>>>>>> of it solving a bug.
>>>>>>>>
>>>>>>>> It cannot solve a thing. If it does, then we have a problem.
>>>>>>>>
>>>>>>>> What kind of bug do you see ? How can I replicate it ? Can you send
>>>>>>>> me a testcase?
>>>>>>>
>>>>>>> As I said this code sometimes work. If you put a printf before this
>>>>>>> call it sometimes work. So I think we have a race somewhere.
>>>>>>>
>>>>>>> When I were debugging this I found that when it works we have 10
>>>>>>> active channels, it seems.
>>>>>>
>>>>>> Uh, the read_raw() should exit with -EBUSY, since the mutex_tryload()
>>>>>> will fail iff buffered operation is in progress. Or what do you mean by
>>>>>> having "10 active channels"?
>>>>>
>>>>> The read_raw returned invalid. The onehot always returns false and I
>>>>> have 10 as mask comparing return.
>>>>
>>>> Looking at the code.
>>>>
>>>> the bitmap_set() will set (1 << chan->channel) into "mask" var.
>>>>
>>>> The iio_validate_scan_mask_onehot is implemented like this:
>>>>
>>>> 688 bool iio_validate_scan_mask_onehot(struct iio_dev *indio_dev,
>>>> 689         const unsigned long *mask)
>>>> 690 {
>>>> 691         return bitmap_weight(mask, indio_dev->masklength) == 1;
>>>> 692 }
>>>>
>>>> So if mask _always_ has exactly one bit set, the
>>>> iio_validate_scan_mask_onehot will always return 1. Aaaand ... now I see
>>>> why it fixes a bug for you.
>>>>
>>>> if (ret)
>>>>
>>>>    return -EINVAL;
>>>>
>>>> This stuff above will always be if (1) so will always return -EINVAL.
>>>> Dang!
>>>
>>> Yes, this was exactly what was happening on my MX28 when trying to read one
>>> channel. Now, after the different comments I'm confused, was this a bug
>>> that your patch solved or was I doing something wrong?
>>
>> This patch accidentally (in completely unintended way) solved a bug, yes. But
>> the main idea was to just remove redundant code.
> 
> Maybe you should then amend the commit message to make clear that it solves a bug.
Definitely.

I'll apply a version that mentions the bug in the commit comment to a branch that
will hopefully be applied just after rc1 then can be picked up by stable as appropriate.

Jonathan
> 
> Best regards,
> -- 
> Hector Palacios
> -- 
> 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

Re: [PATCH] iio: mxs-lradc: Remove useless check in read_raw

[Alexandre Belloni]

Hi,

On 02/07/2013 18:13, Marek Vasut wrote:
> Dear Alexandre Belloni,
> 
>> On 02/07/2013 14:03, Marek Vasut wrote:
>>> Dear Alexandre Belloni,
>>>
>>>> Dear Marek,
>>>>
>>>> I don't seem to be hitting that issue. I'm using 3.10rc7. Do you know
>>>> how to reproduce it ?
>>>
>>> The check is just redundant, it's not a bug.
>>
>> Ok, that's what I understood first but then got confused by reports of
>> it solving a bug.
> 
> It cannot solve a thing. If it does, then we have a problem.
> 
> What kind of bug do you see ? How can I replicate it ? Can you send me a 
> testcase?
> 

It doesn't solve anything on my side, I never saw any issue with that
code. I was surprised by those reports though:

http://marc.info/?l=linux-iio&m=137275710622191&w=2
http://marc.info/?l=linux-iio&m=137273629716559&w=2

Hector and Otavio seem to be the ones with an issue ;)

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com