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

[Rodrigo Rivas Costa]

On Tue, Feb 20, 2018 at 05:49:02PM +0100, Benjamin Tissoires wrote:
> On Fri, Feb 16, 2018 at 9:59 PM, Rodrigo Rivas Costa
> > 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?
It sound fine to me. I'll try to write a small comment explaining the
+1.

Thanks
Rodrigo

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

[Rodrigo Rivas Costa]

On Tue, Feb 20, 2018 at 05:49:02PM +0100, Benjamin Tissoires wrote:
> On Fri, Feb 16, 2018 at 9:59 PM, Rodrigo Rivas Costa
> > 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?
It sound fine to me. I'll try to write a small comment explaining the
+1.

Thanks
Rodrigo

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

[Benjamin Tissoires]

On Fri, Feb 16, 2018 at 9:59 PM, Rodrigo Rivas Costa
 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
>>  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

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

[Benjamin Tissoires]

On Fri, Feb 16, 2018 at 9:59 PM, Rodrigo Rivas Costa
 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
>>  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

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

[Rodrigo Rivas Costa]

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

Regards.
Rodrigo

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

[Rodrigo Rivas Costa]

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

Regards.
Rodrigo

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

[Benjamin Tissoires]

On Fri, Feb 16, 2018 at 10:57 AM, Rodrigo Rivas Costa
 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

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

[Benjamin Tissoires]

On Fri, Feb 16, 2018 at 10:57 AM, Rodrigo Rivas Costa
 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

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

[Rodrigo Rivas Costa]

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


Regards.
Rodrigo

[1]: http://eleccelerator.com/usbdescreqparser/
D: 0
R: 33 06 00 ff 09 01 a1 01 15 00 26 ff 00 75 08 95 40 09 01 81 02 95 40 09 01 
91 02 95 40 09 01 b1 02 c0
N: Valve Software Wired Controller
P: usb-:00:1d.0-1.2/input2
I: 3 28de 1102
D: 0
E: 0.00 64 01 00 04 0b 06 01 00 00 00 00 00 00 67 14 64 00 00 00 00 00 00 
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
E: 1.007977 64 01 00 04 0b 07 01 00 00 00 00 00 00 67 14 64 00 00 00 00 00 00 
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
E: 1.087971 64 01 00 01 3c 08 01 00 00 00 00 00 00 00 00 00 00 8b f7 9b 02 00 
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
00 00 00 78 01 78 01 3b 02 0c 02 00 00 00 00 67 14
E: 1.095962 64 01 00 01 3c 09 01 00 00 00 00 00 00 00 00 00 00 57 f0 d1 06 00 
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
00 00 00 78 01 78 01 42 02 17 02 00 00 00 00 67 14
E: 1.103966 64 01 00 01 3c 0a 01 00 00 00 00 00 00 00 00 00 00 11 e5 1e 0d 00 
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
00 00 00 78 01 78 01 4f 02 22 02 00 00 00 00 67 14
E: 1.115961 64 01 00 01 3c 0b 01 00 00 00 00 00 00 00 00 00 00 4a d7 c5 1a 00 
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
00 00 00 79 01 78 01 5e 02 3a 02 00 00 00 00 67 14
E: 1.123963 64 01 00 01 3c 0c 01 00 00 00 00 00 00 00 00 00 00 40 cb 24 28 00 
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
00 00 00 79 01 78 01 6c 02 4f 02 00 00 00 00 67 14
E: 1.131962 64 01 00 01 3c 0d 01 00 00 00 00 00 00 00 00 00 00 fc be 92 3e 00 
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
00 00 00 78 01 78 01 7a 02 71 02 00 00 00 00 67 14
E: 1.139954 64 01 00 01 3c 0e 01 00 00 00 00 00 00 00 00 00 00 40 b5 2e 4d 00 
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
00 00 00 78 01 78 01 86 02 86 02 00 00 00 00 67 14
E: 1.151964 64 01 00 01 3c 0f 01 00 00 00 00 00 00 00 00 00 00 3c ac 98 61 00 
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 

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

[Rodrigo Rivas Costa]

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


Regards.
Rodrigo

