Re: [PATCH] drm/xe/mmio: Assert MMIO is available

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

On 5/28/2026 11:13 AM, Matthew Auld wrote:
> On 27/05/2026 18:54, Michal Wajdeczko wrote:
>> We shouldn't access device registers after the device was unplugged.
>> Instead of relying on the NPD splat due to zeroed xe.mmio.regs, which
>> might be unreliable anyway as not all xe_mmio are using that directly,
>> add an explicit assert during xe_mmio read/write operations to catch
>> invalid accesses to MMIO after device was unplugged.
>>
>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> ---
>> Cc: Matthew Auld <matthew.auld@intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_mmio.c | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_mmio.c b/drivers/gpu/drm/xe/xe_mmio.c
>> index 78adb303b663..b77a717f0556 100644
>> --- a/drivers/gpu/drm/xe/xe_mmio.c
>> +++ b/drivers/gpu/drm/xe/xe_mmio.c
>> @@ -10,6 +10,7 @@
>>   #include <linux/minmax.h>
>>   #include <linux/pci.h>
>>   +#include <drm/drm_drv.h>
>>   #include <drm/drm_managed.h>
>>   #include <drm/drm_print.h>
>>   @@ -128,6 +129,11 @@ void xe_mmio_init(struct xe_mmio *mmio, struct xe_tile *tile, void __iomem *ptr,
>>       mmio->tile = tile;
>>   }
>>   +static void mmio_assert_available(struct xe_mmio *mmio)
>> +{
>> +    xe_tile_assert(mmio->tile, !drm_dev_is_unplugged(&mmio->tile->xe->drm));
> 
> Yeah, I was hopeful this would work, but as per CI the unplug=true needs to happen before the devm actions run, 

yup, we mark drm.unplugged = true in our pci.remove hook:

void xe_device_remove(struct xe_device *xe)
{
...	drm_dev_unplug(&xe->drm);

while devm actions are called as part of the kobj.release hook:

static void device_release(struct kobject *kobj)
{
...	devres_release_all(dev);


> so we get a pile of false positives with this. I think the best we can do is NULL, or perhaps mmio.unplugged and check that here?

there is pci_dev_is_disconnected() but that one will likely cover real unplug scenarios, for which we might be completely not prepared ;(

but now I'm wondering if maybe those 'false positives' are actually a good one, as it might be risky to access the HW during final SW unwind, like here:

<4> [45.999577]  xe_mmio_read32+0x38/0x290 [xe]
<4> [46.000710]  ggtt_node_remove+0xbb/0xf0 [xe]
<4> [46.001167]  xe_ggtt_node_remove+0x40/0xa0 [xe]
<4> [46.001618]  xe_ggtt_remove_bo+0x87/0x250 [xe]
<4> [46.002076]  xe_ttm_bo_destroy+0xa2/0x2d0 [xe]
<4> [46.002917]  ttm_bo_release+0x70/0x310 [ttm]
<4> [46.004082]  ttm_bo_fini+0x3c/0x70 [ttm]
<4> [46.004424]  xe_gem_object_free+0x1a/0x30 [xe]
<4> [46.004857]  drm_gem_object_free+0x1d/0x40
<4> [46.005231]  xe_bo_put+0x12a/0x190 [xe]
<4> [46.005618]  __xe_bo_unpin_map_no_vm+0x49/0x70 [xe]
<4> [46.006097]  devm_action_release+0x16/0x30
<4> [46.006449]  release_nodes+0x3d/0x150

and the fact that mmio.regs is still non-NULL and points to the connected HW, is just our luck?

maybe we should kill the HW immediately on pci.remove, if it is still present, and just unwind SW state using devm/drmm actions?

> 
>> +}
>> +
>>   static void mmio_flush_pending_writes(struct xe_mmio *mmio)
>>   {
>>   #define DUMMY_REG_OFFSET    0x130030
>> @@ -146,6 +152,7 @@ u8 xe_mmio_read8(struct xe_mmio *mmio, struct xe_reg reg)
>>       u32 addr = xe_mmio_adjusted_addr(mmio, reg.addr);
>>       u8 val;
>>   +    mmio_assert_available(mmio);
>>       mmio_flush_pending_writes(mmio);
>>         val = readb(mmio->regs + addr);
>> @@ -158,6 +165,7 @@ void xe_mmio_write8(struct xe_mmio *mmio, struct xe_reg reg, u8 val)
>>   {
>>       u32 addr = xe_mmio_adjusted_addr(mmio, reg.addr);
>>   +    mmio_assert_available(mmio);
>>       trace_xe_reg_rw(mmio, true, addr, val, sizeof(val));
>>         writeb(val, mmio->regs + addr);
>> @@ -168,6 +176,7 @@ u16 xe_mmio_read16(struct xe_mmio *mmio, struct xe_reg reg)
>>       u32 addr = xe_mmio_adjusted_addr(mmio, reg.addr);
>>       u16 val;
>>   +    mmio_assert_available(mmio);
>>       mmio_flush_pending_writes(mmio);
>>         val = readw(mmio->regs + addr);
>> @@ -180,6 +189,7 @@ void xe_mmio_write32(struct xe_mmio *mmio, struct xe_reg reg, u32 val)
>>   {
>>       u32 addr = xe_mmio_adjusted_addr(mmio, reg.addr);
>>   +    mmio_assert_available(mmio);
>>       trace_xe_reg_rw(mmio, true, addr, val, sizeof(val));
>>         if (!reg.vf && IS_SRIOV_VF(mmio->tile->xe))
>> @@ -194,6 +204,7 @@ u32 xe_mmio_read32(struct xe_mmio *mmio, struct xe_reg reg)
>>       u32 addr = xe_mmio_adjusted_addr(mmio, reg.addr);
>>       u32 val;
>>   +    mmio_assert_available(mmio);
>>       mmio_flush_pending_writes(mmio);
>>         if (!reg.vf && IS_SRIOV_VF(mmio->tile->xe))
>