Re: [PATCH 05/16] irqchip/bcm7038-l1: clean up init failure paths

[Florian Fainelli]

On 7/14/26 06:24, Haofeng Li wrote:
> From: Haofeng Li <lihaofeng@xxxxxxxxxx>
>
> bcm7038_l1_init_one() stores the allocated CPU object in
> intc->cpus[idx] before mapping its registers and parent interrupt.
> Failures after that allocation leak the CPU object and possibly its
> MMIO mapping.
>
> A later IRQ domain allocation failure also frees intc while leaving
> parent mappings and chained handlers installed for CPUs initialized
> successfully. Those handlers retain intc as their data and can access
> freed memory.
>
> Free the current CPU object on local failures. Remember every parent
> IRQ so the common error path can disable wake when it was enabled,
> remove the chained handler, dispose the mapping, unmap the registers,
> and free the CPU object before freeing intc.
>
> Fixes: 5f7f0317ed28 ("IRQCHIP: Add new driver for BCM7038-style level 1 interrupt controllers")
>
> Signed-off-by: Haofeng Li <lihaofeng@xxxxxxxxxx>
> ---
>   drivers/irqchip/irq-bcm7038-l1.c | 24 ++++++++++++++++++++----
>   1 file changed, 20 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/irqchip/irq-bcm7038-l1.c b/drivers/irqchip/irq-bcm7038-l1.c
> index 54a8557ef557..b42c4f4df6bf 100644
> --- a/drivers/irqchip/irq-bcm7038-l1.c
> +++ b/drivers/irqchip/irq-bcm7038-l1.c
> @@ -49,6 +49,8 @@ struct bcm7038_l1_chip {
>   
>   struct bcm7038_l1_cpu {
>   	void __iomem		*map_base;
> +	unsigned int		parent_irq;
> +	bool			wake_enabled;
>   	u32			mask_cache[];
>   };
>   
> @@ -247,8 +249,11 @@ static int bcm7038_l1_init_one(struct device_node *dn, unsigned int idx,
>   		return -ENOMEM;
>   
>   	cpu->map_base = ioremap(res.start, sz);
> -	if (!cpu->map_base)
> +	if (!cpu->map_base) {
> +		kfree(cpu);
> +		intc->cpus[idx] = NULL;

This pattern repeats below, consider creating a new label, that would do 
the 'cpu' variable freeing and assigng intc->cpus[idx] to NULL?

>   		return -ENOMEM;
> +	}
>   
>   	for (i = 0; i < n_words; i++) {
>   		l1_writel(~intc->irq_fwd_mask[i],
> @@ -261,12 +266,17 @@ static int bcm7038_l1_init_one(struct device_node *dn, unsigned int idx,
>   	parent_irq = irq_of_parse_and_map(dn, idx);
>   	if (!parent_irq) {
>   		pr_err("failed to map parent interrupt %d\n", parent_irq);
> +		iounmap(cpu->map_base);

And this would belong to a new label as well

> +		kfree(cpu);
> +		intc->cpus[idx] = NULL;

That new label would be used here.

>   		return -EINVAL;
>   	}
>   
> -	if (of_property_read_bool(dn, "brcm,irq-can-wake"))
> -		enable_irq_wake(parent_irq);
> +	if (of_property_read_bool(dn, "brcm,irq-can-wake") &&
> +	    !enable_irq_wake(parent_irq))
> +		cpu->wake_enabled = true;
>   
> +	cpu->parent_irq = parent_irq;
>   	irq_set_chained_handler_and_data(parent_irq, bcm7038_l1_irq_handle,
>   					 intc);
>   
> @@ -408,7 +418,7 @@ static int bcm7038_l1_probe(struct platform_device *pdev, struct device_node *pa
>   			if (idx)
>   				break;
>   			pr_err("failed to remap intc L1 registers\n");
> -			goto out_free;
> +			goto out_unmap;
>   		}
>   	}
>   
> @@ -440,6 +450,12 @@ static int bcm7038_l1_probe(struct platform_device *pdev, struct device_node *pa
>   		struct bcm7038_l1_cpu *cpu = intc->cpus[idx];
>   
>   		if (cpu) {
> +			if (cpu->parent_irq) {
> +				if (cpu->wake_enabled)
> +					disable_irq_wake(cpu->parent_irq);
> +				irq_set_chained_handler_and_data(cpu->parent_irq, NULL, NULL);
> +				irq_dispose_mapping(cpu->parent_irq);
> +			}
>   			if (cpu->map_base)
>   				iounmap(cpu->map_base);
>   			kfree(cpu);

--
Florian