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@xxxxxxxxxxxxxxxxxxxx> > > --- > > > > 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
Attachment:
signature.asc
Description: PGP signature