Re: [PATCH AUTOSEL 6.18-6.1] pinctrl: qcom: Add irq_get/set_irqchip_state() for msm gpio irqchip

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] pinctrl: qcom: missing parent delegation in msm_gpio_irq_set_irqchip_state()
- [Medium] pinctrl: qcom: intr_ack_high ignored in msm_gpio_irq_set_irqchip_state()
--

commit 301c33b42ee8c08eddc97fb199477c476ddbf18c
Author: Maulik Shah <maulik.shah@oss.qualcomm.com>

pinctrl: qcom: Add irq_get/set_irqchip_state() for msm gpio irqchip

This commit implements the .irq_get_irqchip_state and .irq_set_irqchip_state
callbacks for msm_gpio_irq_chip to support replaying edge interrupts for
IRQCHIP_STATE_PENDING after wakeup on MPM-based Qualcomm SoCs.

Link: https://patch.msgid.link/20260424-pinctrl_irqchip_states-v1-1-85286f078916@oss.qualcomm.com

> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> index e99871b90ab9c..4804d5cba011c 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> @@ -1289,6 +1289,43 @@ static int msm_gpio_irq_set_affinity(struct irq_data *d,
>  	return -EINVAL;
>  }
>  
> +static int msm_gpio_irq_set_irqchip_state(struct irq_data *d,
> +					  enum irqchip_irq_state which, bool val)
> +{
> +	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> +	struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
> +	const struct msm_pingroup *g = &pctrl->soc->groups[d->hwirq];
> +
> +	if (which != IRQCHIP_STATE_PENDING)
> +		return -EINVAL;
> +
> +	if (test_bit(d->hwirq, pctrl->skip_wake_irqs))
> +		return -EINVAL;

[Severity: High]
Does returning -EINVAL here and in msm_gpio_irq_get_irqchip_state() break
parent interrupt state queries and updates on PDC-based Qualcomm SoCs?

In kernel/irq/manage.c, __irq_get_irqchip_state() and irq_set_irqchip_state()
walk up data->parent_data and stop at the first irq_chip that defines the
callback, without falling back to parent_data when -EINVAL is returned:

kernel/irq/manage.c:__irq_get_irqchip_state() {
    ...
    do {
        chip = irq_data_get_irq_chip(data);
        if (WARN_ON_ONCE(!chip))
            return -ENODEV;
        if (chip->irq_get_irqchip_state)
            break;
#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
        data = data->parent_data;
#else
        data = NULL;
#endif
    } while (data);
    ...
}

On PDC-based SoCs (IRQ_DOMAIN_FLAG_QCOM_PDC_WAKEUP), msm_gpio_init() sets
pctrl->skip_wake_irqs for wakeup-capable GPIOs because interrupt detection is
handled by the parent hierarchy (msmgpio -> PDC -> GICv3), and
qcom_pdc_gic_chip in drivers/irqchip/qcom-pdc.c explicitly wires:

    .irq_get_irqchip_state = irq_chip_get_parent_state,
    .irq_set_irqchip_state = irq_chip_set_parent_state,

Previously, msm_gpio_irq_chip left both callbacks NULL, so calls walked past
msmgpio to PDC and GICv3. With this change, when synchronize_irq(),
disable_irq(), or __free_irq() calls __synchronize_hardirq() ->
__irq_get_irqchip_state(irqd, IRQCHIP_STATE_ACTIVE, &inprogress), or when
KVM vgic, machine_kexec_mask_interrupts(), or irq_inject_interrupt() queries
or updates state on a PDC-routed GPIO, the walk stops at msmgpio and fails
with -EINVAL.

Should msm_gpio_irq_set_irqchip_state() and msm_gpio_irq_get_irqchip_state()
delegate to irq_chip_set_parent_state(d, which, val) and
irq_chip_get_parent_state(d, which, val) when d->parent_data &&
test_bit(d->hwirq, pctrl->skip_wake_irqs), similar to
msm_gpio_irq_set_affinity() and msm_gpio_irq_set_vcpu_affinity()?

> +
> +	msm_writel_intr_status(val, pctrl, g);
> +
> +	return 0;
> +}
> +
> +static int msm_gpio_irq_get_irqchip_state(struct irq_data *d,
> +					  enum irqchip_irq_state which, bool *val)
> +{
> +	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> +	struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
> +	const struct msm_pingroup *g = &pctrl->soc->groups[d->hwirq];
> +
> +	if (which != IRQCHIP_STATE_PENDING)
> +		return -EINVAL;
> +
> +	if (test_bit(d->hwirq, pctrl->skip_wake_irqs))
> +		return -EINVAL;
> +
> +	g = &pctrl->soc->groups[d->hwirq];
> +	*val = msm_readl_intr_status(pctrl, g);
> +
> +	return 0;
> +}

[Severity: Medium]
How does msm_gpio_irq_set_irqchip_state() behave on TLMM v2 SoCs where
g->intr_ack_high is set (such as apq8064, ipq8064, mdm9615, msm8660, and
msm8960), or if g->intr_status_bit is non-zero?

In drivers/pinctrl/qcom/pinctrl-msm.c, msm_ack_intr_status() clears a pending
interrupt by accounting for both g->intr_ack_high and g->intr_status_bit:

static void msm_ack_intr_status(struct msm_pinctrl *pctrl,
				const struct msm_pingroup *g)
{
	u32 val = g->intr_ack_high ? BIT(g->intr_status_bit) : 0;

	msm_writel_intr_status(val, pctrl, g);
}

By passing the raw boolean val directly to msm_writel_intr_status(val, pctrl, g),
won't calling irq_set_irqchip_state(irq, IRQCHIP_STATE_PENDING, true) on an
intr_ack_high platform acknowledge and clear the pending interrupt instead of
setting it pending, while passing false writes 0 and fails to clear it?

Also, in msm_gpio_irq_get_irqchip_state(), g = &pctrl->soc->groups[d->hwirq]
is initialized twice, and *val = msm_readl_intr_status(pctrl, g) converts the
full 32-bit register read to bool without masking with BIT(g->intr_status_bit)
(unlike msm_gpio_irq_handler()). Should both functions mask/shift with
BIT(g->intr_status_bit) and account for g->intr_ack_high?

[ ... ]

>
> -- 
> 2.34.1
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019135441.1420968-105-sashal@kernel.org?part=1