[PATCH 0/5] iommu/amd: Cleanup and fixes

[PATCH 0/5] iommu/amd: Cleanup and fixes

[Vasant Hegde via iommu]

This series contains various cleanup and trivial fixes.

Suravee Suthikulpanit (2):
  iommu/amd: Improve error handling for amd_iommu_init_pci
  iommu/amd: Improve amd_iommu_v2_exit()

Vasant Hegde (3):
  iommu/amd: Call memunmap in error path
  iommu/amd: Clean up function declarations
  iommu/amd: Remove unused struct fault.devid

 drivers/iommu/amd/amd_iommu.h |  4 ----
 drivers/iommu/amd/init.c      | 16 ++++++++++++----
 drivers/iommu/amd/iommu_v2.c  | 35 +++++++++++++++++------------------
 3 files changed, 29 insertions(+), 26 deletions(-)

-- 
2.27.0

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH 1/5] iommu/amd: Improve error handling for amd_iommu_init_pci

[Vasant Hegde via iommu]

From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

Add error messages to prevent silent failure.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
 drivers/iommu/amd/init.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index 1eacd43cb436..770ac679b682 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -1942,9 +1942,10 @@ static int __init amd_iommu_init_pci(void)
 
 	for_each_iommu(iommu) {
 		ret = iommu_init_pci(iommu);
-		if (ret)
-			break;
-
+		if (ret) {
+			pr_err("IOMMU:%d Failed to initialize!\n", iommu->index);
+			goto out;
+		}
 		/* Need to setup range after PCI init */
 		iommu_set_cwwb_range(iommu);
 	}
@@ -1960,6 +1961,10 @@ static int __init amd_iommu_init_pci(void)
 	 * active.
 	 */
 	ret = amd_iommu_init_api();
+	if (ret) {
+		pr_err("IOMMU: Failed to initialize api!\n");
+		goto out;
+	}
 
 	init_device_table_dma();
 
@@ -1969,6 +1974,7 @@ static int __init amd_iommu_init_pci(void)
 	if (!ret)
 		print_iommu_info();
 
+out:
 	return ret;
 }
 
-- 
2.27.0

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH 2/5] iommu/amd: Call memunmap in error path

[Vasant Hegde via iommu]

From: Vasant Hegde <Vasant.Hegde@amd.com>

Unmap old_devtb in error path.

Co-developed-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
 drivers/iommu/amd/init.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index 770ac679b682..92c489c1467d 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -979,6 +979,7 @@ static bool copy_device_table(void)
 				get_order(dev_table_size));
 	if (old_dev_tbl_cpy == NULL) {
 		pr_err("Failed to allocate memory for copying old device table!\n");
+		memunmap(old_devtb);
 		return false;
 	}
 
@@ -1009,6 +1010,7 @@ static bool copy_device_table(void)
 			if ((int_ctl != DTE_IRQ_REMAP_INTCTL) ||
 			    (int_tab_len != DTE_INTTABLEN)) {
 				pr_err("Wrong old irq remapping flag: %#x\n", devid);
+				memunmap(old_devtb);
 				return false;
 			}
 
-- 
2.27.0

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH 3/5] iommu/amd: Clean up function declarations

[Vasant Hegde via iommu]

Remove unused declarations and add static keyword as needed.

Co-developed-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
 drivers/iommu/amd/amd_iommu.h | 4 ----
 drivers/iommu/amd/init.c      | 2 +-
 2 files changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
index 416815a525d6..94d33626d692 100644
--- a/drivers/iommu/amd/amd_iommu.h
+++ b/drivers/iommu/amd/amd_iommu.h
@@ -14,10 +14,6 @@
 extern irqreturn_t amd_iommu_int_thread(int irq, void *data);
 extern irqreturn_t amd_iommu_int_handler(int irq, void *data);
 extern void amd_iommu_apply_erratum_63(u16 devid);
-extern void amd_iommu_reset_cmd_buffer(struct amd_iommu *iommu);
-extern int amd_iommu_init_devices(void);
-extern void amd_iommu_uninit_devices(void);
-extern void amd_iommu_init_notifier(void);
 extern int amd_iommu_init_api(void);
 
 #ifdef CONFIG_AMD_IOMMU_DEBUGFS
diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index 92c489c1467d..df11c50deb74 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -660,7 +660,7 @@ static int __init alloc_command_buffer(struct amd_iommu *iommu)
  * This function resets the command buffer if the IOMMU stopped fetching
  * commands from it.
  */
