| Message ID | 20180922094405.4067-1-andreas@kemnade.info (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] phy: phy-twl4030-usb: fix denied runtime access | expand |
* 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
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
* 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
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
* 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
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)
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(+)