| Message ID | 20180213120308.23879-2-rodrigorivascosta@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Feb 13, 2018 at 1:03 PM, Rodrigo Rivas Costa <rodrigorivascosta@gmail.com> 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 <rodrigorivascosta@gmail.com> > --- > 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, "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 > Rest is OK (minus the multi lines comments as pointed in 1/3) Cheers, Benjamin -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Feb 15, 2018 at 11:16 PM, Rodrigo Rivas Costa <rodrigorivascosta@gmail.com> 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. -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Feb 16, 2018 at 10:02 AM, Rodrigo Rivas Costa <rodrigorivascosta@gmail.com> 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. -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-0000:00:1d.0-1.2/input2 I: 3 28de 1102 D: 0 E: 0.000000 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 00 00 00 00 00 00 00 00 00 00 78 01 78 01 91 02 a3 02 00 00 00 00 67 14 E: 1.159960 64 01 00 01 3c 10 01 00 00 00 00 00 00 00 00 00 00 72 a0 6b 76 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 79 01 a0 02 c0 02 00 00 00 00 67 14 E: 1.167958 64 01 00 01 3c 11 01 00 00 00 00 00 00 00 00 00 00 26 94 ff 7f 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 b0 02 d8 02 00 00 00 00 67 14 E: 1.175961 64 01 00 01 3c 12 01 00 00 00 00 00 00 00 00 00 00 9d 92 ff 7f 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 b2 02 f8 02 00 00 00 00 67 14 E: 1.239961 64 01 00 01 3c 13 01 00 00 00 00 00 00 00 00 00 00 62 93 ff 7f 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 b1 02 fc 02 00 00 00 00 67 14 E: 1.247958 64 01 00 01 3c 14 01 00 00 00 00 00 00 00 00 00 00 e9 94 ff 7f 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 af 02 01 03 00 00 00 00 67 14 E: 1.259967 64 01 00 01 3c 15 01 00 00 00 00 00 00 00 00 00 00 40 a0 ff 7f 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 a0 02 05 03 00 00 00 00 67 14 E: 1.267959 64 01 00 01 3c 16 01 00 00 00 00 00 00 00 00 00 00 cb ac ff 7f 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 8f 02 0f 03 00 00 00 00 67 14 E: 1.275960 64 01 00 01 3c 17 01 00 00 00 00 00 00 00 00 00 00 f5 b8 ff 7f 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 79 01 7e 02 14 03 00 00 00 00 67 14 E: 1.283957 64 01 00 01 3c 18 01 00 00 00 00 00 00 00 00 00 00 ba ca ff 7f 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 77 01 78 01 64 02 1c 03 00 00 00 00 67 14 E: 1.295958 64 01 00 01 3c 19 01 00 00 00 00 00 00 00 00 00 00 de e0 ff 7f 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 41 02 27 03 00 00 00 00 67 14 E: 1.303943 64 01 00 01 3c 1a 01 00 00 00 00 00 00 00 00 00 00 0b f1 ff 7f 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 25 02 2c 03 00 00 00 00 67 14 E: 1.311956 64 01 00 01 3c 1b 01 00 00 00 00 00 00 00 00 00 00 11 02 ff 7f 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 ff 01 2e 03 00 00 00 00 67 14 E: 1.319957 64 01 00 01 3c 1c 01 00 00 00 00 00 00 00 00 00 00 bf 11 ff 7f 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 e2 01 2e 03 00 00 00 00 67 14 E: 1.331959 64 01 00 01 3c 1d 01 00 00 00 00 00 00 00 00 00 00 2b 22 ff 7f 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 c6 01 2c 03 00 00 00 00 67 14 E: 1.339957 64 01 00 01 3c 1e 01 00 00 00 00 00 00 00 00 00 00 fa 31 ff 7f 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 ad 01 27 03 00 00 00 00 67 14 E: 1.347956 64 01 00 01 3c 1f 01 00 00 00 00 00 00 00 00 00 00 28 42 ff 7f 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 95 01 1d 03 00 00 00 00 67 14 E: 1.355970 64 01 00 01 3c 20 01 00 00 00 00 00 00 00 00 00 00 97 4f ff 7f 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 82 01 15 03 00 00 00 00 67 14 E: 1.367958 64 01 00 01 3c 21 01 00 00 00 00 00 00 00 00 00 00 f2 61 f0 7d 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 69 01 0f 03 00 00 00 00 67 14 E: 1.375951 64 01 00 01 3c 22 01 00 00 00 00 00 00 00 00 00 00 a0 6f 84 78 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 57 01 05 03 00 00 00 00 67 14 E: 1.383956 64 01 00 01 3c 23 01 00 00 00 00 00 00 00 00 00 00 c1 79 af 72 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 4a 01 fa 02 00 00 00 00 67 14 E: 1.391957 64 01 00 01 3c 24 01 00 00 00 00 00 00 00 00 00 00 ff 7f e8 6b 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 35 01 ed 02 00 00 00 00 67 14 E: 1.403955 64 01 00 01 3c 25 01 00 00 00 00 00 00 00 00 00 00 ff 7f 99 62 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 26 01 db 02 00 00 00 00 67 14 E: 1.411955 64 01 00 01 3c 26 01 00 00 00 00 00 00 00 00 00 00 ff 7f 72 59 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 1b 01 c9 02 00 00 00 00 67 14 E: 1.419958 64 01 00 01 3c 27 01 00 00 00 00 00 00 00 00 00 00 ff 7f fe 4f 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 0b 01 b6 02 00 00 00 00 67 14 E: 1.427953 64 01 00 01 3c 28 01 00 00 00 00 00 00 00 00 00 00 ff 7f cf 45 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 79 01 fa 00 a1 02 00 00 00 00 67 14
On Fri, Feb 16, 2018 at 10:57 AM, Rodrigo Rivas Costa <rodrigorivascosta@gmail.com> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@gmail.com> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Feb 16, 2018 at 9:59 PM, Rodrigo Rivas Costa <rodrigorivascosta@gmail.com> 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@gmail.com> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; }
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 <rodrigorivascosta@gmail.com> --- drivers/hid/hid-steam.c | 89 +++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 86 insertions(+), 3 deletions(-)