From owner-freebsd-usb@FreeBSD.ORG Mon Nov 1 19:22:45 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 8D9E11065670; Mon, 1 Nov 2010 19:22:45 +0000 (UTC) (envelope-from weongyo.jeong@gmail.com) Received: from mail-fx0-f54.google.com (mail-fx0-f54.google.com [209.85.161.54]) by mx1.freebsd.org (Postfix) with ESMTP id E4ECD8FC14; Mon, 1 Nov 2010 19:22:44 +0000 (UTC) Received: by fxm17 with SMTP id 17so5425343fxm.13 for ; Mon, 01 Nov 2010 12:22:43 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:received:received:received:from:date:to:cc :subject:message-id:reply-to:references:mime-version:content-type :content-disposition:in-reply-to:user-agent:organization :x-operation-sytem; bh=IujDf+I0f1rVO24HwT/82eVFHBm/Bd6DL9HF1G4md08=; b=ne5LXuBsgL/eEUHbw1MpI2jFIXJAvJBphesTxEjVm7RkDW7kTAILRyTN+WmvMHnLmO LPif2no5Y+oaQV76vptRZ7mnl0xt3RoJrfugmxSCdh9gjZ6qOMr3vmt/uUOw6JzrK1W5 VYD7JLcNwcCj0FLmBi+C+pdJND3Mj++QKr+mA= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=from:date:to:cc:subject:message-id:reply-to:references:mime-version :content-type:content-disposition:in-reply-to:user-agent :organization:x-operation-sytem; b=UH+Lkn8mLKpWiP9SQpqGIUfsgxZR0nwt8w2uvYRF5MLQY/wACMK4zX/nOI15cbX7pr Q8A+r07pPUHrfCD9juSJEutGGZKvDKABWRcGxuuafuXPcGDZn/inx8lR2HlxJbJhaYBi pcpYyBvmB2CiSt5RWT1yjO++9kOmoq+q9swso= Received: by 10.223.96.74 with SMTP id g10mr4436279fan.84.1288639363813; Mon, 01 Nov 2010 12:22:43 -0700 (PDT) Received: from weongyo ([174.35.1.224]) by mx.google.com with ESMTPS id c25sm2637463fac.9.2010.11.01.12.22.39 (version=SSLv3 cipher=RC4-MD5); Mon, 01 Nov 2010 12:22:42 -0700 (PDT) Received: by weongyo (sSMTP sendmail emulation); Mon, 01 Nov 2010 12:22:37 -0700 From: Weongyo Jeong Date: Mon, 1 Nov 2010 12:22:37 -0700 To: Hans Petter Selasky Message-ID: <20101101192237.GF3918@weongyo> References: <20101030231901.GA83161@weongyo> <201010311509.49138.hselasky@c2i.net> <20101101020348.GD3918@weongyo> <201011010910.43121.hselasky@c2i.net> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <201011010910.43121.hselasky@c2i.net> User-Agent: Mutt/1.4.2.3i Organization: CDNetworks. X-Operation-Sytem: FreeBSD 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 Reply-To: Weongyo Jeong List-Id: FreeBSD support for USB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 01 Nov 2010 19:22:45 -0000 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 _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