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

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

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);
>   }
>   
>

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.

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?

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.

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

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.

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.

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

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

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

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;

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

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
>

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.

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.