Re: [PATCH 10/12] soc: fsl: qe: Rename host member to domain in struct qepic_data

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Off-by-one mapping between hardware interrupts and virtual IRQs due to incorrect bit index arithmetic.
- [High] Uninitialized pointer dereference because the chained IRQ handler is registered before dependencies are populated.
--

commit 2f6243965deebfddabc0af7472b701df854ecb78
Author: Paul Louvel <paul.louvel@bootlin.com>

soc: fsl: qe: Rename host member to domain in struct qepic_data

This commit renames the host field to domain in struct qepic_data to match
the common kernel naming convention for irq_domain pointers.

> diff --git a/drivers/soc/fsl/qe/qe_ports_ic.c b/drivers/soc/fsl/qe/qe_ports_ic.c
> index 718703dab0240..1bf2af087b784 100644
> --- a/drivers/soc/fsl/qe/qe_ports_ic.c
> +++ b/drivers/soc/fsl/qe/qe_ports_ic.c

[ ... ]

> @@ -85,7 +85,7 @@ static void qepic_cascade(struct irq_desc *desc)
>  	}
>  
>  	for_each_set_bit(bit, &event, 32)
> -		generic_handle_domain_irq(data->host, 32 - bit);
> +		generic_handle_domain_irq(data->domain, 32 - bit);

[Severity: High]
Does this code map the hardware interrupts to the wrong virtual IRQs?

Since for_each_set_bit() provides a 0-based index for bit, mapping it
using 32 - bit shifts all interrupts by one (e.g. mapping hardware
interrupt 31 to 32, which is out of bounds). Should this be 31 - bit
instead?

>  out:
>  	chained_irq_exit(chip, desc);

[ ... ]

> @@ -158,11 +158,11 @@ static int qepic_probe(struct platform_device *pdev)
>  	if (data->parent_irq < 0)
>  		return data->parent_irq;
>  
> -	data->host = devm_irq_domain_instantiate(dev, &d_info);
> -	if (IS_ERR(data->host))
> -		return PTR_ERR(data->host);
> +	data->domain = devm_irq_domain_instantiate(dev, &d_info);
> +	if (IS_ERR(data->domain))
> +		return PTR_ERR(data->domain);
>  
> -	data->gc = irq_get_domain_generic_chip(data->host, 0);
> +	data->gc = irq_get_domain_generic_chip(data->domain, 0);

[Severity: High]
Can this result in a NULL pointer dereference if the parent interrupt
fires during probe?

Calling devm_irq_domain_instantiate() synchronously invokes the domain
init function, which registers the chained handler (qepic_cascade) and
unmasks the parent interrupt. If the interrupt triggers immediately,
qepic_cascade() will execute before data->gc is populated here:

drivers/soc/fsl/qe/qe_ports_ic.c:qepic_cascade() {
    ...
	struct irq_chip_type *ct = data->gc->chip_types;
    ...
}

Will data->gc still be NULL when the handler tries to access it?

>  	if (!data->gc)
>  		return -ENODEV;
>  	data->gc->reg_base = data->reg;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260703-qe-pic-gpios-v1-0-6c3e706e27dc@bootlin.com?part=10