[RFC PATCH 0/4] I2C writes with interrupts disabled

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

-- 
2.16.4




[Index of Archives]     [Linux GPIO]     [Linux SPI]     [Linux Hardward Monitoring]     [LM Sensors]     [Linux USB Devel]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux