Re: [PATCH v3 5/8] irqchip/qcom-pdc: Configure PDC to pass through mode

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Read-Modify-Write (RMW) operations on `irq_cfg_reg` race with asynchronous hardware updates to the latched status bit, risking lost interrupts.
--

commit 5bfc60b86ecb20a923a4f052f469cb8a8237153f
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 using an SCM write
access. If the write fails, it falls back to a secondary controller mode
where GPIO IRQs are latched at the PDC and sent to the GIC as level
interrupts.

> diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
> index 1aa6be42307ca..c6f2935ff7887 100644
> --- a/drivers/irqchip/qcom-pdc.c
> +++ b/drivers/irqchip/qcom-pdc.c

[ ... ]

> @@ -231,6 +260,30 @@ 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;
> +
> +	if (pdc->version < PDC_VERSION_3_0)
> +		return;
> +
> +	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;
> +
> +	if (pdc->version < PDC_VERSION_3_0)
> +		return;
> +
> +	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]
Could this read-modify-write operation accidentally clear a pending interrupt?

Since pdc_clear_gpio_cfg() clears the hardware latch by setting gpio_irq_sts
to 0, it appears this is a Write-0-to-Clear (W0C) register bit. 

If a new interrupt arrives and the hardware sets gpio_irq_sts to 1 right
after the read but before the write completes:

CPU
pdc_unmask_gpio_cfg()
    gpio_mask = pdc_reg_read(...) // reads gpio_irq_sts as 0

Hardware
Interrupt arrives, sets gpio_irq_sts to 1

CPU
pdc_unmask_gpio_cfg()
    pdc_reg_write(..., gpio_mask) // writes 0 back to gpio_irq_sts

Will this write of 0 inadvertently clear the newly latched interrupt before
it can be processed by the system?

> +}
> +
>  static void pdc_enable_intr_cfg(int pin_out, bool on)
>  {
>  	unsigned long enable = pdc_reg_read(pdc->regs->irq_cfg_reg, pin_out);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260616-hamoa_pdc_v3-v3-0-4d8e1504ea75@oss.qualcomm.com?part=5