On Tue, 15 Nov 2011, Hans Petter Selasky wrote:
On Tuesday 15 November 2011 21:54:28 m...@freebsd.org wrote:
On Tue, Nov 15, 2011 at 12:48 PM, Hans Petter Selasky
<hsela...@freebsd.org> 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
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
