| Message ID | 20180813211614.16440-1-contact@stefanchrist.eu (mailing list archive) |
|---|---|
| Headers | show |
| Series | I2C writes with interrupts disabled | expand |
On Tue, Aug 14, 2018 at 12:16 AM, Stefan Lengfeld <contact@stefanchrist.eu> wrote: > The idea is taken from the PM subsystem and the function > pm_runtime_irq_safe(). Just a side note, please, do not use this exact API (irq_safe in PM) as a good design reference. That is a workaround which has more pain than solves something. I hope that in I2C you meant something different (in terms of design).
Hi all, to continue the discussion: Here is a new spin of an I2C IRQ less patch set. The approach does not introduce a new I2C transfer callback like master_xfer_irqless(). Instead if follows Tero Kristo first patch and the I2C driver code should detect itself, whether it's called in IRQ disabled context with code like bool polling = irqs_disabled(); Using this function seems appropriate, because the I2C core does the same in the function i2c_transfer(). Furthermore the patchset introduces a boolean flag 'irq_safe' in the i2c_adapter. Every driver author should announce in the driver's probe function whether the driver is safe to be called in atomic or IRQ disabled contexts with the function i2c_adapter_irq_safe(). The idea is taken from the PM subsystem and the function pm_runtime_irq_safe(). The i2c_adapter_irq_safe() function should address Wolfram Sang's comment: > * Also, there is no white-listing, all transfers will be executed, so > buggy drivers will go unnoticed. Drivers that are not coded with IRQ less operation in mind get noticed at once. Responding to Tero Kristo's comment: > So, what is the plan going forward? > > 1) Modify i2c core APIs? And modify all relevant i2c drivers to support this? > 2) Just add the shutdown handler switch to the relevant drivers? (My RFCv2 > patch) > 3) Something else? This patchset combines your RFCv1 patch with a minimal I2C core change. For Wolfram Sang's comment: > IIRC the latest design we discussed is to add a new callback to struct > i2c_algorithm like 'master_xfer_irqless' and teach the I2C core when to > call which callback. Which might not be so super straightforward because > for most drivers (except PMICs probably) using I2C when interrupts are > disabled is a bug and we also shouldn't hide that by providing a generic > fallback. To detect buggy users of the I2C API, we would need to change the I2C API and add a function like: i2c_transfer(); // existing function i2c_transfer_irqless(); // new function for PMIC drivers Using a flag like 'I2C_M_IRQLESS' seems wrong, because IRQ less operation is a per-transfer, not per message thing. Nevertheless, as you already said, such a new API function would required tree-wide changes, e.g in the regmap framework. For my comment: > There are two distinct problems: > * Sending I2C messages in an atomic/irqless/sleep-free context > * Blocking the I2C-adapter for other users before starting the > sequence poweroff/reboot. The second issue I cannot reproduce anymore. Maybe it's due to having a single core i.MX6 or something else. So the corresponding patch to modify the I2C API is left out in the patchset. I Just a not for development the code. Without the help of the LOCKDEP framework and config options: CONFIG_PROVE_LOCKING=y CONFIG_DEBUG_ATOMIC_SLEEP=y it's nearly impossible two write sleep/schedule free code. There are just to many kernel functions that sleep internally. The patches are tested on a kernel v4.18-rc8 and a phyCORE-i.MX6 Solo board. Kind regards, Stefan Lengfeld Stefan Christ (1): ARM: dts: phyboard-mira-dl: rely on PMIC for reboot and watchdog Stefan Lengfeld (3): i2c: allow drivers to announce that they are IRQ safe i2c: imx: implement IRQ less master_xfer function watchdog: da9062: avoid regmap in restart handler arch/arm/boot/dts/imx6dl-phytec-mira-rdk-nand.dts | 8 ++ drivers/i2c/busses/i2c-imx.c | 113 ++++++++++++++++------ drivers/i2c/i2c-core-base.c | 8 ++ drivers/mfd/da9062-core.c | 1 + drivers/watchdog/da9062_wdt.c | 17 +++- include/linux/i2c.h | 16 +++ include/linux/mfd/da9062/core.h | 1 + 7 files changed, 128 insertions(+), 36 deletions(-)