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

[SRINIVASAN SHANMUGAM <srinivasan.shanmugam@xxxxxxx>]

Hi Christian,

On 2/26/2024 1:46 PM, Christian König wrote:
> Am 24.02.24 um 07:38 schrieb Srinivasan Shanmugam:
> > This ensures that the memory mapped by ioremap for adev->rmmio, is
> > properly handled in amdgpu_device_init(). If the function exits early
> > due to an error, the memory is unmapped. If the function completes
> > successfully, the memory remains mapped.
> >
> > Fixes the below:
> > drivers/gpu/drm/amd/amdgpu/amdgpu_device.c:4337 amdgpu_device_init() 
> > warn: 'adev->rmmio' from ioremap() not released on lines: 
> > 4035,4045,4051,4058,4068,4337
>
> Hui? How do you got that warning?
It was caught by smatch & will update the same in the commit message.
> > Cc: Christian König <christian.koenig@xxxxxxx>
> > Cc: Alex Deucher <alexander.deucher@xxxxxxx>
> > Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam@xxxxxxx>
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 18 ++++++++++++++----
> >   1 file changed, 14 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c 
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > index 1ef892bea488..68bf5e910cb8 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > @@ -4031,8 +4031,10 @@ 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)
> > +    if (!adev->reset_domain) {
> > +        iounmap(adev->rmmio);
> >           return -ENOMEM;
> > +    }
>
> Please use a goto label and error handling instead. Apart from that 
> looks good to me.

Thanks a lot for all your reviews, highly appreciate it, will send v2 
for this.

Best Wishes,

Srini

> Regards,
> Christian.
>
> >         /* detect hw virtualization here */
> >       amdgpu_detect_virtualization(adev);
> > @@ -4042,20 +4044,25 @@ 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");
> > +        iounmap(adev->rmmio);
> >           return r;
> >       }
> >         /* early init functions */
> >       r = amdgpu_device_ip_early_init(adev);
> > -    if (r)
> > +    if (r) {
> > +        iounmap(adev->rmmio);
> >           return r;
> > +    }
> >         amdgpu_device_set_mcbp(adev);
> >         /* Get rid of things like offb */
> >       r = 
> > drm_aperture_remove_conflicting_pci_framebuffers(adev->pdev, 
> > &amdgpu_kms_driver);
> > -    if (r)
> > +    if (r) {
> > +        iounmap(adev->rmmio);
> >           return r;
> > +    }
> >         /* Enable TMZ based on IP_VERSION */
> >       amdgpu_gmc_tmz_set(adev);
> > @@ -4064,8 +4071,10 @@ int amdgpu_device_init(struct amdgpu_device 
> > *adev,
> >       /* Need to get xgmi info early to decide the reset behavior*/
> >       if (adev->gmc.xgmi.supported) {
> >           r = adev->gfxhub.funcs->get_xgmi_info(adev);
> > -        if (r)
> > +        if (r) {
> > +            iounmap(adev->rmmio);
> >               return r;
> > +        }
> >       }
> >         /* enable PCIE atomic ops */
> > @@ -4334,6 +4343,7 @@ int amdgpu_device_init(struct amdgpu_device *adev,
> >   failed:
> >       amdgpu_vf_error_trans_all(adev);
> >   +    iounmap(adev->rmmio);
> >       return r;
> >   }