tmff2: defer gain and autocenter changes to the workqueue - #220
Conversation
|
Thanks! I'll copy a comment I previously left on this matter: I looked back at the previous PRs and IIUC, the original reason for inserting set_gain etc. into the workqueue was that you encountered a crash if it was done directly. I had a quick look and found at least one other driver that sends out USB messages directly in the attribute handler: https://elixir.bootlin.com/linux/v7.2.2/source/drivers/hid/hid-appletb-kbd.c#L126 (where appletb_kbd_set_mode ultimately calls hid_hw_request). That would point towards something being buggy, either that other driver, the USB layer (when dealing with the T500?) or some previous development version of this PR that these changes try to work around. I'm alright with workarounds, but I'd like to know what we're working around, could you try to check that out? |
| /* Re-queue the last in-game gain; the worker scales it with the | ||
| * new sysfs gain via tmff2_scale_gain() */ | ||
| spin_lock_irqsave(&tmff2->lock, flags); | ||
| tmff2->pending_gain = tmff2->last_input_gain; |
There was a problem hiding this comment.
Are there 'two' different changes in this commit? The first being moving set_gain to the workqueue, and the other seemingly fixes a bug where setting the gain via sysfs overrides the in-game requested gain?
I added a comment about the first, and I think this approach to the second one is a bit messy. I wonder if it might not make more sense to have sysfs send over the gain to the wheel, and tmff2_set_gain then sets a game-specific gain that's just a software calculation, probably something like t300rs_calculate_constant_level(level, gain, direction). Although that still leaves a situation like Game1 requests 50% gain, user exits, Game2 doesn't set a gain level. Now changing the sysfs attribute doesn't increase Game2's gain beyond 50%. Maybe resetting the game-specific gain if a game exits, but not sure how that would be monitored. close doesn't seem to be called for situations like the above, not entirely sure why. Maybe evdev keeps an open FD or something?
There was a problem hiding this comment.
Come to think of it, on Windows there's a button that can select between letting the game or the Thrustmaster wheel manager set the gain. sysfs is kind of like the wheel manager, and input is the game, and I wonder if the button just ignores which source is used as the 'actual' gain value.
Should probably check with something like lg4ff what they do in this situation.
There was a problem hiding this comment.
new-lg4ff doesn't seem to have a separate wheel command for gain, but yeah, it has a separate master_gain for sysfs gain and a game-specific gain that are used to calculate the final gain that is sent over to the wheel. Presumably the scenario of Game2 not setting a gain is rare enough as to not really be an issue?
There was a problem hiding this comment.
new-lg4ffdoesn't seem to have a separate wheel command for gain, but yeah, it has a separatemaster_gainforsysfsgain and a game-specificgainthat are used to calculate the final gain that is sent over to the wheel. Presumably the scenario ofGame2not setting a gain is rare enough as to not really be an issue?
Fair point! I force-pushed a rework that removes the last_input_gain re-apply hack entirely. Gain is now plain live state:
- tmff2_set_gain (input) stores the game gain and marks the gain pending
- the sysfs store (master) just marks the gain pending
- the worker computes the product master × game via tmff2_scale_gain() at flush time and sends one hardware gain command
So a sysfs change still takes effect immediately, but there's no replay logic. The "second change" is gone and the commit is one mechanism. (The inline comments may show as outdated after the force-push; current code is at gain_store/tmff2_set_gain/tmff2_work_handler.)
On the lg4ff comparison, that's exactly the model now implemented: master_gain (sysfs) × game gain (input) → final gain sent to the wheel, matching new-lg4ff.
One difference though: the T300/T500 do have a hardware gain command, so the product goes over the wire as the wheel gain rather than being applied to effect levels in software. This also keeps behaviour for existing wheels identical to what the worker already sent when a game set gain.
The "Game2 never sets a gain" edge case. I agreed it's rare enough to accept (same assumption as lg4ff). The game gain persists per-device until the next input write. Detecting game exit isn't reliable here for the reason you mentioned
Ah, I think I see what's going on here. The next PR uses |
18aadcb to
704475f
Compare
The input core invokes the FF gain and autocenter callbacks from event context, which may run with IRQs disabled, and the sysfs gain store calls them from process context. Model drivers issue blocking HID output in these callbacks, which is illegal in atomic context and races the effect worker on shared transfer buffers. Route both through the existing effect workqueue instead: mark the request pending in the device entry and apply it in the worker, serialized with effect processing. Gain follows the master x game model (as lg4ff): the sysfs gain parameter acts as the master gain, the value written through the input API is the game gain, and the worker sends their product to the wheel via tmff2_scale_gain(). Both sources only mark the gain pending; the worker recomputes the product from the live values on each flush, so a sysfs change takes effect immediately without having to replay the last in-game request.
704475f to
9361ede
Compare
Dug into it — here's the concrete path. The freeze (whole system hang, nothing in dmesg) happened with the T500 whenever gain/autocenter changed while effects were running, e.g. dragging Oversteer's sliders. The problematic context is not sysfs — the sysfs store runs in process context and may sleep, which is why appletb-kbd doing hid_hw_request() from an attribute handler is fine. The crash path is the input event path: userspace writes an EV_FF/FF_GAIN event → input_inject_event() takes dev->event_lock with IRQs disabled (input.c (https://elixir.bootlin.com/linux/v6.18/source/drivers/input/input.c#L445), guard(spinlock_irqsave)) → input_handle_event() → dev->event() which is input_ff_event() (https://elixir.bootlin.com/linux/v6.18/source/drivers/input/ff-core.c#L193) → ff->set_gain() → our driver callback issues a synchronous USB transfer, i.e. sleeps under a spinlock with IRQs off. That's the freeze. Note the T300 path has the same latent contract violation on master today (t300rs_set_gain → hid_hw_request() → usb_control_msg(), also a sleeping call) — it just rarely bites since gain/autocenter changes are infrequent. Deferring both callbacks to the existing effect worker fixes the contract for every wheel model, not just the T500. |
Two reasons:
Once the gain/autocenter HID I/O runs from the worker, either transport would be legal — so if you prefer hid_hw_request() for consistency with the T300, that's testable follow-up work on the T500 PR. The deferral is needed either way. |
Please don't be a meat proxy.. The above links to source code don't correspond to the supposed execution path, and the given cause for the freeze, dragging Oversteer sliders, uses the
which is contradictory, no? |
I couldn't find the path to If we go with the approach of just setting an internal For what it's worth, I can see how it's nice to not have to rely on external functions being atomic-safe, especially when they're not documented for it. As a bit of history, the workqueue was originally just added to limit how often the driver sends out requests to the wheel, not to avoid atomic contexts. Adding a second responsibility arguably muddles the waters a bit from a perspective of the mental model needed to understand this driver. |
2 participants
The input core invokes the FF gain and autocenter callbacks from event context, which may run with IRQs disabled, and the sysfs gain store calls them from process context. Model drivers issue blocking HID output in these callbacks, which is illegal in atomic context and races the effect worker on shared transfer buffers.
Route both through the existing effect workqueue instead: store the latest request in the device entry and apply it in the worker, serialized with effect processing. The sysfs gain store re-queues the last in-game gain so level changes take effect immediately; in-game gain continues to be scaled by the sysfs gain parameter via tmff2_scale_gain(), now applied in the worker.