Re: [PATCH v2 1/2] drm/xe/i2c: Expose AMC Alert reason sysfs

[Michal Wajdeczko <michal.wajdeczko@intel.com>]

On 9/10/2026 2:41 PM, Badal Nilawar wrote:
> AMC raises an SMBUS alert before performing a power removal or
> power-cycle operation. The xe driver then places the device into
> vendor-specific wedge mode until the recovery is performed.
> 
> Expose a read-only xe_amc_alert_reason sysfs attribute to help users
> identify the required recovery action.
> 
> Assisted-by: Claude:claude-opus-4.8
> Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
> ---
> v2:
>  - Created sysfs during i2c probe time
>  - Fix Documentation (Rodrigo)
> v3:
>  - Keep limited information in "DOC:" section (Michal)
>  - Handle AMC alert none and unknown cases, document
>    in ABI
>  - Clear i2c->amc on sysfs creation failure (Sashiko)
> ---
>  .../ABI/testing/sysfs-driver-intel-xe-amc     | 25 ++++++
>  drivers/gpu/drm/xe/xe_amc.c                   | 78 ++++++++++++++++---
>  2 files changed, 93 insertions(+), 10 deletions(-)
>  create mode 100644 Documentation/ABI/testing/sysfs-driver-intel-xe-amc
> 
> 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 000000000000..3bae78d1e0d9
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-driver-intel-xe-amc
> @@ -0,0 +1,25 @@
> +What:		/sys/bus/pci/drivers/xe/.../xe_amc_alert_reason

do we need this 'xe' prefix?
it is already listed as available on xe drivers only

> +Date:		September 2026
> +KernelVersion:	7.4
> +Contact:	intel-xe@lists.freedesktop.org
> +Description:
> +		This file exposes the reason for the most recent Add-In
> +		Management Controller (AMC) alert on Intel Xe platforms.

I guess we should add "... on selected Xe platforms."

> +
> +		An AMC alert is delivered via an SMBUS interrupt and causes the
> +		device to be wedged, requiring vendor-specific recovery. This
> +		attribute is created when such an alert is handled and is
> +		available to all users as read-only.
> +
> +		Read returns a single line containing one of the following
> +		alert reasons:
> +
> +			none

other ABI documentations are quoting possible names:

			"none"

> +				No AMC alert has been received
> +			unknown

			"unknown"
...

> +				AMC alert received but with invalid reason code

maybe either change above tag to "invalid" or description: "... with unknown reason ..."

