LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Grygorii Strashko <grygorii.strashko@ti.com>
To: Wolfram Sang <wsa@the-dreams.de>
Cc: Baolin Wang <baolin.wang@linaro.org>,
	Mark Brown <broonie@kernel.org>, <linux-i2c@vger.kernel.org>,
	LKML <linux-kernel@vger.kernel.org>
Subject: Re: I2C PM overhaul needed? (Re: [PATCH 1/2] i2c: sprd: Prevent i2c accesses after suspend is called)
Date: Fri, 11 May 2018 10:14:23 -0500	[thread overview]
Message-ID: <ee6cb844-733b-dc5f-4ba1-65b692e75946@ti.com> (raw)
In-Reply-To: <20180508163221.2slrtg3cidvpj7g2@ninjato>



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

      parent reply	other threads:[~2018-05-11 15:14 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-04-09  6:40 [PATCH 1/2] i2c: sprd: Prevent i2c accesses after suspend is called Baolin Wang
2018-04-09  6:40 ` [PATCH 2/2] i2c: sprd: Fix the i2c count issue Baolin Wang
2018-04-27 12:14   ` Wolfram Sang
2018-04-09 20:56 ` [PATCH 1/2] i2c: sprd: Prevent i2c accesses after suspend is called Grygorii Strashko
2018-04-10  8:08   ` Baolin Wang
2018-04-27 12:14 ` Wolfram Sang
2018-05-02  3:27   ` Baolin Wang
2018-05-02  5:23     ` Wolfram Sang
2018-05-02  5:48       ` Baolin Wang
2018-05-03 16:26         ` Grygorii Strashko
2018-05-04 12:24           ` I2C PM overhaul needed? (Re: [PATCH 1/2] i2c: sprd: Prevent i2c accesses after suspend is called) Wolfram Sang
2018-05-05  1:54             ` Mark Brown
2018-05-05  8:32               ` Wolfram Sang
2018-05-09  8:18                 ` Mark Brown
2018-05-07 17:48             ` Grygorii Strashko
2018-05-08 16:32               ` Wolfram Sang
2018-05-08 18:31                 ` Peter Rosin
2018-05-11 15:14                 ` Grygorii Strashko [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ee6cb844-733b-dc5f-4ba1-65b692e75946@ti.com \
    --to=grygorii.strashko@ti.com \
    --cc=baolin.wang@linaro.org \
    --cc=broonie@kernel.org \
    --cc=linux-i2c@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=wsa@the-dreams.de \
    --subject='Re: I2C PM overhaul needed? (Re: [PATCH 1/2] i2c: sprd: Prevent i2c accesses after suspend is called)' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).