[PATCH 1/2] i2c: sprd: Prevent i2c accesses after suspend is called

[PATCH 1/2] i2c: sprd: Prevent i2c accesses after suspend is called

[Baolin Wang]

Add one flag to indicate if the i2c controller has been in suspend state,
which can prevent i2c accesses after i2c controller is suspended following
system suspend.

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
 drivers/i2c/busses/i2c-sprd.c |   16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/i2c/busses/i2c-sprd.c b/drivers/i2c/busses/i2c-sprd.c
index 25fcc3c..2fdad63 100644
--- a/drivers/i2c/busses/i2c-sprd.c
+++ b/drivers/i2c/busses/i2c-sprd.c
@@ -86,6 +86,7 @@ struct sprd_i2c {
 	u32 count;
 	int irq;
 	int err;
+	bool is_suspended;
 };
 
 static void sprd_i2c_set_count(struct sprd_i2c *i2c_dev, u32 count)
@@ -283,6 +284,9 @@ static int sprd_i2c_master_xfer(struct i2c_adapter *i2c_adap,
 	struct sprd_i2c *i2c_dev = i2c_adap->algo_data;
 	int im, ret;
 
+	if (i2c_dev->is_suspended)
+		return -EBUSY;
+
 	ret = pm_runtime_get_sync(i2c_dev->dev);
 	if (ret < 0)
 		return ret;
@@ -586,11 +590,23 @@ static int sprd_i2c_remove(struct platform_device *pdev)
 
 static int __maybe_unused sprd_i2c_suspend_noirq(struct device *pdev)
 {
+	struct sprd_i2c *i2c_dev = dev_get_drvdata(pdev);
+
+	i2c_lock_adapter(&i2c_dev->adap);
+	i2c_dev->is_suspended = true;
+	i2c_unlock_adapter(&i2c_dev->adap);
+
 	return pm_runtime_force_suspend(pdev);
 }
 
 static int __maybe_unused sprd_i2c_resume_noirq(struct device *pdev)
 {
+	struct sprd_i2c *i2c_dev = dev_get_drvdata(pdev);
+
+	i2c_lock_adapter(&i2c_dev->adap);
+	i2c_dev->is_suspended = false;
+	i2c_unlock_adapter(&i2c_dev->adap);
+
 	return pm_runtime_force_resume(pdev);
 }
 
-- 
1.7.9.5

[PATCH 2/2] i2c: sprd: Fix the i2c count issue

[Baolin Wang]

We found the I2C controller count register is unreliable sometimes,
that will cause I2C to lose data. Thus we can read the data count
from 'i2c_dev->count' instead of the I2C controller count register.

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
 drivers/i2c/busses/i2c-sprd.c |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-sprd.c b/drivers/i2c/busses/i2c-sprd.c
index 2fdad63..4053259 100644
--- a/drivers/i2c/busses/i2c-sprd.c
+++ b/drivers/i2c/busses/i2c-sprd.c
@@ -368,13 +368,12 @@ static irqreturn_t sprd_i2c_isr_thread(int irq, void *dev_id)
 	struct sprd_i2c *i2c_dev = dev_id;
 	struct i2c_msg *msg = i2c_dev->msg;
 	bool ack = !(readl(i2c_dev->base + I2C_STATUS) & I2C_RX_ACK);
-	u32 i2c_count = readl(i2c_dev->base + I2C_COUNT);
 	u32 i2c_tran;
 
 	if (msg->flags & I2C_M_RD)
 		i2c_tran = i2c_dev->count >= I2C_FIFO_FULL_THLD;
 	else
-		i2c_tran = i2c_count;
+		i2c_tran = i2c_dev->count;
 
 	/*
 	 * If we got one ACK from slave when writing data, and we did not
@@ -412,14 +411,13 @@ static irqreturn_t sprd_i2c_isr(int irq, void *dev_id)
 {
 	struct sprd_i2c *i2c_dev = dev_id;
 	struct i2c_msg *msg = i2c_dev->msg;
-	u32 i2c_count = readl(i2c_dev->base + I2C_COUNT);
 	bool ack = !(readl(i2c_dev->base + I2C_STATUS) & I2C_RX_ACK);
 	u32 i2c_tran;
 
 	if (msg->flags & I2C_M_RD)
 		i2c_tran = i2c_dev->count >= I2C_FIFO_FULL_THLD;
 	else
-		i2c_tran = i2c_count;
+		i2c_tran = i2c_dev->count;
 
 	/*
 	 * If we did not get one ACK from slave when writing data, then we
-- 
1.7.9.5

Re: [PATCH 2/2] i2c: sprd: Fix the i2c count issue

[Wolfram Sang]

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

On Mon, Apr 09, 2018 at 02:40:55PM +0800, Baolin Wang wrote:
> We found the I2C controller count register is unreliable sometimes,
> that will cause I2C to lose data. Thus we can read the data count
> from 'i2c_dev->count' instead of the I2C controller count register.
> 
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>

Applied to for-current, thanks!


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

Re: [PATCH 1/2] i2c: sprd: Prevent i2c accesses after suspend is called

[Grygorii Strashko]

On 04/09/2018 01:40 AM, Baolin Wang wrote:
> Add one flag to indicate if the i2c controller has been in suspend state,
> which can prevent i2c accesses after i2c controller is suspended following
> system suspend.

This usually indicates some bigger problem - there should be no i2c access to
 the I2C driver once it's suspended. But if happens - 
it means suspend dependencies between drivers are broken or there some 
scheduling primitives are not disabled properly.
(possible sources - kthreads, delayed works, timers or even threaded irqs)

> 
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> ---
>   drivers/i2c/busses/i2c-sprd.c |   16 ++++++++++++++++
>   1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-sprd.c b/drivers/i2c/busses/i2c-sprd.c
> index 25fcc3c..2fdad63 100644
> --- a/drivers/i2c/busses/i2c-sprd.c
> +++ b/drivers/i2c/busses/i2c-sprd.c
> @@ -86,6 +86,7 @@ struct sprd_i2c {
>   	u32 count;
>   	int irq;
>   	int err;
> +	bool is_suspended;
>   };
>   
>   static void sprd_i2c_set_count(struct sprd_i2c *i2c_dev, u32 count)
> @@ -283,6 +284,9 @@ static int sprd_i2c_master_xfer(struct i2c_adapter *i2c_adap,
>   	struct sprd_i2c *i2c_dev = i2c_adap->algo_data;
>   	int im, ret;
>   
> +	if (i2c_dev->is_suspended)
> +		return -EBUSY;
> +
>   	ret = pm_runtime_get_sync(i2c_dev->dev);
>   	if (ret < 0)
>   		return ret;
> @@ -586,11 +590,23 @@ static int sprd_i2c_remove(struct platform_device *pdev)
>   
>   static int __maybe_unused sprd_i2c_suspend_noirq(struct device *pdev)
>   {
> +	struct sprd_i2c *i2c_dev = dev_get_drvdata(pdev);
> +
> +	i2c_lock_adapter(&i2c_dev->adap);
> +	i2c_dev->is_suspended = true;
> +	i2c_unlock_adapter(&i2c_dev->adap);
> +
>   	return pm_runtime_force_suspend(pdev);
>   }
>   
>   static int __maybe_unused sprd_i2c_resume_noirq(struct device *pdev)
>   {
> +	struct sprd_i2c *i2c_dev = dev_get_drvdata(pdev);
> +
> +	i2c_lock_adapter(&i2c_dev->adap);
> +	i2c_dev->is_suspended = false;
> +	i2c_unlock_adapter(&i2c_dev->adap);
> +
>   	return pm_runtime_force_resume(pdev);
>   }
>   
> 

-- 
regards,
-grygorii

Re: [PATCH 1/2] i2c: sprd: Prevent i2c accesses after suspend is called

[Grygorii Strashko]

On 04/09/2018 01:40 AM, Baolin Wang wrote:
> Add one flag to indicate if the i2c controller has been in suspend state,
> which can prevent i2c accesses after i2c controller is suspended following
> system suspend.

This usually indicates some bigger problem - there should be no i2c access to
 the I2C driver once it's suspended. But if happens - 
it means suspend dependencies between drivers are broken or there some 
scheduling primitives are not disabled properly.
(possible sources - kthreads, delayed works, timers or even threaded irqs)

> 
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> ---
>   drivers/i2c/busses/i2c-sprd.c |   16 ++++++++++++++++
>   1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-sprd.c b/drivers/i2c/busses/i2c-sprd.c
> index 25fcc3c..2fdad63 100644
> --- a/drivers/i2c/busses/i2c-sprd.c
> +++ b/drivers/i2c/busses/i2c-sprd.c
> @@ -86,6 +86,7 @@ struct sprd_i2c {
>   	u32 count;
>   	int irq;
>   	int err;
> +	bool is_suspended;
>   };
>   
>   static void sprd_i2c_set_count(struct sprd_i2c *i2c_dev, u32 count)
> @@ -283,6 +284,9 @@ static int sprd_i2c_master_xfer(struct i2c_adapter *i2c_adap,
>   	struct sprd_i2c *i2c_dev = i2c_adap->algo_data;
>   	int im, ret;
>   
> +	if (i2c_dev->is_suspended)
> +		return -EBUSY;
> +
>   	ret = pm_runtime_get_sync(i2c_dev->dev);
>   	if (ret < 0)
>   		return ret;
> @@ -586,11 +590,23 @@ static int sprd_i2c_remove(struct platform_device *pdev)
>   
>   static int __maybe_unused sprd_i2c_suspend_noirq(struct device *pdev)
>   {
> +	struct sprd_i2c *i2c_dev = dev_get_drvdata(pdev);
> +
> +	i2c_lock_adapter(&i2c_dev->adap);
> +	i2c_dev->is_suspended = true;
> +	i2c_unlock_adapter(&i2c_dev->adap);
> +
>   	return pm_runtime_force_suspend(pdev);
>   }
>   
>   static int __maybe_unused sprd_i2c_resume_noirq(struct device *pdev)
>   {
> +	struct sprd_i2c *i2c_dev = dev_get_drvdata(pdev);
> +
> +	i2c_lock_adapter(&i2c_dev->adap);
> +	i2c_dev->is_suspended = false;
> +	i2c_unlock_adapter(&i2c_dev->adap);
> +
>   	return pm_runtime_force_resume(pdev);
>   }
>   
> 

-- 
regards,
-grygorii

Re: [PATCH 1/2] i2c: sprd: Prevent i2c accesses after suspend is called

[Baolin Wang]

Hi Grygorii,

On 10 April 2018 at 04:56, Grygorii Strashko <grygorii.strashko@ti.com> wrote:
>
>
> On 04/09/2018 01:40 AM, Baolin Wang wrote:
>> Add one flag to indicate if the i2c controller has been in suspend state,
>> which can prevent i2c accesses after i2c controller is suspended following
>> system suspend.
>
> This usually indicates some bigger problem - there should be no i2c access to
>  the I2C driver once it's suspended. But if happens -
> it means suspend dependencies between drivers are broken or there some
> scheduling primitives are not disabled properly.

Correct. But on Spreadtrum platform there are some I2C slave devices
(like some sensors), they do not care the system suspend, and we can
not control them. So we can add this flag to make sure I2C driver can
be accessed safely no matter considering other slaves's dependency
like other drivers did (i2c-tegra.c, i2c-brcmstb.c and i2c-zx2967.c).
Thanks for your comments.

-- 
Baolin.wang
Best Regards

Re: [PATCH 1/2] i2c: sprd: Prevent i2c accesses after suspend is called

[Wolfram Sang]

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

On Mon, Apr 09, 2018 at 02:40:54PM +0800, Baolin Wang wrote:
> Add one flag to indicate if the i2c controller has been in suspend state,
> which can prevent i2c accesses after i2c controller is suspended following
> system suspend.
> 
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>

Applied to for-current, thanks!

We should maybe handle this in the core somewhen, though. Or?


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

Re: [PATCH 1/2] i2c: sprd: Prevent i2c accesses after suspend is called

[Baolin Wang]

Hi Wolfram,

On 27 April 2018 at 20:14, Wolfram Sang <wsa@the-dreams.de> wrote:
> On Mon, Apr 09, 2018 at 02:40:54PM +0800, Baolin Wang wrote:
>> Add one flag to indicate if the i2c controller has been in suspend state,
>> which can prevent i2c accesses after i2c controller is suspended following
>> system suspend.
>>
>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>
> Applied to for-current, thanks!
>
> We should maybe handle this in the core somewhen, though. Or?

Thanks. Yes, It will more helpful if we can handle this in the i2c core.

-- 
Baolin.wang
Best Regards

Re: [PATCH 1/2] i2c: sprd: Prevent i2c accesses after suspend is called

[Wolfram Sang]

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


> > We should maybe handle this in the core somewhen, though. Or?
> 
> Thanks. Yes, It will more helpful if we can handle this in the i2c core.

To understand the issue better: which kind of devices in your system
still send I2C transfers when the system is going to suspend? Do you
know?


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

Re: [PATCH 1/2] i2c: sprd: Prevent i2c accesses after suspend is called

[Baolin Wang]

On 2 May 2018 at 13:23, Wolfram Sang <wsa@the-dreams.de> wrote:
>
>> > We should maybe handle this in the core somewhen, though. Or?
>>
>> Thanks. Yes, It will more helpful if we can handle this in the i2c core.
>
> To understand the issue better: which kind of devices in your system
> still send I2C transfers when the system is going to suspend? Do you
> know?

Now we found the touch screen device will trigger I2C transfers when
the system is going to suspend.

-- 
Baolin.wang
Best Regards

Re: [PATCH 1/2] i2c: sprd: Prevent i2c accesses after suspend is called

[Grygorii Strashko]

On 05/02/2018 12:48 AM, Baolin Wang wrote:
> On 2 May 2018 at 13:23, Wolfram Sang <wsa@the-dreams.de> wrote:
>>
>>>> We should maybe handle this in the core somewhen, though. Or?
>>>
>>> Thanks. Yes, It will more helpful if we can handle this in the i2c core.
>>
>> To understand the issue better: which kind of devices in your system
>> still send I2C transfers when the system is going to suspend? Do you
>> know?
> 
> Now we found the touch screen device will trigger I2C transfers when
> the system is going to suspend.
> 

And you have to fix it (touch screen) - not your i2c driver. Otherwise, you can get
situation when set of I2C transfers (executed from some kthread/work/threaded_irq/..)
will be just interrupted in the middle - usual behavior after this is (I2C timeout) [and/or
not-functional I2C client device [and/or I2C bus stuck (worst case)].

In case, somebody is trying to access I2C after .suspend_noirq() stage I2C bus driver 
should produce big fat warning and, most probably, abort suspend.
Above, in general, can be part of I2C core functionality.

-- 
regards,
-grygorii

Re: [PATCH 1/2] i2c: sprd: Prevent i2c accesses after suspend is called

[Grygorii Strashko]

On 05/02/2018 12:48 AM, Baolin Wang wrote:
> On 2 May 2018 at 13:23, Wolfram Sang <wsa@the-dreams.de> wrote:
>>
>>>> We should maybe handle this in the core somewhen, though. Or?
>>>
>>> Thanks. Yes, It will more helpful if we can handle this in the i2c core.
>>
>> To understand the issue better: which kind of devices in your system
>> still send I2C transfers when the system is going to suspend? Do you
>> know?
> 
> Now we found the touch screen device will trigger I2C transfers when
> the system is going to suspend.
> 

And you have to fix it (touch screen) - not your i2c driver. Otherwise, you can get
situation when set of I2C transfers (executed from some kthread/work/threaded_irq/..)
will be just interrupted in the middle - usual behavior after this is (I2C timeout) [and/or
not-functional I2C client device [and/or I2C bus stuck (worst case)].

In case, somebody is trying to access I2C after .suspend_noirq() stage I2C bus driver 
should produce big fat warning and, most probably, abort suspend.
Above, in general, can be part of I2C core functionality.

-- 
regards,
-grygorii

I2C PM overhaul needed? (Re: [PATCH 1/2] i2c: sprd: Prevent i2c accesses after suspend is called)

[Wolfram Sang]

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

Hi Grygorii,

thanks for stepping in.  I kept thinking about better I2C core support
for such situations and the more input the better.

> And you have to fix it (touch screen) - not your i2c driver. Otherwise, you can get
> situation when set of I2C transfers (executed from some kthread/work/threaded_irq/..)
> will be just interrupted in the middle - usual behavior after this is (I2C timeout) [and/or
> not-functional I2C client device [and/or I2C bus stuck (worst case)].

This. I also tend to think that most issues need to be fixed in the
client drivers ensuring proper states of client devices when suspending
/ resuming.. I wonder, though, if the core shouldn't assist by
guaranteeing that an on-going transfer has finished before suspending?
Or more technically, wait for a locked adapter to become unlocked again?

I still need to set it up and test, yet seeing that the EEPROM driver
at24.c has no suspend/resume callbacks, I'd assume a big write operation
will only be partially done when interrupted by a suspend.

> In case, somebody is trying to access I2C after .suspend_noirq() stage I2C bus driver 
> should produce big fat warning and, most probably, abort suspend.
> Above, in general, can be part of I2C core functionality.

Also this. However, there might be an exception of devices like PMICs
which may need to be accessed to trigger the final suspend state.

I at least know of some Renesas boards which needed the I2C connected
PMIC to do a system reset (not sure about suspend, need to recheck
that). That still today causes problems because interrupts are disabled
then.

To handle that, I imagined an additional adapter callback like
'master_xfer_irqless' to be used for such special I2C messages. These
kind of special messages could be tagged with a new I2C_M_something
flag.

And maybe this could be used here, too? Introduce this flag for very
late/early messages. If they have it, messages are even sent in
suspend_noirq() phase with the master_xfer_irqless() callback, otherwise
we will have the WARNing printed out.

Thoughts? Any other cases missed so far?

Kind regards,

   Wolfram


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

Re: I2C PM overhaul needed? (Re: [PATCH 1/2] i2c: sprd: Prevent i2c accesses after suspend is called)

[Mark Brown]

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

On Fri, May 04, 2018 at 02:24:47PM +0200, Wolfram Sang wrote:

> To handle that, I imagined an additional adapter callback like
> 'master_xfer_irqless' to be used for such special I2C messages. These
> kind of special messages could be tagged with a new I2C_M_something
> flag.

> And maybe this could be used here, too? Introduce this flag for very
> late/early messages. If they have it, messages are even sent in
> suspend_noirq() phase with the master_xfer_irqless() callback, otherwise
> we will have the WARNing printed out.

It feels like it'd be more elegant to have the core select the irqless
function automatically if called after interrupts have been disabled -
otherwise we end up with the need to special case through other layers
of the stack like regmap as well which seems like it'd be error prone.
OTOH it does mean we might not notice things happening later than they
should so it's not 100% clear...

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

Re: I2C PM overhaul needed? (Re: [PATCH 1/2] i2c: sprd: Prevent i2c accesses after suspend is called)

[Wolfram Sang]

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

Hi Mark,

> > And maybe this could be used here, too? Introduce this flag for very
> > late/early messages. If they have it, messages are even sent in
> > suspend_noirq() phase with the master_xfer_irqless() callback, otherwise
> > we will have the WARNing printed out.
> 
> It feels like it'd be more elegant to have the core select the irqless
> function automatically if called after interrupts have been disabled -
> otherwise we end up with the need to special case through other layers
> of the stack like regmap as well which seems like it'd be error prone.

Yes, I was concerned about thae (i.e. regmap accessors), too.

> OTOH it does mean we might not notice things happening later than they
> should so it's not 100% clear...

What do you mean here?

Thanks,

   Wolfram


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

Re: I2C PM overhaul needed? (Re: [PATCH 1/2] i2c: sprd: Prevent i2c accesses after suspend is called)

[Mark Brown]

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

On Sat, May 05, 2018 at 10:32:55AM +0200, Wolfram Sang wrote:

> > OTOH it does mean we might not notice things happening later than they
> > should so it's not 100% clear...

> What do you mean here?

If someone is doing I/O later than they should currently we'll notice as
they try to do something that needs interrupts with interrupts disabled.
With transparent noirq then it'll just work on some percentage of
systems with controllers that support that but fail on others.  That may
or may not be what we want but it's worth considering.

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

Re: I2C PM overhaul needed? (Re: [PATCH 1/2] i2c: sprd: Prevent i2c accesses after suspend is called)

[Grygorii Strashko]

On 05/04/2018 07:24 AM, Wolfram Sang wrote:
> Hi Grygorii,
> 
> thanks for stepping in.  I kept thinking about better I2C core support
> for such situations and the more input the better.
> 
>> And you have to fix it (touch screen) - not your i2c driver. Otherwise, you can get
>> situation when set of I2C transfers (executed from some kthread/work/threaded_irq/..)
>> will be just interrupted in the middle - usual behavior after this is (I2C timeout) [and/or
>> not-functional I2C client device [and/or I2C bus stuck (worst case)].
> 
> This. I also tend to think that most issues need to be fixed in the
> client drivers ensuring proper states of client devices when suspending
> / resuming.. I wonder, though, if the core shouldn't assist by
> guaranteeing that an on-going transfer has finished before suspending?
> Or more technically, wait for a locked adapter to become unlocked again?

That would be great, but note:
1) only i2c_transfer() operations are locked, so if driver is doing 
i2c_transfer(1)
i2c_transfer(2) <- suspend in the middle
<- suspend in between
i2c_transfer(3)
It will not help.
In other word, one suspend cycle may be done without warnings or issues,
while another may show up broken devices after suspend or even system crash.
Everything depends on timings :( - in my practice 10000 suspend iteration tests
where required to run many times to catch 3 buggy I2C client drivers.

2) It's normal to abort suspend if system is busy, so if I2C core will be able
to catch active I2C operation - it should abort, but again I do not see how it
 can be detected 100% with current I2C core design or without reworking huge number of drivers.

3) So, only one thing I2C core potentially can do - catch invalid access and
report it. "wait for transfer to finish" wouldn't work as for me.

> 
> I still need to set it up and test, yet seeing that the EEPROM driver
> at24.c has no suspend/resume callbacks, I'd assume a big write operation
> will only be partially done when interrupted by a suspend.
> 
>> In case, somebody is trying to access I2C after .suspend_noirq() stage I2C bus driver
>> should produce big fat warning and, most probably, abort suspend.
>> Above, in general, can be part of I2C core functionality.
> 
> Also this. However, there might be an exception of devices like PMICs
> which may need to be accessed to trigger the final suspend state.
> 
> I at least know of some Renesas boards which needed the I2C connected
> PMIC to do a system reset (not sure about suspend, need to recheck
> that). That still today causes problems because interrupts are disabled
> then.

this was triggered few times already (sry, don't have links), as of now,
 and as I know, the only ways to W/A this is:
- to create barametal platform driver (some time in ASM)
- or delegate final suspend operation to another system controller (co-processor),
  as example TI am335x SoCs,
- or implement I2C driver in hw - TI AVS/SmartReflex. 

> 
> To handle that, I imagined an additional adapter callback like
> 'master_xfer_irqless' to be used for such special I2C messages. These
> kind of special messages could be tagged with a new I2C_M_something
> flag.
> 
> And maybe this could be used here, too? Introduce this flag for very
> late/early messages. If they have it, messages are even sent in
> suspend_noirq() phase with the master_xfer_irqless() callback, otherwise
> we will have the WARNing printed out.

Sry, but 99% percent of I2C client drivers *should not* access I2C bus after
.suspend_noirq() stage it's BUG-BUG!! Any W/A will just hide real problems.

"master_xfer_irqless" might be a not bad idea, but, in my opinion, it 
should be used explicitly by platform code only, and each usage should
be proved to exist. 

Some additional info:
static int suspend_enter(suspend_state_t state, bool *wakeup)
{
[...]

	error = dpm_suspend_noirq(PMSG_SUSPEND);
	if (error) {
		pr_err("noirq suspend of devices failed\n");
		goto Platform_early_resume;
	}

^^^ after this no drivers expect to work, unless they are wake up
sources and even in this case - they should handle wake up in
their .suspend_noirq() or even later.

	error = platform_suspend_prepare_noirq(state);
	if (error)
		goto Platform_wake;

	if (suspend_test(TEST_PLATFORM))
		goto Platform_wake;

^^^ before disable_nonboot_cpus() there can be few processes
    running in parallel on SMP: one is suspend, others can be anything  

	error = disable_nonboot_cpus();
	if (error || suspend_test(TEST_CPUS))
		goto Enable_cpus;

	arch_suspend_disable_irqs();
^^^ after this point only suspend process is active
 any regular drivers usage is prohibited,BUG-BUG (at least in regular way,
 but some special irqless APIs still can be used, but only by platform suspend code)

	BUG_ON(!irqs_disabled());

	error = syscore_suspend();
	if (!error) {

^^^ scheduler and systimers are disabled

		*wakeup = pm_wakeup_pending();
		if (!(suspend_test(TEST_CORE) || *wakeup)) {
			trace_suspend_resume(TPS("machine_suspend"),
				state, true);
			error = suspend_ops->enter(state);

^^^ platform code - final suspend stage

			trace_suspend_resume(TPS("machine_suspend"),
				state, false);
		} else if (*wakeup) {
			error = -EBUSY;
		}
		syscore_resume();
	}

	arch_suspend_enable_irqs();
	BUG_ON(irqs_disabled());

 Enable_cpus:
	enable_nonboot_cpus();

 Platform_wake:
	platform_resume_noirq(state);
	dpm_resume_noirq(PMSG_RESUME);

^^^ I2C can be accessible again

[...]

}
> 
> Thoughts? Any other cases missed so far?
> 

I'm attaching some very old patch (don't ask me why it was not sent :()
I did for Android system - which likes suspend very much. Some
part of below diff are obsolete now (like omap_i2c_suspend()),
but .noirq() callback are still valid and can show over all idea.
Really helped to catch min 3 buggy client drivers with timers, delayed
or periodic works.

Huh, hope it's useful :)

By the way, original patch from this thread is ok in general, but 
WARN() after checking is_suspended flag will be helpful. 

+	if (i2c_dev->is_suspended)
^^ WARN()?
+		return -EBUSY;

-- 
regards,
-grygorii

----
commit 125ef8f7016e7b205886f93862288a45a312b1d8
Author: Grygorii Strashko <grygorii.strashko@ti.com>
Date:   Thu Sep 20 19:56:44 2012 +0300

    I2C: OMAP: Fix PM runtime functionality during suspend/resume
    
    The OMAP I2C devices power management is based on Runtime PM only,
    because they are intended to be used at later/earlier stages of System
    wide suspend/resume execution. In addition, OMAP PM framework is based
    on OMAP HWMOD framework which, in turn, connected to Runtime PM
    subsystem.
    But now, the Runtime PM is disabled for I2C devices immediately after
    execution of its .suspend() callback. As result, I2C devices can't be
    accessible during later suspend/early resume stages.


    
    This patch always enables I2C device from .suspend() callback,
    so it can be accessible during later stages of suspending.

^^^^ this part is not valid any more

    Then, I2C 
    device is finally turned off on exit .suspend_noirq() callback by
    omap_device framework, so it can be accessible during later suspending
    stages.
    From other side, the I2C device is unconditionally turned on before
    entering resume_noirq() callback by omap_device framework, so it can be
    accessible during earlier stages of resuming.
    
    More over, the additional check added to ensure that there are no
    activities on I2C bus, because some drivers may still have active kernel
    thread/works which may be executed on another CPU and try to access I2C
    -- this is the BUG and this may cause system crash.
    To prevent such situations additional flag "suspended" is added in
    struct omap_i2c_dev which is used to forbid access to I2C bus after
    .suspend_noirq() callback execution.
   
    Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 5ef9b7e..f5914c9 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -207,6 +207,9 @@ struct omap_i2c_dev {
        u16                     westate;
        u16                     errata;
        int                     dev_lost_count;
+       bool                    suspended;      /* if true - I2C device
+                                                  suspended and can't be
+                                                  accessible*/
 };
 
 static const u8 reg_map_ip_v1[] = {
@@ -643,6 +646,12 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
                return -EINVAL;
        }
 
