From owner-svn-src-all@FreeBSD.ORG Sat Nov 19 05:52:46 2011 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 37D5F106566C; Sat, 19 Nov 2011 05:52:46 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail15.syd.optusnet.com.au (mail15.syd.optusnet.com.au [211.29.132.196]) by mx1.freebsd.org (Postfix) with ESMTP id 8F1BB8FC0A; Sat, 19 Nov 2011 05:52:45 +0000 (UTC) Received: from c211-28-227-231.carlnfd1.nsw.optusnet.com.au (c211-28-227-231.carlnfd1.nsw.optusnet.com.au [211.28.227.231]) by mail15.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id pAJ5qVDY027481 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Sat, 19 Nov 2011 16:52:32 +1100 Date: Sat, 19 Nov 2011 16:52:31 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Hans Petter Selasky In-Reply-To: <201111152202.24093.hselasky@c2i.net> Message-ID: <20111119153204.K953@besplex.bde.org> References: <201111152048.pAFKmvNC016452@svn.freebsd.org> <201111152202.24093.hselasky@c2i.net> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: "svn-src-head@freebsd.org" , mdf@freebsd.org, "svn-src-all@freebsd.org" , "src-committers@freebsd.org" Subject: Re: svn commit: r227541 - head/sys/dev/usb/controller X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 19 Nov 2011 05:52:46 -0000 On Tue, 15 Nov 2011, Hans Petter Selasky wrote: > On Tuesday 15 November 2011 21:54:28 mdf@freebsd.org wrote: >> On Tue, Nov 15, 2011 at 12:48 PM, Hans Petter Selasky >> >> wrote: >>> Log: >>> Some brands of XHCI controllers needs more time to reset. >> >> ... and since there's no guarantee that hz is 1000 or has any >> particular value, most of these seem a bit spurious. hz/N is the usual method of converting timeouts in inverse-seconds to ticks -- hz must be used for correct scaling. (N is the timeout in inverse-seconds, 1/N is the timeout in seconds, and hz/N is the timeout in ticks.) However, N must be reasonably small for this to work, since hz may be 100 or even lower, so when N is 1000 hz/N is 0. This bug is common, so callout_reset(9) and its ancestores have always defended against it by silently converting timeouts of 0 to 1 (tick), but not all code that uses times in ticks does this. usb_pause() does do this. > The delays are a bit loosely defined, hence I think there is no clear > definition what the minimum and maximum delay time is in the XHCI spec. The > delay given just defines the minimum. It is not critical if the delay is > larger, just that there is some kind of timeout. > > I use pause() because that doesn't block if the modules are loaded after boot. > > usb_pause_mtx() has an "if (cold)" check inside. > >> Is there some reason these functions aren't asking for a delay in >> terms of milli- or microseconds, and converting to hz internally? I >> would expect a delay while waiting for hardware to have a wall-clock >> time, not a time relative to hz, which has no predefined range. > > I have some macros that convert from hz to ms internally in the USB stack, > though I see your point that if hz is low, then the specified value derivates > from the real delay time. usb_pause() already does internal conversions back to (micro)seconds in the `cold' case, using slow home made code. In the !cold case, it uses pause(9) which uses tsleep(9), for which a timeout of 0 means infinity. usb_pause() handles sloppy callers by adding 1 itself. % /*------------------------------------------------------------------------* % * usb_pause_mtx - factored out code % * % * This function will delay the code by the passed number of system % * ticks. The passed mutex "mtx" will be dropped while waiting, if % * "mtx" is not NULL. % *------------------------------------------------------------------------*/ % void % usb_pause_mtx(struct mtx *mtx, int _ticks) % { Bogus variable name (1). `ticks' would shadow the global variable `ticks', but we mostly don't worry about shadowing and do it elsewhere for `ticks' itself. _ticks looks like it belongs to the implementation even more than does `ticks', and it only avoids shadowing because the implemenation doesn't us _ticks. % if (mtx != NULL) % mtx_unlock(mtx); % % if (cold) { % /* convert to milliseconds */ % _ticks = (_ticks * 1000) / hz; Bogus variable name (2). _ticks now has nothing to do with ticks. It is a time in milliseconds. % /* convert to microseconds, rounded up */ % _ticks = (_ticks + 1) * 1000; Bogus variable name (3). _ticks now has nothing to do with ticks. It is a time in microseconds. The comment misdescribes the adjustment to make _ticks nonzero. There is no rounding up. Instead, 1000 microseconds is added in a slightly obfuscated way, after previous steps rounded down, so if the rounding down gave 0, the result is 1000. This magic number is now inconsistent with the magic number in callers. There is a kernel global `tick = 1000000 / hz;' which is supposed in be used in such conversions. Using it here and removing the obfuscations and other style bugs, and changing the adjustment to 1 tick to be consistent with the !cold case gives: if (cold) DELAY(((_ticks + 1) * tick); See tvtohz(9) for a more careful conversion that worries about overflows corresponding to overflows in the addition and the multiplication in the above, but for the inverse conversion. hz must divide 1000000 exactly for the initial calculation of `tick' to work. `tick' used to be used for adjtime(2) and may have been changed from its initial value when adjtime() is active. Then the conversion would have tracked virtual real time instead of real time. This is not what is wanted for physical devices, but the slew is so slow that it should cause no problem with physical devices. This probably no longer happens, and it never happened for the cold case. `tick' might also be changed from its initial value to compensate for a large error in the value of `hz' (hz must be integral, so the timing hardware might not be able to get close to it and the error might be as much as 0.5% when hz = 100). I've never seen this used. If it were used, then `tick' would give the correct conversion factor for real time where 10000000/hz wouldn't. % DELAY(_ticks); % % } else { % % /* % * Add one to the number of ticks so that we don't return % * too early! % */ % _ticks++; The comment matches the code in this clause. This code used to almost double the timeout in cases that worked as intended (from 0-1 milliseconds to 1-2 milliseconds with old callers when hz = 1000). Now it has a smaller effect. With my simplification above, 1 tick is added in both clauses, so things can be simplified further by moving the adjustment to the top of the function. % % if (pause("USBWAIT", _ticks)) { % /* ignore */ % } % } % if (mtx != NULL) % mtx_lock(mtx); % } % > For USB compliant operation, the USB stack requires hz to be greater or equal > to 250 hz, to put it like that. Mostly a requirement in USB gadget/device > mode. FreeBSD never guaranteed any particular timeout granularity, but 10 msec can be assumed in practice (since hz = 100 was normal). Callers still have to be careful not to expand timeouts from 0-10 msec to 20-30 msec by themself or lower-level code rounding up and adding 1. Probably devices wanting 4 msec (from hz = 250) sort of work, but have much longer timeouts so they work slowly for polled operations. Bruce