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

On Thu, Feb 15, 2018 at 11:16 PM, Rodrigo Rivas Costa
[off-list ref] wrote:

On Wed, Feb 14, 2018 at 03:51:31PM +0100, Benjamin Tissoires wrote:

quoted

On Tue, Feb 13, 2018 at 1:03 PM, Rodrigo Rivas Costa

quoted

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