Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <1ea13a2a-90fd-07d3-2031-19e81ea349b4@csgroup.eu>
Date:   Tue, 1 Feb 2022 12:31:55 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.4.0
Subject: Re: [PATCH] powerpc/xive: Add some error handling code to
 'xive_spapr_init()'
Content-Language: fr-FR
To:     Christophe JAILLET <christophe.jaillet@wanadoo.fr>,
        benh@kernel.crashing.org, paulus@samba.org, mpe@ellerman.id.au,
        allison@lohutok.net, tglx@linutronix.de, clg@kaod.org,
        groug@kaod.org
Cc:     kernel-janitors@vger.kernel.org, linuxppc-dev@lists.ozlabs.org,
        linux-kernel@vger.kernel.org
References: <20190801110956.8517-1-christophe.jaillet@wanadoo.fr>
From:   Christophe Leroy <christophe.leroy@csgroup.eu>
In-Reply-To: <20190801110956.8517-1-christophe.jaillet@wanadoo.fr>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi,

Le 01/08/2019 à 13:09, Christophe JAILLET a écrit :
> 'xive_irq_bitmap_add()' can return -ENOMEM.
> In this case, we should free the memory already allocated and return
> 'false' to the caller.
> 
> Also add an error path which undoes the 'tima = ioremap(...)'

This old patch doesn't apply, if it is still relevant can you please 
rebase ?

Thanks
Christophe

> 
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> NOT compile tested (I don't have a cross compiler and won't install one).
> So if some correction or improvement are needed, feel free to propose and
> commit it directly.
> ---
>   arch/powerpc/sysdev/xive/spapr.c | 36 +++++++++++++++++++++++++-------
>   1 file changed, 28 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/powerpc/sysdev/xive/spapr.c b/arch/powerpc/sysdev/xive/spapr.c
> index 52198131c75e..b3ae0b76c433 100644
> --- a/arch/powerpc/sysdev/xive/spapr.c
> +++ b/arch/powerpc/sysdev/xive/spapr.c
> @@ -64,6 +64,17 @@ static int xive_irq_bitmap_add(int base, int count)
>   	return 0;
>   }
>   
> +static void xive_irq_bitmap_remove_all(void)
> +{
> +	struct xive_irq_bitmap *xibm, *tmp;
> +
> +	list_for_each_entry_safe(xibm, tmp, &xive_irq_bitmaps, list) {
> +		list_del(&xibm->list);
> +		kfree(xibm->bitmap);
> +		kfree(xibm);
> +	}
> +}
> +
>   static int __xive_irq_bitmap_alloc(struct xive_irq_bitmap *xibm)
>   {
>   	int irq;
> @@ -723,7 +734,7 @@ bool __init xive_spapr_init(void)
>   	u32 val;
>   	u32 len;
>   	const __be32 *reg;
> -	int i;
> +	int i, err;
>   
>   	if (xive_spapr_disabled())
>   		return false;
> @@ -748,23 +759,26 @@ bool __init xive_spapr_init(void)
>   	}
>   
>   	if (!xive_get_max_prio(&max_prio))
> -		return false;
> +		goto err_unmap;
>   
>   	/* Feed the IRQ number allocator with the ranges given in the DT */
>   	reg = of_get_property(np, "ibm,xive-lisn-ranges", &len);
>   	if (!reg) {
>   		pr_err("Failed to read 'ibm,xive-lisn-ranges' property\n");
> -		return false;
> +		goto err_unmap;
>   	}
>   
>   	if (len % (2 * sizeof(u32)) != 0) {
>   		pr_err("invalid 'ibm,xive-lisn-ranges' property\n");
> -		return false;
> +		goto err_unmap;
>   	}
>   
> -	for (i = 0; i < len / (2 * sizeof(u32)); i++, reg += 2)
> -		xive_irq_bitmap_add(be32_to_cpu(reg[0]),
> -				    be32_to_cpu(reg[1]));
> +	for (i = 0; i < len / (2 * sizeof(u32)); i++, reg += 2) {
> +		err = xive_irq_bitmap_add(be32_to_cpu(reg[0]),
> +					  be32_to_cpu(reg[1]));
> +		if (err < 0)
> +			goto err_mem_free;
> +	}
>   
>   	/* Iterate the EQ sizes and pick one */
>   	of_property_for_each_u32(np, "ibm,xive-eq-sizes", prop, reg, val) {
> @@ -775,8 +789,14 @@ bool __init xive_spapr_init(void)
>   
>   	/* Initialize XIVE core with our backend */
>   	if (!xive_core_init(&xive_spapr_ops, tima, TM_QW1_OS, max_prio))
> -		return false;
> +		goto err_mem_free;
>   
>   	pr_info("Using %dkB queues\n", 1 << (xive_queue_shift - 10));
>   	return true;
> +
> +err_mem_free:
> +	xive_irq_bitmap_remove_all();
> +err_unmap:
> +	iounmap(tima);
> +	return false;
>   }