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
