Re: [PATCH 1/1] platform/x86/tuxedo: Add virtual LampArray for TUXEDO NB04 devices

[Werner Sembach <wse@tuxedocomputers.com>]

Am 22.10.24 um 11:05 schrieb Benjamin Tissoires:
> Sorry I should have answered earlier...
>
> On Oct 09 2024, Werner Sembach wrote:
>> Resend because HTML mail ..., but I think I now know when Thunderbird does
>> it: Every time I include a link it gets converted.
>>
>> Hi
>>
>> Am 08.10.24 um 17:21 schrieb Benjamin Tissoires:
>>> On Oct 08 2024, Werner Sembach wrote:
>>>> [...]
>>> Yeah, it just means that you can query or send the data. You can also
>>> use HIDIOCGINPUT() and HIDIOCSOUTPUT() to get a current input report and
>>> set an output report through the hidraw ioctl...
>>>
>>> Internally, HIDIOCGINPUT() uses the same code path than
>>> HIDIOCGFEATURE(), but with the report type being an Input instead of a
>>> Feature. Same for HIDIOCSOUTPUT() and HIDIOCSFEATURE().
>> Ok so just a difference in definition not in implementation.
>>
>> Then I use a get feature report for the device status function and use it as
>> input and output at the same time, and use a set output report for the led
>> update function (which technically has a return value but i think it's
>> always 0 anyway).
> not quite. You can not use a get feature to set something on the device.
>
> The semantic is:
> Set -> "write" something on the device (from host to device)
> Get -> "read" something from the device (from device to host)
>
> Features can be set/get.
> Input can only be get.
> Output can only be set.
>
> The implementation in the kernel should enforce that.
>
>> I scoured the old thread about exposing WMI calls to userspace, because I
>> remembered that something here came up already.
>>
>> 1. https://lore.kernel.org/all/6b32fb73-0544-4a68-95ba-e82406a4b188@gmx.de/
>> -> Should be no problem? Because this is not generally exposing wmi calls,
>> just mapping two explicitly with sanitized input (whitelisting basically).
>>
>> 2.
>> https://lore.kernel.org/all/b6d79727-ae94-44b1-aa88-069416435c14@redhat.com/
>> -> Do this concerns this apply here? The actual API to be used is LampArray
>> and the HID mapped WMI calls are just an "internal" interface for the BPF
>> driver, but technically UAPI.
>>
>> Also at Armin and Hans: Do you have comments on this approach?
>>
>>>> (well as far as I can tell the hut doesn't actual specify, if they need to
>>>> be feature reports, or am I missing something?)
>>> They can be both actually. The HUT is missing what's expected here :(.
>>>
>>> However, looking at the HUT RR 84:
>>> https://www.usb.org/sites/default/files/hutrr84_-_lighting_and_illumination_page.pdf
>>>
>>> There is an example of a report descriptor, and they are using Features.
>>> Not Input+Output.
>>>
>>> And looking even further (above), in 3.5 Usage Definitions:
>>> 3.5.2, 3.5.3 and 3.5.5 all of them are meant to be a feature, like:
>>> LampArrayAttributesReport CL – Feature -
>>> LampAttributesRequestReport CL – Feature –
>>> LampAttributesResponseReport CL – Feature –
>>> LampArrayControlReport CL – Feature –
>>>
>>> 3.5.4: can be either feature or output, like:
>>> LampMultiUpdateReport CL – Feature/Output –
>>> LampRangeUpdateReport CL – Feature/ Output –
>>>
>>> So I guess the MS implementation can handle Feature only for all but the
>>> update commands.
>> Thanks for the link, I guess for the BPF driver I will stick to feature
>> reports for the LampArray part until there is actually a hid descriptor
>> spotted in the wild defining LampMultiUpdateReport and LampRangeUpdateReport
>> as Output and not feature.
>>>> and there is the pair with LampAttributesRequestReport and
>>>> LampAttributesResponseReport.
>>> Yeah, not a big deal. The bold IN and OUT are just to say that calling a
>>> setReport on a LampAttributesResponseReport is just ignored AFAIU.
>>>
>>>> Sorry for my confusion over the hid spec.
>>> No worries. It is definitely confusing :)
>> On this note as I fathom:
>>
>> Input Report (usually always get report): Interrupts (the ioctl just there
>> to repeat the last one?)
> yeah, but from hidraw the kernel calls the device directly to query the
> report, so some device don't like that and just hang.
>
> Rule of thumbs: never use get_report on an input report, unless the
> specification explicitely says that the device is supposed to support
> it for the given usage.
>
>> Output Report (usually always set report): Async write, no return value
>> (Buffer should stay untouched)
> yep
>
>> Feature report set: Sync write, no return value (Buffer should stay untouched)
> yep
>
>> Feature report get: Sync read/write (intended only for read, but not limited
>> to it, uses singular buffer for both input and output)
> sync read only, no write. The existing values in the incoming buffer are
> just overwritten.
Sorry I'm still confused: You said i could do input and output in a singular 
feature report, but now you say i can't do input or i can't do output, so i 
still need to use 2?
>
>> I kind of don't get why feature report set exists, but well it's the specs ^^.
> if "feature report set" doesn't exist, you can not write a vlaue to a
> feature on a device (because get doesn't allow you to write).
>
> Anyway, it's a USB implementation detail: input/output are using URB, so
> direct USB read/write, when Features are using the control endpoint,
> which allows for a slightly different approach.
>
> And this transfered as output being async, when features are
> synchronous.
>
> Cheers,
> Benjamin