Re: [PATCH v4 4/7] irqchip/qcom-pdc: Configure PDC to pass through mode

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Edge-triggered GPIO interrupts on PDC secondary controllers are never cleared from the hardware latch, leading to an infinite interrupt storm.
- [High] Unprotected Read-Modify-Write (RMW) operations on `irq_cfg_reg` can overwrite and clear the hardware interrupt latch (`gpio_irq_sts`), resulting in lost interrupts.
- [Low] Redundant call to `irq_domain_set_hwirq_and_chip` in `qcom_pdc_alloc`.
--

--- Patch [4]: [PATCH v4 4/7] irqchip/qcom-pdc: Configure PDC to pass through mode ---
commit 283bb75867e93f6367d30274c8a45615e4e22c2c
Author: Maulik Shah <maulik.shah@oss.qualcomm.com>

irqchip/qcom-pdc: Configure PDC to pass through mode

This commit configures the PDC to pass-through mode via SCM write, and
falls back to a secondary controller mode if it fails. It introduces separate
IRQ chip operations to handle GPIO interrupts in secondary mode.

> diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
> index 53a477aa97655..d122e8e7dc9d7 100644
> --- a/drivers/irqchip/qcom-pdc.c
> +++ b/drivers/irqchip/qcom-pdc.c
[ ... ]
> @@ -229,6 +258,24 @@ static void pdc_enable_intr_bank(int pin_out, bool on)
>  		pdc_reg_write(pdc->regs->irq_en_reg, index, enable);
>  }
>  
> +static void pdc_clear_gpio_cfg(int pin_out)
> +{
> +	unsigned long gpio_sts;
> +
> +	gpio_sts = pdc_reg_read(pdc->regs->irq_cfg_reg, pin_out);
> +	__clear_bit(pdc->cfg_fields->gpio_irq_sts, &gpio_sts);
> +	pdc_reg_write(pdc->regs->irq_cfg_reg, pin_out, gpio_sts);
> +}
> +
> +static void pdc_unmask_gpio_cfg(int pin_out, bool unmask)
> +{
> +	unsigned long gpio_mask;
> +
> +	gpio_mask = pdc_reg_read(pdc->regs->irq_cfg_reg, pin_out);
> +	__assign_bit(pdc->cfg_fields->gpio_irq_mask, &gpio_mask, !unmask);
> +	pdc_reg_write(pdc->regs->irq_cfg_reg, pin_out, gpio_mask);
> +}

[Severity: High]
Can these software Read-Modify-Write operations on irq_cfg_reg race with
hardware setting the interrupt status bit?

If a hardware interrupt arrives between the software read and write, the
software will read a 0 for gpio_irq_sts. When the hardware sets it to 1,
and the software writes back the stale 0, does this inadvertently clear
the newly arrived interrupt and drop it entirely?

[ ... ]
> +static void qcom_pdc_secondary_ack(struct irq_data *d)
> +{
> +	if (!irqd_is_level_type(d))
> +		pdc->clear_gpio(d->hwirq);
> +}
> +
> +static void qcom_pdc_gic_secondary_eoi(struct irq_data *d)
> +{
> +	if (irqd_is_level_type(d))
> +		pdc->clear_gpio(d->hwirq);
> +
> +	irq_chip_eoi_parent(d);
> +}

[Severity: High]
For edge-triggered interrupts, does this logic result in the hardware latch
never being cleared, leading to an interrupt storm?

Since qcom_pdc_alloc() forces these interrupts to IRQ_TYPE_LEVEL_HIGH before
passing them to the parent GIC domain, the GIC will unconditionally configure
them to use handle_fasteoi_irq().

Because handle_fasteoi_irq() only calls .irq_eoi and never calls .irq_ack,
qcom_pdc_secondary_ack() appears to be unreachable dead code. Meanwhile,
qcom_pdc_gic_secondary_eoi() explicitly skips clearing the latch for edge
types.

Would we need to call irq_set_handler_locked(d, handle_fasteoi_ack_irq) in
qcom_pdc_gic_secondary_set_type() for edge interrupts to ensure the latch gets
cleared?

[ ... ]
> @@ -388,16 +563,38 @@ static int qcom_pdc_alloc(struct irq_domain *domain, unsigned int virq,
>  	if (ret)
>  		return ret;
>  
> +	/*
> +	 * PDC secondary chip is only set for the GPIO interrupts as SPIs.
> +	 * Direct SPI interrupts are still in pass through mode (no latching
> +	 * at PDC).
> +	 */
> +	if (pdc->mode == PDC_SECONDARY_MODE && pdc_pin_is_gpio(hwirq)) {
> +		ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
> +						    &qcom_pdc_gic_secondary_chip,
> +						    NULL);
> +		if (ret)
> +			return ret;
> +
> +		/* Secondary mode converts all interrupts to LEVEL HIGH type */
> +		type = IRQ_TYPE_LEVEL_HIGH;
> +	} else {
> +		ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
> +						    &qcom_pdc_gic_chip,
> +						    NULL);
> +		if (ret)
> +			return ret;

[Severity: Low]
Is this call to irq_domain_set_hwirq_and_chip() for &qcom_pdc_gic_chip
redundant?

There is already an unconditional call to irq_domain_set_hwirq_and_chip()
for &qcom_pdc_gic_chip just before the block added by this hunk. Does this
duplicate the earlier setup?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260707-hamoa_pdc_v3-v4-0-dfd1f4a3ae89@oss.qualcomm.com?part=4