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 >