+       if (dev->suspended) {
+               WARN(true, "%s: Access denied - device already suspended\n",
+                    dev_name(dev->dev));
+               return -EACCES;
+       }
+
        r = omap_i2c_hwspinlock_lock(dev);
        /* To-Do: if we are unable to acquire the lock, we must
        try to recover somehow */
@@ -651,7 +660,6 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 
        r = pm_runtime_get_sync(dev->dev);
        if (r < 0) {
-               pm_runtime_put_noidle(dev->dev);
                goto err_pm;
        }
 
@@ -690,8 +698,8 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
        omap_i2c_wait_for_bb(dev);
 out:
        disable_irq(dev->irq);
-       pm_runtime_put_sync(dev->dev);
 err_pm:
+       pm_runtime_put_sync(dev->dev);
        omap_i2c_hwspinlock_unlock(dev);
        return r;
 }
@@ -1270,7 +1278,79 @@ static int omap_i2c_runtime_resume(struct device *dev)
 }
 #endif /* CONFIG_PM_RUNTIME */
 
+static int omap_i2c_suspend(struct device *dev)
+{
+       int ret;
+
+       /*
+        *  Enable I2C device, so it will be accessible during
+        * later stages of suspending when device Runtime PM is disabled.
+        * I2C device will be turned off at "noirq" suspend stage.
+        */
+       ret = pm_runtime_resume(dev);
+       if (ret < 0)
+               return ret;
+       return 0;
+}
+
+static int omap_i2c_suspend_noirq(struct device *dev)
+{
+       struct platform_device *pdev = to_platform_device(dev);
+       struct omap_i2c_dev *_dev = platform_get_drvdata(pdev);
+
+       /*
+        * Below code is needed only to ensure that there are no
+        * activities on I2C bus. if at this moment any driver
+        * is trying to use I2C bus - this is the BUG and this
+        * may cause system crash.
+        *
+        * So forbid access to I2C device using _dev->suspended flag.
+        */
+
+       i2c_lock_adapter(&_dev->adapter);
+
+       /* Check for active I2C transaction */
+       if (atomic_read(&dev->power.usage_count) > 1) {
+               dev_info(dev,
+                        "active I2C transaction detected - suspend aborted\n");
+               return -EBUSY;
+       }
+
+       _dev->suspended = true;
+
+       i2c_unlock_adapter(&_dev->adapter);
+
+       /*
+        * I2C device will be turned off by omap_device framework
+        * on exit from .suspend_noirq() callback.
+        * So, it will not be accessible any more.
+        */
+       return 0;
+}
+
+static int omap_i2c_resume_noirq(struct device *dev)
+{
+       struct platform_device *pdev = to_platform_device(dev);
+       struct omap_i2c_dev *_dev = platform_get_drvdata(pdev);
+
+       /*
+        * I2C device  has been turned on already by omap_device framework
+        * unconditionally. So, it will be accessible during earlier
+        * stages of resuming when device Runtime PM is disabled
+        */
+
+       /* Allow access to I2C bus*/
+       i2c_lock_adapter(&_dev->adapter);
+       _dev->suspended = false;
+       i2c_unlock_adapter(&_dev->adapter);
+
+       return 0;
+}
+
 static struct dev_pm_ops omap_i2c_pm_ops = {
+       .suspend_noirq = omap_i2c_suspend_noirq,
+       .resume_noirq = omap_i2c_resume_noirq,
+       SET_SYSTEM_SLEEP_PM_OPS(omap_i2c_suspend, NULL)
        SET_RUNTIME_PM_OPS(omap_i2c_runtime_suspend,
                           omap_i2c_runtime_resume, NULL)
 };

