[PATCH 1/3] pinctrl: cherryview: Serialize register access in suspend and resume hooks

[PATCH 1/3] pinctrl: cherryview: Serialize register access in suspend and resume hooks

[Mika Westerberg]

If async suspend is enabled, the driver may access registers concurrently
with another instance which may fail because of the bug in Cherryview GPIO
hardware. Prevent this by taking the shared lock while accessing the
hardware in suspend and resume hooks.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/pinctrl/intel/pinctrl-cherryview.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c
index 30389f4ccab4..097d835b3a50 100644
--- a/drivers/pinctrl/intel/pinctrl-cherryview.c
+++ b/drivers/pinctrl/intel/pinctrl-cherryview.c
@@ -1656,8 +1656,11 @@ static int chv_pinctrl_suspend(struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);
 	struct chv_pinctrl *pctrl = platform_get_drvdata(pdev);
+	unsigned long flags;
 	int i;
 
+	raw_spin_lock_irqsave(&chv_lock, flags);
+
 	pctrl->saved_intmask = readl(pctrl->regs + CHV_INTMASK);
 
 	for (i = 0; i < pctrl->community->npins; i++) {
@@ -1678,6 +1681,8 @@ static int chv_pinctrl_suspend(struct device *dev)
 		ctx->padctrl1 = readl(reg);
 	}
 
+	raw_spin_unlock_irqrestore(&chv_lock, flags);
+
 	return 0;
 }
 
@@ -1685,8 +1690,11 @@ static int chv_pinctrl_resume(struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);
 	struct chv_pinctrl *pctrl = platform_get_drvdata(pdev);
+	unsigned long flags;
 	int i;
 
