[01/12] i2c: remove use of in_atomic()

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
commit b7a3670131c7 ("i2c-pxa: Add polling transfer"). Its commit
message says explicitly it was added "for cases where I2C transactions
have to occur at times interrup[t]s 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.

After a discussion with Peter Zijlstra[2], we came up with a better set
of conditionals to match the use 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). This is why we add a
helper for it.

[1] https://lwn.net/Articles/274695/
[2] http://patchwork.ozlabs.org/patch/1067437/

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/i2c-core-base.c |  2 +-
 drivers/i2c/i2c-core.h      | 10 ++++++++++
 2 files changed, 11 insertions(+), 1 deletion(-)

On Wed, Apr 3, 2019 at 3:43 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
> commit b7a3670131c7 ("i2c-pxa: Add polling transfer"). Its commit
> message says explicitly it was added "for cases where I2C transactions
> have to occur at times interrup[t]s 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.
>
> After a discussion with Peter Zijlstra[2], we came up with a better set
> of conditionals to match the use 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). This is why we add a
> helper for it.
>
> [1] https://lwn.net/Articles/274695/
> [2] http://patchwork.ozlabs.org/patch/1067437/
>

Reviewed-by Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  drivers/i2c/i2c-core-base.c |  2 +-
>  drivers/i2c/i2c-core.h      | 10 ++++++++++
>  2 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 38af18645133..f8502064cd6b 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 (i2c_in_atomic_xfer_mode()) {
>                 ret = i2c_trylock_bus(adap, I2C_LOCK_SEGMENT);
>                 if (!ret)
>                         /* I2C activity is ongoing. */
> diff --git a/drivers/i2c/i2c-core.h b/drivers/i2c/i2c-core.h
> index 37576f50fe20..9d8526415b26 100644
> --- a/drivers/i2c/i2c-core.h
> +++ b/drivers/i2c/i2c-core.h
> @@ -29,6 +29,16 @@ extern int           __i2c_first_dynamic_bus_num;
>
>  int i2c_check_7bit_addr_validity_strict(unsigned short addr);
>
> +/*
> + * We only allow atomic transfers for very late communication, e.g. to send
> + * the powerdown command to a PMIC. Atomic transfers are a corner case and not
> + * for generic use!
> + */
> +static inline bool i2c_in_atomic_xfer_mode(void)
> +{
> +       return system_state > SYSTEM_RUNNING && irqs_disabled();
> +}
> +
>  #ifdef CONFIG_ACPI
>  const struct acpi_device_id *
>  i2c_acpi_match_device(const struct acpi_device_id *matches,
> --
> 2.11.0
>

Hi Wolfram,

On Wed, Apr 03, 2019 at 02:40:08PM +0200, 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
> commit b7a3670131c7 ("i2c-pxa: Add polling transfer"). Its commit
> message says explicitly it was added "for cases where I2C transactions
> have to occur at times interrup[t]s 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.
> 
> After a discussion with Peter Zijlstra[2], we came up with a better set
> of conditionals to match the use 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). This is why we add a
> helper for it.
> 
> [1] https://lwn.net/Articles/274695/
> [2] http://patchwork.ozlabs.org/patch/1067437/
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Tested-by: Stefan Lengfeld <contact@stefanchrist.eu>

snipped

> diff --git a/drivers/i2c/i2c-core.h b/drivers/i2c/i2c-core.h
> index 37576f50fe20..9d8526415b26 100644
> --- a/drivers/i2c/i2c-core.h
> +++ b/drivers/i2c/i2c-core.h
> @@ -29,6 +29,16 @@ extern int		__i2c_first_dynamic_bus_num;
>  
>  int i2c_check_7bit_addr_validity_strict(unsigned short addr);
>  
> +/*
> + * We only allow atomic transfers for very late communication, e.g. to send
> + * the powerdown command to a PMIC. Atomic transfers are a corner case and not
> + * for generic use!
> + */
> +static inline bool i2c_in_atomic_xfer_mode(void)
> +{
> +	return system_state > SYSTEM_RUNNING && irqs_disabled();
> +}

I agree that this code is a lot better than the previous
'irqs_disabled() || in_atomic()'. It also makes clear that the atomic
I2C transfers is only meant for late shutdown I2C communcation.


After I read the article https://lwn.net/Articles/274695/ again I
nevertheless cannot really say whether the usage of 'irqs_disabled()' is
the theoretical correct approach. The article states explicitly that
only the caller can really decided whether the operation should be
atomic or not.

Recap from previous discussions:

But then you have to patch every call site to use either a new function
like 'i2c_transfer_atomic()' or the extend I2C message flags. And mostly
also supported this trough regmap and maybe other translation layers,
which seems unpractical, may breaking existing usages and maybe not
worth the hassle.

For me this patch seems good and I don't know better.

Kind regards,
Stefan

Hi Stefan,

> Tested-by: Stefan Lengfeld <contact@stefanchrist.eu>

Thanks for your comments and testing! I will fix the issues you
mentioned and add your tags.

> > +/*
> > + * We only allow atomic transfers for very late communication, e.g. to send
> > + * the powerdown command to a PMIC. Atomic transfers are a corner case and not
> > + * for generic use!
> > + */
> > +static inline bool i2c_in_atomic_xfer_mode(void)
> > +{
> > +	return system_state > SYSTEM_RUNNING && irqs_disabled();
> > +}
> 
> I agree that this code is a lot better than the previous
> 'irqs_disabled() || in_atomic()'. It also makes clear that the atomic
> I2C transfers is only meant for late shutdown I2C communcation.
> 
> 
> After I read the article https://lwn.net/Articles/274695/ again I
> nevertheless cannot really say whether the usage of 'irqs_disabled()' is
> the theoretical correct approach. The article states explicitly that
> only the caller can really decided whether the operation should be
> atomic or not.

During the discussion with Peter, he stated we need irqs_disabled()
because 'system_state > SYSTEM_RUNNING' alone won't do.

> Recap from previous discussions:
> 
> But then you have to patch every call site to use either a new function
> like 'i2c_transfer_atomic()' or the extend I2C message flags. And mostly
> also supported this trough regmap and maybe other translation layers,
> which seems unpractical, may breaking existing usages and maybe not
> worth the hassle.

Yes, I kinda gave up on white-listing late I2C transfers. My hope is
that not too many drivers will have an atomic callback, so the WARN will
trigger often enough to find late transfers which are inappropriate.

Another idea just popping up: Maybe we can improve that even further by
first globally disabling atomic transfers. Drivers knowing they need
this can then call an I2C core helper to enable them (again globally).
Still not perfect as some unwanted late I2C transfers from another
driver could slip through, but this should be rare enough. The pro-side
is we will detect more unwanted late transfers if support for them is
default off. It should be noted that "disabling" means keeping the old
behaviour which is: we try the regular transfer but complain about it.
Only enabling atomic will make the core quiet.

Regards,

   Wolfram