linux-input.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH AUTOSEL 6.17-6.12] HID: pidff: Use direction fix only for conditional effects
       [not found] <20251025160905.3857885-1-sashal@kernel.org>
@ 2025-10-25 15:56 ` Sasha Levin
  2025-10-25 16:01 ` [PATCH AUTOSEL 6.17-6.6] HID: asus: add Z13 folio to generic group for multitouch to work Sasha Levin
  1 sibling, 0 replies; 2+ messages in thread
From: Sasha Levin @ 2025-10-25 15:56 UTC (permalink / raw)
  To: patches, stable
  Cc: Tomasz Pakuła, Oleg Makarenko, Jiri Kosina, Sasha Levin,
	jikos, bentiss, linux-input, linux-usb

From: Tomasz Pakuła <tomasz.pakula.oficjalny@gmail.com>

[ Upstream commit f345a4798dab800159b09d088e7bdae0f16076c3 ]

The already fixed bug in SDL only affected conditional effects. This
should fix FFB in Forza Horizion 4/5 on Moza Devices as Forza Horizon
flips the constant force direction instead of using negative magnitude
values.

Changing the direction in the effect directly in pidff_upload_effect()
would affect it's value in further operations like comparing to the old
effect and/or just reading the effect values in the user application.

This, in turn, would lead to constant PID_SET_EFFECT spam as the effect
direction would constantly not match the value that's set by the
application.

This way, it's still transparent to any software/API.

Only affects conditional effects now so it's better for it to explicitly
state that in the name. If any HW ever needs fixed direction for other
effects, we'll add more quirks.

Signed-off-by: Tomasz Pakuła <tomasz.pakula.oficjalny@gmail.com>
Reviewed-by: Oleg Makarenko <oleg@makarenk.ooo>
Signed-off-by: Jiri Kosina <jkosina@suse.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---

LLM Generated explanations, may be completely bogus:

YES

- What it fixes
  - The earlier quirk forced a fixed direction (0x4000) for all effect
    types on specific wheelbases to work around mis-set PID directions
    in some user stacks (dinput → wine → SDL). That broad forcing breaks
    games that legitimately use the direction field for non-conditional
    effects (e.g., Forza Horizon 4/5 uses direction flips for constant
    force instead of negative magnitudes). This patch narrows the quirk
    to conditional effects only (spring, damper, inertia, friction),
    matching where the SDL-side bug actually applied.

- How it changes behavior
  - Adds a helper to detect conditional effects and a wrapper to set the
    direction only when appropriate:
    - New logic: pidff_is_effect_conditional() and
      pidff_set_effect_direction() in drivers/hid/usbhid/hid-pidff.c
      (replaces unconditional forcing in Set Effect).
    - Set Effect now calls the helper instead of unconditionally forcing
      direction for all effects.
  - Renames the quirk to reflect scope:
    HID_PIDFF_QUIRK_FIX_WHEEL_DIRECTION →
    HID_PIDFF_QUIRK_FIX_CONDITIONAL_DIRECTION, still using the same bit
    (BIT(3)).
  - Updates device ID table entries to use the new quirk name for Moza
    devices in drivers/hid/hid-universal-pidff.c.

- Why it’s appropriate for stable
  - User-visible bug/regression: Fixes incorrect or missing FFB in Forza
    Horizon 4/5 on Moza devices when the kernel overrode constant-force
    directions. The message explicitly states this resolves that case.
  - Small and contained: Limited to HID PID force-feedback code and
    quirk tables; no architectural changes.
  - Minimal risk: Only affects devices already marked with the quirk
    bit. Behavior is narrowed (less intrusive) by applying the fixed
    direction only to conditional effects; other effect types now honor
    the application-provided direction as intended.
  - No side effects on unrelated subsystems: Touches only HID FFB code
    paths.
  - Clear lineage: It logically corrects the earlier
    “FIX_WHEEL_DIRECTION” quirk (drivers/hid/usbhid/hid-pidff.c:397)
    that forced direction for all effect types.

- Specific code references
  - Current unconditional forcing (to be replaced by helper call):
    drivers/hid/usbhid/hid-pidff.c:397
  - Quirk bit definition that’s renamed but remains BIT(3):
    drivers/hid/usbhid/hid-pidff.h:20
  - Fixed direction constant (still used, but now applied
    conditionally): drivers/hid/usbhid/hid-pidff.c:151
  - Device entries using the quirk (updated to new name):
    drivers/hid/hid-universal-pidff.c:6

