diff mbox series

[RFC,1/4] i2c: allow drivers to announce that they are IRQ safe

Message ID 20180813211614.16440-2-contact@stefanchrist.eu (mailing list archive)
State New, archived
Headers show
Series I2C writes with interrupts disabled | expand

Commit Message

Stefan Lengfeld Aug. 13, 2018, 9:16 p.m. UTC
Like the PM subsystem routine pm_runtime_irq_safe() and flag 'irq_safe'
add a similar function i2c_adapter_irq_safe() to the I2C core. A driver
should be able to announce whether his transfer implementations are safe
to be called in IRQ disabled or atomic contexts, also called polling
mode or sleep free operation.

Making I2C transfers in atomic context is sometimes needed, e.g.  for
reboot handlers.

Every driver should be able to declare explicitly whether IRQ disabled
operation is supported or not. When you try make a I2C transfer in
atomic contexts, it's already hard enough to ensure that every code path
through the kernel is sleep-free. So give the curious developer a strong
hint whether a driver supports atomic operation or not. Fail early
instead of hoping that the LOCKDEP framework noticed the programming
error.

TODOs:
- checkpatch complains hat in_atomic() should not be used in driver code

Signed-off-by: Stefan Lengfeld <contact@stefanchrist.eu>
---
 drivers/i2c/i2c-core-base.c |  8 ++++++++
 include/linux/i2c.h         | 16 ++++++++++++++++
 2 files changed, 24 insertions(+)

Comments

Andy Shevchenko Aug. 14, 2018, 11:20 a.m. UTC | #1
On Tue, Aug 14, 2018 at 12:16 AM, Stefan Lengfeld
<contact@stefanchrist.eu> wrote:
> Like the PM subsystem routine pm_runtime_irq_safe() and flag 'irq_safe'

No, it seems unlike as in PM. PM has one crucial detail which is done
together with enabling IRQ safety.

> add a similar function i2c_adapter_irq_safe() to the I2C core. A driver
> should be able to announce whether his transfer implementations are safe
> to be called in IRQ disabled or atomic contexts, also called polling
> mode or sleep free operation.
>
> Making I2C transfers in atomic context is sometimes needed, e.g.  for
> reboot handlers.
>
> Every driver should be able to declare explicitly whether IRQ disabled
> operation is supported or not. When you try make a I2C transfer in
> atomic contexts, it's already hard enough to ensure that every code path
> through the kernel is sleep-free. So give the curious developer a strong
> hint whether a driver supports atomic operation or not. Fail early
> instead of hoping that the LOCKDEP framework noticed the programming
> error.
Tony Lindgren Aug. 20, 2018, 2:54 p.m. UTC | #2
Hi,

* Stefan Lengfeld <contact@stefanchrist.eu> [180813 21:19]:
> Making I2C transfers in atomic context is sometimes needed, e.g.  for
> reboot handlers.

Just a generic comment related to where this might be needed.
Let's try to avoid the slippery slope of making i2c work super
early.. That will just make the race to the bottom initcall
level and defferd probe situation worse :)

It's best to initialize everything late instead of trying to
initialize something early except for what's needed for system
timers and interrupts.

So from that point of view I don't have any issues limiting
atomic i2c to shutdown time only.

Regards,

Tony
Stefan Lengfeld Aug. 21, 2018, 6:32 p.m. UTC | #3
Hi Tony,

> * Stefan Lengfeld <contact@stefanchrist.eu> [180813 21:19]:
> > Making I2C transfers in atomic context is sometimes needed, e.g.  for
> > reboot handlers.
> 
> Just a generic comment related to where this might be needed.
> Let's try to avoid the slippery slope of making i2c work super
> early.. That will just make the race to the bottom initcall
> level and defferd probe situation worse :)
> 
> It's best to initialize everything late instead of trying to
> initialize something early except for what's needed for system
> timers and interrupts.
> 
> So from that point of view I don't have any issues limiting
> atomic i2c to shutdown time only.

