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

Groups > linux.kernel > #1272416 > unrolled thread

[PATCH 0/8] staging: comedi: some comedi_write() changes

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/8] staging: comedi: some comedi_write() changes Ian Abbott <abbotti@mev.co.uk> - 2015-11-18 19:00 +0100
    [PATCH 1/8] staging: comedi: rearrange comedi_write() code Ian Abbott <abbotti@mev.co.uk> - 2015-11-18 19:00 +0100
      RE: [PATCH 1/8] staging: comedi: rearrange comedi_write() code Hartley Sweeten <HartleyS@visionengravers.com> - 2015-11-19 18:10 +0100
    [PATCH 5/8] staging: comedi: allow buffer wraparound in comedi_write() Ian Abbott <abbotti@mev.co.uk> - 2015-11-18 19:00 +0100
    [PATCH 8/8] staging: comedi: check for more errors for zero-length write Ian Abbott <abbotti@mev.co.uk> - 2015-11-18 19:00 +0100
    [PATCH 4/8] staging: comedi: avoid bad truncation of a size_t in comedi_write() Ian Abbott <abbotti@mev.co.uk> - 2015-11-18 19:00 +0100
    [PATCH 6/8] staging: comedi: return error on "write" if no command set up Ian Abbott <abbotti@mev.co.uk> - 2015-11-18 19:00 +0100
    [PATCH 7/8] staging: comedi: simplify returned errors for comedi_write() Ian Abbott <abbotti@mev.co.uk> - 2015-11-18 19:00 +0100
    RE: [PATCH 0/8] staging: comedi: some comedi_write() changes Hartley Sweeten <HartleyS@visionengravers.com> - 2015-11-19 18:40 +0100

#1272416 — [PATCH 0/8] staging: comedi: some comedi_write() changes

Tidy up the "write" file operation handler, `comedi_write()` a bit and
improve the error handling.

1) staging: comedi: rearrange comedi_write() code
2) staging: comedi: do extra checks for becoming non-busy for "write"
3) staging: comedi: make some variables unsigned in comedi_write()
4) staging: comedi: avoid bad truncation of a size_t in comedi_write()
5) staging: comedi: allow buffer wraparound in comedi_write()
6) staging: comedi: return error on "write" if no command set up
7) staging: comedi: simplify returned errors for comedi_write()
8) staging: comedi: check for more errors for zero-length write

 drivers/staging/comedi/comedi_fops.c | 124 ++++++++++++++++-------------------
 1 file changed, 56 insertions(+), 68 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]

#1272417 — [PATCH 1/8] staging: comedi: rearrange comedi_write() code

Rearrange the code in `comedi_write()` to reduce the amount of
indentation.  The code never reiterates the `while` loop once `count`
has become non-zero, so we can check that in the `while` condition to
save an indentation level.  (Note that `nbytes` has been checked to be
non-zero before entering the loop, so we can remove that check.)  Move
the code that makes the subdevice "become non-busy" outside the `while`
loop, using a new flag variable `become_nonbusy` to decide whether it
needs to be done.  This simplifies the wait queue handling so there is a
single place where the task is removed from the wait queue, and we can
remove the `on_wait_queue` flag variable.

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

diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
index 7b4af51..c9da6f3 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -2307,7 +2307,7 @@ static ssize_t comedi_write(struct file *file, const char __user *buf,
 	DECLARE_WAITQUEUE(wait, current);
 	struct comedi_file *cfp = file->private_data;
 	struct comedi_device *dev = cfp->dev;
-	bool on_wait_queue = false;
+	bool become_nonbusy = false;
 	bool attach_locked;
 	unsigned int old_detach_count;
 
@@ -2342,48 +2342,16 @@ static ssize_t comedi_write(struct file *file, const char __user *buf,
 	}
 
 	add_wait_queue(&async->wait_head, &wait);
-	on_wait_queue = true;
-	while (nbytes > 0 && !retval) {
+	while (count == 0 && !retval) {
 		unsigned runflags;
 
 		set_current_state(TASK_INTERRUPTIBLE);
 
 		runflags = comedi_get_subdevice_runflags(s);
 		if (!comedi_is_runflags_running(runflags)) {
-			if (count == 0) {
-				struct comedi_subdevice *new_s;
-
-				if (comedi_is_runflags_in_error(runflags))
-					retval = -EPIPE;
-				else
-					retval = 0;
-				/*
-				 * To avoid deadlock, cannot acquire dev->mutex
-				 * while dev->attach_lock is held.  Need to
-				 * remove task from the async wait queue before
-				 * releasing dev->attach_lock, as it might not
-				 * be valid afterwards.
-				 */
-				remove_wait_queue(&async->wait_head, &wait);
-				on_wait_queue = false;
-				up_read(&dev->attach_lock);
-				attach_locked = false;
-				mutex_lock(&dev->mutex);
-				/*
-				 * Become non-busy unless things have changed
-				 * behind our back.  Checking dev->detach_count
-				 * is unchanged ought to be sufficient (unless
-				 * there have been 2**32 detaches in the
-				 * meantime!), but check the subdevice pointer
-				 * as well just in case.
-				 */
-				new_s = comedi_file_write_subdevice(file);
-				if (dev->attached &&
-				    old_detach_count == dev->detach_count &&
-				    s == new_s && new_s->async == async)
-					do_become_nonbusy(dev, s);
-				mutex_unlock(&dev->mutex);
-			}
+			if (comedi_is_runflags_in_error(runflags))
+				retval = -EPIPE;
+			become_nonbusy = true;
 			break;
 		}
 
@@ -2433,12 +2401,33 @@ static ssize_t comedi_write(struct file *file, const char __user *buf,
 		nbytes -= n;
 
 		buf += n;
-		break;		/* makes device work like a pipe */
 	}
-out:
-	if (on_wait_queue)
-		remove_wait_queue(&async->wait_head, &wait);
+	remove_wait_queue(&async->wait_head, &wait);
 	set_current_state(TASK_RUNNING);
+	if (become_nonbusy && count == 0) {
+		struct comedi_subdevice *new_s;
+
+		/*
+		 * To avoid deadlock, cannot acquire dev->mutex
+		 * while dev->attach_lock is held.
+		 */
+		up_read(&dev->attach_lock);
+		attach_locked = false;
+		mutex_lock(&dev->mutex);
+		/*
+		 * Check device hasn't become detached behind our back.
+		 * Checking dev->detach_count is unchanged ought to be
+		 * sufficient (unless there have been 2**32 detaches in the
+		 * meantime!), but check the subdevice pointer as well just in
+		 * case.
+		 */
+		new_s = comedi_file_write_subdevice(file);
+		if (dev->attached && old_detach_count == dev->detach_count &&
+		    s == new_s && new_s->async == async)
+			do_become_nonbusy(dev, s);
+		mutex_unlock(&dev->mutex);
+	}
+out:
 	if (attach_locked)
 		up_read(&dev->attach_lock);
 
-- 
2.6.2

--
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]

#1273291 — RE: [PATCH 1/8] staging: comedi: rearrange comedi_write() code

On Wednesday, November 18, 2015 10:55 AM, Ian Abbott wrote:
> Rearrange the code in `comedi_write()` to reduce the amount of
> indentation.  The code never reiterates the `while` loop once `count`
> has become non-zero, so we can check that in the `while` condition to
> save an indentation level.  (Note that `nbytes` has been checked to be
> non-zero before entering the loop, so we can remove that check.)  Move
> the code that makes the subdevice "become non-busy" outside the `while`
> loop, using a new flag variable `become_nonbusy` to decide whether it
> needs to be done.  This simplifies the wait queue handling so there is a
> single place where the task is removed from the wait queue, and we can
> remove the `on_wait_queue` flag variable.
>
> Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
> ---
>  drivers/staging/comedi/comedi_fops.c | 71 +++++++++++++++---------------------
>  1 file changed, 30 insertions(+), 41 deletions(-)

Ian,

Minor nit-pick...

[snip]

> -out:
> -	if (on_wait_queue)
> -		remove_wait_queue(&async->wait_head, &wait);
> +	remove_wait_queue(&async->wait_head, &wait);
>  	set_current_state(TASK_RUNNING);
> +	if (become_nonbusy && count == 0) {

It looks like 'count' will always be 0 here.

Regards
Hartley

--
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]

#1272419 — [PATCH 5/8] staging: comedi: allow buffer wraparound in comedi_write()

`comedi_write()` copies data from the user buffer to the acquisition
data buffer, which is cyclic, using a single call to `copy_from_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_from_user()` if necessary.

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

diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
index 8c784c4..4dd4289 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -2346,6 +2346,7 @@ static ssize_t comedi_write(struct file *file, const char __user *buf,
 	add_wait_queue(&async->wait_head, &wait);
 	while (count == 0 && !retval) {
 		unsigned runflags;
+		unsigned int wp, n1, n2;
 
 		set_current_state(TASK_INTERRUPTIBLE);
 
@@ -2360,9 +2361,6 @@ static ssize_t comedi_write(struct file *file, const char __user *buf,
 		/* Allocate all free buffer space. */
 		comedi_buf_write_alloc(s, async->prealloc_bufsz);
 		m = comedi_buf_write_n_allocated(s);
-		/* Avoid buffer wraparound. */
-		if (async->buf_write_ptr + m > async->prealloc_bufsz)
-			m = async->prealloc_bufsz - async->buf_write_ptr;
 		n = min_t(size_t, m, nbytes);
 
 		if (n == 0) {
@@ -2388,8 +2386,14 @@ static ssize_t comedi_write(struct file *file, const char __user *buf,
 			continue;
 		}
 
-		m = copy_from_user(async->prealloc_buf + async->buf_write_ptr,
-				   buf, n);
+		wp = async->buf_write_ptr;
+		n1 = min(n, async->prealloc_bufsz - wp);
+		n2 = n - n1;
+		m = copy_from_user(async->prealloc_buf + wp, buf, n1);
+		if (m)
+			m += n2;
+		else if (n2)
+			m = copy_from_user(async->prealloc_buf, buf + n1, n2);
 		if (m) {
 			n -= m;
 			retval = -EFAULT;
-- 
2.6.2

--
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]

#1272420 — [PATCH 8/8] staging: comedi: check for more errors for zero-length write

If the "write" file operation handler, `comedi_write()` is passed 0 for
the amount to write, 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 write of
non-zero length) and the subdevice gets marked as non-busy.

A zero-length write that returns 0 has no other effects, in particular,
it does not cause the subdevice to be marked as non-busy.

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

diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
index 5a9c9d9..d57fade 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -2331,8 +2331,6 @@ static ssize_t comedi_write(struct file *file, const char __user *buf,
 	}
 
 	async = s->async;
-	if (!nbytes)
-		goto out;
 	if (s->busy != file || !(async->cmd.flags & CMDF_WRITE)) {
 		retval = -EINVAL;
 		goto out;
@@ -2349,9 +2347,12 @@ static ssize_t comedi_write(struct file *file, const char __user *buf,
 		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;
 
 		/* Allocate all free buffer space. */
 		comedi_buf_write_alloc(s, async->prealloc_bufsz);
-- 
2.6.2

--
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]

#1272422 — [PATCH 4/8] staging: comedi: avoid bad truncation of a size_t in comedi_write()

At one point in `comedi_write()`, the variable `n` gets assigned to the
minimum of the parameter `nbytes` and the amount of writeable buffer
space.  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 | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
index 188a12a..8c784c4 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -2357,16 +2357,13 @@ static ssize_t comedi_write(struct file *file, const char __user *buf,
 			break;
 		}
 
-		n = nbytes;
-
-		m = n;
+		/* Allocate all free buffer space. */
+		comedi_buf_write_alloc(s, async->prealloc_bufsz);
+		m = comedi_buf_write_n_allocated(s);
+		/* Avoid buffer wraparound. */
 		if (async->buf_write_ptr + m > async->prealloc_bufsz)
 			m = async->prealloc_bufsz - async->buf_write_ptr;
-		comedi_buf_write_alloc(s, async->prealloc_bufsz);
-		if (m > comedi_buf_write_n_allocated(s))
-			m = comedi_buf_write_n_allocated(s);
-		if (m < n)
-			n = m;
+		n = min_t(size_t, m, nbytes);
 
 		if (n == 0) {
 			if (file->f_flags & O_NONBLOCK) {
-- 
2.6.2

--
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]

#1272424 — [PATCH 6/8] staging: comedi: return error on "write" if no command set up

The "write" file operation handler, `comedi_write()` returns an error
for pretty much any condition that prevents a "write" going ahead.  One
of the conditions that prevents a "write" 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 | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
index 4dd4289..2a2b5a0 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -2331,9 +2331,12 @@ static ssize_t comedi_write(struct file *file, const char __user *buf,
 	}
 
 	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;
@@ -2373,8 +2376,10 @@ static ssize_t comedi_write(struct file *file, const char __user *buf,
 				retval = -ERESTARTSYS;
 				break;
 			}
-			if (!s->busy)
+			if (!s->busy) {
+				retval = -EINVAL;
 				break;
+			}
 			if (s->busy != file) {
 				retval = -EACCES;
 				break;
-- 
2.6.2

--
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]

#1272426 — [PATCH 7/8] staging: comedi: simplify returned errors for comedi_write()

In order to perform a "write" file operation, an asynchronous COMEDI
command in the "write" direction needs to have been set up by the
current file object on the COMEDI "write" subdevice associated with the
file object.  If there is a "write" subdevice, but a command has not
been set up by the file object (or is has been set-up in the wrong
direction), `comedi_write()` 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 2a2b5a0..5a9c9d9 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -2333,15 +2333,7 @@ static ssize_t comedi_write(struct file *file, const char __user *buf,
 	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;
 	}
@@ -2376,15 +2368,8 @@ static ssize_t comedi_write(struct file *file, const char __user *buf,
 				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.2

--
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]

#1273303

On Wednesday, November 18, 2015 10:55 AM, Ian Abbott wrote:
> Tidy up the "write" file operation handler, `comedi_write()` a bit and
> improve the error handling.
>
> 1) staging: comedi: rearrange comedi_write() code
> 2) staging: comedi: do extra checks for becoming non-busy for "write"
> 3) staging: comedi: make some variables unsigned in comedi_write()
> 4) staging: comedi: avoid bad truncation of a size_t in comedi_write()
> 5) staging: comedi: allow buffer wraparound in comedi_write()
> 6) staging: comedi: return error on "write" if no command set up
> 7) staging: comedi: simplify returned errors for comedi_write()
> 8) staging: comedi: check for more errors for zero-length write
>
>  drivers/staging/comedi/comedi_fops.c | 124 ++++++++++++++++-------------------
>  1 file changed, 56 insertions(+), 68 deletions(-)

Ian,

Other than the minor nit-pick in patch 1 about the 'count == 0'
when becoming non-busy (the same situation is in comedi_read),
this looks good to me. It also makes the 'write' look more like the
'read'.

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