Re: [CFR] add usb_sleepout.[ch]

[Hans Petter Selasky]

On Monday 01 November 2010 03:03:48 Weongyo Jeong wrote:
 On Sun, Oct 31, 2010 at 03:09:49PM +0100, Hans Petter Selasky wrote:
  On Sunday 31 October 2010 01:19:01 Weongyo Jeong wrote:
   Hello USB guys,
  
  1) All the sleepout_xxx() functions need mutex asserts.
 

Hi,

 It looks it don't need it because callout(9) does it instead of sleepout
 routines.  Moreover sleepout don't create any mutexes in itself.

Ok.

 
  2) Is it allowed to call callout_stop() / callout_reset() unlocked at
  all?
 
 Yes as long as it doesn't have side effects.  It seems no drivers hold a
 lock to call callout_stop() / callout_reset().

All USB drivers do to ensure state coherency.

 
  What are the concequences if the mutex associated with the sleepout is
  NULL ?
 
 I added KASSERT macro to check this case at below.  However the sleepout
 pointer normally never be NULL because the intention of usage was as
 follows:
 
   struct driver_softc {
   struct sleepout sleepout;
   struct sleepout_task sleepout_task;
   };
 
   sleepout_create(sc-sleepout, blah);
 
 Only it could be happen if `struct sleepout' is allocated by
 dynamically though it's not my first purpose.
 
  3) As per the current implementation it can happen that the task'ed
  timeout is called after that sleepout_stop() is used. The code should
  have a check in the task function to see if the sleepout() has been
  cancelled or not, when the mutex associated with the sleepout is locked.
 
 Yes it's true but it'd better to implement first taskqueue_stop() than
 adding it sleepout routines directly.  However no plans yet because I
 couldn't imagine a side effect due to lack of this feature.  Please let
 me know if I missed the something important.

Maybe not when you are implementing a watchdog timer for ethernet, but then 
you are limiting the use of those functions to USB ethernet only. The problem 
happens when you are updating a state-machine in the callback and cannot trust 
that a stop cancelled the sleepout. All existing USB functions are written 
this way. I.E. no completion done callback after usbd_transfer_stop().

For example if your watchdog is calling if_start() somehow, and then you do an 
if_down() which stops the watchdog, and then you can end up triggering the 
if_start() after if_down(), which is incorrect.

 
  4) Should the functions be prefixed by usb_ ?
 
 I don't have a preference for this.  I think it's no problem to change
 it.  It could happen soon.
 
  5) In drain you should drain the callout first, then drain the tasqueue.
  Else the timer can trigger after the taskqueue is drained.

Have you considered using the USB sub-systems taskqueue, instead of the kernel 
one, so that jobs can be serialised among the two? Else you end up introducing 
SX-locks in all drivers? Is that on purpose?

 It's fixed.  Thank you for your review and the updated version is
 embedded at this email.

--HPS
___
freebsd-usb@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-usb
To unsubscribe, send any mail to freebsd-usb-unsubscr...@freebsd.org

Re: [CFR] add usb_sleepout.[ch]

[Weongyo Jeong]

On Mon, Nov 01, 2010 at 09:10:43AM +0100, Hans Petter Selasky wrote:
 On Monday 01 November 2010 03:03:48 Weongyo Jeong wrote:
  On Sun, Oct 31, 2010 at 03:09:49PM +0100, Hans Petter Selasky wrote:
   On Sunday 31 October 2010 01:19:01 Weongyo Jeong wrote:
Hello USB guys,
   
   1) All the sleepout_xxx() functions need mutex asserts.
  
 
 Hi,
 
  It looks it don't need it because callout(9) does it instead of sleepout
  routines.  Moreover sleepout don't create any mutexes in itself.
 
 Ok.
 
  
   2) Is it allowed to call callout_stop() / callout_reset() unlocked at
   all?
  
  Yes as long as it doesn't have side effects.  It seems no drivers hold a
  lock to call callout_stop() / callout_reset().
 
 All USB drivers do to ensure state coherency.
 
  
   What are the concequences if the mutex associated with the sleepout is
   NULL ?
  
  I added KASSERT macro to check this case at below.  However the sleepout
  pointer normally never be NULL because the intention of usage was as
  follows:
  
struct driver_softc {
struct sleepout sleepout;
struct sleepout_task sleepout_task;
};
  
sleepout_create(sc-sleepout, blah);
  
  Only it could be happen if `struct sleepout' is allocated by
  dynamically though it's not my first purpose.
  
   3) As per the current implementation it can happen that the task'ed
   timeout is called after that sleepout_stop() is used. The code should
   have a check in the task function to see if the sleepout() has been
   cancelled or not, when the mutex associated with the sleepout is locked.
  
  Yes it's true but it'd better to implement first taskqueue_stop() than
  adding it sleepout routines directly.  However no plans yet because I
  couldn't imagine a side effect due to lack of this feature.  Please let
  me know if I missed the something important.
 
 Maybe not when you are implementing a watchdog timer for ethernet, but then 
 you are limiting the use of those functions to USB ethernet only. The problem 
 happens when you are updating a state-machine in the callback and cannot 
 trust 
 that a stop cancelled the sleepout. All existing USB functions are written 
 this way. I.E. no completion done callback after usbd_transfer_stop().
 
 For example if your watchdog is calling if_start() somehow, and then you do 
 an 
 if_down() which stops the watchdog, and then you can end up triggering the 
 if_start() after if_down(), which is incorrect.

