Re: [PATCH] Revert "drm/amd/amdgpu: Fix potential ioremap() memory leaks in amdgpu_device_init()"

2024-03-19 Thread Ma, Jun 
Thanks, I will fix it when push to the next branch

Regards,
Ma Jun

On 3/19/2024 4:22 PM, Christian König wrote:
> Am 19.03.24 um 09:07 schrieb Ma Jun:
>> This patch causes the following iounmap erorr and calltrace
>> iounmap: bad address d0b3631f
> 
>> So just revert it and amdgpu_device_fini_sw() will cleanup the
>> rmmio mapping.
> 
> I think we can improve the wording here. Something like this:
> 
> The original patch was unjustified because amdgpu_device_fini_sw() will 
> always cleanup the
> rmmio mapping.
> 
>>
>> This reverts commit 923f7a82d2e12a99744a940954f3829ab18a9dc7.
> 
> You Signed-off-by tag is missing.
> 
> With that fixed the patch is Reviewed-by: Christian König 
> 
> 
> Regards,
> Christian.
> 
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 16 ++--
>>   1 file changed, 6 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> index 70261eb9b0bb..3204b8f6edeb 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> @@ -4042,10 +4042,8 @@ int amdgpu_device_init(struct amdgpu_device *adev,
>>   * early on during init and before calling to RREG32.
>>   */
>>  adev->reset_domain = amdgpu_reset_create_reset_domain(SINGLE_DEVICE, 
>> "amdgpu-reset-dev");
>> -if (!adev->reset_domain) {
>> -r = -ENOMEM;
>> -goto unmap_memory;
>> -}
>> +if (!adev->reset_domain)
>> +return -ENOMEM;
>>   
>>  /* detect hw virtualization here */
>>  amdgpu_detect_virtualization(adev);
>> @@ -4055,7 +4053,7 @@ int amdgpu_device_init(struct amdgpu_device *adev,
>>  r = amdgpu_device_get_job_timeout_settings(adev);
>>  if (r) {
>>  dev_err(adev->dev, "invalid lockup_timeout parameter syntax\n");
>> -goto unmap_memory;
>> +return r;
>>  }
>>   
>>  amdgpu_device_set_mcbp(adev);
>> @@ -4063,12 +4061,12 @@ int amdgpu_device_init(struct amdgpu_device *adev,
>>  /* early init functions */
>>  r = amdgpu_device_ip_early_init(adev);
>>  if (r)
>> -goto unmap_memory;
>> +return r;
>>   
>>  /* Get rid of things like offb */
>>  r = drm_aperture_remove_conflicting_pci_framebuffers(adev->pdev, 
>> _kms_driver);
>>  if (r)
>> -goto unmap_memory;
>> +return r;
>>   
>>  /* Enable TMZ based on IP_VERSION */
>>  amdgpu_gmc_tmz_set(adev);
>> @@ -4078,7 +4076,7 @@ int amdgpu_device_init(struct amdgpu_device *adev,
>>  if (adev->gmc.xgmi.supported) {
>>  r = adev->gfxhub.funcs->get_xgmi_info(adev);
>>  if (r)
>> -goto unmap_memory;
>> +return r;
>>  }
>>   
>>  /* enable PCIE atomic ops */
>> @@ -4347,8 +4345,6 @@ int amdgpu_device_init(struct amdgpu_device *adev,
>>   failed:
>>  amdgpu_vf_error_trans_all(adev);
>>   
>> -unmap_memory:
>> -iounmap(adev->rmmio);
>>  return r;
>>   }
>>   
>

Re: [PATCH] Revert "drm/amd/amdgpu: Fix potential ioremap() memory leaks in amdgpu_device_init()"

2024-03-19 Thread Christian König 

Am 19.03.24 um 09:07 schrieb Ma Jun:

This patch causes the following iounmap erorr and calltrace
iounmap: bad address d0b3631f



So just revert it and amdgpu_device_fini_sw() will cleanup the
rmmio mapping.


I think we can improve the wording here. Something like this:

The original patch was unjustified because amdgpu_device_fini_sw() will 
always cleanup the

rmmio mapping.



This reverts commit 923f7a82d2e12a99744a940954f3829ab18a9dc7.


You Signed-off-by tag is missing.

With that fixed the patch is Reviewed-by: Christian König 



Regards,
Christian.


---
  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 16 ++--
  1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 70261eb9b0bb..3204b8f6edeb 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -4042,10 +4042,8 @@ int amdgpu_device_init(struct amdgpu_device *adev,
 * early on during init and before calling to RREG32.
 */
adev->reset_domain = amdgpu_reset_create_reset_domain(SINGLE_DEVICE, 
"amdgpu-reset-dev");
-   if (!adev->reset_domain) {
-   r = -ENOMEM;
-   goto unmap_memory;
-   }
+   if (!adev->reset_domain)
+   return -ENOMEM;
  
  	/* detect hw virtualization here */

amdgpu_detect_virtualization(adev);
@@ -4055,7 +4053,7 @@ int amdgpu_device_init(struct amdgpu_device *adev,
r = amdgpu_device_get_job_timeout_settings(adev);
if (r) {
dev_err(adev->dev, "invalid lockup_timeout parameter syntax\n");
-   goto unmap_memory;
+   return r;
}
  
  	amdgpu_device_set_mcbp(adev);

@@ -4063,12 +4061,12 @@ int amdgpu_device_init(struct amdgpu_device *adev,
/* early init functions */
r = amdgpu_device_ip_early_init(adev);
if (r)
-   goto unmap_memory;
+   return r;
  
  	/* Get rid of things like offb */

r = drm_aperture_remove_conflicting_pci_framebuffers(adev->pdev, 
_kms_driver);
if (r)
-   goto unmap_memory;
+   return r;
  
  	/* Enable TMZ based on IP_VERSION */

amdgpu_gmc_tmz_set(adev);
@@ -4078,7 +4076,7 @@ int amdgpu_device_init(struct amdgpu_device *adev,
if (adev->gmc.xgmi.supported) {
r = adev->gfxhub.funcs->get_xgmi_info(adev);
if (r)
-   goto unmap_memory;
+   return r;
}
  
  	/* enable PCIE atomic ops */

@@ -4347,8 +4345,6 @@ int amdgpu_device_init(struct amdgpu_device *adev,
  failed:
amdgpu_vf_error_trans_all(adev);
  
-unmap_memory:

-   iounmap(adev->rmmio);
return r;
  }

[PATCH] Revert "drm/amd/amdgpu: Fix potential ioremap() memory leaks in amdgpu_device_init()"

2024-03-19 Thread Ma Jun 
This patch causes the following iounmap erorr and calltrace
iounmap: bad address d0b3631f

So just revert it and amdgpu_device_fini_sw() will cleanup the
rmmio mapping.

This reverts commit 923f7a82d2e12a99744a940954f3829ab18a9dc7.
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 16 ++--
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 70261eb9b0bb..3204b8f6edeb 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -4042,10 +4042,8 @@ int amdgpu_device_init(struct amdgpu_device *adev,
 * early on during init and before calling to RREG32.
 */
adev->reset_domain = amdgpu_reset_create_reset_domain(SINGLE_DEVICE, 
"amdgpu-reset-dev");
-   if (!adev->reset_domain) {
-   r = -ENOMEM;
-   goto unmap_memory;
-   }
+   if (!adev->reset_domain)
+   return -ENOMEM;
 
/* detect hw virtualization here */
amdgpu_detect_virtualization(adev);
@@ -4055,7 +4053,7 @@ int amdgpu_device_init(struct amdgpu_device *adev,
r = amdgpu_device_get_job_timeout_settings(adev);
if (r) {
dev_err(adev->dev, "invalid lockup_timeout parameter syntax\n");
-   goto unmap_memory;
+   return r;
}
 
amdgpu_device_set_mcbp(adev);
@@ -4063,12 +4061,12 @@ int amdgpu_device_init(struct amdgpu_device *adev,
/* early init functions */
r = amdgpu_device_ip_early_init(adev);
if (r)
-   goto unmap_memory;
+   return r;
 
/* Get rid of things like offb */
r = drm_aperture_remove_conflicting_pci_framebuffers(adev->pdev, 
_kms_driver);
if (r)
-   goto unmap_memory;
+   return r;
 
/* Enable TMZ based on IP_VERSION */
amdgpu_gmc_tmz_set(adev);
@@ -4078,7 +4076,7 @@ int amdgpu_device_init(struct amdgpu_device *adev,
if (adev->gmc.xgmi.supported) {
r = adev->gfxhub.funcs->get_xgmi_info(adev);
if (r)
-   goto unmap_memory;
+   return r;
}
 
/* enable PCIE atomic ops */
@@ -4347,8 +4345,6 @@ int amdgpu_device_init(struct amdgpu_device *adev,
 failed:
amdgpu_vf_error_trans_all(adev);
 
-unmap_memory:
-   iounmap(adev->rmmio);
return r;
 }
 
-- 
2.34.1