On 09/08/16 01:19, Brian Norris wrote: > When using CONFIG_DEBUG_ATOMIC_SLEEP, the scheduler nicely points out > that we're calling sleeping primitives within the wait_event loop, which > means we might clobber the task state: > > [ 10.831289] do not call blocking ops when !TASK_RUNNING; state=1 set at [<ffffffc00026b610>] > [ 10.845531] ------------[ cut here ]------------ > [ 10.850161] WARNING: at kernel/sched/core.c:7630 > ... > [ 12.164333] ---[ end trace 45409966a9a76438 ]--- > [ 12.168942] Call trace: > [ 12.171391] [<ffffffc00024ed44>] __might_sleep+0x64/0x90 > [ 12.176699] [<ffffffc000954774>] mutex_lock_nested+0x50/0x3fc > [ 12.182440] [<ffffffc0007b9424>] iio_kfifo_buf_data_available+0x28/0x4c > [ 12.189043] [<ffffffc0007b76ac>] iio_buffer_ready+0x60/0xe0 > [ 12.194608] [<ffffffc0007b7834>] iio_buffer_read_first_n_outer+0x108/0x1a8 > [ 12.201474] [<ffffffc000370d48>] __vfs_read+0x58/0x114 > [ 12.206606] [<ffffffc000371740>] vfs_read+0x94/0x118 > [ 12.211564] [<ffffffc0003720f8>] SyS_read+0x64/0xb4 > [ 12.216436] [<ffffffc000203cb4>] el0_svc_naked+0x24/0x28 > > To avoid this, we should (a la https://lwn.net/Articles/628628/) use the > wait_woken() function, which avoids the nested sleeping while still > handling races between waiting / wake-events. > > Signed-off-by: Brian Norris <briannorris@xxxxxxxxxxxx> Looks good to me, but given Lars' involvement in the discussion I'd like his review before applying this. Jonathan > --- > v2: > * Only add/remove to/from the wait queue once per call > * Restore interruptability via explicit signal_pending() call > * Refactor to avoid nested loop > > Note that I did not remove the first (semi-redundant) test for > !indio_dev->info, so as to avoid disturbing the error codes used here. > > drivers/iio/industrialio-buffer.c | 23 +++++++++++++++++------ > 1 file changed, 17 insertions(+), 6 deletions(-) > > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c > index 90462fcf5436..49bf9c59f117 100644 > --- a/drivers/iio/industrialio-buffer.c > +++ b/drivers/iio/industrialio-buffer.c > @@ -107,6 +107,7 @@ ssize_t iio_buffer_read_first_n_outer(struct file *filp, char __user *buf, > { > struct iio_dev *indio_dev = filp->private_data; > struct iio_buffer *rb = indio_dev->buffer; > + DEFINE_WAIT_FUNC(wait, woken_wake_function); > size_t datum_size; > size_t to_wait; > int ret; > @@ -131,19 +132,29 @@ ssize_t iio_buffer_read_first_n_outer(struct file *filp, char __user *buf, > else > to_wait = min_t(size_t, n / datum_size, rb->watermark); > > + add_wait_queue(&rb->pollq, &wait); > do { > - ret = wait_event_interruptible(rb->pollq, > - iio_buffer_ready(indio_dev, rb, to_wait, n / datum_size)); > - if (ret) > - return ret; > + if (!indio_dev->info) { > + ret = -ENODEV; > + break; > + } > > - if (!indio_dev->info) > - return -ENODEV; > + if (!iio_buffer_ready(indio_dev, rb, to_wait, n / datum_size)) { > + if (signal_pending(current)) { > + ret = -ERESTARTSYS; > + break; > + } > + > + wait_woken(&wait, TASK_INTERRUPTIBLE, > + MAX_SCHEDULE_TIMEOUT); > + continue; > + } > > ret = rb->access->read_first_n(rb, n, buf); > if (ret == 0 && (filp->f_flags & O_NONBLOCK)) > ret = -EAGAIN; > } while (ret == 0); > + 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
