Re: [PATCH -next 2/3] HID: amd_sfh: Add operating-mode telemetry sysfs attributes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] amd_sfh: torn read of telemetry data via memcpy_fromio()
- [Low] amd_sfh: build failures due to undefined identifiers
--

commit 74c8f353108e77a41e51aa1613b0b46c89693fcd
Author: Basavaraj Natikar <Basavaraj.Natikar@amd.com>

HID: amd_sfh: Add operating-mode telemetry sysfs attributes

The Sensor Fusion Hub reports additional operating-mode telemetry
through the SRA sensor, such as in-bag and out-of-bag detection,
on-table or on-lap placement, hinge angle, lid status and the platform
device mode.

> diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_sysfs.c b/drivers/hid/amd-sfh-hid/amd_sfh_sysfs.c
> index db1cc205d25b9..8af4df09760f0 100644
> --- a/drivers/hid/amd-sfh-hid/amd_sfh_sysfs.c
> +++ b/drivers/hid/amd-sfh-hid/amd_sfh_sysfs.c

[ ... ]

> @@ -41,19 +43,78 @@ static ssize_t hpd_store(struct device *dev,
>  }
>  static DEVICE_ATTR_RW(hpd);
>  
> +/* Operating-mode telemetry reported by the SRA sensor, exposed read-only. */
> +static int amd_sfh_read_op_mode(struct amd_mp2_dev *mp2, struct sfh_op_mode *mode)
> +{
> +	struct sfh_accel_data accel_data;
> +	void __iomem *sensoraddr;
> +
> +	if (!mp2->sfh1_1_ops || !mp2->dev_en.is_sra_present)
> +		return -ENODEV;
> +
> +	if (mp2->mp2_ver >= MP2_VER_1_2) {

[Severity: Low]
Is MP2_VER_1_2 defined in this tree? Compiling this patch results in a build
failure because MP2_VER_1_2 is undeclared.

> +		sensoraddr = mp2->vsbase +
> +			(SRA_IDX * SENSOR_DATA_MEM_SIZE_DEFAULT) +
> +			OFFSET_SENSOR_DATA_DEFAULT;
> +		memcpy_fromio(&accel_data, sensoraddr, sizeof(struct sfh_accel_data));

[Severity: Medium]
Could this memcpy_fromio() lead to torn reads? Since memcpy_fromio() performs
bulk non-atomic reads, concurrent firmware updates to the shared MMIO region
might expose a corrupted mixture of old and new telemetry states to userspace
when reading these sysfs attributes.

Should this use an atomic read like readl() instead?

> +		mode->val = accel_data.c2p3_data;

[Severity: Low]
Does struct sfh_accel_data contain a c2p3_data field? Looking at
drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.h:

struct sfh_accel_data {
        struct sfh_common_data commondata;
        struct sfh_float32 acceldata;
        u32 accelstatus;
};

It appears the field c2p3_data is missing, which causes a build failure
when compiling this driver.

> +	} else {
> +		mode->val = readl(mp2->mmio + amd_get_c2p_val(mp2, 3));
> +	}
> +
> +	return 0;
> +}

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