Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] drm/xe: TOCTOU race in xe_display_register leaves display active without IRQs - [High] drm/xe: double suspend of display hardware due to wedge during PM --
commit f1762c4030fdcf19e70d9970658fdb7e79d5019c Author: Arvind Yadav <[email protected]> drm/xe: Isolate a wedged device before notifying userspace A permanently wedged device must stop interrupt and DMA activity before userspace is notified that recovery is required. The commit runs the sleepable isolation steps from the wedge worker. > diff --git a/drivers/gpu/drm/xe/display/xe_display.c > b/drivers/gpu/drm/xe/display/xe_display.c > --- a/drivers/gpu/drm/xe/display/xe_display.c > +++ b/drivers/gpu/drm/xe/display/xe_display.c > @@ -150,21 +150,27 @@ void xe_display_register(struct xe_device *xe) > { > struct intel_display *display = xe->display; > > - if (!xe->info.probe_display) > + if (!xe->info.probe_display || xe->display_registered) > return; > > intel_display_driver_register(display); > intel_display_driver_runtime_pm_enable(display); > + > + xe->display_registered = true; [Severity: High] Could the async wedge worker race with the display probe thread here? The display is registered without any locking to prevent concurrent isolation by the async wedge worker. > } > > void xe_display_unregister(struct xe_device *xe) > { > struct intel_display *display = xe->display; > > - if (!xe->info.probe_display) > + if (!xe->info.probe_display || !xe->display_registered) > return; [Severity: High] The wedge worker reads xe->display_registered unprotected and returns early if registration is still in progress in the probe thread. [ ... ] > @@ -172,7 +178,8 @@ void xe_display_shutdown(struct xe_device *xe) > { > struct intel_display *display = xe->display; > > - if (!xe->info.probe_display) > + if (!xe->info.probe_display || !xe->display_registered || > + xe->display_shutdown) > return; > > intel_display_driver_shutdown(display); > @@ -180,6 +187,8 @@ void xe_display_shutdown(struct xe_device *xe) > intel_opregion_suspend(display, PCI_D3cold); > > intel_dmc_suspend(display); > + > + xe->display_shutdown = true; > } [Severity: High] When a synchronous wedge worker calls xe_device_wedged_isolate() during suspend, it executes xe_display_shutdown(). Since xe->display_shutdown is false, it proceeds to call intel_dmc_suspend(), intel_opregion_suspend(), and intel_display_driver_shutdown() a second time. Could this cause a wakeref underflow and unbalanced runtime PM state for the display hardware? [ ... ] > diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c > --- a/drivers/gpu/drm/xe/xe_device.c > +++ b/drivers/gpu/drm/xe/xe_device.c [ ... ] > +static void xe_device_wedged_isolate(struct xe_device *xe) > +{ [ ... ] > + /* Shut down display hardware before stopping device access. */ > + xe_display_shutdown(xe); > + xe_display_unregister(xe); > + > + /* Stop interrupt and DMA activity before changing mappings. */ > + xe_irq_suspend(xe); > + pci_clear_master(to_pci_dev(xe->drm.dev)); [Severity: High] If the device wedges asynchronously during device probe, could the wedge worker miss the display unregistration because xe->display_registered is not yet true, but then continue to disable hardware access? If the probe thread then proceeds to complete xe_display_register(), the display remains registered and exposed to userspace, but the device has its interrupts and DMA (PCI master) permanently disabled. This could cause userspace modesets and page flips to time out and hang since VBLANK interrupts will never fire. [ ... ] > diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c > --- a/drivers/gpu/drm/xe/xe_pci.c > +++ b/drivers/gpu/drm/xe/xe_pci.c > @@ -1325,19 +1325,38 @@ static int xe_pci_suspend(struct device *dev) > { [ ... ] > + } else { > + err = xe_pm_suspend(xe); > + xe_device_io_put(io_idx); > + > + if (xe_device_wedged(xe)) { > + wait_for_completion(&xe->wedged.prepared); > + flush_work(&xe->wedged.work); > + } else if (err) { > + return err; > + } > + } [Severity: High] Can a wedge during suspend cause a double suspend of the display hardware? If a wedge event occurs during xe_pm_suspend(), the display has already been suspended via xe_display_pm_suspend(). Since xe_pm_suspend() does not set the xe->display_shutdown flag, this flush_work() will execute the wedge worker synchronously, resulting in xe_display_shutdown() suspending the hardware again. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=11
