[v2] Introduce debugfs support in IOMMU

[PATCH v2 1/8] iommu/amd: Refactor AMD IOMMU debugfs initial setup

Posted by Dheeraj Kumar Srivastava 1 year, 3 months ago

Rearrange initial setup of AMD IOMMU debugfs to segregate per IOMMU
setup and setup which is common for all IOMMUs. This ensures that common
debugfs paths (introduced in subsequent patches) are created only once
instead of being created for each IOMMU.

With the change, there is no need to use lock as amd_iommu_debugfs_setup()
will be called only once during AMD IOMMU initialization. So remove lock
acquisition in amd_iommu_debugfs_setup().

Signed-off-by: Dheeraj Kumar Srivastava <dheerajkumar.srivastava@amd.com>
Reviewed-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 drivers/iommu/amd/amd_iommu.h |  4 ++--
 drivers/iommu/amd/debugfs.c   | 16 +++++++---------
 drivers/iommu/amd/init.c      |  5 ++---
 3 files changed, 11 insertions(+), 14 deletions(-)

diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
index 6386fa4556d9..68821b62730c 100644
--- a/drivers/iommu/amd/amd_iommu.h
+++ b/drivers/iommu/amd/amd_iommu.h
@@ -29,9 +29,9 @@ void *__init iommu_alloc_4k_pages(struct amd_iommu *iommu,
 				  gfp_t gfp, size_t size);
 
 #ifdef CONFIG_AMD_IOMMU_DEBUGFS
-void amd_iommu_debugfs_setup(struct amd_iommu *iommu);
+void amd_iommu_debugfs_setup(void);
 #else
-static inline void amd_iommu_debugfs_setup(struct amd_iommu *iommu) {}
+static inline void amd_iommu_debugfs_setup(void) {}
 #endif
 
 /* Needed for interrupt remapping */
diff --git a/drivers/iommu/amd/debugfs.c b/drivers/iommu/amd/debugfs.c
index 545372fcc72f..ff9520e002be 100644
--- a/drivers/iommu/amd/debugfs.c
+++ b/drivers/iommu/amd/debugfs.c
@@ -13,20 +13,18 @@
 #include "amd_iommu.h"
 
 static struct dentry *amd_iommu_debugfs;
-static DEFINE_MUTEX(amd_iommu_debugfs_lock);
 
 #define	MAX_NAME_LEN	20
 
-void amd_iommu_debugfs_setup(struct amd_iommu *iommu)
+void amd_iommu_debugfs_setup(void)
 {
+	struct amd_iommu *iommu;
 	char name[MAX_NAME_LEN + 1];
 
-	mutex_lock(&amd_iommu_debugfs_lock);
-	if (!amd_iommu_debugfs)
-		amd_iommu_debugfs = debugfs_create_dir("amd",
-						       iommu_debugfs_dir);
-	mutex_unlock(&amd_iommu_debugfs_lock);
+	amd_iommu_debugfs = debugfs_create_dir("amd", iommu_debugfs_dir);
 
-	snprintf(name, MAX_NAME_LEN, "iommu%02d", iommu->index);
-	iommu->debugfs = debugfs_create_dir(name, amd_iommu_debugfs);
+	for_each_iommu(iommu) {
+		snprintf(name, MAX_NAME_LEN, "iommu%02d", iommu->index);
+		iommu->debugfs = debugfs_create_dir(name, amd_iommu_debugfs);
+	}
 }
diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index 43131c3a2172..d78dc96bbec3 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -3377,7 +3377,6 @@ int amd_iommu_enable_faulting(unsigned int cpu)
  */
 static int __init amd_iommu_init(void)
 {
-	struct amd_iommu *iommu;
 	int ret;
 
 	ret = iommu_go_to_state(IOMMU_INITIALIZED);
@@ -3391,8 +3390,8 @@ static int __init amd_iommu_init(void)
 	}
 #endif
 
-	for_each_iommu(iommu)
-		amd_iommu_debugfs_setup(iommu);
+	if (!ret)
+		amd_iommu_debugfs_setup();
 
 	return ret;
 }
-- 
2.25.1

Re: [PATCH v2 1/8] iommu/amd: Refactor AMD IOMMU debugfs initial setup

Posted by Bjorn Helgaas 1 year, 2 months ago

On Wed, Nov 06, 2024 at 01:16:32PM +0530, Dheeraj Kumar Srivastava wrote:
> Rearrange initial setup of AMD IOMMU debugfs to segregate per IOMMU
> setup and setup which is common for all IOMMUs. This ensures that common
> debugfs paths (introduced in subsequent patches) are created only once
> instead of being created for each IOMMU.
> ...

