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

["Paul Louvel" <paul.louvel@bootlin.com>]

On Fri Jul 3, 2026 at 3:45 PM CEST, sashiko-bot wrote:
> 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]?
>
> [ ... ]
>

Valid point.

>> @@ -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?

Valid point.




-- 
Paul Louvel, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com