Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1244882 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH 00/10] staging: comedi: some comedi_read() changes Ian Abbott <abbotti@mev.co.uk> - 2015-10-12 18:30 +0200
    [PATCH 01/10] staging: comedi: remain busy until read end-of-file Ian Abbott <abbotti@mev.co.uk> - 2015-10-12 18:30 +0200
    [PATCH 07/10] staging: comedi: remove superfluous retval = 0 in comedi_read() Ian Abbott <abbotti@mev.co.uk> - 2015-10-12 18:30 +0200
    [PATCH 02/10] staging: comedi: don't consider "unmunged" data when becoming non-busy Ian Abbott <abbotti@mev.co.uk> - 2015-10-12 18:30 +0200
    [PATCH 04/10] staging: comedi: make some variables unsigned in comedi_read() Ian Abbott <abbotti@mev.co.uk> - 2015-10-12 18:30 +0200
    [PATCH 03/10] staging: comedi: do extra checks for becoming non-busy for "read" Ian Abbott <abbotti@mev.co.uk> - 2015-10-12 18:30 +0200
    RE: [PATCH 00/10] staging: comedi: some comedi_read() changes Hartley Sweeten <HartleyS@visionengravers.com> - 2015-10-12 19:10 +0200

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

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(-)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

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

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

`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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1244887 — [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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

`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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1244927

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>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web