[1]: http://eleccelerator.com/usbdescreqparser/
D: 0
R: 33 06 00 ff 09 01 a1 01 15 00 26 ff 00 75 08 95 40 09 01 81 02 95 40 09 01 
91 02 95 40 09 01 b1 02 c0
N: Valve Software Wired Controller
P: usb-:00:1d.0-1.2/input2
I: 3 28de 1102
D: 0
E: 0.00 64 01 00 04 0b 06 01 00 00 00 00 00 00 67 14 64 00 00 00 00 00 00 
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
E: 1.007977 64 01 00 04 0b 07 01 00 00 00 00 00 00 67 14 64 00 00 00 00 00 00 
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
E: 1.087971 64 01 00 01 3c 08 01 00 00 00 00 00 00 00 00 00 00 8b f7 9b 02 00 
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
00 00 00 78 01 78 01 3b 02 0c 02 00 00 00 00 67 14
E: 1.095962 64 01 00 01 3c 09 01 00 00 00 00 00 00 00 00 00 00 57 f0 d1 06 00 
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
00 00 00 78 01 78 01 42 02 17 02 00 00 00 00 67 14
E: 1.103966 64 01 00 01 3c 0a 01 00 00 00 00 00 00 00 00 00 00 11 e5 1e 0d 00 
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
00 00 00 78 01 78 01 4f 02 22 02 00 00 00 00 67 14
E: 1.115961 64 01 00 01 3c 0b 01 00 00 00 00 00 00 00 00 00 00 4a d7 c5 1a 00 
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
00 00 00 79 01 78 01 5e 02 3a 02 00 00 00 00 67 14
E: 1.123963 64 01 00 01 3c 0c 01 00 00 00 00 00 00 00 00 00 00 40 cb 24 28 00 
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
00 00 00 79 01 78 01 6c 02 4f 02 00 00 00 00 67 14
E: 1.131962 64 01 00 01 3c 0d 01 00 00 00 00 00 00 00 00 00 00 fc be 92 3e 00 
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
00 00 00 78 01 78 01 7a 02 71 02 00 00 00 00 67 14
E: 1.139954 64 01 00 01 3c 0e 01 00 00 00 00 00 00 00 00 00 00 40 b5 2e 4d 00 
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
00 00 00 78 01 78 01 86 02 86 02 00 00 00 00 67 14
E: 1.151964 64 01 00 01 3c 0f 01 00 00 00 00 00 00 00 00 00 00 3c ac 98 61 00 
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 

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

[Benjamin Tissoires]

On Fri, Feb 16, 2018 at 10:02 AM, Rodrigo Rivas Costa
 wrote:
> On Fri, Feb 16, 2018 at 09:44:34AM +0100, Benjamin Tissoires wrote:
>> > I have an issue with this one. The problem is that using
>> > hid_report_len() on the feature report returns 64. But I must call
>> > hid_hw_raw_request() with 65 or it will fail with EOVERFLOW.
>> >
>> > Currently I'm allocating a buffer of 65 bytes and all is well.
>> > If I change to hid_alloc_report_buf(), the current implementation
>> > allocates (64+7), so I'm still safe. But I'm worried that the extra
>> > bytes are not guaranteed and a future implementation could return
>> > exactly 64 bytes, leaving me 1 byte short.
>> >
>> > About why an array of 65 is required for a report of size 64, I think it
>> > is related to hid_report->id == 0 (so hid_report_enum->numbered == 0).
>>
>> That's the other way around actually. If you are just using the output
>> of hid_report_len(), it will take into account the extra byte for the
>> report ID.
>> *But*, given the way implement() is working (see the comment in the
>> implementation of hid_alloc_report()), you need to have up to 7 extra
>> bytes to not have the EOVERFLOW.
>>
>> So if we ever change the implement() function (which is *really*
>> unlikely), we will have to make sure hid_alloc_report() still works,
>> so you are on the safe side if you use hid_alloc_report().
>
> 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 :)

>
> Maybe there is a bug in hid_hw_raw_request() and it should add 1 to the
> given buffer len? But then, custom buffer allocations will overflow by
> one!

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)?

Cheers,
Benjamin

>
> Rodrigo.

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

[Benjamin Tissoires]

On Fri, Feb 16, 2018 at 10:02 AM, Rodrigo Rivas Costa
 wrote:
> On Fri, Feb 16, 2018 at 09:44:34AM +0100, Benjamin Tissoires wrote:
>> > I have an issue with this one. The problem is that using
>> > hid_report_len() on the feature report returns 64. But I must call
>> > hid_hw_raw_request() with 65 or it will fail with EOVERFLOW.
>> >
>> > Currently I'm allocating a buffer of 65 bytes and all is well.
>> > If I change to hid_alloc_report_buf(), the current implementation
>> > allocates (64+7), so I'm still safe. But I'm worried that the extra
>> > bytes are not guaranteed and a future implementation could return
>> > exactly 64 bytes, leaving me 1 byte short.
>> >
>> > About why an array of 65 is required for a report of size 64, I think it
>> > is related to hid_report->id == 0 (so hid_report_enum->numbered == 0).
>>
>> That's the other way around actually. If you are just using the output
>> of hid_report_len(), it will take into account the extra byte for the
>> report ID.
>> *But*, given the way implement() is working (see the comment in the
>> implementation of hid_alloc_report()), you need to have up to 7 extra
>> bytes to not have the EOVERFLOW.
>>
>> So if we ever change the implement() function (which is *really*
>> unlikely), we will have to make sure hid_alloc_report() still works,
>> so you are on the safe side if you use hid_alloc_report().
>
> 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 :)

>
> Maybe there is a bug in hid_hw_raw_request() and it should add 1 to the
> given buffer len? But then, custom buffer allocations will overflow by
> one!

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)?

Cheers,
Benjamin

>
> Rodrigo.

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

[Rodrigo Rivas Costa]

