• Matthew Auld's avatar
    drm/xe: fix xe_device_mem_access_get() races · a00b8f1a
    Matthew Auld authored
    It looks like there is at least one race here, given that the
    pm_runtime_suspended() check looks to return false if we are in the
    process of suspending the device (RPM_SUSPENDING vs RPM_SUSPENDED).  We
    later also do xe_pm_runtime_get_if_active(), but since the device is
    suspending or has now suspended, this doesn't do anything either.
    Following from this we can potentially return from
    xe_device_mem_access_get() with the device suspended or about to be,
    leading to broken behaviour.
    
    Attempt to fix this by always grabbing the runtime ref when our internal
    ref transitions from 0 -> 1. The hard part is then dealing with the
    runtime_pm callbacks also calling xe_device_mem_access_get() and
    deadlocking, which the pm_runtime_suspended() check prevented.
    
    v2:
     - ct->lock looks to be primed with fs_reclaim, so holding that and then
       allocating memory will cause lockdep to complain. Now that we
       unconditionally grab the mem_access.lock around mem_access_{get,put}, we
       need to change the ordering wrt to grabbing the ct->lock, since some of
       the runtime_pm routines can allocate memory (or at least that's what
       lockdep seems to suggest). Hopefully not a big deal.  It might be that
       there were already issues with this, just that the atomics where
       "hiding" the potential issues.
    v3:
     - Use Thomas Hellström' idea with tracking the active task that is
       executing in the resume or suspend callback, in order to avoid
       recursive resume/suspend calls deadlocking on itself.
     - Split the ct->lock change.
    v4:
     - Add smb_mb() around accessing the pm_callback_task for extra safety.
       (Thomas Hellström)
    v5:
     - Clarify the kernel-doc for the mem_access.lock, given that it is quite
       strange in what it protects (data vs code). The real motivation is to
       aid lockdep. (Rodrigo Vivi)
    v6:
     - Split out the lock change. We still want this as a lockdep aid but
       only for the xe_device_mem_access_get() path. Sticking a lock on the
       put() looks be a no-go, also the runtime_put() there is always async.
     - Now that the lock is gone move to atomics and rely on the pm code
       serialising multiple callers on the 0 -> 1 transition.
     - g2h_worker_func() looks to be the next issue, given that
       suspend-resume callbacks are using CT, so try to handle that.
    v7:
     - Add xe_device_mem_access_get_if_ongoing(), and use it in
       g2h_worker_func().
    v8 (Anshuman):
     - Just always grab the rpm, instead of just on the 0 -> 1 transition,
       which is a lot clearer and simplifies the code quite a bit.
    v9:
     - Make sure we also adjust the CT fast-path with if-active.
    
    Closes: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/258Signed-off-by: default avatarMatthew Auld <matthew.auld@intel.com>
    Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
    Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
    Cc: Matthew Brost <matthew.brost@intel.com>
    Cc: Anshuman Gupta <anshuman.gupta@intel.com>
    Acked-by: default avatarAnshuman Gupta <anshuman.gupta@intel.com>
    Reviewed-by: default avatarRodrigo Vivi <rodrigo.vivi@intel.com>
    Signed-off-by: default avatarRodrigo Vivi <rodrigo.vivi@intel.com>
    a00b8f1a
xe_device.h 4.3 KB