[PATCH 0/7] staging: comedi: comedi_fops: some runflag and event handling changes

[PATCH 0/7] staging: comedi: comedi_fops: some runflag and event handling changes

[Ian Abbott]

Various changes to the core comedi code, mostly to do with spin-lock
usage for the comedi subdevice runflags and event handling.

1) staging: comedi: comedi_fops: rename comedi_set_subdevice_runflags()
2) staging: comedi: comedi_fops: eliminate a use of subdevice spin-lock
3) staging: comedi: comedi_fops: simplify comedi_is_subdevice_idle()
4) staging: comedi: comedi_fops: remove unnecessary s->async use
5) staging: comedi: comedi_fops: always clear events
6) staging: comedi: comedi_fops: send SIGIO according to command
   direction
7) staging: comedi: comedi_fops: extend spin-lock scope in comedi_event()

 drivers/staging/comedi/comedi_fops.c | 115 ++++++++++++++++++++++-------------
 1 file changed, 73 insertions(+), 42 deletions(-)

[PATCH 1/7] staging: comedi: comedi_fops: rename comedi_set_subdevice_runflags()

[Ian Abbott]

`comedi_set_subdevice_runflags()` changes the comedi subdevice's
`runflags` member according to a bit-mask and new bit values.  It's name
might suggest that it only "sets", not "clears".  Rename it to
`comedi_update_subdevice_runflags()` to avoid confusion.

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

diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
index 8bf57b7..2b75b7a1 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -601,8 +601,8 @@ static struct attribute *comedi_dev_attrs[] = {
 };
 ATTRIBUTE_GROUPS(comedi_dev);
 
