analogdevicesinc / libiio

A cross platform library for interfacing with local and remote Linux IIO devices
http://analogdevicesinc.github.io/libiio/
GNU Lesser General Public License v2.1
484 stars 312 forks source link

Disassociating Trigger after iio_buffer_destroy() #189

Open gavingrant opened 6 years ago

gavingrant commented 6 years ago

Hi,

I have encountered an issue when trying to disassociate a device trigger. If I immediately call iio_device_set_trigger(dev, NULL) after iio_buffer_destroy(), I get an EBUSY error.

I tried to prevent this by inserting a call to iio_buffer_cancel() before iio_buffer_destroy(). This appears to make the error less frequent, but it can still occur. If I insert a call to sleep(1) between iio_buffer_destroy() & iio_device_set_trigger(dev, NULL), the error does not occur.

Reproducible with https://github.com/analogdevicesinc/libiio/blob/master/examples/dummy-iiostream.c:

diff --git a/examples/dummy-iiostream.c b/examples/dummy-iiostream.c
index 994ac49..229aa52 100644
--- a/examples/dummy-iiostream.c
+++ b/examples/dummy-iiostream.c
@@ -42,6 +42,7 @@
 #include <stdio.h>
 #include <errno.h>
 #include <getopt.h>
+#include <unistd.h>

 #ifdef __APPLE__
 #include <iio/iio.h>
@@ -84,10 +85,16 @@ static void shutdown()
        if (channels) { free(channels); }

        printf("* Destroying buffers\n");
-       if (rxbuf) { iio_buffer_destroy(rxbuf); }
+       if (rxbuf) {
+            iio_buffer_destroy(rxbuf);
+            sleep(1);
+        }

        printf("* Disassociate trigger\n");
-       if (dev) { iio_device_set_trigger(dev, NULL); }
+       if (dev) {
+            int status = iio_device_set_trigger(dev, NULL);
+            printf("\nstatus:%d\n", status);
+        }

Thanks very much for all the work on libiio by the way, it is super useful! :)

gavingrant commented 6 years ago

This was modified to run using a network context by the way; not sure if that makes a difference.

@@ -186,7 +193,7 @@ int main (int argc, char **argv)
        has_repeat = major >= 0 && minor >= 8 ? true : false;

        printf("* Acquiring IIO context\n");
-       assert((ctx = iio_create_default_context()) && "No context");
+       assert((ctx = iio_create_network_context("192.168.3.2")) && "No context");
        assert(iio_context_get_devices_count(ctx) > 0 && "No devices");
pcercuei commented 6 years ago

Can you reproduce it with the local backend?

gavingrant commented 6 years ago

Paul,

No, only with the network backend between the host and target. If however I use the network backend on the target using the loopback device (127.0.0.1), it is fine, which perhaps suggests it is a network performance issue?

I should note here that the target is a QEMU ARM machine, connected to the host via SLIRP.

lclausen-adi commented 6 years ago

The problem is that in the IIOD there is a capture thread. This thread keeps a local buffer open, when you destroy the remote buffer the read is shut down asynchronously. This means iio_buffer_destroy() can return before the thread actually stopped and the local buffer is destroyed. The reason for this is that there can be multiple readers and when you destroy the network buffer the local buffer might still be in use with by other readers and might not get destroyed at all.

Now what I suppose can happen is that the set_trigger function gets executed before the local buffer is destroyed and hence it returns an EBUSY error.

A short sleep is a good workaround, but long term this should work without the need for it. Not sure how to implement it though.

pcercuei commented 6 years ago

Could maybe the last client who close a device in IIOD wait for the R/W thread to stop? Then we'd be able to remove the sleep hack we have in open_dev_helper() as well.

lclausen-adi commented 6 years ago

We don't know if it is the last. A new client could connect just as it is shutting down.

pcercuei commented 5 years ago

@lclausen-adi, I suppose set_trigger() in iiod/ops.c could be modified to handle that particular case.