Re: [PATCH 11/12] soc: fsl: qe: Remove useless struct member

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] The chained cascade handler is registered prematurely during IRQ domain instantiation before the generic IRQ chip and its register base are populated, leading to a potential NULL pointer dereference.
- [High] Off-by-one error in calculating `hwirq` from the interrupt event bit index, leading to out-of-bounds array access and shifted IRQ mapping.

Pre-existing issues:
- [High] Missing synchronization around shared memory-mapped registers in the custom mask, unmask, and set_type operations.
--

commit 5ecb4528440dd9ca9845f840100f9f68d449b15d
Author: Paul Louvel <paul.louvel@bootlin.com>

soc: fsl: qe: Remove useless struct member
    
The reg member from the struct qepic_data is no longer needed, since it
is now embedded in reg_base of struct irq_chip_generic. Remove it.

> diff --git a/drivers/soc/fsl/qe/qe_ports_ic.c b/drivers/soc/fsl/qe/qe_ports_ic.c
> index 1bf2af087b784..6cf23a721ff36 100644
> --- a/drivers/soc/fsl/qe/qe_ports_ic.c
> +++ b/drivers/soc/fsl/qe/qe_ports_ic.c
[ ... ]
> @@ -165,7 +165,7 @@ static int qepic_probe(struct platform_device *pdev)
>  	data->gc = irq_get_domain_generic_chip(data->domain, 0);
>  	if (!data->gc)
>  		return -ENODEV;
> -	data->gc->reg_base = data->reg;
> +	data->gc->reg_base = reg;

[Severity: Critical]
Can this lead to a NULL pointer dereference?

The devm_irq_domain_instantiate() function is called earlier in qepic_probe(),
which synchronously triggers qepic_domain_init().

Inside qepic_domain_init(), irq_set_chained_handler_and_data() is called:

drivers/soc/fsl/qe/qe_ports_ic.c:qepic_domain_init() {
    ...
    irq_set_chained_handler_and_data(data->parent_irq, qepic_cascade, data);
    ...
}

This immediately registers and unmasks the parent interrupt. If a pending
interrupt fires right after this, qepic_cascade() will execute and
dereference data->gc->chip_types:

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

However, data->gc is only allocated and initialized here, after
devm_irq_domain_instantiate() returns. Could data->gc be NULL if the hardware
interrupt is asserted early?


[Severity: High]
Is there an off-by-one error when calculating the hardware IRQ?

Looking at qepic_cascade(), when an interrupt fires, it seems to pass an
incorrect hwirq to the subsystem:

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

If bit is 0, this calculates a hwirq of 32. Since the domain size is 32 (max
hwirq 31), will calling generic_handle_domain_irq() with 32 fail?

Does this also shift the IRQ mapping, making the 0th bit interrupt
unserviceable?


[Severity: High]
This is a pre-existing issue, but is there missing synchronization around the
shared memory-mapped registers?

The custom mask, unmask, and set_type functions perform non-atomic
read-modify-write operations on shared hardware registers without holding
any locks:

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

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

On SMP systems, could concurrent masking or unmasking of different interrupts
on different CPUs race and silently clobber the mask state?

>  
>  	return 0;
>  }

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