Re: I2C PM overhaul needed? (Re: [PATCH 1/2] i2c: sprd: Prevent i2c accesses after suspend is called)

[Grygorii Strashko]

On 05/04/2018 07:24 AM, Wolfram Sang wrote:
> Hi Grygorii,
> 
> thanks for stepping in.  I kept thinking about better I2C core support
> for such situations and the more input the better.
> 
>> And you have to fix it (touch screen) - not your i2c driver. Otherwise, you can get
>> situation when set of I2C transfers (executed from some kthread/work/threaded_irq/..)
>> will be just interrupted in the middle - usual behavior after this is (I2C timeout) [and/or
>> not-functional I2C client device [and/or I2C bus stuck (worst case)].
> 
> This. I also tend to think that most issues need to be fixed in the
> client drivers ensuring proper states of client devices when suspending
> / resuming.. I wonder, though, if the core shouldn't assist by
> guaranteeing that an on-going transfer has finished before suspending?
> Or more technically, wait for a locked adapter to become unlocked again?

That would be great, but note:
1) only i2c_transfer() operations are locked, so if driver is doing 
i2c_transfer(1)
i2c_transfer(2) <- suspend in the middle
<- suspend in between
i2c_transfer(3)
It will not help.
In other word, one suspend cycle may be done without warnings or issues,
while another may show up broken devices after suspend or even system crash.
Everything depends on timings :( - in my practice 10000 suspend iteration tests
where required to run many times to catch 3 buggy I2C client drivers.

2) It's normal to abort suspend if system is busy, so if I2C core will be able
to catch active I2C operation - it should abort, but again I do not see how it
 can be detected 100% with current I2C core design or without reworking huge number of drivers.

