| Message ID | 20190327211256.17232-1-wsa+renesas@sang-engineering.com |
|---|---|
| State | RFC |
| Headers | show |
| Series | [RFC] i2c: remove use of in_atomic() | expand |
Hi Wolfram, On Wed, Mar 27, 2019 at 10:13 PM Wolfram Sang <wsa+renesas@sang-engineering.com> wrote: > Commit cea443a81c9c ("i2c: Support i2c_transfer in atomic contexts") > added in_atomic() to the I2C core. However, the use of in_atomic() > outside of core kernel code is discouraged and was already[1] when this > code was added in early 2008. The above commit was a preparation for > b7a3670131c7 ("i2c-pxa: Add polling transfer"). Its commit message says > explicitly it was added "for cases where I2C transactions have to occur > at times interrups are disabled". So, the intention was 'disabled > interrupts'. This matches the use cases for atomic I2C transfers I have > seen so far: very late communication (mostly to a PMIC) to powerdown or > reboot the system. For those cases, interrupts are disabled then. It > doesn't seem that in_atomic() adds value. > > Note that only ~10 out of ~120 bus master drivers support atomic > transfers, mostly by polling always when no irq is supplied. A generic > I2C client driver cannot assume support for atomic transfers. This is > currently a platform-dependent corner case. > > The I2C core will soon gain an extra callback into bus drivers > especially for atomic transfers to make them more generic. The code > deciding which transfer to use (atomic/non-atomic) should mimic the > behaviour which locking to use (trylock/lock). Because I don't want to > add more in_atomic() to the I2C core, this patch simply removes it. > > [1] https://lwn.net/Articles/274695/ > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> LGTM, so: Ackeded-by: Geert Uytterhoeven <geert+renesas@glider.be> But please wait for Peter and/or Frederic to give their fiat, too. Gr{oetje,eeting}s, Geert
On Wed, Mar 27, 2019 at 10:12:56PM +0100, Wolfram Sang wrote: > Commit cea443a81c9c ("i2c: Support i2c_transfer in atomic contexts") > added in_atomic() to the I2C core. However, the use of in_atomic() > outside of core kernel code is discouraged and was already[1] when this > code was added in early 2008. The above commit was a preparation for > b7a3670131c7 ("i2c-pxa: Add polling transfer"). Its commit message says > explicitly it was added "for cases where I2C transactions have to occur > at times interrups are disabled". So, the intention was 'disabled > interrupts'. This matches the use cases for atomic I2C transfers I have > seen so far: very late communication (mostly to a PMIC) to powerdown or > reboot the system. For those cases, interrupts are disabled then. It > doesn't seem that in_atomic() adds value. > > Note that only ~10 out of ~120 bus master drivers support atomic > transfers, mostly by polling always when no irq is supplied. A generic > I2C client driver cannot assume support for atomic transfers. This is > currently a platform-dependent corner case. > > The I2C core will soon gain an extra callback into bus drivers > especially for atomic transfers to make them more generic. The code > deciding which transfer to use (atomic/non-atomic) should mimic the > behaviour which locking to use (trylock/lock). Because I don't want to > add more in_atomic() to the I2C core, this patch simply removes it. > > [1] https://lwn.net/Articles/274695/ > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > --- > > So, I had to dive into this in_atomic() topic and this is what I > concluded. I don't see any reasonable constellation where this could > cause a regression, but I am all open for missing something and being > pointed to it. This is why the patch is RFC. I'd really welcome > comments. Thanks! > > > drivers/i2c/i2c-core-base.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c > index 38af18645133..943bebeec3ed 100644 > --- a/drivers/i2c/i2c-core-base.c > +++ b/drivers/i2c/i2c-core-base.c > @@ -1946,7 +1946,7 @@ int i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num) > * one (discarding status on the second message) or errno > * (discarding status on the first one). > */ > - if (in_atomic() || irqs_disabled()) { > + if (irqs_disabled()) { > ret = i2c_trylock_bus(adap, I2C_LOCK_SEGMENT); > if (!ret) > /* I2C activity is ongoing. */ So I know absolutely nothing about i2c, except that it is supposedly fsck all slow. In that context, busy-spinning for i2c completions seems like a terrible idea, _esp_ in atomic contexts. I did a quick grep for trylock_bus() and found i2c_mux_trylock_bus() which uses rt_mutex_trylock and therefore the calling context must already exclude IRQs and NMIs and the like. That leaves task context with preemption/IRQs disabled. Of that, you retain the IRQs disabled test, which is by far the worst possible condition to spin-wait in. Why must we allow i2c usage with IRQs disabled? Just say NO?
Hi Peter, thanks for your answer! On Mon, Apr 01, 2019 at 12:47:56PM +0200, Peter Zijlstra wrote: > On Wed, Mar 27, 2019 at 10:12:56PM +0100, Wolfram Sang wrote: > > Commit cea443a81c9c ("i2c: Support i2c_transfer in atomic contexts") > > added in_atomic() to the I2C core. However, the use of in_atomic() > > outside of core kernel code is discouraged and was already[1] when this > > code was added in early 2008. The above commit was a preparation for > > b7a3670131c7 ("i2c-pxa: Add polling transfer"). Its commit message says > > explicitly it was added "for cases where I2C transactions have to occur > > at times interrups are disabled". So, the intention was 'disabled > > interrupts'. This matches the use cases for atomic I2C transfers I have > > seen so far: very late communication (mostly to a PMIC) to powerdown or > > reboot the system. For those cases, interrupts are disabled then. It > > doesn't seem that in_atomic() adds value. > > > > Note that only ~10 out of ~120 bus master drivers support atomic > > transfers, mostly by polling always when no irq is supplied. A generic > > I2C client driver cannot assume support for atomic transfers. This is > > currently a platform-dependent corner case. > > > > The I2C core will soon gain an extra callback into bus drivers > > especially for atomic transfers to make them more generic. The code > > deciding which transfer to use (atomic/non-atomic) should mimic the > > behaviour which locking to use (trylock/lock). Because I don't want to > > add more in_atomic() to the I2C core, this patch simply removes it. > > > > [1] https://lwn.net/Articles/274695/ > > > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > > --- > > > > So, I had to dive into this in_atomic() topic and this is what I > > concluded. I don't see any reasonable constellation where this could > > cause a regression, but I am all open for missing something and being > > pointed to it. This is why the patch is RFC. I'd really welcome > > comments. Thanks! > > > > > > drivers/i2c/i2c-core-base.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c > > index 38af18645133..943bebeec3ed 100644 > > --- a/drivers/i2c/i2c-core-base.c > > +++ b/drivers/i2c/i2c-core-base.c > > @@ -1946,7 +1946,7 @@ int i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num) > > * one (discarding status on the second message) or errno > > * (discarding status on the first one). > > */ > > - if (in_atomic() || irqs_disabled()) { > > + if (irqs_disabled()) { > > ret = i2c_trylock_bus(adap, I2C_LOCK_SEGMENT); > > if (!ret) > > /* I2C activity is ongoing. */ > > So I know absolutely nothing about i2c, except that it is supposedly > fsck all slow. > > In that context, busy-spinning for i2c completions seems like a terrible > idea, _esp_ in atomic contexts. > > I did a quick grep for trylock_bus() and found i2c_mux_trylock_bus() > which uses rt_mutex_trylock and therefore the calling context must > already exclude IRQs and NMIs and the like. > > That leaves task context with preemption/IRQs disabled. Of that, you > retain the IRQs disabled test, which is by far the worst possible > condition to spin-wait in. > > Why must we allow i2c usage with IRQs disabled? Just say NO? I'd love to. But quoting my patch description: "This matches the use cases for atomic I2C transfers I have seen so far: very late communication (mostly to a PMIC) to powerdown or reboot the system." And yes, I would never recommend a HW design to use I2C for shutting down/rebooting. But such HW is out there. Regards, Wolfram
On Mon, Apr 01, 2019 at 01:13:24PM +0200, Wolfram Sang wrote: > > Why must we allow i2c usage with IRQs disabled? Just say NO? > > I'd love to. But quoting my patch description: > > "This matches the use cases for atomic I2C transfers I have seen so far: > very late communication (mostly to a PMIC) to powerdown or reboot the > system." Ah, sorry, I missed that. > And yes, I would never recommend a HW design to use I2C for shutting > down/rebooting. But such HW is out there. Can we then make the whole thing conditional on: system_state > SYSTEM_RUNNING Such that we're sure to never trigger this under any other conditions?
> > "This matches the use cases for atomic I2C transfers I have seen so far: > > very late communication (mostly to a PMIC) to powerdown or reboot the > > system." > > Ah, sorry, I missed that. > > > And yes, I would never recommend a HW design to use I2C for shutting > > down/rebooting. But such HW is out there. > > Can we then make the whole thing conditional on: > > system_state > SYSTEM_RUNNING > > Such that we're sure to never trigger this under any other conditions? Oh, we can for sure modify the code to something else. Actually, this is why I was calling out to you. I was never comfortable with the old 'in_atomic() || irqs_disabled()' code, but wasn't sure what would be an adequate replacement which will not risk regressions. The above condition makes much more sense to me and is also much more readable. Can it simply replace irqs_disabled()? Are interrupts already disabled for system_state > SYSTEM_RUNNING? (I got a bit lost in the code paths when trying to figure it out)
On Mon, Apr 01, 2019 at 01:54:14PM +0200, Wolfram Sang wrote: > > > > "This matches the use cases for atomic I2C transfers I have seen so far: > > > very late communication (mostly to a PMIC) to powerdown or reboot the > > > system." > > > > Ah, sorry, I missed that. > > > > > And yes, I would never recommend a HW design to use I2C for shutting > > > down/rebooting. But such HW is out there. > > > > Can we then make the whole thing conditional on: > > > > system_state > SYSTEM_RUNNING > > > > Such that we're sure to never trigger this under any other conditions? > > Oh, we can for sure modify the code to something else. Actually, this is > why I was calling out to you. I was never comfortable with the old > 'in_atomic() || irqs_disabled()' code, but wasn't sure what would be an > adequate replacement which will not risk regressions. > > The above condition makes much more sense to me and is also much more > readable. Can it simply replace irqs_disabled()? Are interrupts already > disabled for system_state > SYSTEM_RUNNING? (I got a bit lost in the > code paths when trying to figure it out) Looking at kernel_restart_prepare(), we set SYSTEM_REBOOT in normal context, specifically it just did a blocking notifier call. So no, you'll have to combine the two.
> Looking at kernel_restart_prepare(), we set SYSTEM_REBOOT in normal > context, specifically it just did a blocking notifier call. > > So no, you'll have to combine the two. I see now. Combining the two is still readable, but I'll abstract this away into some 'i2c_is_atomic_xfer_mode()' helper with proper explanation. Thank you for the input!
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c index 38af18645133..943bebeec3ed 100644 --- a/drivers/i2c/i2c-core-base.c +++ b/drivers/i2c/i2c-core-base.c @@ -1946,7 +1946,7 @@ int i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num) * one (discarding status on the second message) or errno * (discarding status on the first one). */ - if (in_atomic() || irqs_disabled()) { + if (irqs_disabled()) { ret = i2c_trylock_bus(adap, I2C_LOCK_SEGMENT); if (!ret) /* I2C activity is ongoing. */
Commit cea443a81c9c ("i2c: Support i2c_transfer in atomic contexts") added in_atomic() to the I2C core. However, the use of in_atomic() outside of core kernel code is discouraged and was already[1] when this code was added in early 2008. The above commit was a preparation for b7a3670131c7 ("i2c-pxa: Add polling transfer"). Its commit message says explicitly it was added "for cases where I2C transactions have to occur at times interrups are disabled". So, the intention was 'disabled interrupts'. This matches the use cases for atomic I2C transfers I have seen so far: very late communication (mostly to a PMIC) to powerdown or reboot the system. For those cases, interrupts are disabled then. It doesn't seem that in_atomic() adds value. Note that only ~10 out of ~120 bus master drivers support atomic transfers, mostly by polling always when no irq is supplied. A generic I2C client driver cannot assume support for atomic transfers. This is currently a platform-dependent corner case. The I2C core will soon gain an extra callback into bus drivers especially for atomic transfers to make them more generic. The code deciding which transfer to use (atomic/non-atomic) should mimic the behaviour which locking to use (trylock/lock). Because I don't want to add more in_atomic() to the I2C core, this patch simply removes it. [1] https://lwn.net/Articles/274695/ Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> --- So, I had to dive into this in_atomic() topic and this is what I concluded. I don't see any reasonable constellation where this could cause a regression, but I am all open for missing something and being pointed to it. This is why the patch is RFC. I'd really welcome comments. Thanks! drivers/i2c/i2c-core-base.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)