Re: [PATCH] PCI: ibmphp: Fix double unmap of io_mem

From: Bjorn Helgaas <helgaas@kernel.org>
To: Vishal Aslot <os.vaslot@gmail.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>,
	linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org,
	Lukas Wunner <lukas@wunner.de>
Subject: Re: [PATCH] PCI: ibmphp: Fix double unmap of io_mem
Date: Thu, 2 Sep 2021 12:05:52 -0500	[thread overview]
Message-ID: <20210902170552.GA345124@bjorn-Precision-5520> (raw)
In-Reply-To: <20210818165751.591185-1-os.vaslot@gmail.com>

[+cc Lukas, author of the TODO you refer to]

On Wed, Aug 18, 2021 at 11:57:51AM -0500, Vishal Aslot wrote:
> ebda_rsrc_controller() calls iounmap(io_mem) on the error path. It's
> caller, ibmphp_access_ebda() also calls iounmap(io_mem) on good and
> error paths. Removing the iounmap(io_mem) invocation inside
> ebda_rsrc_controller().
> 
> Signed-off-by: Vishal Aslot <os.vaslot@gmail.com>

Applied to pci/hotplug for v5.15, thanks!

I also removed the item from the TODO list, so the patch looks like
this:

commit faa2e05ad0dc ("PCI: ibmphp: Fix double unmap of io_mem")
Author: Vishal Aslot <os.vaslot@gmail.com>
Date:   Wed Aug 18 11:57:51 2021 -0500

    PCI: ibmphp: Fix double unmap of io_mem
    
    ebda_rsrc_controller() calls iounmap(io_mem) on the error path. Its caller,
    ibmphp_access_ebda(), also calls iounmap(io_mem) on good and error paths.
    
    Remove the iounmap(io_mem) invocation from ebda_rsrc_controller().
    
    [bhelgaas: remove item from TODO]
    Link: https://lore.kernel.org/r/20210818165751.591185-1-os.vaslot@gmail.com
    Signed-off-by: Vishal Aslot <os.vaslot@gmail.com>
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

diff --git a/drivers/pci/hotplug/TODO b/drivers/pci/hotplug/TODO
index a32070be5adf..cc6194aa24c1 100644
--- a/drivers/pci/hotplug/TODO
+++ b/drivers/pci/hotplug/TODO
@@ -40,9 +40,6 @@ ibmphp:
 
 * The return value of pci_hp_register() is not checked.
 
-* iounmap(io_mem) is called in the error path of ebda_rsrc_controller()
-  and once more in the error path of its caller ibmphp_access_ebda().
-
 * The various slot data structures are difficult to follow and need to be
   simplified.  A lot of functions are too large and too complex, they need
   to be broken up into smaller, manageable pieces.  Negative examples are
diff --git a/drivers/pci/hotplug/ibmphp_ebda.c b/drivers/pci/hotplug/ibmphp_ebda.c
index 11a2661dc062..7fb75401ad8a 100644
--- a/drivers/pci/hotplug/ibmphp_ebda.c
+++ b/drivers/pci/hotplug/ibmphp_ebda.c
@@ -714,8 +714,7 @@ static int __init ebda_rsrc_controller(void)
 		/* init hpc structure */
 		hpc_ptr = alloc_ebda_hpc(slot_num, bus_num);
 		if (!hpc_ptr) {
-			rc = -ENOMEM;
-			goto error_no_hpc;
+			return -ENOMEM;
 		}
 		hpc_ptr->ctlr_id = ctlr_id;
 		hpc_ptr->ctlr_relative_id = ctlr;
@@ -910,8 +909,6 @@ static int __init ebda_rsrc_controller(void)
 	kfree(tmp_slot);
 error_no_slot:
 	free_ebda_hpc(hpc_ptr);
-error_no_hpc:
-	iounmap(io_mem);
 	return rc;
 }
 
> ---
> 
> Why am I fixing this?
> I found this clean up item in drivers/pci/hotplug/TODO [lines 43-44]
> and decided to fix it. This is my 2nd patch ever in linux so my
> apologies for any style issues. I am very teachable. :)
> 
>  drivers/pci/hotplug/ibmphp_ebda.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/hotplug/ibmphp_ebda.c b/drivers/pci/hotplug/ibmphp_ebda.c
> index 11a2661dc062..7fb75401ad8a 100644
> --- a/drivers/pci/hotplug/ibmphp_ebda.c
> +++ b/drivers/pci/hotplug/ibmphp_ebda.c
> @@ -714,8 +714,7 @@ static int __init ebda_rsrc_controller(void)
>  		/* init hpc structure */
>  		hpc_ptr = alloc_ebda_hpc(slot_num, bus_num);
>  		if (!hpc_ptr) {
> -			rc = -ENOMEM;
> -			goto error_no_hpc;
> +			return -ENOMEM;
>  		}
>  		hpc_ptr->ctlr_id = ctlr_id;
>  		hpc_ptr->ctlr_relative_id = ctlr;
> @@ -910,8 +909,6 @@ static int __init ebda_rsrc_controller(void)
>  	kfree(tmp_slot);
>  error_no_slot:
>  	free_ebda_hpc(hpc_ptr);
> -error_no_hpc:
> -	iounmap(io_mem);
>  	return rc;
>  }
>  
> -- 
> 2.27.0
> 