> +			firmware_download
> +			thermal_trip
> +			oob_request
> +			oob_reset
> +			catastrophic
> diff --git a/drivers/gpu/drm/xe/xe_amc.c b/drivers/gpu/drm/xe/xe_amc.c
> index edd50bf8261e..bb9d260f800f 100644
> --- a/drivers/gpu/drm/xe/xe_amc.c
> +++ b/drivers/gpu/drm/xe/xe_amc.c
> @@ -18,6 +18,7 @@
>  #include "xe_device.h"
>  #include "xe_i2c.h"
>  #include "xe_mmio.h"
> +#include "xe_printk.h"
>  
>  /**
>   * DOC: Add-In Management Controller (AMC)
> @@ -43,19 +44,23 @@ enum xe_amc_alert {
>  	AMC_ALERT_OOB_REQUEST,
>  	AMC_ALERT_OOB_RESET,
>  	AMC_ALERT_CATERR,
> +	AMC_ALERT_NONE = U8_MAX,

is this defined by AMC (as part of the AMC ABI?

if not, then maybe you need private field:

	bool alert_valid;

>  };
>  
>  static const char * const amc_alert[] = {
> -	[AMC_ALERT_FW_DOWNLOAD]		= "Firmware Download",
> -	[AMC_ALERT_THERMAL_TRIP]	= "Thermal Trip",
> -	[AMC_ALERT_OOB_REQUEST]		= "OOB Request",
> -	[AMC_ALERT_OOB_RESET]		= "OOB Reset",
> -	[AMC_ALERT_CATERR]		= "Catastrophic",
> +	[AMC_ALERT_UNKNOWN]		= "unknown",
> +	[AMC_ALERT_FW_DOWNLOAD]		= "firmware_download",
> +	[AMC_ALERT_THERMAL_TRIP]	= "thermal_trip",
> +	[AMC_ALERT_OOB_REQUEST]		= "oob_request",
> +	[AMC_ALERT_OOB_RESET]		= "oob_reset",
> +	[AMC_ALERT_CATERR]		= "catastrophic",
> +	[AMC_ALERT_NONE]		= "none",

is there any strict requirement that we need to use lowercase/underscores only?

for sysfs we are printing text line, IMO we should be good with old names

>  };
>  
>  struct xe_amc {
>  	struct xe_i2c *i2c;
>  	struct work_struct work;
> +	u8 alert_reason;
>  };
>  
>  struct amc_header {
> @@ -104,6 +109,42 @@ static const struct amc_request amc_get_alert_reason = {
>  	},
>  };
>  
> +/**
> + * DOC: AMC Alert Reason
> + *
> + * On Intel Xe platforms, AMC sends an alert notification via an SMBUS interrupt
> + * to notify events such as firmware download, thermal trip or a
> + * catastrophic error. See enum xe_amc_alert for the full list of reasons.
> + * Upon an AMC alert the device is wedged and requires vendor-specific recovery.
> + *
> + * See Documentation/ABI/testing/sysfs-driver-intel-xe-amc for the ABI
> + * specification.
> + */
> +
> +static ssize_t xe_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;
> +
> +	return sysfs_emit(buf, "%s\n", amc_alert[amc->alert_reason]);
> +}
> +static DEVICE_ATTR_RO(xe_amc_alert_reason);
> +
> +static void xe_amc_remove_alert_sysfs(struct xe_i2c *i2c)
> +{
> +	struct device *dev = i2c->drm_dev;
> +
> +	device_remove_file(dev, &dev_attr_xe_amc_alert_reason);
> +}
> +
> +static int xe_amc_create_alert_sysfs(struct xe_i2c *i2c)
> +{
> +	struct device *dev = i2c->drm_dev;
> +
> +	return device_create_file(dev, &dev_attr_xe_amc_alert_reason);

can't we use managed variant of sysfs initialization?

> +}
> +
>  static void xe_amc_work(struct work_struct *work)
>  {
>  	const struct amc_request *request = &amc_get_alert_reason;
> @@ -158,12 +199,19 @@ static void xe_amc_work(struct work_struct *work)
>  	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);
> +
> +		dev_warn(amc->i2c->drm_dev,
> +			 "AMC Alert: %s (%u)\n", amc_alert[alert_reason], alert_reason);

maybe we should use xe_log(AMC) ?
> +		amc->alert_reason = alert_reason;
> +		xe_device_set_wedged_method(xe, DRM_WEDGE_RECOVERY_VENDOR);
> +		xe_device_declare_wedged(xe);
>  		break;
> +	}
>  	default:
> -		dev_warn(amc->i2c->drm_dev, "unknown AMC alert: %d\n", alert_reason);
> +		amc->alert_reason = AMC_ALERT_UNKNOWN;

hmm, is AMC_ALERT_UNKNOWN(0) actual AMC ABI definition or it is just a driver define?

shouldn't we still store alert_reason code?
then in sysfs you will be able to emit:

		"Unknown alert %#x"

> +		dev_warn(amc->i2c->drm_dev, "AMC Alert: unknown (%u)\n", alert_reason);
>  		break;
>  	}
>  }
> @@ -176,6 +224,7 @@ void xe_amc_handle_alert(struct xe_i2c *i2c)
>  int xe_amc_init(struct xe_i2c *i2c)
>  {
>  	struct xe_amc *amc;
> +	int ret;
>  
>  	amc = kzalloc_obj(*amc);
>  	if (!amc)
> @@ -185,13 +234,22 @@ int xe_amc_init(struct xe_i2c *i2c)
>  	i2c->amc = amc;
>  	amc->i2c = i2c;
>  
> -	return 0;
> +	amc->alert_reason = AMC_ALERT_NONE;
> +	ret = xe_amc_create_alert_sysfs(i2c);
> +	if (ret) {
> +		kfree(i2c->amc);
> +		i2c->amc = NULL;
> +	}
> +
> +	return ret;
>  }
>  
>  void xe_amc_exit(struct xe_i2c *i2c)
>  {
>  	if (i2c->amc) {
> +		xe_amc_remove_alert_sysfs(i2c);
>  		cancel_work_sync(&i2c->amc->work);
>  		kfree(i2c->amc);
> +		i2c->amc = NULL;
>  	}
>  }