Re: [PATCH v3 04/12] drm/xe/cper: Prepare CPER record

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Reading the PF-only CRI_FRU_ID SOC_BASE MMIO resource from a Virtual Function (VF) context will cause a hardware timeout.

Pre-existing issues:
- [High] Logic inversion in `xe_emit_hardware_error_cper` prevents emitting CPER records for hardware errors.
--

commit 9f7667556e38b9fe8c03f7e28c7370b664d666ce
Author: Badal Nilawar <badal.nilawar@intel.com>
Date:   Sun Sep 6 17:26:09 2026 +0000

    drm/xe/cper: Prepare CPER record
    
    Initialize Intel-specific CPER metadata and construct
    CPER record for Intel GPU hardware errors.

> diff --git a/drivers/gpu/drm/xe/xe_cper.c b/drivers/gpu/drm/xe/xe_cper.c
> index f04a91223a43c..31ca53ce1aa75 100644
> --- a/drivers/gpu/drm/xe/xe_cper.c
> +++ b/drivers/gpu/drm/xe/xe_cper.c
[ ... ]
> +static guid_t read_fru_id(struct xe_device *xe)
> +{
> +	struct xe_mmio *mmio = xe_root_tile_mmio(xe);
> +	guid_t guid = GUID_INIT(0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0);
> +	u64 val;
> +
> +	val = xe_mmio_read64_2x32(mmio, CRI_FRU_ID);
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
Does this code execute safely on Virtual Functions?

Accessing PF-only SOC_BASE registers from a VF causes hardware failures or
MMIO timeouts. The CRI_FRU_ID register is in the SOC_BASE region.

When a hardware error is logged from a Virtual Function,
xe_emit_hardware_error_cper() calls xe_cper_record_emit() which
unconditionally calls read_fru_id(). Shouldn't this read be guarded with an
IS_SRIOV_VF(xe) check to prevent a driver crash or wedged device?

> +
> +	memcpy(&guid, &val, sizeof(val));
> +
> +	return guid;
> +}
[ ... ]
> @@ -48,5 +209,15 @@ void xe_emit_hardware_error_cper(struct pci_dev *pdev, int cper_sev, enum xe_sig
>  		}
>  	}
>  
> +	xe_cper_init_intel_err_hdr(xe,
> +				   (const u8 *)counter,
> +				   counter_response->timestamp,
> +				   sigid,
> +				   counter_response->value,
> +				   &ihdr);
> +
> +	xe_cper_record_emit(xe, cper_sev, &INTEL_CPER_NOTIFY_GPU_ERROR,
> +			    &ihdr, NULL, 0);
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
This isn't a bug introduced by this patch, but will this code ever execute
for hardware errors?

Earlier in xe_emit_hardware_error_cper(), there is a pre-existing check:

	if ((int)sigid >= INTEL_SIGID_GPU_XE_HARDWARE_START)
		return;

This appears to abort execution for all valid hardware SIGIDs, meaning the
new CPER record generation will act as dead code and fail to emit records
for genuine hardware errors. Could we fix that logic inversion?

> +
>  	/* TODO */
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260906172604.2215987-14-badal.nilawar@intel.com?part=4