OK that it makes sense to me.  I'll try to make a patch, taskqueue_stop().

   4) Should the functions be prefixed by usb_ ?
  
  I don't have a preference for this.  I think it's no problem to change
  it.  It could happen soon.
  
   5) In drain you should drain the callout first, then drain the tasqueue.
   Else the timer can trigger after the taskqueue is drained.
 
 Have you considered using the USB sub-systems taskqueue, instead of the 
 kernel 
 one, so that jobs can be serialised among the two? Else you end up 
 introducing 
 SX-locks in all drivers? Is that on purpose?

As mentioned at the previous email.  I prefer to use SX lock than
taskqueue.  Please refer my email which sent just minutes ago.

  It's fixed.  Thank you for your review and the updated version is
  embedded at this email.

regards,
Weongyo Jeong

___
freebsd-usb@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-usb
To unsubscribe, send any mail to freebsd-usb-unsubscr...@freebsd.org

Re: [CFR] add usb_sleepout.[ch]

[Hans Petter Selasky]

On Sunday 31 October 2010 01:19:01 Weongyo Jeong wrote:
 Hello USB guys,
 
 The following patch is to add a implementation, called `sleepout'.
 Please reviews.  I'd like to commit it into HEAD if no objections.
 
   Adds `sleepout' prototype which is a comic combination of callout(9) and
   taskqueue(8) only for USB drivers to implement one step timer.  In
   current USB drivers using callout(9) interface they all have two step
   execution flow as follows:
 
 1. callout callback is fired by the interrupt context.  Then it needs
to pass it to USB process context because it could sleep(!) while
callout(9) don't allow it.
 2. In the USB process context it operates USB commands that most of
times it'd be blocked at least 125 us (it'd be always true for USB)
 
   In a view of driver developer it'd be more convenient if USB stack has a
   feature like this (timer supporting blocking).
 

Hi,

I think this is a good idea too.

However, there are some atomic issues I think you need to fix first.

1) All the sleepout_xxx() functions need mutex asserts.

2) Is it allowed to call callout_stop() / callout_reset() unlocked at all? 
What are the concequences if the mutex associated with the sleepout is NULL ?

3) As per the current implementation it can happen that the task'ed timeout is 
called after that sleepout_stop() is used. The code should have a check in the 
task function to see if the sleepout() has been cancelled or not, when the 
mutex associated with the sleepout is locked.

4) Should the functions be prefixed by usb_ ?

5) In drain you should drain the callout first, then drain the tasqueue. Else 
the timer can trigger after the taskqueue is drained.

--HPS
___
freebsd-usb@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-usb
To unsubscribe, send any mail to freebsd-usb-unsubscr...@freebsd.org

Re: [CFR] add usb_sleepout.[ch]

[Weongyo Jeong]

On Sun, Oct 31, 2010 at 03:09:49PM +0100, Hans Petter Selasky wrote:
 On Sunday 31 October 2010 01:19:01 Weongyo Jeong wrote:
  Hello USB guys,
  
  The following patch is to add a implementation, called `sleepout'.
  Please reviews.  I'd like to commit it into HEAD if no objections.
  
Adds `sleepout' prototype which is a comic combination of callout(9) and
taskqueue(8) only for USB drivers to implement one step timer.  In
current USB drivers using callout(9) interface they all have two step
execution flow as follows:
  
  1. callout callback is fired by the interrupt context.  Then it needs
 to pass it to USB process context because it could sleep(!) while
 callout(9) don't allow it.
  2. In the USB process context it operates USB commands that most of
 times it'd be blocked at least 125 us (it'd be always true for USB)
  
In a view of driver developer it'd be more convenient if USB stack has a
feature like this (timer supporting blocking).
  
 
 Hi,
 
 I think this is a good idea too.
 
 However, there are some atomic issues I think you need to fix first.
 
 1) All the sleepout_xxx() functions need mutex asserts.

