[PATCH v1] i2c: tegra: Remove suspend-resume

[PATCH v1] i2c: tegra: Remove suspend-resume

[Dmitry Osipenko]

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(-)

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
-- 
2.17.0

Re: [PATCH v1] i2c: tegra: Remove suspend-resume

[Thierry Reding]

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

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

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

Re: [PATCH v1] i2c: tegra: Remove suspend-resume

[Wolfram Sang]

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

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.


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

Re: [PATCH v1] i2c: tegra: Remove suspend-resume

[Dmitry Osipenko]

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.

Re: [PATCH v1] i2c: tegra: Remove suspend-resume

[Laxman Dewangan]

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.

Re: [PATCH v1] i2c: tegra: Remove suspend-resume

[Thierry Reding]

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

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

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

Re: [PATCH v1] i2c: tegra: Remove suspend-resume

[Dmitry Osipenko]

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.

Re: [PATCH v1] i2c: tegra: Remove suspend-resume

[Wolfram Sang]

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


> > 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?


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

Re: [PATCH v1] i2c: tegra: Remove suspend-resume

[Laxman Dewangan]

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>

Re: [PATCH v1] i2c: tegra: Remove suspend-resume

[Wolfram Sang]

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

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!


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

Re: [PATCH v1] i2c: tegra: Remove suspend-resume

[Wolfram Sang]

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


> 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.


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

Re: [PATCH v1] i2c: tegra: Remove suspend-resume

[Dmitry Osipenko]

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.

Re: [PATCH v1] i2c: tegra: Remove suspend-resume

[Jon Hunter]

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,
-- 
1.9.1

-- 
nvpublic

Re: [PATCH v1] i2c: tegra: Remove suspend-resume

[Dmitry Osipenko]

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

Re: [PATCH v1] i2c: tegra: Remove suspend-resume

[Jon Hunter]

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

-- 
nvpublic

Re: [PATCH v1] i2c: tegra: Remove suspend-resume

[Dmitry Osipenko]

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.