> -void amd_iommu_debugfs_setup(struct amd_iommu *iommu)
> +void amd_iommu_debugfs_setup(void)
>  {
> +	struct amd_iommu *iommu;
>  	char name[MAX_NAME_LEN + 1];
>  
> -	mutex_lock(&amd_iommu_debugfs_lock);
> -	if (!amd_iommu_debugfs)
> -		amd_iommu_debugfs = debugfs_create_dir("amd",
> -						       iommu_debugfs_dir);
> -	mutex_unlock(&amd_iommu_debugfs_lock);
> +	amd_iommu_debugfs = debugfs_create_dir("amd", iommu_debugfs_dir);
>  
> -	snprintf(name, MAX_NAME_LEN, "iommu%02d", iommu->index);
> -	iommu->debugfs = debugfs_create_dir(name, amd_iommu_debugfs);
> +	for_each_iommu(iommu) {
> +		snprintf(name, MAX_NAME_LEN, "iommu%02d", iommu->index);
> +		iommu->debugfs = debugfs_create_dir(name, amd_iommu_debugfs);
> +	}

Doing this setup with for_each_iommu() precludes any hot-add of
IOMMUs, but I guess there's no indication of hotplug support anyway
given all the uses of for_each_iommu() in init_iommu_all(),
amd_iommu_init_pci(), amd_iommu_enable_interrupts(), etc.

>  }
> diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
> index 43131c3a2172..d78dc96bbec3 100644
> --- a/drivers/iommu/amd/init.c
> +++ b/drivers/iommu/amd/init.c
> @@ -3377,7 +3377,6 @@ int amd_iommu_enable_faulting(unsigned int cpu)
>   */
>  static int __init amd_iommu_init(void)
>  {
> -	struct amd_iommu *iommu;
>  	int ret;
>  
>  	ret = iommu_go_to_state(IOMMU_INITIALIZED);
> @@ -3391,8 +3390,8 @@ static int __init amd_iommu_init(void)
>  	}
>  #endif
>  
> -	for_each_iommu(iommu)
> -		amd_iommu_debugfs_setup(iommu);
> +	if (!ret)
> +		amd_iommu_debugfs_setup();
>  
>  	return ret;
>  }
> -- 
> 2.25.1
>

Re: [PATCH v2 1/8] iommu/amd: Refactor AMD IOMMU debugfs initial setup

Posted by Srivastava, Dheeraj Kumar 1 year, 1 month ago

Hi,

On 11/27/2024 12:42 AM, Bjorn Helgaas wrote:
> On Wed, Nov 06, 2024 at 01:16:32PM +0530, Dheeraj Kumar Srivastava wrote:
>> Rearrange initial setup of AMD IOMMU debugfs to segregate per IOMMU
>> setup and setup which is common for all IOMMUs. This ensures that common
>> debugfs paths (introduced in subsequent patches) are created only once
>> instead of being created for each IOMMU.
>> ...
> 
>> -void amd_iommu_debugfs_setup(struct amd_iommu *iommu)
>> +void amd_iommu_debugfs_setup(void)
>>   {
>> +	struct amd_iommu *iommu;
>>   	char name[MAX_NAME_LEN + 1];
>>   
>> -	mutex_lock(&amd_iommu_debugfs_lock);
>> -	if (!amd_iommu_debugfs)
>> -		amd_iommu_debugfs = debugfs_create_dir("amd",
>> -						       iommu_debugfs_dir);
>> -	mutex_unlock(&amd_iommu_debugfs_lock);
>> +	amd_iommu_debugfs = debugfs_create_dir("amd", iommu_debugfs_dir);
>>   
>> -	snprintf(name, MAX_NAME_LEN, "iommu%02d", iommu->index);
>> -	iommu->debugfs = debugfs_create_dir(name, amd_iommu_debugfs);
>> +	for_each_iommu(iommu) {
>> +		snprintf(name, MAX_NAME_LEN, "iommu%02d", iommu->index);
>> +		iommu->debugfs = debugfs_create_dir(name, amd_iommu_debugfs);
>> +	}
> 
> Doing this setup with for_each_iommu() precludes any hot-add of
> IOMMUs, but I guess there's no indication of hotplug support anyway
> given all the uses of for_each_iommu() in init_iommu_all(),
> amd_iommu_init_pci(), amd_iommu_enable_interrupts(), etc.
> 

Yeah and here i don't see any concerns with the approach for now. Please 
do let me know if you have a better way to do this.

Thanks
Dheeraj

>>   }
>> diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
>> index 43131c3a2172..d78dc96bbec3 100644
>> --- a/drivers/iommu/amd/init.c
>> +++ b/drivers/iommu/amd/init.c
>> @@ -3377,7 +3377,6 @@ int amd_iommu_enable_faulting(unsigned int cpu)
>>    */
>>   static int __init amd_iommu_init(void)
>>   {
>> -	struct amd_iommu *iommu;
>>   	int ret;
>>   
>>   	ret = iommu_go_to_state(IOMMU_INITIALIZED);
>> @@ -3391,8 +3390,8 @@ static int __init amd_iommu_init(void)
>>   	}
>>   #endif
>>   
>> -	for_each_iommu(iommu)
>> -		amd_iommu_debugfs_setup(iommu);
>> +	if (!ret)
>> +		amd_iommu_debugfs_setup();
>>   
>>   	return ret;
>>   }
>> -- 
>> 2.25.1
>>