3) So, only one thing I2C core potentially can do - catch invalid access and
report it. "wait for transfer to finish" wouldn't work as for me.

> 
> I still need to set it up and test, yet seeing that the EEPROM driver
> at24.c has no suspend/resume callbacks, I'd assume a big write operation
> will only be partially done when interrupted by a suspend.
> 
>> In case, somebody is trying to access I2C after .suspend_noirq() stage I2C bus driver
>> should produce big fat warning and, most probably, abort suspend.
>> Above, in general, can be part of I2C core functionality.
> 
> Also this. However, there might be an exception of devices like PMICs
> which may need to be accessed to trigger the final suspend state.
> 
> I at least know of some Renesas boards which needed the I2C connected
> PMIC to do a system reset (not sure about suspend, need to recheck
> that). That still today causes problems because interrupts are disabled
> then.

this was triggered few times already (sry, don't have links), as of now,
 and as I know, the only ways to W/A this is:
- to create barametal platform driver (some time in ASM)
- or delegate final suspend operation to another system controller (co-processor),
  as example TI am335x SoCs,
- or implement I2C driver in hw - TI AVS/SmartReflex. 

> 
> To handle that, I imagined an additional adapter callback like
> 'master_xfer_irqless' to be used for such special I2C messages. These
> kind of special messages could be tagged with a new I2C_M_something
> flag.
> 
> And maybe this could be used here, too? Introduce this flag for very
> late/early messages. If they have it, messages are even sent in
> suspend_noirq() phase with the master_xfer_irqless() callback, otherwise
> we will have the WARNing printed out.

Sry, but 99% percent of I2C client drivers *should not* access I2C bus after
.suspend_noirq() stage it's BUG-BUG!! Any W/A will just hide real problems.

"master_xfer_irqless" might be a not bad idea, but, in my opinion, it 
should be used explicitly by platform code only, and each usage should
be proved to exist. 

Some additional info:
static int suspend_enter(suspend_state_t state, bool *wakeup)
{
[...]

	error = dpm_suspend_noirq(PMSG_SUSPEND);
	if (error) {
		pr_err("noirq suspend of devices failed\n");
		goto Platform_early_resume;
	}

^^^ after this no drivers expect to work, unless they are wake up
sources and even in this case - they should handle wake up in
their .suspend_noirq() or even later.

	error = platform_suspend_prepare_noirq(state);
	if (error)
		goto Platform_wake;

	if (suspend_test(TEST_PLATFORM))
		goto Platform_wake;

^^^ before disable_nonboot_cpus() there can be few processes
    running in parallel on SMP: one is suspend, others can be anything  

	error = disable_nonboot_cpus();
	if (error || suspend_test(TEST_CPUS))
		goto Enable_cpus;

	arch_suspend_disable_irqs();
^^^ after this point only suspend process is active
 any regular drivers usage is prohibited,BUG-BUG (at least in regular way,
 but some special irqless APIs still can be used, but only by platform suspend code)

	BUG_ON(!irqs_disabled());

	error = syscore_suspend();
	if (!error) {

^^^ scheduler and systimers are disabled

		*wakeup = pm_wakeup_pending();
		if (!(suspend_test(TEST_CORE) || *wakeup)) {
			trace_suspend_resume(TPS("machine_suspend"),
				state, true);
			error = suspend_ops->enter(state);

^^^ platform code - final suspend stage

			trace_suspend_resume(TPS("machine_suspend"),
				state, false);
		} else if (*wakeup) {
			error = -EBUSY;
		}
		syscore_resume();
	}

	arch_suspend_enable_irqs();
	BUG_ON(irqs_disabled());

 Enable_cpus:
	enable_nonboot_cpus();

 Platform_wake:
	platform_resume_noirq(state);
	dpm_resume_noirq(PMSG_RESUME);

^^^ I2C can be accessible again

[...]

}
> 
> Thoughts? Any other cases missed so far?
> 

I'm attaching some very old patch (don't ask me why it was not sent :()
I did for Android system - which likes suspend very much. Some
part of below diff are obsolete now (like omap_i2c_suspend()),
but .noirq() callback are still valid and can show over all idea.
Really helped to catch min 3 buggy client drivers with timers, delayed
or periodic works.

Huh, hope it's useful :)

By the way, original patch from this thread is ok in general, but 
WARN() after checking is_suspended flag will be helpful. 

+	if (i2c_dev->is_suspended)
^^ WARN()?
+		return -EBUSY;

-- 
regards,
-grygorii

----
commit 125ef8f7016e7b205886f93862288a45a312b1d8
Author: Grygorii Strashko <grygorii.strashko@ti.com>
Date:   Thu Sep 20 19:56:44 2012 +0300

    I2C: OMAP: Fix PM runtime functionality during suspend/resume
    
    The OMAP I2C devices power management is based on Runtime PM only,
    because they are intended to be used at later/earlier stages of System
    wide suspend/resume execution. In addition, OMAP PM framework is based
    on OMAP HWMOD framework which, in turn, connected to Runtime PM
    subsystem.
    But now, the Runtime PM is disabled for I2C devices immediately after
    execution of its .suspend() callback. As result, I2C devices can't be
    accessible during later suspend/early resume stages.


    
    This patch always enables I2C device from .suspend() callback,
    so it can be accessible during later stages of suspending.

^^^^ this part is not valid any more

    Then, I2C 
    device is finally turned off on exit .suspend_noirq() callback by
    omap_device framework, so it can be accessible during later suspending
    stages.
    From other side, the I2C device is unconditionally turned on before
    entering resume_noirq() callback by omap_device framework, so it can be
    accessible during earlier stages of resuming.
    
    More over, the additional check added to ensure that there are no
    activities on I2C bus, because some drivers may still have active kernel
    thread/works which may be executed on another CPU and try to access I2C
    -- this is the BUG and this may cause system crash.
    To prevent such situations additional flag "suspended" is added in
    struct omap_i2c_dev which is used to forbid access to I2C bus after
    .suspend_noirq() callback execution.
   
    Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 5ef9b7e..f5914c9 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -207,6 +207,9 @@ struct omap_i2c_dev {
        u16                     westate;
        u16                     errata;
        int                     dev_lost_count;
+       bool                    suspended;      /* if true - I2C device
+                                                  suspended and can't be
+                                                  accessible*/
 };
 
 static const u8 reg_map_ip_v1[] = {
@@ -643,6 +646,12 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
                return -EINVAL;
        }
 
+       if (dev->suspended) {
+               WARN(true, "%s: Access denied - device already suspended\n",
+                    dev_name(dev->dev));
+               return -EACCES;
+       }
+
        r = omap_i2c_hwspinlock_lock(dev);
        /* To-Do: if we are unable to acquire the lock, we must
        try to recover somehow */
@@ -651,7 +660,6 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 
        r = pm_runtime_get_sync(dev->dev);
        if (r < 0) {
-               pm_runtime_put_noidle(dev->dev);
                goto err_pm;
        }
 
@@ -690,8 +698,8 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
        omap_i2c_wait_for_bb(dev);
 out:
        disable_irq(dev->irq);
-       pm_runtime_put_sync(dev->dev);
 err_pm:
+       pm_runtime_put_sync(dev->dev);
        omap_i2c_hwspinlock_unlock(dev);
        return r;
 }
