Message-ID: <zmecavqq5iztb5pkdkk6q52lupolpnufisid4ljydesqh2dtoe@qynhfo3n2i6q>
Date: Tue, 22 Oct 2024 11:05:21 +0200
From: Benjamin Tissoires <bentiss@...nel.org>
To: Werner Sembach <wse@...edocomputers.com>
Cc: Armin Wolf <W_Armin@....de>, Pavel Machek <pavel@....cz>, 
	Hans de Goede <hdegoede@...hat.com>, Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>, 
	dri-devel@...ts.freedesktop.org, jelle@...aa.nl, jikos@...nel.org, lee@...nel.org, 
	linux-input@...r.kernel.org, linux-kernel@...r.kernel.org, linux-leds@...r.kernel.org, 
	miguel.ojeda.sandonis@...il.com, ojeda@...nel.org, onitake@...il.com, 
	platform-driver-x86@...r.kernel.org
Subject: Re: [PATCH 1/1] platform/x86/tuxedo: Add virtual LampArray for
 TUXEDO NB04 devices

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.

> 
> 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

Powered by blists - more mailing lists