Re: [RFC PATCH] i2c: remove use of in_atomic()

[Wolfram Sang]

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