@@ -1270,7 +1278,79 @@ static int omap_i2c_runtime_resume(struct device *dev)
 }
 #endif /* CONFIG_PM_RUNTIME */
 
+static int omap_i2c_suspend(struct device *dev)
+{
+       int ret;
+
+       /*
+        *  Enable I2C device, so it will be accessible during
+        * later stages of suspending when device Runtime PM is disabled.
+        * I2C device will be turned off at "noirq" suspend stage.
+        */
+       ret = pm_runtime_resume(dev);
+       if (ret < 0)
+               return ret;
+       return 0;
+}
+
+static int omap_i2c_suspend_noirq(struct device *dev)
+{
+       struct platform_device *pdev = to_platform_device(dev);
+       struct omap_i2c_dev *_dev = platform_get_drvdata(pdev);
+
+       /*
+        * Below code is needed only to ensure that there are no
+        * activities on I2C bus. if at this moment any driver
+        * is trying to use I2C bus - this is the BUG and this
+        * may cause system crash.
+        *
+        * So forbid access to I2C device using _dev->suspended flag.
+        */
+
+       i2c_lock_adapter(&_dev->adapter);
+
+       /* Check for active I2C transaction */
+       if (atomic_read(&dev->power.usage_count) > 1) {
+               dev_info(dev,
+                        "active I2C transaction detected - suspend aborted\n");
+               return -EBUSY;
+       }
+
+       _dev->suspended = true;
+
+       i2c_unlock_adapter(&_dev->adapter);
+
+       /*
+        * I2C device will be turned off by omap_device framework
+        * on exit from .suspend_noirq() callback.
+        * So, it will not be accessible any more.
+        */
+       return 0;
+}
+
+static int omap_i2c_resume_noirq(struct device *dev)
+{
+       struct platform_device *pdev = to_platform_device(dev);
+       struct omap_i2c_dev *_dev = platform_get_drvdata(pdev);
+
+       /*
+        * I2C device  has been turned on already by omap_device framework
+        * unconditionally. So, it will be accessible during earlier
+        * stages of resuming when device Runtime PM is disabled
+        */
+
+       /* Allow access to I2C bus*/
+       i2c_lock_adapter(&_dev->adapter);
+       _dev->suspended = false;
+       i2c_unlock_adapter(&_dev->adapter);
+
+       return 0;
+}
+
 static struct dev_pm_ops omap_i2c_pm_ops = {
+       .suspend_noirq = omap_i2c_suspend_noirq,
+       .resume_noirq = omap_i2c_resume_noirq,
+       SET_SYSTEM_SLEEP_PM_OPS(omap_i2c_suspend, NULL)
        SET_RUNTIME_PM_OPS(omap_i2c_runtime_suspend,
                           omap_i2c_runtime_resume, NULL)
 };

