| Message ID | 20180813211614.16440-2-contact@stefanchrist.eu |
|---|---|
| State | RFC |
| Headers | show |
| Series | I2C writes with interrupts disabled | expand |
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.
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
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
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
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.
> 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);
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 > >
> 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;
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
> 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.
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;
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
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; > > >
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.
> > 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.
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 --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 */
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(+)