| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAF8uH3u9L1cVyAZiY=981bDewYgVYM=27kcV0GwqHFURg21FgA@mail.gmail.com>
Date: Tue, 19 Mar 2024 22:20:47 -0600
From: Sam Edwards <sam@...ingpi.com>
To: Gregory CLEMENT <gregory.clement@...tlin.com>, Andi Shyti <andi.shyti@...nel.org>
Cc: linux-i2c@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: [RESEND RFC PATCH 5/5] i2c: mv64xxx: Implement I2C_FUNC_NOSTART
There are certain I2C devices [1] that require some extra parameter bytes
in any read operation, after the start byte but before the bus turnaround,
in contrast to the "conventional" approach of using a normal write to
parameterize the upcoming read.
The (Allwinner variant of) mv64xxx supports this kind of transfer, for
up to 3 bytes of additional data, through a register called "TWI_EFR"
("Enhance Feature Register"). These devices can be supported by setting
this register appropriately when beginning the read operation.
In absence of a specialized flag to represent this in Linux's I2C
subsystem, clients use I2C_M_NOSTART to achieve this. In fact, this
appears to be the most common use of I2C_M_NOSTART in the Linux
codebase, with the second most common being to implement "gather" I/O by
chaining a series of NOSTART writes.
This patch implements both of these use cases when the EFR is present:
1) Continuing a write message with further write bytes, as a sort of
"gather" operation.
2) Inserting extra parameter bytes after the address of a read
operation, using a zero-byte read, a small (<= 3 bytes) NOSTART
write, and then a NOSTART read.
..the hardware is likely too strict to allow any other uses, so the
message bundle is checked for proper use of NOSTART before it begins.
That being said, there's a good chance that these are the only two uses
of NOSTART "in the wild," which would mean that this isn't much of a
limitation.
The implementation redesigns the hardware event handler slightly, so
that the FSM can be invoked in a loop if the do_action function
generates follow-up events. The NEXT_MESSAGE (formerly SEND_RESTART)
action now results in either a restart, or (for NOSTART) a follow-up
NOSTART event to the FSM, which allows it to bypass the entire start
sequence and jump straight to the data.
[1]: See e.g. `as5011_i2c_read`, `ivch_read`, `maven_get_reg`
Signed-off-by: Sam Edwards <sam@...ingpi.com>
---
drivers/i2c/busses/i2c-mv64xxx.c | 105 +++++++++++++++++++++++++++----
1 file changed, 94 insertions(+), 11 deletions(-)
diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
index 6a205cca603a..f09f23404784 100644
--- a/drivers/i2c/busses/i2c-mv64xxx.c
+++ b/drivers/i2c/busses/i2c-mv64xxx.c
@@ -100,6 +100,7 @@ enum mv64xxx_i2c_state {
enum mv64xxx_i2c_event {
MV64XXX_I2C_EVENT_INVALID,
MV64XXX_I2C_EVENT_STARTED,
+ MV64XXX_I2C_EVENT_NOSTART,
MV64XXX_I2C_EVENT_ADDR_ACK,
MV64XXX_I2C_EVENT_ADDR_NO_ACK,
MV64XXX_I2C_EVENT_WR_ACK,
@@ -112,7 +113,7 @@ enum mv64xxx_i2c_event {
enum mv64xxx_i2c_action {
MV64XXX_I2C_ACTION_INVALID,
MV64XXX_I2C_ACTION_CONTINUE,
- MV64XXX_I2C_ACTION_SEND_RESTART,
+ MV64XXX_I2C_ACTION_NEXT_MESSAGE,
MV64XXX_I2C_ACTION_SEND_ADDR_1,
MV64XXX_I2C_ACTION_SEND_ADDR_2,
MV64XXX_I2C_ACTION_SEND_DATA,
@@ -129,6 +130,7 @@ struct mv64xxx_i2c_regs {
u8 status;
u8 clock;
u8 soft_reset;
+ u8 enh_feat;
};
struct mv64xxx_i2c_data {
@@ -185,6 +187,7 @@ static struct mv64xxx_i2c_regs mv64xxx_i2c_regs_sun4i = {
.status = 0x10,
.clock = 0x14,
.soft_reset = 0x18,
+ .enh_feat = 0x1c,
};
static void
@@ -306,9 +309,9 @@ mv64xxx_i2c_decode_status(struct mv64xxx_i2c_data
*drv_data, u32 status)
}
static void
-mv64xxx_i2c_fsm(struct mv64xxx_i2c_data *drv_data, u32 status)
+mv64xxx_i2c_fsm(struct mv64xxx_i2c_data *drv_data,
+ enum mv64xxx_i2c_event event, u32 status)
{
- enum mv64xxx_i2c_event event;
enum mv64xxx_i2c_state prev_state = drv_data->state;
drv_data->action = MV64XXX_I2C_ACTION_INVALID;
@@ -329,7 +332,6 @@ mv64xxx_i2c_fsm(struct mv64xxx_i2c_data *drv_data,
u32 status)
* 2) Handle hardware event driven state transitions
* 3) Perform internal state transitions and action emission
*/
- event = mv64xxx_i2c_decode_status(drv_data, status);
/* Handle event; determine state transition */
switch (event) {
@@ -337,6 +339,7 @@ mv64xxx_i2c_fsm(struct mv64xxx_i2c_data *drv_data,
u32 status)
drv_data->state = MV64XXX_I2C_STATE_SEND_ADDR_1;
break;
+ case MV64XXX_I2C_EVENT_NOSTART:
case MV64XXX_I2C_EVENT_ADDR_ACK:
if ((drv_data->state == MV64XXX_I2C_STATE_SEND_ADDR_1)
&& (drv_data->msg->flags & I2C_M_TEN))
@@ -399,7 +402,7 @@ mv64xxx_i2c_fsm(struct mv64xxx_i2c_data *drv_data,
u32 status)
drv_data->action = MV64XXX_I2C_ACTION_SEND_STOP;
drv_data->state = MV64XXX_I2C_STATE_IDLE;
} else {
- drv_data->action =
MV64XXX_I2C_ACTION_SEND_RESTART;
+ drv_data->action =
MV64XXX_I2C_ACTION_NEXT_MESSAGE;
drv_data->state = MV64XXX_I2C_STATE_RESTART;
}
} else {
@@ -429,7 +432,7 @@ mv64xxx_i2c_fsm(struct mv64xxx_i2c_data *drv_data,
u32 status)
drv_data->action = MV64XXX_I2C_ACTION_SEND_STOP;
drv_data->state = MV64XXX_I2C_STATE_IDLE;
} else {
- drv_data->action =
MV64XXX_I2C_ACTION_SEND_RESTART;
+ drv_data->action =
MV64XXX_I2C_ACTION_NEXT_MESSAGE;
drv_data->state = MV64XXX_I2C_STATE_RESTART;
}
} else {
@@ -444,18 +447,38 @@ mv64xxx_i2c_fsm(struct mv64xxx_i2c_data
*drv_data, u32 status)
static void mv64xxx_i2c_send_start(struct mv64xxx_i2c_data *drv_data)
{
+ u8 extra_bytes;
+
drv_data->msg = drv_data->msgs;
drv_data->byte_posn = 0;
drv_data->bytes_left = drv_data->msg->len;
drv_data->aborting = 0;
drv_data->rc = 0;
+ if (drv_data->msg->flags & I2C_M_NOSTART)
+ return;
+
+ /*
+ * If this is a zero-length read, and the next message is a NOSTART
+ * write, the client driver is trying to insert extra bytes after the
+ * address but before the read proper.
+ */
+ if ((drv_data->msg->len == 0) && (drv_data->msg->flags & I2C_M_RD) &&
+ (drv_data->num_msgs > 1) && (drv_data->msgs[1].flags ==
I2C_M_NOSTART))
+ extra_bytes = drv_data->msgs[1].len;
+ else
+ extra_bytes = 0;
+
+ if (drv_data->reg_offsets.enh_feat != 0)
+ writel(extra_bytes,
+ drv_data->reg_base + drv_data->reg_offsets.enh_feat);
+
mv64xxx_i2c_prepare_for_io(drv_data, drv_data->msgs);
writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_START,
drv_data->reg_base + drv_data->reg_offsets.control);
}
-static void
+static enum mv64xxx_i2c_event
mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
{
if (drv_data->action & MV64XXX_I2C_ACTION_RECEIVE)
@@ -464,7 +487,7 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
drv_data->action &= ~MV64XXX_I2C_ACTION_RECEIVE;
switch(drv_data->action) {
- case MV64XXX_I2C_ACTION_SEND_RESTART:
+ case MV64XXX_I2C_ACTION_NEXT_MESSAGE:
/* We should only get here if we have further messages */
BUG_ON(drv_data->num_msgs == 0);
@@ -481,6 +504,10 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
* Thankfully, do not advertise support for that feature.
*/
drv_data->send_stop = drv_data->num_msgs == 1;
+
+ if (drv_data->msg->flags & I2C_M_NOSTART)
+ return MV64XXX_I2C_EVENT_NOSTART;
+
break;
case MV64XXX_I2C_ACTION_CONTINUE:
@@ -525,6 +552,8 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
wake_up(&drv_data->waitq);
break;
}
+
+ return MV64XXX_I2C_EVENT_INVALID;
}
static void
@@ -595,6 +624,7 @@ static irqreturn_t
mv64xxx_i2c_intr(int irq, void *dev_id)
{
struct mv64xxx_i2c_data *drv_data = dev_id;
+ enum mv64xxx_i2c_event event;
u32 status;
irqreturn_t rc = IRQ_NONE;
@@ -617,8 +647,11 @@ mv64xxx_i2c_intr(int irq, void *dev_id)
ndelay(100);
status = readl(drv_data->reg_base +
drv_data->reg_offsets.status);
- mv64xxx_i2c_fsm(drv_data, status);
- mv64xxx_i2c_do_action(drv_data);
+ event = mv64xxx_i2c_decode_status(drv_data, status);
+ do {
+ mv64xxx_i2c_fsm(drv_data, event, status);
+ event = mv64xxx_i2c_do_action(drv_data);
+ } while (event != MV64XXX_I2C_EVENT_INVALID);
if (drv_data->irq_clear_inverted)
writel(drv_data->cntl_bits |
MV64XXX_I2C_REG_CONTROL_IFLG,
@@ -830,7 +863,54 @@ mv64xxx_i2c_can_offload(struct mv64xxx_i2c_data *drv_data)
static u32
mv64xxx_i2c_functionality(struct i2c_adapter *adap)
{
- return I2C_FUNC_I2C | I2C_FUNC_10BIT_ADDR | I2C_FUNC_SMBUS_EMUL;
+ struct mv64xxx_i2c_data *drv_data = i2c_get_adapdata(adap);
+ u32 func = I2C_FUNC_I2C | I2C_FUNC_10BIT_ADDR | I2C_FUNC_SMBUS_EMUL;
+
+ if (drv_data->reg_offsets.enh_feat != 0)
+ func |= I2C_FUNC_NOSTART;
+
+ return func;
+}
+
+static bool
+mv64xxx_i2c_check_msgs(struct i2c_msg msgs[], int num)
+{
+ int i;
+ bool is_write, prev_is_write;
+
+ /*
+ * The I2C hardware is pretty strict about ensuring that the protocol
+ * is followed properly, and doesn't allow a lot of "creativity" how
+ * transfers are composed. This driver advertises I2C_FUNC_NOSTART, but
+ * can only support the two most common patterns for use of NOSTART:
+ * 1) Continuing a write message with further write bytes, as a sort of
+ * "gather" operation.
+ * 2) Inserting extra parameter bytes after the address of a read
+ * operation, using a zero-byte read, a small (<= 3 bytes) NOSTART
+ * write, and then a NOSTART read.
+ */
+
+ for (i = 0; i < num; i++) {
+ /* Check for case 1 */
+ if (msgs[i].flags & I2C_M_NOSTART) {
+ if (i == 0)
+ return false;
+ is_write = !(msgs[i].flags & I2C_M_RD);
+ prev_is_write = !(msgs[i-1].flags & I2C_M_RD);
+ if (!is_write || !prev_is_write)
+ return false;
+ }
+
+ /* Check for case 2 */
+ if (i + 2 < num) {
+ if ((msgs[i].flags == I2C_M_RD) && (msgs[i].len == 0) &&
+ (msgs[i+1].flags == I2C_M_NOSTART) &&
(msgs[i+1].len <= 3) &&
+ (msgs[i+2].flags == (I2C_M_NOSTART|I2C_M_RD)))
+ i += 2;
+ }
+ }
+
+ return true;
}
static int
@@ -840,6 +920,9 @@ mv64xxx_i2c_xfer_core(struct i2c_adapter *adap,
struct i2c_msg msgs[], int num)
int rc, ret = num;
u32 status;
+ if (!mv64xxx_i2c_check_msgs(msgs, num))
+ return -ENOTSUPP;
+
rc = pm_runtime_resume_and_get(&adap->dev);
if (rc)
return rc;
--
2.43.2
Powered by blists - more mailing lists