Nut upsdev - Feb 2018 - [PATCH 1/3] Skip non-feature HID reports

On Feb 3, 2018, at 7:19 PM, Russell King wrote:
> 
> Input and Output reports are used for interrupt endpoints rather than
> control endpoints.  HIDGetItemData() only ever requests feature
> report IDs, and requesting non-feature report IDs as feature IDs may
> lead to undesirable results and errors.
This one made me scratch my head a bit.

I don't think it's unreasonable to send a control request for an Input
report value-- it seems to be the recommended way for a host to fetch the
initial value (before the device sends updates over the interrupt pipe).

Also, HIDGetItemData() searches the parsed descriptor to match the type as well
as the Usage path, so I'm not sure if I can figure out a way that it would
get an Input report ID confused with a Feature report ID (unless interrupt_only
is set, as it is for powercom devices).

That said, if I am overlooking a case where that might happen, I think we might
need to put this check somewhere deeper in the call tree. HIDDumpTree() is
effectively a NOP if debugging is not enabled, so this is mostly just removing
some partially redundant information from the debug output (redundant in that
most descriptors have both Input and Feature entries for the same Usage IDs).

-- 
Charles Lepple
clepple at gmail

Hi Charles,

Sorry for the long email, I feel this deserves a fuller explanation.

On Wed, Feb 14, 2018 at 10:30:18PM -0500, Charles Lepple
wrote:
> On Feb 3, 2018, at 7:19 PM, Russell King wrote:
> > 
> > Input and Output reports are used for interrupt endpoints rather than
> > control endpoints.  HIDGetItemData() only ever requests feature
> > report IDs, and requesting non-feature report IDs as feature IDs may
> > lead to undesirable results and errors.
> 
> This one made me scratch my head a bit.
> 
> I don't think it's unreasonable to send a control request for an
Input
> report value-- it seems to be the recommended way for a host to fetch
> the initial value (before the device sends updates over the interrupt
> pipe).
Unfortunately, this isn't what is being done - see below.
> Also, HIDGetItemData() searches the parsed descriptor to match the type
> as well as the Usage path, so I'm not sure if I can figure out a way
> that it would get an Input report ID confused with a Feature report ID
> (unless interrupt_only is set, as it is for powercom devices).
libusb_get_report() has:

        ret = usb_control_msg(udev,
                USB_ENDPOINT_IN + USB_TYPE_CLASS + USB_RECIP_INTERFACE,
                0x01, /* HID_REPORT_GET */
                ReportId+(0x03<<8), /* HID_REPORT_TYPE_FEATURE */
                0, raw_buf, ReportSize, USB_TIMEOUT);

Hence, libusb_get_report() always requests the report ID as a feature
report, and never uses the report type that was stored.

This means that when HIDDumpTree() iterates over all reports that were
gathered from in the USB report descriptor, it tries to obtain their
values irrespective of whether they are input, output or feature
reports as if they were all feature reports.

The USB HID specification says:

	Input items define input reports accessible via the Control
	pipe with a Get_Report (Input) request.

which means that, yes, we can read Input items through a Get_Report
request via the control pipe, but it states explicitly that we should
be requesting an Input report, not a Feature report to read them.
> That said, if I am overlooking a case where that might happen, I think
> we might need to put this check somewhere deeper in the call tree.
> HIDDumpTree() is effectively a NOP if debugging is not enabled, so
> this is mostly just removing some partially redundant information from
> the debug output (redundant in that most descriptors have both Input
> and Feature entries for the same Usage IDs).
It may be that some UPSes use the same report ID for both feature and
input items, which would mean that requesting the feature report for
the input item would probably work (provided the offsets in the input
report and feature report are the same), but that is making an
assumption beyond what is specified in the HID specification:

	The same Report ID value can be encountered more than once in
	a report descriptor. Subsequently declared Input, Output, or
	Feature main items will be found in the respective ID/Type
	(Input, Output, or Feature) report.

So, if we want to read input items, then we should at the very least
request them using input reports rather than feature reports.

Now, if you read down in the HID specification to the examples, and
find "B.1 Protocol 1 (Keyboard)" then there's an example of a HID
report descriptor that contains both input and output items, using
the same report ID and the corresponding reports and their lengths.

The descriptor defines items in a single report in the order of
input (8bits), input (8bits), output (5bits), input (8bits).

The corresponding report examples given show that an input report
_only_ contains the input items, and the output report _only_
cotnains the output items.

So, in this case, the first input item would be located in the input
report at data offset 0, and the first output item would similarly be
at data offset 0 in the output report (not the 3rd.)

Requesting an input report and interpreting output items in it makes
no sense, similarly requesting an output report and interpreting
input items also doesn't work (as the output report in this instance
is 8 bytes.)

Hence, this also applies to feature reports: feature reports contain
feature items, and do not contain input or output items.  To read an
input or an output item, the appropriate report type must be
requested even if it has the same report ID as a different report
type.

So, going back to the subject of this patch: since HIDDumpTree()
iterates over all items, requesting their values via HIDGetDataValue(),
and since HIDGetDataValue() is only capable of retrieving Feature
reports, it makes absolutely no sense to pass a non-feature report
to HIDGetDataValue().  Hence, we should skip non-feature reports
in this loop _unless_ HIDGetDataValue() is changed to be capable of
retriving non-feature reports.

Thanks.

-- 
Russell King

On Feb 16, 2018, at 6:55 AM, Russell King wrote:
> 
> Hence, libusb_get_report() always requests the report ID as a feature
> report, and never uses the report type that was stored.
> 
My apologies. I saw the HIDData_t pointer (with its report type field) getting
passed down to HIDGetItemData(), and did not look much further down the tree.
Thank you for your patience in explaining this.

Side note: we have been trying to make sure that all of the libusb code works
with libusb 1.0 as well as 0.1. I will probably merge these commits to master,
which is currently 0.1-only, after doing a quick test merge to the 0.1+1.0
branch, while this discussion is fresh in my mind.
>> That said, if I am overlooking a case where that might happen, I think
>> we might need to put this check somewhere deeper in the call tree.
[snipping out my incorrect assumptions about HIDDumpTree's
callees]
> 
> So, if we want to read input items, then we should at the very least
> request them using input reports rather than feature reports.
I guess this is the part that I didn't want to just paper over without a bit
more explanation.

It is entirely possible for a broken UPS to have different values for the Input
and Feature versions of a given Usage, so that is something I would like the
HIDDumpTree() function to show (even though we currently always fetch the
feature report via EP0). I'll create an issue so we don't forget about
that later, but in the mean time, I might fold in some of this discussion into
the code comments.