- Additional considerations
  - The commit avoids mutating effect->direction in
    pidff_upload_effect(), preventing mismatches with the application’s
    state and avoiding needless PID_SET_EFFECT churn. It keeps behavior
    transparent to user space.
  - If any future hardware requires fixed direction for non-conditional
    effects, the commit message notes that more targeted quirks will be
    added, which further limits regression risk.

Given it’s a targeted regression fix for a real-world breakage, small in
scope, and reduces the quirk’s blast radius, it’s a strong candidate for
stable backport (especially on branches that already carry the earlier
FIX_WHEEL_DIRECTION quirk).

 drivers/hid/hid-universal-pidff.c | 20 ++++++++++----------
 drivers/hid/usbhid/hid-pidff.c    | 28 +++++++++++++++++++++++-----
 drivers/hid/usbhid/hid-pidff.h    |  2 +-
 3 files changed, 34 insertions(+), 16 deletions(-)

diff --git a/drivers/hid/hid-universal-pidff.c b/drivers/hid/hid-universal-pidff.c
index 554a6559aeb73..70fce0f88e825 100644
--- a/drivers/hid/hid-universal-pidff.c
+++ b/drivers/hid/hid-universal-pidff.c
@@ -144,25 +144,25 @@ static int universal_pidff_input_configured(struct hid_device *hdev,
 
 static const struct hid_device_id universal_pidff_devices[] = {
 	{ HID_USB_DEVICE(USB_VENDOR_ID_MOZA, USB_DEVICE_ID_MOZA_R3),
-		.driver_data = HID_PIDFF_QUIRK_FIX_WHEEL_DIRECTION },
+		.driver_data = HID_PIDFF_QUIRK_FIX_CONDITIONAL_DIRECTION },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_MOZA, USB_DEVICE_ID_MOZA_R3_2),
-		.driver_data = HID_PIDFF_QUIRK_FIX_WHEEL_DIRECTION },
+		.driver_data = HID_PIDFF_QUIRK_FIX_CONDITIONAL_DIRECTION },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_MOZA, USB_DEVICE_ID_MOZA_R5),
-		.driver_data = HID_PIDFF_QUIRK_FIX_WHEEL_DIRECTION },
+		.driver_data = HID_PIDFF_QUIRK_FIX_CONDITIONAL_DIRECTION },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_MOZA, USB_DEVICE_ID_MOZA_R5_2),
-		.driver_data = HID_PIDFF_QUIRK_FIX_WHEEL_DIRECTION },
+		.driver_data = HID_PIDFF_QUIRK_FIX_CONDITIONAL_DIRECTION },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_MOZA, USB_DEVICE_ID_MOZA_R9),
-		.driver_data = HID_PIDFF_QUIRK_FIX_WHEEL_DIRECTION },
+		.driver_data = HID_PIDFF_QUIRK_FIX_CONDITIONAL_DIRECTION },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_MOZA, USB_DEVICE_ID_MOZA_R9_2),
-		.driver_data = HID_PIDFF_QUIRK_FIX_WHEEL_DIRECTION },
+		.driver_data = HID_PIDFF_QUIRK_FIX_CONDITIONAL_DIRECTION },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_MOZA, USB_DEVICE_ID_MOZA_R12),
-		.driver_data = HID_PIDFF_QUIRK_FIX_WHEEL_DIRECTION },
+		.driver_data = HID_PIDFF_QUIRK_FIX_CONDITIONAL_DIRECTION },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_MOZA, USB_DEVICE_ID_MOZA_R12_2),
-		.driver_data = HID_PIDFF_QUIRK_FIX_WHEEL_DIRECTION },
+		.driver_data = HID_PIDFF_QUIRK_FIX_CONDITIONAL_DIRECTION },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_MOZA, USB_DEVICE_ID_MOZA_R16_R21),
-		.driver_data = HID_PIDFF_QUIRK_FIX_WHEEL_DIRECTION },
+		.driver_data = HID_PIDFF_QUIRK_FIX_CONDITIONAL_DIRECTION },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_MOZA, USB_DEVICE_ID_MOZA_R16_R21_2),
-		.driver_data = HID_PIDFF_QUIRK_FIX_WHEEL_DIRECTION },
+		.driver_data = HID_PIDFF_QUIRK_FIX_CONDITIONAL_DIRECTION },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_CAMMUS, USB_DEVICE_ID_CAMMUS_C5) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_CAMMUS, USB_DEVICE_ID_CAMMUS_C12) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_VRS, USB_DEVICE_ID_VRS_DFP),
diff --git a/drivers/hid/usbhid/hid-pidff.c b/drivers/hid/usbhid/hid-pidff.c
index 614a20b620231..c6b4f61e535d5 100644
--- a/drivers/hid/usbhid/hid-pidff.c
+++ b/drivers/hid/usbhid/hid-pidff.c
@@ -205,6 +205,14 @@ struct pidff_device {
 	u8 effect_count;
 };
 
