HID: ayaneo: Add AYANEO 3 detachable controller driver

[matmartinez]

Submitting here for review prior to LKML, per @pastaq in ShadowBlip/OpenGamepadUI#528.

Same commit as OpenGamingCollective/linux#101 (rebased onto this master; happy to close whichever of the two is redundant — guidance welcome on how these flow together).

What it does

Driver for the AYANEO 3 detachable controller ("Magic Modules") vendor HID interface (1c4f:0002, application usage 0xff000001; DMI-gated to the AYANEO 3 since the VID/PID is a generic SigmaMicro ID):

• module_left / module_right sysfs attrs — raw firmware module-type IDs (bits 0–5 type, bit 6 rotated)
• eject sysfs attr (left/right/both) — blocks until the firmware confirms the release handshake
• reset — quick controller config reset
• multicolor LED class device ayaneo:rgb:joystick_rings (the name InputPlumber's 50-ayaneo_3.yaml already expects)

EC power-off is deliberately left to userspace (write 0 to ayaneo-ec's controller_power after eject returns) so orchestration/UX stays in the OpenGamepadUI layer. Protocol reverse engineered in Handheld Daemon by Antheas Kapenekakis (he'll be CC'd on the LKML series). Includes Documentation/ABI/ and MAINTAINERS entries; checkpatch --strict clean except the standard -ENOSYS output-report-fallback false positive.

Testing

On an AYANEO 3 / Bazzite 44 (OGC 7.2.0-ogc4.1): probe identifies modules (left 0x04 right 0x50), RGB via LED class verified, and full physical eject → power-off → release → reinsert → repower → re-enumeration → rebind cycles, both from the shell and driven by a working OpenGamepadUI quick-bar plugin (see ShadowBlip/OpenGamepadUI#528). Community testing guide: https://github.com/matmartinez/ayaneo-3-bazzite-compat/blob/main/TESTING.md

[NeroReflex]

here I would use scoped_guard to spare the line of mutex_unlock

[NeroReflex]

kernel practice is not to print anything when things go as planned

[NeroReflex]

I have rarely seen hid devices requiring an entry in MAINTAINERS, are you sure?

[pastaq]

It would only be used for people to send patches, and the module author is most likely to be the one to test it. I see no issues here.

[NeroReflex]

I think this function deserves a kernel-doc also explaining arguments and the locking

[NeroReflex]

personally I would use a fixed-width type here, like u32 or u64 depending on if the result would fit the u32.... Maybe a u16 can also work? Especially since you then use put_unaligned_le16

[NeroReflex]

I can guarantee you sahiko-bot is going to cry over this with a bunch of "what if user uses sysfs attributes while a remove is started?"

[NeroReflex]

I would do here u8 mode = (led_on_condition) ? AYA3_RGB_SOLID : AYA3_RGB_OFF; and spare the next two lines.

[NeroReflex]

I think I would do it the other way around: create a const u8 buf[SIZE] = {}; that will be zero-filled automatically on unspecified elements and then copy that to the dma buffer.

[NeroReflex]

Since on error IDK what happens I would use a hid_err here in case ret has unexpected values

[NeroReflex]

sahiko-bot is going to complain about the name with a "what if an aya3 spoofed device is being emulated?". I would suggest doing what hid-asus does and compose this name with a dynamic part.

[matmartinez]

Thanks for the thorough review @NeroReflex! All addressed, I pushed each point as a separate commit for easy re-review (I'll squash everything back into the single patch before this goes to LKML):

• scoped_guard in probe → done.
• No print on success → done; dropped the hid_info() and the now-unused response buffer, kept only the warning path.
• kernel-doc for aya3_cmd() → done; documents the arguments, the subcommand-echo reply matching, and the locking.
• Fixed-width checksum → done; u16 — the sum of the 58 payload bytes maxes out at 14790, and the device consumes it as LE16 anyway.
• Ternary for the RGB mode → done.
• Zero-filled template for the config command → done; static const u8 template[] with designated initializers, memcpy'd into the DMA buffer, then only the dynamic fields (RGB, eject, vibration, checksum) are filled in.
• hid_err on LED update failure → done.
• Dynamic LED name → done; devm_kasprintf("%s:rgb:joystick_rings", dev_name(&hdev->dev)). I checked the userspace side: InputPlumber matches sys_name as a glob, so its 50-ayaneo_3.yaml just needs a one-liner to *:rgb:joystick_rings once this lands (its config is DMI-gated to the AYANEO 3, so the glob is safe). I'll send that PR when the driver is merged.
• USB parent check → as you noted, hid_is_usb() a few lines down covers it.

I think the current code works well in two spots, let me know if you see it differently:

• sysfs vs remove() race: the driver core removes driver->dev_groups before calling remove() (device_remove() in drivers/base/dd.c), and kernfs holds an active reference for in-flight show/store calls, so unbind waits for them to drain. A store racing with unbind either completes before hid_hw_stop() or never starts.
• MAINTAINERS entry: per-driver HID entries do exist where someone volunteers as maintainer (HID PHOENIX RC, HID WACOM, HID++ LOGITECH, …), and the entry also covers the ABI doc file. I'm committing to maintain this, so I'd keep it, but I'll defer to whatever the hid maintainers say on the list, and can drop it here if you prefer.

checkpatch --strict is clean on all the new commits, and I re-tested the updated driver on my AYANEO 3: probe is now silent, module type reads work, and the renamed LED sets/clears the joystick rings correctly (which also exercises the template-built config command end-to-end, since the device has to ACK it).

[NeroReflex]

Thanks for the thorough review @NeroReflex! All addressed, I pushed each point as a separate commit for easy re-review (I'll squash everything back into the single patch before this goes to LKML):

• scoped_guard in probe → done.
• No print on success → done; dropped the hid_info() and the now-unused response buffer, kept only the warning path.
• kernel-doc for aya3_cmd() → done; documents the arguments, the subcommand-echo reply matching, and the locking.
• Fixed-width checksum → done; u16 — the sum of the 58 payload bytes maxes out at 14790, and the device consumes it as LE16 anyway.
• Ternary for the RGB mode → done.
• Zero-filled template for the config command → done; static const u8 template[] with designated initializers, memcpy'd into the DMA buffer, then only the dynamic fields (RGB, eject, vibration, checksum) are filled in.

I'm not sure adding the static is good idea: first time it will get populated and later on not touched... Are you sure driver still works? Beside it adds to the .bss without any real reason. I thing const is enough here.

• hid_err on LED update failure → done.
• Dynamic LED name → done; devm_kasprintf("%s:rgb:joystick_rings", dev_name(&hdev->dev)). I checked the userspace side: InputPlumber matches sys_name as a glob, so its 50-ayaneo_3.yaml just needs a one-liner to *:rgb:joystick_rings once this lands (its config is DMI-gated to the AYANEO 3, so the glob is safe). I'll send that PR when the driver is merged.

sashiko-bot will tell you: what if dev_name(...) is NULL?

• USB parent check → as you noted, hid_is_usb() a few lines down covers it.

Maybe it can be moved above so that the kernel doesn't even try reading the descriptor if it's not a USB?

I think the current code works well in two spots, let me know if you see it differently:

• sysfs vs remove() race: the driver core removes driver->dev_groups before calling remove() (device_remove() in drivers/base/dd.c), and kernfs holds an active reference for in-flight show/store calls, so unbind waits for them to drain. A store racing with unbind either completes before hid_hw_stop() or never starts.
• MAINTAINERS entry: per-driver HID entries do exist where someone volunteers as maintainer (HID PHOENIX RC, HID WACOM, HID++ LOGITECH, …), and the entry also covers the ABI doc file. I'm committing to maintain this, so I'd keep it, but I'll defer to whatever the hid maintainers say on the list, and can drop it here if you prefer.
  Splendid.

checkpatch --strict is clean on all the new commits, and I re-tested the updated driver on my AYANEO 3: probe is now silent, module type reads work, and the renamed LED sets/clears the joystick rings correctly (which also exercises the template-built config command end-to-end, since the device has to ACK it).

[NeroReflex]

You should merge every modification into the single patch, then do a [NOT-FOR-UPSTREAM] patch that adds to the fragment in this repo the CONFIG_AYANEO required to build the driver so that the github workflow can compile the driver.

[matmartinez]

@NeroReflex Thank you!

Done on both process points: everything is squashed back into the single [FOR-UPSTREAM] patch, and there's now a [NOT-FOR-UPSTREAM] commit adding CONFIG_HID_AYANEO=m to .github/packaging/config.fragment (noted as mirroring kernel-packages#35, which stays the real config change once the driver merges).

On the three code points:

• hid_is_usb() before parsing → done, moved above hid_parse() so non-USB transports bail before the descriptor is even parsed. Rebuilt and re-tested on the device.
• static const template: it doesn't behave like a runtime-populated static — static const with an initializer is materialized at compile time into .rodata (.bss only holds zero-initialized writable data), so there's no "first time it gets populated" and nothing can touch it afterwards; the section is read-only. Dropping static would actually make it worse: a non-static const local is constructed on the stack on every call, only to be memcpy'd right after. And yes — re-verified on hardware: the device ACKs the template-built config command and the RGB rings respond, so the bytes coming out are correct.
• dev_name() NULL: it can't be NULL by the time probe runs — HID core names the device in hid_add_device() (dev_set_name(&hdev->dev, "%04X:%04X:%04X.%04X", ...)) before the device is ever offered to a driver, and probe can only run on a registered device. The allocation that can fail is devm_kasprintf() itself, and that return is checked.

Current state: checkpatch --strict on the squashed patch reports only the well-known ENOSYS false positive (hid_hw_output_report() returns -ENOSYS by contract when the transport has no output-report support; the fallback-to-SET_REPORT pattern is standard across drivers/hid/).

I think the earlier CI failure was indeed the config gate flagging the missing CONFIG_HID_AYANEO, the fragment commit should make it pass. The new run needs workflow approval whenever you get a chance 🙏

[NeroReflex]

Perfect, thank you. As soon as CI compiles the driver I will merge

[NeroReflex]

I asked claude to review one of my drivers. It has said this, I will paste it because I think it's useful to you too.

Feel free to start from a HEAD prior to my merge and reopen another PR. I will take care of the rest.

Good find — and this one's subtler than a simple missing check. hid_is_usb(hdev) only inspects hdev->bus:

static inline bool hid_is_usb(const struct hid_device *hdev)
{
	return hdev->bus == BUS_USB;
}

That field is attacker-controlled: any unprivileged process with access to /dev/uhid can issue UHID_CREATE and set bus = BUS_USB while the actual hdev->dev.parent is the uhid virtual device, not a struct usb_interface. So hid_is_usb(hdev) returning true does not guarantee hdev->dev.parent is safe to cast with to_usb_interface()/interface_to_usbdev(). This affects every USB-cast site in the driver gated only by hid_is_usb(), not just the one hunk the bot flagged in hid_asus_ally_probe() — ally_get_endpoint_address(), asus_kbd_register_leds(), and the QUIRK_T100_KEYBOARD/QUIRK_MEDION_E1239T branches in asus_probe() all have the same gap.

The fix: verify the parent device is actually attached to the USB bus (dev->bus == &usb_bus_type) before trusting the cast, not just the spoofable hdev->bus field.

+/*
+ * hid_is_usb() only checks hdev->bus, which is attacker-controlled by any
+ * process with access to /dev/uhid: UHID_CREATE lets userspace claim an
+ * arbitrary bus id, including BUS_USB, while hdev->dev.parent is the uhid
+ * virtual device, not a struct usb_interface. Casting dev.parent based on
+ * hid_is_usb() alone lets such a spoofed "USB" HID device redirect the
+ * cast at unrelated memory. Confirm the parent is actually on the USB bus
+ * before trusting the cast.
+ */
+static bool asus_hdev_is_usb(struct hid_device *hdev)
+{
+	return hid_is_usb(hdev) && hdev->dev.parent &&
+	       hdev->dev.parent->bus == &usb_bus_type;
+}
+
 static int ally_get_endpoint_address(struct hid_device *hdev)
 {
 	struct usb_host_endpoint *ep;
 	struct usb_interface *intf;
 
-	if (!hid_is_usb(hdev))
+	if (!asus_hdev_is_usb(hdev))
 		return -ENODEV;
 
 	intf = to_usb_interface(hdev->dev.parent);

[NeroReflex]

NVM. It's slop.

https://elixir.bootlin.com/linux/v7.2/source/drivers/hid/usbhid/hid-core.c#L1358

[matmartinez]

@NeroReflex Thanks for the merge and the fast review cycle!

For completeness I checked hid-ayaneo against the underlying concern anyway: the driver never casts hdev->dev.parent (no to_usb_interface() anywhere — everything goes through hid_hw_output_report()/hid_hw_raw_request()), so there was no unsafe dereference to guard even under the stale definition.

Next on my side; the InputPlumber sys_name glob one-liner for the renamed LED, then the upstream series!

[NeroReflex]

When you send it upstream please include

Reviewed-by: Denis Benato <[email protected]>

And send to me too please.

[matmartinez]

Done! Submitted to linux-input/LKML with your Reviewed-by in the trailers and you on Cc, alongside Antheas:

https://lore.kernel.org/linux-input/[email protected]/

Rebased onto hid.git for-next (applied cleanly, base-commit included) with one commit-message fix: the LED-name paragraph now describes the per-device <device name>:rgb:joystick_rings naming from your review instead of the old fixed name. Matching InputPlumber change is up as ShadowBlip/InputPlumber#666.

Thanks again for the review and the merge <3 I'll follow up here if the upstream review produces changes worth backporting to the OGC tree.

[NeroReflex]

Done! Submitted to linux-input/LKML with your Reviewed-by in the trailers and you on Cc, alongside Antheas:

https://lore.kernel.org/linux-input/[email protected]/

Rebased onto hid.git for-next (applied cleanly, base-commit included) with one commit-message fix: the LED-name paragraph now describes the per-device <device name>:rgb:joystick_rings naming from your review instead of the old fixed name. Matching InputPlumber change is up as ShadowBlip/InputPlumber#666.

Thanks again for the review and the merge <3 I'll follow up here if the upstream review produces changes worth backporting to the OGC tree.

Now we wait for sashiko-bot XD

[pastaq]

I have a few nuts, and some suggestions. Some of it is negotiable.

I briefly mentioned it below but wanted to expound more here regarding debounce. Have you fully stress tested the write speed of the RGB interface? In my experience it is best to do a write queue using mod_delayed_work() with a timeout that is approximately what the return time for a write to the interface is in ms. Some userspace applications (like steam) write once per increment of a slider in a single threaded operation. When sliding over the entire color spectrum (255^3 options) that can significantly delay a system even if the return time is only a few ms, added up it becomes seconds. mod_delayed_work() will ensure that the sysfs returns immediately and only the most recent write is sent to the device.

Since this protocol uses a single command buffer to write all attributes, that means you can protect everything with the same mod_delayed_work() which should reduce the complexity if this approach compared to other drivers. It will need special handling in suspend/resume to make sashiko happy, but you can gate re-arming with a bool on drvdata.

[pastaq]

It's better to use guard or scoped_guard from cleanup.h for new code in the kernel. They will unlock themselves immediately as they go out of scope and it prevents mistaken drops of the unlock in future revisions.

Applies to all instances

[pastaq]

I would make these function titles more generic (hid_ayaneo_*). If a future device uses this protocol it won't be confusing, and if the protocol updates later for a new generation it produces less churn turning the entry points into branching probes/resumes.

[pastaq]

Please give the parent led_cdev the color index LED_COLOR_ID_RGB. That will allow userspace to detect the interface and an RGB interface an plumb up things like KDE's chameleon service automatically.

[pastaq]

This implies that it is variable. I would make this an enum with all values and expose a rumble_intensity attribute as well, with a rumble_intensity_index to expose to userspace the options.

[pastaq]

Please add a RO eject_index attr so that userspace can detect options automatically without the need to consult the kernel docs

[pastaq]

It would only be used for people to send patches, and the module author is most likely to be the one to test it. I see no issues here.

[pastaq]

Is there no way to prove the device for its current state? It would be preferable that the interface reflects the status of the hardware at all times rather than when it's written to from userspace. Other programs (like HHD or huesync) write to the hid interface directly, which can desync the sysfs from the hardware state

[pastaq]

It would be better IMO to have an ayaneo_resp struct that you can cast the response into to improve readability and prevent mistakes with offsets.

[pastaq]

I would turn this into a struct and then cast it into a byte buffer before sending. It improves readability significantly. See hid-lenovo-go or hid-oxp for examples.

[pastaq]

Consider adding this attribute as configurable as well

[matmartinez]

Thank you @pastaq!

Great, exactly the input I was holding v3 for. Here's what I've adopted (staged for the LKML v3; I'll bring the OGC tree in sync once the upstream scope discussion settles):

• Scoped guards → done; every lock site now uses scoped_cond_guard(mutex_intr, return -EINTR, ...) (probe already used scoped_guard).
• Generic function names → done for the driver plumbing (ayaneo_probe/ayaneo_remove/ayaneo_cmd/…). I kept the byte-level constants and wire structs as AYA3_*/aya3_* since they describe this firmware generation specifically — a future protocol revision would add its own constants block and branch in the generic entry points, which I think is the structure you were pointing at.
• LED_COLOR_ID_RGB on the parent classdev → done (matches hid-msi, hid-oxp, hid-lenovo-go).
• Wire-format structs → done; the config command and the reply are now __packed structs (aya3_config/aya3_resp) with static_assert on their sizes, replacing the byte-offset defines. Took the hid-lenovo-go shape as suggested.
• Vibration enum → done; all four firmware levels are named (LOW/MEDIUM/HIGH/OFF, stored in the high nibble from the hhd protocol).

On the rest:

• rumble_intensity (+ index) and eject_index attributes: happy to add both, rumble is a single config field and easy to verify on hardware, and I agree on discoverability for eject (it matches the available_*/*_choices pattern elsewhere in sysfs). Since the driver's scope is still being discussed on the list, I don't want to grow the ABI mid-thread; I'll propose both in the v3 text and add them if the HID maintainers are fine with the direction.
• Notification framework toward ayaneo-ec: agreed this would dissolve the split-policy concern, eject would become one write with the kernel owning the whole sequence, and no way to strand the EC half. It's cross-subsystem (HID + pdx86), so I'll do exactly what you suggest: raise it in the v3 cover letter and Cc Ilpo and Armin for a direction check before putting in the effort.
• Interface reflecting hardware state: the module and eject state are probed live, every module_left/module_right read sends a fresh status command to the firmware (that's the "can take up to a second" note in the ABI doc), so those can't desync. RGB/vibration unfortunately have no read-back: the protocol as reverse engineered has exactly one config write (0x21/0x09), and the status reply only carries module IDs and eject progress, hhd never reads config back either. So brightness is necessarily last-write state, same as the other HID RGB drivers; a raw hidraw writer bypassing the driver can desync any of them.
• Sensitivity as configurable: that one resolved differently upstream—v2 dropped the sensitivity bytes from the config command entirely (at Antheas's request, hardware-verified), so the driver no longer clobbers the firmware's own setting on every RGB update. The OGC tree still carries the v1 code; the full sync will bring that along.
• MAINTAINERS: thanks, keeping it.

checkpatch --strict is clean on the reworked file (only the well-known ENOSYS false positive), and I've re-tested the rebuilt driver on the AYANEO 3: module ID reads, RGB solid/pulse/off through the new struct-built config command (the device ACKs each one), rejection of malformed hw_pattern writes, and a module reload cycle, all behave as before.

[pastaq]

Great. Please CC me on the LKML as well.

Derek J. Clark [email protected]

[pastaq]

@matmartinez I noticed you didn't mention anything about the debounce issue I brought up, did you see that portion as well?

[matmartinez]

@pastaq apologies, I worked through the inline comments and missed the review body entirely!

I went and traced the path in the tree, and I believe the scenario you describe is already covered by the LED core, because the driver deliberately registers only brightness_set_blocking (no brightness_set):

• Both brightness_store and multi_intensity_store funnel into led_set_brightness() → led_set_brightness_nopm(). With no non-blocking op available, that path just stores delayed_set_value and queue_work()s the classdev's set_brightness_work, the sysfs write returns immediately, with no device I/O in the writer's context (drivers/leds/led-core.c, led_set_brightness_nopm()).
• queue_work() on an already-queued work item is a no-op, so a Steam-style single-threaded slider drag coalesces exactly the way mod_delayed_work() would: each execution of the work item sends one config command carrying whatever brightness/intensity state is newest at that moment. The device never sees one write per slider increment, and userspace never waits on the interface round-trip.

I stress-tested while chasing the teardown race: tight-loop brightness hammer sustained over several rmmod cycles, minutes at a time results in no lag buildup, no protocol errors, and the rings track the most recent value (it's the same work item remove() double-flushes in the teardown fix, so the lifetime story stays coherent).

What mod_delayed_work() would add on top is rate-limiting below one-per-round-trip, fewer USB commands during a continuous drag. Given the stock mechanism already provides the immediate-return + latest-only semantics, my inclination is to stay on the LED core's plumbing and skip the extra suspend/resume re-arming state but if you think the write rate during drags is still worth capping, I'm happy to add the delayed-work variant in v3 and measure/test both on hardware.

[pastaq]

I'm not confident that is accurate, my concern comes from real world experience with this issue. I experienced significant userspace hitching with the go, OXP, and MSI drivers. The results are amplified depending on the round trip time for a urb in the device, the wait_for_completion timeout becomes a bottleneck and the queued calls pile up. I haven't mitigated this in go_s drivers because it has a sub 4ms round trip and it's not really possible to over queue the buffer in the same way

[matmartinez]

@pastaq I went and traced all three drivers, and I think the difference is which LED-core op they register.

hid-lenovo-go, hid-oxp and hid-msi all register brightness_set, which the LED core invokes synchronously in the writer's context. So whatever the callback does happens on Steam's thread: the Go sends the command inline (hid_go_brightness_set() → rgb_cfg_call() → wait_for_completion_interruptible_timeout), meaning every slider increment is a full round trip in the writer's context; fine at sub-4 ms, hitchy beyond. The OXP and Claw callbacks mitigate it exactly the way you describe, saving state and mod_delayed_work(50ms).

hid-ayaneo registers only brightness_set_blocking, and for that op the core never calls the driver from the writer's context: led_set_brightness_nopm() stores delayed_set_value, queues the classdev's own set_brightness_work, and the store returns. queue_work() on an already-queued item is a no-op, so there's no queue that can deepen — a burst of stores collapses into at most one further work run carrying the newest state, and the wait_for_completion in my command path only ever runs on the kworker. It's effectively the same mitigation you built by hand in oxp/msi, already in the core, minus the settle timer.

I just measured on my hardware:

• Command+ACK round trip on the AYA3 vendor interface: 5.3 ms average (3.8–8 ms across singles, N=100). So by your Go criterion this device is too slow to write from the store context, and in the Go's architecture it would hitch.
• Slider-drag simulation, 1000 single-threaded multi_intensity writes in a tight loop: 12 ms total (~12 µs per store). At a 5.3 ms round trip, at most 2–3 commands even fit in that window — a thousand writes coalesced into a couple, and the writer never blocked. The same sequence through a synchronous store path would take ~5.3 s.

The residual thing your 50 ms settle would still buy is a lower device-command rate during a continuous drag (~190/s at this round trip → ~20/s). The rings held up fine under sustained tight-loop hammering in the teardown stress tests, so I'd lean toward keeping the stock core plumbing but if you've seen a brightness_set_blocking driver misbehave, or you think the drag-rate is worth having regardless, again I'm happy to implement it in v3.

[pastaq]

I suppose it depends on if you're going to implement the additional attribute for effect using the same ABI I did for the other drivers. It's probably worth me investigating if I can get brightness_set_blocking() working in those contexts as well. I did try using that but my implementation wasn't successful. I don't recall why precisely.

I wouldn't call this a blocker for v3 if you're getting reasonable performance.

[matmartinez]

Yeah, no custom effect attribute planned... the AYA3's only effect is exposed through the stock ledtrig-pattern hw_pattern ABI, and the firmware has no speed/parameters to slide. So the whole RGB path stays on the LED core's deferred path.

One tip in case you retry brightness_set_blocking: a racing brightness store can requeue set_brightness_work during led_classdev_unregister(), so with devm registration the work can run after the transport is gone, that cost me a hard crash to find. Fix was non-devm LED, unregister first in remove, then a second flush_work(). If your earlier attempt crashed on unbind/suspend, that'd be my first suspect.

Thanks for helping me on this @pastaq!