Re: [PATCH v3] pinctrl: qcom: Restore the pin configuration after hibernation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] pinctrl: qcom: msm_pinctrl_restore_noirq() enables IRQ before clearing status
--

commit d8439285386d0a36cbc72eb8f4753391eb0fd921
Author: Birk Skyum <birk.skyum@pm.me>

pinctrl: qcom: Restore the pin configuration after hibernation

This commit adds freeze_noirq and restore_noirq hibernation callbacks to
pinctrl-msm to save and restore the control, output, and interrupt
configuration registers for pins in use by the kernel as well as the SDC
pads. It also wires up msm_pinctrl_dev_pm_ops in the x1e80100 pinctrl
driver.

> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> index 27f06bbc113c9..8ddb4261bf89a 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c

[ ... ]

> +static int msm_pinctrl_restore_noirq(struct device *dev)
> +{
> +	struct msm_pinctrl *pctrl = dev_get_drvdata(dev);
> +	const struct msm_pingroup *g;
> +	struct msm_pin_state *s;
> +	unsigned int i;
> +	u32 val;
> +
> +	guard(raw_spinlock_irqsave)(&pctrl->lock);
> +
> +	for (i = 0; i < pctrl->soc->ngroups; i++) {
> +		g = &pctrl->soc->groups[i];
> +		s = &pctrl->saved[i];
> +		if (!s->saved)
> +			continue;
> +
> +		if (i >= pctrl->chip.ngpio) {
> +			msm_writel_ctl(s->ctl, pctrl, g);
> +			continue;
> +		}
> +
> +		/* Set the level before the pin starts driving it. */
> +		val = msm_readl_io(pctrl, g);
> +		val &= ~BIT(g->out_bit);
> +		val |= s->io & BIT(g->out_bit);
> +		msm_writel_io(val, pctrl, g);
> +
> +		msm_writel_ctl(s->ctl, pctrl, g);
> +		if (!g->intr_cfg_reg)
> +			continue;
> +
> +		if (msm_pin_has_own_intr_target(pctrl, g))
> +			msm_writel_intr_target(s->intr_target, pctrl, g);
> +		val = msm_readl_intr_cfg(pctrl, g);
> +		msm_writel_intr_cfg(s->intr_cfg, pctrl, g);
> +
> +		/*
> +		 * Changing the configuration can latch an interrupt. One that
> +		 * is latched with the configuration unchanged is a real one.
> +		 */
> +		if (val != s->intr_cfg)
> +			msm_ack_intr_status(pctrl, g);

[Severity: Medium]
Can writing s->intr_cfg before calling msm_ack_intr_status() in
msm_pinctrl_restore_noirq() in drivers/pinctrl/qcom/pinctrl-msm.c assert a
spurious interrupt on the TLMM summary IRQ line?

In msm_gpio_irq_set_type(), writing intr_cfg before clearing the latched
status is noted as safe only because the interrupt is masked:

drivers/pinctrl/qcom/pinctrl-msm.c:msm_gpio_irq_set_type() {
	...
	/*
	 * The first time we set RAW_STATUS_EN it could trigger an interrupt.
	 * Clear the interrupt.  This is safe because we have
	 * IRQCHIP_SET_TYPE_MASKED. When changing the interrupt type, we could
	 * also still have a non-matching interrupt latched, so clear whenever
	 * making changes to the interrupt configuration.
	 */
	if (val != oldval)
		msm_ack_intr_status(pctrl, g);
	...
}

During hibernation freeze, device_wakeup_arm_wake_irqs() and
suspend_device_irqs() -> suspend_device_irq() in kernel/irq/pm.c run before
msm_pinctrl_freeze_noirq(). For any GPIO IRQ that is not in
pctrl->skip_wake_irqs and has wakeup enabled (or uses IRQF_NO_SUSPEND),
suspend_device_irq() leaves the line unmasked:

kernel/irq/pm.c:suspend_device_irq() {
	if (!desc->action || irq_desc_is_chained(desc) ||
	    desc->no_suspend_depth)
		return false;

	if (irqd_is_wakeup_set(irqd)) {
		irqd_set(irqd, IRQD_WAKEUP_ARMED);

		if ((chipflags & IRQCHIP_ENABLE_WAKEUP_ON_SUSPEND) &&
		     irqd_irq_disabled(irqd)) {
			__enable_irq(desc);
			irqd_set(irqd, IRQD_IRQ_ENABLED_ON_SUSPEND);
		}
		return true;
	}
	...
}

As a result, msm_pinctrl_freeze_noirq() saves s->intr_cfg with both
BIT(g->intr_raw_status_bit) and BIT(g->intr_enable_bit) set, and bit i
remains set in pctrl->enabled_irqs.

On hibernation restore in create_image() in kernel/power/hibernate.c,
local_irq_enable() and pm_sleep_enable_secondary_cpus() run before
dpm_resume_start(PMSG_RESTORE) -> dpm_resume_noirq():

kernel/power/hibernate.c:create_image() {
	...
 Enable_irqs:
	system_state = SYSTEM_RUNNING;
	local_irq_enable();

 Enable_cpus:
	pm_sleep_enable_secondary_cpus();
	...
	dpm_resume_start(in_suspend ?
		(error ? PMSG_RECOVER : PMSG_THAW) : PMSG_RESTORE);
	...
}

Because the parent TLMM summary IRQ (pctrl->irq) is a chained handler
(msm_gpio_irq_handler), suspend_device_irq() skips it via
irq_desc_is_chained(), leaving it active and unmasked at the GIC while
secondary CPUs are online with IRQs enabled.

When msm_pinctrl_restore_noirq() writes s->intr_cfg with
BIT(g->intr_enable_bit) set before calling msm_ack_intr_status(), any
status bit latched by toggling RAW_STATUS_EN or changing detection/polarity
bits can immediately assert the summary IRQ. Since msm_gpio_irq_handler()
does not take pctrl->lock, another CPU servicing the summary IRQ could
either observe the bogus status bit and dispatch a spurious interrupt, or
observe handled == 0 after msm_ack_intr_status() clears it and call
handle_bad_irq().

Would it be safer to write s->intr_cfg with BIT(g->intr_enable_bit) masked
first, call msm_ack_intr_status() if the configuration changed, and only
then set BIT(g->intr_enable_bit) if it was saved as enabled?

> +	}
> +
> +	return 0;
> +}

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021195250.83540-1-birk.skyum@pm.me?part=1