Re: [PATCH] m68k/PCI: Fix a memory leak in an error handling path

Re: [PATCH] m68k/PCI: Fix a memory leak in an error handling path

[Markus Elfring]

> If 'ioremap' fails, we must free 'bridge', as done in other error handling
> path bellow.

I suggest to improve this change description.

* Please avoid a typo.

* Is an imperative wording preferred?


…
> +++ b/arch/m68k/coldfire/pci.c
> @@ -216,8 +216,10 @@ static int __init mcf_pci_init(void)
…

I propose to move the pci_free_host_bridge() call for the desired
exception handling to the end of this function implementation.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?id=c0d73a868d9b411bd2d0c8e5ff9d98bfa8563cb1#n450

Regards,
Markus

Re: [PATCH] m68k/PCI: Fix a memory leak in an error handling path

[Christophe JAILLET]

Le 18/04/2020 à 22:00, Markus Elfring a écrit :
>> If 'ioremap' fails, we must free 'bridge', as done in other error handling
>> path bellow.
> I suggest to improve this change description.

I suggest you stop proposing over and over useless comments.
Please just ignore my proposals as I do for your boring, never 
constructing, replies.


> * Please avoid a typo.
>
> * Is an imperative wording preferred?
>
* is Melissa still around?

> …
>> +++ b/arch/m68k/coldfire/pci.c
>> @@ -216,8 +216,10 @@ static int __init mcf_pci_init(void)
> …
>
> I propose to move the pci_free_host_bridge() call for the desired

I propose to let patch submitter and maintainer decide about it.
I don't need your point of view. I guess that maintainers don't either.

No need to waste time trying to engage any discussion with me.
This is the first and very last exchange we will ever have.

Best regards,
CJ


> exception handling to the end of this function implementation.
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?id=c0d73a868d9b411bd2d0c8e5ff9d98bfa8563cb1#n450
>
> Regards,
> Markus
>

[PATCH] m68k/PCI: Fix a memory leak in an error handling path

[Christophe JAILLET]

If 'ioremap' fails, we must free 'bridge', as done in other error handling
path bellow.

Fixes: 19cc4c843f40 ("m68k/PCI: Replace pci_fixup_irqs() call with host bridge IRQ mapping hooks")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
 arch/m68k/coldfire/pci.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/m68k/coldfire/pci.c b/arch/m68k/coldfire/pci.c
index 62b0eb6cf69a..84eab0f5e00a 100644
--- a/arch/m68k/coldfire/pci.c
+++ b/arch/m68k/coldfire/pci.c
@@ -216,8 +216,10 @@ static int __init mcf_pci_init(void)
 
 	/* Keep a virtual mapping to IO/config space active */
 	iospace = (unsigned long) ioremap(PCI_IO_PA, PCI_IO_SIZE);
-	if (iospace == 0)
+	if (iospace == 0) {
+		pci_free_host_bridge(bridge);
 		return -ENODEV;
+	}
 	pr_info("Coldfire: PCI IO/config window mapped to 0x%x\n",
 		(u32) iospace);
 
-- 
2.20.1

Re: [PATCH] m68k/PCI: Fix a memory leak in an error handling path

[Geert Uytterhoeven]

On Sat, Apr 18, 2020 at 9:07 AM Christophe JAILLET
<christophe.jaillet@wanadoo.fr> wrote:
> If 'ioremap' fails, we must free 'bridge', as done in other error handling
> path bellow.
>
> Fixes: 19cc4c843f40 ("m68k/PCI: Replace pci_fixup_irqs() call with host bridge IRQ mapping hooks")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>

Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] m68k/PCI: Fix a memory leak in an error handling path

[Greg Ungerer]

Hi Christophe,

On 18/4/20 5:07 pm, Christophe JAILLET wrote:
> If 'ioremap' fails, we must free 'bridge', as done in other error handling
> path bellow.
> 
> Fixes: 19cc4c843f40 ("m68k/PCI: Replace pci_fixup_irqs() call with host bridge IRQ mapping hooks")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>

Looks good, thanks.
I will queue this in the for-next branch of the m68knommu tree.

Regards
Greg


> ---
>   arch/m68k/coldfire/pci.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/m68k/coldfire/pci.c b/arch/m68k/coldfire/pci.c
> index 62b0eb6cf69a..84eab0f5e00a 100644
> --- a/arch/m68k/coldfire/pci.c
> +++ b/arch/m68k/coldfire/pci.c
> @@ -216,8 +216,10 @@ static int __init mcf_pci_init(void)
>   
>   	/* Keep a virtual mapping to IO/config space active */
>   	iospace = (unsigned long) ioremap(PCI_IO_PA, PCI_IO_SIZE);
> -	if (iospace == 0)
> +	if (iospace == 0) {
> +		pci_free_host_bridge(bridge);
>   		return -ENODEV;
> +	}
>   	pr_info("Coldfire: PCI IO/config window mapped to 0x%x\n",
>   		(u32) iospace);
>   
>