Re: HID issues.

[Vojtech P. <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. 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.

> * 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.

> * 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.

> This is a "PID Block Load Report", as reported by lsusb:
> (Yeah, I know, but this was the shortest one that shows this...)
> 
> Note how the "Block Load {Success,Full,Error}" fields
> have three layers of usages:
> * The report usage (PID Block Load Report)
> * The logical collection (Block Load Status)
> * The field's actual usage.
> 
>     Item(Global): Usage Page, data= [ 0x0f ] 15
>                     PID Page
>     Item(Local ): Usage, data= [ 0x89 ] 137
>                     PID Block Load Report
>     Item(Main  ): Collection, data= [ 0x02 ] 2
>                     Logical
>         Item(Global): Report ID, data= [ 0x02 ] 2 (feature)
>         Item(Local ): Usage, data= [ 0x22 ] 34
>                         Effect Block Index
>         Item(Global): Logical Maximum, data= [ 0x28 ] 40
>         Item(Global): Logical Minimum, data= [ 0x01 ] 1
>         Item(Global): Physical Minimum, data= [ 0x01 ] 1
>         Item(Global): Physical Maximum, data= [ 0x28 ] 40
>         Item(Global): Report Size, data= [ 0x08 ] 8
>         Item(Global): Report Count, data= [ 0x01 ] 1
>         Item(Main  ): Feature, data= [ 0x02 ] 2
>                     Data Variable Absolute No_Wrap Linear
>                     Preferred_State No_Null_Position Non_Volatile Bitfield
>         Item(Local ): Usage, data= [ 0x8b ] 139
>                     Block Load Status
>         Item(Main  ): Collection, data= [ 0x02 ] 2
>                         Logical
>             Item(Local ): Usage, data= [ 0x8c ] 140
>                             Block Load Success
>             Item(Local ): Usage, data= [ 0x8d ] 141
>                             Block Load Full
>             Item(Local ): Usage, data= [ 0x8e ] 142
>                             Block Load Error
>             Item(Global): Logical Maximum, data= [ 0x03 ] 3
>             Item(Global): Logical Minimum, data= [ 0x01 ] 1
>             Item(Global): Physical Minimum, data= [ 0x01 ] 1
>             Item(Global): Physical Maximum, data= [ 0x03 ] 3
>             Item(Global): Report Size, data= [ 0x08 ] 8
>             Item(Global): Report Count, data= [ 0x01 ] 1
>             Item(Main  ): Feature, data= [ 0x00 ] 0
>                             Data Array Absolute No_Wrap Linear
>                             Preferred_State No_Null_Position Non_Volatile Bitfield
>         Item(Main  ): End Collection, data=none
>         Item(Local ): Usage, data= [ 0xac ] 172
>                         RAM Pool Available
>         Item(Global): Logical Minimum, data= [ 0x00 ] 0
>         Item(Global): Logical Maximum, data= [ 0xff 0xff 0x00 0x00 ] 65535
>         Item(Global): Physical Minimum, data= [ 0x00 ] 0
>         Item(Global): Physical Maximum, data= [ 0xff 0xff 0x00 0x00 ] 65535
>         Item(Global): Report Size, data= [ 0x10 ] 16
>         Item(Global): Report Count, data= [ 0x01 ] 1
>         Item(Main  ): Feature, data= [ 0x00 ] 0
>                         Data Array Absolute No_Wrap Linear
>                         Preferred_State No_Null_Position Non_Volatile Bitfield
>     Item(Main  ): End Collection, data=none
> 
> This is how the kernel parses it:
> 
> localhost kernel:   FEATURE(2)[FEATURE]
> localhost kernel:     Field(0)
> localhost kernel:       Logical(PhysicalInterfaceDevice.PID_Block_Load_Report)
> localhost kernel:       Usage(1)
> localhost kernel:         PhysicalInterfaceDevice.Effect_Block_Index
> localhost kernel:       Logical Minimum(1)
> localhost kernel:       Logical Maximum(40)
> localhost kernel:       Physical Minimum(1)
> localhost kernel:       Physical Maximum(40)
> localhost kernel:       Report Size(8)
> localhost kernel:       Report Count(1)
> localhost kernel:       Report Offset(0)
> localhost kernel:       Flags( Variable Absolute )
> localhost kernel:     Field(1)
> localhost kernel:       Logical(PhysicalInterfaceDevice.Block_Load_Status)
> localhost kernel:       Usage(3)
> localhost kernel:         PhysicalInterfaceDevice.Block_Load_Success
> localhost kernel:         PhysicalInterfaceDevice.Block_Load_Full
> localhost kernel:         PhysicalInterfaceDevice.Block_Load_Error
> localhost kernel:       Logical Minimum(1)
> localhost kernel:       Logical Maximum(3)
> localhost kernel:       Physical Minimum(1)
> localhost kernel:       Physical Maximum(3)
> localhost kernel:       Report Size(8)
> localhost kernel:       Report Count(1)
> localhost kernel:       Report Offset(8)
> localhost kernel:       Flags( Array Absolute )
> localhost kernel:     Field(2)
> localhost kernel:       Logical(PhysicalInterfaceDevice.PID_Block_Load_Report)
> localhost kernel:       Usage(1)
> localhost kernel:         PhysicalInterfaceDevice.RAM_Pool_Available
> localhost kernel:       Logical Minimum(0)
> localhost kernel:       Logical Maximum(65535)
> localhost kernel:       Physical Minimum(0)
> localhost kernel:       Physical Maximum(65535)
> localhost kernel:       Report Size(16)
> localhost kernel:       Report Count(1)
> localhost kernel:       Report Offset(16)
> localhost kernel:       Flags( Array Absolute )
> 
> Presently, my strategy for finding a specific report is to walk the
> tree, looking for a field with a Logical Usage that's the same as the
> report Usage I'm looking for, and then return the report pointer from
> that field.
> 
> This happens to work for my device, since there's always some field in
> every report that isn't an array, but that's not neccesarily true for
> all devices.
> 
> In other words: If we had a report with all array fields, we wouldn't
> know which usage that report has/represents.

-- 
Vojtech Pavlik
SuSE Labs