[v1] i2c: tegra: Remove suspend-resume

Message ID 20180513211347.7187-1-digetx@gmail.com
State New
Headers show
Series
  • [v1] i2c: tegra: Remove suspend-resume
Related show

Commit Message

Dmitry Osipenko May 13, 2018, 9:13 p.m.
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(-)

Comments

Thierry Reding May 14, 2018, 11:59 a.m. | #1
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
Wolfram Sang May 14, 2018, 12:18 p.m. | #2
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.
Laxman Dewangan May 14, 2018, 12:21 p.m. | #3
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
Thierry Reding May 14, 2018, 12:47 p.m. | #4
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
Dmitry Osipenko May 14, 2018, 12:51 p.m. | #5
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
Dmitry Osipenko May 14, 2018, 1:03 p.m. | #6
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

Patch

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