Hi Felipe, Initially you made the change (66b9298878742f08cb6e79b7c7d5632d782fd1e1): https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/i2c/busses/i2c-omap.c?id=66b9298878742f08cb6e79b7c7d5632d782fd1e1 dev_dbg(dev->dev, "IRQ (ISR = 0x%04x)\n", stat); if (count++ == 100) { dev_warn(dev->dev, "Too much work in one IRQ\n"); - break; + omap_i2c_complete_cmd(dev, err); + return IRQ_HANDLED; } Than that change transformed into (4a7ec4eda58269a507501f240955d99312fdfd5f): https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/i2c/busses/i2c-omap.c?id=4a7ec4eda58269a507501f240955d99312fdfd5f @@ -853,24 +853,21 @@ omap_i2c_isr(int this_irq, void *dev_id) dev_dbg(dev->dev, "IRQ (ISR = 0x%04x)\n", stat); if (count++ == 100) { dev_warn(dev->dev, "Too much work in one IRQ\n"); - omap_i2c_complete_cmd(dev, err); - return IRQ_HANDLED; + goto out; } +out: + omap_i2c_complete_cmd(dev, err); return IRQ_HANDLED; } As a result we have in master: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/i2c/busses/i2c-omap.c do { bits = omap_i2c_read_reg(dev, OMAP_I2C_IE_REG); stat = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG); stat &= bits; ... dev_dbg(dev->dev, "IRQ (ISR = 0x%04x)\n", stat); if (count++ == 100) { dev_warn(dev->dev, "Too much work in one IRQ\n"); break; } ... } while (stat); omap_i2c_complete_cmd(dev, err); out: spin_unlock_irqrestore(&dev->lock, flags); While original code was: { bits = omap_i2c_read_reg(dev, OMAP_I2C_IE_REG); while ((stat = (omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG))) & bits) { dev_dbg(dev->dev, "IRQ (ISR = 0x%04x)\n", stat); if (count++ == 100) { dev_warn(dev->dev, "Too much work in one IRQ\n"); break; } err = 0; complete: ... if (stat & (OMAP_I2C_STAT_ARDY | OMAP_I2C_STAT_NACK | OMAP_I2C_STAT_AL)) { ... omap_i2c_complete_cmd(dev, err); return IRQ_HANDLED; } ... } return count ? IRQ_HANDLED : IRQ_NONE; } > how ? This is an interesting bug which deserves further explanation. Look at the loops above, and at the omap_i2c_complete_cmd: static inline void omap_i2c_complete_cmd(struct omap_i2c_dev *dev, u16 err) { dev->cmd_err |= err; complete(&dev->cmd_complete); } You can see, loop will be aborted if counter reached 100. Final state of transfer depends on values stored in the 'err' and 'dev->cmd_err'. If 'err' and 'dev->cmd_err' are zero, than transfer would be aborted with status 0. > quite frankly, this looks *very* wrong. It creates the possibility for > us never completing a command which would cause several timeouts. Pre 66b9298878742f08cb6e79b7c7d5632d782fd1e1 omap-i2c.c version didn't cause deadlocks. > > How have you tested this and how have you figured this was the actual > bug ? Based on commit log not matching the patch body (which 'original > logic' are you talking about ?), I have to NAK this patch. Well, I tried to debug I2C ISR hang issue (thats another question I want to discuss later) using output to console. I places few dev_warn (not dev_dbg) messages into ISR (like got NACK, got ARDY and so on) and got "Too much work in one IRQ" with incorrect booting. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
