From cc3748ad469b85bb3969f67c347a296ee98dae1a Mon Sep 17 00:00:00 2001 From: Ben Hutchings Date: Tue, 13 Aug 2013 18:36:33 +0000 Subject: [PATCH] [armel/orion5x] I2C: mv64xxx: fix race between FSM/interrupt and process context (Closes: #622325) svn path=/dists/sid/linux/; revision=20495 --- debian/changelog | 2 + ...2C-mv64xxx-remove-I2C_M_NOSTART-code.patch | 57 +++++++ ...race-between-FSM-interrupt-and-proce.patch | 141 ++++++++++++++++++ ...4xxx-move-mv64xxx_i2c_prepare_for_io.patch | 86 +++++++++++ debian/patches/series | 4 + 5 files changed, 290 insertions(+) create mode 100644 debian/patches/bugfix/arm/I2C-I2C-mv64xxx-remove-I2C_M_NOSTART-code.patch create mode 100644 debian/patches/bugfix/arm/I2C-mv64xxx-fix-race-between-FSM-interrupt-and-proce.patch create mode 100644 debian/patches/bugfix/arm/I2C-mv64xxx-move-mv64xxx_i2c_prepare_for_io.patch diff --git a/debian/changelog b/debian/changelog index 50da6c1c8..06628307d 100644 --- a/debian/changelog +++ b/debian/changelog @@ -2,6 +2,8 @@ linux (3.10.5-2) UNRELEASED; urgency=low [ Ben Hutchings ] * [x86] Enable ASUS_OLED as module (Closes: #680016) + * [armel/orion5x] I2C: mv64xxx: fix race between FSM/interrupt and process + context (Closes: #622325) [ Thorsten Glaser ] * [m68k] debian/patches/bugfix/m68k/atari-irqs.patch: patch from mailing list diff --git a/debian/patches/bugfix/arm/I2C-I2C-mv64xxx-remove-I2C_M_NOSTART-code.patch b/debian/patches/bugfix/arm/I2C-I2C-mv64xxx-remove-I2C_M_NOSTART-code.patch new file mode 100644 index 000000000..347d53a46 --- /dev/null +++ b/debian/patches/bugfix/arm/I2C-I2C-mv64xxx-remove-I2C_M_NOSTART-code.patch @@ -0,0 +1,57 @@ +From: Russell King +Date: Thu, 16 May 2013 21:37:11 +0100 +Subject: [1/3] I2C: mv64xxx: remove I2C_M_NOSTART code +Origin: https://git.kernel.org/linus/aa6bce5319a54c050af26e095287472854abccfd +Bug-Debian: http://bugs.debian.org/622325 + +As this driver does not advertise protocol mangling support +(I2C_FUNC_PROTOCOL_MANGLING is not set), having code to act on +I2C_M_NOSTART is illogical, and in any case isn't supportable on +anything but the first message - which makes no sense. Remove +the I2C_M_NOSTART code. + +Signed-off-by: Russell King +Acked-by: Mark A. Greer +Signed-off-by: Wolfram Sang +--- + drivers/i2c/busses/i2c-mv64xxx.c | 26 +++++--------------------- + 1 file changed, 5 insertions(+), 21 deletions(-) + +diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c +index 4b45c73..f11cb25 100644 +--- a/drivers/i2c/busses/i2c-mv64xxx.c ++++ b/drivers/i2c/busses/i2c-mv64xxx.c +@@ -419,28 +419,12 @@ mv64xxx_i2c_execute_msg(struct mv64xxx_i2c_data *drv_data, struct i2c_msg *msg, + spin_lock_irqsave(&drv_data->lock, flags); + mv64xxx_i2c_prepare_for_io(drv_data, msg); + +- if (unlikely(msg->flags & I2C_M_NOSTART)) { /* Skip start/addr phases */ +- if (drv_data->msg->flags & I2C_M_RD) { +- /* No action to do, wait for slave to send a byte */ +- drv_data->action = MV64XXX_I2C_ACTION_CONTINUE; +- drv_data->state = +- MV64XXX_I2C_STATE_WAITING_FOR_SLAVE_DATA; +- } else { +- drv_data->action = MV64XXX_I2C_ACTION_SEND_DATA; +- drv_data->state = +- MV64XXX_I2C_STATE_WAITING_FOR_SLAVE_ACK; +- drv_data->bytes_left--; +- } ++ if (is_first) { ++ drv_data->action = MV64XXX_I2C_ACTION_SEND_START; ++ drv_data->state = MV64XXX_I2C_STATE_WAITING_FOR_START_COND; + } else { +- if (is_first) { +- drv_data->action = MV64XXX_I2C_ACTION_SEND_START; +- drv_data->state = +- MV64XXX_I2C_STATE_WAITING_FOR_START_COND; +- } else { +- drv_data->action = MV64XXX_I2C_ACTION_SEND_ADDR_1; +- drv_data->state = +- MV64XXX_I2C_STATE_WAITING_FOR_ADDR_1_ACK; +- } ++ drv_data->action = MV64XXX_I2C_ACTION_SEND_ADDR_1; ++ drv_data->state = MV64XXX_I2C_STATE_WAITING_FOR_ADDR_1_ACK; + } + + drv_data->send_stop = is_last; diff --git a/debian/patches/bugfix/arm/I2C-mv64xxx-fix-race-between-FSM-interrupt-and-proce.patch b/debian/patches/bugfix/arm/I2C-mv64xxx-fix-race-between-FSM-interrupt-and-proce.patch new file mode 100644 index 000000000..1268931da --- /dev/null +++ b/debian/patches/bugfix/arm/I2C-mv64xxx-fix-race-between-FSM-interrupt-and-proce.patch @@ -0,0 +1,141 @@ +From: Russell King +Date: Thu, 16 May 2013 21:39:12 +0100 +Subject: [3/3] I2C: mv64xxx: fix race between FSM/interrupt and process + context +Origin: https://git.kernel.org/linus/4243fa0bad551b8c8d4ff7104e8fd557ae848845 +Bug-Debian: http://bugs.debian.org/622325 + +Asking for a multi-part message to be handled by this driver is racy; it +has been observed that the following sequence is possible with this +driver: + + - send start + - send address + write + - send data + - send (repeated) start + - send address + write + - send (repeated) start + - send address + read + - unrecoverable bus hang (except by system reset) + +The problem is that the interrupt handling sees the next event after the +first repeated start is sent - the IFLG bit is set in the register even +though INTEN is disabled. + +Let's fix this by moving all of the message processing into interrupt +context, rather than having it partly in IRQ and partly in process +context. This allows us to move immediately to the next message in the +interrupt handler and get on with the transfer, rather than incuring a +couple of scheduling switches to get the next message. + +Signed-off-by: Russell King +Acked-by: Mark A. Greer +Signed-off-by: Wolfram Sang +--- + drivers/i2c/busses/i2c-mv64xxx.c | 54 +++++++++++++++++++++++++--------------- + 1 file changed, 34 insertions(+), 20 deletions(-) + +diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c +index bb37e14..6356439 100644 +--- a/drivers/i2c/busses/i2c-mv64xxx.c ++++ b/drivers/i2c/busses/i2c-mv64xxx.c +@@ -86,6 +86,8 @@ enum { + }; + + struct mv64xxx_i2c_data { ++ struct i2c_msg *msgs; ++ int num_msgs; + int irq; + u32 state; + u32 action; +@@ -194,7 +196,7 @@ mv64xxx_i2c_fsm(struct mv64xxx_i2c_data *drv_data, u32 status) + if ((drv_data->bytes_left == 0) + || (drv_data->aborting + && (drv_data->byte_posn != 0))) { +- if (drv_data->send_stop) { ++ if (drv_data->send_stop || drv_data->aborting) { + drv_data->action = MV64XXX_I2C_ACTION_SEND_STOP; + drv_data->state = MV64XXX_I2C_STATE_IDLE; + } else { +@@ -271,12 +273,25 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data) + { + switch(drv_data->action) { + case MV64XXX_I2C_ACTION_SEND_RESTART: ++ /* We should only get here if we have further messages */ ++ BUG_ON(drv_data->num_msgs == 0); ++ + drv_data->cntl_bits |= MV64XXX_I2C_REG_CONTROL_START; +- drv_data->cntl_bits &= ~MV64XXX_I2C_REG_CONTROL_INTEN; + writel(drv_data->cntl_bits, + drv_data->reg_base + MV64XXX_I2C_REG_CONTROL); +- drv_data->block = 0; +- wake_up(&drv_data->waitq); ++ ++ drv_data->msgs++; ++ drv_data->num_msgs--; ++ ++ /* Setup for the next message */ ++ mv64xxx_i2c_prepare_for_io(drv_data, drv_data->msgs); ++ ++ /* ++ * We're never at the start of the message here, and by this ++ * time it's already too late to do any protocol mangling. ++ * Thankfully, do not advertise support for that feature. ++ */ ++ drv_data->send_stop = drv_data->num_msgs == 1; + break; + + case MV64XXX_I2C_ACTION_CONTINUE: +@@ -412,20 +427,15 @@ mv64xxx_i2c_wait_for_completion(struct mv64xxx_i2c_data *drv_data) + + static int + mv64xxx_i2c_execute_msg(struct mv64xxx_i2c_data *drv_data, struct i2c_msg *msg, +- int is_first, int is_last) ++ int is_last) + { + unsigned long flags; + + spin_lock_irqsave(&drv_data->lock, flags); + mv64xxx_i2c_prepare_for_io(drv_data, msg); + +- if (is_first) { +- drv_data->action = MV64XXX_I2C_ACTION_SEND_START; +- drv_data->state = MV64XXX_I2C_STATE_WAITING_FOR_START_COND; +- } else { +- drv_data->action = MV64XXX_I2C_ACTION_SEND_ADDR_1; +- drv_data->state = MV64XXX_I2C_STATE_WAITING_FOR_ADDR_1_ACK; +- } ++ drv_data->action = MV64XXX_I2C_ACTION_SEND_START; ++ drv_data->state = MV64XXX_I2C_STATE_WAITING_FOR_START_COND; + + drv_data->send_stop = is_last; + drv_data->block = 1; +@@ -453,16 +463,20 @@ static int + mv64xxx_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) + { + struct mv64xxx_i2c_data *drv_data = i2c_get_adapdata(adap); +- int i, rc; ++ int rc, ret = num; + +- for (i = 0; i < num; i++) { +- rc = mv64xxx_i2c_execute_msg(drv_data, &msgs[i], +- i == 0, i + 1 == num); +- if (rc < 0) +- return rc; +- } ++ BUG_ON(drv_data->msgs != NULL); ++ drv_data->msgs = msgs; ++ drv_data->num_msgs = num; ++ ++ rc = mv64xxx_i2c_execute_msg(drv_data, &msgs[0], num == 1); ++ if (rc < 0) ++ ret = rc; ++ ++ drv_data->num_msgs = 0; ++ drv_data->msgs = NULL; + +- return num; ++ return ret; + } + + static const struct i2c_algorithm mv64xxx_i2c_algo = { diff --git a/debian/patches/bugfix/arm/I2C-mv64xxx-move-mv64xxx_i2c_prepare_for_io.patch b/debian/patches/bugfix/arm/I2C-mv64xxx-move-mv64xxx_i2c_prepare_for_io.patch new file mode 100644 index 000000000..27d487483 --- /dev/null +++ b/debian/patches/bugfix/arm/I2C-mv64xxx-move-mv64xxx_i2c_prepare_for_io.patch @@ -0,0 +1,86 @@ +From: Russell King +Date: Thu, 16 May 2013 21:38:11 +0100 +Subject: [2/3] I2C: mv64xxx: move mv64xxx_i2c_prepare_for_io() +Origin: https://git.kernel.org/linus/3420afbc06058c9c13f7d69cf48b9d5429db6bd9 +Bug-Debian: http://bugs.debian.org/622325 + +Move mv64xxx_i2c_prepare_for_io() higher up in the driver to avoid a +future forward declaration for this function. + +Signed-off-by: Russell King +Acked-by: Mark A. Greer +Signed-off-by: Wolfram Sang +--- + drivers/i2c/busses/i2c-mv64xxx.c | 52 ++++++++++++++++++++-------------------- + 1 file changed, 26 insertions(+), 26 deletions(-) + +diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c +index f11cb25..bb37e14 100644 +--- a/drivers/i2c/busses/i2c-mv64xxx.c ++++ b/drivers/i2c/busses/i2c-mv64xxx.c +@@ -110,6 +110,32 @@ struct mv64xxx_i2c_data { + struct i2c_adapter adapter; + }; + ++static void ++mv64xxx_i2c_prepare_for_io(struct mv64xxx_i2c_data *drv_data, ++ struct i2c_msg *msg) ++{ ++ u32 dir = 0; ++ ++ drv_data->msg = msg; ++ drv_data->byte_posn = 0; ++ drv_data->bytes_left = msg->len; ++ drv_data->aborting = 0; ++ drv_data->rc = 0; ++ drv_data->cntl_bits = MV64XXX_I2C_REG_CONTROL_ACK | ++ MV64XXX_I2C_REG_CONTROL_INTEN | MV64XXX_I2C_REG_CONTROL_TWSIEN; ++ ++ if (msg->flags & I2C_M_RD) ++ dir = 1; ++ ++ if (msg->flags & I2C_M_TEN) { ++ drv_data->addr1 = 0xf0 | (((u32)msg->addr & 0x300) >> 7) | dir; ++ drv_data->addr2 = (u32)msg->addr & 0xff; ++ } else { ++ drv_data->addr1 = ((u32)msg->addr & 0x7f) << 1 | dir; ++ drv_data->addr2 = 0; ++ } ++} ++ + /* + ***************************************************************************** + * +@@ -347,32 +373,6 @@ mv64xxx_i2c_intr(int irq, void *dev_id) + ***************************************************************************** + */ + static void +-mv64xxx_i2c_prepare_for_io(struct mv64xxx_i2c_data *drv_data, +- struct i2c_msg *msg) +-{ +- u32 dir = 0; +- +- drv_data->msg = msg; +- drv_data->byte_posn = 0; +- drv_data->bytes_left = msg->len; +- drv_data->aborting = 0; +- drv_data->rc = 0; +- drv_data->cntl_bits = MV64XXX_I2C_REG_CONTROL_ACK | +- MV64XXX_I2C_REG_CONTROL_INTEN | MV64XXX_I2C_REG_CONTROL_TWSIEN; +- +- if (msg->flags & I2C_M_RD) +- dir = 1; +- +- if (msg->flags & I2C_M_TEN) { +- drv_data->addr1 = 0xf0 | (((u32)msg->addr & 0x300) >> 7) | dir; +- drv_data->addr2 = (u32)msg->addr & 0xff; +- } else { +- drv_data->addr1 = ((u32)msg->addr & 0x7f) << 1 | dir; +- drv_data->addr2 = 0; +- } +-} +- +-static void + mv64xxx_i2c_wait_for_completion(struct mv64xxx_i2c_data *drv_data) + { + long time_left; diff --git a/debian/patches/series b/debian/patches/series index 4d4d4149a..10175b6e4 100644 --- a/debian/patches/series +++ b/debian/patches/series @@ -120,3 +120,7 @@ bugfix/m68k/atari-irqs.patch # m68k workaround for div64 called with wrong type args bugfix/m68k/type-fix-div64.patch + +bugfix/arm/I2C-I2C-mv64xxx-remove-I2C_M_NOSTART-code.patch +bugfix/arm/I2C-mv64xxx-move-mv64xxx_i2c_prepare_for_io.patch +bugfix/arm/I2C-mv64xxx-fix-race-between-FSM-interrupt-and-proce.patch