Re: I2C PM overhaul needed? (Re: [PATCH 1/2] i2c: sprd: Prevent i2c accesses after suspend is called)

[Wolfram Sang]

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

Grygorii,

thanks a lot for your input. Much appreciated!

> That would be great, but note:
> 1) only i2c_transfer() operations are locked, so if driver is doing 
> i2c_transfer(1)
> i2c_transfer(2) <- suspend in the middle
> <- suspend in between
> i2c_transfer(3)
> It will not help.

Will it not improve the situation by ensuring that at least the transfer
with its (potenitally) multiple messages got completed? That we are at
least in a bus-free state (assuming single-master here) before
suspending?

> Everything depends on timings :( - in my practice 10000 suspend iteration tests
> where required to run many times to catch 3 buggy I2C client drivers.

Matches my experiences that creating a reliable test case for that is
not that easy as I thought. Or I am missing something obvious.

> 2) It's normal to abort suspend if system is busy, so if I2C core will be able
> to catch active I2C operation - it should abort, but again I do not see how it
>  can be detected 100% with current I2C core design or without reworking huge number of drivers.

I agree. After second thought, waiting for i2c_transfer to finish maybe
won't be enough, I am afraid. We don't know if STOP has been put on the
wires yet. My best bet now is that we implement such a
'is-transfer-ongoing'-check in the suspend function of the master
driver? That check should be optional, but recommended.

