Re: [PATCH v4 04/11] drm/xe/configfs: Use mutex from xe_configfs subsystem

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

On 7/28/2025 9:50 PM, Rodrigo Vivi wrote:
> On Sun, Jul 27, 2025 at 07:20:01PM +0200, Michal Wajdeczko wrote:
>> It should be sufficient to protect all configuration parameters
>> using single mutex already defined inside xe_configfs subsystem.
>>
>> While around, also convert code to use guard/scoped_guard classes.
> 
> agree on the guard/scope change, but I believe we should keep
> the local lock for the local variables only instead of re-using
> a global one...

Lucas was fine with that, hence the patch, see [1]:

"I think it would be fine. This is very rarely changed and doesn't matter
much it crosses all devices."

[1] https://patchwork.freedesktop.org/patch/665141/?series=151773&rev=4#comment_1219134

> 
>>
>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>> ---
>>  drivers/gpu/drm/xe/xe_configfs.c | 29 +++++++++++------------------
>>  1 file changed, 11 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_configfs.c b/drivers/gpu/drm/xe/xe_configfs.c
>> index 6aa0531bcf76..e32517afae58 100644
>> --- a/drivers/gpu/drm/xe/xe_configfs.c
>> +++ b/drivers/gpu/drm/xe/xe_configfs.c
>> @@ -85,14 +85,15 @@
>>   *	rmdir /sys/kernel/config/xe/0000:03:00.0/
>>   */
>>  
>> +/*
>> + * Use xe_configfs.su_mutex (also accessible from group.cg_subsys->su_mutex)
>> + * to protect configuration data.
>> + */
>>  struct xe_config_device {
>>  	struct config_group group;
>>  
>>  	bool survivability_mode;
>>  	u64 engines_allowed;
>> -
>> -	/* protects attributes */
>> -	struct mutex lock;
>>  };
>>  
>>  struct engine_info {
>> @@ -135,9 +136,8 @@ static ssize_t survivability_mode_store(struct config_item *item, const char *pa
>>  	if (ret)
>>  		return ret;
>>  
>> -	mutex_lock(&dev->lock);
>> -	dev->survivability_mode = survivability_mode;
>> -	mutex_unlock(&dev->lock);
>> +	scoped_guard(mutex, &item->ci_group->cg_subsys->su_mutex)
>> +		dev->survivability_mode = survivability_mode;
>>  
>>  	return len;
>>  }
>> @@ -219,9 +219,8 @@ static ssize_t engines_allowed_store(struct config_item *item, const char *page,
>>  		val |= mask;
>>  	}
>>  
>> -	mutex_lock(&dev->lock);
>> -	dev->engines_allowed = val;
>> -	mutex_unlock(&dev->lock);
>> +	scoped_guard(mutex, &item->ci_group->cg_subsys->su_mutex)
>> +		dev->engines_allowed = val;
>>  
>>  	return len;
>>  }
>> @@ -239,7 +238,6 @@ static void xe_config_device_release(struct config_item *item)
>>  {
>>  	struct xe_config_device *dev = to_xe_config_device(item);
>>  
>> -	mutex_destroy(&dev->lock);
>>  	kfree(dev);
>>  }
>>  
>> @@ -286,8 +284,6 @@ static struct config_group *xe_config_make_device_group(struct config_group *gro
>>  
>>  	config_group_init_type_name(&dev->group, name, &xe_config_device_type);
>>  
>> -	mutex_init(&dev->lock);
>> -
>>  	return &dev->group;
>>  }
>>  
>> @@ -311,12 +307,10 @@ static struct configfs_subsystem xe_configfs = {
>>  
>>  static struct xe_config_device *configfs_find_group(struct pci_dev *pdev)
>>  {
>> +	guard(mutex)(&xe_configfs.su_mutex);
>>  	struct config_item *item;
>>  
>> -	mutex_lock(&xe_configfs.su_mutex);
>>  	item = config_group_find_item(&xe_configfs.su_group, pci_name(pdev));
>> -	mutex_unlock(&xe_configfs.su_mutex);
>> -
>>  	if (!item)
>>  		return NULL;
>>  
>> @@ -360,9 +354,8 @@ void xe_configfs_clear_survivability_mode(struct pci_dev *pdev)
>>  	if (!dev)
>>  		return;
>>  
>> -	mutex_lock(&dev->lock);
>> -	dev->survivability_mode = 0;
>> -	mutex_unlock(&dev->lock);
>> +	scoped_guard(mutex, &xe_configfs.su_mutex)
>> +		dev->survivability_mode = 0;
>>  
>>  	config_item_put(&dev->group.cg_item);
>>  }
>> -- 
>> 2.47.1
>>