[PING PATCH] drm/bochs: replace ioremap with devm_ioremap to avoid immo leak

[PING PATCH] drm/bochs: replace ioremap with devm_ioremap to avoid immo leak

[Gencen Gan]

From: Gan Gecen <gangecen@hust.edu.cn>

Smatch reports:

	drivers/gpu/drm/tiny/bochs.c:290 bochs_hw_init()
	warn: 'bochs->mmio' from ioremap() not released on
	lines: 246,250,254.

In the function bochs_load() that calls bochs_hw_init()
only, if bochs_hw_init(dev) returns -ENODEV(-19), it
will jumps to err_free_dev instead of err_hw_fini, so
bochs->immo won't be freed.

We would prefer to replace ioremap with devm_ioremap
to avoid adding lengthy error handling. The function 
`devm_ioremap` will automatically release the allocated
resources after use.

Signed-off-by: Gan Gecen <gangecen@hust.edu.cn>
---
 drivers/gpu/drm/tiny/bochs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/tiny/bochs.c b/drivers/gpu/drm/tiny/bochs.c
index 024346054c70..0d7e119a732f 100644
--- a/drivers/gpu/drm/tiny/bochs.c
+++ b/drivers/gpu/drm/tiny/bochs.c
@@ -223,7 +223,7 @@ static int bochs_hw_init(struct drm_device *dev)
 		}
 		ioaddr = pci_resource_start(pdev, 2);
 		iosize = pci_resource_len(pdev, 2);
-		bochs->mmio = ioremap(ioaddr, iosize);
+		bochs->mmio = devm_ioremap(&pdev->dev, ioaddr, iosize);
 		if (bochs->mmio == NULL) {
 			DRM_ERROR("Cannot map mmio region\n");
 			return -ENOMEM;
-- 
2.34.1

Re: [PING] drm/bochs: replace ioremap with devm_ioremap to avoid immo leak

[Sui Jingfeng]

Hi,

I'm noticed you patch, interesting!

On 2023/3/29 13:26, Gencen Gan wrote:
> From: Gan Gecen <gangecen@hust.edu.cn>
>
> Smatch reports:
>
> 	drivers/gpu/drm/tiny/bochs.c:290 bochs_hw_init()
> 	warn: 'bochs->mmio' from ioremap() not released on
> 	lines: 246,250,254.
>
> In the function bochs_load() that calls bochs_hw_init()
> only, if bochs_hw_init(dev) returns -ENODEV(-19), it
> will jumps to err_free_dev instead of err_hw_fini, so
> bochs->immo won't be freed.

    `mmio`, not `immo`,  you should also fix the typos in you patch's 
commit title.

> We would prefer to replace ioremap with devm_ioremap
> to avoid adding lengthy error handling. The function
> `devm_ioremap` will automatically release the allocated
> resources after use.

Yet, I notice that there is iounmap in bochs_hw_fini() function, does 
double free will happen?

static void bochs_hw_fini(struct drm_device *dev)
{
     struct bochs_device *bochs = dev->dev_private;
     // ...
     if (bochs->mmio)
         iounmap(bochs->mmio);
     // ...
}


I still advise you free it by adding error handling code, free it manually.

Because still there other ioremap() function in the driver.

But you can choose to heard other reviewer's voice, I can only help to 
review.

> Signed-off-by: Gan Gecen <gangecen@hust.edu.cn>
> ---
>   drivers/gpu/drm/tiny/bochs.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/tiny/bochs.c b/drivers/gpu/drm/tiny/bochs.c
> index 024346054c70..0d7e119a732f 100644
> --- a/drivers/gpu/drm/tiny/bochs.c
> +++ b/drivers/gpu/drm/tiny/bochs.c
> @@ -223,7 +223,7 @@ static int bochs_hw_init(struct drm_device *dev)
>   		}
>   		ioaddr = pci_resource_start(pdev, 2);
>   		iosize = pci_resource_len(pdev, 2);
> -		bochs->mmio = ioremap(ioaddr, iosize);
> +		bochs->mmio = devm_ioremap(&pdev->dev, ioaddr, iosize);
>   		if (bochs->mmio == NULL) {
>   			DRM_ERROR("Cannot map mmio region\n");
>   			return -ENOMEM;