Re: [Intel-xe] [PATCH] drm/xe: fix xe_device_mem_access_get() race

["Thomas Hellström" <thomas.hellstrom@linux.intel.com>]

On 5/4/23 07:31, Matthew Brost wrote:
> On Wed, May 03, 2023 at 04:28:02PM +0100, Matthew Auld wrote:
>> 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, and then wrap the whole thing with a lock
>> to ensure callers are serialized.
>>
>> Closes: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/258
>> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_device.c       | 22 ++++++++++++----------
>>   drivers/gpu/drm/xe/xe_device_types.h |  5 +++++
>>   drivers/gpu/drm/xe/xe_pm.c           |  9 ++-------
>>   drivers/gpu/drm/xe/xe_pm.h           |  2 +-
>>   4 files changed, 20 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
>> index 45d6e5ff47fd..5f6554bb34d2 100644
>> --- a/drivers/gpu/drm/xe/xe_device.c
>> +++ b/drivers/gpu/drm/xe/xe_device.c
>> @@ -209,6 +209,8 @@ struct xe_device *xe_device_create(struct pci_dev *pdev,
>>   
>>   	xe->ordered_wq = alloc_ordered_workqueue("xe-ordered-wq", 0);
>>   
>> +	drmm_mutex_init(&xe->drm, &xe->mem_access.lock);
>> +
>>   	err = xe_display_create(xe);
>>   	if (WARN_ON(err))
>>   		goto err_put;
>> @@ -404,26 +406,26 @@ u32 xe_device_ccs_bytes(struct xe_device *xe, u64 size)
>>   
>>   void xe_device_mem_access_get(struct xe_device *xe)
>>   {
>> -	bool resumed = xe_pm_runtime_resume_if_suspended(xe);
>> -	int ref = atomic_inc_return(&xe->mem_access.ref);
>> +	int ref;
>>   
>> +	mutex_lock(&xe->mem_access.lock);
>> +	ref = atomic_inc_return(&xe->mem_access.ref);
> Drive by comment, if we have a lock then why does this need to be
> atomic?
>
> For the review maybe loop in Maarten because if I recall correctly he
> changed this to an atomic to fix some lockdep splat.
>
> Matt

I also stumbled upon this race some time ago but didn't dig deeper. I 
agree we need a mutex here but unless we access the mem_access.ref 
somewhere without the mutex, doesn't need to be atomic.

An alternative construct is to grab the mutex only on 0->1 and 1->0 
transitions and keep the ref atomic, but I'm not sure how much is gained 
assuming nearly all mutex locks are not contended.

/Thomas


>>   	if (ref == 1)
>> -		xe->mem_access.hold_rpm = xe_pm_runtime_get_if_active(xe);
>> -
>> -	/* The usage counter increased if device was immediately resumed */
>> -	if (resumed)
>> -		xe_pm_runtime_put(xe);
>> +		xe->mem_access.hold_rpm = xe_pm_runtime_resume_and_get(xe);
>> +	mutex_unlock(&xe->mem_access.lock);
>>   
>>   	XE_WARN_ON(ref == S32_MAX);
>>   }
>>   
>>   void xe_device_mem_access_put(struct xe_device *xe)
>>   {
>> -	bool hold = xe->mem_access.hold_rpm;
>> -	int ref = atomic_dec_return(&xe->mem_access.ref);
>> +	int ref;
>>   
>> -	if (!ref && hold)
>> +	mutex_lock(&xe->mem_access.lock);
>> +	ref = atomic_dec_return(&xe->mem_access.ref);
>> +	if (!ref && xe->mem_access.hold_rpm)
>>   		xe_pm_runtime_put(xe);
>> +	mutex_unlock(&xe->mem_access.lock);
>>   
>>   	XE_WARN_ON(ref < 0);
>>   }
>> diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
>> index 1cb404e48aaa..e8d320f93852 100644
>> --- a/drivers/gpu/drm/xe/xe_device_types.h
>> +++ b/drivers/gpu/drm/xe/xe_device_types.h
>> @@ -257,6 +257,11 @@ struct xe_device {
>>   	 * triggering additional actions when they occur.
>>   	 */
>>   	struct {
>> +		/**
>> +		 * @lock: Serialize xe_device_mem_access users, and protect the
>> +		 * below internal state.
>> +		 */
>> +		struct mutex lock;
>>   		/** @ref: ref count of memory accesses */
>>   		atomic_t ref;
>>   		/** @hold_rpm: need to put rpm ref back at the end */
>> diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c
>> index b7b57f10ba25..b2ffa001e6f7 100644
>> --- a/drivers/gpu/drm/xe/xe_pm.c
>> +++ b/drivers/gpu/drm/xe/xe_pm.c
>> @@ -210,14 +210,9 @@ int xe_pm_runtime_put(struct xe_device *xe)
>>   	return pm_runtime_put_autosuspend(xe->drm.dev);
>>   }
>>   
>> -/* Return true if resume operation happened and usage count was increased */
>> -bool xe_pm_runtime_resume_if_suspended(struct xe_device *xe)
>> +bool xe_pm_runtime_resume_and_get(struct xe_device *xe)
>>   {
>> -	/* In case we are suspended we need to immediately wake up */
>> -	if (pm_runtime_suspended(xe->drm.dev))
>> -		return !pm_runtime_resume_and_get(xe->drm.dev);
>> -
>> -	return false;
>> +	return !pm_runtime_resume_and_get(xe->drm.dev);
>>   }
>>   
>>   int xe_pm_runtime_get_if_active(struct xe_device *xe)
>> diff --git a/drivers/gpu/drm/xe/xe_pm.h b/drivers/gpu/drm/xe/xe_pm.h
>> index 6a885585f653..1b4c15b5e71a 100644
>> --- a/drivers/gpu/drm/xe/xe_pm.h
>> +++ b/drivers/gpu/drm/xe/xe_pm.h
>> @@ -19,7 +19,7 @@ int xe_pm_runtime_suspend(struct xe_device *xe);
>>   int xe_pm_runtime_resume(struct xe_device *xe);
>>   int xe_pm_runtime_get(struct xe_device *xe);
>>   int xe_pm_runtime_put(struct xe_device *xe);
>> -bool xe_pm_runtime_resume_if_suspended(struct xe_device *xe);
>> +bool xe_pm_runtime_resume_and_get(struct xe_device *xe);
>>   int xe_pm_runtime_get_if_active(struct xe_device *xe);
>>   
>>   #endif
>> -- 
>> 2.40.0
>>