Re: [PATCH 1/4] HID: amd_sfh: Add DMI quirk and module param to disable interrupts

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The DMI quirk for ASUS VivoBook inadvertently ENABLES hardware interrupts, directly contradicting the commit message which states the device requires them DISABLED.
- [Medium] The DMI quirk unconditionally overwrites the module parameter during probe, making it impossible for a user to override the quirk on matching hardware.
- [Low] The module parameter permissions are read-only, preventing the 'runtime override' advertised in the commit message.
--

commit ec52ca66216e002a750c44dedd096bba34993086
Author: Helge Bahmann <hcb@chaoticmind.net>

HID: amd_sfh: Add DMI quirk and module param to disable interrupts

This commit exposes the intr_disable variable as a module parameter
and adds a DMI quirk for the ASUSTeK VivoBook.

> Expose intr_disable as a module parameter to allow runtime override.

[Severity: Low]
The commit message mentions allowing a runtime override, but the module
parameter is defined with 0444 permissions. Doesn't this make it read-only
and prevent runtime modification via sysfs?

> Add DMI quirk for ASUSTeK VivoBook TP420UA/TM420UA which requires
> interrupts disabled for correct sensor operation.
> 
> Signed-off-by: Helge Bahmann <hcb@chaoticmind.net>
> Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>

> diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c b/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c
> index eda26a094d3f1..92486f25460bc 100644
> --- a/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c
> +++ b/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c
> @@ -41,6 +41,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);

[Severity: Medium]
Does the DMI quirk evaluation in amd_mp2_pci_probe() unconditionally
overwrite this module parameter?

During probe, dmi_check_system(dmi_sfh_table) is called, which executes
mp2_disable_intr() for matching hardware. If a user attempts to override the
interrupt setting via module parameters, won't their setting be silently
ignored and clobbered by the quirk's callback?

> +MODULE_PARM_DESC(intr_disable, "override the interrupt disable sensor bit");
>  
>  static int amd_sfh_wait_response_v2(struct amd_mp2_dev *mp2, u8 sid, u32 sensor_sts)

[ ... ]

> @@ -313,6 +315,13 @@ static const struct dmi_system_id dmi_sfh_table[] = {
>  			DMI_MATCH(DMI_PRODUCT_NAME, "HP ProBook x360 435 G7"),
>  		},
>  	},
> +	{
> +		.callback = mp2_disable_intr,

[Severity: High]
Does this quirk actually enable interrupts rather than disable them?

The commit message states the ASUS VivoBook requires interrupts disabled.
However, the mp2_disable_intr() callback sets the global intr_disable = false.

By setting this to false, the hardware is configured with
cmd_v2.intr_disable = 0, which turns interrupts on. Is this the intended
behavior for a device that requires them to be disabled?

> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "VivoBook_ASUSLaptop TP420UA_TM420UA"),
> +		},
> +	},
>  	{}
>  };
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260902134431.3146117-1-Basavaraj.Natikar@amd.com?part=1