2023-03-29 05:32:06

by Gencen Gan

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

From: Gan Gecen <[email protected]>

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 <[email protected]>
---
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

2023-03-31 15:19:15

by Sui Jingfeng

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

Hi,

I'm noticed you patch, interesting!

On 2023/3/29 13:26, Gencen Gan wrote:
> From: Gan Gecen <[email protected]>
>
> 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 <[email protected]>
> ---
> 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;