-void amd_iommu_reset_cmd_buffer(struct amd_iommu *iommu)
+static void amd_iommu_reset_cmd_buffer(struct amd_iommu *iommu)
 {
 	iommu_feature_disable(iommu, CONTROL_CMDBUF_EN);
 
-- 
2.27.0

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH 4/5] iommu/amd: Remove unused struct fault.devid

[Vasant Hegde via iommu]

This variable has not been used since it was introduced.

Co-developed-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
 drivers/iommu/amd/iommu_v2.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/iommu/amd/iommu_v2.c b/drivers/iommu/amd/iommu_v2.c
index 58da08cc3d01..2daf37c21b85 100644
--- a/drivers/iommu/amd/iommu_v2.c
+++ b/drivers/iommu/amd/iommu_v2.c
@@ -71,7 +71,6 @@ struct fault {
 	struct pasid_state *state;
 	struct mm_struct *mm;
 	u64 address;
-	u16 devid;
 	u32 pasid;
 	u16 tag;
 	u16 finish;
-- 
2.27.0

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH 5/5] iommu/amd: Improve amd_iommu_v2_exit()

[Vasant Hegde via iommu]

From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

During module exit, the current logic loops through all possible
16-bit device ID space to search for existing devices and clean up
device state structures. This can be simplified by looping through
the device state list.

Also, refactor various clean up logic into free_device_state()
for better reusability.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
 drivers/iommu/amd/iommu_v2.c | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/drivers/iommu/amd/iommu_v2.c b/drivers/iommu/amd/iommu_v2.c
index 2daf37c21b85..c72969ac4956 100644
--- a/drivers/iommu/amd/iommu_v2.c
+++ b/drivers/iommu/amd/iommu_v2.c
@@ -24,7 +24,6 @@
 MODULE_LICENSE("GPL v2");
 MODULE_AUTHOR("Joerg Roedel <jroedel@suse.de>");
 
-#define MAX_DEVICES		0x10000
 #define PRI_QUEUE_SIZE		512
 
 struct pri_queue {
@@ -124,6 +123,15 @@ static void free_device_state(struct device_state *dev_state)
 {
 	struct iommu_group *group;
 
+	/* Get rid of any remaining pasid states */
+	free_pasid_states(dev_state);
+
+	/*
+	 * Wait until the last reference is dropped before freeing
+	 * the device state.
+	 */
+	wait_event(dev_state->wq, !atomic_read(&dev_state->count));
+
 	/*
 	 * First detach device from domain - No more PRI requests will arrive
 	 * from that device after it is unbound from the IOMMUv2 domain.
@@ -849,15 +857,7 @@ void amd_iommu_free_device(struct pci_dev *pdev)
 
 	spin_unlock_irqrestore(&state_lock, flags);
 
-	/* Get rid of any remaining pasid states */
-	free_pasid_states(dev_state);
-
 	put_device_state(dev_state);
-	/*
-	 * Wait until the last reference is dropped before freeing
-	 * the device state.
-	 */
-	wait_event(dev_state->wq, !atomic_read(&dev_state->count));
 	free_device_state(dev_state);
 }
 EXPORT_SYMBOL(amd_iommu_free_device);
@@ -954,8 +954,8 @@ static int __init amd_iommu_v2_init(void)
 
 static void __exit amd_iommu_v2_exit(void)
 {
-	struct device_state *dev_state;
-	int i;
+	struct device_state *dev_state, *next;
+	unsigned long flags;
 
 	if (!amd_iommu_v2_supported())
 		return;
@@ -968,18 +968,18 @@ static void __exit amd_iommu_v2_exit(void)
 	 * The loop below might call flush_workqueue(), so call
 	 * destroy_workqueue() after it
 	 */
-	for (i = 0; i < MAX_DEVICES; ++i) {
-		dev_state = get_device_state(i);
-
-		if (dev_state == NULL)
-			continue;
+	spin_lock_irqsave(&state_lock, flags);
 
+	list_for_each_entry_safe(dev_state, next, &state_list, list) {
 		WARN_ON_ONCE(1);
 
 		put_device_state(dev_state);
-		amd_iommu_free_device(dev_state->pdev);
+		list_del(&dev_state->list);
+		free_device_state(dev_state);
 	}
 
+	spin_unlock_irqrestore(&state_lock, flags);
+
 	destroy_workqueue(iommu_wq);
 }
 
-- 
2.27.0

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 1/5] iommu/amd: Improve error handling for amd_iommu_init_pci

[Joerg Roedel]

On Mon, Feb 21, 2022 at 10:29:12AM +0530, Vasant Hegde wrote:
> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> 
> Add error messages to prevent silent failure.
> 
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> ---
>  drivers/iommu/amd/init.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
> index 1eacd43cb436..770ac679b682 100644
> --- a/drivers/iommu/amd/init.c
> +++ b/drivers/iommu/amd/init.c
> @@ -1942,9 +1942,10 @@ static int __init amd_iommu_init_pci(void)
>  
>  	for_each_iommu(iommu) {
>  		ret = iommu_init_pci(iommu);
> -		if (ret)
> -			break;
> -
> +		if (ret) {
> +			pr_err("IOMMU:%d Failed to initialize!\n", iommu->index);

Please make that message "IOMMU%d: Failed to initialize IOMMU Hardware (error=%d)!\n".

> +			goto out;
> +		}
>  		/* Need to setup range after PCI init */
>  		iommu_set_cwwb_range(iommu);
>  	}
> @@ -1960,6 +1961,10 @@ static int __init amd_iommu_init_pci(void)
>  	 * active.
>  	 */
>  	ret = amd_iommu_init_api();
> +	if (ret) {
> +		pr_err("IOMMU: Failed to initialize api!\n");

And that "IOMMU: Failed to initialize IOMMU-API interface (error=%d)!\n"

> +		goto out;
> +	}
>  
>  	init_device_table_dma();
>  
> @@ -1969,6 +1974,7 @@ static int __init amd_iommu_init_pci(void)
>  	if (!ret)
>  		print_iommu_info();
>  
> +out:
>  	return ret;
>  }
>  
> -- 
> 2.27.0
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 0/5] iommu/amd: Cleanup and fixes

