[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