Re: [PATCH 08/12] soc: fsl: qe: Convert to generic IRQ chip

[sashiko-bot@kernel.org]

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

New issues:
- [High] Incorrect hwirq calculation in `qepic_cascade()` maps bit indices [0..31] to hwirqs [32..1] instead of [31..0], causing out-of-bounds mapping and shifting all IRQs by one.
- [High] Race condition on probe: The chained interrupt handler is registered before `data->gc` is initialized, causing a NULL pointer dereference if an interrupt fires early.

Pre-existing issues:
- [High] The IRQ mask, unmask, and set_type callbacks use lockless read-modify-write operations on shared registers, leading to data races.
--

commit 39268a559b43e95ee6bcd43807086719d4997f6e
Author: Paul Louvel <paul.louvel@bootlin.com>

soc: fsl: qe: Convert to generic IRQ chip

The generic IRQ chip framework is available to handle IRQ chips. Using
this framework for the QE interrupt controller allows to simplify the
driver. Indeed, the framework internally handles operations coded
directly in the driver.

> diff --git a/drivers/soc/fsl/qe/qe_ports_ic.c b/drivers/soc/fsl/qe/qe_ports_ic.c
> index c8b73b0aa2334..d022aa224f6d6 100644
> --- a/drivers/soc/fsl/qe/qe_ports_ic.c
> +++ b/drivers/soc/fsl/qe/qe_ports_ic.c
> @@ -20,63 +20,65 @@ struct qepic_data {
>  	void __iomem *reg;
>  	struct irq_domain *host;
>  	int irq;
> +	struct irq_chip_generic *gc;
>  };
>  
>  static void qepic_mask(struct irq_data *d)
>  {
> -	struct qepic_data *data = irq_data_get_irq_chip_data(d);
> +	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
> +	struct irq_chip_type *ct = irq_data_get_chip_type(d);
>  
> -	clrbits32(data->reg + CEPIMR, 1 << (31 - irqd_to_hwirq(d)));
> +	clrbits32(gc->reg_base + ct->regs.mask, d->mask);
>  }

[Severity: High]
This is a pre-existing issue, but does this lockless read-modify-write
operation cause data races?

The qepic_mask(), qepic_unmask(), and qepic_set_type() callbacks use
clrbits32() and setbits32() on shared registers without a shared controller
lock. If multiple CPUs modify different IRQs concurrently, could updates to
the shared mask or type registers be lost?

[ ... ]

>  static void qepic_cascade(struct irq_desc *desc)
>  {
>  	struct qepic_data *data = irq_desc_get_handler_data(desc);
> +	struct irq_chip_type *ct = data->gc->chip_types;
>  	struct irq_chip *chip = irq_desc_get_chip(desc);
>  	unsigned long event, bit;
>  
>  	chained_irq_enter(chip, desc);
>  
> -	event = in_be32(data->reg + CEPIER);
> +	event = in_be32(data->gc->reg_base + ct->regs.eoi);
>  	if (!event) {
>  		handle_bad_irq(desc);
>  		goto out;
>  	}

[Severity: High]
Could the hwirq calculation in qepic_cascade() be incorrect?

When an interrupt fires, the trailing loop (using for_each_set_bit) yields a
0-based bit index, but generic_handle_domain_irq() evaluates the hwirq using
"32 - bit".

For example, if bit 0 is set, it maps to hwirq 32, which exceeds the domain's
hwirq_max of 32 and leaves all IRQs shifted out of bounds. Does this need to
be "31 - bit" to correctly map bit indices [0..31] to hwirqs [31..0]?

[ ... ]

> @@ -125,14 +158,16 @@ static int qepic_probe(struct platform_device *pdev)
>  	if (data->irq < 0)
>  		return data->irq;
>  
> -	data->host = irq_domain_create_linear(dev_fwnode(dev), 32, &qepic_host_ops, data);
> -	if (!data->host)
> -		return -ENODEV;
> +	data->host = devm_irq_domain_instantiate(dev, &d_info);
> +	if (IS_ERR(data->host))
> +		return PTR_ERR(data->host);
>  
> -	irq_set_chained_handler_and_data(data->irq, qepic_cascade, data);
> -
> -	return devm_add_action_or_reset(dev, qepic_remove, data);
> +	data->gc = irq_get_domain_generic_chip(data->host, 0);
> +	if (!data->gc)
> +		return -ENODEV;
> +	data->gc->reg_base = data->reg;
>  
> +	return 0;
>  }

[Severity: High]
Can this probe sequence trigger a NULL pointer dereference?

When devm_irq_domain_instantiate() is called, it triggers the init callback
qepic_domain_init(), which registers the chained handler and unmasks the
parent interrupt:

qepic_domain_init()
    irq_set_chained_handler_and_data(data->parent_irq, qepic_cascade, data);

If an interrupt is pending and fires immediately, qepic_cascade() will
execute and attempt to dereference data->gc->reg_base. However, data->gc is
not initialized until irq_get_domain_generic_chip() is called after the
domain instantiation returns.

Could this cause a crash if a pending interrupt is triggered during probe?

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