Re: I2C PM overhaul needed? (Re: [PATCH 1/2] i2c: sprd: Prevent i2c accesses after suspend is called)
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 1 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)
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 1 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)
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. signature.asc Description: PGP signature
Re: I2C PM overhaul needed? (Re: [PATCH 1/2] i2c: sprd: Prevent i2c accesses after suspend is called)
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. signature.asc Description: PGP signature
Re: I2C PM overhaul needed? (Re: [PATCH 1/2] i2c: sprd: Prevent i2c accesses after suspend is called)
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 1 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)
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 1 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, 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 1 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 signature.asc Description: PGP signature
Re: I2C PM overhaul needed? (Re: [PATCH 1/2] i2c: sprd: Prevent i2c accesses after suspend is called)
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 1 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 signature.asc Description: PGP signature
Re: I2C PM overhaul needed? (Re: [PATCH 1/2] i2c: sprd: Prevent i2c accesses after suspend is called)
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 1 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
Re: I2C PM overhaul needed? (Re: [PATCH 1/2] i2c: sprd: Prevent i2c accesses after suspend is called)
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 1 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
Re: I2C PM overhaul needed? (Re: [PATCH 1/2] i2c: sprd: Prevent i2c accesses after suspend is called)
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 signature.asc Description: PGP signature
Re: I2C PM overhaul needed? (Re: [PATCH 1/2] i2c: sprd: Prevent i2c accesses after suspend is called)
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 signature.asc Description: PGP signature
Re: I2C PM overhaul needed? (Re: [PATCH 1/2] i2c: sprd: Prevent i2c accesses after suspend is called)
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... signature.asc Description: PGP signature
Re: I2C PM overhaul needed? (Re: [PATCH 1/2] i2c: sprd: Prevent i2c accesses after suspend is called)
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... signature.asc Description: PGP signature
I2C PM overhaul needed? (Re: [PATCH 1/2] i2c: sprd: Prevent i2c accesses after suspend is called)
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 signature.asc Description: PGP signature
I2C PM overhaul needed? (Re: [PATCH 1/2] i2c: sprd: Prevent i2c accesses after suspend is called)
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 signature.asc Description: PGP signature