+	raw_spin_lock_irqsave(&chv_lock, flags);
+
 	/*
 	 * Mask all interrupts before restoring per-pin configuration
 	 * registers because we don't know in which state BIOS left them
@@ -1731,6 +1739,8 @@ static int chv_pinctrl_resume(struct device *dev)
 	chv_writel(0xffff, pctrl->regs + CHV_INTSTAT);
 	chv_writel(pctrl->saved_intmask, pctrl->regs + CHV_INTMASK);
 
+	raw_spin_unlock_irqrestore(&chv_lock, flags);
+
 	return 0;
 }
 #endif
-- 
2.9.3

[PATCH 2/3] pinctrl: cherryview: Prevent possible interrupt storm on resume

[Mika Westerberg]

When the system is suspended to S3 the BIOS might re-initialize certain
GPIO pins back to their original state or it may re-program interrupt mask
of others. For example Acer TravelMate B116-M had BIOS bug where certain
GPIO pin (MF_ISH_GPIO_5) was programmed to trigger on high level, and the
pin state was high once the BIOS gave control to the OS on resume.

This triggers lots of messages like:

 irq 117, desc: ffff88017a61e600, depth: 1, count: 0, unhandled: 0
 ->handle_irq():  ffffffff8109b613, handle_bad_irq+0x0/0x1e0
 ->irq_data.chip(): ffffffffa0020180, chv_pinctrl_exit+0x2d84/0x12 [pinctrl_cherryview]
 ->action():           (null)
    IRQ_NOPROBE set

We reset the mask back to known state in chv_pinctrl_resume() but that is
called only after device interrupts have already been enabled.

Now, this particular issue was fixed by upgrading the BIOS to the latest
(v1.23) but not everybody upgrades their BIOSes so we fix it up in the
driver as well.

Prevent the possible interrupt storm by moving suspend and resume hooks to
be called at _noirq time instead. Since device interrupts are still
disabled we can restore the mask back to known state before interrupt storm
happens.

Reported-by: Christian Steiner <christian.steiner@outlook.de>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/pinctrl/intel/pinctrl-cherryview.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c
index 097d835b3a50..c43b1e9a06af 100644
--- a/drivers/pinctrl/intel/pinctrl-cherryview.c
+++ b/drivers/pinctrl/intel/pinctrl-cherryview.c
@@ -1652,7 +1652,7 @@ static int chv_pinctrl_probe(struct platform_device *pdev)
 }
 
 #ifdef CONFIG_PM_SLEEP
-static int chv_pinctrl_suspend(struct device *dev)
+static int chv_pinctrl_suspend_noirq(struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);
 	struct chv_pinctrl *pctrl = platform_get_drvdata(pdev);
@@ -1686,7 +1686,7 @@ static int chv_pinctrl_suspend(struct device *dev)
 	return 0;
 }
 
-static int chv_pinctrl_resume(struct device *dev)
+static int chv_pinctrl_resume_noirq(struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);
 	struct chv_pinctrl *pctrl = platform_get_drvdata(pdev);
@@ -1746,7 +1746,8 @@ static int chv_pinctrl_resume(struct device *dev)
 #endif
 
 static const struct dev_pm_ops chv_pinctrl_pm_ops = {
-	SET_LATE_SYSTEM_SLEEP_PM_OPS(chv_pinctrl_suspend, chv_pinctrl_resume)
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(chv_pinctrl_suspend_noirq,
+				      chv_pinctrl_resume_noirq)
 };
 
 static const struct acpi_device_id chv_pinctrl_acpi_match[] = {
-- 
2.9.3

[PATCH 3/3] pinctrl: cherryview: Drop ctrlX prefix from the pin debugfs output

[Mika Westerberg]

Printing the prefix does not provide any additional information. In
addition this makes the output look more consistent with pinctrl-intel.c.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/pinctrl/intel/pinctrl-cherryview.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c
index c43b1e9a06af..5e66860a5e67 100644
--- a/drivers/pinctrl/intel/pinctrl-cherryview.c
+++ b/drivers/pinctrl/intel/pinctrl-cherryview.c
@@ -762,7 +762,7 @@ static void chv_pin_dbg_show(struct pinctrl_dev *pctldev, struct seq_file *s,
 		seq_printf(s, "mode %d ", mode);
 	}
 
-	seq_printf(s, "ctrl0 0x%08x ctrl1 0x%08x", ctrl0, ctrl1);
+	seq_printf(s, "0x%08x 0x%08x", ctrl0, ctrl1);
 
 	if (locked)
 		seq_puts(s, " [LOCKED]");
-- 
2.9.3

Re: [PATCH 1/3] pinctrl: cherryview: Serialize register access in suspend and resume hooks

[Linus Walleij]

On Mon, Oct 31, 2016 at 3:57 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:

> If async suspend is enabled, the driver may access registers concurrently
> with another instance which may fail because of the bug in Cherryview GPIO
> hardware. Prevent this by taking the shared lock while accessing the
> hardware in suspend and resume hooks.
>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Patch applied.

Yours,
Linus Walleij

Re: [PATCH 2/3] pinctrl: cherryview: Prevent possible interrupt storm on resume

[Linus Walleij]

On Mon, Oct 31, 2016 at 3:57 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:

> When the system is suspended to S3 the BIOS might re-initialize certain
> GPIO pins back to their original state or it may re-program interrupt mask
> of others. For example Acer TravelMate B116-M had BIOS bug where certain
> GPIO pin (MF_ISH_GPIO_5) was programmed to trigger on high level, and the
> pin state was high once the BIOS gave control to the OS on resume.
>
> This triggers lots of messages like:
>
>  irq 117, desc: ffff88017a61e600, depth: 1, count: 0, unhandled: 0
>  ->handle_irq():  ffffffff8109b613, handle_bad_irq+0x0/0x1e0
>  ->irq_data.chip(): ffffffffa0020180, chv_pinctrl_exit+0x2d84/0x12 [pinctrl_cherryview]
>  ->action():           (null)
>     IRQ_NOPROBE set
>
> We reset the mask back to known state in chv_pinctrl_resume() but that is
> called only after device interrupts have already been enabled.
>
> Now, this particular issue was fixed by upgrading the BIOS to the latest
> (v1.23) but not everybody upgrades their BIOSes so we fix it up in the
> driver as well.
>
> Prevent the possible interrupt storm by moving suspend and resume hooks to
> be called at _noirq time instead. Since device interrupts are still
> disabled we can restore the mask back to known state before interrupt storm
> happens.
>
> Reported-by: Christian Steiner <christian.steiner@outlook.de>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Patch applied.

Yours,
Linus Walleij

Re: [PATCH 3/3] pinctrl: cherryview: Drop ctrlX prefix from the pin debugfs output

[Linus Walleij]

On Mon, Oct 31, 2016 at 3:57 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:

> Printing the prefix does not provide any additional information. In
> addition this makes the output look more consistent with pinctrl-intel.c.
>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Patch applied.

The first two patches were applied for fixes, this one for next.

Yours,
Linus Walleij

Re: [PATCH 1/3] pinctrl: cherryview: Serialize register access in suspend and resume hooks

[Linus Walleij]

On Fri, Nov 4, 2016 at 4:12 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Mon, Oct 31, 2016 at 3:57 PM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
>
>> If async suspend is enabled, the driver may access registers concurrently
>> with another instance which may fail because of the bug in Cherryview GPIO
>> hardware. Prevent this by taking the shared lock while accessing the
>> hardware in suspend and resume hooks.
>>
>> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
>
> Patch applied.

Mika should this and patch 2/3 be tagged for stable?

Yours,
Linus Walleij

Re: [PATCH 1/3] pinctrl: cherryview: Serialize register access in suspend and resume hooks

[Mika Westerberg]

On Fri, Nov 04, 2016 at 04:15:59PM +0100, Linus Walleij wrote:
> On Fri, Nov 4, 2016 at 4:12 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> > On Mon, Oct 31, 2016 at 3:57 PM, Mika Westerberg
> > <mika.westerberg@linux.intel.com> wrote:
> >
> >> If async suspend is enabled, the driver may access registers concurrently
> >> with another instance which may fail because of the bug in Cherryview GPIO
> >> hardware. Prevent this by taking the shared lock while accessing the
> >> hardware in suspend and resume hooks.
> >>
> >> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> >
> > Patch applied.
> 
> Mika should this and patch 2/3 be tagged for stable?

Yes, I think they should.