Re: [PATCH 1/2] amd-sfh-hid: tablet mode hid report and asus quirk
From: sashiko-bot@kernel.org
Date: 2026-07-21 14:47:10
Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider: New issues: - [Medium] Missing HID Input Mapping for tablet mode switch - [Low] Incomplete Implementation / Dead Code for Feature Report - [Low] Unintentional Parameter Override via DMI Quirk - [Low] Unused Macros for Tablet State Evaluated by Magic Number - [Low] Introduction of Non-English Comments Pre-existing issues: - [High] Double Free / Use-After-Free in amdtp_hid_probe error path --
--- Patch [1]: [PATCH 1/2] amd-sfh-hid: tablet mode hid report and asus quirk ---commit 8025fc07a2e48355e8daeee6c47f30c940eb5a30 Author: Helge Bahmann [off-list ref] amd-sfh-hid: tablet mode hid report and asus quirk Add an input driver that interprets the "operation mode" sensor offered by the amd sfh as a hid driver to generate a custom hid report (tablet mode switch). Add a quirk to restore compatibility of the driver with the Asus VivoBook (turn off the "disable interrupts flag). Expose the intr_disable flag as a module parameter in case it turns out to be needed on further laptop models.
quoted hunk ↗ jump to hunk
diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_client.c b/drivers/hid/amd-sfh-hid/amd_sfh_client.c index 96ae792beeb6c..8c03279646023 100644 --- a/drivers/hid/amd-sfh-hid/amd_sfh_client.c +++ b/drivers/hid/amd-sfh-hid/amd_sfh_client.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -327,12 +314,6 @@ int amd_sfh_hid_client_init(struct amd_mp2_dev *privdata) for (i = 0; i < cl_data->num_hid_devices; i++) { cl_data->cur_hid_dev = i; - if (cl_data->sensor_idx[i] == op_idx) { - dev_dbg(dev, "sid 0x%x (%s) status 0x%x\n", - cl_data->sensor_idx[i], get_sensor_name(cl_data->sensor_idx[i]), - cl_data->sensor_sts[i]); - continue; - } if (cl_data->sensor_sts[i] == SENSOR_ENABLED) { rc = amdtp_hid_probe(i, cl_data);
[Severity: High] This isn't a bug introduced by this patch, but is a pre-existing issue in the driver's error-handling path. If amdtp_hid_probe() fails (e.g., in hid_add_device()), it destroys the hid device but does not clear the dangling pointer in cli_data->hid_sensor_hubs[cur_hid_dev]. When this failure propagates up to amd_sfh_hid_client_init(), it jumps to the cleanup label and invokes amd_sfh_hid_client_deinit() -> amdtp_hid_remove(). amdtp_hid_remove() will then iterate over the hid_sensor_hubs array and call hid_destroy_device() again on the dangling pointer, which can cause a double free. Should the pointer be set to NULL in amdtp_hid_probe() on error?
quoted hunk ↗ jump to hunk
diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c b/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c index 4b81cebdc3359..924329bfef8fe 100644 --- a/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c +++ b/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c@@ -39,6 +39,8 @@ module_param_named(sensor_mask, sensor_mask_override, int, 0444); MODULE_PARM_DESC(sensor_mask, "override the detected sensors mask"); static bool intr_disable = true; +module_param_named(intr_disable, intr_disable, bool, 0444); +MODULE_PARM_DESC(intr_disable, "override the interrupt disable sensor bit");
[Severity: Low] Because intr_disable is implemented as a simple boolean, the DMI match quirk for Asus VivoBook unconditionally invokes mp2_disable_intr(), which overwrites intr_disable to false. Does this prevent the driver from differentiating between the default state and an explicit user override on the command line (e.g., amd_sfh.intr_disable=1)?
quoted hunk ↗ jump to hunk
diff --git a/drivers/hid/amd-sfh-hid/hid_descriptor/amd_sfh_hid_desc.c b/drivers/hid/amd-sfh-hid/hid_descriptor/amd_sfh_hid_desc.c index ef1f9be8b8938..7e807b92d2960 100644 --- a/drivers/hid/amd-sfh-hid/hid_descriptor/amd_sfh_hid_desc.c +++ b/drivers/hid/amd-sfh-hid/hid_descriptor/amd_sfh_hid_desc.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -164,6 +180,11 @@ static u8 get_feature_report(int sensor_idx, int report_id, u8 *feature_report) memcpy(feature_report, &gyro_feature, sizeof(gyro_feature)); report_size = sizeof(gyro_feature); break; + case op_idx: /* op */ + get_common_features(&tablet_mode_feature.common_property, report_id); + memcpy(feature_report, &tablet_mode_feature, sizeof(tablet_mode_feature)); + report_size = sizeof(tablet_mode_feature); + break;
[Severity: Low] The driver allocates and populates a Feature report (tablet_mode_feature_report) here, but the corresponding tablet_mode_report_descriptor lacks any Feature items (0xB1). Since the HID core strictly relies on the parsed descriptor to create reports, will a feature report with ID 0x11 ever be requested, or is this unused dead code?
quoted hunk ↗ jump to hunk
@@ -238,6 +260,15 @@ static u8 get_input_report(u8 current_index, int sensor_idx, int report_id, memcpy(input_report, &gyro_input, sizeof(gyro_input)); report_size = sizeof(gyro_input); break; + case op_idx: /* op */ + tablet_mode_input.report_id = 0x11; + if (sensor_virt_addr[0] == 3) + tablet_mode_input.tablet_state = 1; + else + tablet_mode_input.tablet_state = 0;
[Severity: Low] This code relies on a hardcoded magic number 3. Should this evaluate the sensor payload using the newly defined AMD_SFH_OP_IDX_MODE_TABLET macro instead?
quoted hunk ↗ jump to hunk
diff --git a/drivers/hid/amd-sfh-hid/hid_descriptor/amd_sfh_hid_desc.h b/drivers/hid/amd-sfh-hid/hid_descriptor/amd_sfh_hid_desc.h index 882434b1501ff..ff82e47eb3ca4 100644 --- a/drivers/hid/amd-sfh-hid/hid_descriptor/amd_sfh_hid_desc.h +++ b/drivers/hid/amd-sfh-hid/hid_descriptor/amd_sfh_hid_desc.h@@ -114,4 +114,13 @@ struct hpd_input_report { u8 human_presence; } __packed; +struct tablet_mode_feature_report { + struct common_feature_property common_property; +} __packed; + +struct tablet_mode_input_report { + u8 report_id; /* Muss 0x11 (17) sein */
[Severity: Low] Is this German comment intentional? Kernel coding style typically requires all comments to be written in English.
quoted hunk ↗ jump to hunk
diff --git a/drivers/hid/amd-sfh-hid/hid_descriptor/amd_sfh_hid_report_desc.h b/drivers/hid/amd-sfh-hid/hid_descriptor/amd_sfh_hid_report_desc.h index 67ec2d6a417de..f95368534fc60 100644 --- a/drivers/hid/amd-sfh-hid/hid_descriptor/amd_sfh_hid_report_desc.h +++ b/drivers/hid/amd-sfh-hid/hid_descriptor/amd_sfh_hid_report_desc.h@@ -776,4 +776,22 @@ static const u8 hpd_report_descriptor[] = { 0X81, 0x02, /* HID Input (Data_Var_Abs) */ 0xC0 /* HID end collection */ }; + +/* tablet mode switch */ +static const u8 tablet_mode_report_descriptor[] = { +0x05, 0x0C, /* HID usage consumer electronics */ +0x09, 0x01, /* HID usage */ +0xA1, 0x00, /* HID collection (Physical) */ +0x85, 0x11, /* HID report id */ +0x0A, 0xFF, 0x02, /* HID usage (unallocated -- (ab)used for tablet mode) */
[Severity: Medium] Because the driver does not provide a custom .input_mapping callback to intercept and map this unallocated Consumer usage to SW_TABLET_MODE, will the standard hid-input core ignore it or map it to KEY_UNKNOWN (240)? Could this result in physical changes in tablet mode emitting spurious unknown key events instead of a standard EV_SW switch event? -- Sashiko AI review · https://sashiko.dev/#/patchset/ecadaac8-e222-420a-ac5b-4d529fb3317e@amd.com?part=1