Re: [PATCH v13 16/16] drm/xe/vsec: Support survivability mode for CRI

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] drm/xe/vsec: NULL sc->mmio dereference in wait_for_fw() on survivability error
--

commit 6e126caaac7b556b33b3c1891921257efd2a418a
Author: Michael J. Ruhl <michael.j.ruhl@intel.com>

drm/xe/vsec: Support survivability mode for CRI

This commit adds a VSEC configuration for Crescent Island in survivability
mode to expose the PUNIT crashlog PMT capability and initializes the SoC
remapper during boot survivability mode.

> diff --git a/drivers/gpu/drm/xe/xe_survivability_mode.c b/drivers/gpu/drm/xe/xe_survivability_mode.c
> index d73cdad0b3a9c..4ce84bafcbf57 100644
> --- a/drivers/gpu/drm/xe/xe_survivability_mode.c
> +++ b/drivers/gpu/drm/xe/xe_survivability_mode.c
[ ... ]
> @@ -352,6 +353,7 @@ static int enable_boot_survivability_mode(struct pci_dev *pdev)
>  
>  	xe_heci_gsc_init(xe);
>  
> +	xe_soc_remapper_init(xe);
>  	xe_vsec_init(xe);
>  
>  	if (survivability->fdo_mode) {
> diff --git a/drivers/gpu/drm/xe/xe_vsec.c b/drivers/gpu/drm/xe/xe_vsec.c
> index 1ed904ec14965..6d4ef9cb9f60f 100644
> --- a/drivers/gpu/drm/xe/xe_vsec.c
> +++ b/drivers/gpu/drm/xe/xe_vsec.c
[ ... ]
> @@ -316,9 +336,10 @@ static void cri_late_bind_probe_work(struct work_struct *work)
>  
>  static bool wait_for_fw(struct xe_device *xe)
>  {
> +	struct xe_survivability *survivability = &xe->survivability;
>  	int retries = VSEC_LATE_BIND_RETRY;  /* wait up to 20 secs */
>  
> -	if (xe->info.platform != XE_CRESCENTISLAND)
> +	if (xe->info.platform != XE_CRESCENTISLAND || survivability->mode)
>  		return true;
>  
>  	while (retries--) {

[Severity: Medium]
Can this dereference a NULL xe->sc.mmio pointer and lock an uninitialized
xe->sc.cmd_lock mutex if enable_boot_survivability_mode() fails after
xe_vsec_init()?

During early probe in xe_device_probe_early(), xe_sysctrl_init() has not run
yet, so xe->sc.cmd_lock is uninitialized and xe->sc.mmio is NULL.
enable_boot_survivability_mode() sets survivability->mode = true and calls
xe_vsec_init(xe), which registers the crashlog auxiliary device via
intel_vsec_register() with devm-managed cleanup:

drivers/gpu/drm/xe/xe_survivability_mode.c:enable_boot_survivability_mode() {
	...
	survivability->mode = true;

	xe_heci_gsc_init(xe);

	xe_soc_remapper_init(xe);
	xe_vsec_init(xe);

	if (survivability->fdo_mode) {
		ret = xe_nvm_init(xe);
		if (ret)
			goto failed;
	}

	ret = xe_i2c_probe(xe);
	if (ret)
		goto failed;
	...
failed:
	xe_log_err_fatal(pdev, SURVIVABILITY, ret, "Failed to enable Boot Mode!\n");
	survivability->mode = false;
	return ret;
}

If xe_nvm_init() or xe_i2c_probe() fails, survivability->mode is reset to
false while the VSEC crashlog device and its sysfs attributes remain
registered until PCI probe teardown runs devres_release_all().

If userspace reads the crashlog sysfs attributes during that teardown window,
xe_pmt_telem_read() or xe_pmt_read_reg() calls wait_for_fw(xe), which sees
survivability->mode == false and calls:

  wait_for_fw(xe)
    -> xe_sysctrl_is_oobmsm_fw_ready(xe)
      -> xe_sysctrl_send_command(xe, ...)
        -> guard(mutex)(&sc->cmd_lock)
        -> sysctrl_send_command(sc, ...)

Because xe_sysctrl_init() was never called, would this lock the uninitialized
sc->cmd_lock and dereference the NULL sc->mmio pointer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102221314.119028-18-michael.j.ruhl@intel.com?part=16