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

[Russell King]

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