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

[Benjamin Tissoires]

On Fri, Feb 16, 2018 at 9:59 PM, Rodrigo Rivas Costa
<rodrigorivascosta@xxxxxxxxx> wrote:
> On Fri, Feb 16, 2018 at 11:38:11AM +0100, Benjamin Tissoires wrote:
>> 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
>> >>
>>
>> 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.
>
> I've been trying to make sense of all this numbered/buf++/count-- stuff,
> and I admit I don't understand half of it...
>
> But about that +7 in hid_alloc_report_buf(), isn't it to make room for
> the implement()/extract() functions? And IIUIC those are not used for
> raw_requests... they are instead passed directly to usb_control_msg()
> (or whatever the ll driver does). That's the point of being raw, isn't
> it?
>
> If I'm right with that, would it make sense to go back to kzalloc(65)?
>
> If I'm wrong, then if you agree, I'll default to:
>
>         hid_hw_raw_request(steam->hid_dev, 0x00,
>                 buf, hid_report_len(r) + 1, /* count the request number */
>                 HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
>
> Then, if hid_report_len() is ever updated to return the +1, this one
> should be removed. And even if it is not, we still have +6 extra bytes
> from hid_alloc_report_buf(), so no real harm done.

I am fine with your analysis except for the last point :)
We need all 7 extra bytes, not 6 (in implement()). However, as you
said, implement() is not used in the low_level transport functions, so
there is no point bike shedding for ages.

I'd say please stick to hid_report_alloc_buf (maybe add a comment
about the missing report ID added by usb), and use hid_report_len(r) +
1 while calling hid_hw_raw_request(). This way, we can always fix the
behavior later and have something which will not break.

How does that sound?

Cheers,
Benjamin

>
> Regards.
> Rodrigo