[PATCH] iio: ensure ret is initialized to zero before entering do loop

[PATCH] iio: ensure ret is initialized to zero before entering do loop

[Colin King]

From: Colin Ian King <colin.king@canonical.com>

A recent fix to iio_buffer_read_first_n_outer removed ret from being set by
a return from wait_event_interruptible and also added a continue in a loop
which causes the variable ret to not be set when it reaches the end of the
loop.  Fix this by initializing ret to zero.

Also remove extraneous white space at the end of the loop.

Fixes: fcf68f3c0bb2a5 ("fix sched WARNING "do not call blocking ops when !TASK_RUNNING")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 drivers/iio/industrialio-buffer.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index 49bf9c5..158aaf4 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -110,7 +110,7 @@ ssize_t iio_buffer_read_first_n_outer(struct file *filp, char __user *buf,
 	DEFINE_WAIT_FUNC(wait, woken_wake_function);
 	size_t datum_size;
 	size_t to_wait;
-	int ret;
+	int ret = 0;
 
 	if (!indio_dev->info)
 		return -ENODEV;
@@ -153,7 +153,7 @@ ssize_t iio_buffer_read_first_n_outer(struct file *filp, char __user *buf,
 		ret = rb->access->read_first_n(rb, n, buf);
 		if (ret == 0 && (filp->f_flags & O_NONBLOCK))
 			ret = -EAGAIN;
-	 } while (ret == 0);
+	} while (ret == 0);
 	remove_wait_queue(&rb->pollq, &wait);
 
 	return ret;
-- 
2.9.3

Re: [PATCH] iio: ensure ret is initialized to zero before entering do loop

[Jonathan Cameron]

