linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] phy: phy-twl4030-usb: fix denied runtime access
@ 2018-09-22  9:44 Andreas Kemnade
  2019-02-20 22:31 ` Tony Lindgren
  0 siblings, 1 reply; 6+ messages in thread
From: Andreas Kemnade @ 2018-09-22  9:44 UTC (permalink / raw)
  To: Dmitry Torokhov, kishon, lee.jones, daniel.thompson, wsa,
	linux-omap, linux-kernel, Discussions about the Letux Kernel
  Cc: Andreas Kemnade

When runtime is not enabled, pm_runtime_get_sync() returns -EACCESS,
the counter will be incremented but the resume callback not called,
so enumeration and charging will not start properly.
To avoid that happen, disable irq on suspend and recheck on resume.

Practically this happens when the device is woken up from suspend by
plugging in usb.

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
Changes in v2:
* use pm suspend/resume callback instead of delayed_work
  as suggested by Dmitry

 drivers/phy/ti/phy-twl4030-usb.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/drivers/phy/ti/phy-twl4030-usb.c b/drivers/phy/ti/phy-twl4030-usb.c
index a44680d64f9b..c267afb68f07 100644
--- a/drivers/phy/ti/phy-twl4030-usb.c
+++ b/drivers/phy/ti/phy-twl4030-usb.c
@@ -144,6 +144,7 @@
 #define PMBR1				0x0D
 #define GPIO_USB_4PIN_ULPI_2430C	(3 << 0)
 
+static irqreturn_t twl4030_usb_irq(int irq, void *_twl);
 /*
  * If VBUS is valid or ID is ground, then we know a
  * cable is present and we need to be runtime-enabled
@@ -395,6 +396,33 @@ static void __twl4030_phy_power(struct twl4030_usb *twl, int on)
 	WARN_ON(twl4030_usb_write_verify(twl, PHY_PWR_CTRL, pwr) < 0);
 }
 
+static int __maybe_unused twl4030_usb_suspend(struct device *dev)
+{
+	struct twl4030_usb *twl = dev_get_drvdata(dev);
+
+	/*
+	 * we need enabled runtime on resume,
+	 * so turn irq off here, so we do not get it early
+	 * note: wakeup on usb plug works independently of this
+	 */
+	dev_dbg(twl->dev, "%s\n", __func__);
+	disable_irq(twl->irq);
+
+	return 0;
+}
+
+static int __maybe_unused twl4030_usb_resume(struct device *dev)
+{
+	struct twl4030_usb *twl = dev_get_drvdata(dev);
+
+	dev_dbg(twl->dev, "%s\n", __func__);
+	enable_irq(twl->irq);
+	/* check whether cable status changed */
+	twl4030_usb_irq(0, twl);
+
+	return 0;
+}
+
 static int __maybe_unused twl4030_usb_runtime_suspend(struct device *dev)
 {
 	struct twl4030_usb *twl = dev_get_drvdata(dev);
@@ -655,6 +683,7 @@ static const struct phy_ops ops = {
 static const struct dev_pm_ops twl4030_usb_pm_ops = {
 	SET_RUNTIME_PM_OPS(twl4030_usb_runtime_suspend,
 			   twl4030_usb_runtime_resume, NULL)
+	SET_SYSTEM_SLEEP_PM_OPS(twl4030_usb_suspend, twl4030_usb_resume)
 };
 
 static int twl4030_usb_probe(struct platform_device *pdev)
-- 
2.11.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] phy: phy-twl4030-usb: fix denied runtime access
  2018-09-22  9:44 [PATCH v2] phy: phy-twl4030-usb: fix denied runtime access Andreas Kemnade
@ 2019-02-20 22:31 ` Tony Lindgren
  2019-02-21 16:08   ` Andreas Kemnade
  0 siblings, 1 reply; 6+ messages in thread
From: Tony Lindgren @ 2019-02-20 22:31 UTC (permalink / raw)
  To: Andreas Kemnade
  Cc: Dmitry Torokhov, kishon, lee.jones, daniel.thompson, wsa,
	linux-omap, linux-kernel, Discussions about the Letux Kernel

* Andreas Kemnade <andreas@kemnade.info> [180922 09:48]:
> When runtime is not enabled, pm_runtime_get_sync() returns -EACCESS,
> the counter will be incremented but the resume callback not called,
> so enumeration and charging will not start properly.
> To avoid that happen, disable irq on suspend and recheck on resume.
> 
> Practically this happens when the device is woken up from suspend by
> plugging in usb.
> 
> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> ---
> Changes in v2:
> * use pm suspend/resume callback instead of delayed_work
>   as suggested by Dmitry

Hmm it just occurred to me that this issue too might be fixed with commit
c6e2bd956936 ("i2c: omap: Use noirq system sleep pm ops to idle device
for suspend"). Andreas, care to check? That is if this is still an
issue.

Regards,

Tony

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] phy: phy-twl4030-usb: fix denied runtime access
  2019-02-20 22:31 ` Tony Lindgren
