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

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

[Birk Skyum]

A hibernation image is restored after a power cycle. The TLMM registers
then hold what the boot firmware and the kernel that loaded the image
made of them. The restored kernel has the configuration it applied in
memory only, and nothing writes it to the hardware again, so every pin
that the loading kernel did not set up the same way stays wrong.

On a Lenovo Yoga Slim 7x (X1E80100) the Bluetooth and Wi-Fi enable lines
are two of them. Their drivers are loaded from the root file system, so
the kernel that loads the image never touches the pins. This is
/sys/kernel/debug/gpio before the hibernation and after the resume:

  gpio116 : out low  func0 2mA no pull
  gpio117 : out high func0 2mA no pull

  gpio116 : in  low  func0 2mA pull down
  gpio117 : out high func0 16mA no pull

The driver that owns gpio116 still believes it drives the line.

Save the control, output and interrupt configuration registers of every
valid pin in freeze_noirq and write them back in restore_noirq, the
output level first, so that a pin drives the right level from the
moment it becomes an output. Enabling the raw interrupt status can
latch an interrupt, so the status is cleared afterwards, as
msm_gpio_irq_set_type() does.

msm_pinctrl_dev_pm_ops gets the two callbacks and x1e80100 starts using
these ops. With that x1e80100 also gets their suspend and resume
callbacks, which apply the sleep and default state of the controller's
own pins where a board defines them.

Signed-off-by: Birk Skyum <birk.skyum@pm.me>
---
RFC because the two callbacks go into msm_pinctrl_dev_pm_ops, which the
sc7180, sc7280, sdm845 and sm7150 drivers use as well, and I can only
test x1e80100. If you would rather have this for x1e80100 alone, or
done another way, I am happy to redo it.

Tested on a Lenovo Yoga Slim 7x with Arch Linux ARM's v7.2 kernel plus
this patch. The kernel also carried the PCI change in [1], without
which this machine does not resume at all, the smp2p change in [2] and
a GENI serial change that is not ready for the list. The patch is
based on pinctrl/devel, where it builds without warnings (W=1,
CONFIG_HIBERNATION=y).

- Without the patch, gpio116 and gpio117 come back as shown above
  after a hibernation with a real power-off. With it, in four such
  hibernations, the TLMM part of /sys/kernel/debug/gpio differs only
  in the level read on two inputs (gpio93 and gpio153).
- pm_test=core and test_resume pass.
- A suspend (deep) and wake still works. On x1e80100 it now goes
  through msm_pinctrl_suspend() and msm_pinctrl_resume().

Not covered here: the LPASS LPI pin controller, whose pins also came
back at their reset values after a hibernation.

[1] https://lore.kernel.org/r/179157746908.73485.2343672865542690797@pm.me
[2] https://lore.kernel.org/r/179158127177.70185.5691100900886332545@pm.me

 drivers/pinctrl/qcom/pinctrl-msm.c      | 106 +++++++++++++++++++++++-
 drivers/pinctrl/qcom/pinctrl-x1e80100.c |   1 +
 2 files changed, 105 insertions(+), 2 deletions(-)

diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index 27f06bbc1..fba8a4165 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -39,6 +39,14 @@
 #define MAX_NR_TILES 4
 #define PS_HOLD_OFFSET 0x820
 
+/* Register values of one pin, kept across a hibernation. */
+struct msm_pin_state {
+	u32 ctl;
+	u32 io;
+	u32 intr_cfg;
+	u32 intr_target;
+};
+
 /**
  * struct msm_pinctrl - state for a pinctrl-msm device
  * @dev:            device handle.
@@ -58,6 +66,7 @@
  * @soc:            Reference to soc_data of platform specific data.
  * @regs:           Base addresses for the TLMM tiles.
  * @phys_base:      Physical base address
+ * @saved:          Pin registers kept while a hibernation image is taken.
  */
 struct msm_pinctrl {
 	struct device *dev;
@@ -80,6 +89,8 @@ struct msm_pinctrl {
 	const struct msm_pinctrl_soc_data *soc;
 	void __iomem *regs[MAX_NR_TILES];
 	u32 phys_base[MAX_NR_TILES];
+
+	struct msm_pin_state *saved;
 };
 
 #define MSM_ACCESSOR(name) \
@@ -1583,9 +1594,93 @@ static __maybe_unused int msm_pinctrl_resume(struct device *dev)
 	return pinctrl_force_default(pctrl->pctrl);
 }
 
