Date: Mon, 1 Nov 2010 12:22:37 -0700 From: Weongyo Jeong <weongyo.jeong@gmail.com> To: Hans Petter Selasky <hselasky@c2i.net> Cc: freebsd-usb@freebsd.org Subject: Re: [CFR] add usb_sleepout.[ch] Message-ID: <20101101192237.GF3918@weongyo> In-Reply-To: <201011010910.43121.hselasky@c2i.net> References: <20101030231901.GA83161@weongyo> <201010311509.49138.hselasky@c2i.net> <20101101020348.GD3918@weongyo> <201011010910.43121.hselasky@c2i.net>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20101101192237.GF3918>