staging: comedi: COMEDI_BUFINFO: some behavioural changes

staging: comedi: COMEDI_BUFINFO: some behavioural changes

[Ian Abbott]

These patches change the behavior of the `COMEDI_BUFINFO` ioctl, which
is used to manage buffer positions for a previously set up asynchronous
acquisition command.  It is used instead of the read and write file
operations when the buffer has been mmapped.

Patches 1 to 4 are fairly innocuous.  Patches 5 to 8 change the error
handling when the asynchronous command is no longer running, allow the
subdevice to become "non-busy" automatically in more cases (causing
subsequent calls to return `-EINVAL`), and report abnormal command
termination with return value `-EPIPE` (which also makes the subdevice
become non-busy).

1) staging: comedi: COMEDI_BUFINFO: get amount freed, not amount
   allocated
2) staging: comedi: COMEDI_BUFINFO: force bytes_read or bytes_written to
   0
3) staging: comedi: COMEDI_BUFINFO: update buffer before becoming
   non-busy
4) staging: comedi: COMEDI_BUFINFO: force bytes_written to 0 if stopped
5) staging: comedi: COMEDI_BUFINFO: return error if no active command
6) staging: comedi: COMEDI_BUFINFO: become non-busy even if bytes_read
   is 0
7) staging: comedi: COMEDI_BUFINFO: return -EPIPE for abnormal read
8) staging: comedi: COMEDI_BUFINFO: terminate "write" command when
   stopped

 drivers/staging/comedi/comedi_fops.c | 84 +++++++++++++++++++-----------------
 1 file changed, 44 insertions(+), 40 deletions(-)

[PATCH 1/8] staging: comedi: COMEDI_BUFINFO: get amount freed, not amount allocated

[Ian Abbott]

The `COMEDI_BUFINFO` ioctl is used to advance the current position in
the buffer by a specified amount (which can be 0) and get the new
position.  On input, the `bytes_read` member of `struct comedi_bufinfo`
specifies the amount to advance the "read" position for an asynchronous
command in the "read" direction, and the `bytes_written` member
specifies the amount to advance the "write" position for a command in
the "write" direction.  The handler `do_bufinfo_ioctl()` may limit the
specified values according to amount of readable or writable space in
the buffer.  On output, the `struct comedi_bufinfo` is filled in with
the updated position information, along with the adjusted `bytes_read`
and `bytes_written` members.

Advancing the buffer position occurs in two steps: first, some buffer
space is allocated, and second, it is freed, advancing the current
"read" or "write" position.  Currently, `do_bufinfo_ioctl()` limits
`bytes_read` or `bytes_written` to the amount it could allocate in the
first step, but that is invisible and irrelevant to the ioctl user.
It's mostly irrelevant to the COMEDI internals as well, apart from
limiting how much can be freed in the second step.  Change it to ignore
how much it managed to allocate in the first step and just use the
amount that was actually freed in the second step, which is the amount
the current buffer position was actually moved by this ioctl call.

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

diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
index d57fade..2cfb61e 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -1142,8 +1142,8 @@ static int do_bufinfo_ioctl(struct comedi_device *dev,
 		return -EACCES;
 
 	if (bi.bytes_read && !(async->cmd.flags & CMDF_WRITE)) {
-		bi.bytes_read = comedi_buf_read_alloc(s, bi.bytes_read);
-		comedi_buf_read_free(s, bi.bytes_read);
+		comedi_buf_read_alloc(s, bi.bytes_read);
+		bi.bytes_read = comedi_buf_read_free(s, bi.bytes_read);
 
 		if (comedi_is_subdevice_idle(s) &&
 		    comedi_buf_read_n_available(s) == 0) {
@@ -1152,9 +1152,8 @@ static int do_bufinfo_ioctl(struct comedi_device *dev,
 	}
 
 	if (bi.bytes_written && (async->cmd.flags & CMDF_WRITE)) {
-		bi.bytes_written =
-		    comedi_buf_write_alloc(s, bi.bytes_written);
-		comedi_buf_write_free(s, bi.bytes_written);
+		comedi_buf_write_alloc(s, bi.bytes_written);
+		bi.bytes_written = comedi_buf_write_free(s, bi.bytes_written);
 	}
 
 copyback_position:
-- 
2.7.0

[PATCH 2/8] staging: comedi: COMEDI_BUFINFO: force bytes_read or bytes_written to 0

[Ian Abbott]

The `COMEDI_BUFINFO` ioctl is used to advance the current position in
the buffer by a specified amount (which can be 0) and get the new
position.  On input, the `bytes_read` member of `struct comedi_bufinfo`
specifies the amount to advance the "read" position for an asynchronous
command in the "read" direction, and the `bytes_written` member
specifies the amount to advance the "write" position for a command in
the "write" direction.  The handler `do_bufinfo_ioctl()` may adjust
these by the amount the position is actually advanced before copying
them back to the user.  Currently, it ignores the specified `bytes_read`
value for a command in the "write" direction, and ignores the specified
`bytes_written` for a command in the "read" direction, so the values
copied back to the user are unchanged.  Change it to force the ignored
value to 0 before copying the values back to the user.

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

diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
index 2cfb61e..04d6040 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -1141,19 +1141,25 @@ static int do_bufinfo_ioctl(struct comedi_device *dev,
 	if (s->busy != file)
 		return -EACCES;
 
-	if (bi.bytes_read && !(async->cmd.flags & CMDF_WRITE)) {
-		comedi_buf_read_alloc(s, bi.bytes_read);
-		bi.bytes_read = comedi_buf_read_free(s, bi.bytes_read);
-
-		if (comedi_is_subdevice_idle(s) &&
-		    comedi_buf_read_n_available(s) == 0) {
-			do_become_nonbusy(dev, s);
+	if (!(async->cmd.flags & CMDF_WRITE)) {
+		/* command was set up in "read" direction */
+		if (bi.bytes_read) {
+			comedi_buf_read_alloc(s, bi.bytes_read);
+			bi.bytes_read = comedi_buf_read_free(s, bi.bytes_read);
+
+			if (comedi_is_subdevice_idle(s) &&
+			    comedi_buf_read_n_available(s) == 0)
+				do_become_nonbusy(dev, s);
 		}
-	}
-
-	if (bi.bytes_written && (async->cmd.flags & CMDF_WRITE)) {
-		comedi_buf_write_alloc(s, bi.bytes_written);
-		bi.bytes_written = comedi_buf_write_free(s, bi.bytes_written);
+		bi.bytes_written = 0;
+	} else {
+		/* command was set up in "write" direction */
+		if (bi.bytes_written) {
+			comedi_buf_write_alloc(s, bi.bytes_written);
+			bi.bytes_written =
+			    comedi_buf_write_free(s, bi.bytes_written);
+		}
+		bi.bytes_read = 0;
 	}
 
 copyback_position:
-- 
2.7.0

[PATCH 3/8] staging: comedi: COMEDI_BUFINFO: update buffer before becoming non-busy

[Ian Abbott]

The `COMEDI_BUFINFO` ioctl is used to advance the current position in
the buffer by a specified amount (which can be 0) and get the new
position.  For an asynchronous command in the "read" direction, if the
command has finished acquiring data normally, `do_become_nonbusy()` is
called to terminate the command.  That resets the buffer position, and
currently, the position information returned back to the user is after
the buffer has been reset.  It should be more useful to return the
buffer position before the reset, so move the call to
`do_become_nonbusy()` after the code that gets the updated buffer
position.

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

diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
index 04d6040..e625ef2 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -1111,6 +1111,7 @@ static int do_bufinfo_ioctl(struct comedi_device *dev,
 	struct comedi_bufinfo bi;
 	struct comedi_subdevice *s;
 	struct comedi_async *async;
+	bool become_nonbusy = false;
 
 	if (copy_from_user(&bi, arg, sizeof(bi)))
 		return -EFAULT;
@@ -1149,7 +1150,7 @@ static int do_bufinfo_ioctl(struct comedi_device *dev,
 
 			if (comedi_is_subdevice_idle(s) &&
 			    comedi_buf_read_n_available(s) == 0)
-				do_become_nonbusy(dev, s);
+				become_nonbusy = true;
 		}
 		bi.bytes_written = 0;
 	} else {
@@ -1168,6 +1169,9 @@ copyback_position:
 	bi.buf_read_count = async->buf_read_count;
 	bi.buf_read_ptr = async->buf_read_ptr;
 
+	if (become_nonbusy)
+		do_become_nonbusy(dev, s);
+
 copyback:
 	if (copy_to_user(arg, &bi, sizeof(bi)))
 		return -EFAULT;
-- 
2.7.0

[PATCH 4/8] staging: comedi: COMEDI_BUFINFO: force bytes_written to 0 if stopped

[Ian Abbott]

The `COMEDI_BUFINFO` ioctl is used to advance the current position in
the buffer by a specified amount (which can be 0) and get the new
position.  On input, the `bytes_written` member of `struct
comedi_bufinfo` specifies the amount to advance the "write" position for
an asynchronous command in the "write" direction.  On output, the member
indicates the amount the "write" position has actually been advanced.
Advancing the "write" position is current done even if the command has
stopped and cannot use any more written data.  Change it to force the
amount successfully written to 0 in that case.

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

diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
index e625ef2..b7c9270 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -1155,6 +1155,8 @@ static int do_bufinfo_ioctl(struct comedi_device *dev,
 		bi.bytes_written = 0;
 	} else {
 		/* command was set up in "write" direction */
+		if (!comedi_is_subdevice_running(s))
+			bi.bytes_written = 0;
 		if (bi.bytes_written) {
 			comedi_buf_write_alloc(s, bi.bytes_written);
 			bi.bytes_written =
-- 
2.7.0

[PATCH 5/8] staging: comedi: COMEDI_BUFINFO: return error if no active command

[Ian Abbott]

The `COMEDI_BUFINFO` ioctl is used to advance the current position in
the buffer and/or get the current buffer position.  If no asynchronous
command is active (started via the file object that issued this ioctl),
this information is meaningless.  Change it to return an error
(`-EINVAL`) in this case.  Prior to this change, if a command was
started via a different file object, the ioctl returned `-EACCES`, but
now it will return `-EINVAL`, which is consistent with the current
behavior of the "read" and "write" file operation handlers.

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

diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
index b7c9270..a9fabf7 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -1123,24 +1123,8 @@ static int do_bufinfo_ioctl(struct comedi_device *dev,
 
 	async = s->async;
 
-	if (!async) {
-		dev_dbg(dev->class_dev,
-			"subdevice does not have async capability\n");
-		bi.buf_write_ptr = 0;
-		bi.buf_read_ptr = 0;
-		bi.buf_write_count = 0;
-		bi.buf_read_count = 0;
-		bi.bytes_read = 0;
-		bi.bytes_written = 0;
-		goto copyback;
-	}
-	if (!s->busy) {
-		bi.bytes_read = 0;
-		bi.bytes_written = 0;
-		goto copyback_position;
-	}
-	if (s->busy != file)
-		return -EACCES;
+	if (!async || s->busy != file)
+		return -EINVAL;
 
 	if (!(async->cmd.flags & CMDF_WRITE)) {
 		/* command was set up in "read" direction */
@@ -1165,7 +1149,6 @@ static int do_bufinfo_ioctl(struct comedi_device *dev,
 		bi.bytes_read = 0;
 	}
 
-copyback_position:
 	bi.buf_write_count = async->buf_write_count;
 	bi.buf_write_ptr = async->buf_write_ptr;
 	bi.buf_read_count = async->buf_read_count;
@@ -1174,7 +1157,6 @@ copyback_position:
 	if (become_nonbusy)
 		do_become_nonbusy(dev, s);
 
-copyback:
 	if (copy_to_user(arg, &bi, sizeof(bi)))
 		return -EFAULT;
 
-- 
2.7.0

[PATCH 6/8] staging: comedi: COMEDI_BUFINFO: become non-busy even if bytes_read is 0

[Ian Abbott]

The `COMEDI_BUFINFO` ioctl is used to advance the current position in
the buffer by a specified amount (which can be 0) and get the new
position.  On input, the `bytes_read` member of `struct comedi_bufinfo`
specified the amount to advance the "read" position for an asynchronous
command in the "read" direction.  If the command has already stopped
normally, and the "read" position has been advanced to the end of all
available data, the command is terminated by calling
`do_become_nonbusy()`.  (That is not currently done if the command
stopped with an error.)  Currently, the command is only terminated if
the user is trying to advance the "read" position by a non-zero amount.
Change it to allow the command to be terminated even if the user is not
trying to advance the "read" position.  This is justifiable, as the only
time a command stops without error is when it has been set up to read a
finite amount of data.

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

diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
index a9fabf7..ffe58208 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -1131,11 +1131,10 @@ static int do_bufinfo_ioctl(struct comedi_device *dev,
 		if (bi.bytes_read) {
 			comedi_buf_read_alloc(s, bi.bytes_read);
 			bi.bytes_read = comedi_buf_read_free(s, bi.bytes_read);
-
-			if (comedi_is_subdevice_idle(s) &&
-			    comedi_buf_read_n_available(s) == 0)
-				become_nonbusy = true;
 		}
+		if (comedi_is_subdevice_idle(s) &&
+		    comedi_buf_read_n_available(s) == 0)
+			become_nonbusy = true;
 		bi.bytes_written = 0;
 	} else {
 		/* command was set up in "write" direction */
-- 
2.7.0

[PATCH 7/8] staging: comedi: COMEDI_BUFINFO: return -EPIPE for abnormal read

[Ian Abbott]

The `COMEDI_BUFINFO` ioctl is used to advance the current position in
the buffer by a specified amount (which can be 0) and get the current
position.  If an asynchronous command in the "read" direction has
stopped normally, the command is terminated as soon as the position has
been advanced to the end of all available data.  This is not currently
done if the command terminated with an error.  Change it to allow the
command to be terminated even if it stopped with an error, but report an
`EPIPE` error to the user first.  The `EPIPE` error will not be
reported until the "read" position reported back to the user has been
advanced to the end of all available data.

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

diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
index ffe58208..7cb1d06 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -686,13 +686,6 @@ static bool __comedi_is_subdevice_running(struct comedi_subdevice *s)
 	return comedi_is_runflags_running(runflags);
 }
 
-static bool comedi_is_subdevice_idle(struct comedi_subdevice *s)
-{
-	unsigned runflags = comedi_get_subdevice_runflags(s);
-
-	return !(runflags & COMEDI_SRF_BUSY_MASK);
-}
-
 bool comedi_can_auto_free_spriv(struct comedi_subdevice *s)
 {
 	unsigned runflags = __comedi_get_subdevice_runflags(s);
@@ -1111,6 +1104,8 @@ static int do_bufinfo_ioctl(struct comedi_device *dev,
 	struct comedi_bufinfo bi;
 	struct comedi_subdevice *s;
 	struct comedi_async *async;
+	unsigned int runflags;
+	int retval = 0;
 	bool become_nonbusy = false;
 
 	if (copy_from_user(&bi, arg, sizeof(bi)))
@@ -1132,9 +1127,20 @@ static int do_bufinfo_ioctl(struct comedi_device *dev,
 			comedi_buf_read_alloc(s, bi.bytes_read);
 			bi.bytes_read = comedi_buf_read_free(s, bi.bytes_read);
 		}
-		if (comedi_is_subdevice_idle(s) &&
-		    comedi_buf_read_n_available(s) == 0)
+		/*
+		 * If nothing left to read, and command has stopped, and
+		 * {"read" position not updated or command stopped normally},
+		 * then become non-busy.
+		 */
+		runflags = comedi_get_subdevice_runflags(s);
+		if (comedi_buf_read_n_available(s) == 0 &&
+		    !comedi_is_runflags_running(runflags) &&
+		    (bi.bytes_read == 0 ||
+		     !comedi_is_runflags_in_error(runflags))) {
 			become_nonbusy = true;
+			if (comedi_is_runflags_in_error(runflags))
+				retval = -EPIPE;
+		}
 		bi.bytes_written = 0;
 	} else {
 		/* command was set up in "write" direction */
@@ -1156,6 +1162,9 @@ static int do_bufinfo_ioctl(struct comedi_device *dev,
 	if (become_nonbusy)
 		do_become_nonbusy(dev, s);
 
+	if (retval)
+		return retval;
+
 	if (copy_to_user(arg, &bi, sizeof(bi)))
 		return -EFAULT;
 
-- 
2.7.0

[PATCH 8/8] staging: comedi: COMEDI_BUFINFO: terminate "write" command when stopped

[Ian Abbott]

The `COMEDI_BUFINFO` ioctl is used to advance the current position in
the buffer by a specified amount (which can be 0) and get the current
position.  An asynchronous command in the "read" direction is terminated
automatically once it has stopped and information about the final
position and error has been reported back to the user.  That is not
currently done for commands in the "write" direction.  Change it to
terminate the command in the "write" direction automatically.  If the
command stopped with an error, report an `EPIPE` error back to the user,
otherwise just report the final buffer position back to the user.

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

diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
index 7cb1d06..d1cf6a1 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -1121,6 +1121,7 @@ static int do_bufinfo_ioctl(struct comedi_device *dev,
 	if (!async || s->busy != file)
 		return -EINVAL;
 
+	runflags = comedi_get_subdevice_runflags(s);
 	if (!(async->cmd.flags & CMDF_WRITE)) {
 		/* command was set up in "read" direction */
 		if (bi.bytes_read) {
@@ -1132,7 +1133,6 @@ static int do_bufinfo_ioctl(struct comedi_device *dev,
 		 * {"read" position not updated or command stopped normally},
 		 * then become non-busy.
 		 */
-		runflags = comedi_get_subdevice_runflags(s);
 		if (comedi_buf_read_n_available(s) == 0 &&
 		    !comedi_is_runflags_running(runflags) &&
 		    (bi.bytes_read == 0 ||
@@ -1144,9 +1144,12 @@ static int do_bufinfo_ioctl(struct comedi_device *dev,
 		bi.bytes_written = 0;
 	} else {
 		/* command was set up in "write" direction */
-		if (!comedi_is_subdevice_running(s))
+		if (!comedi_is_runflags_running(runflags)) {
 			bi.bytes_written = 0;
-		if (bi.bytes_written) {
+			become_nonbusy = true;
+			if (comedi_is_runflags_in_error(runflags))
+				retval = -EPIPE;
+		} else if (bi.bytes_written) {
 			comedi_buf_write_alloc(s, bi.bytes_written);
 			bi.bytes_written =
 			    comedi_buf_write_free(s, bi.bytes_written);
-- 
2.7.0

Re: staging: comedi: COMEDI_BUFINFO: some behavioural changes

[Ian Abbott]

On 19/02/16 16:13, Ian Abbott wrote:
...stuff...

Sorry, I forgot to tag that email as "[PATCH 0/8]".

-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@mev.co.uk> )=-
-=(                          Web: http://www.mev.co.uk/  )=-