Re: [PATCH v3 00/17] drm/panthor: Fix the unplug logic

[Liviu Dudau <liviu.dudau@arm.com>]

On Thu, Aug 13, 2026 at 01:23:20PM +0200, Boris Brezillon wrote:
> +Danilo, since you worked on the 'bound lifetime stuff in rust, and I
> feel this is related to the problem I'm trying to fix here.
> 
> On Thu, 13 Aug 2026 12:56:58 +0200
> Boris Brezillon <boris.brezillon@collabora.com> wrote:
> 
> > The current unplug logic is broken in multiple ways. This is an attempt
> > at addressing the various problems found along the way (some were
> > reported by Sashiko, others have been found while trying to address
> > Sashiko's concerns).
> > 
> > Sending a new version even though v2 didn't receive any human review
> > just to try and address the new stuff pointed out by Sashiko.

Sorry, I was on holiday at the beginning of the week, back today.

> 
> Just a note I forgot to add to my cover letter. I've already spent way
> more time than I wanted on this, not just because Sashiko keeps finding
> new issues at each of my attempt, but also because the whole idea of
> pretending a device on a platform bus is unplugged and can't harm us is
> doomed. This is not an hot-pluggable bus, and the device is still there,
> so, unless we can be absolutely sure it's inactive (which a RESET can
> provide, but RESETs are fallible) we just have two options:
> 
> 1. prevent the device from going away until we managed to properly
>    shutdown the GPU

That's going to be event harder with the upcoming HW where the GPU
slice can be made inaccessible by an arbiter.

> 
> 2. make sure all resources the HW might have its hands on at the time
>    the failure of RESET in the unplug path happened are leaked

There is another option which is to make sure that the HW can only
access the dummy pages. We're still in control of the MMU and the page
tables, once we update those and flush them we should be safe in the
knowledge that the HW cannot access live resources.


> 
> Option 1 is no longer possible since platform_driver::remove() can't
> return an error. That leaves options 2, which is basically what this
> patchset is doing, but the whole idea of leaking resources when the
> final RESET in the unplug path fails has various nasty implications,
> like the fact we end up with dangling drm_device (drm_gpuvm retains a
> ref, and each GPU mapping we kept alive in the gpuvm is what keeps the
> gpuvm and the BOs alive). In practice, there should be no one
> triggering operations on this drm_device, because all the user-facing
> interfaces have been shutdown by drm_dev_unregister() (which is called
> by drm_dev_unplug()), but as things stand now, this drm_device still
> has access to module-specific vtables, and there's nothing retaining
> the module either.
> 
> TLDR; this is all super fragile stuff, on the other hand the current
> situation is probably even worse. so if anyone has any idea how to
> handle this properly (or at least a bit better than we do), please let
> me know. I know a lot of this stuff is currently being considered as
> part of the drm-rust abstractions, so hopefully we have a long-term
> solution for rust drivers, but I'd really like a short-term solution
> for panthor that doesn't involve nasty tricks or overly complex
> refactoring.

I think some of the pain we're suffering comes from the overlap (that
you've tried to address in this series) between the resources that
are visible to the HW and the ones that are visible to user space. The
split of AS and VM is the right thing to do.

My proposal for handling the unplugging would be to have race as quick
as possible to the MMU unplug and then free up all BOs and VMs that
were allocated at the request of user space, then go back and free
the kernel BOs. Then hopefully we should be in a position where there
are no GPU mappings and we can unplug the drm_gpuvm.

Best regards,
Liviu

> 
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> > ---
> > Changes in v3:
> > - Fix a race in the reset reschedule logic we added to
> >   panthor_device_resume() (missing smp_mb__after_atomic())
> > - Fix a VM leak when reset and suspend are racing with each other
> > - Add missing drm_dev_enter/exit() sections
> > - Insert the groups in the user_owned list even if the group creation
> >   happens during a reset
> > - Try to document why some of the issues pointed out by Sashiko are
> >   either not real issues, or are expected (either fixed in a later
> >   commits, or just expected behavior)
> > - Fix a race between panthor_device_unplug() and vm_prep_for_cleanup()
> >   (introduced in v2)
> > - Link to v2: https://patch.msgid.link/20260811-panthor-unplug-fixes-v2-0-6b583e37f9ae@collabora.com
> > 
> > Changes in v2:
> > - Fix UAFs caused by deferred cleanup works
> > - Fix UAFs caused by open FDs closed after unplug
> > - Fix deadlock when device_unplug() is called from the reset work
> > - Make sure reset requests are not lost in the resume and post_reset
> >   paths
> > - Fix a deadlock in the suspend path
> > - Fix a clk prepare_enable leak in the unplug path
> > - Don't use a drmm_action to flush the cleanup queue (this could cause
> >   UAFs)
> > - Drop the now unused panthor_vm::unusable field
> > - Keep track of user owned resources to prevent leaks and/or UAFs
> > - Link to v1: https://patch.msgid.link/20260804-panthor-unplug-fixes-v1-0-abbbd2d41b13@collabora.com
> > 
> > ---
> > Boris Brezillon (17):
> >       drm/panthor: Disable reset work before unplug
> >       drm/panthor: Further delay reset work enablement
> >       drm/panthor: Make sure reset requests in the resume path are not lost
> >       drm/panthor: Make sure reset requests in the post reset path are not lost
> >       drm/panthor: Flush the cleanup_wq in the unplug path
> >       drm/panthor: Drop unused vm argument passed to panthor_vm_prepare_sync_only_op_ctx()
> >       drm/panthor: Move the debugfs initialization to panthor_device.c
> >       drm/panthor: Split panthor_vm
> >       drm/panthor: Add fine-grained restrictions on VMs
> >       drm/panthor: Check AS state before disabling
> >       drm/panthor: Don't pre-allocate VMAs or page tables when preparing a full VM unmap
> >       drm/panthor: Make the VM cleanup path more robust against UAF
> >       drm/panthor: Track user owned VMs
> >       drm/panthor: Track user owned groups
> >       drm/panthor: Fix the unplug logic
> >       drm/panthor: Add a debugfs knob to simulate unplug failures
> >       drm/panthor: Add a debugfs knobs to simulate reset failures
> > 
> >  drivers/gpu/drm/panthor/panthor_device.c |  189 +++-
> >  drivers/gpu/drm/panthor/panthor_device.h |   38 +
> >  drivers/gpu/drm/panthor/panthor_drv.c    |  132 ++-
> >  drivers/gpu/drm/panthor/panthor_fw.c     |    9 +-
> >  drivers/gpu/drm/panthor/panthor_mmu.c    | 1493 ++++++++++++++++++------------
> >  drivers/gpu/drm/panthor/panthor_mmu.h    |    4 +-
> >  drivers/gpu/drm/panthor/panthor_sched.c  |  132 ++-
> >  7 files changed, 1344 insertions(+), 653 deletions(-)
> > ---
> > base-commit: 44e9eb5a762142a4aa46c0b5da7c39bfeb78910e
> > change-id: 20260804-panthor-unplug-fixes-7927b3ddc2f9
> > 
> > Best regards,
> > --  
> > Boris Brezillon <boris.brezillon@collabora.com>
> > 
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