[PATCH 0/7] staging: comedi: enforce data transfer direction

[PATCH 0/7] staging: comedi: enforce data transfer direction

[Ian Abbott]

Most comedi subdevices that support asynchronous data acquisition
commands only support data transfer in a single direction, as indicated
by the `SDF_CMD_READ` and `SDF_CMD_WRITE` subdevice flags.  A few allow
data transfer in either direction, but only a single direction at a
time, as indicated by the `CMDF_WRITE` command flag in the `flags`
member of the `struct comedi_cmd` from user-space used to set up the
asynchronous command.  The `CMDF_WRITE` flag isn't commonly set for
"write" commands if that is the only direction the subdevice supports.

Since it would be useful for comedi to have a clear indication of which
data transfer direction is being used, patch 1 forces the `CMDF_WRITE`
command flag to the correct state if only one direction is supported.
The flag can then be checked by comedi in the subdevice's local copy of
the current command (in `s->async->cmd`) to determine the data transfer
direction unambiguously.

Patch 2 stops the "me4000" driver clobbering the command flags.

Patch 3 removes some now redundant modification of the `CMDF_WRITE` flag
in "ni_mio_common.c" (used by the "ni_pcimio", "ni_atmio" and
"ni_mio_cs" drivers).

Patches 4 and 5 disallow the read() and write() file operations if the
subdevice has been set up with a command in the "write" or "read" data
transfer direction respectively.

Patch 6 changes the readable/writeable checks for the poll() file
operation.  It already considers the file readable/writeable if
read()/write() would fail with an error, so change it to check for
"wrong direction" as one of the possible failures.

Patch 7 makes the direction test in the handling of the `COMEDI_BUFINFO`
ioctl (used to update buffer positions for mmapped buffers) less
ambiguous.  The current test used the `SDF_CMD_READ` and `SDF_CMD_WRITE`
subdevice flags to check the direction, which is ambiguous if both flags
are set.  Update it to use the `CMDF_WRITE` command flag instead.

1) staging: comedi: maybe force CMDF_WRITE command flag
2) staging: comedi: me4000: don't clobber command flags
3) staging: comedi: ni_mio_common: don't change CMDF_WRITE flag
4) staging: comedi: don't allow read() on async command set up for "write"
5) staging: comedi: don't allow write() on async command set up for "read"
6) staging: comedi: check command direction in poll() file operation
7) staging: comedi: check actual data direction for COMEDI_BUFINFO ioctl

 drivers/staging/comedi/comedi_fops.c           | 37 ++++++++++++++++++++++++--
 drivers/staging/comedi/drivers/me4000.c        |  3 ---
 drivers/staging/comedi/drivers/ni_mio_common.c |  6 -----
 3 files changed, 35 insertions(+), 11 deletions(-)

[PATCH 1/7] staging: comedi: maybe force CMDF_WRITE command flag

[Ian Abbott]

Most comedi subdevices that support asynchronous commands only support
data transfer in either the "read" or "write" direction, as indicated by
the `SDF_CMD_READ` and `SDF_CMD_WRITE` subdevice flags, although a few
support both directions on the same subdevice (though not
simultaneously).  The `struct comedi_cmd` structure passed via ioctl
call to set up the command contains a `CMDF_WRITE` flag that can be used
to choose the direction if the subdevice supports both directions, but
the flag is optional if the subdevice only supports data transfer in one
direction.

If the subdevice only supports asynchronous data transfer in a sing
direction, set the `CMDF_WRITE` flag to the correct state so that Comedi
can make use of it later.  In the case of the `COMEDI_CMDTEST` ioctl,
the updated flag will be written back to the `struct comedi_cmd` in
user-space.  In the case of the `COMEDI_CMD` ioctl, the flag only gets
written back if an error is detected while testing the command, or if
the `CMDF_BOGUS` command flag is set.  Since `__comedi_get_user_cmd()`
is called for both ioctls, that's a good place to set the flag.

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

diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
index 224af2b..fef68da 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -1451,6 +1451,21 @@ static int __comedi_get_user_cmd(struct comedi_device *dev,
 		return -EINVAL;
 	}
 
