| Message ID | 20180513211347.7187-1-digetx@gmail.com |
|---|---|
| State | Deferred |
| Headers | show |
| Series | [v1] i2c: tegra: Remove suspend-resume | expand |
On Mon, May 14, 2018 at 12:13:47AM +0300, Dmitry Osipenko wrote: > Nothing prevents I2C clients to access I2C while Tegra's driver is being > suspended, this results in -EBUSY error returned to the clients and that > may have unfortunate consequences. In particular this causes problems > for the TPS6586x MFD driver which emits hundreds of "failed to read > interrupt status" error messages on resume from suspend. This happens if > TPS6586X is used to wake system from suspend by the expired RTC alarm > timer because TPS6586X is an I2C device driver and its IRQ handler reads > the status register while Tegra's I2C driver is suspended, i.e. just after > kernel enabled IRQ's during of resume-from-suspend process. > > Note that the removed tegra_i2c_resume() invoked tegra_i2c_init() which > performs HW reset. That seems was also not entirely correct because moving > tegra_i2c_resume to an earlier stage of resume-from-suspend process causes > I2C transfer to fail in the case of TPS6586X. It is fine to remove the > HW-reinitialization for now because it should be only needed in a case of > using lowest power-mode during suspend, which upstream kernel doesn't > support. > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > Cc: <stable@vger.kernel.org> > --- > drivers/i2c/busses/i2c-tegra.c | 33 --------------------------------- > 1 file changed, 33 deletions(-) Shardar, Laxman, any thoughts on this? The is_suspended thing looks to me like a workaround of some sort that may not be needed if clients have proper suspend/resume implementations. Even without suspend/resume support in client drivers, the driver core should resume devices in the right order (I2C adapter before any of the clients), so I don't see any cases where the is_suspended logic would be useful. Thierry
On Mon, May 14, 2018 at 01:59:33PM +0200, Thierry Reding wrote: > On Mon, May 14, 2018 at 12:13:47AM +0300, Dmitry Osipenko wrote: > > Nothing prevents I2C clients to access I2C while Tegra's driver is being > > suspended, this results in -EBUSY error returned to the clients and that > > may have unfortunate consequences. In particular this causes problems > > for the TPS6586x MFD driver which emits hundreds of "failed to read > > interrupt status" error messages on resume from suspend. This happens if > > TPS6586X is used to wake system from suspend by the expired RTC alarm > > timer because TPS6586X is an I2C device driver and its IRQ handler reads > > the status register while Tegra's I2C driver is suspended, i.e. just after > > kernel enabled IRQ's during of resume-from-suspend process. > > > > Note that the removed tegra_i2c_resume() invoked tegra_i2c_init() which > > performs HW reset. That seems was also not entirely correct because moving > > tegra_i2c_resume to an earlier stage of resume-from-suspend process causes > > I2C transfer to fail in the case of TPS6586X. It is fine to remove the > > HW-reinitialization for now because it should be only needed in a case of > > using lowest power-mode during suspend, which upstream kernel doesn't > > support. > > > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > > Cc: <stable@vger.kernel.org> > > --- > > drivers/i2c/busses/i2c-tegra.c | 33 --------------------------------- > > 1 file changed, 33 deletions(-) > > Shardar, Laxman, any thoughts on this? The is_suspended thing looks to > me like a workaround of some sort that may not be needed if clients have > proper suspend/resume implementations. Even without suspend/resume > support in client drivers, the driver core should resume devices in the > right order (I2C adapter before any of the clients), so I don't see any > cases where the is_suspended logic would be useful. Thanks for this patch! This is closely related to a discussion we started recently: "I2C PM overhaul needed?" https://lkml.org/lkml/2018/5/4/329 And part of the outcome is that the I2C core should print a WARN if an I2C client tries to use I2C at suspend_noirq state or later. This is to remove logic like in this patch from all I2C host drivers and to make it more explicit that those I2C client drivers need their PM fixed.
On Monday 14 May 2018 05:29 PM, Thierry Reding wrote: > * PGP Signed by an unknown key > > On Mon, May 14, 2018 at 12:13:47AM +0300, Dmitry Osipenko wrote: >> Nothing prevents I2C clients to access I2C while Tegra's driver is being >> suspended, this results in -EBUSY error returned to the clients and that >> may have unfortunate consequences. In particular this causes problems >> for the TPS6586x MFD driver which emits hundreds of "failed to read >> interrupt status" error messages on resume from suspend. This happens if >> TPS6586X is used to wake system from suspend by the expired RTC alarm >> timer because TPS6586X is an I2C device driver and its IRQ handler reads >> the status register while Tegra's I2C driver is suspended, i.e. just after >> kernel enabled IRQ's during of resume-from-suspend process. >> >> Note that the removed tegra_i2c_resume() invoked tegra_i2c_init() which >> performs HW reset. That seems was also not entirely correct because moving >> tegra_i2c_resume to an earlier stage of resume-from-suspend process causes >> I2C transfer to fail in the case of TPS6586X. It is fine to remove the >> HW-reinitialization for now because it should be only needed in a case of >> using lowest power-mode during suspend, which upstream kernel doesn't >> support. >> >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >> Cc: <stable@vger.kernel.org> >> --- >> drivers/i2c/busses/i2c-tegra.c | 33 --------------------------------- >> 1 file changed, 33 deletions(-) > Shardar, Laxman, any thoughts on this? The is_suspended thing looks to > me like a workaround of some sort that may not be needed if clients have > proper suspend/resume implementations. Even without suspend/resume > support in client drivers, the driver core should resume devices in the > right order (I2C adapter before any of the clients), so I don't see any > cases where the is_suspended logic would be useful. > Our I2C driver is based on the interrupt. So we have converted the suspend/resume to suspend_noirq and reseume_noirq so that we will not allow the transfer when system interrupt disabled in downstream. SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(tegra_i2c_suspend, tegra_i2c_resume) In shutdown path, where interrupt disabled and still need i2c, we use the bit-bang method via GPIO for i2c transfer. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, May 14, 2018 at 05:51:58PM +0530, Laxman Dewangan wrote: > > > On Monday 14 May 2018 05:29 PM, Thierry Reding wrote: > > * PGP Signed by an unknown key > > > > On Mon, May 14, 2018 at 12:13:47AM +0300, Dmitry Osipenko wrote: > > > Nothing prevents I2C clients to access I2C while Tegra's driver is being > > > suspended, this results in -EBUSY error returned to the clients and that > > > may have unfortunate consequences. In particular this causes problems > > > for the TPS6586x MFD driver which emits hundreds of "failed to read > > > interrupt status" error messages on resume from suspend. This happens if > > > TPS6586X is used to wake system from suspend by the expired RTC alarm > > > timer because TPS6586X is an I2C device driver and its IRQ handler reads > > > the status register while Tegra's I2C driver is suspended, i.e. just after > > > kernel enabled IRQ's during of resume-from-suspend process. > > > > > > Note that the removed tegra_i2c_resume() invoked tegra_i2c_init() which > > > performs HW reset. That seems was also not entirely correct because moving > > > tegra_i2c_resume to an earlier stage of resume-from-suspend process causes > > > I2C transfer to fail in the case of TPS6586X. It is fine to remove the > > > HW-reinitialization for now because it should be only needed in a case of > > > using lowest power-mode during suspend, which upstream kernel doesn't > > > support. > > > > > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > > > Cc: <stable@vger.kernel.org> > > > --- > > > drivers/i2c/busses/i2c-tegra.c | 33 --------------------------------- > > > 1 file changed, 33 deletions(-) > > Shardar, Laxman, any thoughts on this? The is_suspended thing looks to > > me like a workaround of some sort that may not be needed if clients have > > proper suspend/resume implementations. Even without suspend/resume > > support in client drivers, the driver core should resume devices in the > > right order (I2C adapter before any of the clients), so I don't see any > > cases where the is_suspended logic would be useful. > > > > Our I2C driver is based on the interrupt. So we have converted the > suspend/resume to suspend_noirq and reseume_noirq so that we will not allow > the transfer when system interrupt disabled in downstream. > SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(tegra_i2c_suspend, tegra_i2c_resume) That seems to me like it shouldn't be necessary. If all clients are properly suspended, then when the I2C controller gets suspended there should be no pending transfers. That's provided that the driver model properly orders suspend of clients vs. their controller. I think it didn't use to do that (especially when deferred probing was involved) but I remember that getting fixed sometime in the last couple of years. > In shutdown path, where interrupt disabled and still need i2c, we use the > bit-bang method via GPIO for i2c transfer. Do we really need to bit-bang the GPIOs? Couldn't the I2C controller operate in a polling mode where only the interrupt was disabled but we still polled the status register to know when a transfer was finished? Thierry
On 14.05.2018 15:21, Laxman Dewangan wrote: > > > On Monday 14 May 2018 05:29 PM, Thierry Reding wrote: >> * PGP Signed by an unknown key >> >> On Mon, May 14, 2018 at 12:13:47AM +0300, Dmitry Osipenko wrote: >>> Nothing prevents I2C clients to access I2C while Tegra's driver is being >>> suspended, this results in -EBUSY error returned to the clients and that >>> may have unfortunate consequences. In particular this causes problems >>> for the TPS6586x MFD driver which emits hundreds of "failed to read >>> interrupt status" error messages on resume from suspend. This happens if >>> TPS6586X is used to wake system from suspend by the expired RTC alarm >>> timer because TPS6586X is an I2C device driver and its IRQ handler reads >>> the status register while Tegra's I2C driver is suspended, i.e. just after >>> kernel enabled IRQ's during of resume-from-suspend process. >>> >>> Note that the removed tegra_i2c_resume() invoked tegra_i2c_init() which >>> performs HW reset. That seems was also not entirely correct because moving >>> tegra_i2c_resume to an earlier stage of resume-from-suspend process causes >>> I2C transfer to fail in the case of TPS6586X. It is fine to remove the >>> HW-reinitialization for now because it should be only needed in a case of >>> using lowest power-mode during suspend, which upstream kernel doesn't >>> support. >>> >>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >>> Cc: <stable@vger.kernel.org> >>> --- >>> drivers/i2c/busses/i2c-tegra.c | 33 --------------------------------- >>> 1 file changed, 33 deletions(-) >> Shardar, Laxman, any thoughts on this? The is_suspended thing looks to >> me like a workaround of some sort that may not be needed if clients have >> proper suspend/resume implementations. Even without suspend/resume >> support in client drivers, the driver core should resume devices in the >> right order (I2C adapter before any of the clients), so I don't see any >> cases where the is_suspended logic would be useful. >> > > Our I2C driver is based on the interrupt. So we have converted the > suspend/resume to suspend_noirq and reseume_noirq so that we will not allow the > transfer when system interrupt disabled in downstream. > SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(tegra_i2c_suspend, tegra_i2c_resume) > > In shutdown path, where interrupt disabled and still need i2c, we use the > bit-bang method via GPIO for i2c transfer. In the current upstream kernel suspend/resume can't be simply moved to the 'noirq' stage because resume invokes tegra_i2c_init() which uses runtime PM and that doesn't work with the IRQ's being disabled. But things do not work even with the tegra_i2c_init() changed to work with the disabled IRQ's, like I wrote above the I2C transfer fails (due to timeout) and a "fix" for that failure was to remove reset_control_assert/deassert from the tegra_i2c_init(). So I'd go for a complete suspend/resume removal for now as it is causes problem. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 14.05.2018 15:18, Wolfram Sang wrote: > On Mon, May 14, 2018 at 01:59:33PM +0200, Thierry Reding wrote: >> On Mon, May 14, 2018 at 12:13:47AM +0300, Dmitry Osipenko wrote: >>> Nothing prevents I2C clients to access I2C while Tegra's driver is being >>> suspended, this results in -EBUSY error returned to the clients and that >>> may have unfortunate consequences. In particular this causes problems >>> for the TPS6586x MFD driver which emits hundreds of "failed to read >>> interrupt status" error messages on resume from suspend. This happens if >>> TPS6586X is used to wake system from suspend by the expired RTC alarm >>> timer because TPS6586X is an I2C device driver and its IRQ handler reads >>> the status register while Tegra's I2C driver is suspended, i.e. just after >>> kernel enabled IRQ's during of resume-from-suspend process. >>> >>> Note that the removed tegra_i2c_resume() invoked tegra_i2c_init() which >>> performs HW reset. That seems was also not entirely correct because moving >>> tegra_i2c_resume to an earlier stage of resume-from-suspend process causes >>> I2C transfer to fail in the case of TPS6586X. It is fine to remove the >>> HW-reinitialization for now because it should be only needed in a case of >>> using lowest power-mode during suspend, which upstream kernel doesn't >>> support. >>> >>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >>> Cc: <stable@vger.kernel.org> >>> --- >>> drivers/i2c/busses/i2c-tegra.c | 33 --------------------------------- >>> 1 file changed, 33 deletions(-) >> >> Shardar, Laxman, any thoughts on this? The is_suspended thing looks to >> me like a workaround of some sort that may not be needed if clients have >> proper suspend/resume implementations. Even without suspend/resume >> support in client drivers, the driver core should resume devices in the >> right order (I2C adapter before any of the clients), so I don't see any >> cases where the is_suspended logic would be useful. > > Thanks for this patch! > > This is closely related to a discussion we started recently: > > "I2C PM overhaul needed?" https://lkml.org/lkml/2018/5/4/329 > > And part of the outcome is that the I2C core should print a WARN if an > I2C client tries to use I2C at suspend_noirq state or later. This is to > remove logic like in this patch from all I2C host drivers and to make it > more explicit that those I2C client drivers need their PM fixed. > Thank you very much for pointing at the discussion. Indeed it should be nicer if I2C core handled the buggy client drivers and the rest of related suspend/resume issues instead of having each I2C BUS driver to do it on its own. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > Our I2C driver is based on the interrupt. So we have converted the > > suspend/resume to suspend_noirq and reseume_noirq so that we will not allow the > > transfer when system interrupt disabled in downstream. > > SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(tegra_i2c_suspend, tegra_i2c_resume) > > > > In shutdown path, where interrupt disabled and still need i2c, we use the > > bit-bang method via GPIO for i2c transfer. > In the current upstream kernel suspend/resume can't be simply moved to the > 'noirq' stage because resume invokes tegra_i2c_init() which uses runtime PM and > that doesn't work with the IRQ's being disabled. But things do not work even > with the tegra_i2c_init() changed to work with the disabled IRQ's, like I wrote > above the I2C transfer fails (due to timeout) and a "fix" for that failure was > to remove reset_control_assert/deassert from the tegra_i2c_init(). So I'd go for > a complete suspend/resume removal for now as it is causes problem. Laxman, are you convinced or do you have still objections?
On Tuesday 29 May 2018 11:36 PM, Wolfram Sang wrote: >>> Our I2C driver is based on the interrupt. So we have converted the >>> suspend/resume to suspend_noirq and reseume_noirq so that we will not allow the >>> transfer when system interrupt disabled in downstream. >>> SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(tegra_i2c_suspend, tegra_i2c_resume) >>> >>> In shutdown path, where interrupt disabled and still need i2c, we use the >>> bit-bang method via GPIO for i2c transfer. >> In the current upstream kernel suspend/resume can't be simply moved to the >> 'noirq' stage because resume invokes tegra_i2c_init() which uses runtime PM and >> that doesn't work with the IRQ's being disabled. But things do not work even >> with the tegra_i2c_init() changed to work with the disabled IRQ's, like I wrote >> above the I2C transfer fails (due to timeout) and a "fix" for that failure was >> to remove reset_control_assert/deassert from the tegra_i2c_init(). So I'd go for >> a complete suspend/resume removal for now as it is causes problem. > Laxman, are you convinced or do you have still objections? Fine with me. Please add my Ack Acked-by: Laxman Dewangan <ldewangan@nvidia.com> -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, May 14, 2018 at 12:13:47AM +0300, Dmitry Osipenko wrote: > Nothing prevents I2C clients to access I2C while Tegra's driver is being > suspended, this results in -EBUSY error returned to the clients and that > may have unfortunate consequences. In particular this causes problems > for the TPS6586x MFD driver which emits hundreds of "failed to read > interrupt status" error messages on resume from suspend. This happens if > TPS6586X is used to wake system from suspend by the expired RTC alarm > timer because TPS6586X is an I2C device driver and its IRQ handler reads > the status register while Tegra's I2C driver is suspended, i.e. just after > kernel enabled IRQ's during of resume-from-suspend process. > > Note that the removed tegra_i2c_resume() invoked tegra_i2c_init() which > performs HW reset. That seems was also not entirely correct because moving > tegra_i2c_resume to an earlier stage of resume-from-suspend process causes > I2C transfer to fail in the case of TPS6586X. It is fine to remove the > HW-reinitialization for now because it should be only needed in a case of > using lowest power-mode during suspend, which upstream kernel doesn't > support. > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > Cc: <stable@vger.kernel.org> Applied to for-next, thanks!
> Applied to for-next, thanks!
I removed the stable tag, though. I am not 100% sure if there are not
any side-effects for other users. If you still think it should go to
stable, please mention this patch to stable@ after it was applied to
linus tree. Maybe with a word or two about regression safety.
On 30.05.2018 23:25, Wolfram Sang wrote: > >> Applied to for-next, thanks! > > I removed the stable tag, though. I am not 100% sure if there are not > any side-effects for other users. If you still think it should go to > stable, please mention this patch to stable@ after it was applied to > linus tree. Maybe with a word or two about regression safety. > Backporting to stable isn't really necessary, so no objections from me. Thank you. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 13/05/2018 22:13, Dmitry Osipenko wrote: > Nothing prevents I2C clients to access I2C while Tegra's driver is being > suspended, this results in -EBUSY error returned to the clients and that > may have unfortunate consequences. In particular this causes problems > for the TPS6586x MFD driver which emits hundreds of "failed to read > interrupt status" error messages on resume from suspend. This happens if > TPS6586X is used to wake system from suspend by the expired RTC alarm > timer because TPS6586X is an I2C device driver and its IRQ handler reads > the status register while Tegra's I2C driver is suspended, i.e. just after > kernel enabled IRQ's during of resume-from-suspend process. I have been looking at the above issue with the tps6586x because I am seeing delays on resume caused by this driver on the stable branches. I understand that this patch was dropped for stable, but looking at the specific issue with the tps6586x I am curious why the tps6586x driver was not fixed because it appears to me that the issue largely resides with that driver and any other device that uses the tps6586x is susceptible to it. I was able to fix the tps6586x driver by doing the following and I am interested in your thoughts ... Subject: [PATCH] mfd: tps6586x: Handle interrupts on suspend The tps6586x device is registered as an irqchip and the tps6586x-rtc interrupt is one of it's interrupt sources. When using the tps6586x-rtc as a wake-up device from suspend, the following is seen: PM: Syncing filesystems ... done. Freezing user space processes ... (elapsed 0.001 seconds) done. OOM killer disabled. Freezing remaining freezable tasks ... (elapsed 0.000 seconds) done. Disabling non-boot CPUs ... Entering suspend state LP1 Enabling non-boot CPUs ... CPU1 is up tps6586x 3-0034: failed to read interrupt status tps6586x 3-0034: failed to read interrupt status The reason why the tps6586x interrupt status cannot be read is because the tps6586x interrupt is not masked during suspend and when the tps6586x-rtc interrupt occurs, to wake-up the device, the interrupt is seen before the i2c controller has been resumed in order to read the tps6586x interrupt status. The tps6586x-rtc driver sets it's interrupt as a wake-up source during suspend, which gets propagated to the parent tps6586x interrupt. However, the tps6586x-rtc driver cannot disable it's interrupt during suspend otherwise we would never be woken up and so the tps6586x must disable it's interrupt instead. Fix this by disabling the tps6586x interrupt on entering suspend and re-enabling it on resuming from suspend. --- drivers/mfd/tps6586x.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/drivers/mfd/tps6586x.c b/drivers/mfd/tps6586x.c index 5628a6b5b19b..165ac8a3d22c 100644 --- a/drivers/mfd/tps6586x.c +++ b/drivers/mfd/tps6586x.c @@ -594,6 +594,27 @@ static int tps6586x_i2c_remove(struct i2c_client *client) return 0; } +static int __maybe_unused tps6586x_i2c_suspend(struct device *dev) +{ + struct tps6586x *tps6586x = dev_get_drvdata(dev); + + disable_irq(tps6586x->client->irq); + + return 0; +} + +static int __maybe_unused tps6586x_i2c_resume(struct device *dev) +{ + struct tps6586x *tps6586x = dev_get_drvdata(dev); + + enable_irq(tps6586x->client->irq); + + return 0; +} + +static SIMPLE_DEV_PM_OPS(tps6586x_pm_ops, tps6586x_i2c_suspend, + tps6586x_i2c_resume); + static const struct i2c_device_id tps6586x_id_table[] = { { "tps6586x", 0 }, { }, @@ -604,6 +625,7 @@ static int tps6586x_i2c_remove(struct i2c_client *client) .driver = { .name = "tps6586x", .of_match_table = of_match_ptr(tps6586x_of_match), + .pm = &tps6586x_pm_ops, }, .probe = tps6586x_i2c_probe, .remove = tps6586x_i2c_remove,
On 10/17/18 4:59 PM, Jon Hunter wrote: > > On 13/05/2018 22:13, Dmitry Osipenko wrote: >> Nothing prevents I2C clients to access I2C while Tegra's driver is being >> suspended, this results in -EBUSY error returned to the clients and that >> may have unfortunate consequences. In particular this causes problems >> for the TPS6586x MFD driver which emits hundreds of "failed to read >> interrupt status" error messages on resume from suspend. This happens if >> TPS6586X is used to wake system from suspend by the expired RTC alarm >> timer because TPS6586X is an I2C device driver and its IRQ handler reads >> the status register while Tegra's I2C driver is suspended, i.e. just after >> kernel enabled IRQ's during of resume-from-suspend process. > > I have been looking at the above issue with the tps6586x because I am > seeing delays on resume caused by this driver on the stable branches. I > understand that this patch was dropped for stable, but looking at the > specific issue with the tps6586x I am curious why the tps6586x driver > was not fixed because it appears to me that the issue largely resides > with that driver and any other device that uses the tps6586x is > susceptible to it. I was able to fix the tps6586x driver by doing the > following and I am interested in your thoughts ... > > Subject: [PATCH] mfd: tps6586x: Handle interrupts on suspend > > The tps6586x device is registered as an irqchip and the tps6586x-rtc > interrupt is one of it's interrupt sources. When using the tps6586x-rtc > as a wake-up device from suspend, the following is seen: > > PM: Syncing filesystems ... done. > Freezing user space processes ... (elapsed 0.001 seconds) done. > OOM killer disabled. > Freezing remaining freezable tasks ... (elapsed 0.000 seconds) done. > Disabling non-boot CPUs ... > Entering suspend state LP1 > Enabling non-boot CPUs ... > CPU1 is up > tps6586x 3-0034: failed to read interrupt status > tps6586x 3-0034: failed to read interrupt status > > The reason why the tps6586x interrupt status cannot be read is because > the tps6586x interrupt is not masked during suspend and when the > tps6586x-rtc interrupt occurs, to wake-up the device, the interrupt is > seen before the i2c controller has been resumed in order to read the > tps6586x interrupt status. > > The tps6586x-rtc driver sets it's interrupt as a wake-up source during > suspend, which gets propagated to the parent tps6586x interrupt. > However, the tps6586x-rtc driver cannot disable it's interrupt during > suspend otherwise we would never be woken up and so the tps6586x must > disable it's interrupt instead. > > Fix this by disabling the tps6586x interrupt on entering suspend and > re-enabling it on resuming from suspend. Looks like it should work, but I vaguely recalling that something didn't work after disabling of IRQ on suspend. Maybe wakeup was getting disabled, but seems it is working fine now. Patch is good to me if you're going to propose it for backporting, but you should test that it works properly with all of stable kernels. I just found this [0], seems your patch need to address the same review comment. [0] https://lkml.org/lkml/2011/3/29/18
On 17/10/2018 15:30, Dmitry Osipenko wrote: > On 10/17/18 4:59 PM, Jon Hunter wrote: >> >> On 13/05/2018 22:13, Dmitry Osipenko wrote: >>> Nothing prevents I2C clients to access I2C while Tegra's driver is being >>> suspended, this results in -EBUSY error returned to the clients and that >>> may have unfortunate consequences. In particular this causes problems >>> for the TPS6586x MFD driver which emits hundreds of "failed to read >>> interrupt status" error messages on resume from suspend. This happens if >>> TPS6586X is used to wake system from suspend by the expired RTC alarm >>> timer because TPS6586X is an I2C device driver and its IRQ handler reads >>> the status register while Tegra's I2C driver is suspended, i.e. just after >>> kernel enabled IRQ's during of resume-from-suspend process. >> >> I have been looking at the above issue with the tps6586x because I am >> seeing delays on resume caused by this driver on the stable branches. I >> understand that this patch was dropped for stable, but looking at the >> specific issue with the tps6586x I am curious why the tps6586x driver >> was not fixed because it appears to me that the issue largely resides >> with that driver and any other device that uses the tps6586x is >> susceptible to it. I was able to fix the tps6586x driver by doing the >> following and I am interested in your thoughts ... >> >> Subject: [PATCH] mfd: tps6586x: Handle interrupts on suspend >> >> The tps6586x device is registered as an irqchip and the tps6586x-rtc >> interrupt is one of it's interrupt sources. When using the tps6586x-rtc >> as a wake-up device from suspend, the following is seen: >> >> PM: Syncing filesystems ... done. >> Freezing user space processes ... (elapsed 0.001 seconds) done. >> OOM killer disabled. >> Freezing remaining freezable tasks ... (elapsed 0.000 seconds) done. >> Disabling non-boot CPUs ... >> Entering suspend state LP1 >> Enabling non-boot CPUs ... >> CPU1 is up >> tps6586x 3-0034: failed to read interrupt status >> tps6586x 3-0034: failed to read interrupt status >> >> The reason why the tps6586x interrupt status cannot be read is because >> the tps6586x interrupt is not masked during suspend and when the >> tps6586x-rtc interrupt occurs, to wake-up the device, the interrupt is >> seen before the i2c controller has been resumed in order to read the >> tps6586x interrupt status. >> >> The tps6586x-rtc driver sets it's interrupt as a wake-up source during >> suspend, which gets propagated to the parent tps6586x interrupt. >> However, the tps6586x-rtc driver cannot disable it's interrupt during >> suspend otherwise we would never be woken up and so the tps6586x must >> disable it's interrupt instead. >> >> Fix this by disabling the tps6586x interrupt on entering suspend and >> re-enabling it on resuming from suspend. > > Looks like it should work, but I vaguely recalling that something didn't work after disabling of IRQ on suspend. Maybe wakeup was getting disabled, but seems it is working fine now. Patch is good to me if you're going to propose it for backporting, but you should test that it works properly with all of stable kernels. Indeed, I have been setting up some automated testing of various stable branches (mainly 4.x LTS releases) and I am seeing this problem there. Furthermore, I am using this to validate the change as well. > I just found this [0], seems your patch need to address the same review comment. > > [0] https://lkml.org/lkml/2011/3/29/18 Thanks will do. I know we don't support low power modes (ie. LP0), however, I do wonder if we should have some sort of i2c suspend/resume handler for Tegra? Eventually we will need this. Cheers Jon
On 10/17/18 10:41 PM, Jon Hunter wrote: > > On 17/10/2018 15:30, Dmitry Osipenko wrote: >> On 10/17/18 4:59 PM, Jon Hunter wrote: >>> >>> On 13/05/2018 22:13, Dmitry Osipenko wrote: >>>> Nothing prevents I2C clients to access I2C while Tegra's driver is being >>>> suspended, this results in -EBUSY error returned to the clients and that >>>> may have unfortunate consequences. In particular this causes problems >>>> for the TPS6586x MFD driver which emits hundreds of "failed to read >>>> interrupt status" error messages on resume from suspend. This happens if >>>> TPS6586X is used to wake system from suspend by the expired RTC alarm >>>> timer because TPS6586X is an I2C device driver and its IRQ handler reads >>>> the status register while Tegra's I2C driver is suspended, i.e. just after >>>> kernel enabled IRQ's during of resume-from-suspend process. >>> >>> I have been looking at the above issue with the tps6586x because I am >>> seeing delays on resume caused by this driver on the stable branches. I >>> understand that this patch was dropped for stable, but looking at the >>> specific issue with the tps6586x I am curious why the tps6586x driver >>> was not fixed because it appears to me that the issue largely resides >>> with that driver and any other device that uses the tps6586x is >>> susceptible to it. I was able to fix the tps6586x driver by doing the >>> following and I am interested in your thoughts ... >>> >>> Subject: [PATCH] mfd: tps6586x: Handle interrupts on suspend >>> >>> The tps6586x device is registered as an irqchip and the tps6586x-rtc >>> interrupt is one of it's interrupt sources. When using the tps6586x-rtc >>> as a wake-up device from suspend, the following is seen: >>> >>> PM: Syncing filesystems ... done. >>> Freezing user space processes ... (elapsed 0.001 seconds) done. >>> OOM killer disabled. >>> Freezing remaining freezable tasks ... (elapsed 0.000 seconds) done. >>> Disabling non-boot CPUs ... >>> Entering suspend state LP1 >>> Enabling non-boot CPUs ... >>> CPU1 is up >>> tps6586x 3-0034: failed to read interrupt status >>> tps6586x 3-0034: failed to read interrupt status >>> >>> The reason why the tps6586x interrupt status cannot be read is because >>> the tps6586x interrupt is not masked during suspend and when the >>> tps6586x-rtc interrupt occurs, to wake-up the device, the interrupt is >>> seen before the i2c controller has been resumed in order to read the >>> tps6586x interrupt status. >>> >>> The tps6586x-rtc driver sets it's interrupt as a wake-up source during >>> suspend, which gets propagated to the parent tps6586x interrupt. >>> However, the tps6586x-rtc driver cannot disable it's interrupt during >>> suspend otherwise we would never be woken up and so the tps6586x must >>> disable it's interrupt instead. >>> >>> Fix this by disabling the tps6586x interrupt on entering suspend and >>> re-enabling it on resuming from suspend. >> >> Looks like it should work, but I vaguely recalling that something didn't work after disabling of IRQ on suspend. Maybe wakeup was getting disabled, but seems it is working fine now. Patch is good to me if you're going to propose it for backporting, but you should test that it works properly with all of stable kernels. > > Indeed, I have been setting up some automated testing of various stable > branches (mainly 4.x LTS releases) and I am seeing this problem there. > Furthermore, I am using this to validate the change as well. > >> I just found this [0], seems your patch need to address the same review comment. >> >> [0] https://lkml.org/lkml/2011/3/29/18 > > Thanks will do. > > I know we don't support low power modes (ie. LP0), however, I do wonder > if we should have some sort of i2c suspend/resume handler for Tegra? > Eventually we will need this. I'd suggest to support LP0 in the core first and only then to start making necessary suspend/resume changes in the drivers, otherwise it may end up being wasted time and effort for you and other maintainers.
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c index 60292d243e24..5fccd1f1bca8 100644 --- a/drivers/i2c/busses/i2c-tegra.c +++ b/drivers/i2c/busses/i2c-tegra.c @@ -173,7 +173,6 @@ struct tegra_i2c_hw_feature { * @msg_buf_remaining: size of unsent data in the message buffer * @msg_read: identifies read transfers * @bus_clk_rate: current i2c bus clock rate - * @is_suspended: prevents i2c controller accesses after suspend is called */ struct tegra_i2c_dev { struct device *dev; @@ -194,7 +193,6 @@ struct tegra_i2c_dev { int msg_read; u32 bus_clk_rate; u16 clk_divisor_non_hs_mode; - bool is_suspended; bool is_multimaster_mode; spinlock_t xfer_lock; }; @@ -734,9 +732,6 @@ static int tegra_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int i; int ret = 0; - if (i2c_dev->is_suspended) - return -EBUSY; - ret = pm_runtime_get_sync(i2c_dev->dev); if (ret < 0) { dev_err(i2c_dev->dev, "runtime resume failed %d\n", ret); @@ -1051,37 +1046,9 @@ static int tegra_i2c_remove(struct platform_device *pdev) } #ifdef CONFIG_PM_SLEEP -static int tegra_i2c_suspend(struct device *dev) -{ - struct tegra_i2c_dev *i2c_dev = dev_get_drvdata(dev); - - i2c_lock_adapter(&i2c_dev->adapter); - i2c_dev->is_suspended = true; - i2c_unlock_adapter(&i2c_dev->adapter); - - return 0; -} - -static int tegra_i2c_resume(struct device *dev) -{ - struct tegra_i2c_dev *i2c_dev = dev_get_drvdata(dev); - int ret; - - i2c_lock_adapter(&i2c_dev->adapter); - - ret = tegra_i2c_init(i2c_dev); - if (!ret) - i2c_dev->is_suspended = false; - - i2c_unlock_adapter(&i2c_dev->adapter); - - return ret; -} - static const struct dev_pm_ops tegra_i2c_pm = { SET_RUNTIME_PM_OPS(tegra_i2c_runtime_suspend, tegra_i2c_runtime_resume, NULL) - SET_SYSTEM_SLEEP_PM_OPS(tegra_i2c_suspend, tegra_i2c_resume) }; #define TEGRA_I2C_PM (&tegra_i2c_pm) #else
Nothing prevents I2C clients to access I2C while Tegra's driver is being suspended, this results in -EBUSY error returned to the clients and that may have unfortunate consequences. In particular this causes problems for the TPS6586x MFD driver which emits hundreds of "failed to read interrupt status" error messages on resume from suspend. This happens if TPS6586X is used to wake system from suspend by the expired RTC alarm timer because TPS6586X is an I2C device driver and its IRQ handler reads the status register while Tegra's I2C driver is suspended, i.e. just after kernel enabled IRQ's during of resume-from-suspend process. Note that the removed tegra_i2c_resume() invoked tegra_i2c_init() which performs HW reset. That seems was also not entirely correct because moving tegra_i2c_resume to an earlier stage of resume-from-suspend process causes I2C transfer to fail in the case of TPS6586X. It is fine to remove the HW-reinitialization for now because it should be only needed in a case of using lowest power-mode during suspend, which upstream kernel doesn't support. Signed-off-by: Dmitry Osipenko <digetx@gmail.com> Cc: <stable@vger.kernel.org> --- drivers/i2c/busses/i2c-tegra.c | 33 --------------------------------- 1 file changed, 33 deletions(-)