-static void comedi_set_subdevice_runflags(struct comedi_subdevice *s,
-					  unsigned mask, unsigned bits)
+static void comedi_update_subdevice_runflags(struct comedi_subdevice *s,
+					     unsigned mask, unsigned bits)
 {
 	unsigned long flags;
 
@@ -677,7 +677,7 @@ static void do_become_nonbusy(struct comedi_device *dev,
 {
 	struct comedi_async *async = s->async;
 
-	comedi_set_subdevice_runflags(s, COMEDI_SRF_RUNNING, 0);
+	comedi_update_subdevice_runflags(s, COMEDI_SRF_RUNNING, 0);
 	if (async) {
 		comedi_buf_reset(s);
 		async->inttrig = NULL;
@@ -1678,8 +1678,8 @@ static int do_cmd_ioctl(struct comedi_device *dev,
 	if (async->cmd.flags & CMDF_WAKE_EOS)
 		async->cb_mask |= COMEDI_CB_EOS;
 
-	comedi_set_subdevice_runflags(s, COMEDI_SRF_BUSY_MASK,
-				      COMEDI_SRF_RUNNING);
+	comedi_update_subdevice_runflags(s, COMEDI_SRF_BUSY_MASK,
+					 COMEDI_SRF_RUNNING);
 
 	/*
 	 * Set s->busy _after_ setting COMEDI_SRF_RUNNING flag to avoid
@@ -2657,10 +2657,10 @@ void comedi_event(struct comedi_device *dev, struct comedi_subdevice *s)
 	}
 	if (runflags_mask) {
 		/*
-		 * Sets COMEDI_SRF_ERROR and COMEDI_SRF_RUNNING together
+		 * Changes COMEDI_SRF_ERROR and COMEDI_SRF_RUNNING together
 		 * atomically.
 		 */
-		comedi_set_subdevice_runflags(s, runflags_mask, runflags);
+		comedi_update_subdevice_runflags(s, runflags_mask, runflags);
 	}
 
 	if (async->cb_mask & s->async->events) {
-- 
2.1.4

[PATCH 2/7] staging: comedi: comedi_fops: eliminate a use of subdevice spin-lock

[Ian Abbott]

`comedi_is_subdevice_in_error()` is only used by `comedi_read()` and
`comedi_write()` and is only called (soon) after
`comedi_is_subdevice_running()` returns `false` (with extra conditions
in the case of `comedi_write()`).  `comedi_is_subdevice_running()` and
`comedi_get_subdevice_runflags()` both call
`comedi_get_subdevice_runflags()` which uses the subdevice's spin-lock.

Eliminate one use of the subdevice's spin-lock in `comedi_read()` and
`comedi_write()` by calling `comedi_get_subdevice_runflags()` and
checking the runflags directly.  Add a couple of inline functions to
check the runflags: `comedi_is_runflags_running()` and
`comedi_is_runflags_in_error()`.  These do the same test on runflags as
`comedi_is_subdevice_running()` and `comedi_is_subdevice_in_error()` but
get passed the runflags value directly.

`comedi_is_subdevice_in_error()` is no longer used, so remove it.

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

diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
index 2b75b7a1..17ac285 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -623,6 +623,16 @@ static unsigned comedi_get_subdevice_runflags(struct comedi_subdevice *s)
 	return runflags;
 }
 
+static bool comedi_is_runflags_running(unsigned runflags)
+{
+	return runflags & COMEDI_SRF_RUNNING;
+}
+
+static bool comedi_is_runflags_in_error(unsigned runflags)
+{
+	return runflags & COMEDI_SRF_ERROR;
+}
+
 /**
  * comedi_is_subdevice_running - check if async command running on subdevice
  * @s: comedi_subdevice struct
@@ -634,17 +644,10 @@ bool comedi_is_subdevice_running(struct comedi_subdevice *s)
 {
 	unsigned runflags = comedi_get_subdevice_runflags(s);
 
-	return (runflags & COMEDI_SRF_RUNNING) ? true : false;
+	return comedi_is_runflags_running(runflags);
 }
 EXPORT_SYMBOL_GPL(comedi_is_subdevice_running);
 
-static bool comedi_is_subdevice_in_error(struct comedi_subdevice *s)
-{
-	unsigned runflags = comedi_get_subdevice_runflags(s);
-
-	return (runflags & COMEDI_SRF_ERROR) ? true : false;
-}
-
 static bool comedi_is_subdevice_idle(struct comedi_subdevice *s)
 {
 	unsigned runflags = comedi_get_subdevice_runflags(s);
@@ -2282,13 +2285,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) {
+		unsigned runflags;
+
 		set_current_state(TASK_INTERRUPTIBLE);
 
-		if (!comedi_is_subdevice_running(s)) {
+		runflags = comedi_get_subdevice_runflags(s);
+		if (!comedi_is_runflags_running(runflags)) {
 			if (count == 0) {
 				struct comedi_subdevice *new_s;
 
-				if (comedi_is_subdevice_in_error(s))
+				if (comedi_is_runflags_in_error(runflags))
 					retval = -EPIPE;
 				else
 					retval = 0;
@@ -2435,8 +2441,10 @@ static ssize_t comedi_read(struct file *file, char __user *buf, size_t nbytes,
 			n = m;
 
 		if (n == 0) {
-			if (!comedi_is_subdevice_running(s)) {
-				if (comedi_is_subdevice_in_error(s))
+			unsigned runflags = comedi_get_subdevice_runflags(s);
+
+			if (!comedi_is_runflags_running(runflags)) {
+				if (comedi_is_runflags_in_error(runflags))
 					retval = -EPIPE;
 				else
 					retval = 0;
-- 
2.1.4

[PATCH 3/7] staging: comedi: comedi_fops: simplify comedi_is_subdevice_idle()

[Ian Abbott]

Don't use a conditional operator when a simple "not" will do.

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 17ac285..9e49c8a 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -652,7 +652,7 @@ static bool comedi_is_subdevice_idle(struct comedi_subdevice *s)
 {
 	unsigned runflags = comedi_get_subdevice_runflags(s);
 
-	return (runflags & COMEDI_SRF_BUSY_MASK) ? false : true;
+	return !(runflags & COMEDI_SRF_BUSY_MASK);
 }
 
 /**
-- 
2.1.4

[PATCH 4/7] staging: comedi: comedi_fops: remove unnecessary s->async use

[Ian Abbott]

In places where the `s->async` value has been stored in a local
variable, use the variable instead of repeating `s->async`.

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 9e49c8a..999e7d0 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -687,7 +687,7 @@ static void do_become_nonbusy(struct comedi_device *dev,
 		kfree(async->cmd.chanlist);
 		async->cmd.chanlist = NULL;
 		s->busy = NULL;
-		wake_up_interruptible_all(&s->async->wait_head);
+		wake_up_interruptible_all(&async->wait_head);
 	} else {
 		dev_err(dev->class_dev,
 			"BUG: (?) do_become_nonbusy called with async=NULL\n");
@@ -2652,14 +2652,14 @@ void comedi_event(struct comedi_device *dev, struct comedi_subdevice *s)
 	if (!comedi_is_subdevice_running(s))
 		return;
 
-	if (s->async->events & COMEDI_CB_CANCEL_MASK)
+	if (async->events & COMEDI_CB_CANCEL_MASK)
 		runflags_mask |= COMEDI_SRF_RUNNING;
 
 	/*
 	 * Remember if an error event has occurred, so an error
 	 * can be returned the next time the user does a read().
 	 */
-	if (s->async->events & COMEDI_CB_ERROR_MASK) {
+	if (async->events & COMEDI_CB_ERROR_MASK) {
 		runflags_mask |= COMEDI_SRF_ERROR;
 		runflags |= COMEDI_SRF_ERROR;
 	}
@@ -2671,14 +2671,14 @@ void comedi_event(struct comedi_device *dev, struct comedi_subdevice *s)
 		comedi_update_subdevice_runflags(s, runflags_mask, runflags);
 	}
 
-	if (async->cb_mask & s->async->events) {
+	if (async->cb_mask & async->events) {
 		wake_up_interruptible(&async->wait_head);
 		if (s->subdev_flags & SDF_CMD_READ)
 			kill_fasync(&dev->async_queue, SIGIO, POLL_IN);
 		if (s->subdev_flags & SDF_CMD_WRITE)
 			kill_fasync(&dev->async_queue, SIGIO, POLL_OUT);
 	}
-	s->async->events = 0;
+	async->events = 0;
 }
 EXPORT_SYMBOL_GPL(comedi_event);
 
-- 
2.1.4

[PATCH 5/7] staging: comedi: comedi_fops: always clear events

[Ian Abbott]

`comedi_event()` is called from low-level drivers to handle asynchronous
command event flags that are stored in `s->async->events` for subdevice
`s`.  It normally clears the event flags as well.  As a safety check, it
does nothing if no asynchronous command is running, but it leaves
`s->async->events` unchanged in this case.  For additional safety,
change it to always clear the event flags to avoid leaving stale event
flags set when another asynchronous command is set up.

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

diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
index 999e7d0..68ced20 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -2648,18 +2648,20 @@ void comedi_event(struct comedi_device *dev, struct comedi_subdevice *s)
 	struct comedi_async *async = s->async;
 	unsigned runflags = 0;
 	unsigned runflags_mask = 0;
+	unsigned int events = async->events;
 
+	async->events = 0;
 	if (!comedi_is_subdevice_running(s))
 		return;
 
-	if (async->events & COMEDI_CB_CANCEL_MASK)
+	if (events & COMEDI_CB_CANCEL_MASK)
 		runflags_mask |= COMEDI_SRF_RUNNING;
 
 	/*
 	 * Remember if an error event has occurred, so an error
 	 * can be returned the next time the user does a read().
 	 */
-	if (async->events & COMEDI_CB_ERROR_MASK) {
+	if (events & COMEDI_CB_ERROR_MASK) {
 		runflags_mask |= COMEDI_SRF_ERROR;
 		runflags |= COMEDI_SRF_ERROR;
 	}
@@ -2671,14 +2673,13 @@ void comedi_event(struct comedi_device *dev, struct comedi_subdevice *s)
 		comedi_update_subdevice_runflags(s, runflags_mask, runflags);
 	}
 
-	if (async->cb_mask & async->events) {
+	if (async->cb_mask & events) {
 		wake_up_interruptible(&async->wait_head);
 		if (s->subdev_flags & SDF_CMD_READ)
 			kill_fasync(&dev->async_queue, SIGIO, POLL_IN);
 		if (s->subdev_flags & SDF_CMD_WRITE)
 			kill_fasync(&dev->async_queue, SIGIO, POLL_OUT);
 	}
-	async->events = 0;
 }
 EXPORT_SYMBOL_GPL(comedi_event);
 
-- 
2.1.4

[PATCH 6/7] staging: comedi: comedi_fops: send SIGIO according to command direction

[Ian Abbott]

`comedi_event()` is called from low-level drivers to handle comedi
asynchronous command event flags.  Some events cause waiting tasks to be
woken up, and a `SIGIO` signal to be sent via `kill_fasync()`.  The
signal code is `POLL_OUT` if the subdevice supports commands in the
"write" direction, or `POLL_IN` for the "read" direction.  If the
subdevice supports commands in either direction, it sends two `SIGIO`
signals, one with each code.  Change that latter case to only send one
`SIGIO` signal, using the direction of the current command to determine
the signal code.  If the `CMDF_WRITE` flag is set in the current
command, it's in the "write" direction, otherwise it's in the "read"
direction.

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

diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
index 68ced20..7ae605f 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -2674,11 +2674,11 @@ void comedi_event(struct comedi_device *dev, struct comedi_subdevice *s)
 	}
 
 	if (async->cb_mask & events) {
+		int si_code;
+
 		wake_up_interruptible(&async->wait_head);
-		if (s->subdev_flags & SDF_CMD_READ)
-			kill_fasync(&dev->async_queue, SIGIO, POLL_IN);
-		if (s->subdev_flags & SDF_CMD_WRITE)
-			kill_fasync(&dev->async_queue, SIGIO, POLL_OUT);
+		si_code = async->cmd.flags & CMDF_WRITE ? POLL_OUT : POLL_IN;
+		kill_fasync(&dev->async_queue, SIGIO, si_code);
 	}
 }
 EXPORT_SYMBOL_GPL(comedi_event);
-- 
2.1.4

[PATCH 7/7] staging: comedi: comedi_fops: extend spin-lock scope in comedi_event()

[Ian Abbott]

`comedi_event()` is called from low-level drivers to handle comedi
asynchronous command event flags.  As a safety check, it checks the
subdevice's "run" flags to make sure an asynchronous command is running.
It can also change the run flags to mark the command as no longer
running (possibly also marking it as terminated with an error).
Checking the runflags and modifying them involves two uses of the
subdevice's spin-lock.  It seems better to do it with a single use of
the spin-lock.  This also avoids possible interactions with
`do_become_nonbusy()`.

Acquire the subdevice's spin-lock at the start of `comedi_event()` and
release it near the end, before a possible call to `kill_fasync()` (but
after it's parameter values have been determined).

Add and make use of few new inline helper functions:

* `__comedi_clear_subdevice_runflags()` -- clears some run flags without
  using the spin-lock
* `__comedi_set_subdevice_runflags()` -- sets some run flags without
  using the spin-lock
* `__comedi_get_subdevice_runflags()` -- a spin-lockless version of
  `comedi_get_subdevice_runflags()
* `__comedi_is_subdevice_running()` -- a spin-lockless version of
* `comedi_is_subdevice_running()`

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

diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
index 7ae605f..e78ddbe 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -601,24 +601,41 @@ static struct attribute *comedi_dev_attrs[] = {
 };
 ATTRIBUTE_GROUPS(comedi_dev);
 
+static void __comedi_clear_subdevice_runflags(struct comedi_subdevice *s,
+					      unsigned bits)
+{
+	s->runflags &= ~bits;
+}
+
+static void __comedi_set_subdevice_runflags(struct comedi_subdevice *s,
+					    unsigned bits)
+{
+	s->runflags |= bits;
+}
+
 static void comedi_update_subdevice_runflags(struct comedi_subdevice *s,
 					     unsigned mask, unsigned bits)
 {
 	unsigned long flags;
 
 	spin_lock_irqsave(&s->spin_lock, flags);
-	s->runflags &= ~mask;
-	s->runflags |= (bits & mask);
+	__comedi_clear_subdevice_runflags(s, mask);
+	__comedi_set_subdevice_runflags(s, bits & mask);
 	spin_unlock_irqrestore(&s->spin_lock, flags);
 }
 
+static unsigned __comedi_get_subdevice_runflags(struct comedi_subdevice *s)
+{
+	return s->runflags;
+}
+
 static unsigned comedi_get_subdevice_runflags(struct comedi_subdevice *s)
 {
 	unsigned long flags;
 	unsigned runflags;
 
 	spin_lock_irqsave(&s->spin_lock, flags);
-	runflags = s->runflags;
+	runflags = __comedi_get_subdevice_runflags(s);
 	spin_unlock_irqrestore(&s->spin_lock, flags);
 	return runflags;
 }
@@ -648,6 +665,13 @@ bool comedi_is_subdevice_running(struct comedi_subdevice *s)
 }
 EXPORT_SYMBOL_GPL(comedi_is_subdevice_running);
 
+static bool __comedi_is_subdevice_running(struct comedi_subdevice *s)
+{
+	unsigned runflags = __comedi_get_subdevice_runflags(s);
+
+	return comedi_is_runflags_running(runflags);
+}
+
 static bool comedi_is_subdevice_idle(struct comedi_subdevice *s)
 {
 	unsigned runflags = comedi_get_subdevice_runflags(s);
@@ -2646,40 +2670,38 @@ static const struct file_operations comedi_fops = {
 void comedi_event(struct comedi_device *dev, struct comedi_subdevice *s)
 {
 	struct comedi_async *async = s->async;
-	unsigned runflags = 0;
-	unsigned runflags_mask = 0;
-	unsigned int events = async->events;
+	unsigned int events;
+	int si_code = 0;
+	unsigned long flags;
+
+	spin_lock_irqsave(&s->spin_lock, flags);
 
+	events = async->events;
 	async->events = 0;
-	if (!comedi_is_subdevice_running(s))
+	if (!__comedi_is_subdevice_running(s)) {
+		spin_unlock_irqrestore(&s->spin_lock, flags);
 		return;
+	}
 
 	if (events & COMEDI_CB_CANCEL_MASK)
-		runflags_mask |= COMEDI_SRF_RUNNING;
+		__comedi_clear_subdevice_runflags(s, COMEDI_SRF_RUNNING);
 
 	/*
-	 * Remember if an error event has occurred, so an error
-	 * can be returned the next time the user does a read().
+	 * Remember if an error event has occurred, so an error can be
+	 * returned the next time the user does a read() or write().
 	 */
-	if (events & COMEDI_CB_ERROR_MASK) {
-		runflags_mask |= COMEDI_SRF_ERROR;
-		runflags |= COMEDI_SRF_ERROR;
-	}
-	if (runflags_mask) {
-		/*
-		 * Changes COMEDI_SRF_ERROR and COMEDI_SRF_RUNNING together
-		 * atomically.
-		 */
-		comedi_update_subdevice_runflags(s, runflags_mask, runflags);
-	}
+	if (events & COMEDI_CB_ERROR_MASK)
+		__comedi_set_subdevice_runflags(s, COMEDI_SRF_ERROR);
 
 	if (async->cb_mask & events) {
-		int si_code;
-
 		wake_up_interruptible(&async->wait_head);
 		si_code = async->cmd.flags & CMDF_WRITE ? POLL_OUT : POLL_IN;
-		kill_fasync(&dev->async_queue, SIGIO, si_code);
 	}
+
+	spin_unlock_irqrestore(&s->spin_lock, flags);
+
+	if (si_code)
+		kill_fasync(&dev->async_queue, SIGIO, si_code);
 }
 EXPORT_SYMBOL_GPL(comedi_event);
 
-- 
2.1.4

RE: [PATCH 7/7] staging: comedi: comedi_fops: extend spin-lock scope in comedi_event()

[Hartley Sweeten]

On Friday, March 27, 2015 8:13 AM, Ian Abbott wrote:
> `comedi_event()` is called from low-level drivers to handle comedi
> asynchronous command event flags.  As a safety check, it checks the
> subdevice's "run" flags to make sure an asynchronous command is running.
> It can also change the run flags to mark the command as no longer
> running (possibly also marking it as terminated with an error).
> Checking the runflags and modifying them involves two uses of the
> subdevice's spin-lock.  It seems better to do it with a single use of
> the spin-lock.  This also avoids possible interactions with
> `do_become_nonbusy()`.
>
> Acquire the subdevice's spin-lock at the start of `comedi_event()` and
> release it near the end, before a possible call to `kill_fasync()` (but
> after it's parameter values have been determined).
>
> Add and make use of few new inline helper functions:
>
> * `__comedi_clear_subdevice_runflags()` -- clears some run flags without
>   using the spin-lock
> * `__comedi_set_subdevice_runflags()` -- sets some run flags without
>   using the spin-lock
> * `__comedi_get_subdevice_runflags()` -- a spin-lockless version of
>   `comedi_get_subdevice_runflags()
> * `__comedi_is_subdevice_running()` -- a spin-lockless version of
> * `comedi_is_subdevice_running()`
>
> Signed-off-by: Ian Abbott <abbotti@mev.co.uk>

Ian,

For completeness, the comedi_alloc_spriv() helper should probably use
__comedi_set_subdevice_runflags() to set the COMEDI_SRF_FREE_SPRIV
bit.

Regards,
Hartley

Re: [PATCH 7/7] staging: comedi: comedi_fops: extend spin-lock scope in comedi_event()

[Ian Abbott]

On 30/03/15 17:47, Hartley Sweeten wrote:
> On Friday, March 27, 2015 8:13 AM, Ian Abbott wrote:
>> `comedi_event()` is called from low-level drivers to handle comedi
>> asynchronous command event flags.  As a safety check, it checks the
>> subdevice's "run" flags to make sure an asynchronous command is running.
>> It can also change the run flags to mark the command as no longer
>> running (possibly also marking it as terminated with an error).
>> Checking the runflags and modifying them involves two uses of the
>> subdevice's spin-lock.  It seems better to do it with a single use of
>> the spin-lock.  This also avoids possible interactions with
>> `do_become_nonbusy()`.
>>
>> Acquire the subdevice's spin-lock at the start of `comedi_event()` and
>> release it near the end, before a possible call to `kill_fasync()` (but
>> after it's parameter values have been determined).
>>
>> Add and make use of few new inline helper functions:
>>
>> * `__comedi_clear_subdevice_runflags()` -- clears some run flags without
>>    using the spin-lock
>> * `__comedi_set_subdevice_runflags()` -- sets some run flags without
>>    using the spin-lock
>> * `__comedi_get_subdevice_runflags()` -- a spin-lockless version of
>>    `comedi_get_subdevice_runflags()
>> * `__comedi_is_subdevice_running()` -- a spin-lockless version of
>> * `comedi_is_subdevice_running()`
>>
>> Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
>
> Ian,
>
> For completeness, the comedi_alloc_spriv() helper should probably use
> __comedi_set_subdevice_runflags() to set the COMEDI_SRF_FREE_SPRIV
> bit.
>
> Regards,
> Hartley
>

Good point.  "drivers/staging/comedi/drivers.c" also reads the runflags 
directly, so perhaps __comedi_clear_subdevice_runflags(), 
__comedi_set_subdevice_runflags() and __comedi_get_subdevice_runflags() 
should be placed in "drivers/staging/comedi/comedi_internal.h".  Or we 
could ditch all three of those inline functions as they are just simple 
one-liners.

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

RE: [PATCH 7/7] staging: comedi: comedi_fops: extend spin-lock scope in comedi_event()

[Hartley Sweeten]

On Tuesday, March 31, 2015 2:43 AM, Ian Abbott wrote:
> On 30/03/15 17:47, Hartley Sweeten wrote:
>> On Friday, March 27, 2015 8:13 AM, Ian Abbott wrote:
>>> `comedi_event()` is called from low-level drivers to handle comedi
>>> asynchronous command event flags.  As a safety check, it checks the
>>> subdevice's "run" flags to make sure an asynchronous command is running.
>>> It can also change the run flags to mark the command as no longer
>>> running (possibly also marking it as terminated with an error).
>>> Checking the runflags and modifying them involves two uses of the
>>> subdevice's spin-lock.  It seems better to do it with a single use of
>>> the spin-lock.  This also avoids possible interactions with
>>> `do_become_nonbusy()`.
>>>
>>> Acquire the subdevice's spin-lock at the start of `comedi_event()` and
>>> release it near the end, before a possible call to `kill_fasync()` (but
>>> after it's parameter values have been determined).
>>>
>>> Add and make use of few new inline helper functions:
>>>
>>> * `__comedi_clear_subdevice_runflags()` -- clears some run flags without
>>>    using the spin-lock
>>> * `__comedi_set_subdevice_runflags()` -- sets some run flags without
>>>    using the spin-lock
>>> * `__comedi_get_subdevice_runflags()` -- a spin-lockless version of
>>>    `comedi_get_subdevice_runflags()
>>> * `__comedi_is_subdevice_running()` -- a spin-lockless version of
>>> * `comedi_is_subdevice_running()`
>>>
>>> Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
>>
>> Ian,
>>
>> For completeness, the comedi_alloc_spriv() helper should probably use
>> __comedi_set_subdevice_runflags() to set the COMEDI_SRF_FREE_SPRIV
>> bit.
>
> Good point.  "drivers/staging/comedi/drivers.c" also reads the runflags 
> directly, so perhaps __comedi_clear_subdevice_runflags(), 
> __comedi_set_subdevice_runflags() and __comedi_get_subdevice_runflags() 
> should be placed in "drivers/staging/comedi/comedi_internal.h".  Or we 
> could ditch all three of those inline functions as they are just simple 
> one-liners.

comedi_internal.h works for mw. We just don't want to expose those functions
to the drivers.

Regards,
Hartley

Re: [PATCH 7/7] staging: comedi: comedi_fops: extend spin-lock scope in comedi_event()

[Ian Abbott]

On 31/03/15 17:13, Hartley Sweeten wrote:
> On Tuesday, March 31, 2015 2:43 AM, Ian Abbott wrote:
>> On 30/03/15 17:47, Hartley Sweeten wrote:
>>> On Friday, March 27, 2015 8:13 AM, Ian Abbott wrote:
>>>> `comedi_event()` is called from low-level drivers to handle comedi
>>>> asynchronous command event flags.  As a safety check, it checks the
>>>> subdevice's "run" flags to make sure an asynchronous command is running.
>>>> It can also change the run flags to mark the command as no longer
>>>> running (possibly also marking it as terminated with an error).
>>>> Checking the runflags and modifying them involves two uses of the
>>>> subdevice's spin-lock.  It seems better to do it with a single use of
>>>> the spin-lock.  This also avoids possible interactions with
>>>> `do_become_nonbusy()`.
>>>>
>>>> Acquire the subdevice's spin-lock at the start of `comedi_event()` and
>>>> release it near the end, before a possible call to `kill_fasync()` (but
>>>> after it's parameter values have been determined).
>>>>
>>>> Add and make use of few new inline helper functions:
>>>>
>>>> * `__comedi_clear_subdevice_runflags()` -- clears some run flags without
>>>>     using the spin-lock
>>>> * `__comedi_set_subdevice_runflags()` -- sets some run flags without
>>>>     using the spin-lock
>>>> * `__comedi_get_subdevice_runflags()` -- a spin-lockless version of
>>>>     `comedi_get_subdevice_runflags()
>>>> * `__comedi_is_subdevice_running()` -- a spin-lockless version of
>>>> * `comedi_is_subdevice_running()`
>>>>
>>>> Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
>>>
>>> Ian,
>>>
>>> For completeness, the comedi_alloc_spriv() helper should probably use
>>> __comedi_set_subdevice_runflags() to set the COMEDI_SRF_FREE_SPRIV
>>> bit.
>>
>> Good point.  "drivers/staging/comedi/drivers.c" also reads the runflags
>> directly, so perhaps __comedi_clear_subdevice_runflags(),
>> __comedi_set_subdevice_runflags() and __comedi_get_subdevice_runflags()
>> should be placed in "drivers/staging/comedi/comedi_internal.h".  Or we
>> could ditch all three of those inline functions as they are just simple
>> one-liners.
>
> comedi_internal.h works for mw. We just don't want to expose those functions
> to the drivers.

Okay, I'll do that in a separate patch since you've already signed off 
on this one.

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

RE: [PATCH 0/7] staging: comedi: comedi_fops: some runflag and event handling changes

[Hartley Sweeten]

On Friday, March 27, 2015 8:13 AM, Ian Abbott wrote:
> Various changes to the core comedi code, mostly to do with spin-lock
> usage for the comedi subdevice runflags and event handling.
>
> 1) staging: comedi: comedi_fops: rename comedi_set_subdevice_runflags()
> 2) staging: comedi: comedi_fops: eliminate a use of subdevice spin-lock
> 3) staging: comedi: comedi_fops: simplify comedi_is_subdevice_idle()
> 4) staging: comedi: comedi_fops: remove unnecessary s->async use
> 5) staging: comedi: comedi_fops: always clear events
> 6) staging: comedi: comedi_fops: send SIGIO according to command
>    direction
> 7) staging: comedi: comedi_fops: extend spin-lock scope in comedi_event()
>
>  drivers/staging/comedi/comedi_fops.c | 115 ++++++++++++++++++++++-------------
>  1 file changed, 73 insertions(+), 42 deletions(-)

Other than my comment on patch 7, which can be addressed in a separate
patch if you prefer.

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