* [PATCH v2 0/1] iio:buffer: polling unregistered device stalls user process
@ 2016-09-09 14:20 Gregor Boirie
2016-09-09 14:20 ` [PATCH v2 1/1] iio:buffer: let poll report an error for unregistered devices Gregor Boirie
0 siblings, 1 reply; 7+ messages in thread
From: Gregor Boirie @ 2016-09-09 14:20 UTC (permalink / raw)
To: linux-iio
Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, Gregor Boirie
Version 2 of this patchset.
Changes since v1 :
* merge both patches
* reword commit log subject
* remove fixes tag
Unregistering a device currently in use may stall userspace process polling for
data availability (poll syscall). Here is a typical situation detailed below.
zpa2326 barometer driver is loaded.
<console>
/ # lsmod
Module Size Used by
zpa2326_i2c 2833 0
zpa2326 25360 1 zpa2326_i2c
[...]
</console>
zpa2326 has registered iio device 0 and its own hardware trigger 0.
<console>
/ # lsiio
Device 000: zpa2326
Trigger 000: zpa2326-dev0
/ # ls /dev
bus ptmx tty29 tty52
[...]
iio:device0 tty11 tty35 tty59
</console>
Run an everlasting sampling process (iio_generic_buffer) in hardware triggered
buffer mode then unload module (zpa2326_i2c) driving the device (iio:device0)
under use.
<console>
/ # iio_generic_buffer -a -N 0 -l 2 -c -1 & sleep 1 ; rmmod zpa2326_i2c
iio device number being used is 0
iio trigger number being used is 0
No channels are enabled, enabling all channels
Enabling: in_pressure_en
Enabling: in_timestamp_en
Enabling: in_temp_en
/sys/bus/iio/devices/iio:device0 zpa2326-dev0
103.736877 31265.359375 98504890900
103.734001 31369.199219 98568173608
103.739883 31369.199219 98631470327
103.734070 31369.199219 98694748816
103.733070 31369.199219 98758036681
103.736130 31473.039062 98821330691
103.739006 31369.199219 98884620014
103.737068 31473.039062 98947911524
103.732254 31473.039062 99011214754
103.739006 31473.039062 99074498920
103.739006 31473.039062 99137784962
103.734192 31473.039062 99201085431
103.737190 31473.039062 99264364910
103.741066 31473.039062 99327655274
/ #
</console>
After 1 second, iio_generic_buffer output has been muted. A ps shows
iio_generic_buffer is still there, in interruptible sleep state.
<console>
/ # ps | grep iio_generic_buffer
123 0 1820 S iio_generic_buffer -a -N 0 -l 2 -c -1
128 0 2776 S grep iio_generic_buffer
/ #
</console>
Strace shows iio_generic_buffer is stalled into poll syscall. Forever...
<console>
/ # strace -p 123
Process 123 attached
poll([{fd=3, events=POLLIN}], 1, -1^CProcess 123 detached
<detached ...>
</console>
A sysrq task state trace shows iio_generic_buffer is blocking in...
<console>
/ # [ 202.859427] sysrq: SysRq : Show State
[ 202.863151] task PC stack pid father
[...]
[ 205.667101] iio_generic_buf S 8068a254 0 123 98 0x00000000
[ 205.673507] [<8068a254>] (__schedule) from [<8068ac0c>] (schedule+0x64/0x100)
[ 205.680664] [<8068ac0c>] (schedule) from [<80690820>] (schedule_hrtimeout_range_clock+0x1d8/0x1e8)
[ 205.689641] [<80690820>] (schedule_hrtimeout_range_clock) from [<80690858>] (schedule_hrtimeout_range+0x28/0x30)
[ 205.699831] [<80690858>] (schedule_hrtimeout_range) from [<8022b148>] (poll_schedule_timeout+0x84/0xc8)
[ 205.709241] [<8022b148>] (poll_schedule_timeout) from [<8022d808>] (do_sys_poll+0x640/0x8c4)
[ 205.717697] [<8022d808>] (do_sys_poll) from [<8022db0c>] (do_restart_poll+0x80/0xd4)
[ 205.725461] [<8022db0c>] (do_restart_poll) from [<8004f660>] (sys_restart_syscall+0x30/0x34)
[ 205.733918] [<8004f660>] (sys_restart_syscall) from [<800151a0>] (ret_fast_syscall+0x0/0x1c)
[ 205.742351] Showing busy workqueues and worker pools:
[ 205.747406] workqueue events: flags=0x0
[ 205.751243] pwq 4: cpus=2 node=0 flags=0x0 nice=0 active=1/256
[ 205.757304] in-flight: 23:check_carrier
[ 205.761551] pool 4: cpus=2 node=0 flags=0x0 nice=0 hung=0s workers=4 idle: 61 77 65
</console>
Killing iio_generic_buffer works.
<console>
/ # kill 123
/ # Caught signal 15
failed to open /sys/bus/iio/devices/iio:device0/buffer/enable
Failed to disable buffer: No such file or directory
Enabling/disabling channels: can't open /sys/bus/iio/devices/iio:device0/scan_elements
Failed to disable all channels
[1]+ Done(241) iio_generic_buffer -a -N 0 -l 2 -c -1
/ #
</console>
This situation happens when the device is being unregistered
(iio_device_unregister()) while a userspace process is executing the poll
syscall.
If device has vanished before running the iio_buffer_fileops poll hook, the
latter will keep returning no events and process will be stalled waiting for
events that will never come if no timeout has been specified.
This patch fixes this by ensuring iio_buffer_poll() returns POLLERR if device
has been previously unregistered. Userspace process will be properly notified
something wrong happened so that it may take appropriate action.
Running the same test as above will now show.
<console>
/ # iio_generic_buffer -a -N 0 -l 2 -c -1 & sleep 1; rmmod zpa2326_i2c
iio device number being used is 0
iio trigger number being used is 0
No channels are enabled, enabling all channels
Enabling: in_pressure_en
Enabling: in_timestamp_en
Enabling: in_temp_en
/sys/bus/iio/devices/iio:device0 zpa2326-dev0
103.679695 31680.718750 54318428312
103.684631 31680.718750 54381713521
103.682693 31680.718750 54444999458
103.681694 31784.558594 54508288521
103.679878 31784.558594 54571584458
103.680756 31784.558594 54634876698
103.677002 31784.558594 54698161020
103.682755 31784.558594 54761463208
103.681816 31784.558594 54824749979
103.673004 31784.558594 54888037583
103.675819 31784.558594 54951331958
103.679878 31784.558594 55014612270
103.671066 31784.558594 55077905447
103.683693 31784.558594 55141192583
failed to open /sys/bus/iio/devices/iio:device0/buffer/enable
Failed to disable buffer: No such file or directory
Enabling/disabling channels: can't open /sys/bus/iio/devices/iio:device0/scan_elements
Failed to disable all channels
[1]+ Done(1) iio_generic_buffer -a -N 0 -l 2 -c -1
</console>
Regards,
Gregor.
Gregor Boirie (1):
iio:buffer: let poll report an error for unregistered devices
drivers/iio/industrialio-buffer.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
--
2.1.4
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 1/1] iio:buffer: let poll report an error for unregistered devices
2016-09-09 14:20 [PATCH v2 0/1] iio:buffer: polling unregistered device stalls user process Gregor Boirie
@ 2016-09-09 14:20 ` Gregor Boirie
2016-09-10 15:44 ` Jonathan Cameron
0 siblings, 1 reply; 7+ messages in thread
From: Gregor Boirie @ 2016-09-09 14:20 UTC (permalink / raw)
To: linux-iio
Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, Gregor Boirie
Using iio_device_unregister() on a device currently in use may stall
userspace process polling for data availability (poll syscall).
If device has vanished before running the iio_buffer_fileops poll hook, the
latter will return empty poll event mask. Process will be stalled waiting
for events that will never come (if no timeout specified).
This patch ensures iio_buffer_poll() returns POLLERR if device has just
been unregistered in order to properly notify userspace process something
wrong happened (such as removable device unplugged).
Signed-off-by: Gregor Boirie <gregor.boirie@parrot.com>
---
drivers/iio/industrialio-buffer.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index 90462fc..aad5159 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -155,6 +155,7 @@ ssize_t iio_buffer_read_first_n_outer(struct file *filp, char __user *buf,
* a wait queue
*
* Return: (POLLIN | POLLRDNORM) if data is available for reading
+ * POLLERR if device was unregistered in our back
* or 0 for other cases
*/
unsigned int iio_buffer_poll(struct file *filp,
@@ -162,13 +163,20 @@ unsigned int iio_buffer_poll(struct file *filp,
{
struct iio_dev *indio_dev = filp->private_data;
struct iio_buffer *rb = indio_dev->buffer;
+ bool rdy;
if (!indio_dev->info)
- return 0;
+ return POLLERR;
poll_wait(filp, &rb->pollq, wait);
- if (iio_buffer_ready(indio_dev, rb, rb->watermark, 0))
+ rdy = iio_buffer_ready(indio_dev, rb, rb->watermark, 0);
+
+ if (!indio_dev->info)
+ return POLLERR;
+
+ if (rdy)
return POLLIN | POLLRDNORM;
+
return 0;
}
--
2.1.4
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/1] iio:buffer: let poll report an error for unregistered devices
2016-09-09 14:20 ` [PATCH v2 1/1] iio:buffer: let poll report an error for unregistered devices Gregor Boirie
@ 2016-09-10 15:44 ` Jonathan Cameron
2016-09-12 12:18 ` Lars-Peter Clausen
0 siblings, 1 reply; 7+ messages in thread
From: Jonathan Cameron @ 2016-09-10 15:44 UTC (permalink / raw)
To: Gregor Boirie, linux-iio
Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler
On 09/09/16 15:20, Gregor Boirie wrote:
> Using iio_device_unregister() on a device currently in use may stall
> userspace process polling for data availability (poll syscall).
>
> If device has vanished before running the iio_buffer_fileops poll hook, the
> latter will return empty poll event mask. Process will be stalled waiting
> for events that will never come (if no timeout specified).
>
> This patch ensures iio_buffer_poll() returns POLLERR if device has just
> been unregistered in order to properly notify userspace process something
> wrong happened (such as removable device unplugged).
>
> Signed-off-by: Gregor Boirie <gregor.boirie@parrot.com>
Looks good to me.
Lars, as you were involved in the discussions of v1 could you
take a quick look.
Thanks,
Jonathan
> ---
> drivers/iio/industrialio-buffer.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index 90462fc..aad5159 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -155,6 +155,7 @@ ssize_t iio_buffer_read_first_n_outer(struct file *filp, char __user *buf,
> * a wait queue
> *
> * Return: (POLLIN | POLLRDNORM) if data is available for reading
> + * POLLERR if device was unregistered in our back
> * or 0 for other cases
> */
> unsigned int iio_buffer_poll(struct file *filp,
> @@ -162,13 +163,20 @@ unsigned int iio_buffer_poll(struct file *filp,
> {
> struct iio_dev *indio_dev = filp->private_data;
> struct iio_buffer *rb = indio_dev->buffer;
> + bool rdy;
>
> if (!indio_dev->info)
> - return 0;
> + return POLLERR;
>
> poll_wait(filp, &rb->pollq, wait);
> - if (iio_buffer_ready(indio_dev, rb, rb->watermark, 0))
> + rdy = iio_buffer_ready(indio_dev, rb, rb->watermark, 0);
> +
> + if (!indio_dev->info)
> + return POLLERR;
> +
> + if (rdy)
> return POLLIN | POLLRDNORM;
> +
> return 0;
> }
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/1] iio:buffer: let poll report an error for unregistered devices
2016-09-10 15:44 ` Jonathan Cameron
@ 2016-09-12 12:18 ` Lars-Peter Clausen
2016-09-12 13:24 ` Gregor Boirie
0 siblings, 1 reply; 7+ messages in thread
From: Lars-Peter Clausen @ 2016-09-12 12:18 UTC (permalink / raw)
To: Jonathan Cameron, Gregor Boirie, linux-iio
Cc: Hartmut Knaack, Peter Meerwald-Stadler
Hi,
I had some more comments inline in v1, please see below.
On 09/10/2016 05:44 PM, Jonathan Cameron wrote:
>> @@ -162,13 +163,20 @@ unsigned int iio_buffer_poll(struct file *filp,
>> {
>> struct iio_dev *indio_dev = filp->private_data;
>> struct iio_buffer *rb = indio_dev->buffer;
>> + bool rdy;
>>
>> if (!indio_dev->info)
>> - return 0;
>> + return POLLERR;
I've had a look at what other subsystems do and input returns POLLERR |
POLLHUP. Maybe we should mirror this.
>>
>> poll_wait(filp, &rb->pollq, wait);
>> - if (iio_buffer_ready(indio_dev, rb, rb->watermark, 0))
>> + rdy = iio_buffer_ready(indio_dev, rb, rb->watermark, 0);
>> +
>> + if (!indio_dev->info)
>> + return POLLERR;
Why check indio_dev->info twice though, since there is no locking involved
the second check does gain us anything in terms of race condition avoidance.
I think we should have one check after the poll_wait() call. If the device
is unregistered we wake the pollq. So adding it there should make sure that
the poll() callback is called again if the device is unregistered after the
check.
I'm not sure though if we'd need a explicit memory barrier or whether
poll_wait() can act as a implicit one. But probably we are ok, given that
poll_wait() should take a lock.
To be sure that the code is race free we need a guarantee that there is
strict ordering between the indio_dev->info = NULL and wake_up() in
unregister as well as between poll_wait() and the if (indio_dev->info) check
in poll().
The scenario is basically
CPU0 CPU1
---------------------------------------------------------
poll_wait() indio_dev->info = NULL
if (!indio_dev->info) wake_up()
If strict ordering is not observed for either side we could end up missing
the unregister and poll() will block forever just as before.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/1] iio:buffer: let poll report an error for unregistered devices
2016-09-12 12:18 ` Lars-Peter Clausen
@ 2016-09-12 13:24 ` Gregor Boirie
2016-09-12 13:36 ` Lars-Peter Clausen
0 siblings, 1 reply; 7+ messages in thread
From: Gregor Boirie @ 2016-09-12 13:24 UTC (permalink / raw)
To: Lars-Peter Clausen, Jonathan Cameron, linux-iio
Cc: Hartmut Knaack, Peter Meerwald-Stadler
Hi,
On 09/12/2016 02:18 PM, Lars-Peter Clausen wrote:
> Hi,
>
> I had some more comments inline in v1, please see below.
>
> On 09/10/2016 05:44 PM, Jonathan Cameron wrote:
>>> @@ -162,13 +163,20 @@ unsigned int iio_buffer_poll(struct file *filp,
>>> {
>>> struct iio_dev *indio_dev = filp->private_data;
>>> struct iio_buffer *rb = indio_dev->buffer;
>>> + bool rdy;
>>>
>>> if (!indio_dev->info)
>>> - return 0;
>>> + return POLLERR;
> I've had a look at what other subsystems do and input returns POLLERR |
> POLLHUP. Maybe we should mirror this.
Well, some input device only don't IFAIK (I'm no expert on this) as
POLLHUP might be focused on output errors only.
>
>>>
>>> poll_wait(filp, &rb->pollq, wait);
>>> - if (iio_buffer_ready(indio_dev, rb, rb->watermark, 0))
>>> + rdy = iio_buffer_ready(indio_dev, rb, rb->watermark, 0);
>>> +
>>> + if (!indio_dev->info)
>>> + return POLLERR;
> Why check indio_dev->info twice though, since there is no locking involved
> the second check does gain us anything in terms of race condition avoidance.
I took iio_buffer_read_first_n_outer() as a reference : the check of
indio_dev->info is performed at least twice :
* once at function start
* each time wait_event_interruptible() returns.
I thought it would be a good starting point.
> I think we should have one check after the poll_wait() call. If the device
> is unregistered we wake the pollq. So adding it there should make sure that
> the poll() callback is called again if the device is unregistered after the
> check.
No risk that an "exotic" userspace process perform 2 sys_poll() in the time
between iio_device_unregister() and iio_device_free() ?
I mean : am I wrong in thinking that the filep is still valid as long as
the fd has
not been released completly ?
>
> I'm not sure though if we'd need a explicit memory barrier or whether
> poll_wait() can act as a implicit one. But probably we are ok, given that
> poll_wait() should take a lock.
>
> To be sure that the code is race free we need a guarantee that there is
> strict ordering between the indio_dev->info = NULL and wake_up() in
> unregister as well as between poll_wait() and the if (indio_dev->info) check
> in poll().
>
> The scenario is basically
>
> CPU0 CPU1
> ---------------------------------------------------------
> poll_wait() indio_dev->info = NULL
> if (!indio_dev->info) wake_up()
>
>
> If strict ordering is not observed for either side we could end up missing
> the unregister and poll() will block forever just as before.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/1] iio:buffer: let poll report an error for unregistered devices
2016-09-12 13:24 ` Gregor Boirie
@ 2016-09-12 13:36 ` Lars-Peter Clausen
2016-09-13 17:19 ` Jonathan Cameron
0 siblings, 1 reply; 7+ messages in thread
From: Lars-Peter Clausen @ 2016-09-12 13:36 UTC (permalink / raw)
To: Gregor Boirie, Jonathan Cameron, linux-iio
Cc: Hartmut Knaack, Peter Meerwald-Stadler
On 09/12/2016 03:24 PM, Gregor Boirie wrote:
> Hi,
>
> On 09/12/2016 02:18 PM, Lars-Peter Clausen wrote:
>> Hi,
>>
>> I had some more comments inline in v1, please see below.
>>
>> On 09/10/2016 05:44 PM, Jonathan Cameron wrote:
>>>> @@ -162,13 +163,20 @@ unsigned int iio_buffer_poll(struct file *filp,
>>>> {
>>>> struct iio_dev *indio_dev = filp->private_data;
>>>> struct iio_buffer *rb = indio_dev->buffer;
>>>> + bool rdy;
>>>> if (!indio_dev->info)
>>>> - return 0;
>>>> + return POLLERR;
>> I've had a look at what other subsystems do and input returns POLLERR |
>> POLLHUP. Maybe we should mirror this.
> Well, some input device only don't IFAIK (I'm no expert on this) as
> POLLHUP might be focused on output errors only.
The way I understand it is that the 'output only' description for POLLHUP
means that you can't pass POLLHUP as a event to listen for. A POLLHUP will
always wakeup the poll() and be set in the revents field. It does not meant
that is only set for output devices.
>
>>
>>>> poll_wait(filp, &rb->pollq, wait);
>>>> - if (iio_buffer_ready(indio_dev, rb, rb->watermark, 0))
>>>> + rdy = iio_buffer_ready(indio_dev, rb, rb->watermark, 0);
>>>> +
>>>> + if (!indio_dev->info)
>>>> + return POLLERR;
>> Why check indio_dev->info twice though, since there is no locking involved
>> the second check does gain us anything in terms of race condition avoidance.
> I took iio_buffer_read_first_n_outer() as a reference : the check of
> indio_dev->info is performed at least twice :
> * once at function start
> * each time wait_event_interruptible() returns.
>
> I thought it would be a good starting point.
>
Strictly speaking the first check there is redundant.
>> I think we should have one check after the poll_wait() call. If the device
>> is unregistered we wake the pollq. So adding it there should make sure that
>> the poll() callback is called again if the device is unregistered after the
>> check.
> No risk that an "exotic" userspace process perform 2 sys_poll() in the time
> between iio_device_unregister() and iio_device_free() ?
> I mean : am I wrong in thinking that the filep is still valid as long as the
> fd has
> not been released completly ?
The filep is valid as long as the file is open, same is true for the iio_dev
and iio_buffer structures. The open file holds a reference to them.
What I meant with the race condition here is that we should make sure that
no matter how the device unregister() and the sys_poll() are timed relative
to each other it should still work as expected and not result in a
indefinitely waiting poll() operation.
When multiple CPUs are involved the two operations can happen at the same
time, so we need to cover that.
>
>>
>> I'm not sure though if we'd need a explicit memory barrier or whether
>> poll_wait() can act as a implicit one. But probably we are ok, given that
>> poll_wait() should take a lock.
>>
>> To be sure that the code is race free we need a guarantee that there is
>> strict ordering between the indio_dev->info = NULL and wake_up() in
>> unregister as well as between poll_wait() and the if (indio_dev->info) check
>> in poll().
>>
>> The scenario is basically
>>
>> CPU0 CPU1
>> ---------------------------------------------------------
>> poll_wait() indio_dev->info = NULL
>> if (!indio_dev->info) wake_up()
>>
>>
>> If strict ordering is not observed for either side we could end up missing
>> the unregister and poll() will block forever just as before.
>
> --
> 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/1] iio:buffer: let poll report an error for unregistered devices
2016-09-12 13:36 ` Lars-Peter Clausen
@ 2016-09-13 17:19 ` Jonathan Cameron
0 siblings, 0 replies; 7+ messages in thread
From: Jonathan Cameron @ 2016-09-13 17:19 UTC (permalink / raw)
To: Lars-Peter Clausen, Gregor Boirie, linux-iio
Cc: Hartmut Knaack, Peter Meerwald-Stadler
On 12/09/16 14:36, Lars-Peter Clausen wrote:
> On 09/12/2016 03:24 PM, Gregor Boirie wrote:
>> Hi,
>>
>> On 09/12/2016 02:18 PM, Lars-Peter Clausen wrote:
>>> Hi,
>>>
>>> I had some more comments inline in v1, please see below.
>>>
>>> On 09/10/2016 05:44 PM, Jonathan Cameron wrote:
>>>>> @@ -162,13 +163,20 @@ unsigned int iio_buffer_poll(struct file *filp,
>>>>> {
>>>>> struct iio_dev *indio_dev = filp->private_data;
>>>>> struct iio_buffer *rb = indio_dev->buffer;
>>>>> + bool rdy;
>>>>> if (!indio_dev->info)
>>>>> - return 0;
>>>>> + return POLLERR;
>>> I've had a look at what other subsystems do and input returns POLLERR |
>>> POLLHUP. Maybe we should mirror this.
>> Well, some input device only don't IFAIK (I'm no expert on this) as
>> POLLHUP might be focused on output errors only.
>
> The way I understand it is that the 'output only' description for POLLHUP
> means that you can't pass POLLHUP as a event to listen for. A POLLHUP will
> always wakeup the poll() and be set in the revents field. It does not meant
> that is only set for output devices.
>
>>
>>>
>>>>> poll_wait(filp, &rb->pollq, wait);
>>>>> - if (iio_buffer_ready(indio_dev, rb, rb->watermark, 0))
>>>>> + rdy = iio_buffer_ready(indio_dev, rb, rb->watermark, 0);
>>>>> +
>>>>> + if (!indio_dev->info)
>>>>> + return POLLERR;
>>> Why check indio_dev->info twice though, since there is no locking involved
>>> the second check does gain us anything in terms of race condition avoidance.
>> I took iio_buffer_read_first_n_outer() as a reference : the check of
>> indio_dev->info is performed at least twice :
>> * once at function start
>> * each time wait_event_interruptible() returns.
>>
>> I thought it would be a good starting point.
>>
>
> Strictly speaking the first check there is redundant.
Feel free to post a patch killing it off :)
J
>
>>> I think we should have one check after the poll_wait() call. If the device
>>> is unregistered we wake the pollq. So adding it there should make sure that
>>> the poll() callback is called again if the device is unregistered after the
>>> check.
>> No risk that an "exotic" userspace process perform 2 sys_poll() in the time
>> between iio_device_unregister() and iio_device_free() ?
>> I mean : am I wrong in thinking that the filep is still valid as long as the
>> fd has
>> not been released completly ?
>
> The filep is valid as long as the file is open, same is true for the iio_dev
> and iio_buffer structures. The open file holds a reference to them.
>
> What I meant with the race condition here is that we should make sure that
> no matter how the device unregister() and the sys_poll() are timed relative
> to each other it should still work as expected and not result in a
> indefinitely waiting poll() operation.
>
> When multiple CPUs are involved the two operations can happen at the same
> time, so we need to cover that.
>
>>
>>>
>>> I'm not sure though if we'd need a explicit memory barrier or whether
>>> poll_wait() can act as a implicit one. But probably we are ok, given that
>>> poll_wait() should take a lock.
>>>
>>> To be sure that the code is race free we need a guarantee that there is
>>> strict ordering between the indio_dev->info = NULL and wake_up() in
>>> unregister as well as between poll_wait() and the if (indio_dev->info) check
>>> in poll().
>>>
>>> The scenario is basically
>>>
>>> CPU0 CPU1
>>> ---------------------------------------------------------
>>> poll_wait() indio_dev->info = NULL
>>> if (!indio_dev->info) wake_up()
>>>
>>>
>>> If strict ordering is not observed for either side we could end up missing
>>> the unregister and poll() will block forever just as before.
>>
>> --
>> 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
>
> --
> 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
>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-09-13 17:19 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-09 14:20 [PATCH v2 0/1] iio:buffer: polling unregistered device stalls user process Gregor Boirie
2016-09-09 14:20 ` [PATCH v2 1/1] iio:buffer: let poll report an error for unregistered devices Gregor Boirie
2016-09-10 15:44 ` Jonathan Cameron
2016-09-12 12:18 ` Lars-Peter Clausen
2016-09-12 13:24 ` Gregor Boirie
2016-09-12 13:36 ` Lars-Peter Clausen
2016-09-13 17:19 ` Jonathan Cameron
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.