On 05/09/16 15:39, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> A recent fix to iio_buffer_read_first_n_outer removed ret from being set by
> a return from wait_event_interruptible and also added a continue in a loop
> which causes the variable ret to not be set when it reaches the end of the
> loop.  Fix this by initializing ret to zero.
> 
> Also remove extraneous white space at the end of the loop.
> 
> Fixes: fcf68f3c0bb2a5 ("fix sched WARNING "do not call blocking ops when !TASK_RUNNING")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
Good find.  Strange that got through 0-day without a warning...

Cc'd Brian as author of the fix this is fixing.
Brian can you sanity check this patch as well.

Applied to the fixes-togreg branch of iio.git and marked for stable.
Ah well, another one for the statistics on stable patches that introduce bugs while
fixing other bugs.

Pretty unlikely this will be hit I think, but in theory you never know.

Jonathan
> ---
>  drivers/iio/industrialio-buffer.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index 49bf9c5..158aaf4 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -110,7 +110,7 @@ ssize_t iio_buffer_read_first_n_outer(struct file *filp, char __user *buf,
>  	DEFINE_WAIT_FUNC(wait, woken_wake_function);
>  	size_t datum_size;
>  	size_t to_wait;
> -	int ret;
> +	int ret = 0;
>  
>  	if (!indio_dev->info)
>  		return -ENODEV;
> @@ -153,7 +153,7 @@ ssize_t iio_buffer_read_first_n_outer(struct file *filp, char __user *buf,
>  		ret = rb->access->read_first_n(rb, n, buf);
>  		if (ret == 0 && (filp->f_flags & O_NONBLOCK))
>  			ret = -EAGAIN;
> -	 } while (ret == 0);
> +	} while (ret == 0);
>  	remove_wait_queue(&rb->pollq, &wait);
>  
>  	return ret;
> 

Re: [PATCH] iio: ensure ret is initialized to zero before entering do loop

[Brian Norris]

Hi,

On Mon, Sep 05, 2016 at 09:03:26PM +0100, Jonathan Cameron wrote:
> On 05/09/16 15:39, Colin King wrote:
> > From: Colin Ian King <colin.king@canonical.com>
> > 
> > A recent fix to iio_buffer_read_first_n_outer removed ret from being set by
> > a return from wait_event_interruptible and also added a continue in a loop
> > which causes the variable ret to not be set when it reaches the end of the
> > loop.  Fix this by initializing ret to zero.
> > 
> > Also remove extraneous white space at the end of the loop.
> > 
> > Fixes: fcf68f3c0bb2a5 ("fix sched WARNING "do not call blocking ops when !TASK_RUNNING")

Not that it really matters, but if the commit is still going to be
amended at all, the subject was "iio: fix ...", not just "fix ...".
Definitely not important though.

> > Signed-off-by: Colin Ian King <colin.king@canonical.com>
> Good find.  Strange that got through 0-day without a warning...
> 
> Cc'd Brian as author of the fix this is fixing.
> Brian can you sanity check this patch as well.

Indeed, looks fine, and works fine:

Tested-by: Brian Norris <briannorris@chromium.org>
Reviewed-by: Brian Norris <briannorris@chromium.org>

Thanks for the fix Colin, and sorry for not noticing that error :(

> Applied to the fixes-togreg branch of iio.git and marked for stable.
> Ah well, another one for the statistics on stable patches that introduce bugs while
> fixing other bugs.
> 
> Pretty unlikely this will be hit I think, but in theory you never know.
> 
> Jonathan
> > ---
> >  drivers/iio/industrialio-buffer.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> > index 49bf9c5..158aaf4 100644
> > --- a/drivers/iio/industrialio-buffer.c
> > +++ b/drivers/iio/industrialio-buffer.c
> > @@ -110,7 +110,7 @@ ssize_t iio_buffer_read_first_n_outer(struct file *filp, char __user *buf,
> >  	DEFINE_WAIT_FUNC(wait, woken_wake_function);
> >  	size_t datum_size;
> >  	size_t to_wait;
> > -	int ret;
> > +	int ret = 0;
> >  
> >  	if (!indio_dev->info)
> >  		return -ENODEV;
> > @@ -153,7 +153,7 @@ ssize_t iio_buffer_read_first_n_outer(struct file *filp, char __user *buf,
> >  		ret = rb->access->read_first_n(rb, n, buf);
> >  		if (ret == 0 && (filp->f_flags & O_NONBLOCK))
> >  			ret = -EAGAIN;
> > -	 } while (ret == 0);
> > +	} while (ret == 0);

Personally, I avoided the temptation to fix the whitespace error in a
bugfix patch. But this does scratch my itch :)

Brian

> >  	remove_wait_queue(&rb->pollq, &wait);
> >  
> >  	return ret;
> > 
> 

Re: [PATCH] iio: ensure ret is initialized to zero before entering do loop

[Jonathan Cameron]

On 06/09/16 18:10, Brian Norris wrote:
> Hi,
> 
> On Mon, Sep 05, 2016 at 09:03:26PM +0100, Jonathan Cameron wrote:
>> On 05/09/16 15:39, Colin King wrote:
>>> From: Colin Ian King <colin.king@canonical.com>
>>>
>>> A recent fix to iio_buffer_read_first_n_outer removed ret from being set by
>>> a return from wait_event_interruptible and also added a continue in a loop
>>> which causes the variable ret to not be set when it reaches the end of the
>>> loop.  Fix this by initializing ret to zero.
>>>
>>> Also remove extraneous white space at the end of the loop.
>>>
>>> Fixes: fcf68f3c0bb2a5 ("fix sched WARNING "do not call blocking ops when !TASK_RUNNING")
> 
> Not that it really matters, but if the commit is still going to be
> amended at all, the subject was "iio: fix ...", not just "fix ...".
> Definitely not important though.
> 
>>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>> Good find.  Strange that got through 0-day without a warning...
>>
>> Cc'd Brian as author of the fix this is fixing.
>> Brian can you sanity check this patch as well.
> 
> Indeed, looks fine, and works fine:
> 
> Tested-by: Brian Norris <briannorris@chromium.org>
> Reviewed-by: Brian Norris <briannorris@chromium.org>
Hi Brian,

Thanks for checking this out.  Unfortunately I've already
applied it to fixes-togreg branch of iio.git which is strictly
non rebasing so we'll have to rely on the email trace rather
than git history to pick up on your tested by /reviewed by!

Made me more comfortable sending the pull request to Greg
though so thanks!

Jonathan
> 
> Thanks for the fix Colin, and sorry for not noticing that error :(
> 
>> Applied to the fixes-togreg branch of iio.git and marked for stable.
>> Ah well, another one for the statistics on stable patches that introduce bugs while
>> fixing other bugs.
>>
>> Pretty unlikely this will be hit I think, but in theory you never know.
>>
>> Jonathan
>>> ---
>>>  drivers/iio/industrialio-buffer.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
>>> index 49bf9c5..158aaf4 100644
>>> --- a/drivers/iio/industrialio-buffer.c
>>> +++ b/drivers/iio/industrialio-buffer.c
>>> @@ -110,7 +110,7 @@ ssize_t iio_buffer_read_first_n_outer(struct file *filp, char __user *buf,
>>>  	DEFINE_WAIT_FUNC(wait, woken_wake_function);
>>>  	size_t datum_size;
>>>  	size_t to_wait;
>>> -	int ret;
>>> +	int ret = 0;
>>>  
>>>  	if (!indio_dev->info)
>>>  		return -ENODEV;
>>> @@ -153,7 +153,7 @@ ssize_t iio_buffer_read_first_n_outer(struct file *filp, char __user *buf,
>>>  		ret = rb->access->read_first_n(rb, n, buf);
>>>  		if (ret == 0 && (filp->f_flags & O_NONBLOCK))
>>>  			ret = -EAGAIN;
>>> -	 } while (ret == 0);
>>> +	} while (ret == 0);
> 
> Personally, I avoided the temptation to fix the whitespace error in a
> bugfix patch. But this does scratch my itch :)
> 
> Brian
> 
>>>  	remove_wait_queue(&rb->pollq, &wait);
>>>  
>>>  	return ret;
>>>
>>
> --
> 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
>