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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
