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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] pinctrl: qcom: NULL pointer dereference in freeze_noirq under CONFIG_HIBERNATE_CALLBACKS=y && CONFIG_HIBERNATION=n
- [Medium] pinctrl: qcom: lost edge wakeups due to unconditional ack in restore_noirq
- [Medium] pinctrl: dual-edge interrupt missed after hibernation
--

commit 0f4785b0198c4a184e8d48cdee17d396f8fd5f1c
Author: Birk Skyum <birk.skyum@pm.me>
Subject: pinctrl: qcom: Restore the pin configuration after hibernation

This commit saves the control, output, and interrupt configuration registers
of every valid pin during freeze_noirq. It then restores these saved values
during restore_noirq to ensure pins untouched by the loading kernel regain
their correct pre-hibernation state.

> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> index 27f06bbc113c9..fba8a41655261 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
[ ... ]
> +/* Remember how the pins are set up when a hibernation image is taken. */
> +static int msm_pinctrl_freeze_noirq(struct device *dev)
> +{
> +	struct msm_pinctrl *pctrl = dev_get_drvdata(dev);
[ ... ]
> +	raw_spin_lock_irqsave(&pctrl->lock, flags);
> +	for (i = 0; i < pctrl->chip.ngpio; i++) {
> +		if (!gpiochip_line_is_valid(&pctrl->chip, i))
> +			continue;
> +
> +		g = &pctrl->soc->groups[i];
> +		s = &pctrl->saved[i];
> +		s->ctl = msm_readl_ctl(pctrl, g);

[Severity: Medium]
Will this result in a NULL pointer dereference on kernels built with
CONFIG_HIBERNATION=n but CONFIG_HIBERNATE_CALLBACKS=y?

The pctrl->saved array is allocated strictly under CONFIG_HIBERNATION in
msm_pinctrl_probe():

    if (IS_ENABLED(CONFIG_HIBERNATION)) {
        pctrl->saved = devm_kcalloc(&pdev->dev, pctrl->chip.ngpio, ...);

However, the callbacks are guarded by pm_sleep_ptr() in
msm_pinctrl_dev_pm_ops, which depends on CONFIG_PM_SLEEP (enabled if either
SUSPEND or HIBERNATE_CALLBACKS is set). If a system (such as a Xen guest on
x86 with COMPILE_TEST) triggers PM_EVENT_FREEZE, msm_pinctrl_freeze_noirq()
could be called and unconditionally dereference the unallocated pctrl->saved
array here.

[ ... ]
> +static int msm_pinctrl_restore_noirq(struct device *dev)
> +{
[ ... ]
> +		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);
> +		msm_writel_intr_cfg(s->intr_cfg, pctrl, g);

[Severity: Medium]
Could blindly restoring the saved intr_cfg cause missed dual-edge interrupts?

QCOM hardware emulates dual-edge interrupts by dynamically setting the
polarity opposite to the current physical line level. If the physical line
toggled during hibernation, the restored polarity will match the current
state rather than being opposite to it, causing the hardware to wait for
an edge that already occurred.

Should this re-evaluate the line level and update the polarity by calling
msm_gpio_update_dual_edge_pos() for pins marked in pctrl->dual_edge_irqs
during resume?

> +
> +		/* Enabling the raw status can latch an interrupt. */
> +		if (s->intr_cfg & BIT(g->intr_raw_status_bit))
> +			msm_ack_intr_status(pctrl, g);

[Severity: Medium]
Will unconditionally clearing the interrupt status here drop legitimately
latched edge wakeups?

The TLMM hardware can latch real edge interrupts while the loading kernel
is active or during the suspend/resume window. Unconditionally calling
msm_ack_intr_status() for every pin with RAW_STATUS_EN enabled, without
checking if the configuration actually toggled (as is safely done in
msm_gpio_irq_set_type()), might silently drop pending edge wakeups causing
peripheral drivers to miss events.

> +	}
> +	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
> +
> +	return 0;
> +}

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