> 3) So, only one thing I2C core potentially can do - catch invalid access and
> report it. "wait for transfer to finish" wouldn't work as for me.

And we do this in suspend_noirq function of the i2c core.

> > I at least know of some Renesas boards which needed the I2C connected
> > PMIC to do a system reset (not sure about suspend, need to recheck
> > that). That still today causes problems because interrupts are disabled
> > then.
> 
> this was triggered few times already (sry, don't have links), as of now,
>  and as I know, the only ways to W/A this is:
> - to create barametal platform driver (some time in ASM)
> - or delegate final suspend operation to another system controller (co-processor),
>   as example TI am335x SoCs,
> - or implement I2C driver in hw - TI AVS/SmartReflex. 

Yes. Please note that this is only needed for reset, not suspend. So, it
is a bit easier. Still, it might make more sense to use a platform based
solution. I'll think about that.

> Sry, but 99% percent of I2C client drivers *should not* access I2C bus after
> .suspend_noirq() stage it's BUG-BUG!! Any W/A will just hide real problems.

I do believe you, still is there documentation about such things? I like
to understand more but didn't dig up something up to now. E.g. I grepped
for "noirq" in Documentation/power.

> "master_xfer_irqless" might be a not bad idea, but, in my opinion, it 
> should be used explicitly by platform code only, and each usage should
> be proved to exist. 

Yes, we can think about it once it is really needed.

> Some additional info:

Thanks a lot for that!

> I'm attaching some very old patch (don't ask me why it was not sent :()
> I did for Android system - which likes suspend very much. Some
> part of below diff are obsolete now (like omap_i2c_suspend()),
> but .noirq() callback are still valid and can show over all idea.
> Really helped to catch min 3 buggy client drivers with timers, delayed
> or periodic works.

Ok, so what do you think about my plan to:

1) encourage drivers to check if there is still an ongoing transfer in
their .suspend function (or the core can do that, too, if we agree that
checking for a taken adapter lock is sufficient)

-> to ensure transfers don't get interrupted in the middle

2) use a .suspend_noirq callback in i2c_bus_type.pm to reject and WARN
about transfers still going on in that phase

-> this ensures that buggy drivers are caught

3) write some documentation about our findings / assumptions /
recommendations to a file in Documentation/i2c/

-> this ensures we won't forget why we did things like they are ;)

?

Kind regards,

   Wolfram


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

Re: I2C PM overhaul needed? (Re: [PATCH 1/2] i2c: sprd: Prevent i2c accesses after suspend is called)

[Peter Rosin]

On 2018-05-08 18:32, Wolfram Sang wrote:
> Grygorii,
> 
> thanks a lot for your input. Much appreciated!
> 
>> That would be great, but note:
>> 1) only i2c_transfer() operations are locked, so if driver is doing 
>> i2c_transfer(1)
>> i2c_transfer(2) <- suspend in the middle
>> <- suspend in between
>> i2c_transfer(3)
>> It will not help.
> 
> Will it not improve the situation by ensuring that at least the transfer
> with its (potenitally) multiple messages got completed? That we are at
> least in a bus-free state (assuming single-master here) before
> suspending?
> 
>> Everything depends on timings :( - in my practice 10000 suspend iteration tests
>> where required to run many times to catch 3 buggy I2C client drivers.
> 
> Matches my experiences that creating a reliable test case for that is
> not that easy as I thought. Or I am missing something obvious.
> 
>> 2) It's normal to abort suspend if system is busy, so if I2C core will be able
>> to catch active I2C operation - it should abort, but again I do not see how it
>>  can be detected 100% with current I2C core design or without reworking huge number of drivers.
> 
> I agree. After second thought, waiting for i2c_transfer to finish maybe
> won't be enough, I am afraid. We don't know if STOP has been put on the
> wires yet. My best bet now is that we implement such a
> 'is-transfer-ongoing'-check in the suspend function of the master
> driver? That check should be optional, but recommended.
> 
>> 3) So, only one thing I2C core potentially can do - catch invalid access and
>> report it. "wait for transfer to finish" wouldn't work as for me.
> 
> And we do this in suspend_noirq function of the i2c core.
> 
>>> I at least know of some Renesas boards which needed the I2C connected
>>> PMIC to do a system reset (not sure about suspend, need to recheck
>>> that). That still today causes problems because interrupts are disabled
>>> then.
>>
>> this was triggered few times already (sry, don't have links), as of now,
>>  and as I know, the only ways to W/A this is:
>> - to create barametal platform driver (some time in ASM)
>> - or delegate final suspend operation to another system controller (co-processor),
>>   as example TI am335x SoCs,
>> - or implement I2C driver in hw - TI AVS/SmartReflex. 
> 
> Yes. Please note that this is only needed for reset, not suspend. So, it
> is a bit easier. Still, it might make more sense to use a platform based
> solution. I'll think about that.
> 
>> Sry, but 99% percent of I2C client drivers *should not* access I2C bus after
>> .suspend_noirq() stage it's BUG-BUG!! Any W/A will just hide real problems.
> 
> I do believe you, still is there documentation about such things? I like
> to understand more but didn't dig up something up to now. E.g. I grepped
> for "noirq" in Documentation/power.
> 
>> "master_xfer_irqless" might be a not bad idea, but, in my opinion, it 
>> should be used explicitly by platform code only, and each usage should
>> be proved to exist. 
> 
> Yes, we can think about it once it is really needed.
> 
>> Some additional info:
> 
> Thanks a lot for that!
> 
>> I'm attaching some very old patch (don't ask me why it was not sent :()
>> I did for Android system - which likes suspend very much. Some
>> part of below diff are obsolete now (like omap_i2c_suspend()),
>> but .noirq() callback are still valid and can show over all idea.
>> Really helped to catch min 3 buggy client drivers with timers, delayed
>> or periodic works.
> 
> Ok, so what do you think about my plan to:
> 
> 1) encourage drivers to check if there is still an ongoing transfer in
> their .suspend function (or the core can do that, too, if we agree that
> checking for a taken adapter lock is sufficient)
> 
> -> to ensure transfers don't get interrupted in the middle

A note from the peanut gallery: the adapter lock is not sufficient when
there are mux-locked muxes on the bus.

Cheers,
Peter

> 2) use a .suspend_noirq callback in i2c_bus_type.pm to reject and WARN
> about transfers still going on in that phase
> 
> -> this ensures that buggy drivers are caught
> 
> 3) write some documentation about our findings / assumptions /
> recommendations to a file in Documentation/i2c/
> 
> -> this ensures we won't forget why we did things like they are ;)
> 
> ?
> 
> Kind regards,
> 
>    Wolfram
> 

Re: I2C PM overhaul needed? (Re: [PATCH 1/2] i2c: sprd: Prevent i2c accesses after suspend is called)

[Grygorii Strashko]

