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

[Armin Wolf]

Am 09.10.24 um 11:55 schrieb Werner Sembach:

> 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).
>
> 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@xxxxxx/
> -> Should be no problem? Because this is not generally exposing wmi
> calls, just mapping two explicitly with sanitized input (whitelisting
> basically).

It would be OK to expose a selected set of WMI calls to userspace and sanitizing the input of protect potentially buggy firmware from userspace.

> 2.
> https://lore.kernel.org/all/b6d79727-ae94-44b1-aa88-069416435c14@xxxxxxxxxx/
> -> 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.
I see no benefit of using BPF for creating the whole HID reports. Otherwise the HID interface exported by the driver to userspace would be a HID-mapped IOCTL interface
with no real benefit.

I think it would make more sense for the driver to export a generic HID LampArray interface, which contains placeholder values for the dimensions. Those values can then
be supplied by a HID-BPF snipped for each individual machine model. This would indeed be a suitable use of HID-BPF, as this would allow us to omit having a large quirk
table inside the kernel driver.

Regarding the basic idea of having a virtual HID interface: i would prefer to create a illumination subsystem instead, but i have to agree that we should be doing this
only after enough drivers are inside the kernel, so we can design a suitable interface for them. For now, creating a virtual HID interface seems to be good enough.

Thanks,
Armin Wolf

> 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?)
>
> Output Report (usually always set report): Async write, no return
> value (Buffer should stay untouched)
>
> Feature report set: Sync write, no return value (Buffer should stay
> untouched)
>
> Feature report get: Sync read/write (intended only for read, but not
> limited to it, uses singular buffer for both input and output)
>
> I kind of don't get why feature report set exists, but well it's the
> specs ^^.
>
> Regards,
>
> Werner
>
> [*snip*]