Re: HID issues.

[Vojtech P. <vo...@su...>]

On Fri, Jan 25, 2002 at 02:03:34AM +0100, Bj|rn Augustsson wrote:
> Quoting Vojtech Pavlik <vo...@su...>:
> > On Thu, Jan 24, 2002 at 10:33:02PM +0100, Bj|rn Augustsson wrote:
> > > 
> > > Hello, folks!
> > > 
> > > Looking thru the code, I found a bunch of things that I think someone
> > > should have a look at.
> > > 
> > > * There's a race in the HID init code. What happens is
> > > 
> > > send for all input reports;
> > > send for all feature reports;
> > > block until all feature reports are in;
> > > configure device based on input & feature report data. (hid_init_reports)
> > > 
> > > Some input reports might not have arrived yet when we call
> > > hid_init_reports.
> > > 
> > > (And yes, this actually happens in real life.)
> > 
> > Ugh? I don't think I understand you here fully. hid_init_reports() is
> > the function that makes all the request to get the input and feature
> > data. So *no* reports come before it is called. 
> 
> Quite. My mistake. 
> 
> > > Some input reports might not have arrived yet when we call
> > > hid_init_reports.
> 
> Should have been
> 
> Some input reports might not have arrived yet when 
> hid_init_reports returns.
> 
> This means that we call hidinput_connect() before we have all the 
> data.
> 
> Sorry for the confusion.

You mean when we time out? Well, yes, then we have a problem. But the
timeout is the error case which shouldn't happen. Otherwise, I don't see
how any pending 'get report' requests could stay in the queue - we wait
for both the control and out queue to clear completely. 

> > And it waits for all the
> > report to arrive then - it waits till the queue is empty. Maybe we are
> > each reading a different version? I'm looking at the current CVS.
> 
> Well, I'm not, but fairly close (CVS version tag 1.35).

Maybe update, there was a serious rewrite of that part.

> The way I read the code, it waits for all feature reports to come in.
> (On the control pipe.)
> All the input reports aren't neccessarily in yet though. (On the input
> interrupt pipe.)
> 
> Maybe there's some interaction here that I don't see/know about, but
> that's what it looks like to me.

Both input and feature reports come over the control pipe. The input
interrupt pipe isn't open at that time yet, and thus no reports are
coming over it.

> > > * When you do hid_set_field, there's code that checks the value against
> > > logical max/min for that field. That's useful information, but you're
> > > supposed to be able to set a value outside of that interval - a
> > > Null Value, it's called in the HID spec (5.10 in the spec). 
> > > We should probably warn if the value is out of the interval, but only 
> > > error if it won't actually fit in the field. 
> > > Ie: settting an 8-bit field with logical min = 1 and logical 
> > > max = 40 to 0xFF should probably warn (probably only if DEBUG), but 
> > > it should be allowed.
> > 
> > Correct, feel free to implement a better fix, I'll just remove the check
> > for now.
> 
> Right. This is my next sub-project then. 
> 
> > > * Why hasn't the hid_report struct got a usage field in it?
> > > This makes it difficult to find a certain report by walking the tree,
> > > and worse, information is lost, at least when we parse arrays:
> > 
> > Because it wasn't needed by anything yet. All decisions are made based
> > on the field usage data for the input-only devices and it is enough. So,
> > if you need it, add it.
> 
> Will do.
> 
> /August.
> -- 
> Wrong on most accounts.  const Foo *foo; and Foo const *foo; mean the same: foo
> being a pointer to const Foo.  const Foo const *foo; would mean the same but is
> illegal (double const).  You are confusing this with Foo * const foo; and const
> Foo * const foo; respectively. -David Kastrup, comp.os.linux.development.system

-- 
Vojtech Pavlik
SuSE Labs