On Fri, Feb 16, 2018 at 09:44:34AM +0100, Benjamin Tissoires wrote:
> > I have an issue with this one. The problem is that using
> > hid_report_len() on the feature report returns 64. But I must call
> > hid_hw_raw_request() with 65 or it will fail with EOVERFLOW.
> >
> > Currently I'm allocating a buffer of 65 bytes and all is well.
> > If I change to hid_alloc_report_buf(), the current implementation
> > allocates (64+7), so I'm still safe. But I'm worried that the extra
> > bytes are not guaranteed and a future implementation could return
> > exactly 64 bytes, leaving me 1 byte short.
> >
> > About why an array of 65 is required for a report of size 64, I think it
> > is related to hid_report->id == 0 (so hid_report_enum->numbered == 0).
> 
> That's the other way around actually. If you are just using the output
> of hid_report_len(), it will take into account the extra byte for the
> report ID.
> *But*, given the way implement() is working (see the comment in the
> implementation of hid_alloc_report()), you need to have up to 7 extra
> bytes to not have the EOVERFLOW.
> 
> So if we ever change the implement() function (which is *really*
> unlikely), we will have to make sure hid_alloc_report() still works,
> so you are on the safe side if you use hid_alloc_report().

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.

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().

Maybe there is a bug in hid_hw_raw_request() and it should add 1 to the
given buffer len? But then, custom buffer allocations will overflow by
one!

Rodrigo.

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

[Rodrigo Rivas Costa]

On Fri, Feb 16, 2018 at 09:44:34AM +0100, Benjamin Tissoires wrote:
> > I have an issue with this one. The problem is that using
> > hid_report_len() on the feature report returns 64. But I must call
> > hid_hw_raw_request() with 65 or it will fail with EOVERFLOW.
> >
> > Currently I'm allocating a buffer of 65 bytes and all is well.
> > If I change to hid_alloc_report_buf(), the current implementation
> > allocates (64+7), so I'm still safe. But I'm worried that the extra
> > bytes are not guaranteed and a future implementation could return
> > exactly 64 bytes, leaving me 1 byte short.
> >
> > About why an array of 65 is required for a report of size 64, I think it
> > is related to hid_report->id == 0 (so hid_report_enum->numbered == 0).
> 
> That's the other way around actually. If you are just using the output
> of hid_report_len(), it will take into account the extra byte for the
> report ID.
> *But*, given the way implement() is working (see the comment in the
> implementation of hid_alloc_report()), you need to have up to 7 extra
> bytes to not have the EOVERFLOW.
> 
> So if we ever change the implement() function (which is *really*
> unlikely), we will have to make sure hid_alloc_report() still works,
> so you are on the safe side if you use hid_alloc_report().

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.

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().

Maybe there is a bug in hid_hw_raw_request() and it should add 1 to the
given buffer len? But then, custom buffer allocations will overflow by
one!

Rodrigo.

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

[Benjamin Tissoires]

On Thu, Feb 15, 2018 at 11:16 PM, Rodrigo Rivas Costa
 wrote:
> On Wed, Feb 14, 2018 at 03:51:31PM +0100, Benjamin Tissoires wrote:
>> On Tue, Feb 13, 2018 at 1:03 PM, Rodrigo Rivas Costa
>> > +#define STEAM_FEATURE_REPORT_SIZE 65
>> > +
>> > +static int steam_send_report(struct steam_device *steam,
>> > +   u8 *cmd, int size)
>> > +{
>> > +   int retry;
>> > +   int ret;
>> > +   u8 *buf = kzalloc(STEAM_FEATURE_REPORT_SIZE, GFP_KERNEL);
>>
>> Please use hid_alloc_report_buf() as sometimes we need to allocate a
>> slightly bigger report.
>
> I have an issue with this one. The problem is that using
> hid_report_len() on the feature report returns 64. But I must call
> hid_hw_raw_request() with 65 or it will fail with EOVERFLOW.
>
> Currently I'm allocating a buffer of 65 bytes and all is well.
> If I change to hid_alloc_report_buf(), the current implementation
> allocates (64+7), so I'm still safe. But I'm worried that the extra
> bytes are not guaranteed and a future implementation could return
> exactly 64 bytes, leaving me 1 byte short.
>
> About why an array of 65 is required for a report of size 64, I think it
> is related to hid_report->id == 0 (so hid_report_enum->numbered == 0).

That's the other way around actually. If you are just using the output
of hid_report_len(), it will take into account the extra byte for the
report ID.
*But*, given the way implement() is working (see the comment in the
implementation of hid_alloc_report()), you need to have up to 7 extra
bytes to not have the EOVERFLOW.

So if we ever change the implement() function (which is *really*
unlikely), we will have to make sure hid_alloc_report() still works,
so you are on the safe side if you use hid_alloc_report().

>
> So what would be the proper solution?

hid_alloc_report() is the one you want :)
And generally speaking, using the internal API to deal with reports
and others is the safe bet, as you are guaranteed to not being broken
if the API changes (or it'll be a regression and we will have to take
this as high priority).

Cheers,
Benjamin

>
> Thanks.
> Rodrigo.

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

[Benjamin Tissoires]

On Thu, Feb 15, 2018 at 11:16 PM, Rodrigo Rivas Costa
 wrote:
