Re: [PATCH v3] drm/xe/i2c: Expose AMC Alert reason sysfs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] xe: data race and incorrect ordering in AMC alert sysfs read
- [Low] sysfs: ABI documentation mismatch for xe_amc_alert_reason attribute
--

commit 18d032c7e5837ea6476300e788f566784a9575ac
Author: Badal Nilawar <badal.nilawar@intel.com>
Subject: drm/xe/i2c: Expose AMC Alert reason sysfs

This commit introduces a read-only sysfs attribute to expose the AMC
(Add-In Management Controller) alert reason. It adds documentation and
implements the sysfs creation and value update logic in xe_amc.c.

> diff --git a/Documentation/ABI/testing/sysfs-driver-intel-xe-amc b/Documentation/ABI/testing/sysfs-driver-intel-xe-amc
> new file mode 100644
> index 0000000000000..34927fa5dcfe8
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-driver-intel-xe-amc
> @@ -0,0 +1,24 @@
> +What:		/sys/bus/pci/drivers/xe/.../xe_amc_alert_reason

[ ... ]

> diff --git a/drivers/gpu/drm/xe/xe_amc.c b/drivers/gpu/drm/xe/xe_amc.c
> index edd50bf8261ec..7f21044fd1b7b 100644
> --- a/drivers/gpu/drm/xe/xe_amc.c
> +++ b/drivers/gpu/drm/xe/xe_amc.c

[ ... ]

> @@ -104,6 +108,45 @@ static const struct amc_request amc_get_alert_reason = {
>  	},
>  };
>  
> +/**
> + * DOC: AMC Alert Reason
> + *
> + * Expose AMC alert reason through device sysfs
> + *
> + * See Documentation/ABI/testing/sysfs-driver-intel-xe-amc for the ABI
> + * specification.
> + */
> +
> +static ssize_t amc_alert_reason_show(struct device *dev,
> +				     struct device_attribute *attr, char *buf)
> +{
> +	struct xe_device *xe = pdev_to_xe_device(to_pci_dev(dev));
> +	struct xe_amc *amc = xe->i2c->amc;
> +	u8 alert_reason;
> +
> +	if (!amc->alert_seen)

[Severity: Medium]
Could this read of amc->alert_seen cause a data race?

The sysfs reader relies on this boolean flag to decide whether to read
alert_reason, but it lacks memory barriers or READ_ONCE(). A concurrent
sysfs reader might observe an inconsistent state here while a new alert
is being processed.

> +		return sysfs_emit(buf, "none\n");
> +
> +	alert_reason = READ_ONCE(amc->alert_reason);
> +
> +	return sysfs_emit(buf, "%s\n", amc_alert[alert_reason]);
> +}
> +static DEVICE_ATTR_RO(amc_alert_reason);

[Severity: Low]
Does this macro create an attribute named amc_alert_reason, rather than the
documented ABI name?

The ABI documentation specifies the attribute name as xe_amc_alert_reason,
but this macro creates it without the xe_ prefix. Userspace tools relying on
the ABI documentation might fail to find the sysfs file.

[ ... ]

> @@ -153,17 +196,26 @@ static void xe_amc_work(struct work_struct *work)
>  out_reassert_interrupt:
>  	xe_mmio_rmw32(amc->i2c->mmio, I2C_CONFIG_CMD, PCI_COMMAND_INTX_DISABLE, 0);
>  
> +	amc->alert_seen = true;
> +

[Severity: Medium]
Does setting amc->alert_seen to true before updating amc->alert_reason
expose a race window?

A concurrent sysfs read of the alert attribute could observe
alert_seen == true and then read an uninitialized or stale alert_reason value,
returning an incorrect or default alert reason to userspace.

>  	switch (alert_reason) {
>  	case AMC_ALERT_FW_DOWNLOAD:
>  	case AMC_ALERT_THERMAL_TRIP:
>  	case AMC_ALERT_OOB_REQUEST:
>  	case AMC_ALERT_OOB_RESET:
> -	case AMC_ALERT_CATERR:
> -		dev_warn(amc->i2c->drm_dev, "AMC Alert: %s\n", amc_alert[alert_reason]);
> -		xe_device_declare_wedged(i2c_client_to_xe_device(client));
> +	case AMC_ALERT_CATERR: {
> +		struct xe_device *xe = i2c_client_to_xe_device(client);
> +
> +		WRITE_ONCE(amc->alert_reason, alert_reason);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261001121302.4102629-2-badal.nilawar@intel.com?part=1