@ 2019-02-21 16:08   ` Andreas Kemnade
  2019-02-21 16:48     ` Tony Lindgren
  0 siblings, 1 reply; 6+ messages in thread
From: Andreas Kemnade @ 2019-02-21 16:08 UTC (permalink / raw)
  To: Tony Lindgren
  Cc: Dmitry Torokhov, kishon, lee.jones, daniel.thompson, wsa,
	linux-omap, linux-kernel, Discussions about the Letux Kernel

[-- Attachment #1: Type: text/plain, Size: 1513 bytes --]

Hi,

On Wed, 20 Feb 2019 14:31:32 -0800
Tony Lindgren <tony@atomide.com> wrote:

> * Andreas Kemnade <andreas@kemnade.info> [180922 09:48]:
> > When runtime is not enabled, pm_runtime_get_sync() returns -EACCESS,
> > the counter will be incremented but the resume callback not called,
> > so enumeration and charging will not start properly.
> > To avoid that happen, disable irq on suspend and recheck on resume.
> > 
> > Practically this happens when the device is woken up from suspend by
> > plugging in usb.
> > 
> > Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> > ---
> > Changes in v2:
> > * use pm suspend/resume callback instead of delayed_work
> >   as suggested by Dmitry  
> 
> Hmm it just occurred to me that this issue too might be fixed with commit
> c6e2bd956936 ("i2c: omap: Use noirq system sleep pm ops to idle device
> for suspend"). Andreas, care to check? That is if this is still an
> issue.
> 
this one made already its way into 4.20. But for the records, I will
build the latest 5.0-rc and revert it and will check if the problem
still occurs.

I do not think the patch you mentioned has something to do with this
because here solely the problem is about things happen on resume.

But on gta04 we had several irregular behavior regarding to actions
on the other i2c bus where all the sensors and touchscreen are located.
It happens rarely and most often when I am not up to debugging it.
So maybe your patch helps there.

Regards,
Andreas

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] phy: phy-twl4030-usb: fix denied runtime access
  2019-02-21 16:08   ` Andreas Kemnade
@ 2019-02-21 16:48     ` Tony Lindgren
  2019-02-21 19:39       ` Andreas Kemnade
  0 siblings, 1 reply; 6+ messages in thread
From: Tony Lindgren @ 2019-02-21 16:48 UTC (permalink / raw)
  To: Andreas Kemnade
  Cc: Dmitry Torokhov, kishon, lee.jones, daniel.thompson, wsa,
	linux-omap, linux-kernel, Discussions about the Letux Kernel

* Andreas Kemnade <andreas@kemnade.info> [190221 16:43]:
> Hi,
> 
> On Wed, 20 Feb 2019 14:31:32 -0800
> Tony Lindgren <tony@atomide.com> wrote:
> 
> > * Andreas Kemnade <andreas@kemnade.info> [180922 09:48]:
> > > When runtime is not enabled, pm_runtime_get_sync() returns -EACCESS,
> > > the counter will be incremented but the resume callback not called,
> > > so enumeration and charging will not start properly.
> > > To avoid that happen, disable irq on suspend and recheck on resume.
> > > 
> > > Practically this happens when the device is woken up from suspend by
> > > plugging in usb.
> > > 
> > > Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> > > ---
> > > Changes in v2:
> > > * use pm suspend/resume callback instead of delayed_work
> > >   as suggested by Dmitry  
> > 
> > Hmm it just occurred to me that this issue too might be fixed with commit
> > c6e2bd956936 ("i2c: omap: Use noirq system sleep pm ops to idle device
> > for suspend"). Andreas, care to check? That is if this is still an
> > issue.
> > 
> this one made already its way into 4.20. But for the records, I will
> build the latest 5.0-rc and revert it and will check if the problem
> still occurs.

OK thanks.

> I do not think the patch you mentioned has something to do with this
> because here solely the problem is about things happen on resume.

OK

> But on gta04 we had several irregular behavior regarding to actions
> on the other i2c bus where all the sensors and touchscreen are located.
> It happens rarely and most often when I am not up to debugging it.
> So maybe your patch helps there.

Yeah maybe, it's worth checking :)

Regards,

Tony

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] phy: phy-twl4030-usb: fix denied runtime access
  2019-02-21 16:48     ` Tony Lindgren
@ 2019-02-21 19:39       ` Andreas Kemnade
  2019-02-21 21:27         ` Tony Lindgren
  0 siblings, 1 reply; 6+ messages in thread
From: Andreas Kemnade @ 2019-02-21 19:39 UTC (permalink / raw)
  To: Tony Lindgren
  Cc: Dmitry Torokhov, kishon, lee.jones, daniel.thompson, wsa,
	linux-omap, linux-kernel, Discussions about the Letux Kernel

[-- Attachment #1: Type: text/plain, Size: 2573 bytes --]

On Thu, 21 Feb 2019 08:48:03 -0800
Tony Lindgren <tony@atomide.com> wrote:

> * Andreas Kemnade <andreas@kemnade.info> [190221 16:43]:
> > Hi,
> > 
> > On Wed, 20 Feb 2019 14:31:32 -0800
> > Tony Lindgren <tony@atomide.com> wrote:
> >   
> > > * Andreas Kemnade <andreas@kemnade.info> [180922 09:48]:  
> > > > When runtime is not enabled, pm_runtime_get_sync() returns -EACCESS,
> > > > the counter will be incremented but the resume callback not called,
> > > > so enumeration and charging will not start properly.
> > > > To avoid that happen, disable irq on suspend and recheck on resume.
> > > > 
> > > > Practically this happens when the device is woken up from suspend by
> > > > plugging in usb.
> > > > 
> > > > Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> > > > ---
> > > > Changes in v2:
> > > > * use pm suspend/resume callback instead of delayed_work
> > > >   as suggested by Dmitry    
> > > 
> > > Hmm it just occurred to me that this issue too might be fixed with commit
> > > c6e2bd956936 ("i2c: omap: Use noirq system sleep pm ops to idle device
> > > for suspend"). Andreas, care to check? That is if this is still an
> > > issue.
> > >   
> > this one made already its way into 4.20. But for the records, I will
> > build the latest 5.0-rc and revert it and will check if the problem
> > still occurs.  
> 
> OK thanks.
> 
first of all, suspend is totally broken in v5.0-rc7.
I booted with init=/bin/bash
and just loaded gpio-twl4030 and twl4030-pwrbutton

and did (rtc in compiled in) a
 root@(none):/# rtcwake -s 10 -m mem
rtcwake: wakeup from "mem" using /dev/rtc0 at Sat Jan  1 00:05:24 2000
[   86.434722] PM: suspend entry (deep)
[   86.438842] PM: Syncing filesystems ... done.
[   86.458770] Freezing user space processes ... (elapsed 0.003 seconds) done.
[   86.470947] OOM killer disabled.
[   86.474365] Freezing remaining freezable tasks ... (elapsed 0.002 seconds) done.
[   86.485473] printk: Suspending console(s) (use no_console_suspend to debug)
[   86.555572] Disabling non-boot CPUs ...
[   86.555664] Successfully put all powerdomains to target state
[   86.563720] twl: Read failed (mod 1, reg 0x01 count 1)
[   86.563751] twl4030: I2C error -13 reading PIH ISR
[   86.563812] twl: Read failed (mod 1, reg 0x01 count 1)
[   86.563812] twl4030: I2C error -13 reading PIH ISR
[   86.563873] twl: Read failed (mod 1, reg 0x01 count 1)
[   86.563903] twl4030: I2C error -13 reading PIH ISR

and this goes on forever.
I will try to bisect that.

Regards,
Andreas

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] phy: phy-twl4030-usb: fix denied runtime access
  2019-02-21 19:39       ` Andreas Kemnade
@ 2019-02-21 21:27         ` Tony Lindgren
  0 siblings, 0 replies; 6+ messages in thread
From: Tony Lindgren @ 2019-02-21 21:27 UTC (permalink / raw)
  To: Andreas Kemnade
  Cc: Dmitry Torokhov, kishon, lee.jones, daniel.thompson, wsa,
	linux-omap, linux-kernel, Discussions about the Letux Kernel

* Andreas Kemnade <andreas@kemnade.info> [190221 19:40]:
> On Thu, 21 Feb 2019 08:48:03 -0800
> Tony Lindgren <tony@atomide.com> wrote:
> 
> > * Andreas Kemnade <andreas@kemnade.info> [190221 16:43]:
> > > Hi,
> > > 
> > > On Wed, 20 Feb 2019 14:31:32 -0800
> > > Tony Lindgren <tony@atomide.com> wrote:
> > >   
> > > > * Andreas Kemnade <andreas@kemnade.info> [180922 09:48]:  
> > > > > When runtime is not enabled, pm_runtime_get_sync() returns -EACCESS,
> > > > > the counter will be incremented but the resume callback not called,
> > > > > so enumeration and charging will not start properly.
> > > > > To avoid that happen, disable irq on suspend and recheck on resume.
> > > > > 
> > > > > Practically this happens when the device is woken up from suspend by
> > > > > plugging in usb.
> > > > > 
> > > > > Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> > > > > ---
> > > > > Changes in v2:
> > > > > * use pm suspend/resume callback instead of delayed_work
> > > > >   as suggested by Dmitry    
> > > > 
> > > > Hmm it just occurred to me that this issue too might be fixed with commit
> > > > c6e2bd956936 ("i2c: omap: Use noirq system sleep pm ops to idle device
> > > > for suspend"). Andreas, care to check? That is if this is still an
> > > > issue.
> > > >   
> > > this one made already its way into 4.20. But for the records, I will
> > > build the latest 5.0-rc and revert it and will check if the problem
> > > still occurs.  
> > 
> > OK thanks.
> > 
> first of all, suspend is totally broken in v5.0-rc7.
> I booted with init=/bin/bash
> and just loaded gpio-twl4030 and twl4030-pwrbutton
> 
> and did (rtc in compiled in) a
>  root@(none):/# rtcwake -s 10 -m mem
> rtcwake: wakeup from "mem" using /dev/rtc0 at Sat Jan  1 00:05:24 2000
> [   86.434722] PM: suspend entry (deep)
> [   86.438842] PM: Syncing filesystems ... done.
> [   86.458770] Freezing user space processes ... (elapsed 0.003 seconds) done.
> [   86.470947] OOM killer disabled.
> [   86.474365] Freezing remaining freezable tasks ... (elapsed 0.002 seconds) done.
> [   86.485473] printk: Suspending console(s) (use no_console_suspend to debug)
> [   86.555572] Disabling non-boot CPUs ...
> [   86.555664] Successfully put all powerdomains to target state
> [   86.563720] twl: Read failed (mod 1, reg 0x01 count 1)
> [   86.563751] twl4030: I2C error -13 reading PIH ISR
> [   86.563812] twl: Read failed (mod 1, reg 0x01 count 1)
> [   86.563812] twl4030: I2C error -13 reading PIH ISR
> [   86.563873] twl: Read failed (mod 1, reg 0x01 count 1)
> [   86.563903] twl4030: I2C error -13 reading PIH ISR
> 
> and this goes on forever.
> I will try to bisect that.

Strange, I'm not seeing that here. Anyways let's debug more
in your bisect email thread.

Thanks,

Tony


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2019-02-21 21:27 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-22  9:44 [PATCH v2] phy: phy-twl4030-usb: fix denied runtime access Andreas Kemnade
2019-02-20 22:31 ` Tony Lindgren
2019-02-21 16:08   ` Andreas Kemnade
2019-02-21 16:48     ` Tony Lindgren
2019-02-21 19:39       ` Andreas Kemnade
2019-02-21 21:27         ` Tony Lindgren

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).