I agree to that. A solution should only/mainly focus on reboot and
shutdown handlers. Getting I2C transfers in atomic/disabled IRQ contexts
is already hard enough there :-)

Kind regards,
Stefan
Stefan Lengfeld Aug. 21, 2018, 7:05 p.m. UTC | #4
Hi Andy,

On Tue, Aug 14, 2018 at 02:20:26PM +0300, Andy Shevchenko wrote:
> On Tue, Aug 14, 2018 at 12:16 AM, Stefan Lengfeld
> <contact@stefanchrist.eu> wrote:
> > Like the PM subsystem routine pm_runtime_irq_safe() and flag 'irq_safe'
> 
> No, it seems unlike as in PM. PM has one crucial detail which is done
> together with enabling IRQ safety.
> 
> > add a similar function i2c_adapter_irq_safe() to the I2C core. A driver
> > should be able to announce whether his transfer implementations are safe
> > to be called in IRQ disabled or atomic contexts, also called polling
> > mode or sleep free operation.

Ah, ok. `i2c_adapter_irq_safe()` is a bad idea. Thanks for the feedback.

Then I vote for extending the 'struct i2c_algorithm' with an extra
callback 'master_xfer_irqless'. The idea was already proposed here.

This approach has the same information value than a flag 'irq_safe' and
is even a bit cleaner, since it's more a compile time thing than a
runtime setting.

Kind regards,
Stefan
Wolfram Sang Sept. 1, 2018, 9:57 p.m. UTC | #5
On Mon, Aug 20, 2018 at 07:54:30AM -0700, Tony Lindgren wrote:
> Hi,
> 
> * Stefan Lengfeld <contact@stefanchrist.eu> [180813 21:19]:
> > Making I2C transfers in atomic context is sometimes needed, e.g.  for
> > reboot handlers.
> 
> Just a generic comment related to where this might be needed.
> Let's try to avoid the slippery slope of making i2c work super
> early.. That will just make the race to the bottom initcall
> level and defferd probe situation worse :)
> 
> It's best to initialize everything late instead of trying to
> initialize something early except for what's needed for system
> timers and interrupts.
> 
> So from that point of view I don't have any issues limiting
> atomic i2c to shutdown time only.

I buy this.

There was only one occasion I recall where early, irqless I2C access was
desired and I can't even find the discussion now. The use case which
came back repeatedly is shutdown/reboot via PMIC.

Let's stick to that unless someone speaks up now having a good reason
for the early case.
Wolfram Sang Sept. 3, 2018, 5:21 p.m. UTC | #6
> Then I vote for extending the 'struct i2c_algorithm' with an extra
> callback 'master_xfer_irqless'. The idea was already proposed here.

Yes, I proposed the idea a few times and now I cooked up an RFC.
Only build tested and merely a starter for discussion.

Let me know what you think, people.

===

From: Wolfram Sang <wsa@the-dreams.de>
Subject: [RFC PATCH] i2c: introduce master_xfer_irqless callback

