This series adds support for very late atomic transfers to the I2C subsystem.
It finally reached a state which I think is ready-to-apply. This is mainly
because of two things:
a) we decided to respect the current locking scheme and to not give atomic
transfers a priority. The code needed for that would have been either
incomplete or very invasive. And we cannot guarantee successful transfers
anyhow. See [1] for the discussion and other write-ups for design choices.
b) thanks to a discussion with Peter Zijlstra[2], the conditions when to allow
atomic transfers became much clearer. The new helper i2c_in_atomic_xfer_mode()
adds readability, too.
In detail, changes since RFC v2:
* dropped coding style patch because already applied
* added new patch 1 to drop in_atomic() and have better conditions when
to enter the atomic path
* added support to the mux-core
* simplified omap conversion a little
* added new conversions for ocores, stu300, and algo-bit/gpio
* typo corrections found by Simon and Stefan
* added tags to drivers
* dropped tags from core patches because that part changed too much
All tested on a Renesas Lager board (R-Car H2). Sadly, the i2c-sh_mobile driver
cannot be converted now because of other work needed first. I tested with the
i2c-gpio driver, though. The other driver patches are build tested. A branch
can be found here:
git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/i2c/atomic_xfer
I am happy for reviews and comments. Please note if you review (especially the
core parts), I'd like to have a short summary of your review even if there is
no proposed change. Like what you did, what you think about it, etc. Some stuff
in here is subtle, so if you went through the effort to double check my
assumptions you should name it :)
Finally, a big thank you and credit to Renesas for funding this work, of course!
Happy hacking,
Wolfram
[1] https://lkml.org/lkml/2019/3/2/76
[2] http://patchwork.ozlabs.org/patch/1067437/
Wolfram Sang (12):
i2c: remove use of in_atomic()
i2c: core: use I2C locking behaviour also for SMBUS
i2c: core: introduce callbacks for atomic transfers
i2c: mux: populate the new *_atomic callbacks
i2c: demux: handle the new atomic callbacks
i2c: omap: Add the master_xfer_irqless hook
i2c: tegra-bpmp: convert to use new atomic callbacks
i2c: ocores: refactor setup for polling
i2c: ocores: enable atomic xfers
i2c: stu300: use xfer_atomic callback to bail out early
i2c: algo: bit: add flag to whitelist atomic transfers
i2c: gpio: flag atomic capability if possible
drivers/i2c/algos/i2c-algo-bit.c | 22 +++++++++-
drivers/i2c/busses/i2c-gpio.c | 2 +
drivers/i2c/busses/i2c-ocores.c | 16 +++-----
drivers/i2c/busses/i2c-omap.c | 76 +++++++++++++++++++++++++++++------
drivers/i2c/busses/i2c-stu300.c | 25 +++++-------
drivers/i2c/busses/i2c-tegra-bpmp.c | 25 +++++++++---
drivers/i2c/i2c-core-base.c | 17 ++++----
drivers/i2c/i2c-core-smbus.c | 25 +++++++++---
drivers/i2c/i2c-core.h | 25 ++++++++++++
drivers/i2c/i2c-mux.c | 6 +++
drivers/i2c/muxes/i2c-demux-pinctrl.c | 2 +
include/linux/i2c-algo-bit.h | 1 +
include/linux/i2c.h | 15 +++++--
13 files changed, 194 insertions(+), 63 deletions(-)
--
2.11.0
Use the new callback to reject atomic transfers.
Signed-off-by: Wolfram Sang <[email protected]>
---
drivers/i2c/busses/i2c-stu300.c | 25 ++++++++++---------------
1 file changed, 10 insertions(+), 15 deletions(-)
diff --git a/drivers/i2c/busses/i2c-stu300.c b/drivers/i2c/busses/i2c-stu300.c
index 5503fa171df0..743c161b22c5 100644
--- a/drivers/i2c/busses/i2c-stu300.c
+++ b/drivers/i2c/busses/i2c-stu300.c
@@ -328,12 +328,6 @@ static int stu300_start_and_await_event(struct stu300_dev *dev,
{
int ret;
- if (unlikely(irqs_disabled())) {
- /* TODO: implement polling for this case if need be. */
- WARN(1, "irqs are disabled, cannot poll for event\n");
- return -EIO;
- }
-
/* Lock command issue, fill in an event we wait for */
spin_lock_irq(&dev->cmd_issue_lock);
init_completion(&dev->cmd_complete);
@@ -380,13 +374,6 @@ static int stu300_await_event(struct stu300_dev *dev,
{
int ret;
- if (unlikely(irqs_disabled())) {
- /* TODO: implement polling for this case if need be. */
- dev_err(&dev->pdev->dev, "irqs are disabled on this "
- "system!\n");
- return -EIO;
- }
-
/* Is it already here? */
spin_lock_irq(&dev->cmd_issue_lock);
dev->cmd_err = STU300_ERROR_NONE;
@@ -846,6 +833,13 @@ static int stu300_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
return num;
}
+static int stu300_xfer_todo(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
+{
+ /* TODO: implement polling for this case if need be. */
+ WARN(1, "%s: atomic transfers not implemented\n", dev_name(&adap->dev));
+ return -EOPNOTSUPP;
+}
+
static u32 stu300_func(struct i2c_adapter *adap)
{
/* This is the simplest thing you can think of... */
@@ -853,8 +847,9 @@ static u32 stu300_func(struct i2c_adapter *adap)
}
static const struct i2c_algorithm stu300_algo = {
- .master_xfer = stu300_xfer,
- .functionality = stu300_func,
+ .master_xfer = stu300_xfer,
+ .master_xfer_atomic = stu300_xfer_todo,
+ .functionality = stu300_func,
};
static const struct i2c_adapter_quirks stu300_quirks = {
--
2.11.0
Use the new xfer_atomic callback to check a newly introduced flag to
whitelist atomic transfers. This will report configurations which
worked accidently.
Signed-off-by: Wolfram Sang <[email protected]>
---
drivers/i2c/algos/i2c-algo-bit.c | 22 ++++++++++++++++++++--
include/linux/i2c-algo-bit.h | 1 +
2 files changed, 21 insertions(+), 2 deletions(-)
diff --git a/drivers/i2c/algos/i2c-algo-bit.c b/drivers/i2c/algos/i2c-algo-bit.c
index 5e5990a83da5..913db013fe90 100644
--- a/drivers/i2c/algos/i2c-algo-bit.c
+++ b/drivers/i2c/algos/i2c-algo-bit.c
@@ -603,6 +603,23 @@ static int bit_xfer(struct i2c_adapter *i2c_adap,
return ret;
}
+/*
+ * We print a warning when we are not flagged to support atomic transfers but
+ * will try anyhow. That's what the I2C core would do as well. Sadly, we can't
+ * modify the algorithm struct at probe time because this struct is exported
+ * 'const'.
+ */
+static int bit_xfer_atomic(struct i2c_adapter *i2c_adap, struct i2c_msg msgs[],
+ int num)
+{
+ struct i2c_algo_bit_data *adap = i2c_adap->algo_data;
+
+ if (!adap->can_do_atomic)
+ dev_warn(&i2c_adap->dev, "not flagged for atomic transfers\n");
+
+ return bit_xfer(i2c_adap, msgs, num);
+}
+
static u32 bit_func(struct i2c_adapter *adap)
{
return I2C_FUNC_I2C | I2C_FUNC_NOSTART | I2C_FUNC_SMBUS_EMUL |
@@ -615,8 +632,9 @@ static u32 bit_func(struct i2c_adapter *adap)
/* -----exported algorithm data: ------------------------------------- */
const struct i2c_algorithm i2c_bit_algo = {
- .master_xfer = bit_xfer,
- .functionality = bit_func,
+ .master_xfer = bit_xfer,
+ .master_xfer_atomic = bit_xfer_atomic,
+ .functionality = bit_func,
};
EXPORT_SYMBOL(i2c_bit_algo);
diff --git a/include/linux/i2c-algo-bit.h b/include/linux/i2c-algo-bit.h
index 69045df78e2d..7fd5575a368f 100644
--- a/include/linux/i2c-algo-bit.h
+++ b/include/linux/i2c-algo-bit.h
@@ -33,6 +33,7 @@ struct i2c_algo_bit_data {
minimum 5 us for standard-mode I2C and SMBus,
maximum 50 us for SMBus */
int timeout; /* in jiffies */
+ bool can_do_atomic; /* callbacks don't sleep, we can be atomic */
};
int i2c_bit_add_bus(struct i2c_adapter *);
--
2.11.0
The driver did handle this internally, convert it to use the new
callbacks.
Reviewed-by: Timo Alho <[email protected]>
Acked-by: Thierry Reding <[email protected]>
Reviewed-by: Simon Horman <[email protected]>
Signed-off-by: Wolfram Sang <[email protected]>
---
drivers/i2c/busses/i2c-tegra-bpmp.c | 25 ++++++++++++++++++++-----
1 file changed, 20 insertions(+), 5 deletions(-)
diff --git a/drivers/i2c/busses/i2c-tegra-bpmp.c b/drivers/i2c/busses/i2c-tegra-bpmp.c
index f6cd35d0a2ac..9bb085793a0c 100644
--- a/drivers/i2c/busses/i2c-tegra-bpmp.c
+++ b/drivers/i2c/busses/i2c-tegra-bpmp.c
@@ -207,7 +207,8 @@ static int tegra_bpmp_i2c_msg_len_check(struct i2c_msg *msgs, unsigned int num)
static int tegra_bpmp_i2c_msg_xfer(struct tegra_bpmp_i2c *i2c,
struct mrq_i2c_request *request,
- struct mrq_i2c_response *response)
+ struct mrq_i2c_response *response,
+ bool atomic)
{
struct tegra_bpmp_message msg;
int err;
@@ -222,7 +223,7 @@ static int tegra_bpmp_i2c_msg_xfer(struct tegra_bpmp_i2c *i2c,
msg.rx.data = response;
msg.rx.size = sizeof(*response);
- if (irqs_disabled())
+ if (atomic)
err = tegra_bpmp_transfer_atomic(i2c->bpmp, &msg);
else
err = tegra_bpmp_transfer(i2c->bpmp, &msg);
@@ -230,8 +231,9 @@ static int tegra_bpmp_i2c_msg_xfer(struct tegra_bpmp_i2c *i2c,
return err;
}
-static int tegra_bpmp_i2c_xfer(struct i2c_adapter *adapter,
- struct i2c_msg *msgs, int num)
+static int tegra_bpmp_i2c_xfer_common(struct i2c_adapter *adapter,
+ struct i2c_msg *msgs, int num,
+ bool atomic)
{
struct tegra_bpmp_i2c *i2c = i2c_get_adapdata(adapter);
struct mrq_i2c_response response;
@@ -253,7 +255,7 @@ static int tegra_bpmp_i2c_xfer(struct i2c_adapter *adapter,
return err;
}
- err = tegra_bpmp_i2c_msg_xfer(i2c, &request, &response);
+ err = tegra_bpmp_i2c_msg_xfer(i2c, &request, &response, atomic);
if (err < 0) {
dev_err(i2c->dev, "failed to transfer message: %d\n", err);
return err;
@@ -268,6 +270,18 @@ static int tegra_bpmp_i2c_xfer(struct i2c_adapter *adapter,
return num;
}
+static int tegra_bpmp_i2c_xfer(struct i2c_adapter *adapter,
+ struct i2c_msg *msgs, int num)
+{
+ return tegra_bpmp_i2c_xfer_common(adapter, msgs, num, false);
+}
+
+static int tegra_bpmp_i2c_xfer_atomic(struct i2c_adapter *adapter,
+ struct i2c_msg *msgs, int num)
+{
+ return tegra_bpmp_i2c_xfer_common(adapter, msgs, num, true);
+}
+
static u32 tegra_bpmp_i2c_func(struct i2c_adapter *adapter)
{
return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | I2C_FUNC_10BIT_ADDR |
@@ -276,6 +290,7 @@ static u32 tegra_bpmp_i2c_func(struct i2c_adapter *adapter)
static const struct i2c_algorithm tegra_bpmp_i2c_algo = {
.master_xfer = tegra_bpmp_i2c_xfer,
+ .master_xfer_atomic = tegra_bpmp_i2c_xfer_atomic,
.functionality = tegra_bpmp_i2c_func,
};
--
2.11.0
If the parent has an atomic callback, we need to translate it the same
way as the non-atomic callback.
Signed-off-by: Wolfram Sang <[email protected]>
---
drivers/i2c/muxes/i2c-demux-pinctrl.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/i2c/muxes/i2c-demux-pinctrl.c b/drivers/i2c/muxes/i2c-demux-pinctrl.c
index 035032e20327..d50454c565ee 100644
--- a/drivers/i2c/muxes/i2c-demux-pinctrl.c
+++ b/drivers/i2c/muxes/i2c-demux-pinctrl.c
@@ -99,6 +99,8 @@ static int i2c_demux_activate_master(struct i2c_demux_pinctrl_priv *priv, u32 ne
/* Now fill out current adapter structure. cur_chan must be up to date */
priv->algo.master_xfer = i2c_demux_master_xfer;
+ if (adap->algo->master_xfer_atomic)
+ priv->algo.master_xfer_atomic = i2c_demux_master_xfer;
priv->algo.functionality = i2c_demux_functionality;
snprintf(priv->cur_adap.name, sizeof(priv->cur_adap.name),
--
2.11.0
Add the master_xfer_irqless hook to enable i2c transactions
in irq disabled contexts like the poweroff case.
Signed-off-by: Tero Kristo <[email protected]>
Signed-off-by: Keerthy <[email protected]>
[wsa: simplified code a little: 'timeout = !ret']
Reviewed-by: Simon Horman <[email protected]>
Signed-off-by: Wolfram Sang <[email protected]>
---
drivers/i2c/busses/i2c-omap.c | 76 +++++++++++++++++++++++++++++++++++--------
1 file changed, 63 insertions(+), 13 deletions(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index cd9c65f3d404..faa0394048a0 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -269,6 +269,8 @@ static const u8 reg_map_ip_v2[] = {
[OMAP_I2C_IP_V2_IRQENABLE_CLR] = 0x30,
};
+static int omap_i2c_xfer_data(struct omap_i2c_dev *omap);
+
static inline void omap_i2c_write_reg(struct omap_i2c_dev *omap,
int reg, u16 val)
{
@@ -648,15 +650,28 @@ static void omap_i2c_resize_fifo(struct omap_i2c_dev *omap, u8 size, bool is_rx)
(1000 * omap->speed / 8);
}
+static void omap_i2c_wait(struct omap_i2c_dev *omap)
+{
+ u16 stat;
+ u16 mask = omap_i2c_read_reg(omap, OMAP_I2C_IE_REG);
+ int count = 0;
+
+ do {
+ stat = omap_i2c_read_reg(omap, OMAP_I2C_STAT_REG);
+ count++;
+ } while (!(stat & mask) && count < 5);
+}
+
/*
* Low level master read/write transaction.
*/
static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
- struct i2c_msg *msg, int stop)
+ struct i2c_msg *msg, int stop, bool polling)
{
struct omap_i2c_dev *omap = i2c_get_adapdata(adap);
unsigned long timeout;
u16 w;
+ int ret;
dev_dbg(omap->dev, "addr: 0x%04x, len: %d, flags: 0x%x, stop: %d\n",
msg->addr, msg->len, msg->flags, stop);
@@ -680,7 +695,8 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
w |= OMAP_I2C_BUF_RXFIF_CLR | OMAP_I2C_BUF_TXFIF_CLR;
omap_i2c_write_reg(omap, OMAP_I2C_BUF_REG, w);
- reinit_completion(&omap->cmd_complete);
+ if (!polling)
+ reinit_completion(&omap->cmd_complete);
omap->cmd_err = 0;
w = OMAP_I2C_CON_EN | OMAP_I2C_CON_MST | OMAP_I2C_CON_STT;
@@ -732,8 +748,18 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
* REVISIT: We should abort the transfer on signals, but the bus goes
* into arbitration and we're currently unable to recover from it.
*/
- timeout = wait_for_completion_timeout(&omap->cmd_complete,
- OMAP_I2C_TIMEOUT);
+ if (!polling) {
+ timeout = wait_for_completion_timeout(&omap->cmd_complete,
+ OMAP_I2C_TIMEOUT);
+ } else {
+ do {
+ omap_i2c_wait(omap);
+ ret = omap_i2c_xfer_data(omap);
+ } while (ret == -EAGAIN);
+
+ timeout = !ret;
+ }
+
if (timeout == 0) {
dev_err(omap->dev, "controller timed out\n");
omap_i2c_reset(omap);
@@ -772,7 +798,8 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
* to do the work during IRQ processing.
*/
static int
-omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
+omap_i2c_xfer_common(struct i2c_adapter *adap, struct i2c_msg msgs[], int num,
+ bool polling)
{
struct omap_i2c_dev *omap = i2c_get_adapdata(adap);
int i;
@@ -794,7 +821,8 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
omap->set_mpu_wkup_lat(omap->dev, omap->latency);
for (i = 0; i < num; i++) {
- r = omap_i2c_xfer_msg(adap, &msgs[i], (i == (num - 1)));
+ r = omap_i2c_xfer_msg(adap, &msgs[i], (i == (num - 1)),
+ polling);
if (r != 0)
break;
}
@@ -813,6 +841,18 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
return r;
}
+static int
+omap_i2c_xfer_irq(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
+{
+ return omap_i2c_xfer_common(adap, msgs, num, false);
+}
+
+static int
+omap_i2c_xfer_polling(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
+{
+ return omap_i2c_xfer_common(adap, msgs, num, true);
+}
+
static u32
omap_i2c_func(struct i2c_adapter *adap)
{
@@ -1035,10 +1075,8 @@ omap_i2c_isr(int irq, void *dev_id)
return ret;
}
-static irqreturn_t
-omap_i2c_isr_thread(int this_irq, void *dev_id)
+static int omap_i2c_xfer_data(struct omap_i2c_dev *omap)
{
- struct omap_i2c_dev *omap = dev_id;
u16 bits;
u16 stat;
int err = 0, count = 0;
@@ -1056,7 +1094,8 @@ omap_i2c_isr_thread(int this_irq, void *dev_id)
if (!stat) {
/* my work here is done */
- goto out;
+ err = -EAGAIN;
+ break;
}
dev_dbg(omap->dev, "IRQ (ISR = 0x%04x)\n", stat);
@@ -1165,14 +1204,25 @@ omap_i2c_isr_thread(int this_irq, void *dev_id)
}
} while (stat);
- omap_i2c_complete_cmd(omap, err);
+ return err;
+}
+
+static irqreturn_t
+omap_i2c_isr_thread(int this_irq, void *dev_id)
+{
+ int ret;
+ struct omap_i2c_dev *omap = dev_id;
+
+ ret = omap_i2c_xfer_data(omap);
+ if (ret != -EAGAIN)
+ omap_i2c_complete_cmd(omap, ret);
-out:
return IRQ_HANDLED;
}
static const struct i2c_algorithm omap_i2c_algo = {
- .master_xfer = omap_i2c_xfer,
+ .master_xfer = omap_i2c_xfer_irq,
+ .master_xfer_atomic = omap_i2c_xfer_polling,
.functionality = omap_i2c_func,
};
--
2.11.0
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|smbus}_xfer callback if
this new atomic 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 <[email protected]>
---
drivers/i2c/i2c-core-base.c | 6 +++++-
drivers/i2c/i2c-core-smbus.c | 18 ++++++++++++++----
drivers/i2c/i2c-core.h | 7 +++++--
include/linux/i2c.h | 15 ++++++++++++---
4 files changed, 36 insertions(+), 10 deletions(-)
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index ad14f38a67e4..4e6300dc2c4e 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -1890,7 +1890,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 (i2c_in_atomic_xfer_mode() && adap->algo->master_xfer_atomic)
+ ret = adap->algo->master_xfer_atomic(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/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
index 357e083e8f45..fdb0fb9fb9aa 100644
--- a/drivers/i2c/i2c-core-smbus.c
+++ b/drivers/i2c/i2c-core-smbus.c
@@ -548,6 +548,9 @@ s32 __i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr,
unsigned short flags, char read_write,
u8 command, int protocol, union i2c_smbus_data *data)
{
+ int (*xfer_func)(struct i2c_adapter *adap, u16 addr,
+ unsigned short flags, char read_write,
+ u8 command, int size, union i2c_smbus_data *data);
unsigned long orig_jiffies;
int try;
s32 res;
@@ -562,13 +565,20 @@ s32 __i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr,
flags &= I2C_M_TEN | I2C_CLIENT_PEC | I2C_CLIENT_SCCB;
- if (adapter->algo->smbus_xfer) {
+ xfer_func = adapter->algo->smbus_xfer;
+ if (i2c_in_atomic_xfer_mode()) {
+ if (adapter->algo->smbus_xfer_atomic)
+ xfer_func = adapter->algo->smbus_xfer_atomic;
+ else if (adapter->algo->master_xfer_atomic)
+ xfer_func = NULL; /* fallback to I2C emulation */
+ }
+
+ if (xfer_func) {
/* Retry automatically on arbitration loss */
orig_jiffies = jiffies;
for (res = 0, try = 0; try <= adapter->retries; try++) {
- res = adapter->algo->smbus_xfer(adapter, addr, flags,
- read_write, command,
- protocol, data);
+ res = xfer_func(adapter, addr, flags, read_write,
+ command, protocol, data);
if (res != -EAGAIN)
break;
if (time_after(jiffies,
diff --git a/drivers/i2c/i2c-core.h b/drivers/i2c/i2c-core.h
index deea47c576e5..f9d0c417b5a5 100644
--- a/drivers/i2c/i2c-core.h
+++ b/drivers/i2c/i2c-core.h
@@ -43,10 +43,13 @@ static inline int __i2c_lock_bus_helper(struct i2c_adapter *adap)
{
int ret = 0;
- if (i2c_in_atomic_xfer_mode())
+ if (i2c_in_atomic_xfer_mode()) {
+ WARN(!adap->algo->master_xfer_atomic && !adap->algo->smbus_xfer_atomic,
+ "No atomic I2C transfer handler for '%s'\n", dev_name(&adap->dev));
ret = i2c_trylock_bus(adap, I2C_LOCK_SEGMENT) ? 0 : -EAGAIN;
- else
+ } else {
i2c_lock_bus(adap, I2C_LOCK_SEGMENT);
+ }
return ret;
}
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 758a6db864c9..03755d4c9229 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -499,9 +499,13 @@ 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_atomic: same as @master_xfer. Yet, only using atomic context
+ * so e.g. PMICs can be accessed very late before shutdown. Optional.
* @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.
+ * @smbus_xfer_atomic: same as @smbus_xfer. Yet, only using atomic context
+ * so e.g. PMICs can be accessed very late before shutdown. Optional.
* @functionality: Return the flags that this algorithm/adapter pair supports
* from the I2C_FUNC_* flags.
* @reg_slave: Register given client to I2C slave mode of this adapter
@@ -512,9 +516,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{_atomic} fields 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 {
/*
@@ -528,9 +532,14 @@ struct i2c_algorithm {
*/
int (*master_xfer)(struct i2c_adapter *adap, struct i2c_msg *msgs,
int num);
+ int (*master_xfer_atomic)(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);
+ int (*smbus_xfer_atomic)(struct i2c_adapter *adap, u16 addr,
+ unsigned short flags, char read_write,
+ u8 command, int size, union i2c_smbus_data *data);
/* To determine what the adapter supports */
u32 (*functionality)(struct i2c_adapter *adap);
--
2.11.0
If the parent adapter has atomic_xfer callbacks, populate them for the
mux adapter as well. We can use the same translation function as for the
non-atomic xfer callback.
Signed-off-by: Wolfram Sang <[email protected]>
---
drivers/i2c/i2c-mux.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
index f330690b4125..603252fa1284 100644
--- a/drivers/i2c/i2c-mux.c
+++ b/drivers/i2c/i2c-mux.c
@@ -310,12 +310,18 @@ int i2c_mux_add_adapter(struct i2c_mux_core *muxc,
else
priv->algo.master_xfer = __i2c_mux_master_xfer;
}
+ if (parent->algo->master_xfer_atomic)
+ priv->algo.master_xfer_atomic = priv->algo.master_xfer;
+
if (parent->algo->smbus_xfer) {
if (muxc->mux_locked)
priv->algo.smbus_xfer = i2c_mux_smbus_xfer;
else
priv->algo.smbus_xfer = __i2c_mux_smbus_xfer;
}
+ if (parent->algo->smbus_xfer_atomic)
+ priv->algo.smbus_xfer_atomic = priv->algo.smbus_xfer;
+
priv->algo.functionality = i2c_mux_functionality;
/* Now fill out new adapter structure */
--
2.11.0
The driver already has the routine in place, tie it to the new callback.
Signed-off-by: Wolfram Sang <[email protected]>
Cc: Andrew Lunn <[email protected]>
---
drivers/i2c/busses/i2c-ocores.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c
index 1b99f467aae0..c3dabee0aa35 100644
--- a/drivers/i2c/busses/i2c-ocores.c
+++ b/drivers/i2c/busses/i2c-ocores.c
@@ -442,6 +442,7 @@ static u32 ocores_func(struct i2c_adapter *adap)
static struct i2c_algorithm ocores_algorithm = {
.master_xfer = ocores_xfer,
+ .master_xfer_atomic = ocores_xfer_polling,
.functionality = ocores_func,
};
--
2.11.0
If switching GPIOs does not sleep, then we can support atomic transfers.
Signed-off-by: Wolfram Sang <[email protected]>
---
drivers/i2c/busses/i2c-gpio.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/i2c/busses/i2c-gpio.c b/drivers/i2c/busses/i2c-gpio.c
index bba5c4627de3..9684a0ac2a6d 100644
--- a/drivers/i2c/busses/i2c-gpio.c
+++ b/drivers/i2c/busses/i2c-gpio.c
@@ -413,6 +413,8 @@ static int i2c_gpio_probe(struct platform_device *pdev)
if (gpiod_cansleep(priv->sda) || gpiod_cansleep(priv->scl))
dev_warn(dev, "Slow GPIO pins might wreak havoc into I2C/SMBus bus timing");
+ else
+ bit_data->can_do_atomic = true;
bit_data->setsda = i2c_gpio_setsda_val;
bit_data->setscl = i2c_gpio_setscl_val;
--
2.11.0
If I2C transfers are executed in atomic contexts, trylock is used
instead of lock. This behaviour was missing for SMBUS, although a lot of
transfers are of SMBUS type, either emulated or direct. So, factor out
the locking routine into a helper and use it for I2C and SMBUS.
Signed-off-by: Wolfram Sang <[email protected]>
---
drivers/i2c/i2c-core-base.c | 11 +++--------
drivers/i2c/i2c-core-smbus.c | 7 ++++++-
drivers/i2c/i2c-core.h | 12 ++++++++++++
3 files changed, 21 insertions(+), 9 deletions(-)
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index f8502064cd6b..ad14f38a67e4 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -1946,14 +1946,9 @@ 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 (i2c_in_atomic_xfer_mode()) {
- ret = i2c_trylock_bus(adap, I2C_LOCK_SEGMENT);
- if (!ret)
- /* I2C activity is ongoing. */
- return -EAGAIN;
- } else {
- i2c_lock_bus(adap, I2C_LOCK_SEGMENT);
- }
+ ret = __i2c_lock_bus_helper(adap);
+ if (ret)
+ return ret;
ret = __i2c_transfer(adap, msgs, num);
i2c_unlock_bus(adap, I2C_LOCK_SEGMENT);
diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
index 132119112596..357e083e8f45 100644
--- a/drivers/i2c/i2c-core-smbus.c
+++ b/drivers/i2c/i2c-core-smbus.c
@@ -20,6 +20,8 @@
#include <linux/i2c-smbus.h>
#include <linux/slab.h>
+#include "i2c-core.h"
+
#define CREATE_TRACE_POINTS
#include <trace/events/smbus.h>
@@ -530,7 +532,10 @@ s32 i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr,
{
s32 res;
- i2c_lock_bus(adapter, I2C_LOCK_SEGMENT);
+ res = __i2c_lock_bus_helper(adapter);
+ if (res)
+ return res;
+
res = __i2c_smbus_xfer(adapter, addr, flags, read_write,
command, protocol, data);
i2c_unlock_bus(adapter, I2C_LOCK_SEGMENT);
diff --git a/drivers/i2c/i2c-core.h b/drivers/i2c/i2c-core.h
index 9d8526415b26..deea47c576e5 100644
--- a/drivers/i2c/i2c-core.h
+++ b/drivers/i2c/i2c-core.h
@@ -39,6 +39,18 @@ static inline bool i2c_in_atomic_xfer_mode(void)
return system_state > SYSTEM_RUNNING && irqs_disabled();
}
+static inline int __i2c_lock_bus_helper(struct i2c_adapter *adap)
+{
+ int ret = 0;
+
+ if (i2c_in_atomic_xfer_mode())
+ ret = i2c_trylock_bus(adap, I2C_LOCK_SEGMENT) ? 0 : -EAGAIN;
+ else
+ i2c_lock_bus(adap, I2C_LOCK_SEGMENT);
+
+ return ret;
+}
+
#ifdef CONFIG_ACPI
const struct acpi_device_id *
i2c_acpi_match_device(const struct acpi_device_id *matches,
--
2.11.0
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 <[email protected]>
---
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
By properly setting up the algorithm at probe time, we can skip the
check at every transfer. This allows us to get rid of the flags
completely.
Signed-off-by: Wolfram Sang <[email protected]>
Cc: Andrew Lunn <[email protected]>
---
drivers/i2c/busses/i2c-ocores.c | 15 ++++-----------
1 file changed, 4 insertions(+), 11 deletions(-)
diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c
index 4e1a077fb688..1b99f467aae0 100644
--- a/drivers/i2c/busses/i2c-ocores.c
+++ b/drivers/i2c/busses/i2c-ocores.c
@@ -26,8 +26,6 @@
#include <linux/spinlock.h>
#include <linux/jiffies.h>
-#define OCORES_FLAG_POLL BIT(0)
-
/*
* 'process_lock' exists because ocores_process() and ocores_process_timeout()
* can't run in parallel.
@@ -37,7 +35,6 @@ struct ocores_i2c {
int iobase;
u32 reg_shift;
u32 reg_io_width;
- unsigned long flags;
wait_queue_head_t wait;
struct i2c_adapter adap;
struct i2c_msg *msg;
@@ -403,11 +400,7 @@ static int ocores_xfer_polling(struct i2c_adapter *adap,
static int ocores_xfer(struct i2c_adapter *adap,
struct i2c_msg *msgs, int num)
{
- struct ocores_i2c *i2c = i2c_get_adapdata(adap);
-
- if (i2c->flags & OCORES_FLAG_POLL)
- return ocores_xfer_polling(adap, msgs, num);
- return ocores_xfer_core(i2c, msgs, num, false);
+ return ocores_xfer_core(i2c_get_adapdata(adap), msgs, num, false);
}
static int ocores_init(struct device *dev, struct ocores_i2c *i2c)
@@ -447,7 +440,7 @@ static u32 ocores_func(struct i2c_adapter *adap)
return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
}
-static const struct i2c_algorithm ocores_algorithm = {
+static struct i2c_algorithm ocores_algorithm = {
.master_xfer = ocores_xfer,
.functionality = ocores_func,
};
@@ -673,13 +666,13 @@ static int ocores_i2c_probe(struct platform_device *pdev)
irq = platform_get_irq(pdev, 0);
if (irq == -ENXIO) {
- i2c->flags |= OCORES_FLAG_POLL;
+ ocores_algorithm.master_xfer = ocores_xfer_polling;
} else {
if (irq < 0)
return irq;
}
- if (!(i2c->flags & OCORES_FLAG_POLL)) {
+ if (ocores_algorithm.master_xfer != ocores_xfer_polling) {
ret = devm_request_irq(&pdev->dev, irq, ocores_isr, 0,
pdev->name, i2c);
if (ret) {
--
2.11.0
On Wed, Apr 03, 2019 at 02:40:07PM +0200, Wolfram Sang wrote:
> This series adds support for very late atomic transfers to the I2C subsystem.
> It finally reached a state which I think is ready-to-apply. This is mainly
> because of two things:
>
> a) we decided to respect the current locking scheme and to not give atomic
> transfers a priority. The code needed for that would have been either
> incomplete or very invasive. And we cannot guarantee successful transfers
> anyhow. See [1] for the discussion and other write-ups for design choices.
>
> b) thanks to a discussion with Peter Zijlstra[2], the conditions when to allow
> atomic transfers became much clearer. The new helper i2c_in_atomic_xfer_mode()
> adds readability, too.
>
> In detail, changes since RFC v2:
>
> * dropped coding style patch because already applied
> * added new patch 1 to drop in_atomic() and have better conditions when
> to enter the atomic path
> * added support to the mux-core
> * simplified omap conversion a little
> * added new conversions for ocores, stu300, and algo-bit/gpio
> * typo corrections found by Simon and Stefan
> * added tags to drivers
> * dropped tags from core patches because that part changed too much
>
> All tested on a Renesas Lager board (R-Car H2). Sadly, the i2c-sh_mobile driver
> cannot be converted now because of other work needed first. I tested with the
> i2c-gpio driver, though. The other driver patches are build tested. A branch
> can be found here:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/i2c/atomic_xfer
>
> I am happy for reviews and comments. Please note if you review (especially the
> core parts), I'd like to have a short summary of your review even if there is
> no proposed change. Like what you did, what you think about it, etc. Some stuff
> in here is subtle, so if you went through the effort to double check my
> assumptions you should name it :)
>
Thank you!
FWIW,
Reviewed-by Andy Shevchenko <[email protected]>
for patches 1-5,12.
Indeed, atomic condition sounds clear now.
>
> Finally, a big thank you and credit to Renesas for funding this work, of course!
>
> Happy hacking,
>
> Wolfram
>
> [1] https://lkml.org/lkml/2019/3/2/76
> [2] http://patchwork.ozlabs.org/patch/1067437/
>
> Wolfram Sang (12):
> i2c: remove use of in_atomic()
> i2c: core: use I2C locking behaviour also for SMBUS
> i2c: core: introduce callbacks for atomic transfers
> i2c: mux: populate the new *_atomic callbacks
> i2c: demux: handle the new atomic callbacks
> i2c: omap: Add the master_xfer_irqless hook
> i2c: tegra-bpmp: convert to use new atomic callbacks
> i2c: ocores: refactor setup for polling
> i2c: ocores: enable atomic xfers
> i2c: stu300: use xfer_atomic callback to bail out early
> i2c: algo: bit: add flag to whitelist atomic transfers
> i2c: gpio: flag atomic capability if possible
>
> drivers/i2c/algos/i2c-algo-bit.c | 22 +++++++++-
> drivers/i2c/busses/i2c-gpio.c | 2 +
> drivers/i2c/busses/i2c-ocores.c | 16 +++-----
> drivers/i2c/busses/i2c-omap.c | 76 +++++++++++++++++++++++++++++------
> drivers/i2c/busses/i2c-stu300.c | 25 +++++-------
> drivers/i2c/busses/i2c-tegra-bpmp.c | 25 +++++++++---
> drivers/i2c/i2c-core-base.c | 17 ++++----
> drivers/i2c/i2c-core-smbus.c | 25 +++++++++---
> drivers/i2c/i2c-core.h | 25 ++++++++++++
> drivers/i2c/i2c-mux.c | 6 +++
> drivers/i2c/muxes/i2c-demux-pinctrl.c | 2 +
> include/linux/i2c-algo-bit.h | 1 +
> include/linux/i2c.h | 15 +++++--
> 13 files changed, 194 insertions(+), 63 deletions(-)
>
> --
> 2.11.0
>
--
With Best Regards,
Andy Shevchenko
On 2019-04-03 14:40, Wolfram Sang wrote:
> If the parent adapter has atomic_xfer callbacks, populate them for the
> mux adapter as well. We can use the same translation function as for the
> non-atomic xfer callback.
>
> Signed-off-by: Wolfram Sang <[email protected]>
> ---
> drivers/i2c/i2c-mux.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
> index f330690b4125..603252fa1284 100644
> --- a/drivers/i2c/i2c-mux.c
> +++ b/drivers/i2c/i2c-mux.c
> @@ -310,12 +310,18 @@ int i2c_mux_add_adapter(struct i2c_mux_core *muxc,
> else
> priv->algo.master_xfer = __i2c_mux_master_xfer;
> }
> + if (parent->algo->master_xfer_atomic)
> + priv->algo.master_xfer_atomic = priv->algo.master_xfer;
> +
> if (parent->algo->smbus_xfer) {
> if (muxc->mux_locked)
> priv->algo.smbus_xfer = i2c_mux_smbus_xfer;
> else
> priv->algo.smbus_xfer = __i2c_mux_smbus_xfer;
> }
> + if (parent->algo->smbus_xfer_atomic)
> + priv->algo.smbus_xfer_atomic = priv->algo.smbus_xfer;
> +
> priv->algo.functionality = i2c_mux_functionality;
>
> /* Now fill out new adapter structure */
>
Hmmm, what happens if a driver implements .master_xfer and relies on
emulation for SMBus, and then someone implements .smbus_xfer_atomic
to handle some corner case at power-down?
Then someone hides the power-down device behind a mux. That would end
with xfers destined for .smbus_xfer_atomic being emulated by the
non-atomic .master_xfer, no?
Maybe too weird to care about?
I guess the question is if it is allowed to have .master_xfer_atomic
but not .master_xfer (and similarly for .smbus_xfer{,_atomic})? Maybe
that decision should be made explicit? And perhaps enforced?
I don't care deeply about the above though, so feel free to do
something about it, or
Reviewed-by: Peter Rosin <[email protected]>
Cheers,
Peter
On Wed, Apr 3, 2019 at 7:40 PM Wolfram Sang
<[email protected]> wrote:
> Use the new callback to reject atomic transfers.
>
> Signed-off-by: Wolfram Sang <[email protected]>
OK that is a reasonable placeholder.
Reviewed-by: Linus Walleij <[email protected]>
Yours,
Linus Walleij
On Wed, Apr 3, 2019 at 7:40 PM Wolfram Sang
<[email protected]> wrote:
> If switching GPIOs does not sleep, then we can support atomic transfers.
>
> Signed-off-by: Wolfram Sang <[email protected]>
Neat and intuitive.
Reviewed-by: Linus Walleij <[email protected]>
Yours,
Linus Walleij
>>>>> "Wolfram" == Wolfram Sang <[email protected]> writes:
Hi,
> By properly setting up the algorithm at probe time, we can skip the
> check at every transfer. This allows us to get rid of the flags
> completely.
> Signed-off-by: Wolfram Sang <[email protected]>
> Cc: Andrew Lunn <[email protected]>
Acked-by: Peter Korsgaard <[email protected]>
--
Bye, Peter Korsgaard
On Wed, Apr 03, 2019 at 02:40:15PM +0200, Wolfram Sang wrote:
> By properly setting up the algorithm at probe time, we can skip the
> check at every transfer. This allows us to get rid of the flags
> completely.
>
> Signed-off-by: Wolfram Sang <[email protected]>
> Cc: Andrew Lunn <[email protected]>
Reviewed-by: Andrew Lunn <[email protected]>
Andrew
On Wed, Apr 03, 2019 at 02:40:16PM +0200, Wolfram Sang wrote:
> The driver already has the routine in place, tie it to the new callback.
>
> Signed-off-by: Wolfram Sang <[email protected]>
> Cc: Andrew Lunn <[email protected]>
The polling function was not really designed with the intention to be
used in atomic context. But it does appear to be safe to use in that
context.
So
Reviewed-by: Andrew Lunn <[email protected]>
Andrew
> I guess the question is if it is allowed to have .master_xfer_atomic
> but not .master_xfer (and similarly for .smbus_xfer{,_atomic})? Maybe
> that decision should be made explicit? And perhaps enforced?
xfer_atomic callbacks are optional. One xfer callback is mandatory. I
did a check for falling back to master_xfer_atomic if there is no
suitable smbus_xfer_atomic. I will think about the vice-versa case you
mentioned. Yet, this is indeed a super corner case, so I prefer to fix
this incrementally.
> I don't care deeply about the above though, so feel free to do
> something about it, or
>
> Reviewed-by: Peter Rosin <[email protected]>
Thanks for the review!
On Wed, Apr 03, 2019 at 04:15:10PM +0300, Andy Shevchenko wrote:
> On Wed, Apr 03, 2019 at 02:40:07PM +0200, Wolfram Sang wrote:
> > This series adds support for very late atomic transfers to the I2C subsystem.
> > It finally reached a state which I think is ready-to-apply. This is mainly
> > because of two things:
> >
> > a) we decided to respect the current locking scheme and to not give atomic
> > transfers a priority. The code needed for that would have been either
> > incomplete or very invasive. And we cannot guarantee successful transfers
> > anyhow. See [1] for the discussion and other write-ups for design choices.
> >
> > b) thanks to a discussion with Peter Zijlstra[2], the conditions when to allow
> > atomic transfers became much clearer. The new helper i2c_in_atomic_xfer_mode()
> > adds readability, too.
> >
> > In detail, changes since RFC v2:
> >
> > * dropped coding style patch because already applied
> > * added new patch 1 to drop in_atomic() and have better conditions when
> > to enter the atomic path
> > * added support to the mux-core
> > * simplified omap conversion a little
> > * added new conversions for ocores, stu300, and algo-bit/gpio
> > * typo corrections found by Simon and Stefan
> > * added tags to drivers
> > * dropped tags from core patches because that part changed too much
> >
> > All tested on a Renesas Lager board (R-Car H2). Sadly, the i2c-sh_mobile driver
> > cannot be converted now because of other work needed first. I tested with the
> > i2c-gpio driver, though. The other driver patches are build tested. A branch
> > can be found here:
> >
> > git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/i2c/atomic_xfer
> >
> > I am happy for reviews and comments. Please note if you review (especially the
> > core parts), I'd like to have a short summary of your review even if there is
> > no proposed change. Like what you did, what you think about it, etc. Some stuff
> > in here is subtle, so if you went through the effort to double check my
> > assumptions you should name it :)
> >
>
> Thank you!
>
> FWIW,
>
> Reviewed-by Andy Shevchenko <[email protected]>
>
> for patches 1-5,12.
Thanks for the review, Andy! May I ask you once more to tag the patches
individually so patchwork can pick them up for me?
On Wed, Apr 03, 2019 at 02:40:07PM +0200, Wolfram Sang wrote:
> This series adds support for very late atomic transfers to the I2C subsystem.
> It finally reached a state which I think is ready-to-apply. This is mainly
> because of two things:
>
> a) we decided to respect the current locking scheme and to not give atomic
> transfers a priority. The code needed for that would have been either
> incomplete or very invasive. And we cannot guarantee successful transfers
> anyhow. See [1] for the discussion and other write-ups for design choices.
>
> b) thanks to a discussion with Peter Zijlstra[2], the conditions when to allow
> atomic transfers became much clearer. The new helper i2c_in_atomic_xfer_mode()
> adds readability, too.
>
> In detail, changes since RFC v2:
>
> * dropped coding style patch because already applied
> * added new patch 1 to drop in_atomic() and have better conditions when
> to enter the atomic path
> * added support to the mux-core
> * simplified omap conversion a little
> * added new conversions for ocores, stu300, and algo-bit/gpio
> * typo corrections found by Simon and Stefan
> * added tags to drivers
> * dropped tags from core patches because that part changed too much
>
> All tested on a Renesas Lager board (R-Car H2). Sadly, the i2c-sh_mobile driver
> cannot be converted now because of other work needed first. I tested with the
> i2c-gpio driver, though. The other driver patches are build tested. A branch
> can be found here:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/i2c/atomic_xfer
>
> I am happy for reviews and comments. Please note if you review (especially the
> core parts), I'd like to have a short summary of your review even if there is
> no proposed change. Like what you did, what you think about it, etc. Some stuff
> in here is subtle, so if you went through the effort to double check my
> assumptions you should name it :)
>
>
> Finally, a big thank you and credit to Renesas for funding this work, of course!
No major critcism voiced here, so applied to for-next! Let's see how
this series does there...
On Mon, Apr 15, 2019 at 02:06:11PM +0200, Wolfram Sang wrote:
> On Wed, Apr 03, 2019 at 04:15:10PM +0300, Andy Shevchenko wrote:
> > On Wed, Apr 03, 2019 at 02:40:07PM +0200, Wolfram Sang wrote:
> >
> > FWIW,
> >
> > Reviewed-by Andy Shevchenko <[email protected]>
> >
> > for patches 1-5,12.
>
> Thanks for the review, Andy! May I ask you once more to tag the patches
> individually so patchwork can pick them up for me?
It seems I already cleaned up them from my mailbox. I can check if it's
available in another one.
--
With Best Regards,
Andy Shevchenko
On Wed, Apr 3, 2019 at 3:42 PM Wolfram Sang
<[email protected]> wrote:
>
> If the parent has an atomic callback, we need to translate it the same
> way as the non-atomic callback.
>
Reviewed-by Andy Shevchenko <[email protected]>
> Signed-off-by: Wolfram Sang <[email protected]>
> ---
> drivers/i2c/muxes/i2c-demux-pinctrl.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/i2c/muxes/i2c-demux-pinctrl.c b/drivers/i2c/muxes/i2c-demux-pinctrl.c
> index 035032e20327..d50454c565ee 100644
> --- a/drivers/i2c/muxes/i2c-demux-pinctrl.c
> +++ b/drivers/i2c/muxes/i2c-demux-pinctrl.c
> @@ -99,6 +99,8 @@ static int i2c_demux_activate_master(struct i2c_demux_pinctrl_priv *priv, u32 ne
>
> /* Now fill out current adapter structure. cur_chan must be up to date */
> priv->algo.master_xfer = i2c_demux_master_xfer;
> + if (adap->algo->master_xfer_atomic)
> + priv->algo.master_xfer_atomic = i2c_demux_master_xfer;
> priv->algo.functionality = i2c_demux_functionality;
>
> snprintf(priv->cur_adap.name, sizeof(priv->cur_adap.name),
> --
> 2.11.0
>
--
With Best Regards,
Andy Shevchenko
On Wed, Apr 3, 2019 at 3:42 PM Wolfram Sang
<[email protected]> wrote:
>
> If switching GPIOs does not sleep, then we can support atomic transfers.
>
Reviewed-by Andy Shevchenko <[email protected]>
> Signed-off-by: Wolfram Sang <[email protected]>
> ---
> drivers/i2c/busses/i2c-gpio.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/i2c/busses/i2c-gpio.c b/drivers/i2c/busses/i2c-gpio.c
> index bba5c4627de3..9684a0ac2a6d 100644
> --- a/drivers/i2c/busses/i2c-gpio.c
> +++ b/drivers/i2c/busses/i2c-gpio.c
> @@ -413,6 +413,8 @@ static int i2c_gpio_probe(struct platform_device *pdev)
>
> if (gpiod_cansleep(priv->sda) || gpiod_cansleep(priv->scl))
> dev_warn(dev, "Slow GPIO pins might wreak havoc into I2C/SMBus bus timing");
> + else
> + bit_data->can_do_atomic = true;
>
> bit_data->setsda = i2c_gpio_setsda_val;
> bit_data->setscl = i2c_gpio_setscl_val;
> --
> 2.11.0
>
--
With Best Regards,
Andy Shevchenko
On Wed, Apr 3, 2019 at 3:43 PM Wolfram Sang
<[email protected]> wrote:
>
> 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|smbus}_xfer callback if
> this new atomic 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.
>
Reviewed-by Andy Shevchenko <[email protected]>
> Signed-off-by: Wolfram Sang <[email protected]>
> ---
> drivers/i2c/i2c-core-base.c | 6 +++++-
> drivers/i2c/i2c-core-smbus.c | 18 ++++++++++++++----
> drivers/i2c/i2c-core.h | 7 +++++--
> include/linux/i2c.h | 15 ++++++++++++---
> 4 files changed, 36 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index ad14f38a67e4..4e6300dc2c4e 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -1890,7 +1890,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 (i2c_in_atomic_xfer_mode() && adap->algo->master_xfer_atomic)
> + ret = adap->algo->master_xfer_atomic(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/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
> index 357e083e8f45..fdb0fb9fb9aa 100644
> --- a/drivers/i2c/i2c-core-smbus.c
> +++ b/drivers/i2c/i2c-core-smbus.c
> @@ -548,6 +548,9 @@ s32 __i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr,
> unsigned short flags, char read_write,
> u8 command, int protocol, union i2c_smbus_data *data)
> {
> + int (*xfer_func)(struct i2c_adapter *adap, u16 addr,
> + unsigned short flags, char read_write,
> + u8 command, int size, union i2c_smbus_data *data);
> unsigned long orig_jiffies;
> int try;
> s32 res;
> @@ -562,13 +565,20 @@ s32 __i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr,
>
> flags &= I2C_M_TEN | I2C_CLIENT_PEC | I2C_CLIENT_SCCB;
>
> - if (adapter->algo->smbus_xfer) {
> + xfer_func = adapter->algo->smbus_xfer;
> + if (i2c_in_atomic_xfer_mode()) {
> + if (adapter->algo->smbus_xfer_atomic)
> + xfer_func = adapter->algo->smbus_xfer_atomic;
> + else if (adapter->algo->master_xfer_atomic)
> + xfer_func = NULL; /* fallback to I2C emulation */
> + }
> +
> + if (xfer_func) {
> /* Retry automatically on arbitration loss */
> orig_jiffies = jiffies;
> for (res = 0, try = 0; try <= adapter->retries; try++) {
> - res = adapter->algo->smbus_xfer(adapter, addr, flags,
> - read_write, command,
> - protocol, data);
> + res = xfer_func(adapter, addr, flags, read_write,
> + command, protocol, data);
> if (res != -EAGAIN)
> break;
> if (time_after(jiffies,
> diff --git a/drivers/i2c/i2c-core.h b/drivers/i2c/i2c-core.h
> index deea47c576e5..f9d0c417b5a5 100644
> --- a/drivers/i2c/i2c-core.h
> +++ b/drivers/i2c/i2c-core.h
> @@ -43,10 +43,13 @@ static inline int __i2c_lock_bus_helper(struct i2c_adapter *adap)
> {
> int ret = 0;
>
> - if (i2c_in_atomic_xfer_mode())
> + if (i2c_in_atomic_xfer_mode()) {
> + WARN(!adap->algo->master_xfer_atomic && !adap->algo->smbus_xfer_atomic,
> + "No atomic I2C transfer handler for '%s'\n", dev_name(&adap->dev));
> ret = i2c_trylock_bus(adap, I2C_LOCK_SEGMENT) ? 0 : -EAGAIN;
> - else
> + } else {
> i2c_lock_bus(adap, I2C_LOCK_SEGMENT);
> + }
>
> return ret;
> }
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index 758a6db864c9..03755d4c9229 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -499,9 +499,13 @@ 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_atomic: same as @master_xfer. Yet, only using atomic context
> + * so e.g. PMICs can be accessed very late before shutdown. Optional.
> * @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.
> + * @smbus_xfer_atomic: same as @smbus_xfer. Yet, only using atomic context
> + * so e.g. PMICs can be accessed very late before shutdown. Optional.
> * @functionality: Return the flags that this algorithm/adapter pair supports
> * from the I2C_FUNC_* flags.
> * @reg_slave: Register given client to I2C slave mode of this adapter
> @@ -512,9 +516,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{_atomic} fields 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 {
> /*
> @@ -528,9 +532,14 @@ struct i2c_algorithm {
> */
> int (*master_xfer)(struct i2c_adapter *adap, struct i2c_msg *msgs,
> int num);
> + int (*master_xfer_atomic)(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);
> + int (*smbus_xfer_atomic)(struct i2c_adapter *adap, u16 addr,
> + unsigned short flags, char read_write,
> + u8 command, int size, union i2c_smbus_data *data);
>
> /* To determine what the adapter supports */
> u32 (*functionality)(struct i2c_adapter *adap);
> --
> 2.11.0
>
--
With Best Regards,
Andy Shevchenko
On Wed, Apr 3, 2019 at 3:43 PM Wolfram Sang
<[email protected]> wrote:
>
> If I2C transfers are executed in atomic contexts, trylock is used
> instead of lock. This behaviour was missing for SMBUS, although a lot of
> transfers are of SMBUS type, either emulated or direct. So, factor out
> the locking routine into a helper and use it for I2C and SMBUS.
>
Reviewed-by Andy Shevchenko <[email protected]>
> Signed-off-by: Wolfram Sang <[email protected]>
> ---
> drivers/i2c/i2c-core-base.c | 11 +++--------
> drivers/i2c/i2c-core-smbus.c | 7 ++++++-
> drivers/i2c/i2c-core.h | 12 ++++++++++++
> 3 files changed, 21 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index f8502064cd6b..ad14f38a67e4 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -1946,14 +1946,9 @@ 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 (i2c_in_atomic_xfer_mode()) {
> - ret = i2c_trylock_bus(adap, I2C_LOCK_SEGMENT);
> - if (!ret)
> - /* I2C activity is ongoing. */
> - return -EAGAIN;
> - } else {
> - i2c_lock_bus(adap, I2C_LOCK_SEGMENT);
> - }
> + ret = __i2c_lock_bus_helper(adap);
> + if (ret)
> + return ret;
>
> ret = __i2c_transfer(adap, msgs, num);
> i2c_unlock_bus(adap, I2C_LOCK_SEGMENT);
> diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
> index 132119112596..357e083e8f45 100644
> --- a/drivers/i2c/i2c-core-smbus.c
> +++ b/drivers/i2c/i2c-core-smbus.c
> @@ -20,6 +20,8 @@
> #include <linux/i2c-smbus.h>
> #include <linux/slab.h>
>
> +#include "i2c-core.h"
> +
> #define CREATE_TRACE_POINTS
> #include <trace/events/smbus.h>
>
> @@ -530,7 +532,10 @@ s32 i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr,
> {
> s32 res;
>
> - i2c_lock_bus(adapter, I2C_LOCK_SEGMENT);
> + res = __i2c_lock_bus_helper(adapter);
> + if (res)
> + return res;
> +
> res = __i2c_smbus_xfer(adapter, addr, flags, read_write,
> command, protocol, data);
> i2c_unlock_bus(adapter, I2C_LOCK_SEGMENT);
> diff --git a/drivers/i2c/i2c-core.h b/drivers/i2c/i2c-core.h
> index 9d8526415b26..deea47c576e5 100644
> --- a/drivers/i2c/i2c-core.h
> +++ b/drivers/i2c/i2c-core.h
> @@ -39,6 +39,18 @@ static inline bool i2c_in_atomic_xfer_mode(void)
> return system_state > SYSTEM_RUNNING && irqs_disabled();
> }
>
> +static inline int __i2c_lock_bus_helper(struct i2c_adapter *adap)
> +{
> + int ret = 0;
> +
> + if (i2c_in_atomic_xfer_mode())
> + ret = i2c_trylock_bus(adap, I2C_LOCK_SEGMENT) ? 0 : -EAGAIN;
> + else
> + i2c_lock_bus(adap, I2C_LOCK_SEGMENT);
> +
> + return ret;
> +}
> +
> #ifdef CONFIG_ACPI
> const struct acpi_device_id *
> i2c_acpi_match_device(const struct acpi_device_id *matches,
> --
> 2.11.0
>
--
With Best Regards,
Andy Shevchenko
On Wed, Apr 3, 2019 at 3:43 PM Wolfram Sang
<[email protected]> 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 <[email protected]>
> Signed-off-by: Wolfram Sang <[email protected]>
> ---
> 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
>
--
With Best Regards,
Andy Shevchenko
On Wed, Apr 3, 2019 at 3:42 PM Wolfram Sang
<[email protected]> wrote:
>
> If the parent adapter has atomic_xfer callbacks, populate them for the
> mux adapter as well. We can use the same translation function as for the
> non-atomic xfer callback.
>
Reviewed-by Andy Shevchenko <[email protected]>
> Signed-off-by: Wolfram Sang <[email protected]>
> ---
> drivers/i2c/i2c-mux.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
> index f330690b4125..603252fa1284 100644
> --- a/drivers/i2c/i2c-mux.c
> +++ b/drivers/i2c/i2c-mux.c
> @@ -310,12 +310,18 @@ int i2c_mux_add_adapter(struct i2c_mux_core *muxc,
> else
> priv->algo.master_xfer = __i2c_mux_master_xfer;
> }
> + if (parent->algo->master_xfer_atomic)
> + priv->algo.master_xfer_atomic = priv->algo.master_xfer;
> +
> if (parent->algo->smbus_xfer) {
> if (muxc->mux_locked)
> priv->algo.smbus_xfer = i2c_mux_smbus_xfer;
> else
> priv->algo.smbus_xfer = __i2c_mux_smbus_xfer;
> }
> + if (parent->algo->smbus_xfer_atomic)
> + priv->algo.smbus_xfer_atomic = priv->algo.smbus_xfer;
> +
> priv->algo.functionality = i2c_mux_functionality;
>
> /* Now fill out new adapter structure */
> --
> 2.11.0
>
--
With Best Regards,
Andy Shevchenko
On Mon, Apr 15, 2019 at 03:35:54PM +0300, Andy Shevchenko wrote:
> On Mon, Apr 15, 2019 at 02:06:11PM +0200, Wolfram Sang wrote:
> > On Wed, Apr 03, 2019 at 04:15:10PM +0300, Andy Shevchenko wrote:
> > > On Wed, Apr 03, 2019 at 02:40:07PM +0200, Wolfram Sang wrote:
> > >
> > > FWIW,
> > >
> > > Reviewed-by Andy Shevchenko <[email protected]>
> > >
> > > for patches 1-5,12.
> >
> > Thanks for the review, Andy! May I ask you once more to tag the patches
> > individually so patchwork can pick them up for me?
>
> It seems I already cleaned up them from my mailbox. I can check if it's
> available in another one.
Done!
--
With Best Regards,
Andy Shevchenko
On Mon, Apr 15, 2019 at 03:48:29PM +0300, Andy Shevchenko wrote:
> On Mon, Apr 15, 2019 at 03:35:54PM +0300, Andy Shevchenko wrote:
> > On Mon, Apr 15, 2019 at 02:06:11PM +0200, Wolfram Sang wrote:
> > > On Wed, Apr 03, 2019 at 04:15:10PM +0300, Andy Shevchenko wrote:
> > > > On Wed, Apr 03, 2019 at 02:40:07PM +0200, Wolfram Sang wrote:
> > > >
> > > > FWIW,
> > > >
> > > > Reviewed-by Andy Shevchenko <[email protected]>
> > > >
> > > > for patches 1-5,12.
> > >
> > > Thanks for the review, Andy! May I ask you once more to tag the patches
> > > individually so patchwork can pick them up for me?
> >
> > It seems I already cleaned up them from my mailbox. I can check if it's
> > available in another one.
>
> Done!
Thanks a ton!
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 <[email protected]>
Tested-by: Stefan Lengfeld <[email protected]>
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 Wolfram,
On Wed, Apr 03, 2019 at 02:40:10PM +0200, Wolfram Sang wrote:
> 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|smbus}_xfer callback if
> this new atomic 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 <[email protected]>
Tested-by: Stefan Lengfeld <[email protected]>
Kind regards,
Stefan
Hi Wolfram,
the subject line of this patch
i2c: omap: Add the master_xfer_irqless hook
still contains the old name of the callback '_irqless'. It should be
'_atomic' instead.
On Wed, Apr 03, 2019 at 02:40:13PM +0200, Wolfram Sang wrote:
> Add the master_xfer_irqless hook to enable i2c transactions
Here again. It should be 'master_xfer_atomic'.
> in irq disabled contexts like the poweroff case.
>
> Signed-off-by: Tero Kristo <[email protected]>
> Signed-off-by: Keerthy <[email protected]>
> [wsa: simplified code a little: 'timeout = !ret']
> Reviewed-by: Simon Horman <[email protected]>
> Signed-off-by: Wolfram Sang <[email protected]>
> ---
> drivers/i2c/busses/i2c-omap.c | 76 +++++++++++++++++++++++++++++++++++--------
> 1 file changed, 63 insertions(+), 13 deletions(-)
>
snipped
> @@ -648,15 +650,28 @@ static void omap_i2c_resize_fifo(struct omap_i2c_dev *omap, u8 size, bool is_rx)
> (1000 * omap->speed / 8);
> }
>
> +static void omap_i2c_wait(struct omap_i2c_dev *omap)
> +{
> + u16 stat;
> + u16 mask = omap_i2c_read_reg(omap, OMAP_I2C_IE_REG);
> + int count = 0;
> +
> + do {
> + stat = omap_i2c_read_reg(omap, OMAP_I2C_STAT_REG);
> + count++;
> + } while (!(stat & mask) && count < 5);
> +}
> +
> /*
> * Low level master read/write transaction.
> */
> static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
> - struct i2c_msg *msg, int stop)
> + struct i2c_msg *msg, int stop, bool polling)
Nitpick. In the patches for the other drivers the boolean flag is called
'atomic' and not 'polling'.
> {
> struct omap_i2c_dev *omap = i2c_get_adapdata(adap);
> unsigned long timeout;
> u16 w;
> + int ret;
>
> dev_dbg(omap->dev, "addr: 0x%04x, len: %d, flags: 0x%x, stop: %d\n",
> msg->addr, msg->len, msg->flags, stop);
sniped
> @@ -1165,14 +1204,25 @@ omap_i2c_isr_thread(int this_irq, void *dev_id)
> }
> } while (stat);
>
> - omap_i2c_complete_cmd(omap, err);
> + return err;
> +}
> +
> +static irqreturn_t
> +omap_i2c_isr_thread(int this_irq, void *dev_id)
> +{
> + int ret;
> + struct omap_i2c_dev *omap = dev_id;
> +
> + ret = omap_i2c_xfer_data(omap);
> + if (ret != -EAGAIN)
> + omap_i2c_complete_cmd(omap, ret);
>
> -out:
> return IRQ_HANDLED;
> }
>
> static const struct i2c_algorithm omap_i2c_algo = {
> - .master_xfer = omap_i2c_xfer,
> + .master_xfer = omap_i2c_xfer_irq,
> + .master_xfer_atomic = omap_i2c_xfer_polling,
When consistency with other drivers is a goal, the functions should be
named like:
.master_xfe = omap_i2c_xfer,
.master_xfer_atomic = omap_i2c_xfer_atomic,
The first without a suffix and the second with the '_atomic' suffix.
Kind regards,
Stefan
Hi Stefan,
> Tested-by: Stefan Lengfeld <[email protected]>
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
> the subject line of this patch
>
> i2c: omap: Add the master_xfer_irqless hook
>
> still contains the old name of the callback '_irqless'. It should be
> '_atomic' instead.
>
>
> On Wed, Apr 03, 2019 at 02:40:13PM +0200, Wolfram Sang wrote:
> > Add the master_xfer_irqless hook to enable i2c transactions
>
> Here again. It should be 'master_xfer_atomic'.
Yes, thanks, fixed!
> > static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
> > - struct i2c_msg *msg, int stop)
> > + struct i2c_msg *msg, int stop, bool polling)
>
> Nitpick. In the patches for the other drivers the boolean flag is called
> 'atomic' and not 'polling'.
This patch originally is not from me, so I prefer to not change it.
Also, I don't want to enforce strict naming within drivers. As long as
it is readable, I am fine with it.
> > static const struct i2c_algorithm omap_i2c_algo = {
> > - .master_xfer = omap_i2c_xfer,
> > + .master_xfer = omap_i2c_xfer_irq,
> > + .master_xfer_atomic = omap_i2c_xfer_polling,
>
> When consistency with other drivers is a goal, the functions should be
> named like:
>
> .master_xfe = omap_i2c_xfer,
> .master_xfer_atomic = omap_i2c_xfer_atomic,
>
> The first without a suffix and the second with the '_atomic' suffix.
ditto.