-SIMPLE_DEV_PM_OPS(msm_pinctrl_dev_pm_ops, msm_pinctrl_suspend,
-		  msm_pinctrl_resume);
+static bool msm_pin_has_own_intr_target(struct msm_pinctrl *pctrl,
+					const struct msm_pingroup *g)
+{
+	return !pctrl->intr_target_use_scm && g->intr_target_reg &&
+	       g->intr_target_reg != g->intr_cfg_reg;
+}
+
+/* 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);
+	const struct msm_pingroup *g;
+	struct msm_pin_state *s;
+	unsigned long flags;
+	unsigned int i;
+
+	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);
+		s->io = msm_readl_io(pctrl, g);
+		if (!g->intr_cfg_reg)
+			continue;
+
+		s->intr_cfg = msm_readl_intr_cfg(pctrl, g);
+		if (msm_pin_has_own_intr_target(pctrl, g))
+			s->intr_target = msm_readl_intr_target(pctrl, g);
+	}
+	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
+
+	return 0;
+}
+
+/*
+ * The image is restored after a power cycle, when the pins are back at what
+ * the firmware and the kernel that loaded the image made of them. Nothing else
+ * writes the configuration again, so put back what was there.
+ */
+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 long flags;
+	unsigned int i;
+	u32 val;
+
+	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];
+
+		/* 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);
+		msm_writel_intr_cfg(s->intr_cfg, pctrl, g);
+
+		/* Enabling the raw status can latch an interrupt. */
+		if (s->intr_cfg & BIT(g->intr_raw_status_bit))
+			msm_ack_intr_status(pctrl, g);
+	}
+	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
+
+	return 0;
+}
+
+const struct dev_pm_ops msm_pinctrl_dev_pm_ops = {
+	SYSTEM_SLEEP_PM_OPS(msm_pinctrl_suspend, msm_pinctrl_resume)
+	.freeze_noirq = pm_sleep_ptr(msm_pinctrl_freeze_noirq),
+	.restore_noirq = pm_sleep_ptr(msm_pinctrl_restore_noirq),
+};
 EXPORT_SYMBOL(msm_pinctrl_dev_pm_ops);
 
 int msm_pinctrl_probe(struct platform_device *pdev,
@@ -1663,6 +1758,13 @@ int msm_pinctrl_probe(struct platform_device *pdev,
 	if (ret)
 		return ret;
 
+	if (IS_ENABLED(CONFIG_HIBERNATION)) {
+		pctrl->saved = devm_kcalloc(&pdev->dev, pctrl->chip.ngpio,
+					    sizeof(*pctrl->saved), GFP_KERNEL);
+		if (!pctrl->saved)
+			return -ENOMEM;
+	}
+
 	platform_set_drvdata(pdev, pctrl);
 
 	dev_dbg(&pdev->dev, "Probed Qualcomm pinctrl driver\n");
diff --git a/drivers/pinctrl/qcom/pinctrl-x1e80100.c b/drivers/pinctrl/qcom/pinctrl-x1e80100.c
index e4c0abcd9..bfe122fc7 100644
--- a/drivers/pinctrl/qcom/pinctrl-x1e80100.c
+++ b/drivers/pinctrl/qcom/pinctrl-x1e80100.c
@@ -1855,6 +1855,7 @@ static struct platform_driver x1e80100_pinctrl_driver = {
 	.driver = {
 		.name = "x1e80100-tlmm",
 		.of_match_table = x1e80100_pinctrl_of_match,
+		.pm = &msm_pinctrl_dev_pm_ops,
 	},
 	.probe = x1e80100_pinctrl_probe,
 };
-- 
2.53.0

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

[sashiko-bot]

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

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

[Birk Skyum]

> Will this result in a NULL pointer dereference on kernels built with
> CONFIG_HIBERNATION=n but CONFIG_HIBERNATE_CALLBACKS=y?

Yes, the two conditions do not match. The only other option that
selects HIBERNATE_CALLBACKS is XEN_SAVE_RESTORE on x86, where no TLMM
exists, but nothing in the driver says so. In v2 the array and the
callbacks depend on the same option:

	.freeze_noirq = PTR_IF(IS_ENABLED(CONFIG_HIBERNATE_CALLBACKS),
			       msm_pinctrl_freeze_noirq),
	.restore_noirq = PTR_IF(IS_ENABLED(CONFIG_HIBERNATE_CALLBACKS),
				msm_pinctrl_restore_noirq),

and in msm_pinctrl_probe():

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

> Will unconditionally clearing the interrupt status here drop legitimately
> latched edge wakeups?

Yes. The kernel that loads the image can have set a pin up the same
way, and an edge that is latched there is a real one. The commit message
says the patch does what msm_gpio_irq_set_type() does, but that function
clears the status only where it changed the register. v2 does that too:

		val = msm_readl_intr_cfg(pctrl, g);
		msm_writel_intr_cfg(s->intr_cfg, pctrl, g);
		if (val != s->intr_cfg)
			msm_ack_intr_status(pctrl, g);

> Could blindly restoring the saved intr_cfg cause missed dual-edge interrupts?
> [...]
> 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?

Not with the drivers that use these ops today. The polarity is flipped
only for pins with one-bit detection (intr_detection_width == 1), which
apq8064, ipq8064, mdm9615, msm8660 and msm8960 have. sc7180, sc7280,
sdm845, sm7150 and x1e80100 detect both edges in hardware. Where the
parent emulates both edges (wakeirq_dual_edge_errata), the polarity is
in the parent, and this patch does not touch it.

I would not call msm_gpio_update_dual_edge_pos() for it either. It
flips the polarity before it compares the levels, so for a line that
did not change it would leave the opposite of what was primed. A driver
with such pins would need the polarity set from the level instead. I
have no such SoC to test that on, so v2 only gets a comment that says
these pins are not primed again.

Both changes ran on the Yoga Slim 7x before I wrote this, on the same
kernel as before with v2 of the patch. pm_test=core and test_resume
pass, and after a hibernation with a power-off the TLMM part of
/sys/kernel/debug/gpio again differs only in the level read on gpio93
and gpio153.

I will hold v2 until a maintainer has had a look at the RFC question.