[Joerg Roedel]

On Mon, Feb 21, 2022 at 10:29:11AM +0530, Vasant Hegde wrote:
> This series contains various cleanup and trivial fixes.
> 
> Suravee Suthikulpanit (2):
>   iommu/amd: Improve error handling for amd_iommu_init_pci
>   iommu/amd: Improve amd_iommu_v2_exit()
> 
> Vasant Hegde (3):
>   iommu/amd: Call memunmap in error path
>   iommu/amd: Clean up function declarations
>   iommu/amd: Remove unused struct fault.devid
> 
>  drivers/iommu/amd/amd_iommu.h |  4 ----
>  drivers/iommu/amd/init.c      | 16 ++++++++++++----
>  drivers/iommu/amd/iommu_v2.c  | 35 +++++++++++++++++------------------
>  3 files changed, 29 insertions(+), 26 deletions(-)

Besides the error messages in the first patch this looks good to me.
Please re-send with the updates and I consider it for v5.18.

Thanks,

	Joerg
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 1/5] iommu/amd: Improve error handling for amd_iommu_init_pci

[Vasant Hegde via iommu]

On 2/28/2022 6:17 PM, Joerg Roedel wrote:
> On Mon, Feb 21, 2022 at 10:29:12AM +0530, Vasant Hegde wrote:
>> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>>
>> Add error messages to prevent silent failure.
>>
>> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
>> ---
>>  drivers/iommu/amd/init.c | 12 +++++++++---
>>  1 file changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
>> index 1eacd43cb436..770ac679b682 100644
>> --- a/drivers/iommu/amd/init.c
>> +++ b/drivers/iommu/amd/init.c
>> @@ -1942,9 +1942,10 @@ static int __init amd_iommu_init_pci(void)
>>  
>>  	for_each_iommu(iommu) {
>>  		ret = iommu_init_pci(iommu);
>> -		if (ret)
>> -			break;
>> -
>> +		if (ret) {
>> +			pr_err("IOMMU:%d Failed to initialize!\n", iommu->index);
> 
> Please make that message "IOMMU%d: Failed to initialize IOMMU Hardware (error=%d)!\n".

Thanks Joerg for the review. I have fixed it in v2.



> 
>> +			goto out;
>> +		}
>>  		/* Need to setup range after PCI init */
>>  		iommu_set_cwwb_range(iommu);
>>  	}
>> @@ -1960,6 +1961,10 @@ static int __init amd_iommu_init_pci(void)
>>  	 * active.
>>  	 */
>>  	ret = amd_iommu_init_api();
>> +	if (ret) {
>> +		pr_err("IOMMU: Failed to initialize api!\n");
> 
> And that "IOMMU: Failed to initialize IOMMU-API interface (error=%d)!\n"

Fixed in v2.

-Vasant

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu