So, finally, here is the second RFC for supporting I2C transfers in atomic
contexts (i.e. very late). This will need some text because I tried some things
on the way but had to discard them. However, I think it is important to have
that documented.
One thing I really wanted to have is a kind of whitelist for devices which are
allowed to use atomic transfers. So we could identify the "unauthorized" ones
as buggy. To be useful, this should not add new API calls for transfers,
otherwise things would have become way more complicated for I2C users like
regmap. So, I tried e.g. to flag clients and provide that information
throughout the i2c tree (think muxes here). In the end, I concluded that this
is not an I2C specific problem, so it can't have an I2C specifc solution.
Imagine a GPIO which is needed to reboot (drivers/power/reset/gpio-restart.c).
This is the device which needs to whitelisted but the driver doesn't even know
if the GPIO is behind I2C or not. So, if we want this, it should probably be
handled on 'struct device' level. Including all the hierarchy. Postponed.
So, this RFC v2 is much more similar to v1 than I expected. Main changes:
* cleaned up 'struct i2c_adapter' a bit before adding the new stuff
* added an atomic callback for SMBus, too. Only build-tested so far. But spent
a few braincells of getting the SMBus logic readable because we could have
an I2C fallback just for the atomic case
* add a WARN for atomic transfers with no atomic transfer handler
* added support for the i2c-demuxer, so I could test the series. Support
for I2C muxes is missing because of the locking issue (see later) which
may mean a redesign anyhow
* imported the omap support into this series to have another user. I didn't
pick up the patch for imx from Stefan because it is bigger and probably
needs seperate review first
* I converted the tegra-bpmp driver which already had handling for the atomic
case*. I did not convert the pxa driver which has a polling-only mode, too.
This also seems like a bigger task and its current behaviour shouldn't be
affected by this series. *only build tested
* added a HACK to allow the i2c-gpio driver atomic transfers. This will only
work if accessing the GPIO can be done in atomic contexts, too, so this is
for testing only
For the regular cases this series works well on my Renesas Lager board*
which needs an I2C access to the PMIC to reboot the board. *if I use the
i2c-gpio driver, the i2c-sh_mobile is not converted yet.
However, during the last review, Russell King brought up an interesting corner
case. What if we want to reboot because of a panic and the bus is not in a
consistent state? To create this situation, I recently created the 'inject-panic'
fault injector [1] which is merged into i2c/for-next meanwhile.
With this fault injector and 'reboot after panic' settings, I can create
the problem Russell described: a) the bus is in an inconsistent state because
the driver was interrupted (SCL/SDA both low) and b) the lock for this driver
is taken, so trylock fails.
I think b) is an interesting question: shall we give atomic transfers priority
and ignore the lock? Do we need a seperate one then (SMP is turned off already,
or?)? If so, that would probably mean way more complicated mux-locking code
(Peter?)? And what if some mux in the path needs interrupts? And how academic
is all that? Because someone putting the reboot functionality behind muxed I2C
is kind of asking for problems :)
That being said: this is an issue I think it is worth tackling. However, this
issue is not introduced by this series. It is already there. It might just
become more visible.
Sidenote: I think problem a) is easier once we solved b). E.g. if we decide on
a higher priority, we can postulate that IP cores should be reset first and
bus recovery can also be applied. This will not help all cases but IMO is all
we can do.
Another topic where I'd like input from other people is the use of 'in_atomic'
in this series. It was already there, so I kept it to avoid regressions. I am
aware that 'in_atomic' should not be used in drivers. So, if someone has
expertise to say if it can be removed or replaced with something else, I am
all ears.
A branch (based on i2c/for-next) can be found here:
git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/i2c/atomic_xfer
Sorry, no TLDR; text here - I think this topic deserves a few words ;)
Looking forward to comments, thanks!
Wolfram
[1] http://patchwork.ozlabs.org/patch/1044789/
Tero Kristo (1):
i2c: busses: omap: Add the master_xfer_irqless hook
Wolfram Sang (6):
i2c: apply coding style for struct i2c_adapter
i2c: core: use I2C locking behaviour also for SMBUS
i2c: core: introduce callbacks for atomic transfers
i2c: demux: WIP: handle the new atomic callbacks
i2c: tegra-bpmp: convert to use new atomic callbacks
i2c: algo: bit: HACK! add atomic callback
drivers/i2c/algos/i2c-algo-bit.c | 5 ++-
drivers/i2c/busses/i2c-omap.c | 79 +++++++++++++++++++++++++++++------
drivers/i2c/busses/i2c-tegra-bpmp.c | 27 +++++++++---
drivers/i2c/i2c-core-base.c | 17 ++++----
drivers/i2c/i2c-core-smbus.c | 25 ++++++++---
drivers/i2c/i2c-core.h | 15 +++++++
drivers/i2c/muxes/i2c-demux-pinctrl.c | 3 ++
include/linux/i2c.h | 38 +++++++++++------
8 files changed, 162 insertions(+), 47 deletions(-)
--
2.11.0
This is only for testing and will work only for bitbanging which is done
without generating irqs. Not for upstream!
Signed-off-by: Wolfram Sang <[email protected]>
---
drivers/i2c/algos/i2c-algo-bit.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/i2c/algos/i2c-algo-bit.c b/drivers/i2c/algos/i2c-algo-bit.c
index 5e5990a83da5..a9c361947de1 100644
--- a/drivers/i2c/algos/i2c-algo-bit.c
+++ b/drivers/i2c/algos/i2c-algo-bit.c
@@ -615,8 +615,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,
+ .functionality = bit_func,
};
EXPORT_SYMBOL(i2c_bit_algo);
--
2.11.0
Signed-off-by: Wolfram Sang <[email protected]>
---
include/linux/i2c.h | 23 +++++++++++++----------
1 file changed, 13 insertions(+), 10 deletions(-)
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 383510b4f083..758a6db864c9 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -517,20 +517,23 @@ i2c_register_board_info(int busnum, struct i2c_board_info const *info,
* Documentation file Documentation/i2c/fault-codes.
*/
struct i2c_algorithm {
- /* If an adapter algorithm can't do I2C-level access, set master_xfer
- to NULL. If an adapter algorithm can do SMBus access, set
- smbus_xfer. If set to NULL, the SMBus protocol is simulated
- using common I2C messages */
- /* master_xfer should return the number of messages successfully
- processed, or a negative value on error */
+ /*
+ * If an adapter algorithm can't do I2C-level access, set master_xfer
+ * to NULL. If an adapter algorithm can do SMBus access, set
+ * smbus_xfer. If set to NULL, the SMBus protocol is simulated
+ * using common I2C messages.
+ *
+ * master_xfer should return the number of messages successfully
+ * processed, or a negative value on error
+ */
int (*master_xfer)(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)(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 *);
+ u32 (*functionality)(struct i2c_adapter *adap);
#if IS_ENABLED(CONFIG_I2C_SLAVE)
int (*reg_slave)(struct i2c_client *client);
--
2.11.0
From: Tero Kristo <[email protected]>
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]>
Signed-off-by: Wolfram Sang <[email protected]>
---
drivers/i2c/busses/i2c-omap.c | 79 ++++++++++++++++++++++++++++++++++++-------
1 file changed, 66 insertions(+), 13 deletions(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index cd9c65f3d404..bf48126faf94 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,21 @@ 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);
+
+ if (!ret)
+ timeout = 1;
+ else
+ timeout = 0;
+ }
+
if (timeout == 0) {
dev_err(omap->dev, "controller timed out\n");
omap_i2c_reset(omap);
@@ -772,7 +801,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 +824,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 +844,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 +1078,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 +1097,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 +1207,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 004f8a3b6365..2127dd08ff01 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 ((in_atomic() || irqs_disabled()) && 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..e01a548fc559 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 (in_atomic() || irqs_disabled()) {
+ 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 6e98aa811980..01a6cb9b53aa 100644
--- a/drivers/i2c/i2c-core.h
+++ b/drivers/i2c/i2c-core.h
@@ -33,10 +33,13 @@ static inline int __i2c_lock_bus_helper(struct i2c_adapter *adap)
{
int ret = 0;
- if (in_atomic() || irqs_disabled())
+ if (in_atomic() || irqs_disabled()) {
+ 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..3cd921dd39e3 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} 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 {
/*
@@ -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 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 | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/i2c/muxes/i2c-demux-pinctrl.c b/drivers/i2c/muxes/i2c-demux-pinctrl.c
index 035032e20327..5d00adfbe578 100644
--- a/drivers/i2c/muxes/i2c-demux-pinctrl.c
+++ b/drivers/i2c/muxes/i2c-demux-pinctrl.c
@@ -99,6 +99,9 @@ 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;
+ /* FIXME: regular muxes need proper handling, too! */
+ 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
The driver did handle this internally, convert it to use the new
callbacks.
Signed-off-by: Wolfram Sang <[email protected]>
---
drivers/i2c/busses/i2c-tegra-bpmp.c | 27 ++++++++++++++++++++++-----
1 file changed, 22 insertions(+), 5 deletions(-)
diff --git a/drivers/i2c/busses/i2c-tegra-bpmp.c b/drivers/i2c/busses/i2c-tegra-bpmp.c
index f6cd35d0a2ac..02b78ba5b23b 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,20 @@ 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 +292,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 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]>
Acked-by: Peter Rosin <[email protected]>
Acked-by: Tony Lindgren <[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 cb6c5cb0df0b..004f8a3b6365 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 (in_atomic() || irqs_disabled()) {
- 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 37576f50fe20..6e98aa811980 100644
--- a/drivers/i2c/i2c-core.h
+++ b/drivers/i2c/i2c-core.h
@@ -29,6 +29,18 @@ extern int __i2c_first_dynamic_bus_num;
int i2c_check_7bit_addr_validity_strict(unsigned short addr);
+static inline int __i2c_lock_bus_helper(struct i2c_adapter *adap)
+{
+ int ret = 0;
+
+ if (in_atomic() || irqs_disabled())
+ 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
On Sat, Mar 2, 2019 at 3:49 PM Wolfram Sang
<[email protected]> wrote:
>
> So, finally, here is the second RFC for supporting I2C transfers in atomic
> contexts (i.e. very late). This will need some text because I tried some things
> on the way but had to discard them. However, I think it is important to have
> that documented.
> Sorry, no TLDR; text here - I think this topic deserves a few words ;)
Thank you for this work! It was indeed interesting reading.
And since your series is targetting some exiting use cases, I would
drop as well academic variants of brain-damaged hw design, I think it
worth to go.
WRT patches,
Reviewed-by: Andy Shevchenko <[email protected]>
for patches 1-2
and FWIW for patches 3-4.
For the individual drivers I can't say much, code looks good, but I
dunno if it's correct or not.
>
> Looking forward to comments, thanks!
>
> Wolfram
>
>
> [1] http://patchwork.ozlabs.org/patch/1044789/
>
>
> Tero Kristo (1):
> i2c: busses: omap: Add the master_xfer_irqless hook
>
> Wolfram Sang (6):
> i2c: apply coding style for struct i2c_adapter
> i2c: core: use I2C locking behaviour also for SMBUS
> i2c: core: introduce callbacks for atomic transfers
> i2c: demux: WIP: handle the new atomic callbacks
> i2c: tegra-bpmp: convert to use new atomic callbacks
> i2c: algo: bit: HACK! add atomic callback
>
> drivers/i2c/algos/i2c-algo-bit.c | 5 ++-
> drivers/i2c/busses/i2c-omap.c | 79 +++++++++++++++++++++++++++++------
> drivers/i2c/busses/i2c-tegra-bpmp.c | 27 +++++++++---
> drivers/i2c/i2c-core-base.c | 17 ++++----
> drivers/i2c/i2c-core-smbus.c | 25 ++++++++---
> drivers/i2c/i2c-core.h | 15 +++++++
> drivers/i2c/muxes/i2c-demux-pinctrl.c | 3 ++
> include/linux/i2c.h | 38 +++++++++++------
> 8 files changed, 162 insertions(+), 47 deletions(-)
>
> --
> 2.11.0
>
--
With Best Regards,
Andy Shevchenko
On Sat, Mar 2, 2019 at 2:49 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.
>
> Signed-off-by: Wolfram Sang <[email protected]>
> Acked-by: Peter Rosin <[email protected]>
> Acked-by: Tony Lindgren <[email protected]>
Reviewed-by: Geert Uytterhoeven <[email protected]>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On 2.3.2019 15.47, Wolfram Sang wrote:
> The driver did handle this internally, convert it to use the new
> callbacks.
>
> Signed-off-by: Wolfram Sang <[email protected]>
> ---
> drivers/i2c/busses/i2c-tegra-bpmp.c | 27 ++++++++++++++++++++++-----
> 1 file changed, 22 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-tegra-bpmp.c b/drivers/i2c/busses/i2c-tegra-bpmp.c
> index f6cd35d0a2ac..02b78ba5b23b 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,20 @@ 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 +292,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,
> };
>
>
Looks good to me.
Reviewed-by: Timo Alho <[email protected]>
On Sat, Mar 02, 2019 at 02:47:34PM +0100, Wolfram Sang wrote:
> The driver did handle this internally, convert it to use the new
> callbacks.
>
> Signed-off-by: Wolfram Sang <[email protected]>
> ---
> drivers/i2c/busses/i2c-tegra-bpmp.c | 27 ++++++++++++++++++++++-----
> 1 file changed, 22 insertions(+), 5 deletions(-)
Acked-by: Thierry Reding <[email protected]>
On 2019-03-02 14:47, Wolfram Sang wrote:
> So, finally, here is the second RFC for supporting I2C transfers in atomic
> contexts (i.e. very late). This will need some text because I tried some things
> on the way but had to discard them. However, I think it is important to have
> that documented.
>
> One thing I really wanted to have is a kind of whitelist for devices which are
> allowed to use atomic transfers. So we could identify the "unauthorized" ones
> as buggy. To be useful, this should not add new API calls for transfers,
> otherwise things would have become way more complicated for I2C users like
> regmap. So, I tried e.g. to flag clients and provide that information
> throughout the i2c tree (think muxes here). In the end, I concluded that this
> is not an I2C specific problem, so it can't have an I2C specifc solution.
> Imagine a GPIO which is needed to reboot (drivers/power/reset/gpio-restart.c).
> This is the device which needs to whitelisted but the driver doesn't even know
> if the GPIO is behind I2C or not. So, if we want this, it should probably be
> handled on 'struct device' level. Including all the hierarchy. Postponed.
>
> So, this RFC v2 is much more similar to v1 than I expected. Main changes:
>
> * cleaned up 'struct i2c_adapter' a bit before adding the new stuff
> * added an atomic callback for SMBus, too. Only build-tested so far. But spent
> a few braincells of getting the SMBus logic readable because we could have
> an I2C fallback just for the atomic case
> * add a WARN for atomic transfers with no atomic transfer handler
> * added support for the i2c-demuxer, so I could test the series. Support
> for I2C muxes is missing because of the locking issue (see later) which
> may mean a redesign anyhow
> * imported the omap support into this series to have another user. I didn't
> pick up the patch for imx from Stefan because it is bigger and probably
> needs seperate review first
> * I converted the tegra-bpmp driver which already had handling for the atomic
> case*. I did not convert the pxa driver which has a polling-only mode, too.
> This also seems like a bigger task and its current behaviour shouldn't be
> affected by this series. *only build tested
> * added a HACK to allow the i2c-gpio driver atomic transfers. This will only
> work if accessing the GPIO can be done in atomic contexts, too, so this is
> for testing only
>
> For the regular cases this series works well on my Renesas Lager board*
> which needs an I2C access to the PMIC to reboot the board. *if I use the
> i2c-gpio driver, the i2c-sh_mobile is not converted yet.
>
> However, during the last review, Russell King brought up an interesting corner
> case. What if we want to reboot because of a panic and the bus is not in a
> consistent state? To create this situation, I recently created the 'inject-panic'
> fault injector [1] which is merged into i2c/for-next meanwhile.
>
> With this fault injector and 'reboot after panic' settings, I can create
> the problem Russell described: a) the bus is in an inconsistent state because
> the driver was interrupted (SCL/SDA both low) and b) the lock for this driver
> is taken, so trylock fails.
>
> I think b) is an interesting question: shall we give atomic transfers priority
> and ignore the lock? Do we need a seperate one then (SMP is turned off already,
> or?)? If so, that would probably mean way more complicated mux-locking code
> (Peter?)? And what if some mux in the path needs interrupts? And how academic
> is all that? Because someone putting the reboot functionality behind muxed I2C
> is kind of asking for problems :)
>
> That being said: this is an issue I think it is worth tackling. However, this
> issue is not introduced by this series. It is already there. It might just
> become more visible.
The way I read this series, you are not giving atomic transfers priority. The
only thing that happens is that if an xfer happens in atomic/irq context,
trylock is used instead of an ordinary (unconditional) lock (this is just
like it is already). If a mux is sitting in between the client device and
the root adapter, the trylock operation will percolate to the root. Sure,
there are more trylock ops that may fail and abort the xfer, but if
everything is uncontended, then things should proceed in orderly fashion.
Also, sure, the mux may need additional resources that are no longer
available if the machine is half way down (or worse). But I don't see any
fundamental *locking* issue with muxes that is different from the case
without a mux.
That said, if you then want to introduce xfers that want to circumvent the
locking, then parent-locked muxes are easier since the actual muxing operation
is performed as an unlocked xfer (if one is needed) while the client device
has grabbed the adapter lock "from the outside". Sure, there is a list of
locks going up through the adapter tree to handle, but that can probably be
handled in one place. I.e. the locking must have been avoided prior to the
actual muxing operation, but the code to do so can be in one place. The
mux-locked case is where the trouble is, since the muxing operation is done
as a normal xfer and needs to be classified as a special xfer that just like
the original client xfer also needs to break through any existing locks in
the adapter tree. And those muxing xfers might come from anywhere, e.g.
- IO-expander controlling a gpio/pinctrl mux
- dedicated I2C mux (e.g. the LTC4306)
- regmap device
- etc, who knows what muxing options will evolve?
So, any scheme that require a white-list will work poorly for mux-locked
muxes, unless you can add some new grip/pinctrl/regmap flags to
gpios/pins/registers so that the particular accesses can be white-listed.
Adding those flags seem rather invasive?
But of course, you need to actually do something about the added FIXME in
the demux-pinctrl driver... BTW, that driver should forward ->smbus_xfer
just like it does for ->master_xfer, no?
Cheers,
Peter
> Sidenote: I think problem a) is easier once we solved b). E.g. if we decide on
> a higher priority, we can postulate that IP cores should be reset first and
> bus recovery can also be applied. This will not help all cases but IMO is all
> we can do.
>
> Another topic where I'd like input from other people is the use of 'in_atomic'
> in this series. It was already there, so I kept it to avoid regressions. I am
> aware that 'in_atomic' should not be used in drivers. So, if someone has
> expertise to say if it can be removed or replaced with something else, I am
> all ears.
>
> A branch (based on i2c/for-next) can be found here:
> git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/i2c/atomic_xfer
>
> Sorry, no TLDR; text here - I think this topic deserves a few words ;)
>
> Looking forward to comments, thanks!
>
> Wolfram
>
>
> [1] http://patchwork.ozlabs.org/patch/1044789/
>
>
> Tero Kristo (1):
> i2c: busses: omap: Add the master_xfer_irqless hook
>
> Wolfram Sang (6):
> i2c: apply coding style for struct i2c_adapter
> i2c: core: use I2C locking behaviour also for SMBUS
> i2c: core: introduce callbacks for atomic transfers
> i2c: demux: WIP: handle the new atomic callbacks
> i2c: tegra-bpmp: convert to use new atomic callbacks
> i2c: algo: bit: HACK! add atomic callback
>
> drivers/i2c/algos/i2c-algo-bit.c | 5 ++-
> drivers/i2c/busses/i2c-omap.c | 79 +++++++++++++++++++++++++++++------
> drivers/i2c/busses/i2c-tegra-bpmp.c | 27 +++++++++---
> drivers/i2c/i2c-core-base.c | 17 ++++----
> drivers/i2c/i2c-core-smbus.c | 25 ++++++++---
> drivers/i2c/i2c-core.h | 15 +++++++
> drivers/i2c/muxes/i2c-demux-pinctrl.c | 3 ++
> include/linux/i2c.h | 38 +++++++++++------
> 8 files changed, 162 insertions(+), 47 deletions(-)
>
Hi Peda,
> The way I read this series, you are not giving atomic transfers priority. The
You are reading correctly. I could have made more clear that the issue
pointed out by Russell is not handled by this series but discussion
about it is welcome / needed to decide if we can take this series as is
or if we need to redesign it. But here we are anyhow :)
> only thing that happens is that if an xfer happens in atomic/irq context,
> trylock is used instead of an ordinary (unconditional) lock (this is just
> like it is already). If a mux is sitting in between the client device and
> the root adapter, the trylock operation will percolate to the root. Sure,
> there are more trylock ops that may fail and abort the xfer, but if
> everything is uncontended, then things should proceed in orderly fashion.
> Also, sure, the mux may need additional resources that are no longer
> available if the machine is half way down (or worse). But I don't see any
> fundamental *locking* issue with muxes that is different from the case
> without a mux.
Good, that was my conclusion as well. The series, as is, doesn't change
the locking behaviour, so that will work exactly as before. Or, it will
not work in the case described by Russell. Like before.
> That said, if you then want to introduce xfers that want to circumvent the
> locking, then parent-locked muxes are easier since the actual muxing operation
> is performed as an unlocked xfer (if one is needed) while the client device
> has grabbed the adapter lock "from the outside". Sure, there is a list of
> locks going up through the adapter tree to handle, but that can probably be
> handled in one place. I.e. the locking must have been avoided prior to the
> actual muxing operation, but the code to do so can be in one place. The
That was my gut feeling, too...
> mux-locked case is where the trouble is, since the muxing operation is done
> as a normal xfer and needs to be classified as a special xfer that just like
> the original client xfer also needs to break through any existing locks in
> the adapter tree. And those muxing xfers might come from anywhere, e.g.
>
> - IO-expander controlling a gpio/pinctrl mux
> - dedicated I2C mux (e.g. the LTC4306)
> - regmap device
> - etc, who knows what muxing options will evolve?
>
> So, any scheme that require a white-list will work poorly for mux-locked
> muxes, unless you can add some new grip/pinctrl/regmap flags to
> gpios/pins/registers so that the particular accesses can be white-listed.
> Adding those flags seem rather invasive?
... and sadly, this too. We would need the same kind of flag which I
described in my first paragraph of the original posting where I wanted
the flag to detect "unauthorized" uses of late I2C transfers. And this
is gonna be invasive. And I am not sure it is worth the effort.
I wonder what a reasonable effort is? Simply ignore the lock from the
"current" adapter and hope for the best that there is no mux or at
least no mux which needs interrupts / a lock attached to it?
> But of course, you need to actually do something about the added FIXME in
> the demux-pinctrl driver... BTW, that driver should forward ->smbus_xfer
> just like it does for ->master_xfer, no?
Yes. The idea of having two seperate SMBus controllers in one SoC which
would need demuxing is amusing, but still, it exists for I2C and you are
right.
Thanks,
Wolfram
On 2019-03-04 23:48, Wolfram Sang wrote:
> Hi Peda,
>
>> The way I read this series, you are not giving atomic transfers priority. The
>
> You are reading correctly. I could have made more clear that the issue
> pointed out by Russell is not handled by this series but discussion
> about it is welcome / needed to decide if we can take this series as is
> or if we need to redesign it. But here we are anyhow :)
>
>> only thing that happens is that if an xfer happens in atomic/irq context,
>> trylock is used instead of an ordinary (unconditional) lock (this is just
>> like it is already). If a mux is sitting in between the client device and
>> the root adapter, the trylock operation will percolate to the root. Sure,
>> there are more trylock ops that may fail and abort the xfer, but if
>> everything is uncontended, then things should proceed in orderly fashion.
>> Also, sure, the mux may need additional resources that are no longer
>> available if the machine is half way down (or worse). But I don't see any
>> fundamental *locking* issue with muxes that is different from the case
>> without a mux.
>
> Good, that was my conclusion as well. The series, as is, doesn't change
> the locking behaviour, so that will work exactly as before. Or, it will
> not work in the case described by Russell. Like before.
>
>> That said, if you then want to introduce xfers that want to circumvent the
>> locking, then parent-locked muxes are easier since the actual muxing operation
>> is performed as an unlocked xfer (if one is needed) while the client device
>> has grabbed the adapter lock "from the outside". Sure, there is a list of
>> locks going up through the adapter tree to handle, but that can probably be
>> handled in one place. I.e. the locking must have been avoided prior to the
>> actual muxing operation, but the code to do so can be in one place. The
>
> That was my gut feeling, too...
>
>> mux-locked case is where the trouble is, since the muxing operation is done
>> as a normal xfer and needs to be classified as a special xfer that just like
>> the original client xfer also needs to break through any existing locks in
>> the adapter tree. And those muxing xfers might come from anywhere, e.g.
>>
>> - IO-expander controlling a gpio/pinctrl mux
>> - dedicated I2C mux (e.g. the LTC4306)
>> - regmap device
>> - etc, who knows what muxing options will evolve?
>>
>> So, any scheme that require a white-list will work poorly for mux-locked
>> muxes, unless you can add some new grip/pinctrl/regmap flags to
s/grip/gpio/ of course
>> gpios/pins/registers so that the particular accesses can be white-listed.
>> Adding those flags seem rather invasive?
>
> ... and sadly, this too. We would need the same kind of flag which I
> described in my first paragraph of the original posting where I wanted
> the flag to detect "unauthorized" uses of late I2C transfers. And this
> is gonna be invasive. And I am not sure it is worth the effort.
>
> I wonder what a reasonable effort is? Simply ignore the lock from the
> "current" adapter and hope for the best that there is no mux or at
> least no mux which needs interrupts / a lock attached to it?
Just wanted to add a note that the underlying problem is similar to why
I introduced the mux-locked concept. There is no simple way to identify
*exactly* which xfers that need to be unlocked. Going only by call site
is not enough, since the same call in different context may need to be
muxed (in my case) or irq-less (in this case). If someone comes up with
a solution for that, all muxes can be converted to the parent-locked
scheme and we can get rid of a bunch of complexity. I just don't see how
though, all ideas I have come up with I have immediately discarded as way
too invasive, ugly and/or error prone.
>> But of course, you need to actually do something about the added FIXME in
>> the demux-pinctrl driver... BTW, that driver should forward ->smbus_xfer
>> just like it does for ->master_xfer, no?
>
> Yes. The idea of having two seperate SMBus controllers in one SoC which
> would need demuxing is amusing, but still, it exists for I2C and you are
> right.
Right, I didn't actually think all that far... :-)
Cheers,
Peter
> > So, finally, here is the second RFC for supporting I2C transfers in atomic
> > contexts (i.e. very late). This will need some text because I tried some things
> > on the way but had to discard them. However, I think it is important to have
> > that documented.
>
> > Sorry, no TLDR; text here - I think this topic deserves a few words ;)
>
> Thank you for this work! It was indeed interesting reading.
Thanks, glad to hear that :)
> And since your series is targetting some exiting use cases, I would
> drop as well academic variants of brain-damaged hw design, I think it
> worth to go.
Well, yes and no. I am with you that some complicated muxed setups could
be argued to be way over the top. However, with the panic
fault-injector, I can get my simple "PMIC directly (even exclusively)
attached to root adapter" setup to stall. By simply ignoring the lock,
such setups could work again. But this series does not implement this
because it would need a redesign, i.e. tie into i2c_transfer and not
later in __i2c_transfer.
Yet, before doing this, I was interested in discussion if ignoring the
lock is really desirable.
This series seems like a valid approach to me if we still want to
respect the locking.
On Sat, Mar 02, 2019 at 02:47:29PM +0100, Wolfram Sang wrote:
> Signed-off-by: Wolfram Sang <[email protected]>
Reviewed-by: Simon Horman <[email protected]>
> ---
> include/linux/i2c.h | 23 +++++++++++++----------
> 1 file changed, 13 insertions(+), 10 deletions(-)
>
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index 383510b4f083..758a6db864c9 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -517,20 +517,23 @@ i2c_register_board_info(int busnum, struct i2c_board_info const *info,
> * Documentation file Documentation/i2c/fault-codes.
> */
> struct i2c_algorithm {
> - /* If an adapter algorithm can't do I2C-level access, set master_xfer
> - to NULL. If an adapter algorithm can do SMBus access, set
> - smbus_xfer. If set to NULL, the SMBus protocol is simulated
> - using common I2C messages */
> - /* master_xfer should return the number of messages successfully
> - processed, or a negative value on error */
> + /*
> + * If an adapter algorithm can't do I2C-level access, set master_xfer
> + * to NULL. If an adapter algorithm can do SMBus access, set
> + * smbus_xfer. If set to NULL, the SMBus protocol is simulated
> + * using common I2C messages.
> + *
> + * master_xfer should return the number of messages successfully
> + * processed, or a negative value on error
> + */
> int (*master_xfer)(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)(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 *);
> + u32 (*functionality)(struct i2c_adapter *adap);
>
> #if IS_ENABLED(CONFIG_I2C_SLAVE)
> int (*reg_slave)(struct i2c_client *client);
> --
> 2.11.0
>
On Sat, Mar 02, 2019 at 02:47:30PM +0100, Wolfram Sang 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.
>
> Signed-off-by: Wolfram Sang <[email protected]>
> Acked-by: Peter Rosin <[email protected]>
> Acked-by: Tony Lindgren <[email protected]>
Reviewed-by: Simon Horman <[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 cb6c5cb0df0b..004f8a3b6365 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 (in_atomic() || irqs_disabled()) {
> - 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 37576f50fe20..6e98aa811980 100644
> --- a/drivers/i2c/i2c-core.h
> +++ b/drivers/i2c/i2c-core.h
> @@ -29,6 +29,18 @@ extern int __i2c_first_dynamic_bus_num;
>
> int i2c_check_7bit_addr_validity_strict(unsigned short addr);
>
> +static inline int __i2c_lock_bus_helper(struct i2c_adapter *adap)
> +{
> + int ret = 0;
> +
> + if (in_atomic() || irqs_disabled())
> + 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
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
On Sat, Mar 02, 2019 at 02:47:31PM +0100, 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]>
> ---
> 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 004f8a3b6365..2127dd08ff01 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 ((in_atomic() || irqs_disabled()) && 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..e01a548fc559 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 (in_atomic() || irqs_disabled()) {
> + 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 6e98aa811980..01a6cb9b53aa 100644
> --- a/drivers/i2c/i2c-core.h
> +++ b/drivers/i2c/i2c-core.h
> @@ -33,10 +33,13 @@ static inline int __i2c_lock_bus_helper(struct i2c_adapter *adap)
> {
> int ret = 0;
>
> - if (in_atomic() || irqs_disabled())
> + if (in_atomic() || irqs_disabled()) {
> + WARN(!adap->algo->master_xfer_atomic && !adap->algo->smbus_xfer_atomic,
> + "No atomic I2C transfer handler for '%s'\n", dev_name(&adap->dev));
Is WARN_ONCE more appropriate here?
> 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..3cd921dd39e3 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} field should indicate the
I think "field" should be "fields" in the new text.
> + * 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
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
On Sat, Mar 02, 2019 at 02:47:32PM +0100, Wolfram Sang wrote:
> 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]>
Reviewed-by: Simon Horman <[email protected]>
> ---
> drivers/i2c/muxes/i2c-demux-pinctrl.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/i2c/muxes/i2c-demux-pinctrl.c b/drivers/i2c/muxes/i2c-demux-pinctrl.c
> index 035032e20327..5d00adfbe578 100644
> --- a/drivers/i2c/muxes/i2c-demux-pinctrl.c
> +++ b/drivers/i2c/muxes/i2c-demux-pinctrl.c
> @@ -99,6 +99,9 @@ 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;
> + /* FIXME: regular muxes need proper handling, too! */
> + 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
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
On Sat, Mar 02, 2019 at 02:47:34PM +0100, Wolfram Sang wrote:
> The driver did handle this internally, convert it to use the new
> callbacks.
>
> Signed-off-by: Wolfram Sang <[email protected]>
Reviewed-by: Simon Horman <[email protected]>
> ---
> drivers/i2c/busses/i2c-tegra-bpmp.c | 27 ++++++++++++++++++++++-----
> 1 file changed, 22 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-tegra-bpmp.c b/drivers/i2c/busses/i2c-tegra-bpmp.c
> index f6cd35d0a2ac..02b78ba5b23b 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,20 @@ 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 +292,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
>
On Sat, Mar 02, 2019 at 02:47:33PM +0100, Wolfram Sang wrote:
> From: Tero Kristo <[email protected]>
>
> 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]>
> Signed-off-by: Wolfram Sang <[email protected]>
Minor nit below not withstanding
Reviewed-by: Simon Horman <[email protected]>
> ---
> drivers/i2c/busses/i2c-omap.c | 79 ++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 66 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index cd9c65f3d404..bf48126faf94 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,21 @@ 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);
> +
> + if (!ret)
> + timeout = 1;
> + else
> + timeout = 0;
nit: the following might be cleaner
ret = !!timeout;
> + }
> +
> if (timeout == 0) {
> dev_err(omap->dev, "controller timed out\n");
> omap_i2c_reset(omap);
> @@ -772,7 +801,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 +824,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 +844,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 +1078,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 +1097,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 +1207,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
>
On Fri, Mar 15, 2019 at 01:47:18PM +0100, Simon Horman wrote:
> On Sat, Mar 02, 2019 at 02:47:33PM +0100, Wolfram Sang wrote:
> > + 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);
> > +
> > + if (!ret)
> > + timeout = 1;
> > + else
> > + timeout = 0;
>
>
> nit: the following might be cleaner
>
> ret = !!timeout;
>
Other way around, perhaps,
timeout = !ret;
> > + }
--
With Best Regards,
Andy Shevchenko
> This series seems like a valid approach to me if we still want to
> respect the locking.
And I am leaning to that it is good enough. I think pragmatism is OK
here: The users who want this feature simply want to reboot their
system, mostly development systems. They can't reboot otherwise. Except
for the HW switch they are currently using anyhow.
The panic fault-injector can create an inconsistent situation on the I2C
bus when you want to reboot after an OOPS while a transfer was in
progress. However, if rebooting in such scenarios is critical for you,
you a) shouldn't reboot via I2C, and/or b) should have a watchdog in
place. We can't guarantee to always fix this sitution. At best, we could
just try to be better for some cases. However, this would mean having a
backdoor to skip the locking scheme which doesn't sound right. Maybe
just accepting the deadlock is not too bad because it will reliably
point out a design flaw of the system (hopefully during the development
stage)?
Final call for other thoughts/comments.
PS: I am still interested in the use of in_atomic() here. I wonder if it is
correct. If a change is needed, it should probably be a seperate series,
though.
Hi Wolfram,
On Sat, Mar 02, 2019 at 02:47:34PM +0100, Wolfram Sang wrote:
> The driver did handle this internally, convert it to use the new
> callbacks.
>
> Signed-off-by: Wolfram Sang <[email protected]>
> ---
> drivers/i2c/busses/i2c-tegra-bpmp.c | 27 ++++++++++++++++++++++-----
> 1 file changed, 22 insertions(+), 5 deletions(-)
>
[...]
> @@ -268,6 +270,20 @@ 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)
> +
Here and between the function prototype of 'tegra_bpmp_i2c_xfer' and
it's function body is a unneeded blank line. Nitpick.
Kind regards,
Stefan
> +{
> + 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 +292,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
>
>
On Sat, Mar 02, 2019 at 02:47:29PM +0100, Wolfram Sang wrote:
> Signed-off-by: Wolfram Sang <[email protected]>
I think this preparational patch can be applied right away.
Applied to for-next, thanks!
Hi Simon,
please delete unrelated text. I nearly missed the typo fix later.
> > - if (in_atomic() || irqs_disabled())
> > + if (in_atomic() || irqs_disabled()) {
> > + WARN(!adap->algo->master_xfer_atomic && !adap->algo->smbus_xfer_atomic,
> > + "No atomic I2C transfer handler for '%s'\n", dev_name(&adap->dev));
>
> Is WARN_ONCE more appropriate here?
Why? It could be multiple adapters or clients causing this?
> > + * The return codes from the @master_xfer{_atomic} field should indicate the
>
> I think "field" should be "fields" in the new text.
Fixed, thanks!
Regards,
Wolfram
> > > + if (!ret)
> > > + timeout = 1;
> > > + else
> > > + timeout = 0;
> >
> >
> > nit: the following might be cleaner
> >
> > ret = !!timeout;
> >
>
> Other way around, perhaps,
>
> timeout = !ret;
I saw that but didn't want to modify this patch. But yes, this is easy
enough and looks a lot better.
> > +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)
> > +
>
> Here and between the function prototype of 'tegra_bpmp_i2c_xfer' and
> it's function body is a unneeded blank line. Nitpick.
Thanks, fixed!
> >> the demux-pinctrl driver... BTW, that driver should forward ->smbus_xfer
> >> just like it does for ->master_xfer, no?
> >
> > Yes. The idea of having two seperate SMBus controllers in one SoC which
> > would need demuxing is amusing, but still, it exists for I2C and you are
> > right.
>
> Right, I didn't actually think all that far... :-)
On second thought, we can add smbus support when we need it.
Hi Wolfram,
On Wed, Mar 27, 2019 at 02:47:51PM +0100, Wolfram Sang wrote:
> Hi Simon,
>
> please delete unrelated text. I nearly missed the typo fix later.
Ack, will do.
> > > - if (in_atomic() || irqs_disabled())
> > > + if (in_atomic() || irqs_disabled()) {
> > > + WARN(!adap->algo->master_xfer_atomic && !adap->algo->smbus_xfer_atomic,
> > > + "No atomic I2C transfer handler for '%s'\n", dev_name(&adap->dev));
> >
> > Is WARN_ONCE more appropriate here?
>
> Why? It could be multiple adapters or clients causing this?
Good point. I am concerned about the presence of a case which
causes a logging storm.
...
On Wed, Mar 27, 2019 at 02:50:45PM +0100, Wolfram Sang wrote:
>
> > > > + if (!ret)
> > > > + timeout = 1;
> > > > + else
> > > > + timeout = 0;
> > >
> > >
> > > nit: the following might be cleaner
> > >
> > > ret = !!timeout;
> > >
> >
> > Other way around, perhaps,
> >
> > timeout = !ret;
>
> I saw that but didn't want to modify this patch. But yes, this is easy
> enough and looks a lot better.
Thanks, I don't think this needs to block progress.