* [PATCH] rtc: max77686: Do not allow interrupt to fire before system resume @ 2020-06-15 16:14 Krzysztof Kozlowski 2020-06-29 22:23 ` Alexandre Belloni 2020-07-16 9:15 ` Alexandre Belloni 0 siblings, 2 replies; 4+ messages in thread From: Krzysztof Kozlowski @ 2020-06-15 16:14 UTC (permalink / raw) To: Chanwoo Choi, Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz, Alessandro Zummo, Alexandre Belloni, linux-kernel, linux-rtc Cc: Marek Szyprowski, Lee Jones The rtc-max77686 device shares the main interrupt line with parent MFD device (max77686 driver). During the system suspend, the parent MFD device disables this IRQ to prevent an early event happening before resuming I2C bus controller. The same should be done by rtc-max77686 driver because otherwise the interrupt handler max77686_rtc_alarm_irq() will be called before its resume function (max77686_rtc_resume()). Such issue is not fatal but disabling shared IRQ by all users ensures correct behavior. Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org> --- If this looks ok, I guess all maxim RTC drivers should be updated? --- drivers/rtc/rtc-max77686.c | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/drivers/rtc/rtc-max77686.c b/drivers/rtc/rtc-max77686.c index 03ebcf1c0f3d..645de5af707b 100644 --- a/drivers/rtc/rtc-max77686.c +++ b/drivers/rtc/rtc-max77686.c @@ -805,17 +805,33 @@ static int max77686_rtc_remove(struct platform_device *pdev) #ifdef CONFIG_PM_SLEEP static int max77686_rtc_suspend(struct device *dev) { + struct max77686_rtc_info *info = dev_get_drvdata(dev); + int ret = 0; + if (device_may_wakeup(dev)) { struct max77686_rtc_info *info = dev_get_drvdata(dev); - return enable_irq_wake(info->virq); + ret = enable_irq_wake(info->virq); } - return 0; + /* + * Main IRQ (not virtual) must be disabled during suspend because if it + * happens while suspended it will be handled before resuming I2C. + * + * Since Main IRQ is shared, all its users should disable it to be sure + * it won't fire while one of them is still suspended. + */ + disable_irq(info->rtc_irq); + + return ret; } static int max77686_rtc_resume(struct device *dev) { + struct max77686_rtc_info *info = dev_get_drvdata(dev); + + enable_irq(info->rtc_irq); + if (device_may_wakeup(dev)) { struct max77686_rtc_info *info = dev_get_drvdata(dev); -- 2.17.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] rtc: max77686: Do not allow interrupt to fire before system resume 2020-06-15 16:14 [PATCH] rtc: max77686: Do not allow interrupt to fire before system resume Krzysztof Kozlowski @ 2020-06-29 22:23 ` Alexandre Belloni 2020-06-30 6:28 ` Krzysztof Kozlowski 2020-07-16 9:15 ` Alexandre Belloni 1 sibling, 1 reply; 4+ messages in thread From: Alexandre Belloni @ 2020-06-29 22:23 UTC (permalink / raw) To: Krzysztof Kozlowski Cc: Chanwoo Choi, Bartlomiej Zolnierkiewicz, Alessandro Zummo, linux-kernel, linux-rtc, Marek Szyprowski, Lee Jones Hi, On 15/06/2020 18:14:55+0200, Krzysztof Kozlowski wrote: > The rtc-max77686 device shares the main interrupt line with parent MFD > device (max77686 driver). During the system suspend, the parent MFD > device disables this IRQ to prevent an early event happening before > resuming I2C bus controller. > > The same should be done by rtc-max77686 driver because otherwise the > interrupt handler max77686_rtc_alarm_irq() will be called before its > resume function (max77686_rtc_resume()). Such issue is not fatal but > disabling shared IRQ by all users ensures correct behavior. > > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org> > > --- > > If this looks ok, I guess all maxim RTC drivers should be updated? But then, shouldn't that means that this affects all RTCs on an i2c bus? I'm not completely proficient with PM but shouldn't using _noirq propagate to the parents? Then you are sure that the resume is called with interrupts disabled and this also means that the i2c driver has resumed (hopefully with interrupts disabled). I must admit that I don't know and I hope that the answer would also answer whether moving all the RTC resume to resume_early is safe. (See https://lore.kernel.org/linux-rtc/20200610132403.2539519-1-martin@geanix.com/#t) > --- > drivers/rtc/rtc-max77686.c | 20 ++++++++++++++++++-- > 1 file changed, 18 insertions(+), 2 deletions(-) > > diff --git a/drivers/rtc/rtc-max77686.c b/drivers/rtc/rtc-max77686.c > index 03ebcf1c0f3d..645de5af707b 100644 > --- a/drivers/rtc/rtc-max77686.c > +++ b/drivers/rtc/rtc-max77686.c > @@ -805,17 +805,33 @@ static int max77686_rtc_remove(struct platform_device *pdev) > #ifdef CONFIG_PM_SLEEP > static int max77686_rtc_suspend(struct device *dev) > { > + struct max77686_rtc_info *info = dev_get_drvdata(dev); > + int ret = 0; > + > if (device_may_wakeup(dev)) { > struct max77686_rtc_info *info = dev_get_drvdata(dev); > > - return enable_irq_wake(info->virq); > + ret = enable_irq_wake(info->virq); > } > > - return 0; > + /* > + * Main IRQ (not virtual) must be disabled during suspend because if it > + * happens while suspended it will be handled before resuming I2C. > + * > + * Since Main IRQ is shared, all its users should disable it to be sure > + * it won't fire while one of them is still suspended. > + */ > + disable_irq(info->rtc_irq); > + > + return ret; > } > > static int max77686_rtc_resume(struct device *dev) > { > + struct max77686_rtc_info *info = dev_get_drvdata(dev); > + > + enable_irq(info->rtc_irq); > + > if (device_may_wakeup(dev)) { > struct max77686_rtc_info *info = dev_get_drvdata(dev); > > -- > 2.17.1 > -- Alexandre Belloni, Bootlin Embedded Linux and Kernel engineering https://bootlin.com ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] rtc: max77686: Do not allow interrupt to fire before system resume 2020-06-29 22:23 ` Alexandre Belloni @ 2020-06-30 6:28 ` Krzysztof Kozlowski 0 siblings, 0 replies; 4+ messages in thread From: Krzysztof Kozlowski @ 2020-06-30 6:28 UTC (permalink / raw) To: Alexandre Belloni Cc: Chanwoo Choi, Bartlomiej Zolnierkiewicz, Alessandro Zummo, linux-kernel, linux-rtc, Marek Szyprowski, Lee Jones On Tue, Jun 30, 2020 at 12:23:53AM +0200, Alexandre Belloni wrote: > Hi, > > On 15/06/2020 18:14:55+0200, Krzysztof Kozlowski wrote: > > The rtc-max77686 device shares the main interrupt line with parent MFD > > device (max77686 driver). During the system suspend, the parent MFD > > device disables this IRQ to prevent an early event happening before > > resuming I2C bus controller. > > > > The same should be done by rtc-max77686 driver because otherwise the > > interrupt handler max77686_rtc_alarm_irq() will be called before its > > resume function (max77686_rtc_resume()). Such issue is not fatal but > > disabling shared IRQ by all users ensures correct behavior. > > > > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org> > > > > --- > > > > If this looks ok, I guess all maxim RTC drivers should be updated? > > But then, shouldn't that means that this affects all RTCs on an i2c bus? > > I'm not completely proficient with PM but shouldn't using _noirq > propagate to the parents? Then you are sure that the resume is called > with interrupts disabled and this also means that the i2c driver has > resumed (hopefully with interrupts disabled). This won't work, at least for Exynos platforms now, because of ordering with other drivers. The RTC and MFD enable wake up interrupt which goes through pinctrl... but pinctrl suspends with "late", so before "noirq". Therefore the wake does not work. In the same time, the enable_irq_wake() (I am not sure why) also uses I2C which goes to I2C bus driver... which suspends in "noirq" as well. This results in some I2C timeouts during suspend and resume: [ 82.631344] wake enabled for irq 131 [ 82.632743] wake enabled for irq 132 [ 82.649868] samsung-pinctrl 11000000.pinctrl: Setting external wakeup interrupt mask: 0xfffbf7ff [ 82.658367] max77686_rtc_suspend:811 [ 82.658488] max77686_rtc_suspend:815 [ 87.695000] s3c-i2c 13860000.i2c: timeout waiting for bus idle [ 87.695574] dummy 0-0006: Failed to sync masks in 1 [ 87.700446] wake enabled for irq 127 [ 87.703883] max77686_rtc_suspend:818 [ 87.707408] max77686_rtc_suspend:829 [ 87.716862] max77686_suspend:236 [ 87.716972] max77686_suspend:240 [ 87.717714] max77686_suspend:252 [ 87.723173] Disabling non-boot CPUs ... [ 87.745844] Enabling non-boot CPUs ... [ 87.751164] CPU1 is up [ 87.754842] CPU2 is up [ 87.759008] CPU3 is up [ 87.760598] s3c-i2c 13860000.i2c: slave address 0x00 [ 87.760709] s3c-i2c 13860000.i2c: bus frequency set to 390 KHz [ 87.760851] s3c-i2c 13870000.i2c: slave address 0x00 [ 87.760951] s3c-i2c 13870000.i2c: bus frequency set to 97 KHz [ 87.761085] s3c-i2c 13880000.i2c: slave address 0x00 [ 87.761185] s3c-i2c 13880000.i2c: bus frequency set to 97 KHz [ 87.761319] s3c-i2c 138e0000.i2c: slave address 0x00 [ 87.761418] s3c-i2c 138e0000.i2c: bus frequency set to 97 KHz [ 87.764159] max77686_resume:261 [ 87.764236] max77686_resume:265 [ 87.764303] max77686_resume:268 [ 87.768204] max77686_rtc_resume:837 [ 87.768283] max77686_rtc_resume:840 [ 87.768370] max77686_rtc_resume:844 [ 92.813964] s3c-i2c 13860000.i2c: timeout waiting for bus idle [ 92.814105] dummy 0-0006: Failed to sync masks in 1 [ 92.814205] wake disabled for irq 127 In general, disabling and enabling this shared interrupt seems like a nice way to be able to suspend/resume driver in the "normal" time, without the need to order anything (noirq, late). Best regards, Krzysztof > > I must admit that I don't know and I hope that the answer would also > answer whether moving all the RTC resume to resume_early is safe. (See > https://lore.kernel.org/linux-rtc/20200610132403.2539519-1-martin@geanix.com/#t) > > > --- > > drivers/rtc/rtc-max77686.c | 20 ++++++++++++++++++-- > > 1 file changed, 18 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/rtc/rtc-max77686.c b/drivers/rtc/rtc-max77686.c > > index 03ebcf1c0f3d..645de5af707b 100644 > > --- a/drivers/rtc/rtc-max77686.c > > +++ b/drivers/rtc/rtc-max77686.c > > @@ -805,17 +805,33 @@ static int max77686_rtc_remove(struct platform_device *pdev) > > #ifdef CONFIG_PM_SLEEP > > static int max77686_rtc_suspend(struct device *dev) > > { > > + struct max77686_rtc_info *info = dev_get_drvdata(dev); > > + int ret = 0; > > + > > if (device_may_wakeup(dev)) { > > struct max77686_rtc_info *info = dev_get_drvdata(dev); > > > > - return enable_irq_wake(info->virq); > > + ret = enable_irq_wake(info->virq); > > } > > > > - return 0; > > + /* > > + * Main IRQ (not virtual) must be disabled during suspend because if it > > + * happens while suspended it will be handled before resuming I2C. > > + * > > + * Since Main IRQ is shared, all its users should disable it to be sure > > + * it won't fire while one of them is still suspended. > > + */ > > + disable_irq(info->rtc_irq); > > + > > + return ret; > > } > > > > static int max77686_rtc_resume(struct device *dev) > > { > > + struct max77686_rtc_info *info = dev_get_drvdata(dev); > > + > > + enable_irq(info->rtc_irq); > > + > > if (device_may_wakeup(dev)) { > > struct max77686_rtc_info *info = dev_get_drvdata(dev); > > > > -- > > 2.17.1 > > > > -- > Alexandre Belloni, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] rtc: max77686: Do not allow interrupt to fire before system resume 2020-06-15 16:14 [PATCH] rtc: max77686: Do not allow interrupt to fire before system resume Krzysztof Kozlowski 2020-06-29 22:23 ` Alexandre Belloni @ 2020-07-16 9:15 ` Alexandre Belloni 1 sibling, 0 replies; 4+ messages in thread From: Alexandre Belloni @ 2020-07-16 9:15 UTC (permalink / raw) To: Bartlomiej Zolnierkiewicz, Alessandro Zummo, linux-rtc, linux-kernel, Chanwoo Choi, Krzysztof Kozlowski Cc: Alexandre Belloni, Lee Jones, Marek Szyprowski On Mon, 15 Jun 2020 18:14:55 +0200, Krzysztof Kozlowski wrote: > The rtc-max77686 device shares the main interrupt line with parent MFD > device (max77686 driver). During the system suspend, the parent MFD > device disables this IRQ to prevent an early event happening before > resuming I2C bus controller. > > The same should be done by rtc-max77686 driver because otherwise the > interrupt handler max77686_rtc_alarm_irq() will be called before its > resume function (max77686_rtc_resume()). Such issue is not fatal but > disabling shared IRQ by all users ensures correct behavior. Applied, thanks! [1/1] rtc: max77686: Do not allow interrupt to fire before system resume commit: d8f090dbeafdcc3d30761aa0062f19d1adf9ef08 Best regards, -- Alexandre Belloni <alexandre.belloni@bootlin.com> ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-07-16 9:15 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-06-15 16:14 [PATCH] rtc: max77686: Do not allow interrupt to fire before system resume Krzysztof Kozlowski 2020-06-29 22:23 ` Alexandre Belloni 2020-06-30 6:28 ` Krzysztof Kozlowski 2020-07-16 9:15 ` Alexandre Belloni
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).