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.