+	/*
+	 * Set the CMDF_WRITE flag to the correct state if the subdevice
+	 * supports only "read" commands or only "write" commands.
+	 */
+	switch (s->subdev_flags & (SDF_CMD_READ | SDF_CMD_WRITE)) {
+	case SDF_CMD_READ:
+		cmd->flags &= ~CMDF_WRITE;
+		break;
+	case SDF_CMD_WRITE:
+		cmd->flags |= CMDF_WRITE;
+		break;
+	default:
+		break;
+	}
+
 	return 0;
 }
 
-- 
2.1.1

[PATCH 2/7] staging: comedi: me4000: don't clobber command flags

[Ian Abbott]

The low-level Comedi drivers shouldn't change the `flags` member of
`struct comedi_cmd` as the Comedi core also uses some of those flags.
They should just ignore the flags they don't understand.

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

diff --git a/drivers/staging/comedi/drivers/me4000.c b/drivers/staging/comedi/drivers/me4000.c
index 6516ac0..6e73e6e 100644
--- a/drivers/staging/comedi/drivers/me4000.c
+++ b/drivers/staging/comedi/drivers/me4000.c
@@ -848,9 +848,6 @@ static int me4000_ai_do_cmd_test(struct comedi_device *dev,
 	unsigned int scan_ticks;
 	int err = 0;
 
-	/* Only rounding flags are implemented */
-	cmd->flags &= CMDF_ROUND_NEAREST | CMDF_ROUND_UP | CMDF_ROUND_DOWN;
-
 	/* Round the timer arguments */
 	ai_round_cmd_args(dev, s, cmd, &init_ticks, &scan_ticks, &chan_ticks);
 
-- 
2.1.1

[PATCH 3/7] staging: comedi: ni_mio_common: don't change CMDF_WRITE flag

[Ian Abbott]

There is no need for `ni_ai_cmdtest()` or `ni_ao_cmdtest()` to set the
`CMDF_WRITE` flag to the correct state as it has already been done by
the core comedi module.

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

diff --git a/drivers/staging/comedi/drivers/ni_mio_common.c b/drivers/staging/comedi/drivers/ni_mio_common.c
index 785ad70..852b645 100644
--- a/drivers/staging/comedi/drivers/ni_mio_common.c
+++ b/drivers/staging/comedi/drivers/ni_mio_common.c
@@ -2251,9 +2251,6 @@ static int ni_ai_cmdtest(struct comedi_device *dev, struct comedi_subdevice *s,
 
 	/* Step 1 : check if triggers are trivially valid */
 
-	if ((cmd->flags & CMDF_WRITE))
-		cmd->flags &= ~CMDF_WRITE;
-
 	err |= cfc_check_trigger_src(&cmd->start_src,
 					TRIG_NOW | TRIG_INT | TRIG_EXT);
 	err |= cfc_check_trigger_src(&cmd->scan_begin_src,
@@ -3286,9 +3283,6 @@ static int ni_ao_cmdtest(struct comedi_device *dev, struct comedi_subdevice *s,
 
 	/* Step 1 : check if triggers are trivially valid */
 
-	if ((cmd->flags & CMDF_WRITE) == 0)
-		cmd->flags |= CMDF_WRITE;
-
 	err |= cfc_check_trigger_src(&cmd->start_src, TRIG_INT | TRIG_EXT);
 	err |= cfc_check_trigger_src(&cmd->scan_begin_src,
 					TRIG_TIMER | TRIG_EXT);
-- 
2.1.1

[PATCH 4/7] staging: comedi: don't allow read() on async command set up for "write"

[Ian Abbott]

If a Comedi asynchronous command has been set up for data transfer in
the "write" direction on the current "read" subdevice (for those
subdevices that support both directions), don't allow the "read" file
operation as that would mess with the data in the comedi data buffer
that is read by the low-level comedi hardware driver.

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

diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
index fef68da..6805ec9 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -2210,6 +2210,10 @@ static ssize_t comedi_read(struct file *file, char __user *buf, size_t nbytes,
 		retval = -EACCES;
 		goto out;
 	}
+	if (async->cmd.flags & CMDF_WRITE) {
+		retval = -EINVAL;
+		goto out;
+	}
 
 	add_wait_queue(&async->wait_head, &wait);
 	while (nbytes > 0 && !retval) {
@@ -2249,6 +2253,10 @@ static ssize_t comedi_read(struct file *file, char __user *buf, size_t nbytes,
 				retval = -EACCES;
 				break;
 			}
+			if (async->cmd.flags & CMDF_WRITE) {
+				retval = -EINVAL;
+				break;
+			}
 			continue;
 		}
 		m = copy_to_user(buf, async->prealloc_buf +
-- 
2.1.1

RE: [PATCH 4/7] staging: comedi: don't allow read() on async command set up for "write"

[Hartley Sweeten]

On Thursday, October 30, 2014 5:42 AM, Ian Abbott wrote:
> If a Comedi asynchronous command has been set up for data transfer in
> the "write" direction on the current "read" subdevice (for those
> subdevices that support both directions), don't allow the "read" file
> operation as that would mess with the data in the comedi data buffer
> that is read by the low-level comedi hardware driver.
>
> Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
> ---
>  drivers/staging/comedi/comedi_fops.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
> index fef68da..6805ec9 100644
> --- a/drivers/staging/comedi/comedi_fops.c
> +++ b/drivers/staging/comedi/comedi_fops.c
> @@ -2210,6 +2210,10 @@ static ssize_t comedi_read(struct file *file, char __user *buf, size_t nbytes,
>  		retval = -EACCES;
>  		goto out;
>  	}
> +	if (async->cmd.flags & CMDF_WRITE) {
> +		retval = -EINVAL;
> +		goto out;
> +	}
>  
>  	add_wait_queue(&async->wait_head, &wait);
>  	while (nbytes > 0 && !retval) {
> @@ -2249,6 +2253,10 @@ static ssize_t comedi_read(struct file *file, char __user *buf, size_t nbytes,
>  				retval = -EACCES;
>  				break;
>  			}
> +			if (async->cmd.flags & CMDF_WRITE) {
> +				retval = -EINVAL;
> +				break;
> +			}

Is this second test really needed in the while() loop?

For that matter, are the s->busy tests needed in the while() loop?

>  			continue;
 >		}
 >		m = copy_to_user(buf, async->prealloc_buf +

Regards,
Hartley

Re: [PATCH 4/7] staging: comedi: don't allow read() on async command set up for "write"

[Ian Abbott]

On 30/10/14 18:05, Hartley Sweeten wrote:
> On Thursday, October 30, 2014 5:42 AM, Ian Abbott wrote:
[snip]
>>   	add_wait_queue(&async->wait_head, &wait);
>>   	while (nbytes > 0 && !retval) {
>> @@ -2249,6 +2253,10 @@ static ssize_t comedi_read(struct file *file, char __user *buf, size_t nbytes,
>>   				retval = -EACCES;
>>   				break;
>>   			}
>> +			if (async->cmd.flags & CMDF_WRITE) {
>> +				retval = -EINVAL;
>> +				break;
>> +			}
>
> Is this second test really needed in the while() loop?
>
> For that matter, are the s->busy tests needed in the while() loop?

To answer your second question, some other thread using the same file 
object might have cancelled the asynchronous command, causing the 
current thread to see that the command is no longer active when it wakes up.

To answer your first question, that other thread might have managed to 
set up another asynchronous command in before we wake up, and it might 
have been set up as a "write" command (if the subdevice supports 
commands in both directions).  This doesn't detect the case when the 
other thread has managed to set up another "read" command, but since the 
current read() call hasn't read any data yet, we can just pretend we 
didn't know about the original command and read data from the new 
command instead.  (After all, the calling thread can't prove the read() 
started before the first command was cancelled, so we can just pretend 
it didn't.)

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

RE: [PATCH 4/7] staging: comedi: don't allow read() on async command set up for "write"

[Hartley Sweeten]

On Thursday, October 30, 2014 1:28 PM, Ian Abbott wrote:
> On 30/10/14 18:05, Hartley Sweeten wrote:
>> On Thursday, October 30, 2014 5:42 AM, Ian Abbott wrote:
>  [snip]
>>>   	add_wait_queue(&async->wait_head, &wait);
>>>   	while (nbytes > 0 && !retval) {
>>> @@ -2249,6 +2253,10 @@ static ssize_t comedi_read(struct file *file, char __user *buf, size_t nbytes,
>>>   				retval = -EACCES;
>>>   				break;
>>>   			}
>>> +			if (async->cmd.flags & CMDF_WRITE) {
>>> +				retval = -EINVAL;
>>> +				break;
>>> +			}
>>
>> Is this second test really needed in the while() loop?
>>
>> For that matter, are the s->busy tests needed in the while() loop?
>
> To answer your second question, some other thread using the same file 
> object might have cancelled the asynchronous command, causing the 
> current thread to see that the command is no longer active when it wakes up.
>
> To answer your first question, that other thread might have managed to 
> set up another asynchronous command in before we wake up, and it might 
> have been set up as a "write" command (if the subdevice supports 
> commands in both directions).  This doesn't detect the case when the 
> other thread has managed to set up another "read" command, but since the 
> current read() call hasn't read any data yet, we can just pretend we 
> didn't know about the original command and read data from the new 
> command instead.  (After all, the calling thread can't prove the read() 
> started before the first command was cancelled, so we can just pretend 
> it didn't.)

But when the command is first started by do_cmd_ioctl() we have this sequence:

	if (s->busy)
		return -EBUSY;
	...
	s->busy = file;
	ret = s->do_cmd(dev, s);

>From then on the s->busy pointer can only be cleared in do_become_nonbusy()
(by way of a (*cancel)). So another command cannot be started until the current
command is completed.

The user could do a (*do_cmdtest) while a command is running but that does not
effect the read/write of the async buffer.

Hartley

Re: [PATCH 4/7] staging: comedi: don't allow read() on async command set up for "write"

[Ian Abbott]

On 30/10/14 20:45, Hartley Sweeten wrote:
> On Thursday, October 30, 2014 1:28 PM, Ian Abbott wrote:
>> On 30/10/14 18:05, Hartley Sweeten wrote:
>>> On Thursday, October 30, 2014 5:42 AM, Ian Abbott wrote:
>>   [snip]
>>>>    	add_wait_queue(&async->wait_head, &wait);
>>>>    	while (nbytes > 0 && !retval) {
>>>> @@ -2249,6 +2253,10 @@ static ssize_t comedi_read(struct file *file, char __user *buf, size_t nbytes,
>>>>    				retval = -EACCES;
>>>>    				break;
>>>>    			}
>>>> +			if (async->cmd.flags & CMDF_WRITE) {
>>>> +				retval = -EINVAL;
>>>> +				break;
>>>> +			}
>>>
>>> Is this second test really needed in the while() loop?
>>>
>>> For that matter, are the s->busy tests needed in the while() loop?
>>
>> To answer your second question, some other thread using the same file
>> object might have cancelled the asynchronous command, causing the
>> current thread to see that the command is no longer active when it wakes up.
>>
>> To answer your first question, that other thread might have managed to
>> set up another asynchronous command in before we wake up, and it might
>> have been set up as a "write" command (if the subdevice supports
>> commands in both directions).  This doesn't detect the case when the
>> other thread has managed to set up another "read" command, but since the
>> current read() call hasn't read any data yet, we can just pretend we
>> didn't know about the original command and read data from the new
>> command instead.  (After all, the calling thread can't prove the read()
>> started before the first command was cancelled, so we can just pretend
>> it didn't.)
>
> But when the command is first started by do_cmd_ioctl() we have this sequence:
>
> 	if (s->busy)
> 		return -EBUSY;
> 	...
> 	s->busy = file;
> 	ret = s->do_cmd(dev, s);
>
>  From then on the s->busy pointer can only be cleared in do_become_nonbusy()
> (by way of a (*cancel)). So another command cannot be started until the current
> command is completed.

The other thread could do its own read() after it cancelled the command, 
which would clear the busy condition (once it returns 0 to indicate 
end-of-file), so the current thread's read() still needs to check it.

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

[PATCH 5/7] staging: comedi: don't allow write() on async command set up for "read"

[Ian Abbott]

If a Comedi asynchronous command has been set up for data transfer in
the "read" direction on the current "write" subdevice (for those
subdevices that support both directions), don't allow the "write" file
operation as that would mess with the data in the comedi data buffer
that is written by the low-level comedi hardware driver.

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

diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
index 6805ec9..6328965 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -2075,6 +2075,10 @@ static ssize_t comedi_write(struct file *file, const char __user *buf,
 		retval = -EACCES;
 		goto out;
 	}
+	if (!(async->cmd.flags & CMDF_WRITE)) {
+		retval = -EINVAL;
+		goto out;
+	}
 
 	add_wait_queue(&async->wait_head, &wait);
 	on_wait_queue = true;
@@ -2146,6 +2150,10 @@ static ssize_t comedi_write(struct file *file, const char __user *buf,
 				retval = -EACCES;
 				break;
 			}
+			if (!(async->cmd.flags & CMDF_WRITE)) {
+				retval = -EINVAL;
+				break;
+			}
 			continue;
 		}
 
-- 
2.1.1

RE: [PATCH 5/7] staging: comedi: don't allow write() on async command set up for "read"

[Hartley Sweeten]

On Thursday, October 30, 2014 5:43 AM, Ian Abbott wrote:
> If a Comedi asynchronous command has been set up for data transfer in
> the "read" direction on the current "write" subdevice (for those
> subdevices that support both directions), don't allow the "write" file
> operation as that would mess with the data in the comedi data buffer
> that is written by the low-level comedi hardware driver.
>
> Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
> ---
>  drivers/staging/comedi/comedi_fops.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
> index 6805ec9..6328965 100644
> --- a/drivers/staging/comedi/comedi_fops.c
> +++ b/drivers/staging/comedi/comedi_fops.c
> @@ -2075,6 +2075,10 @@ static ssize_t comedi_write(struct file *file, const char __user *buf,
>  		retval = -EACCES;
>  		goto out;
>  	}
> +	if (!(async->cmd.flags & CMDF_WRITE)) {
> +		retval = -EINVAL;
> +		goto out;
> +	}
>  
>  	add_wait_queue(&async->wait_head, &wait);
>  	on_wait_queue = true;
> @@ -2146,6 +2150,10 @@ static ssize_t comedi_write(struct file *file, const char __user *buf,
>  				retval = -EACCES;
>  				break;
>  			}
> +			if (!(async->cmd.flags & CMDF_WRITE)) {
> +				retval = -EINVAL;
> +				break;
> +			}

Same question as with PATCH 4/7.

Is this test needed in the while () loop. Also, are the s->busy tests needed here?

>  			continue;
>  		}

Regards,
Hartley

Re: [PATCH 5/7] staging: comedi: don't allow write() on async command set up for "read"

[Ian Abbott]

On 30/10/14 18:07, Hartley Sweeten wrote:
> On Thursday, October 30, 2014 5:43 AM, Ian Abbott wrote:
[snip]
>>   	add_wait_queue(&async->wait_head, &wait);
>>   	on_wait_queue = true;
>> @@ -2146,6 +2150,10 @@ static ssize_t comedi_write(struct file *file, const char __user *buf,
>>   				retval = -EACCES;
>>   				break;
>>   			}
>> +			if (!(async->cmd.flags & CMDF_WRITE)) {
>> +				retval = -EINVAL;
>> +				break;
>> +			}
>
> Same question as with PATCH 4/7.
>
> Is this test needed in the while () loop. Also, are the s->busy tests needed here?

Yes, for similar reasons.

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

[PATCH 6/7] staging: comedi: check command direction in poll() file operation

[Ian Abbott]

`comedi_poll()` handles the poll() file operation for comedi devices.

If no asynchronous command has been set up on the current "read"
subdevice, it sets the `POLLIN` and `POLLRDNORM` bits in the return
value to indicate that the read() file operation would not block as it
would return an error.  Add a check so it also does that if the
asynchronous command has been set up in the "write" direction as that
also causes the read() file operation to return an error.

Similarly, if no asynchronous command has need set up on the current
"write" subdevice, it sets the `POLLOUT` and `POLLWRNORM` bits in the
return value to indicate that the write() file operation would not block
as it would return an error.  Add a check so it also does that if the
asynchronous command has been set up in the "read" direction as that
also causes the write() file operation to return an error.

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 6328965..d55f709 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -2017,6 +2017,7 @@ static unsigned int comedi_poll(struct file *file, poll_table *wait)
 	if (s && s->async) {
 		poll_wait(file, &s->async->wait_head, wait);
 		if (!s->busy || !comedi_is_subdevice_running(s) ||
+		    (s->async->cmd.flags & CMDF_WRITE) ||
 		    comedi_buf_read_n_available(s) > 0)
 			mask |= POLLIN | POLLRDNORM;
 	}
@@ -2028,6 +2029,7 @@ static unsigned int comedi_poll(struct file *file, poll_table *wait)
 		poll_wait(file, &s->async->wait_head, wait);
 		comedi_buf_write_alloc(s, s->async->prealloc_bufsz);
 		if (!s->busy || !comedi_is_subdevice_running(s) ||
+		    !(s->async->cmd.flags & CMDF_WRITE) ||
 		    comedi_buf_write_n_allocated(s) >= bps)
 			mask |= POLLOUT | POLLWRNORM;
 	}
-- 
2.1.1

[PATCH 7/7] staging: comedi: check actual data direction for COMEDI_BUFINFO ioctl

[Ian Abbott]

`do_bufinfo_ioctl()` handled the `COMEDI_BUFINFO` ioctl.  It is supposed
to update the read or write positions in the buffer depending on the
direction of data transfer set up by the asynchronous command.
Currently it checks the `SDF_CMD_READ` and `SDF_CMD_WRITE` subdevice
flags.  That's fine for most subdevices - the ones that only support one
direction, but is incorrect for those subdevices that allow the command
to be set up in either direction.  Since we now set the `CMDF_WRITE`
flag according to the data transfer direction of the current command
running on the subdevice, check that flag instead.

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

diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
index d55f709..57ad6cd 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -991,7 +991,7 @@ static int do_bufinfo_ioctl(struct comedi_device *dev,
 	if (s->busy != file)
 		return -EACCES;
 
-	if (bi.bytes_read && (s->subdev_flags & SDF_CMD_READ)) {
+	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);
 
@@ -1001,7 +1001,7 @@ static int do_bufinfo_ioctl(struct comedi_device *dev,
 		}
 	}
 
-	if (bi.bytes_written && (s->subdev_flags & SDF_CMD_WRITE)) {
+	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);
-- 
2.1.1

RE: [PATCH 0/7] staging: comedi: enforce data transfer direction

[Hartley Sweeten]

On Thursday, October 30, 2014 5:42 AM, Ian Abbott wrote:
> Most comedi subdevices that support asynchronous data acquisition
> commands only support data transfer in a single direction, as indicated
> by the `SDF_CMD_READ` and `SDF_CMD_WRITE` subdevice flags.  A few allow
> data transfer in either direction, but only a single direction at a
> time, as indicated by the `CMDF_WRITE` command flag in the `flags`
> member of the `struct comedi_cmd` from user-space used to set up the
> asynchronous command.  The `CMDF_WRITE` flag isn't commonly set for
> "write" commands if that is the only direction the subdevice supports.
>
> Since it would be useful for comedi to have a clear indication of which
> data transfer direction is being used, patch 1 forces the `CMDF_WRITE`
> command flag to the correct state if only one direction is supported.
> The flag can then be checked by comedi in the subdevice's local copy of
> the current command (in `s->async->cmd`) to determine the data transfer
> direction unambiguously.
>
> Patch 2 stops the "me4000" driver clobbering the command flags.
>
> Patch 3 removes some now redundant modification of the `CMDF_WRITE` flag
> in "ni_mio_common.c" (used by the "ni_pcimio", "ni_atmio" and
> "ni_mio_cs" drivers).
>
> Patches 4 and 5 disallow the read() and write() file operations if the
> subdevice has been set up with a command in the "write" or "read" data
> transfer direction respectively.
>
> Patch 6 changes the readable/writeable checks for the poll() file
> operation.  It already considers the file readable/writeable if
> read()/write() would fail with an error, so change it to check for
> "wrong direction" as one of the possible failures.
>
> Patch 7 makes the direction test in the handling of the `COMEDI_BUFINFO`
> ioctl (used to update buffer positions for mmapped buffers) less
> ambiguous.  The current test used the `SDF_CMD_READ` and `SDF_CMD_WRITE`
> subdevice flags to check the direction, which is ambiguous if both flags
> are set.  Update it to use the `CMDF_WRITE` command flag instead.

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