+static int pidff_is_effect_conditional(struct ff_effect *effect)
+{
+	return effect->type == FF_SPRING  ||
+	       effect->type == FF_DAMPER  ||
+	       effect->type == FF_INERTIA ||
+	       effect->type == FF_FRICTION;
+}
+
 /*
  * Clamp value for a given field
  */
@@ -294,6 +302,20 @@ static void pidff_set_duration(struct pidff_usage *usage, u16 duration)
 	pidff_set_time(usage, duration);
 }
 
+static void pidff_set_effect_direction(struct pidff_device *pidff,
+				       struct ff_effect *effect)
+{
+	u16 direction = effect->direction;
+
+	/* Use fixed direction if needed */
+	if (pidff->quirks & HID_PIDFF_QUIRK_FIX_CONDITIONAL_DIRECTION &&
+	    pidff_is_effect_conditional(effect))
+		direction = PIDFF_FIXED_WHEEL_DIRECTION;
+
+	pidff->effect_direction->value[0] =
+		pidff_rescale(direction, U16_MAX, pidff->effect_direction);
+}
+
 /*
  * Send envelope report to the device
  */
@@ -395,11 +417,7 @@ static void pidff_set_effect_report(struct pidff_device *pidff,
 		pidff->set_effect[PID_GAIN].field->logical_maximum;
 	pidff->set_effect[PID_DIRECTION_ENABLE].value[0] = 1;
 
-	/* Use fixed direction if needed */
-	pidff->effect_direction->value[0] = pidff_rescale(
-		pidff->quirks & HID_PIDFF_QUIRK_FIX_WHEEL_DIRECTION ?
-		PIDFF_FIXED_WHEEL_DIRECTION : effect->direction,
-		U16_MAX, pidff->effect_direction);
+	pidff_set_effect_direction(pidff, effect);
 
 	/* Omit setting delay field if it's missing */
 	if (!(pidff->quirks & HID_PIDFF_QUIRK_MISSING_DELAY))
diff --git a/drivers/hid/usbhid/hid-pidff.h b/drivers/hid/usbhid/hid-pidff.h
index a53a8b436baa6..f321f675e1318 100644
--- a/drivers/hid/usbhid/hid-pidff.h
+++ b/drivers/hid/usbhid/hid-pidff.h
@@ -16,7 +16,7 @@
 #define HID_PIDFF_QUIRK_PERMISSIVE_CONTROL	BIT(2)
 
 /* Use fixed 0x4000 direction during SET_EFFECT report upload */
-#define HID_PIDFF_QUIRK_FIX_WHEEL_DIRECTION	BIT(3)
+#define HID_PIDFF_QUIRK_FIX_CONDITIONAL_DIRECTION	BIT(3)
 
 /* Force all periodic effects to be uploaded as SINE */
 #define HID_PIDFF_QUIRK_PERIODIC_SINE_ONLY	BIT(4)
-- 
2.51.0


^ permalink raw reply related	[flat|nested] 2+ messages in thread

* [PATCH AUTOSEL 6.17-6.6] HID: asus: add Z13 folio to generic group for multitouch to work
       [not found] <20251025160905.3857885-1-sashal@kernel.org>
  2025-10-25 15:56 ` [PATCH AUTOSEL 6.17-6.12] HID: pidff: Use direction fix only for conditional effects Sasha Levin
@ 2025-10-25 16:01 ` Sasha Levin
  1 sibling, 0 replies; 2+ messages in thread
From: Sasha Levin @ 2025-10-25 16:01 UTC (permalink / raw)
  To: patches, stable
  Cc: Antheas Kapenekakis, Luke D. Jones, Jiri Kosina, Sasha Levin,
	jikos, bentiss, linux-input

From: Antheas Kapenekakis <lkml@antheas.dev>

[ Upstream commit b595974b4afe0e171dd707da570964ff642742e3 ]

The Asus Z13 folio has a multitouch touchpad that needs to bind
to the hid-multitouch driver in order to work properly. So bind
it to the HID_GROUP_GENERIC group to release the touchpad and
move it to the bottom so that the comment applies to it.

While at it, change the generic KEYBOARD3 name to Z13_FOLIO.

Reviewed-by: Luke D. Jones <luke@ljones.dev>
Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
Signed-off-by: Jiri Kosina <jkosina@suse.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---

LLM Generated explanations, may be completely bogus:

Based on my comprehensive analysis, here is my recommendation:

## Backport Status: **YES**

### Summary of Analysis

This commit fixes a hardware functionality bug where the multitouch
touchpad on the ASUS ROG Z13 folio does not work. The fix is a textbook
example of a stable tree backport candidate.

### Technical Understanding

**What Changed:**
1. **drivers/hid/hid-asus.c (lines 1387-1421)**: The device entry for
   Z13 folio (device ID 0x1a30) was moved from the middle of the device
   table to the bottom, and crucially changed from:
  ```c
  HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
  USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD3)
  ```
  to:
  ```c
  HID_DEVICE(BUS_USB, HID_GROUP_GENERIC, USB_VENDOR_ID_ASUSTEK,
  USB_DEVICE_ID_ASUSTEK_ROG_Z13_FOLIO)
  ```

2. **drivers/hid/hid-ids.h (line 226)**: Renamed the constant from
   `USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD3` to the more descriptive
   `USB_DEVICE_ID_ASUSTEK_ROG_Z13_FOLIO`

**Why This Matters:**
- The Z13 folio is a composite USB device with both a keyboard and
  multitouch touchpad
- `HID_USB_DEVICE` expands to `.bus = BUS_USB` without specifying a
  group, so it matches **all HID groups**
- `HID_DEVICE(BUS_USB, HID_GROUP_GENERIC, ...)` explicitly restricts
  binding to only `HID_GROUP_GENERIC`
- The multitouch touchpad presents itself as `HID_GROUP_MULTITOUCH` or
  `HID_GROUP_MULTITOUCH_WIN_8`
- When hid-asus binds to all groups, it prevents hid-multitouch from
  binding to the touchpad
- By restricting hid-asus to `HID_GROUP_GENERIC` (keyboard), hid-
  multitouch can properly handle the touchpad

### Evidence of Correctness

1. **Established Pattern**: This exact fix was applied to the T101HA
   keyboard in 2021 (commit a94f66aecdaa4). The commit message for that
   fix explicitly states: *"The hid-multitouch hiddev has a group of
   HID_GROUP_MULTITOUCH_WIN_8, so the same result can be achieved by
   making the hid_device_id entry for the dock in the asus_devices[]
   table only match on HID_GROUP_GENERIC"*

2. **Wide Usage**: Verified that `HID_DEVICE(BUS_USB, HID_GROUP_GENERIC,
   ...)` pattern is used extensively in:
   - hid-asus.c (T101HA at drivers/hid/hid-asus.c:1422)
   - hid-google-hammer.c (9+ instances)
   - hid-ite.c (4+ instances)
   - hid-lenovo.c (5+ instances)

3. **Device History**: The Z13 keyboard was originally added in v6.5
   (commit 74e47b2c52ed4) using HID_USB_DEVICE, which inadvertently
   prevented the touchpad from working. This commit corrects that
   oversight.

4. **Author Credibility**: Antheas Kapenekakis has 15+ kernel
   contributions focused on platform/x86 and HID subsystems, including
   other Z13 fixes. The patch was reviewed by Luke D. Jones, the
   original author of Z13 support.

5. **Comment Alignment**: The commit moves the entry below an existing
   comment (lines 1415-1418) that explicitly explains: *"Note bind to
   the HID_GROUP_GENERIC group, so that we only bind to the keyboard
   part, while letting hid-multitouch.c handle the touchpad."* This
   comment now applies to both T101HA and Z13 folio.

### Risk Analysis

**Minimal Risk:**
- **Isolated Scope**: Only affects ASUS device ID 0x1a30 (Z13 folio)
- **No Dependencies**: Standalone fix with no prerequisites
- **No Architectural Changes**: Uses existing, proven pattern
- **Verified Pattern**: Multiple similar devices use this approach since
  2021 without issues
- **Clean Rename**: Old constant name completely removed, no lingering
  references found

**Regression Potential:**
- Extremely low - the change makes a non-working touchpad work
- Users without this fix have a broken touchpad; users with it have
  working touchpad
- The keyboard functionality is unaffected (same quirks applied)

### Stable Tree Criteria Assessment

✅ **Fixes important bug**: Multitouch touchpad completely non-functional
✅ **Small and contained**: 2 files changed, device ID rename + entry
repositioning
✅ **No architectural changes**: Uses established HID group filtering
pattern
✅ **Minimal regression risk**: Proven pattern, isolated to one device
✅ **Confined to subsystem**: HID subsystem only
✅ **Applies to stable**: Any stable kernel >= 6.5 (where Z13 support
exists)

**Note**: While the commit lacks a `Cc: stable@vger.kernel.org` tag or
`Fixes:` tag, it clearly meets stable tree criteria and should be
backported to all stable kernels that contain the original Z13 support
(6.5+).

### Conclusion

This is an **unambiguous YES** for backporting. The commit:
- Fixes real hardware (touchpad doesn't work without it)
- Uses a well-tested, 4+ year old pattern
- Has minimal risk and isolated impact
- Comes from a credible contributor
- Was reviewed by the original device maintainer

This should be backported to all stable trees from 6.5 onwards where the
Z13 folio device support exists.

 drivers/hid/hid-asus.c | 6 +++---
 drivers/hid/hid-ids.h  | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index 8db9d4e7c3b0b..a444d41e53b6c 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -1387,9 +1387,6 @@ static const struct hid_device_id asus_devices[] = {
 	{ HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
 	    USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD2),
 	  QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
-	    USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD3),
-	  QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
 	    USB_DEVICE_ID_ASUSTEK_ROG_Z13_LIGHTBAR),
 	  QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
@@ -1419,6 +1416,9 @@ static const struct hid_device_id asus_devices[] = {
 	 * Note bind to the HID_GROUP_GENERIC group, so that we only bind to the keyboard
 	 * part, while letting hid-multitouch.c handle the touchpad.
 	 */
+	{ HID_DEVICE(BUS_USB, HID_GROUP_GENERIC,
+		USB_VENDOR_ID_ASUSTEK, USB_DEVICE_ID_ASUSTEK_ROG_Z13_FOLIO),
+	  QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
 	{ HID_DEVICE(BUS_USB, HID_GROUP_GENERIC,
 		USB_VENDOR_ID_ASUSTEK, USB_DEVICE_ID_ASUSTEK_T101HA_KEYBOARD) },
 	{ }
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index ded5348d190c5..5721b8414bbdf 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -223,7 +223,7 @@
 #define USB_DEVICE_ID_ASUSTEK_ROG_KEYBOARD3 0x1822
 #define USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD	0x1866
 #define USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD2	0x19b6
-#define USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD3	0x1a30
+#define USB_DEVICE_ID_ASUSTEK_ROG_Z13_FOLIO		0x1a30
 #define USB_DEVICE_ID_ASUSTEK_ROG_Z13_LIGHTBAR		0x18c6
 #define USB_DEVICE_ID_ASUSTEK_ROG_NKEY_ALLY		0x1abe
 #define USB_DEVICE_ID_ASUSTEK_ROG_NKEY_ALLY_X		0x1b4c
-- 
2.51.0


^ permalink raw reply related	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2025-10-25 16:29 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <20251025160905.3857885-1-sashal@kernel.org>
2025-10-25 15:56 ` [PATCH AUTOSEL 6.17-6.12] HID: pidff: Use direction fix only for conditional effects Sasha Levin
2025-10-25 16:01 ` [PATCH AUTOSEL 6.17-6.6] HID: asus: add Z13 folio to generic group for multitouch to work Sasha Levin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).