We had the request to access devices very late when interrupts are not
available anymore multiple times now. Mostly to prepare shutdown or
reboot. Allow adapters to specify a specific callback for this case.
Note that we fall back to the generic master_xfer callback if this new
irqless one is not present. This is intentional to preserve the previous
behaviour and avoid regressions. Because there are drivers not using
interrupts or because it might have worked "accidently" before.

Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
---
 drivers/i2c/i2c-core-base.c |  6 +++++-
 include/linux/i2c.h         | 10 +++++++---
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 9ee9a15e7134..38dc7125293c 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -1887,7 +1887,11 @@ int __i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 	/* Retry automatically on arbitration loss */
 	orig_jiffies = jiffies;
 	for (ret = 0, try = 0; try <= adap->retries; try++) {
-		ret = adap->algo->master_xfer(adap, msgs, num);
+		if (in_atomic() || irqs_disabled())
+			ret = adap->algo->master_xfer_irqless(adap, msgs, num);
+		else
+			ret = adap->algo->master_xfer(adap, msgs, num);
+
 		if (ret != -EAGAIN)
 			break;
 		if (time_after(jiffies, orig_jiffies + adap->timeout))
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 65b4eaed1d96..11e615123bd0 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -498,6 +498,8 @@ i2c_register_board_info(int busnum, struct i2c_board_info const *info,
  * @master_xfer: Issue a set of i2c transactions to the given I2C adapter
  *   defined by the msgs array, with num messages available to transfer via
  *   the adapter specified by adap.
+ * @master_xfer_irqless: same as master_xfer. Yet, not using any interrupts
+ *   so e.g. PMICs can be accessed very late before shutdown
  * @smbus_xfer: Issue smbus transactions to the given I2C adapter. If this
  *   is not present, then the bus layer will try and convert the SMBus calls
  *   into I2C transfers instead.
@@ -511,9 +513,9 @@ i2c_register_board_info(int busnum, struct i2c_board_info const *info,
  * be addressed using the same bus algorithms - i.e. bit-banging or the PCF8584
  * to name two of the most common.
  *
- * The return codes from the @master_xfer field should indicate the type of
- * error code that occurred during the transfer, as documented in the kernel
- * Documentation file Documentation/i2c/fault-codes.
+ * The return codes from the @master_xfer{_irqless} field should indicate the
+ * type of error code that occurred during the transfer, as documented in the
+ * Kernel Documentation file Documentation/i2c/fault-codes.
  */
 struct i2c_algorithm {
 	/* If an adapter algorithm can't do I2C-level access, set master_xfer
@@ -524,6 +526,8 @@ struct i2c_algorithm {
 	   processed, or a negative value on error */
 	int (*master_xfer)(struct i2c_adapter *adap, struct i2c_msg *msgs,
 			   int num);
+	int (*master_xfer_irqless)(struct i2c_adapter *adap,
+				   struct i2c_msg *msgs, int num);
 	int (*smbus_xfer) (struct i2c_adapter *adap, u16 addr,
 			   unsigned short flags, char read_write,
 			   u8 command, int size, union i2c_smbus_data *data);
Andy Shevchenko Sept. 4, 2018, 12:26 p.m. UTC | #7
On Mon, Sep 3, 2018 at 8:21 PM Wolfram Sang <wsa@the-dreams.de> wrote:
>
>
> > Then I vote for extending the 'struct i2c_algorithm' with an extra
> > callback 'master_xfer_irqless'. The idea was already proposed here.
>
> Yes, I proposed the idea a few times and now I cooked up an RFC.
> Only build tested and merely a starter for discussion.
>

> Let me know what you think, people.

LGTM.
Though I would expect that call back optional, so, it means that I2C
core, perhaps, may put some generic one as a stub.

>
> ===
>
> From: Wolfram Sang <wsa@the-dreams.de>
> Subject: [RFC PATCH] i2c: introduce master_xfer_irqless callback
>
> We had the request to access devices very late when interrupts are not
> available anymore multiple times now. Mostly to prepare shutdown or
> reboot. Allow adapters to specify a specific callback for this case.
> Note that we fall back to the generic master_xfer callback if this new
> irqless one is not present. This is intentional to preserve the previous
> behaviour and avoid regressions. Because there are drivers not using
> interrupts or because it might have worked "accidently" before.
>
> Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
> ---
>  drivers/i2c/i2c-core-base.c |  6 +++++-
>  include/linux/i2c.h         | 10 +++++++---
>  2 files changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 9ee9a15e7134..38dc7125293c 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -1887,7 +1887,11 @@ int __i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
>         /* Retry automatically on arbitration loss */
>         orig_jiffies = jiffies;
>         for (ret = 0, try = 0; try <= adap->retries; try++) {
> -               ret = adap->algo->master_xfer(adap, msgs, num);
> +               if (in_atomic() || irqs_disabled())
> +                       ret = adap->algo->master_xfer_irqless(adap, msgs, num);
> +               else
> +                       ret = adap->algo->master_xfer(adap, msgs, num);
> +
>                 if (ret != -EAGAIN)
>                         break;
>                 if (time_after(jiffies, orig_jiffies + adap->timeout))
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index 65b4eaed1d96..11e615123bd0 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -498,6 +498,8 @@ i2c_register_board_info(int busnum, struct i2c_board_info const *info,
>   * @master_xfer: Issue a set of i2c transactions to the given I2C adapter
>   *   defined by the msgs array, with num messages available to transfer via
>   *   the adapter specified by adap.
> + * @master_xfer_irqless: same as master_xfer. Yet, not using any interrupts
> + *   so e.g. PMICs can be accessed very late before shutdown
>   * @smbus_xfer: Issue smbus transactions to the given I2C adapter. If this
>   *   is not present, then the bus layer will try and convert the SMBus calls
>   *   into I2C transfers instead.
> @@ -511,9 +513,9 @@ i2c_register_board_info(int busnum, struct i2c_board_info const *info,
>   * be addressed using the same bus algorithms - i.e. bit-banging or the PCF8584
>   * to name two of the most common.
>   *
> - * The return codes from the @master_xfer field should indicate the type of
> - * error code that occurred during the transfer, as documented in the kernel
> - * Documentation file Documentation/i2c/fault-codes.
> + * The return codes from the @master_xfer{_irqless} field should indicate the
> + * type of error code that occurred during the transfer, as documented in the
> + * Kernel Documentation file Documentation/i2c/fault-codes.
>   */
>  struct i2c_algorithm {
>         /* If an adapter algorithm can't do I2C-level access, set master_xfer
> @@ -524,6 +526,8 @@ struct i2c_algorithm {
>            processed, or a negative value on error */
>         int (*master_xfer)(struct i2c_adapter *adap, struct i2c_msg *msgs,
>                            int num);
> +       int (*master_xfer_irqless)(struct i2c_adapter *adap,
> +                                  struct i2c_msg *msgs, int num);
>         int (*smbus_xfer) (struct i2c_adapter *adap, u16 addr,
>                            unsigned short flags, char read_write,
>                            u8 command, int size, union i2c_smbus_data *data);
> --
> 2.18.0
>
>
Wolfram Sang Sept. 4, 2018, 12:33 p.m. UTC | #8
> LGTM.
> Though I would expect that call back optional, so, it means that I2C
> core, perhaps, may put some generic one as a stub.

Right, stupid me. After some many variations, I missed to add one
condition to this version. It should have been something like the
following:

> > -               ret = adap->algo->master_xfer(adap, msgs, num);
> > +               if (in_atomic() || irqs_disabled())

	if (adap->algo->master_xfer_irqless && (in_atomic() || irqs_disabled()))

> > +                       ret = adap->algo->master_xfer_irqless(adap, msgs, num);
> > +               else
> > +                       ret = adap->algo->master_xfer(adap, msgs, num);
> > +
> >                 if (ret != -EAGAIN)
> >                         break;
Peter Rosin Sept. 4, 2018, 12:44 p.m. UTC | #9
On 2018-09-03 19:21, Wolfram Sang wrote:
> 
>> Then I vote for extending the 'struct i2c_algorithm' with an extra
>> callback 'master_xfer_irqless'. The idea was already proposed here.
> 
> Yes, I proposed the idea a few times and now I cooked up an RFC.
> Only build tested and merely a starter for discussion.
> 
> Let me know what you think, people.

There is still the "if (in_atomic() || irqs_disabled())" thing in
i2c_transfer(), something we did not replicate in the recent
rework of i2c_smbus_xfer()

Do we not need to do that dance in i2c_smbus_xfer() too and as apart
of that add ->smbus_xfer_irqless?

My guess was that many things that needs to be done "late" are
performed as one of the i2c_smbus_read/write... calls, but what do I
know? Or maybe I'm just too far ahead?

Cheers,
Peter
Wolfram Sang Sept. 4, 2018, 2:25 p.m. UTC | #10
> There is still the "if (in_atomic() || irqs_disabled())" thing in
> i2c_transfer(), something we did not replicate in the recent
> rework of i2c_smbus_xfer()
> 
> Do we not need to do that dance in i2c_smbus_xfer() too and as apart
> of that add ->smbus_xfer_irqless?

I'd be pragmtic here and only add it when it is needed. Nobody wanted it
so far. It would throw lockdep warnings and irq related OOPSes if
somebody uses SMBus for this. So, I'd expect we would know of a user.

> My guess was that many things that needs to be done "late" are
> performed as one of the i2c_smbus_read/write... calls, but what do I
> know? Or maybe I'm just too far ahead?

They are probably SMBus calls but my bet is that they will get emulated
to I2C transfers. SMBus controllers are more common on PC hardware and
above I'd say, and those seem to have so far their custom firmware
methods to shut down.

With all that being said, if there is someone who needs it, we can add
it later.
Andy Shevchenko Sept. 4, 2018, 4:39 p.m. UTC | #11
On Tue, Sep 4, 2018 at 3:34 PM Wolfram Sang <wsa@the-dreams.de> wrote:
>
>
> > LGTM.
> > Though I would expect that call back optional, so, it means that I2C
> > core, perhaps, may put some generic one as a stub.
>
> Right, stupid me. After some many variations, I missed to add one
> condition to this version. It should have been something like the
> following:
>
> > > -               ret = adap->algo->master_xfer(adap, msgs, num);
> > > +               if (in_atomic() || irqs_disabled())
>
>         if (adap->algo->master_xfer_irqless && (in_atomic() || irqs_disabled()))

I think something like

  if (in_atomic() || irqs_disabled()) {
      if (adap->algo->master_xfer_irqless)
             ret = adap->algo->master_xfer_irqless(adap, msgs, num);
      else
             ret = -ENOTSUPP;
  }

>
> > > +                       ret = adap->algo->master_xfer_irqless(adap, msgs, num);
> > > +               else
> > > +                       ret = adap->algo->master_xfer(adap, msgs, num);
> > > +
> > >                 if (ret != -EAGAIN)
> > >                         break;
Peter Rosin Sept. 4, 2018, 4:54 p.m. UTC | #12
On 2018-09-04 16:25, Wolfram Sang wrote:
>> There is still the "if (in_atomic() || irqs_disabled())" thing in
>> i2c_transfer(), something we did not replicate in the recent
>> rework of i2c_smbus_xfer()
>>
>> Do we not need to do that dance in i2c_smbus_xfer() too and as apart
>> of that add ->smbus_xfer_irqless?
> 
> I'd be pragmtic here and only add it when it is needed. Nobody wanted it
> so far. It would throw lockdep warnings and irq related OOPSes if
> somebody uses SMBus for this. So, I'd expect we would know of a user.
> 
>> My guess was that many things that needs to be done "late" are
>> performed as one of the i2c_smbus_read/write... calls, but what do I
>> know? Or maybe I'm just too far ahead?
> 
> They are probably SMBus calls but my bet is that they will get emulated
> to I2C transfers. SMBus controllers are more common on PC hardware and
> above I'd say, and those seem to have so far their custom firmware
> methods to shut down.

Right, but keep in mind that (normal) emulation will still go through
the unconditional locking in i2c_smbus_xfer, so that will also be a
source of problems/oopses/splats.

> With all that being said, if there is someone who needs it, we can add
> it later.

Right. I didn't mean to say that there was anything wrong per se, just
that irqless support from the I2C core was a bit incomplete.

Cheers,
Peter
Phil Reid Sept. 5, 2018, 12:48 a.m. UTC | #13
On 5/09/2018 12:39 AM, Andy Shevchenko wrote:
> On Tue, Sep 4, 2018 at 3:34 PM Wolfram Sang <wsa@the-dreams.de> wrote:
>>
>>
>>> LGTM.
>>> Though I would expect that call back optional, so, it means that I2C
>>> core, perhaps, may put some generic one as a stub.
>>
>> Right, stupid me. After some many variations, I missed to add one
>> condition to this version. It should have been something like the
>> following:
>>
>>>> -               ret = adap->algo->master_xfer(adap, msgs, num);
>>>> +               if (in_atomic() || irqs_disabled())
>>
>>          if (adap->algo->master_xfer_irqless && (in_atomic() || irqs_disabled()))
> 
> I think something like
> 
>    if (in_atomic() || irqs_disabled()) {
>        if (adap->algo->master_xfer_irqless)
>               ret = adap->algo->master_xfer_irqless(adap, msgs, num);
>        else
>               ret = -ENOTSUPP;
>    }
> 
At the moment I have an i2c gpio that is set to power off the system.
A design flaw for sure but somehow it is working and so far no problems.
Would this break that, vs falling back to the existing behaviour of
calling master_xfer as proposed.




>>
>>>> +                       ret = adap->algo->master_xfer_irqless(adap, msgs, num);
>>>> +               else
>>>> +                       ret = adap->algo->master_xfer(adap, msgs, num);
>>>> +
>>>>                  if (ret != -EAGAIN)
>>>>                          break;
> 
> 
>
Wolfram Sang Sept. 5, 2018, 10:04 a.m. UTC | #14
On Wed, Sep 05, 2018 at 08:48:23AM +0800, Phil Reid wrote:
> On 5/09/2018 12:39 AM, Andy Shevchenko wrote:
> > On Tue, Sep 4, 2018 at 3:34 PM Wolfram Sang <wsa@the-dreams.de> wrote:
> > > 
> > > 
> > > > LGTM.
> > > > Though I would expect that call back optional, so, it means that I2C
> > > > core, perhaps, may put some generic one as a stub.
> > > 
> > > Right, stupid me. After some many variations, I missed to add one
> > > condition to this version. It should have been something like the
> > > following:
> > > 
> > > > > -               ret = adap->algo->master_xfer(adap, msgs, num);
> > > > > +               if (in_atomic() || irqs_disabled())
> > > 
> > >          if (adap->algo->master_xfer_irqless && (in_atomic() || irqs_disabled()))
> > 
> > I think something like
> > 
> >    if (in_atomic() || irqs_disabled()) {
> >        if (adap->algo->master_xfer_irqless)
> >               ret = adap->algo->master_xfer_irqless(adap, msgs, num);
> >        else
> >               ret = -ENOTSUPP;
> >    }
> > 
> At the moment I have an i2c gpio that is set to power off the system.
> A design flaw for sure but somehow it is working and so far no problems.
> Would this break that, vs falling back to the existing behaviour of
> calling master_xfer as proposed.

Ack. This is exactly the reason why I wrote:

"Note that we fall back to the generic master_xfer callback if this new
irqless one is not present. This is intentional to preserve the previous
behaviour and avoid regressions. Because there are drivers not using
interrupts or because it might have worked "accidently" before."

Maybe I should mention i2c-gpio as a no-interrupt user explicitly there.
Wolfram Sang Sept. 5, 2018, 10:10 a.m. UTC | #15
> > They are probably SMBus calls but my bet is that they will get emulated
> > to I2C transfers. SMBus controllers are more common on PC hardware and
> > above I'd say, and those seem to have so far their custom firmware
> > methods to shut down.
> 
> Right, but keep in mind that (normal) emulation will still go through
> the unconditional locking in i2c_smbus_xfer, so that will also be a
> source of problems/oopses/splats.

Damn, yes, I forgot that smbus_xfer_emulated uses the unlocked
__i2c_transfer. So, you are totally right, we need this extra dance to
handle the locking. When it comes to adding smbus_xfer_irqless, I'd
still propose to wait until we have a user for it.
Stefan Lengfeld Sept. 10, 2018, 8:56 p.m. UTC | #16
Hi Wolfram,

sorry for my late reply. I was on vacation.

On Mon, Sep 03, 2018 at 07:21:48PM +0200, Wolfram Sang wrote:
> 
> > Then I vote for extending the 'struct i2c_algorithm' with an extra
> > callback 'master_xfer_irqless'. The idea was already proposed here.
> 
> Yes, I proposed the idea a few times and now I cooked up an RFC.
> Only build tested and merely a starter for discussion.
> 
> Let me know what you think, people.
> 
> ===
> 
> From: Wolfram Sang <wsa@the-dreams.de>
> Subject: [RFC PATCH] i2c: introduce master_xfer_irqless callback
> 
> We had the request to access devices very late when interrupts are not
> available anymore multiple times now. Mostly to prepare shutdown or
> reboot. Allow adapters to specify a specific callback for this case.
> Note that we fall back to the generic master_xfer callback if this new
> irqless one is not present. This is intentional to preserve the previous
> behaviour and avoid regressions. Because there are drivers not using
> interrupts or because it might have worked "accidently" before.
> 
> Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
> ---
>  drivers/i2c/i2c-core-base.c |  6 +++++-
>  include/linux/i2c.h         | 10 +++++++---
>  2 files changed, 12 insertions(+), 4 deletions(-)

I'm fine with this RFC patch (modulo the missing 'master_xfer_irqless !=
NULL' check already pointed out). It solves my usecase for I2C
poweroff/reboot handlers.

> This is intentional to preserve the previous
> behaviour and avoid regressions. Because there are drivers not using
> interrupts or because it might have worked "accidently" before.

I'm now also on your side for this behaviour and not failing with
"-EOPNOTSUPP". Explicitly breaking existing code, even if it only worked
by chance, is bad, since it will only fail at runtime on a user machine
or device.

Kind regards,
Stefan Lengfeld
diff mbox series

Patch

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 15c95aaa484c..d4a76c8cd777 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -1858,6 +1858,14 @@  int __i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 	if (adap->quirks && i2c_check_for_quirks(adap, msgs, num))
 		return -EOPNOTSUPP;
 
+	if (in_atomic() || irqs_disabled()) {
+		if (!adap->irq_safe) {
+			dev_err(&adap->dev,
+				"IRQ disabled transfers not supported by the driver\n");
+			return -EOPNOTSUPP;
+		}
+	}
+
 	/*
 	 * i2c_trace_msg_key gets enabled when tracepoint i2c_transfer gets
 	 * enabled.  This is an efficient way of keeping the for-loop from
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 254cd34eeae2..f2d31ba09afe 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -664,6 +664,13 @@  struct i2c_adapter {
 	const struct i2c_algorithm *algo; /* the algorithm to access the bus */
 	void *algo_data;
 
+	/*
+	 * implementations in i2c_algorithm like 'master_xfer' are safe to be
+	 * use in IRQ disabled contexts. A driver should set the flag with
+	 * i2c_adapter_irq_safe() in it's probe function.
+	 */
+	bool irq_safe;
+
 	/* data fields that are valid for all devices	*/
 	const struct i2c_lock_operations *lock_ops;
 	struct rt_mutex bus_lock;
@@ -710,6 +717,15 @@  i2c_parent_is_i2c_adapter(const struct i2c_adapter *adapter)
 		return NULL;
 }
 
+/**
+ * i2c_adapter_irq_safe - driver is safe to be used in IRQ disabled context
+ * @adapter: Target I2C bus segment
+ */
+static inline void i2c_adapter_irq_safe(struct i2c_adapter *adapter)
+{
+	adapter->irq_safe = true;
+}
+
 int i2c_for_each_dev(void *data, int (*fn)(struct device *, void *));
 
 /* Adapter locking functions, exported for shared pin cases */