[PATCH 00/10] staging: comedi: some comedi_read() changes

[PATCH 00/10] staging: comedi: some comedi_read() changes

[Ian Abbott]

Tidy up the "read" file operation handler, `comedi_read()` a bit and
improve the error handling and the "end-of-file" handling.

There are some other changes I want to make, such as switching to the
newer wait API (prepare_to_wait()/finish_wait()) and preventing several
tasks trying to read or write the same subdevice at the same time (but
without using the COMEDI device's main mutex as it is too coarse).
Those changes can wait until after I've cleaned up and improved the
"write" file operation handler a bit.

01) staging: comedi: remain busy until read end-of-file
02) staging: comedi: don't consider "unmunged" data when becoming
    non-busy
03) staging: comedi: do extra checks for becoming non-busy for "read"
04) staging: comedi: make some variables unsigned in comedi_read()
05) staging: comedi: avoid bad truncation of a size_t in comedi_read()
06) staging: comedi: allow buffer wraparound in comedi_read()
07) staging: comedi: remove superfluous retval = 0 in comedi_read()
08) staging: comedi: return error on "read" if no command set up
09) staging: comedi: simplify returned errors for comedi_read()
10) staging: comedi: check for more errors for zero-length read

 drivers/staging/comedi/comedi_fops.c | 68 +++++++++++++++++-------------------
 1 file changed, 32 insertions(+), 36 deletions(-)

[PATCH 01/10] staging: comedi: remain busy until read end-of-file

[Ian Abbott]

If a COMEDI subdevice is busy handling an asynchronous command in the
"read" direction, then after the command has terminated itself, the
"read" file operation handler, `comedi_read()` should keep the subdevice
busy until all available data has been read and it has returned 0 to
indicate an "end-of-file" condition.  Currently, it has a bug where it
can mark the subdevice as non-busy even when returning a non-zero count.
The bug is slightly hidden because the next "read" will return 0 because
the subdevice is no longer busy.  Fix it by checking the return count is
0 before deciding to mark the subdevice as non-busy.

The call to `comedi_is_subdevice_idle()` is superfluous as the
`become_nonbusy` variable will have been set to `true` when considering
becoming non-busy.  Strictly speaking, checking the return count is
superfluous too, as `become_nonbusy` doesn't get set to `true` unless
the count is 0, but check the return count anyway to make the intention
clearer.

Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
---
 drivers/staging/comedi/comedi_fops.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
index ef4b58b..f533113 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -2550,7 +2550,7 @@ static ssize_t comedi_read(struct file *file, char __user *buf, size_t nbytes,
 	}
 	remove_wait_queue(&async->wait_head, &wait);
 	set_current_state(TASK_RUNNING);