> On Wed, Feb 14, 2018 at 03:51:31PM +0100, Benjamin Tissoires wrote:
>> On Tue, Feb 13, 2018 at 1:03 PM, Rodrigo Rivas Costa
>> > +#define STEAM_FEATURE_REPORT_SIZE 65
>> > +
>> > +static int steam_send_report(struct steam_device *steam,
>> > +   u8 *cmd, int size)
>> > +{
>> > +   int retry;
>> > +   int ret;
>> > +   u8 *buf = kzalloc(STEAM_FEATURE_REPORT_SIZE, GFP_KERNEL);
>>
>> Please use hid_alloc_report_buf() as sometimes we need to allocate a
>> slightly bigger report.
>
> I have an issue with this one. The problem is that using
> hid_report_len() on the feature report returns 64. But I must call
> hid_hw_raw_request() with 65 or it will fail with EOVERFLOW.
>
> Currently I'm allocating a buffer of 65 bytes and all is well.
> If I change to hid_alloc_report_buf(), the current implementation
> allocates (64+7), so I'm still safe. But I'm worried that the extra
> bytes are not guaranteed and a future implementation could return
> exactly 64 bytes, leaving me 1 byte short.
>
> About why an array of 65 is required for a report of size 64, I think it
> is related to hid_report->id == 0 (so hid_report_enum->numbered == 0).

That's the other way around actually. If you are just using the output
of hid_report_len(), it will take into account the extra byte for the
report ID.
*But*, given the way implement() is working (see the comment in the
implementation of hid_alloc_report()), you need to have up to 7 extra
bytes to not have the EOVERFLOW.

So if we ever change the implement() function (which is *really*
unlikely), we will have to make sure hid_alloc_report() still works,
so you are on the safe side if you use hid_alloc_report().

>
> So what would be the proper solution?

hid_alloc_report() is the one you want :)
And generally speaking, using the internal API to deal with reports
and others is the safe bet, as you are guaranteed to not being broken
if the API changes (or it'll be a regression and we will have to take
this as high priority).

Cheers,
Benjamin

>
> Thanks.
> Rodrigo.

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

[Rodrigo Rivas Costa]

