[PATCH] powerpc/xive: Add some error handling code to 'xive_spapr_init()'

[PATCH] powerpc/xive: Add some error handling code to 'xive_spapr_init()'

[Christophe JAILLET]

'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(...)'

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;
 }
-- 
2.20.1

Re: [PATCH] powerpc/xive: Add some error handling code to 'xive_spapr_init()'

[Cédric Le Goater]

On 01/08/2019 13:09, Christophe JAILLET wrote:
> '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(...)'
> 
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> NOT compile tested (I don't have a cross compiler and won't install one).

All distros have a packaged powerpc cross compiler. 

Then, you need to compile a kernel for a pseries machine and run a pseries
machine with it under QEMU. You can use a simple ppc initrd, a net install 
one for example.

You could also hack the device tree in QEMU to torture the XIVE sPAPR driver.
Nothing too complex, all is here : 

https://git.qemu.org/?p=qemu.git;a=blob;f=hw/intc/spapr_xive.c;h=097f88d4608d8ba160526756a3a224e5176b6e0f;hb=HEAD#l1427


> So if some correction or improvement are needed, feel free to propose and
> commit it directly.

Yes there is I think. I would move at the end all the code that needs a 
rollback.

Thanks for taking a look, I might do that one day.

Cheers,
C.  


> ---
>  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;
>  }
> 

Re: [PATCH] powerpc/xive: Add some error handling code to 'xive_spapr_init()'

[Christophe Leroy]

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;
>   }

Re: [PATCH] powerpc/xive: Add some error handling code to 'xive_spapr_init()'

[Christophe JAILLET]

Le 01/02/2022 à 12:31, Christophe Leroy a écrit :
> 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
> 

Hi, funny to see a 2 1/2 years old patch to pop-up like that :)
It still looks relevant to me.

V2 sent.
Still not compile tested.

CJ

Re: [PATCH] powerpc/xive: Add some error handling code to 'xive_spapr_init()'

[Christophe Leroy]

Le 01/02/2022 à 13:32, Christophe JAILLET a écrit :
> Le 01/02/2022 à 12:31, Christophe Leroy a écrit :
>> 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
>>
> 
> Hi, funny to see a 2 1/2 years old patch to pop-up like that :)
> It still looks relevant to me.

Yeah I'm trying to clean some dust in Patchwork.

> 
> V2 sent.
> Still not compile tested.
> 

At least it's all green at 
https://patchwork.ozlabs.org/project/linuxppc-dev/patch/564998101804886b151235c8a9f93020923bfd2c.1643718324.git.christophe.jaillet@wanadoo.fr/

Christophe