It looks it don't need it because callout(9) does it instead of sleepout
routines.  Moreover sleepout don't create any mutexes in itself.

 2) Is it allowed to call callout_stop() / callout_reset() unlocked at all? 

Yes as long as it doesn't have side effects.  It seems no drivers hold a
lock to call callout_stop() / callout_reset().

 What are the concequences if the mutex associated with the sleepout is NULL ?

I added KASSERT macro to check this case at below.  However the sleepout
pointer normally never be NULL because the intention of usage was as
follows:

  struct driver_softc {
  struct sleepout sleepout;
  struct sleepout_task sleepout_task;
  };

  sleepout_create(sc-sleepout, blah);

Only it could be happen if `struct sleepout' is allocated by
dynamically though it's not my first purpose.

 3) As per the current implementation it can happen that the task'ed timeout 
 is 
 called after that sleepout_stop() is used. The code should have a check in 
 the 
 task function to see if the sleepout() has been cancelled or not, when the 
 mutex associated with the sleepout is locked.

Yes it's true but it'd better to implement first taskqueue_stop() than
adding it sleepout routines directly.  However no plans yet because I
couldn't imagine a side effect due to lack of this feature.  Please let
me know if I missed the something important.

 4) Should the functions be prefixed by usb_ ?

I don't have a preference for this.  I think it's no problem to change
it.  It could happen soon.

 5) In drain you should drain the callout first, then drain the tasqueue. Else 
 the timer can trigger after the taskqueue is drained.

It's fixed.  Thank you for your review and the updated version is
embedded at this email.

regards,
Weongyo Jeong

Index: usb_sleepout.c
===
--- usb_sleepout.c  (revision 0)
+++ usb_sleepout.c  (revision 0)
@@ -0,0 +1,149 @@
+/*-
+ * Permission to use, copy, modify, and distribute this software for any
+ * purpose with or without fee is hereby granted, provided that the above
+ * copyright notice and this permission notice appear in all copies.
+ *
+ * THE SOFTWARE IS PROVIDED AS IS AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+ * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+ * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
+ * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+ * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+ * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+ */
+
+#include sys/cdefs.h
+__FBSDID($FreeBSD$);
+#include sys/param.h
+#include sys/systm.h
+#include sys/kernel.h
+#include sys/lock.h
+#include sys/malloc.h
+#include sys/mutex.h
+
+#include dev/usb/usb_sleepout.h
+
+void
+sleepout_create(struct sleepout *s, const char *name)
+{
+
+   KASSERT(s != NULL, (sleepout ptr is NULL));
+
+   s-s_taskqueue = taskqueue_create(name, M_WAITOK,
+   taskqueue_thread_enqueue, s-s_taskqueue);
+   /* XXX adjusts the priority. */
+   taskqueue_start_threads(s-s_taskqueue, 1, PI_NET, %s sleepout,
+   name);
+}
+
+void
+sleepout_free(struct sleepout *s)
+{
+
+   KASSERT(s != NULL, (sleepout ptr is NULL));
+
+   taskqueue_free(s-s_taskqueue);
+}
+
+static void
+_sleepout_taskqueue_callback(void *arg, int pending)
+{
+   struct sleepout_task *st = arg;
+
+   (void)pending;
+
+   if (st-st_mtx != NULL)
+   mtx_lock(st-st_mtx);
+
+   st-st_func(st-st_arg);
+
+   if (st-st_mtx != NULL)
+   mtx_unlock(st-st_mtx);
+}
+
+void
+sleepout_init(struct sleepout *s, struct sleepout_task 

Re: [CFR] add usb_sleepout.[ch]

[Andrew Thompson]

On 31 October 2010 12:19, Weongyo Jeong weongyo.je...@gmail.com wrote:
 Hello USB guys,

 The following patch is to add a implementation, called `sleepout'.
 Please reviews.  I'd like to commit it into HEAD if no objections.

  Adds `sleepout' prototype which is a comic combination of callout(9) and
  taskqueue(8) only for USB drivers to implement one step timer.  In
  current USB drivers using callout(9) interface they all have two step
  execution flow as follows:

    1. callout callback is fired by the interrupt context.  Then it needs
       to pass it to USB process context because it could sleep(!) while
       callout(9) don't allow it.
    2. In the USB process context it operates USB commands that most of
       times it'd be blocked at least 125 us (it'd be always true for USB)


I think it is a good addition.


Andrew
___
freebsd-usb@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-usb
To unsubscribe, send any mail to freebsd-usb-unsubscr...@freebsd.org