On Wed, Feb 14, 2018 at 03:51:31PM +0100, Benjamin Tissoires wrote:
> On Tue, Feb 13, 2018 at 1:03 PM, Rodrigo Rivas Costa
> > +#define STEAM_FEATURE_REPORT_SIZE 65
> > +
> > +static int steam_send_report(struct steam_device *steam,
> > +   u8 *cmd, int size)
> > +{
> > +   int retry;
> > +   int ret;
> > +   u8 *buf = kzalloc(STEAM_FEATURE_REPORT_SIZE, GFP_KERNEL);
> 
> Please use hid_alloc_report_buf() as sometimes we need to allocate a
> slightly bigger report.

I have an issue with this one. The problem is that using
hid_report_len() on the feature report returns 64. But I must call
hid_hw_raw_request() with 65 or it will fail with EOVERFLOW.

Currently I'm allocating a buffer of 65 bytes and all is well.
If I change to hid_alloc_report_buf(), the current implementation
allocates (64+7), so I'm still safe. But I'm worried that the extra
bytes are not guaranteed and a future implementation could return
exactly 64 bytes, leaving me 1 byte short.

About why an array of 65 is required for a report of size 64, I think it
is related to hid_report->id == 0 (so hid_report_enum->numbered == 0).

So what would be the proper solution?

Thanks.
Rodrigo.

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

[Rodrigo Rivas Costa]

On Wed, Feb 14, 2018 at 03:51:31PM +0100, Benjamin Tissoires wrote:
> On Tue, Feb 13, 2018 at 1:03 PM, Rodrigo Rivas Costa
> > +#define STEAM_FEATURE_REPORT_SIZE 65
> > +
> > +static int steam_send_report(struct steam_device *steam,
> > +   u8 *cmd, int size)
> > +{
> > +   int retry;
> > +   int ret;
> > +   u8 *buf = kzalloc(STEAM_FEATURE_REPORT_SIZE, GFP_KERNEL);
> 
> Please use hid_alloc_report_buf() as sometimes we need to allocate a
> slightly bigger report.

I have an issue with this one. The problem is that using
hid_report_len() on the feature report returns 64. But I must call
hid_hw_raw_request() with 65 or it will fail with EOVERFLOW.

Currently I'm allocating a buffer of 65 bytes and all is well.
If I change to hid_alloc_report_buf(), the current implementation
allocates (64+7), so I'm still safe. But I'm worried that the extra
bytes are not guaranteed and a future implementation could return
exactly 64 bytes, leaving me 1 byte short.

About why an array of 65 is required for a report of size 64, I think it
is related to hid_report->id == 0 (so hid_report_enum->numbered == 0).

So what would be the proper solution?

Thanks.
Rodrigo.

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

[Benjamin Tissoires]

On Tue, Feb 13, 2018 at 1:03 PM, Rodrigo Rivas Costa
 wrote:
> This device has a feature report to send and receive commands.
> Use it to get the serial number and set the device's uniq value.
>
> Signed-off-by: Rodrigo Rivas Costa 
> ---
>  drivers/hid/hid-steam.c | 89 
> +++--
>  1 file changed, 86 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> index 03f912ab5484..f269a986e2be 100644
> --- a/drivers/hid/hid-steam.c
> +++ b/drivers/hid/hid-steam.c
> @@ -34,12 +34,17 @@ struct steam_device {
> unsigned long quirks;
> struct work_struct work_connect;
> bool connected;
> +   char serial_no[11];
>  };
>
>  static int steam_register(struct steam_device *steam);
>  static void steam_unregister(struct steam_device *steam);
>  static void steam_do_connect_event(struct steam_device *steam, bool 
> connected);
>  static void steam_do_input_event(struct steam_device *steam, u8 *data);
> +static int steam_send_report(struct steam_device *steam,
> +   u8 *cmd, int size);
> +static int steam_recv_report(struct steam_device *steam,
> +   u8 *data, int size);
>
>  static int steam_input_open(struct input_dev *dev)
>  {
> @@ -55,6 +60,78 @@ static void steam_input_close(struct input_dev *dev)
> hid_hw_close(steam->hid_dev);
>  }
>
> +#define STEAM_FEATURE_REPORT_SIZE 65
> +
> +static int steam_send_report(struct steam_device *steam,
> +   u8 *cmd, int size)
> +{
> +   int retry;
> +   int ret;
> +   u8 *buf = kzalloc(STEAM_FEATURE_REPORT_SIZE, GFP_KERNEL);

Please use hid_alloc_report_buf() as sometimes we need to allocate a
slightly bigger report.

> +
> +   if (!buf)
> +   return -ENOMEM;
> +
> +   /* The report ID is always 0 */
> +   memcpy(buf + 1, cmd, size);
> +
> +   /* Sometimes the wireless controller fails with EPIPE
> +* when sending a feature report.
> +* Doing a HID_REQ_GET_REPORT and waiting for a while
> +* seems to fix that.
> +*/
> +   for (retry = 0; retry < 10; ++retry) {
> +   ret = hid_hw_raw_request(steam->hid_dev, 0,
> +   buf, size + 1,
> +   HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
> +   if (ret != -EPIPE)
> +   break;
> +   dbg_hid("%s: failed, retrying (%d times)\n", __func__, 
> retry+1);
> +   steam_recv_report(steam, NULL, 0);
> +   msleep(50);
> +   }
> +   kfree(buf);
> +   return ret;
> +}
> +
> +static int steam_recv_report(struct steam_device *steam,
> +   u8 *data, int size)
> +{
> +   int ret;
> +   u8 *buf = kzalloc(STEAM_FEATURE_REPORT_SIZE, GFP_KERNEL);

same her hid_alloc_report_buf should be used

> +
> +   if (!buf)
> +   return -ENOMEM;
> +
> +   /* The report ID is always 0 */
> +   ret = hid_hw_raw_request(steam->hid_dev, 0x00,
> +   buf, STEAM_FEATURE_REPORT_SIZE,
> +   HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
> +   memcpy(data, buf + 1, size);
> +   kfree(buf);
> +   return ret;
> +}
> +
> +static int steam_get_serial(struct steam_device *steam)
> +{
> +   /* Send: 0xae 0x15 0x01
> +* Recv: 0xae 0x15 0x01 serialnumber (10 chars)
> +*/
> +   int ret;
> +   u8 cmd[] = {0xae, 0x15, 0x01};
> +   u8 reply[14];
> +
> +   ret = steam_send_report(steam, cmd, sizeof(cmd));
> +   if (ret < 0)
> +   return ret;
> +   ret = steam_recv_report(steam, reply, sizeof(reply));
> +   if (ret < 0)
> +   return ret;
> +   reply[13] = 0;
> +   strcpy(steam->serial_no, reply + 3);
> +   return 0;
> +}
> +
>  static void steam_work_connect_cb(struct work_struct *work)
>  {
> struct steam_device *steam = container_of(work, struct steam_device,
> @@ -385,7 +462,12 @@ static int steam_register(struct steam_device *steam)
>
> dbg_hid("%s\n", __func__);
>
> -   hid_info(hdev, "Steam Controller connected");
> +   ret = steam_get_serial(steam);
> +   if (ret)
> +   return ret;
> +
> +   hid_info(hdev, "Steam Controller SN: '%s' connected",
> +   steam->serial_no);
>
> input = input_allocate_device();
> if (!input)
> @@ -398,7 +480,7 @@ static int steam_register(struct steam_device *steam)
>
> input->name = "Steam Controller";
> input->phys = hdev->phys;
> -   input->uniq = hdev->uniq;
> +   input->uniq = steam->serial_no;
> input->id.bustype = hdev->bus;
> input->id.vendor  = hdev->vendor;
> input->id.product = hdev->product;
> @@ -447,7 +529,8 @@ static void steam_unregister(struct steam_device *steam)
> dbg_hid("%s\n", __func__);
>
> if 

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

[Benjamin Tissoires]

On Tue, Feb 13, 2018 at 1:03 PM, Rodrigo Rivas Costa
 wrote:
> This device has a feature report to send and receive commands.
> Use it to get the serial number and set the device's uniq value.
>
> Signed-off-by: Rodrigo Rivas Costa 
> ---
>  drivers/hid/hid-steam.c | 89 
> +++--
>  1 file changed, 86 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> index 03f912ab5484..f269a986e2be 100644
> --- a/drivers/hid/hid-steam.c
> +++ b/drivers/hid/hid-steam.c
> @@ -34,12 +34,17 @@ struct steam_device {
> unsigned long quirks;
> struct work_struct work_connect;
> bool connected;
> +   char serial_no[11];
>  };
>
>  static int steam_register(struct steam_device *steam);
>  static void steam_unregister(struct steam_device *steam);
>  static void steam_do_connect_event(struct steam_device *steam, bool 
> connected);
>  static void steam_do_input_event(struct steam_device *steam, u8 *data);
> +static int steam_send_report(struct steam_device *steam,
> +   u8 *cmd, int size);
> +static int steam_recv_report(struct steam_device *steam,
> +   u8 *data, int size);
>
>  static int steam_input_open(struct input_dev *dev)
>  {
> @@ -55,6 +60,78 @@ static void steam_input_close(struct input_dev *dev)
> hid_hw_close(steam->hid_dev);
>  }
>
> +#define STEAM_FEATURE_REPORT_SIZE 65
> +
> +static int steam_send_report(struct steam_device *steam,
> +   u8 *cmd, int size)
> +{
> +   int retry;
> +   int ret;
> +   u8 *buf = kzalloc(STEAM_FEATURE_REPORT_SIZE, GFP_KERNEL);

Please use hid_alloc_report_buf() as sometimes we need to allocate a
slightly bigger report.

> +
> +   if (!buf)
> +   return -ENOMEM;
> +
> +   /* The report ID is always 0 */
> +   memcpy(buf + 1, cmd, size);
> +
> +   /* Sometimes the wireless controller fails with EPIPE
> +* when sending a feature report.
> +* Doing a HID_REQ_GET_REPORT and waiting for a while
> +* seems to fix that.
> +*/
> +   for (retry = 0; retry < 10; ++retry) {
> +   ret = hid_hw_raw_request(steam->hid_dev, 0,
> +   buf, size + 1,
> +   HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
> +   if (ret != -EPIPE)
> +   break;
> +   dbg_hid("%s: failed, retrying (%d times)\n", __func__, 
> retry+1);
> +   steam_recv_report(steam, NULL, 0);
> +   msleep(50);
> +   }
> +   kfree(buf);
> +   return ret;
> +}
> +
> +static int steam_recv_report(struct steam_device *steam,
> +   u8 *data, int size)
> +{
> +   int ret;
> +   u8 *buf = kzalloc(STEAM_FEATURE_REPORT_SIZE, GFP_KERNEL);

same her hid_alloc_report_buf should be used

> +
> +   if (!buf)
> +   return -ENOMEM;
> +
> +   /* The report ID is always 0 */
> +   ret = hid_hw_raw_request(steam->hid_dev, 0x00,
> +   buf, STEAM_FEATURE_REPORT_SIZE,
> +   HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
> +   memcpy(data, buf + 1, size);
> +   kfree(buf);
> +   return ret;
> +}
> +
> +static int steam_get_serial(struct steam_device *steam)
> +{
> +   /* Send: 0xae 0x15 0x01
> +* Recv: 0xae 0x15 0x01 serialnumber (10 chars)
> +*/
> +   int ret;
> +   u8 cmd[] = {0xae, 0x15, 0x01};
> +   u8 reply[14];
> +
> +   ret = steam_send_report(steam, cmd, sizeof(cmd));
> +   if (ret < 0)
> +   return ret;
> +   ret = steam_recv_report(steam, reply, sizeof(reply));
> +   if (ret < 0)
> +   return ret;
> +   reply[13] = 0;
> +   strcpy(steam->serial_no, reply + 3);
> +   return 0;
> +}
> +
>  static void steam_work_connect_cb(struct work_struct *work)
>  {
> struct steam_device *steam = container_of(work, struct steam_device,
> @@ -385,7 +462,12 @@ static int steam_register(struct steam_device *steam)
>
> dbg_hid("%s\n", __func__);
>
> -   hid_info(hdev, "Steam Controller connected");
> +   ret = steam_get_serial(steam);
> +   if (ret)
> +   return ret;
> +
> +   hid_info(hdev, "Steam Controller SN: '%s' connected",
> +   steam->serial_no);
>
> input = input_allocate_device();
> if (!input)
> @@ -398,7 +480,7 @@ static int steam_register(struct steam_device *steam)
>
> input->name = "Steam Controller";
> input->phys = hdev->phys;
> -   input->uniq = hdev->uniq;
> +   input->uniq = steam->serial_no;
> input->id.bustype = hdev->bus;
> input->id.vendor  = hdev->vendor;
> input->id.product = hdev->product;
> @@ -447,7 +529,8 @@ static void steam_unregister(struct steam_device *steam)
> dbg_hid("%s\n", __func__);
>
> if (steam->input_dev) {
> -   hid_info(steam->hid_dev, 

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

[Rodrigo Rivas Costa]

This device has a feature report to send and receive commands.
Use it to get the serial number and set the device's uniq value.

Signed-off-by: Rodrigo Rivas Costa 
---
 drivers/hid/hid-steam.c | 89 +++--
 1 file changed, 86 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
index 03f912ab5484..f269a986e2be 100644
--- a/drivers/hid/hid-steam.c
+++ b/drivers/hid/hid-steam.c
@@ -34,12 +34,17 @@ struct steam_device {
unsigned long quirks;
struct work_struct work_connect;
bool connected;
+   char serial_no[11];
 };
 
 static int steam_register(struct steam_device *steam);
 static void steam_unregister(struct steam_device *steam);
 static void steam_do_connect_event(struct steam_device *steam, bool connected);
 static void steam_do_input_event(struct steam_device *steam, u8 *data);
+static int steam_send_report(struct steam_device *steam,
+   u8 *cmd, int size);
+static int steam_recv_report(struct steam_device *steam,
+   u8 *data, int size);
 
 static int steam_input_open(struct input_dev *dev)
 {
@@ -55,6 +60,78 @@ static void steam_input_close(struct input_dev *dev)
hid_hw_close(steam->hid_dev);
 }
 
+#define STEAM_FEATURE_REPORT_SIZE 65
+
+static int steam_send_report(struct steam_device *steam,
+   u8 *cmd, int size)
+{
+   int retry;
+   int ret;
+   u8 *buf = kzalloc(STEAM_FEATURE_REPORT_SIZE, GFP_KERNEL);
+
+   if (!buf)
+   return -ENOMEM;
+
+   /* The report ID is always 0 */
+   memcpy(buf + 1, cmd, size);
+
+   /* Sometimes the wireless controller fails with EPIPE
+* when sending a feature report.
+* Doing a HID_REQ_GET_REPORT and waiting for a while
+* seems to fix that.
+*/
+   for (retry = 0; retry < 10; ++retry) {
+   ret = hid_hw_raw_request(steam->hid_dev, 0,
+   buf, size + 1,
+   HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
+   if (ret != -EPIPE)
+   break;
+   dbg_hid("%s: failed, retrying (%d times)\n", __func__, retry+1);
+   steam_recv_report(steam, NULL, 0);
+   msleep(50);
+   }
+   kfree(buf);
+   return ret;
+}
+
+static int steam_recv_report(struct steam_device *steam,
+   u8 *data, int size)
+{
+   int ret;
+   u8 *buf = kzalloc(STEAM_FEATURE_REPORT_SIZE, GFP_KERNEL);
+
+   if (!buf)
+   return -ENOMEM;
+
+   /* The report ID is always 0 */
+   ret = hid_hw_raw_request(steam->hid_dev, 0x00,
+   buf, STEAM_FEATURE_REPORT_SIZE,
+   HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
+   memcpy(data, buf + 1, size);
+   kfree(buf);
+   return ret;
+}
+
+static int steam_get_serial(struct steam_device *steam)
+{
+   /* Send: 0xae 0x15 0x01
+* Recv: 0xae 0x15 0x01 serialnumber (10 chars)
+*/
+   int ret;
+   u8 cmd[] = {0xae, 0x15, 0x01};
+   u8 reply[14];
+
+   ret = steam_send_report(steam, cmd, sizeof(cmd));
+   if (ret < 0)
+   return ret;
+   ret = steam_recv_report(steam, reply, sizeof(reply));
+   if (ret < 0)
+   return ret;
+   reply[13] = 0;
+   strcpy(steam->serial_no, reply + 3);
+   return 0;
+}
+
 static void steam_work_connect_cb(struct work_struct *work)
 {
struct steam_device *steam = container_of(work, struct steam_device,
@@ -385,7 +462,12 @@ static int steam_register(struct steam_device *steam)
 
dbg_hid("%s\n", __func__);
 
-   hid_info(hdev, "Steam Controller connected");
+   ret = steam_get_serial(steam);
+   if (ret)
+   return ret;
+
+   hid_info(hdev, "Steam Controller SN: '%s' connected",
+   steam->serial_no);
 
input = input_allocate_device();
if (!input)
@@ -398,7 +480,7 @@ static int steam_register(struct steam_device *steam)
 
input->name = "Steam Controller";
input->phys = hdev->phys;
-   input->uniq = hdev->uniq;
+   input->uniq = steam->serial_no;
input->id.bustype = hdev->bus;
input->id.vendor  = hdev->vendor;
input->id.product = hdev->product;
@@ -447,7 +529,8 @@ static void steam_unregister(struct steam_device *steam)
dbg_hid("%s\n", __func__);
 
if (steam->input_dev) {
-   hid_info(steam->hid_dev, "Steam Controller disconnected");
+   hid_info(steam->hid_dev, "Steam Controller SN: '%s' 
disconnected",
+   steam->serial_no);
input_unregister_device(steam->input_dev);
steam->input_dev = NULL;
}
-- 
2.16.1

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

[Rodrigo Rivas Costa]

This device has a feature report to send and receive commands.
Use it to get the serial number and set the device's uniq value.

Signed-off-by: Rodrigo Rivas Costa 
---
 drivers/hid/hid-steam.c | 89 +++--
 1 file changed, 86 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
index 03f912ab5484..f269a986e2be 100644
--- a/drivers/hid/hid-steam.c
+++ b/drivers/hid/hid-steam.c
@@ -34,12 +34,17 @@ struct steam_device {
unsigned long quirks;
struct work_struct work_connect;
bool connected;
+   char serial_no[11];
 };
 
 static int steam_register(struct steam_device *steam);
 static void steam_unregister(struct steam_device *steam);
 static void steam_do_connect_event(struct steam_device *steam, bool connected);
 static void steam_do_input_event(struct steam_device *steam, u8 *data);
+static int steam_send_report(struct steam_device *steam,
+   u8 *cmd, int size);
+static int steam_recv_report(struct steam_device *steam,
+   u8 *data, int size);
 
 static int steam_input_open(struct input_dev *dev)
 {
@@ -55,6 +60,78 @@ static void steam_input_close(struct input_dev *dev)
hid_hw_close(steam->hid_dev);
 }
 
+#define STEAM_FEATURE_REPORT_SIZE 65
+
+static int steam_send_report(struct steam_device *steam,
+   u8 *cmd, int size)
+{
+   int retry;
+   int ret;
+   u8 *buf = kzalloc(STEAM_FEATURE_REPORT_SIZE, GFP_KERNEL);
+
+   if (!buf)
+   return -ENOMEM;
+
+   /* The report ID is always 0 */
+   memcpy(buf + 1, cmd, size);
+
+   /* Sometimes the wireless controller fails with EPIPE
+* when sending a feature report.
+* Doing a HID_REQ_GET_REPORT and waiting for a while
+* seems to fix that.
+*/
+   for (retry = 0; retry < 10; ++retry) {
+   ret = hid_hw_raw_request(steam->hid_dev, 0,
+   buf, size + 1,
+   HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
+   if (ret != -EPIPE)
+   break;
+   dbg_hid("%s: failed, retrying (%d times)\n", __func__, retry+1);
+   steam_recv_report(steam, NULL, 0);
+   msleep(50);
+   }
+   kfree(buf);
+   return ret;
+}
+
+static int steam_recv_report(struct steam_device *steam,
+   u8 *data, int size)
+{
+   int ret;
+   u8 *buf = kzalloc(STEAM_FEATURE_REPORT_SIZE, GFP_KERNEL);
+
+   if (!buf)
+   return -ENOMEM;
+
+   /* The report ID is always 0 */
+   ret = hid_hw_raw_request(steam->hid_dev, 0x00,
+   buf, STEAM_FEATURE_REPORT_SIZE,
+   HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
+   memcpy(data, buf + 1, size);
+   kfree(buf);
+   return ret;
+}
+
+static int steam_get_serial(struct steam_device *steam)
+{
+   /* Send: 0xae 0x15 0x01
+* Recv: 0xae 0x15 0x01 serialnumber (10 chars)
+*/
+   int ret;
+   u8 cmd[] = {0xae, 0x15, 0x01};
+   u8 reply[14];
+
+   ret = steam_send_report(steam, cmd, sizeof(cmd));
+   if (ret < 0)
+   return ret;
+   ret = steam_recv_report(steam, reply, sizeof(reply));
+   if (ret < 0)
+   return ret;
+   reply[13] = 0;
+   strcpy(steam->serial_no, reply + 3);
+   return 0;
+}
+
 static void steam_work_connect_cb(struct work_struct *work)
 {
struct steam_device *steam = container_of(work, struct steam_device,
@@ -385,7 +462,12 @@ static int steam_register(struct steam_device *steam)
 
dbg_hid("%s\n", __func__);
 
-   hid_info(hdev, "Steam Controller connected");
+   ret = steam_get_serial(steam);
+   if (ret)
+   return ret;
+
+   hid_info(hdev, "Steam Controller SN: '%s' connected",
+   steam->serial_no);
 
input = input_allocate_device();
if (!input)
@@ -398,7 +480,7 @@ static int steam_register(struct steam_device *steam)
 
input->name = "Steam Controller";
input->phys = hdev->phys;
-   input->uniq = hdev->uniq;
+   input->uniq = steam->serial_no;
input->id.bustype = hdev->bus;
input->id.vendor  = hdev->vendor;
input->id.product = hdev->product;
@@ -447,7 +529,8 @@ static void steam_unregister(struct steam_device *steam)
dbg_hid("%s\n", __func__);
 
if (steam->input_dev) {
-   hid_info(steam->hid_dev, "Steam Controller disconnected");
+   hid_info(steam->hid_dev, "Steam Controller SN: '%s' 
disconnected",
+   steam->serial_no);
input_unregister_device(steam->input_dev);
steam->input_dev = NULL;
}
-- 
2.16.1