From owner-freebsd-usb@FreeBSD.ORG Mon Nov 1 08:09:33 2010 Return-Path: Delivered-To: freebsd-usb@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id D6A6A106566C; Mon, 1 Nov 2010 08:09:33 +0000 (UTC) (envelope-from hselasky@c2i.net) Received: from swip.net (mailfe07.swip.net [212.247.154.193]) by mx1.freebsd.org (Postfix) with ESMTP id D08F78FC19; Mon, 1 Nov 2010 08:09:32 +0000 (UTC) X-Cloudmark-Score: 0.000000 [] X-Cloudmark-Analysis: v=1.1 cv=sEolSJAlcSxSMaOm1MQ0bvrIu+BNAN+OqG2UAUgC4Ok= c=1 sm=1 a=5o9ETn4JDi4A:10 a=8nJEP1OIZ-IA:10 a=CL8lFSKtTFcA:10 a=i9M/sDlu2rpZ9XS819oYzg==:17 a=HYGeF8SLwlOEH9T6usgA:9 a=ns9OU_32NzLckYhYfvM_TQET-lgA:4 a=wPNLvfGTeEIA:10 a=i9M/sDlu2rpZ9XS819oYzg==:117 Received: from [188.126.198.129] (account mc467741@c2i.net HELO laptop002.hselasky.homeunix.org) by mailfe07.swip.net (CommuniGate Pro SMTP 5.2.19) with ESMTPA id 42771659; Mon, 01 Nov 2010 09:09:30 +0100 From: Hans Petter Selasky To: Weongyo Jeong Date: Mon, 1 Nov 2010 09:10:43 +0100 User-Agent: KMail/1.13.5 (FreeBSD/8.1-STABLE; KDE/4.4.5; amd64; ; ) References: <20101030231901.GA83161@weongyo> <201010311509.49138.hselasky@c2i.net> <20101101020348.GD3918@weongyo> In-Reply-To: <20101101020348.GD3918@weongyo> X-Face: +~\`s("[*|O,="7?X@L.elg*F"OA\I/3%^p8g?ab%RN'(; _IjlA: hGE..Ew, XAQ*o#\/M~SC=S1-f9{EzRfT'|Hhll5Q]ha5Bt-s|oTlKMusi:1e[wJl}kd}GR Z0adGx-x_0zGbZj'e(Y[(UNle~)8CQWXW@:DX+9)_YlB[tIccCPN$7/L' MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-Id: <201011010910.43121.hselasky@c2i.net> Cc: freebsd-usb@freebsd.org Subject: Re: [CFR] add usb_sleepout.[ch] X-BeenThere: freebsd-usb@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: FreeBSD support for USB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 01 Nov 2010 08:09:34 -0000 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 _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