-	if (become_nonbusy || comedi_is_subdevice_idle(s)) {
+	if (become_nonbusy && count == 0) {
 		struct comedi_subdevice *new_s;
 
 		/*
-- 
2.6.1

[PATCH 01/10] staging: comedi: remain busy until read end-of-file

[Ian Abbott]

If a COMEDI subdevice is busy handling an asynchronous command in the
"read" direction, then after the command has terminated itself, the
"read" file operation handler, `comedi_read()` should keep the subdevice
busy until all available data has been read and it has returned 0 to
indicate an "end-of-file" condition.  Currently, it has a bug where it
can mark the subdevice as non-busy even when returning a non-zero count.
The bug is slightly hidden because the next "read" will return 0 because
the subdevice is no longer busy.  Fix it by checking the return count is
0 before deciding to mark the subdevice as non-busy.

The call to `comedi_is_subdevice_idle()` is superfluous as the
`become_nonbusy` variable will have been set to `true` when considering
becoming non-busy.  Strictly speaking, checking the return count is
superfluous too, as `become_nonbusy` doesn't get set to `true` unless
the count is 0, but check the return count anyway to make the intention
clearer.

Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
Reviewed-by: H Hartley Sweeten <hsweeten@visionengravers.com>
---
 drivers/staging/comedi/comedi_fops.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
index ef4b58b..f533113 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -2550,7 +2550,7 @@ static ssize_t comedi_read(struct file *file, char __user *buf, size_t nbytes,
 	}
 	remove_wait_queue(&async->wait_head, &wait);
 	set_current_state(TASK_RUNNING);
-	if (become_nonbusy || comedi_is_subdevice_idle(s)) {
+	if (become_nonbusy && count == 0) {
 		struct comedi_subdevice *new_s;
 
 		/*
-- 
2.6.1

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 02/10] staging: comedi: don't consider "unmunged" data when becoming non-busy

If an asynchronous "read" command is no longer running but the subdevice
is still busy, it becomes non-busy once there is no more data available
in the buffer.  Some or all of the data written to the buffer might not
have been "munged" yet, and it cannot be read until it has been munged
by the writer.  However, since the command is no longer running, we
cannot expect any remaining unmunged data to get munged so we should
ignore it.  Call `comedi_buf_read_n_available()` to check the amount of
munged data available to be read, replacing the call to
`comedi_buf_n_bytes_ready()` which checked the amount of written (but
possibly not yet munged) data available to be read.  This affects both
the "read" file operation (done in `comedi_read()`) and the
`COMEDI_BUFINFO` ioctl handling (done in `do_bufinfo_ioctl()`).  (The
latter is used when data is transferred directly through the mmapped
buffer instead of via the "read" file operation.)

Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
---
 drivers/staging/comedi/comedi_fops.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
index f533113..89e8e87 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -1146,7 +1146,7 @@ static int do_bufinfo_ioctl(struct comedi_device *dev,
 		comedi_buf_read_free(s, bi.bytes_read);
 
 		if (comedi_is_subdevice_idle(s) &&
-		    comedi_buf_n_bytes_ready(s) == 0) {
+		    comedi_buf_read_n_available(s) == 0) {
 			do_become_nonbusy(dev, s);
 		}
 	}
@@ -2570,7 +2570,8 @@ static ssize_t comedi_read(struct file *file, char __user *buf, size_t nbytes,
 		new_s = comedi_file_read_subdevice(file);
 		if (dev->attached && old_detach_count == dev->detach_count &&
 		    s == new_s && new_s->async == async) {
-			if (become_nonbusy || comedi_buf_n_bytes_ready(s) == 0)
+			if (become_nonbusy ||
+			    comedi_buf_read_n_available(s) == 0)
 				do_become_nonbusy(dev, s);
 		}
 		mutex_unlock(&dev->mutex);
-- 
2.6.1

[PATCH 02/10] staging: comedi: don't consider "unmunged" data when becoming non-busy

[Ian Abbott]

If an asynchronous "read" command is no longer running but the subdevice
is still busy, it becomes non-busy once there is no more data available
in the buffer.  Some or all of the data written to the buffer might not
have been "munged" yet, and it cannot be read until it has been munged
by the writer.  However, since the command is no longer running, we
cannot expect any remaining unmunged data to get munged so we should
ignore it.  Call `comedi_buf_read_n_available()` to check the amount of
munged data available to be read, replacing the call to
`comedi_buf_n_bytes_ready()` which checked the amount of written (but
possibly not yet munged) data available to be read.  This affects both
the "read" file operation (done in `comedi_read()`) and the
`COMEDI_BUFINFO` ioctl handling (done in `do_bufinfo_ioctl()`).  (The
latter is used when data is transferred directly through the mmapped
buffer instead of via the "read" file operation.)

Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
Reviewed-by: H Hartley Sweeten <hsweeten@visionengravers.com>
---
 drivers/staging/comedi/comedi_fops.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
index f533113..89e8e87 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -1146,7 +1146,7 @@ static int do_bufinfo_ioctl(struct comedi_device *dev,
 		comedi_buf_read_free(s, bi.bytes_read);
 
 		if (comedi_is_subdevice_idle(s) &&
-		    comedi_buf_n_bytes_ready(s) == 0) {
+		    comedi_buf_read_n_available(s) == 0) {
 			do_become_nonbusy(dev, s);
 		}
 	}
@@ -2570,7 +2570,8 @@ static ssize_t comedi_read(struct file *file, char __user *buf, size_t nbytes,
 		new_s = comedi_file_read_subdevice(file);
 		if (dev->attached && old_detach_count == dev->detach_count &&
 		    s == new_s && new_s->async == async) {
-			if (become_nonbusy || comedi_buf_n_bytes_ready(s) == 0)
+			if (become_nonbusy ||
+			    comedi_buf_read_n_available(s) == 0)
 				do_become_nonbusy(dev, s);
 		}
 		mutex_unlock(&dev->mutex);
-- 
2.6.1

[PATCH 03/10] staging: comedi: do extra checks for becoming non-busy for "read"

[Ian Abbott]

`comedi_read()` is the handler for the "read" file operation for COMEDI
devices.  It mostly runs without using the main mutex of the COMEDI
device, but uses the `attach_lock` rwsemaphore to protect against the
COMEDI device becoming "detached".  A file object can read data
resulting from a COMEDI asynchonous command if it initiated the command.
The COMEDI subdevice is marked as busy when the command is started.  At
some point, the "read" handler detects that the command has terminated
and all available data has been read and so marks the subdevice as
non-busy.

In order to mark the subdevice as non-busy, the "read" handler needs to
release the `attach_lock` rwsemaphore and `acquire the main `mutex`.
There is a vulnerable point between the two, so it checks that the
device is still attached after acquiring the mutex.  However, it does
not currently check that the conditions for becoming non-busy still
hold.  Add some more checks that the subdevice is still busy with a
command initiated by the same file object, that command is in the correct
direction (in case the subdevice supports both "read" and "write"), that
command has terminated, and has no data available to be read.

Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
---
 drivers/staging/comedi/comedi_fops.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
index 89e8e87..cca3fb1 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -2566,14 +2566,17 @@ static ssize_t comedi_read(struct file *file, char __user *buf, size_t nbytes,
 		 * sufficient (unless there have been 2**32 detaches in the
 		 * meantime!), but check the subdevice pointer as well just in
 		 * case.
+		 *
+		 * Also check the subdevice is still in a suitable state to
+		 * become non-busy in case it changed behind our back.
 		 */
 		new_s = comedi_file_read_subdevice(file);
 		if (dev->attached && old_detach_count == dev->detach_count &&
-		    s == new_s && new_s->async == async) {
-			if (become_nonbusy ||
-			    comedi_buf_read_n_available(s) == 0)
-				do_become_nonbusy(dev, s);
-		}
+		    s == new_s && new_s->async == async && s->busy == file &&
+		    !(async->cmd.flags & CMDF_WRITE) &&
+		    !comedi_is_subdevice_running(s) &&
+		    comedi_buf_read_n_available(s) == 0)
+			do_become_nonbusy(dev, s);
 		mutex_unlock(&dev->mutex);
 	}
 out:
