Re: [PATCH 2/3] HID: steam: add serial number information.

[Benjamin Tissoires]

On Fri, Feb 16, 2018 at 10:57 AM, Rodrigo Rivas Costa
<rodrigorivascosta@xxxxxxxxx> wrote:
> On Fri, Feb 16, 2018 at 10:31:35AM +0100, Benjamin Tissoires wrote:
>> > Ok, I'll do that. The weird thing, however, is that:
>> >
>> >         hid_hw_raw_request(steam->hid_dev, 0x00,
>> >                 buf, hid_report_len(r), /* 64 */
>> >                 HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
>> >
>> > fails with EOVERFLOW. I have to use:
>> >
>> >         hid_hw_raw_request(steam->hid_dev, 0x00,
>> >                 buf, 65
>> >                 HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
>> >
>> > which just feels wrong to me.
>>
>> Indeed
>>
>> >
>> > And looking around drivers/hid/*.c I see that most calls to
>> > hid_hw_raw_request(..., HID_REQ_GET_REPORT) use a buffer allocated with
>> > {devm_,}kzalloc() and a constant length, never using
>> > hid_alloc_report_buf() or hid_report_len().
>>
>> well, hid-input.c and hid-multitouch.c are using
>> hid_alloc_report_buf() and these two are the most generic ones. We
>> haven't converted everybody to use hid_alloc_report_buf(), but it's
>> not a reason to not use it for new drivers :)
>
> Agreed. And they use hid_report_len() on the hid_hw_raw_request().
>
> Now my guess is that hid_report_len() should return 65 instead of 64.
> And it would do that if the report.id would be >0, but, alas, it is =0.
> Maybe feature reports should add 1 without checking id>0? Or maybe
> the Steam report descriptor is wrong and I should use report_fixup or
> something?
>
>> No, it's unlikely that there is a bug. Can you forward to me the
>> report descriptors of the Steam controller (ideally with the tool
>> hid-recorder from http://bentiss.github.io/hid-replay-docs/, so I can
>> get a few events too)?
>
> Sure, see the attached file. It is the wired one, I opened jstest and
> moved the joystick around to get a few events. Unfortunately the
> HID_REQ_GET_REPORT are not recorded.
>
> For the casual reader, here is the decoded report descriptor [1]:
>
> 0x06, 0x00, 0xFF,  // Usage Page (Vendor Defined 0xFF00)
> 0x09, 0x01,        // Usage (0x01)
> 0xA1, 0x01,        // Collection (Application)
> 0x15, 0x00,        //   Logical Minimum (0)
> 0x26, 0xFF, 0x00,  //   Logical Maximum (255)
> 0x75, 0x08,        //   Report Size (8)
> 0x95, 0x40,        //   Report Count (64)
> 0x09, 0x01,        //   Usage (0x01)
> 0x81, 0x02,        //   Input (Data,Var,Abs,No Wrap,Linear,Preferred State,No Null Position)
> 0x95, 0x40,        //   Report Count (64)
> 0x09, 0x01,        //   Usage (0x01)
> 0x91, 0x02,        //   Output (Data,Var,Abs,No Wrap,Linear,Preferred State,No Null Position,Non-volatile)
> 0x95, 0x40,        //   Report Count (64)
> 0x09, 0x01,        //   Usage (0x01)
> 0xB1, 0x02,        //   Feature (Data,Var,Abs,No Wrap,Linear,Preferred State,No Null Position,Non-volatile)
> 0xC0,              // End Collection
>

>
> [1]: http://eleccelerator.com/usbdescreqparser/


Thanks.

Arf, looks like usbhid expects the buffer to start with 0x00 when the
report is not numbered, thus adding one to the report length.

I guess that nobody tried to recently set/get reports on a device
without a report ID. And hidraw matches this behavior too, which means
it's hard to change.

One thing I'd like to try to change is the result of hid_report_len.
If everybody expects the size to be of one more when the report is
unnumbered, maybe we could simply add this placeholder in the report
size from the beginning.

We'd need more testing though that just my gut feeling that it's the
right thing to do.

Cheers,
Benjamin