Re: [PATCH 09/12] soc: fsl: qe: Rename irq variable to parent_irq

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] A race condition during probe can cause a NULL pointer dereference in the chained interrupt handler `qepic_cascade` because the chained IRQ is unmasked before generic chip data is fully initialized.
- [High] An off-by-one logic error in `qepic_cascade` calculates incorrect hardware IRQ indices, rendering interrupts incorrectly routed or completely dead.

Pre-existing issues:
- [High] The mask, unmask, and set_type callbacks perform non-atomic read-modify-write operations on shared controller registers without locking.
--

commit 233494b356181bf50d667f74d4dd3ee1947142f8
Author: Paul Louvel <paul.louvel@bootlin.com>

soc: fsl: qe: Rename irq variable to parent_irq

This commit renames the irq local variable to parent_irq in struct qepic_data
to better describe its role as the upstream interrupt. It updates the
references in qepic_probe, qepic_domain_init, and qepic_domain_exit to use
the new name.

> diff --git a/drivers/soc/fsl/qe/qe_ports_ic.c b/drivers/soc/fsl/qe/qe_ports_ic.c
> index d022aa224f6d6..718703dab0240 100644
> --- a/drivers/soc/fsl/qe/qe_ports_ic.c
> +++ b/drivers/soc/fsl/qe/qe_ports_ic.c
[ ... ]
> @@ -111,7 +111,7 @@ static int qepic_domain_init(struct irq_domain *d)
>  {
>  	struct qepic_data *data = d->host_data;
>  
> -	irq_set_chained_handler_and_data(data->irq, qepic_cascade, data);
> +	irq_set_chained_handler_and_data(data->parent_irq, qepic_cascade, data);

[Severity: High]
Does this chained handler registration have an off-by-one regression when
calculating the hardware IRQ?

Looking at qepic_cascade(), the interrupt index is calculated using a bitwise
operation but retains a subtraction logic from older code:

drivers/soc/fsl/qe/qe_ports_ic.c:qepic_cascade() {
    ...
    for_each_set_bit(bit, &val, 32)
        generic_handle_domain_irq(data->domain, 32 - bit);
    ...
}

Because for_each_set_bit() yields a 0-indexed bit (0 to 31), calculating
32 - bit yields 32 down to 1. For bit 0, it yields hwirq 32, which exceeds
hwirq_max (32, indices 0-31) and gets dropped by the IRQ core. Should this be
31 - bit instead to map correctly?

[ ... ]
> @@ -154,9 +154,9 @@ static int qepic_probe(struct platform_device *pdev)
>  	if (IS_ERR(data->reg))
>  		return PTR_ERR(data->reg);
>  
> -	data->irq = platform_get_irq(pdev, 0);
> -	if (data->irq < 0)
> -		return data->irq;
> +	data->parent_irq = platform_get_irq(pdev, 0);
> +	if (data->parent_irq < 0)
> +		return data->parent_irq;
>  
>  	data->host = devm_irq_domain_instantiate(dev, &d_info);

[Severity: High]
Can this cause a NULL pointer dereference regression during probe if the
interrupt fires immediately?

devm_irq_domain_instantiate() calls qepic_domain_init(), which immediately
registers and unmasks the chained handler qepic_cascade() via
irq_set_chained_handler_and_data().

If an interrupt is already pending or fires before qepic_probe() finishes,
qepic_cascade() will execute and attempt to access data->gc:

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

However, data->gc is not populated until after devm_irq_domain_instantiate()
returns:

drivers/soc/fsl/qe/qe_ports_ic.c:qepic_probe() {
    ...
    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->domain, 0);
    ...
}

Should we defer arming the chained handler until after the generic chip is
fully initialized?

[Severity: High]
This is a pre-existing issue, but do we need to lock the shared registers
during mask/unmask operations?

qepic_mask(), qepic_unmask(), and qepic_set_type() perform non-atomic
read-modify-write operations on shared controller registers:

drivers/soc/fsl/qe/qe_ports_ic.c:qepic_mask() {
    ...
    clrbits32(gc->reg_base + ct->regs.mask, d->mask);
}

These callbacks expand to non-atomic I/O operations. Since the core kernel
only holds the per-IRQ desc->lock during these calls, concurrent interrupt
mask/unmask operations on different CPUs could silently overwrite each other,
causing interrupts to be unintentionally left enabled or disabled.

Would it be safer to acquire gc->lock during these register modifications?

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