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

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

[Wolfram Sang]

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. */
-- 
2.19.1

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

[Geert Uytterhoeven]

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

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Peter Zijlstra]

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?

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 3784 bytes --]

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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Peter Zijlstra]

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?

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1032 bytes --]


> > "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)


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Peter Zijlstra]

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.

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 360 bytes --]


> 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!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]