On 05/08/2018 11:32 AM, Wolfram Sang wrote:
> Grygorii,
> 
> thanks a lot for your input. Much appreciated!
> 
>> That would be great, but note:
>> 1) only i2c_transfer() operations are locked, so if driver is doing
>> i2c_transfer(1)
>> i2c_transfer(2) <- suspend in the middle
>> <- suspend in between
>> i2c_transfer(3)
>> It will not help.
> 
> Will it not improve the situation by ensuring that at least the transfer
> with its (potenitally) multiple messages got completed? That we are at
> least in a bus-free state (assuming single-master here) before
> suspending?
> 
>> Everything depends on timings :( - in my practice 10000 suspend iteration tests
>> where required to run many times to catch 3 buggy I2C client drivers.
> 
> Matches my experiences that creating a reliable test case for that is
> not that easy as I thought. Or I am missing something obvious.
> 
>> 2) It's normal to abort suspend if system is busy, so if I2C core will be able
>> to catch active I2C operation - it should abort, but again I do not see how it
>>   can be detected 100% with current I2C core design or without reworking huge number of drivers.
> 
> I agree. After second thought, waiting for i2c_transfer to finish maybe
> won't be enough, I am afraid. We don't know if STOP has been put on the
> wires yet. My best bet now is that we implement such a
> 'is-transfer-ongoing'-check in the suspend function of the master
> driver? That check should be optional, but recommended.
> 
>> 3) So, only one thing I2C core potentially can do - catch invalid access and
>> report it. "wait for transfer to finish" wouldn't work as for me.
> 
> And we do this in suspend_noirq function of the i2c core.
> 
>>> I at least know of some Renesas boards which needed the I2C connected
>>> PMIC to do a system reset (not sure about suspend, need to recheck
>>> that). That still today causes problems because interrupts are disabled
>>> then.
>>
>> this was triggered few times already (sry, don't have links), as of now,
>>   and as I know, the only ways to W/A this is:
>> - to create barametal platform driver (some time in ASM)
>> - or delegate final suspend operation to another system controller (co-processor),
>>    as example TI am335x SoCs,
>> - or implement I2C driver in hw - TI AVS/SmartReflex.
> 
> Yes. Please note that this is only needed for reset, not suspend. So, it
> is a bit easier. Still, it might make more sense to use a platform based
> solution. I'll think about that.
> 
>> Sry, but 99% percent of I2C client drivers *should not* access I2C bus after
>> .suspend_noirq() stage it's BUG-BUG!! Any W/A will just hide real problems.
> 
> I do believe you, still is there documentation about such things? I like
> to understand more but didn't dig up something up to now. E.g. I grepped
> for "noirq" in Documentation/power.
> 
>> "master_xfer_irqless" might be a not bad idea, but, in my opinion, it
>> should be used explicitly by platform code only, and each usage should
>> be proved to exist.
> 
> Yes, we can think about it once it is really needed.
> 
>> Some additional info:
> 
> Thanks a lot for that!
> 
>> I'm attaching some very old patch (don't ask me why it was not sent :()
>> I did for Android system - which likes suspend very much. Some
>> part of below diff are obsolete now (like omap_i2c_suspend()),
>> but .noirq() callback are still valid and can show over all idea.
>> Really helped to catch min 3 buggy client drivers with timers, delayed
>> or periodic works.
> 
> Ok, so what do you think about my plan to:
> 
> 1) encourage drivers to check if there is still an ongoing transfer in
> their .suspend function (or the core can do that, too, if we agree that
> checking for a taken adapter lock is sufficient)
> 
> -> to ensure transfers don't get interrupted in the middle

It probably should be part of .suspend_noirq() also.

> 
> 2) use a .suspend_noirq callback in i2c_bus_type.pm to reject and WARN
> about transfers still going on in that phase
> 
> -> this ensures that buggy drivers are caught
> 
> 3) write some documentation about our findings / assumptions /
> recommendations to a file in Documentation/i2c/
> 
> -> this ensures we won't forget why we did things like they are ;)
> 
> ?

Sry, for delayed reply. It sounds good.

-- 
regards,
-grygorii

Re: I2C PM overhaul needed? (Re: [PATCH 1/2] i2c: sprd: Prevent i2c accesses after suspend is called)

[Grygorii Strashko]

On 05/08/2018 11:32 AM, Wolfram Sang wrote:
> Grygorii,
> 
> thanks a lot for your input. Much appreciated!
> 
>> That would be great, but note:
>> 1) only i2c_transfer() operations are locked, so if driver is doing
>> i2c_transfer(1)
>> i2c_transfer(2) <- suspend in the middle
>> <- suspend in between
>> i2c_transfer(3)
>> It will not help.
> 
> Will it not improve the situation by ensuring that at least the transfer
> with its (potenitally) multiple messages got completed? That we are at
> least in a bus-free state (assuming single-master here) before
> suspending?
> 
>> Everything depends on timings :( - in my practice 10000 suspend iteration tests
>> where required to run many times to catch 3 buggy I2C client drivers.
> 
> Matches my experiences that creating a reliable test case for that is
> not that easy as I thought. Or I am missing something obvious.
> 
>> 2) It's normal to abort suspend if system is busy, so if I2C core will be able
>> to catch active I2C operation - it should abort, but again I do not see how it
>>   can be detected 100% with current I2C core design or without reworking huge number of drivers.
> 
> I agree. After second thought, waiting for i2c_transfer to finish maybe
> won't be enough, I am afraid. We don't know if STOP has been put on the
> wires yet. My best bet now is that we implement such a
> 'is-transfer-ongoing'-check in the suspend function of the master
> driver? That check should be optional, but recommended.
> 
>> 3) So, only one thing I2C core potentially can do - catch invalid access and
>> report it. "wait for transfer to finish" wouldn't work as for me.
> 
> And we do this in suspend_noirq function of the i2c core.
> 
>>> I at least know of some Renesas boards which needed the I2C connected
>>> PMIC to do a system reset (not sure about suspend, need to recheck
>>> that). That still today causes problems because interrupts are disabled
>>> then.
>>
>> this was triggered few times already (sry, don't have links), as of now,
>>   and as I know, the only ways to W/A this is:
>> - to create barametal platform driver (some time in ASM)
>> - or delegate final suspend operation to another system controller (co-processor),
>>    as example TI am335x SoCs,
>> - or implement I2C driver in hw - TI AVS/SmartReflex.
> 
> Yes. Please note that this is only needed for reset, not suspend. So, it
> is a bit easier. Still, it might make more sense to use a platform based
> solution. I'll think about that.
> 
>> Sry, but 99% percent of I2C client drivers *should not* access I2C bus after
>> .suspend_noirq() stage it's BUG-BUG!! Any W/A will just hide real problems.
> 
> I do believe you, still is there documentation about such things? I like
> to understand more but didn't dig up something up to now. E.g. I grepped
> for "noirq" in Documentation/power.
> 
>> "master_xfer_irqless" might be a not bad idea, but, in my opinion, it
>> should be used explicitly by platform code only, and each usage should
>> be proved to exist.
> 
> Yes, we can think about it once it is really needed.
> 
>> Some additional info:
> 
> Thanks a lot for that!
> 
>> I'm attaching some very old patch (don't ask me why it was not sent :()
>> I did for Android system - which likes suspend very much. Some
>> part of below diff are obsolete now (like omap_i2c_suspend()),
>> but .noirq() callback are still valid and can show over all idea.
>> Really helped to catch min 3 buggy client drivers with timers, delayed
>> or periodic works.
> 
> Ok, so what do you think about my plan to:
> 
> 1) encourage drivers to check if there is still an ongoing transfer in
> their .suspend function (or the core can do that, too, if we agree that
> checking for a taken adapter lock is sufficient)
> 
> -> to ensure transfers don't get interrupted in the middle

It probably should be part of .suspend_noirq() also.

> 
> 2) use a .suspend_noirq callback in i2c_bus_type.pm to reject and WARN
> about transfers still going on in that phase
> 
> -> this ensures that buggy drivers are caught
> 
> 3) write some documentation about our findings / assumptions /
> recommendations to a file in Documentation/i2c/
> 
> -> this ensures we won't forget why we did things like they are ;)
> 
> ?

Sry, for delayed reply. It sounds good.

-- 
regards,
-grygorii