-- 
2.6.1

[PATCH 03/10] staging: comedi: do extra checks for becoming non-busy for "read"

[Ian Abbott]

`comedi_read()` is the handler for the "read" file operation for COMEDI
devices.  It mostly runs without using the main mutex of the COMEDI
device, but uses the `attach_lock` rwsemaphore to protect against the
COMEDI device becoming "detached".  A file object can read data
resulting from a COMEDI asynchonous command if it initiated the command.
The COMEDI subdevice is marked as busy when the command is started.  At
some point, the "read" handler detects that the command has terminated
and all available data has been read and so marks the subdevice as
non-busy.

In order to mark the subdevice as non-busy, the "read" handler needs to
release the `attach_lock` rwsemaphore and `acquire the main `mutex`.
There is a vulnerable point between the two, so it checks that the
device is still attached after acquiring the mutex.  However, it does
not currently check that the conditions for becoming non-busy still
hold.  Add some more checks that the subdevice is still busy with a
command initiated by the same file object, that command is in the correct
direction (in case the subdevice supports both "read" and "write"), that
command has terminated, and has no data available to be read.

Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
Reviewed-by: H Hartley Sweeten <hsweeten@visionengravers.com>
---
 drivers/staging/comedi/comedi_fops.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
index 89e8e87..cca3fb1 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -2566,14 +2566,17 @@ static ssize_t comedi_read(struct file *file, char __user *buf, size_t nbytes,
 		 * sufficient (unless there have been 2**32 detaches in the
 		 * meantime!), but check the subdevice pointer as well just in
 		 * case.
+		 *
+		 * Also check the subdevice is still in a suitable state to
+		 * become non-busy in case it changed behind our back.
 		 */
 		new_s = comedi_file_read_subdevice(file);
 		if (dev->attached && old_detach_count == dev->detach_count &&
-		    s == new_s && new_s->async == async) {
-			if (become_nonbusy ||
-			    comedi_buf_read_n_available(s) == 0)
-				do_become_nonbusy(dev, s);
-		}
+		    s == new_s && new_s->async == async && s->busy == file &&
+		    !(async->cmd.flags & CMDF_WRITE) &&
+		    !comedi_is_subdevice_running(s) &&
+		    comedi_buf_read_n_available(s) == 0)
+			do_become_nonbusy(dev, s);
 		mutex_unlock(&dev->mutex);
 	}
 out:
-- 
2.6.1

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 04/10] staging: comedi: make some variables unsigned in comedi_read()

[Ian Abbott]

In `comedi_read()`, the `n` and `m` variables are of type `int`.  Change
them to `unsigned int` as they are used to measure a positive number of
bytes.  The `count` variable is also of type `int` and holds the
returned number of bytes.  Change it to type `ssize_t` to match the
function's return type.

Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
---
 drivers/staging/comedi/comedi_fops.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
index cca3fb1..ae9d519 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -2449,7 +2449,9 @@ static ssize_t comedi_read(struct file *file, char __user *buf, size_t nbytes,
 {
 	struct comedi_subdevice *s;
 	struct comedi_async *async;
-	int n, m, count = 0, retval = 0;
+	unsigned int n, m;
+	ssize_t count = 0;
+	int retval = 0;
 	DECLARE_WAITQUEUE(wait, current);
 	struct comedi_file *cfp = file->private_data;
 	struct comedi_device *dev = cfp->dev;
-- 
2.6.1

[PATCH 04/10] staging: comedi: make some variables unsigned in comedi_read()

[Ian Abbott]

In `comedi_read()`, the `n` and `m` variables are of type `int`.  Change
them to `unsigned int` as they are used to measure a positive number of
bytes.  The `count` variable is also of type `int` and holds the
returned number of bytes.  Change it to type `ssize_t` to match the
function's return type.

Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
Reviewed-by: H Hartley Sweeten <hsweeten@visionengravers.com>
---
 drivers/staging/comedi/comedi_fops.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
index cca3fb1..ae9d519 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -2449,7 +2449,9 @@ static ssize_t comedi_read(struct file *file, char __user *buf, size_t nbytes,
 {
 	struct comedi_subdevice *s;
 	struct comedi_async *async;
-	int n, m, count = 0, retval = 0;
+	unsigned int n, m;
+	ssize_t count = 0;
+	int retval = 0;
 	DECLARE_WAITQUEUE(wait, current);
 	struct comedi_file *cfp = file->private_data;
 	struct comedi_device *dev = cfp->dev;
-- 
2.6.1

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 05/10] staging: comedi: avoid bad truncation of a size_t in comedi_read()

[Ian Abbott]

At one point in `comedi_read()`, the variable `n` gets assigned to the
minimum of the parameter `nbytes` and the amount of readable buffer
space `m`.  The way that is done currently is unsafe in the unlikely
case that `nbytes` exceeds `UINT_MAX`, so fix it.

Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
---
 drivers/staging/comedi/comedi_fops.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
index ae9d519..d51c94c 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -2492,13 +2492,10 @@ static ssize_t comedi_read(struct file *file, char __user *buf, size_t nbytes,
 	while (nbytes > 0 && !retval) {
 		set_current_state(TASK_INTERRUPTIBLE);
 
-		n = nbytes;
-
 		m = comedi_buf_read_n_available(s);
 		if (async->buf_read_ptr + m > async->prealloc_bufsz)
 			m = async->prealloc_bufsz - async->buf_read_ptr;
-		if (m < n)
-			n = m;
+		n = min_t(size_t, m, nbytes);
 
 		if (n == 0) {
 			unsigned runflags = comedi_get_subdevice_runflags(s);
-- 
2.6.1

[PATCH 05/10] staging: comedi: avoid bad truncation of a size_t in comedi_read()

[Ian Abbott]

At one point in `comedi_read()`, the variable `n` gets assigned to the
minimum of the parameter `nbytes` and the amount of readable buffer
space `m`.  The way that is done currently is unsafe in the unlikely
case that `nbytes` exceeds `UINT_MAX`, so fix it.

Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
Reviewed-by: H Hartley Sweeten <hsweeten@visionengravers.com>
---
 drivers/staging/comedi/comedi_fops.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
index ae9d519..d51c94c 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -2492,13 +2492,10 @@ static ssize_t comedi_read(struct file *file, char __user *buf, size_t nbytes,
 	while (nbytes > 0 && !retval) {
 		set_current_state(TASK_INTERRUPTIBLE);
 
-		n = nbytes;
-
 		m = comedi_buf_read_n_available(s);
 		if (async->buf_read_ptr + m > async->prealloc_bufsz)
 			m = async->prealloc_bufsz - async->buf_read_ptr;
-		if (m < n)
-			n = m;
+		n = min_t(size_t, m, nbytes);
 
 		if (n == 0) {
 			unsigned runflags = comedi_get_subdevice_runflags(s);
-- 
2.6.1

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 06/10] staging: comedi: allow buffer wraparound in comedi_read()

[Ian Abbott]

`comedi_read()` copies data from the acquisition data buffer, which is
cyclic, to the user buffer using a single call to `copy_to_user()`.  It
currently avoids having to deal with wraparound of the cyclic buffer by
limiting the amount it copies (and the amount returned to the user).
Change it to deal with the wraparound using two calls to
`copy_to_user()` if necessary.

Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
---
 drivers/staging/comedi/comedi_fops.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
index d51c94c..b534b49 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -2490,11 +2490,11 @@ static ssize_t comedi_read(struct file *file, char __user *buf, size_t nbytes,
 
 	add_wait_queue(&async->wait_head, &wait);
 	while (nbytes > 0 && !retval) {
+		unsigned int rp, n1, n2;
+
 		set_current_state(TASK_INTERRUPTIBLE);
 
 		m = comedi_buf_read_n_available(s);
-		if (async->buf_read_ptr + m > async->prealloc_bufsz)
-			m = async->prealloc_bufsz - async->buf_read_ptr;
 		n = min_t(size_t, m, nbytes);
 
 		if (n == 0) {
@@ -2531,8 +2531,14 @@ static ssize_t comedi_read(struct file *file, char __user *buf, size_t nbytes,
 			}
 			continue;
 		}
-		m = copy_to_user(buf, async->prealloc_buf +
-				 async->buf_read_ptr, n);
+		rp = async->buf_read_ptr;
+		n1 = min(n, async->prealloc_bufsz - rp);
+		n2 = n - n1;
+		m = copy_to_user(buf, async->prealloc_buf + rp, n1);
+		if (m)
+			m += n2;
+		else if (n2)
+			m = copy_to_user(buf + n1, async->prealloc_buf, n2);
 		if (m) {
 			n -= m;
 			retval = -EFAULT;
-- 
2.6.1

[PATCH 06/10] staging: comedi: allow buffer wraparound in comedi_read()

[Ian Abbott]

`comedi_read()` copies data from the acquisition data buffer, which is
cyclic, to the user buffer using a single call to `copy_to_user()`.  It
currently avoids having to deal with wraparound of the cyclic buffer by
limiting the amount it copies (and the amount returned to the user).
Change it to deal with the wraparound using two calls to
`copy_to_user()` if necessary.

Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
Reviewed-by: H Hartley Sweeten <hsweeten@visionengravers.com>
---
 drivers/staging/comedi/comedi_fops.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
index d51c94c..b534b49 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -2490,11 +2490,11 @@ static ssize_t comedi_read(struct file *file, char __user *buf, size_t nbytes,
 
 	add_wait_queue(&async->wait_head, &wait);
 	while (nbytes > 0 && !retval) {
+		unsigned int rp, n1, n2;
+
 		set_current_state(TASK_INTERRUPTIBLE);
 
 		m = comedi_buf_read_n_available(s);
-		if (async->buf_read_ptr + m > async->prealloc_bufsz)
-			m = async->prealloc_bufsz - async->buf_read_ptr;
 		n = min_t(size_t, m, nbytes);
 
 		if (n == 0) {
@@ -2531,8 +2531,14 @@ static ssize_t comedi_read(struct file *file, char __user *buf, size_t nbytes,
 			}
 			continue;
 		}
-		m = copy_to_user(buf, async->prealloc_buf +
-				 async->buf_read_ptr, n);
+		rp = async->buf_read_ptr;
+		n1 = min(n, async->prealloc_bufsz - rp);
+		n2 = n - n1;
+		m = copy_to_user(buf, async->prealloc_buf + rp, n1);
+		if (m)
+			m += n2;
+		else if (n2)
+			m = copy_to_user(buf + n1, async->prealloc_buf, n2);
 		if (m) {
 			n -= m;
 			retval = -EFAULT;
-- 
2.6.1

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 07/10] staging: comedi: remove superfluous retval = 0 in comedi_read()

[Ian Abbott]

`comedi_read()` initializes `retval` to 0.  The other `retval = 0`
assignments are superfluous, so remove them.

Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
---
 drivers/staging/comedi/comedi_fops.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
index b534b49..9505a34 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -2503,8 +2503,6 @@ static ssize_t comedi_read(struct file *file, char __user *buf, size_t nbytes,
 			if (!comedi_is_runflags_running(runflags)) {
 				if (comedi_is_runflags_in_error(runflags))
 					retval = -EPIPE;
-				else
-					retval = 0;
 				become_nonbusy = true;
 				break;
 			}
@@ -2518,7 +2516,6 @@ static ssize_t comedi_read(struct file *file, char __user *buf, size_t nbytes,
 				break;
 			}
 			if (!s->busy) {
-				retval = 0;
 				break;
 			}
 			if (s->busy != file) {
-- 
2.6.1

[PATCH 07/10] staging: comedi: remove superfluous retval = 0 in comedi_read()

[Ian Abbott]

`comedi_read()` initializes `retval` to 0.  The other `retval = 0`
assignments are superfluous, so remove them.

Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
Reviewed-by: H Hartley Sweeten <hsweeten@visionengravers.com>
---
 drivers/staging/comedi/comedi_fops.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
index b534b49..9505a34 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -2503,8 +2503,6 @@ static ssize_t comedi_read(struct file *file, char __user *buf, size_t nbytes,
 			if (!comedi_is_runflags_running(runflags)) {
 				if (comedi_is_runflags_in_error(runflags))
 					retval = -EPIPE;
-				else
-					retval = 0;
 				become_nonbusy = true;
 				break;
 			}
@@ -2518,7 +2516,6 @@ static ssize_t comedi_read(struct file *file, char __user *buf, size_t nbytes,
 				break;
 			}
 			if (!s->busy) {
-				retval = 0;
 				break;
 			}
 			if (s->busy != file) {
-- 
2.6.1

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 08/10] staging: comedi: return error on "read" if no command set up

[Ian Abbott]

The "read" file operation handler, `comedi_read()` returns an error for
pretty much any condition that prevents a "read" going ahead.  One of
the conditions that prevents a "read" going ahead is that no
asynchronous command has been set up, but that currently results in a
return value of 0 (unless COMEDI instructions are being processed or an
asynchronous command has been set up by a different file object).
Change it to return `-EINVAL` in this case.

Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
---
 drivers/staging/comedi/comedi_fops.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
index 9505a34..db88fa5 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -2477,8 +2477,12 @@ static ssize_t comedi_read(struct file *file, char __user *buf, size_t nbytes,
 	}
 
 	async = s->async;
-	if (!s->busy || !nbytes)
+	if (!nbytes)
+		goto out;
+	if (!s->busy) {
+		retval = -EINVAL;
 		goto out;
+	}
 	if (s->busy != file) {
 		retval = -EACCES;
 		goto out;
@@ -2516,6 +2520,7 @@ static ssize_t comedi_read(struct file *file, char __user *buf, size_t nbytes,
 				break;
 			}
 			if (!s->busy) {
+				retval = -EINVAL;
 				break;
 			}
 			if (s->busy != file) {
-- 
2.6.1

[PATCH 08/10] staging: comedi: return error on "read" if no command set up

[Ian Abbott]

The "read" file operation handler, `comedi_read()` returns an error for
pretty much any condition that prevents a "read" going ahead.  One of
the conditions that prevents a "read" going ahead is that no
asynchronous command has been set up, but that currently results in a
return value of 0 (unless COMEDI instructions are being processed or an
asynchronous command has been set up by a different file object).
Change it to return `-EINVAL` in this case.

Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
Reviewed-by: H Hartley Sweeten <hsweeten@visionengravers.com>
---
 drivers/staging/comedi/comedi_fops.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
index 9505a34..db88fa5 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -2477,8 +2477,12 @@ static ssize_t comedi_read(struct file *file, char __user *buf, size_t nbytes,
 	}
 
 	async = s->async;
-	if (!s->busy || !nbytes)
+	if (!nbytes)
+		goto out;
+	if (!s->busy) {
+		retval = -EINVAL;
 		goto out;
+	}
 	if (s->busy != file) {
 		retval = -EACCES;
 		goto out;
@@ -2516,6 +2520,7 @@ static ssize_t comedi_read(struct file *file, char __user *buf, size_t nbytes,
 				break;
 			}
 			if (!s->busy) {
+				retval = -EINVAL;
 				break;
 			}
 			if (s->busy != file) {
-- 
2.6.1

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 09/10] staging: comedi: simplify returned errors for comedi_read()

[Ian Abbott]

In order to perform a "read" file operation, an asynchronous COMEDI
command in the "read" direction needs to have been set up by the current
file object on the COMEDI "read" subdevice associated with the file
object.  If there is a "read" subdevice, but a command has not been set
up by the file object (or is has been set-up in the wrong direction),
`comedi_read()` currently returns one of two error values `-EINVAL` or
`-EACCES`.  `-EACCES` is returned if the command was set up by a
different subdevice, or somewhat randomly, if a COMEDI "instruction" is
currently being processed.  `-EINVAL` is returned in other cases.
Simplify it by returning `-EINVAL` for all these cases.

Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
---
 drivers/staging/comedi/comedi_fops.c | 21 +++------------------
 1 file changed, 3 insertions(+), 18 deletions(-)

diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
index db88fa5..6e8806b4 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -2479,15 +2479,7 @@ static ssize_t comedi_read(struct file *file, char __user *buf, size_t nbytes,
 	async = s->async;
 	if (!nbytes)
 		goto out;
-	if (!s->busy) {
-		retval = -EINVAL;
-		goto out;
-	}
-	if (s->busy != file) {
-		retval = -EACCES;
-		goto out;
-	}
-	if (async->cmd.flags & CMDF_WRITE) {
+	if (s->busy != file || (async->cmd.flags & CMDF_WRITE)) {
 		retval = -EINVAL;
 		goto out;
 	}
@@ -2519,15 +2511,8 @@ static ssize_t comedi_read(struct file *file, char __user *buf, size_t nbytes,
 				retval = -ERESTARTSYS;
 				break;
 			}
-			if (!s->busy) {
-				retval = -EINVAL;
-				break;
-			}
-			if (s->busy != file) {
-				retval = -EACCES;
-				break;
-			}
-			if (async->cmd.flags & CMDF_WRITE) {
+			if (s->busy != file ||
+			    (async->cmd.flags & CMDF_WRITE)) {
 				retval = -EINVAL;
 				break;
 			}
-- 
2.6.1

[PATCH 09/10] staging: comedi: simplify returned errors for comedi_read()

[Ian Abbott]

In order to perform a "read" file operation, an asynchronous COMEDI
command in the "read" direction needs to have been set up by the current
file object on the COMEDI "read" subdevice associated with the file
object.  If there is a "read" subdevice, but a command has not been set
up by the file object (or is has been set-up in the wrong direction),
`comedi_read()` currently returns one of two error values `-EINVAL` or
`-EACCES`.  `-EACCES` is returned if the command was set up by a
different subdevice, or somewhat randomly, if a COMEDI "instruction" is
currently being processed.  `-EINVAL` is returned in other cases.
Simplify it by returning `-EINVAL` for all these cases.

Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
Reviewed-by: H Hartley Sweeten <hsweeten@visionengravers.com>
---
 drivers/staging/comedi/comedi_fops.c | 21 +++------------------
 1 file changed, 3 insertions(+), 18 deletions(-)

diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
index db88fa5..6e8806b4 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -2479,15 +2479,7 @@ static ssize_t comedi_read(struct file *file, char __user *buf, size_t nbytes,
 	async = s->async;
 	if (!nbytes)
 		goto out;
-	if (!s->busy) {
-		retval = -EINVAL;
-		goto out;
-	}
-	if (s->busy != file) {
-		retval = -EACCES;
-		goto out;
-	}
-	if (async->cmd.flags & CMDF_WRITE) {
+	if (s->busy != file || (async->cmd.flags & CMDF_WRITE)) {
 		retval = -EINVAL;
 		goto out;
 	}
@@ -2519,15 +2511,8 @@ static ssize_t comedi_read(struct file *file, char __user *buf, size_t nbytes,
 				retval = -ERESTARTSYS;
 				break;
 			}
-			if (!s->busy) {
-				retval = -EINVAL;
-				break;
-			}
-			if (s->busy != file) {
-				retval = -EACCES;
-				break;
-			}
-			if (async->cmd.flags & CMDF_WRITE) {
+			if (s->busy != file ||
+			    (async->cmd.flags & CMDF_WRITE)) {
 				retval = -EINVAL;
 				break;
 			}
-- 
2.6.1

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 10/10] staging: comedi: check for more errors for zero-length read

[Ian Abbott]

If the "read" file operation handler, `comedi_read()` is passed 0 for
the amount to read, some error conditions are currently skipped and the
function just returns 0.  Change it to check those error conditions and
return an error value if appropriate.  The trickiest case is the check
for when the previously set up asynchronous command has terminated with
an error.  In that case, `-EPIPE` is returned (as it is for a read of
non-zero length) and the subdevice gets marked as non-busy.

A zero-length read that returns 0 has no other effects, in particular,
it does not cause the subdevice to be marked as non-busy, and the return
value does not indicate an "end-of-file" condition.

Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
---
 drivers/staging/comedi/comedi_fops.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
index 6e8806b4..9fe5f7b 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -2477,15 +2477,13 @@ static ssize_t comedi_read(struct file *file, char __user *buf, size_t nbytes,
 	}
 
 	async = s->async;
-	if (!nbytes)
-		goto out;
 	if (s->busy != file || (async->cmd.flags & CMDF_WRITE)) {
 		retval = -EINVAL;
 		goto out;
 	}
 
 	add_wait_queue(&async->wait_head, &wait);
-	while (nbytes > 0 && !retval) {
+	while (count == 0 && !retval) {
 		unsigned int rp, n1, n2;
 
 		set_current_state(TASK_INTERRUPTIBLE);
@@ -2499,9 +2497,12 @@ static ssize_t comedi_read(struct file *file, char __user *buf, size_t nbytes,
 			if (!comedi_is_runflags_running(runflags)) {
 				if (comedi_is_runflags_in_error(runflags))
 					retval = -EPIPE;
-				become_nonbusy = true;
+				if (retval || nbytes)
+					become_nonbusy = true;
 				break;
 			}
+			if (nbytes == 0)
+				break;
 			if (file->f_flags & O_NONBLOCK) {
 				retval = -EAGAIN;
 				break;
@@ -2538,7 +2539,6 @@ static ssize_t comedi_read(struct file *file, char __user *buf, size_t nbytes,
 		nbytes -= n;
 
 		buf += n;
-		break;		/* makes device work like a pipe */
 	}
 	remove_wait_queue(&async->wait_head, &wait);
 	set_current_state(TASK_RUNNING);
-- 
2.6.1

[PATCH 10/10] staging: comedi: check for more errors for zero-length read

[Ian Abbott]

If the "read" file operation handler, `comedi_read()` is passed 0 for
the amount to read, some error conditions are currently skipped and the
function just returns 0.  Change it to check those error conditions and
return an error value if appropriate.  The trickiest case is the check
for when the previously set up asynchronous command has terminated with
an error.  In that case, `-EPIPE` is returned (as it is for a read of
non-zero length) and the subdevice gets marked as non-busy.

A zero-length read that returns 0 has no other effects, in particular,
it does not cause the subdevice to be marked as non-busy, and the return
value does not indicate an "end-of-file" condition.

Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
Reviewed-by: H Hartley Sweeten <hsweeten@visionengravers.com>
---
 drivers/staging/comedi/comedi_fops.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
index 6e8806b4..9fe5f7b 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -2477,15 +2477,13 @@ static ssize_t comedi_read(struct file *file, char __user *buf, size_t nbytes,
 	}
 
 	async = s->async;
-	if (!nbytes)
-		goto out;
 	if (s->busy != file || (async->cmd.flags & CMDF_WRITE)) {
 		retval = -EINVAL;
 		goto out;
 	}
 
 	add_wait_queue(&async->wait_head, &wait);
-	while (nbytes > 0 && !retval) {
+	while (count == 0 && !retval) {
 		unsigned int rp, n1, n2;
 
 		set_current_state(TASK_INTERRUPTIBLE);
@@ -2499,9 +2497,12 @@ static ssize_t comedi_read(struct file *file, char __user *buf, size_t nbytes,
 			if (!comedi_is_runflags_running(runflags)) {
 				if (comedi_is_runflags_in_error(runflags))
 					retval = -EPIPE;
-				become_nonbusy = true;
+				if (retval || nbytes)
+					become_nonbusy = true;
 				break;
 			}
+			if (nbytes == 0)
+				break;
 			if (file->f_flags & O_NONBLOCK) {
 				retval = -EAGAIN;
 				break;
@@ -2538,7 +2539,6 @@ static ssize_t comedi_read(struct file *file, char __user *buf, size_t nbytes,
 		nbytes -= n;
 
 		buf += n;
-		break;		/* makes device work like a pipe */
 	}
 	remove_wait_queue(&async->wait_head, &wait);
 	set_current_state(TASK_RUNNING);
-- 
2.6.1

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH 00/10] staging: comedi: some comedi_read() changes

[Hartley Sweeten]

On Monday, October 12, 2015 9:21 AM, Ian Abbott wrote:
> Tidy up the "read" file operation handler, `comedi_read()` a bit and
> improve the error handling and the "end-of-file" handling.
>
> There are some other changes I want to make, such as switching to the
> newer wait API (prepare_to_wait()/finish_wait()) and preventing several
> tasks trying to read or write the same subdevice at the same time (but
> without using the COMEDI device's main mutex as it is too coarse).
> Those changes can wait until after I've cleaned up and improved the
> "write" file operation handler a bit.
>
> 01) staging: comedi: remain busy until read end-of-file
> 02) staging: comedi: don't consider "unmunged" data when becoming
>     non-busy
> 03) staging: comedi: do extra checks for becoming non-busy for "read"
> 04) staging: comedi: make some variables unsigned in comedi_read()
> 05) staging: comedi: avoid bad truncation of a size_t in comedi_read()
> 06) staging: comedi: allow buffer wraparound in comedi_read()
> 07) staging: comedi: remove superfluous retval = 0 in comedi_read()
> 08) staging: comedi: return error on "read" if no command set up
> 09) staging: comedi: simplify returned errors for comedi_read()
> 10) staging: comedi: check for more errors for zero-length read
>
>  drivers/staging/comedi/comedi_fops.c | 68 +++++++++++++++++-------------------
>  1 file changed, 32 insertions(+), 36 deletions(-)

Thanks!

Reviewed-by: H Hartley Sweeten <hsweeten@visionengravers.com>

RE: [PATCH 00/10] staging: comedi: some comedi_read() changes

[Hartley Sweeten]

On Monday, October 12, 2015 9:21 AM, Ian Abbott wrote:
> Tidy up the "read" file operation handler, `comedi_read()` a bit and
> improve the error handling and the "end-of-file" handling.
>
> There are some other changes I want to make, such as switching to the
> newer wait API (prepare_to_wait()/finish_wait()) and preventing several
> tasks trying to read or write the same subdevice at the same time (but
> without using the COMEDI device's main mutex as it is too coarse).
> Those changes can wait until after I've cleaned up and improved the
> "write" file operation handler a bit.
>
> 01) staging: comedi: remain busy until read end-of-file
> 02) staging: comedi: don't consider "unmunged" data when becoming
>     non-busy
> 03) staging: comedi: do extra checks for becoming non-busy for "read"
> 04) staging: comedi: make some variables unsigned in comedi_read()
> 05) staging: comedi: avoid bad truncation of a size_t in comedi_read()
> 06) staging: comedi: allow buffer wraparound in comedi_read()
> 07) staging: comedi: remove superfluous retval = 0 in comedi_read()
> 08) staging: comedi: return error on "read" if no command set up
> 09) staging: comedi: simplify returned errors for comedi_read()
> 10) staging: comedi: check for more errors for zero-length read
>
>  drivers/staging/comedi/comedi_fops.c | 68 +++++++++++++++++-------------------
>  1 file changed, 32 insertions(+), 36 deletions(-)

Thanks!

Reviewed-by: H Hartley Sweeten <hsweeten@visionengravers.com>