| Message ID | 1417294803-14729-2-git-send-email-al.kochet@gmail.com |
|---|---|
| State | RFC |
| Headers | show |
* Alexander Kochetkov <al.kochet@gmail.com> [141129 13:02]: > commit dd74548ddece4b9d68e5528287a272fa552c81d0 ("i2c: omap: > resize fifos before each message") dropped check for dev->buf_len. > As result, data processing loop cause dev->buf overruns for > devices with 16-bit data register (omap2420). > > In the dd74548ddece4b9d68 code, for each loop iteration if the > flag OMAP_I2C_FLAG_16BIT_DATA_REG is set (omap2420), dev->buf > is incremented twice, and dev->buf_len decremented twice. > > Also buffer overrun could happen (in theory) due to wrong > ISR handling (bug). > > The commit fix data processing for omap2420 and add guard checks > in the data processing loops do disallow accesses to the buffer, > when dev->buf_len is zero. Also added warnings to unhide the bug. > > Found by code review. > > Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com> > Fixes: dd74548ddece4b9d68e5528287a272fa552c81d0 "i2c: omap: resize fifos before each message" > Reported-by: Tony Lindgren <tony@atomide.com> I think this is a different issue than what I'm seeing. Not sure if I've seen what you're describing.. The $subject patch should be reviewed by Felipe and Aaro, but this does not help things on 2430. Regards, Tony > drivers/i2c/busses/i2c-omap.c | 36 ++++++++++++++++++++++++++++-------- > 1 file changed, 28 insertions(+), 8 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c > index 4563200..e890295 100644 > --- a/drivers/i2c/busses/i2c-omap.c > +++ b/drivers/i2c/busses/i2c-omap.c > @@ -938,20 +938,30 @@ static int errata_omap3_i462(struct omap_i2c_dev *dev) > static void omap_i2c_receive_data(struct omap_i2c_dev *dev, u8 num_bytes, > bool is_rdr) > { > - u16 w; > + u16 w; > + > + if (unlikely(num_bytes > dev->buf_len)) { > + dev_err(dev->dev, "%s interrupt can't receive %u byte(s)\n", > + is_rdr ? "RDR" : "RRDY", (num_bytes - dev->buf_len)); > + num_bytes = dev->buf_len; > + } > > - while (num_bytes--) { > + while (num_bytes) { > w = omap_i2c_read_reg(dev, OMAP_I2C_DATA_REG); > *dev->buf++ = w; > dev->buf_len--; > + num_bytes--; > > /* > * Data reg in 2430, omap3 and > * omap4 is 8 bit wide > */ > if (dev->flags & OMAP_I2C_FLAG_16BIT_DATA_REG) { > - *dev->buf++ = w >> 8; > - dev->buf_len--; > + if (num_bytes) { > + *dev->buf++ = w >> 8; > + dev->buf_len--; > + num_bytes--; > + } > } > } > } > @@ -959,19 +969,29 @@ static void omap_i2c_receive_data(struct omap_i2c_dev *dev, u8 num_bytes, > static int omap_i2c_transmit_data(struct omap_i2c_dev *dev, u8 num_bytes, > bool is_xdr) > { > - u16 w; > + u16 w; > + > + if (unlikely(num_bytes > dev->buf_len)) { > + dev_err(dev->dev, "%s interrupt can't transmit %u byte(s)\n", > + is_xdr ? "XDR" : "XRDY", (num_bytes - dev->buf_len)); > + num_bytes = dev->buf_len; > + } > > - while (num_bytes--) { > + while (num_bytes) { > w = *dev->buf++; > dev->buf_len--; > + num_bytes--; > > /* > * Data reg in 2430, omap3 and > * omap4 is 8 bit wide > */ > if (dev->flags & OMAP_I2C_FLAG_16BIT_DATA_REG) { > - w |= *dev->buf++ << 8; > - dev->buf_len--; > + if (num_bytes) { > + w |= *dev->buf++ << 8; > + dev->buf_len--; > + num_bytes--; > + } > } > > if (dev->errata & I2C_OMAP_ERRATA_I462) { > -- > 1.7.9.5 > -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
01 дек. 2014 г., в 22:58, Tony Lindgren <tony@atomide.com> написал(а):
> I think this is a different issue than what I'm seeing.
Hello, Tony!
Thank you for testing!
Could check i2c-omap.c from commit ca1f8da9ac5ce6e63d8f6933f83fabc1f3f961f4.
As all my changes comes after it[1]. So I can understand was the problem before my work.
I there was no problems, then try with my first commit:
27caca9d2e01c92b26d0690f065aad093fea01c7
The problems you talk about is this?
[ 9.675994] omap_i2c 48072000.i2c: controller timed out
[ 10.704010] omap_i2c 48072000.i2c: controller timed out
[ 11.734069] omap_i2c 48072000.i2c: controller timed out
root@omap2430sdp:/# [ 12.823638] omap_i2c 48072000.i2c: controller timed out
And how it is possible to switch from ti,omap2430-i2c to ti,omap2420-i2c? They are so different IP, from
the driver point of view. They have different data bus width.
Alexander.
[1]
alexander@ubuntu:busses$ git log --pretty=oneline --reverse ca1f8da9ac5ce6e63d8f6933f83fabc1f3f961f4^..HEAD -- i2c-omap.c
ca1f8da9ac5ce6e63d8f6933f83fabc1f3f961f4 i2c: remove FSF address
27caca9d2e01c92b26d0690f065aad093fea01c7 i2c: omap: fix NACK and Arbitration Lost irq handling
854a59425a0b9600ee974b113aae081c873163f6 i2c: omap: cleanup register definitions
903c3859f77f9b0aace551da03267ef7a211dbc4 i2c: omap: implement workaround for handling invalid BB-bit values
80cc361f14e8fa97119afa3324c2c913915e7252 i2c: omap: don't reset controller if Arbitration Lost detected
39370ab406933efdedb425910f0a36c16667c45f i2c: omap: add notes related to i2c multimaster mode
ccfc866356674cb3a61829d239c685af6e85f197 i2c: omap: fix i207 errata handling
7d168dc7ed384e50bb7bff4920b73550fd2e9fcb Merge branch 'i2c/for-3.19' into i2c/for-next
2f769d173f0e6a2e85d75fe396f18f794fc4a615 omap: i2c: don't check bus state IP rev3.3 and earlier
2b6f66d87b44aaf1f34f071e6f6430c3ccaa8812 i2c: omap: fix buffer overruns during RX/TX data processing
30c52545106785405856c7e7e40b683b79c8084a i2c: omap: show that the reason of system lockup is an unhandled ISR event
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
* Alexander Kochetkov <al.kochet@gmail.com> [141202 03:19]: > > 01 дек. 2014 г., в 22:58, Tony Lindgren <tony@atomide.com> написал(а): > > > I think this is a different issue than what I'm seeing. > > Hello, Tony! > > Thank you for testing! > > Could check i2c-omap.c from commit ca1f8da9ac5ce6e63d8f6933f83fabc1f3f961f4. > As all my changes comes after it[1]. So I can understand was the problem before my work. The issue I'm seeing on 2430sdp is some earlier regression that has started happening over the past year or so. No idea when though as it sometimes works and sometimes does not. So it does not have anything to do with your patches. > I there was no problems, then try with my first commit: > 27caca9d2e01c92b26d0690f065aad093fea01c7 > > The problems you talk about is this? > [ 9.675994] omap_i2c 48072000.i2c: controller timed out > [ 10.704010] omap_i2c 48072000.i2c: controller timed out > [ 11.734069] omap_i2c 48072000.i2c: controller timed out > root@omap2430sdp:/# [ 12.823638] omap_i2c 48072000.i2c: controller timed out No, I'm seeing just this: omap_i2c 48070000.i2c: bus 0 rev3.3 at 100 kHz omap_i2c 48072000.i2c: bus 1 rev3.3 at 100 kHz twl 1-0048: PIH (irq 216) chaining IRQs 217..225 twl 1-0048: power (irq 222) chaining IRQs 225..232 And the system just hangs. With i2c-omap debug enabled, the lines around the twl are: omap_i2c 48072000.i2c: addr: 0x004b, len: 1, flags: 0x0, stop: 0 omap_i2c 48072000.i2c: IRQ (ISR = 0x0010) omap_i2c 48072000.i2c: IRQ (ISR = 0x0004) omap_i2c 48072000.i2c: addr: 0x004b, len: 1, flags: 0x1, stop: 1 omap_i2c 48072000.i2c: IRQ (ISR = 0x0008) omap_i2c 48072000.i2c: IRQ (ISR = 0x0004) twl 1-0048: PIH (irq 216) chaining IRQs 217..225 twl 1-0048: power (irq 222) chaining IRQs 225..232 omap_i2c 48072000.i2c: addr: 0x0049, len: 1, flags: 0x0, stop: 0 omap_i2c 48072000.i2c: IRQ (ISR = 0x0010) omap_i2c 48072000.i2c: IRQ (ISR = 0x0004) omap_i2c 48072000.i2c: addr: 0x0049, len: 1, flags: 0x1, stop: 1 omap_i2c 48072000.i2c: IRQ (ISR = 0x0008) omap_i2c 48072000.i2c: IRQ (ISR = 0x0004) omap_i2c 48072000.i2c: addr: 0x0049, len: 2, flags: 0x0, stop: 1 omap_i2c 48072000.i2c: IRQ (ISR = 0x0010) omap_i2c 48072000.i2c: IRQ (ISR = 0x0004) omap_i2c 48072000.i2c: addr: 0x004b, len: 1, flags: 0x0, stop: 0 omap_i2c 48072000.i2c: IRQ (ISR = 0x0010) omap_i2c 48072000.i2c: IRQ (ISR = 0x0004) omap_i2c 48072000.i2c: addr: 0x004b, len: 1, flags: 0x1, stop: 1 omap_i2c 48072000.i2c: IRQ (ISR = 0x0008) omap_i2c 48072000.i2c: IRQ (ISR = 0x0004) omap_i2c 48072000.i2c: addr: 0x0049, len: 1, flags: 0x0, stop: 0 omap_i2c 48072000.i2c: IRQ (ISR = 0x0010) omap_i2c 48072000.i2c: IRQ (ISR = 0x0004) omap_i2c 48072000.i2c: addr: 0x0049, len: 1, flags: 0x1, stop: 1 omap_i2c 48072000.i2c: IRQ (ISR = 0x0008) omap_i2c 48072000.i2c: IRQ (ISR = 0x0004) omap_i2c 48072000.i2c: addr: 0x004b, len: 2, flags: 0x0, stop: 1 omap_i2c 48072000.i2c: IRQ (ISR = 0x0010) And then it hangs. In the working case, the output continues with: omap_i2c 48072000.i2c: addr: 0x004b, len: 1, flags: 0x0, stop: 0 omap_i2c 48072000.i2c: IRQ (ISR = 0x0010) omap_i2c 48072000.i2c: IRQ (ISR = 0x0004) omap_i2c 48072000.i2c: addr: 0x004b, len: 1, flags: 0x1, stop: 1 omap_i2c 48072000.i2c: IRQ (ISR = 0x0008) omap_i2c 48072000.i2c: IRQ (ISR = 0x0004) twl 1-0048: PIH (irq 216) chaining IRQs 217..225 twl 1-0048: power (irq 222) chaining IRQs 225..232 omap_i2c 48072000.i2c: addr: 0x0049, len: 1, flags: 0x0, stop: 0 omap_i2c 48072000.i2c: IRQ (ISR = 0x0010) omap_i2c 48072000.i2c: IRQ (ISR = 0x0004) omap_i2c 48072000.i2c: addr: 0x0049, len: 1, flags: 0x1, stop: 1 omap_i2c 48072000.i2c: IRQ (ISR = 0x0008) omap_i2c 48072000.i2c: IRQ (ISR = 0x0004) omap_i2c 48072000.i2c: addr: 0x0049, len: 2, flags: 0x0, stop: 1 omap_i2c 48072000.i2c: IRQ (ISR = 0x0010) omap_i2c 48072000.i2c: IRQ (ISR = 0x0004) omap_i2c 48072000.i2c: addr: 0x004b, len: 1, flags: 0x0, stop: 0 omap_i2c 48072000.i2c: IRQ (ISR = 0x0010) omap_i2c 48072000.i2c: IRQ (ISR = 0x0004) omap_i2c 48072000.i2c: addr: 0x004b, len: 1, flags: 0x1, stop: 1 omap_i2c 48072000.i2c: IRQ (ISR = 0x0008) omap_i2c 48072000.i2c: IRQ (ISR = 0x0004) omap_i2c 48072000.i2c: addr: 0x004b, len: 2, flags: 0x0, stop: 1 omap_i2c 48072000.i2c: IRQ (ISR = 0x0010) omap_i2c 48072000.i2c: IRQ (ISR = 0x0004) omap_i2c 48072000.i2c: addr: 0x004b, len: 1, flags: 0x0, stop: 0 omap_i2c 48072000.i2c: IRQ (ISR = 0x0010) omap_i2c 48072000.i2c: IRQ (ISR = 0x0004) omap_i2c 48072000.i2c: addr: 0x004b, len: 1, flags: 0x1, stop: 1 omap_i2c 48072000.i2c: IRQ (ISR = 0x0008) omap_i2c 48072000.i2c: IRQ (ISR = 0x0004) omap_i2c 48072000.i2c: addr: 0x004b, len: 1, flags: 0x0, stop: 0 omap_i2c 48072000.i2c: IRQ (ISR = 0x0010) omap_i2c 48072000.i2c: IRQ (ISR = 0x0004) omap_i2c 48072000.i2c: addr: 0x004b, len: 1, flags: 0x1, stop: 1 omap_i2c 48072000.i2c: IRQ (ISR = 0x0008) omap_i2c 48072000.i2c: IRQ (ISR = 0x0004) omap_i2c 48072000.i2c: addr: 0x004b, len: 2, flags: 0x0, stop: 1 omap_i2c 48072000.i2c: IRQ (ISR = 0x0010) omap_i2c 48072000.i2c: IRQ (ISR = 0x0004) omap_i2c 48072000.i2c: addr: 0x004b, len: 1, flags: 0x0, stop: 0 omap_i2c 48072000.i2c: IRQ (ISR = 0x0010) omap_i2c 48072000.i2c: IRQ (ISR = 0x0004) omap_i2c 48072000.i2c: addr: 0x004b, len: 1, flags: 0x1, stop: 1 omap_i2c 48072000.i2c: IRQ (ISR = 0x0008) omap_i2c 48072000.i2c: IRQ (ISR = 0x0004) ... > And how it is possible to switch from ti,omap2430-i2c to ti,omap2420-i2c? > They are so different IP, from the driver point of view. They have different > data bus width. Yes sorry, you're right. Downgrading it to ti,omap2420-i2c just seems to quiet down the errors as the I2C then does not work at all :) Of course the issue I'm seeing could be caused by hung twl4030 PMIC too. Regards, Tony > Alexander. > > [1] > alexander@ubuntu:busses$ git log --pretty=oneline --reverse ca1f8da9ac5ce6e63d8f6933f83fabc1f3f961f4^..HEAD -- i2c-omap.c > ca1f8da9ac5ce6e63d8f6933f83fabc1f3f961f4 i2c: remove FSF address > 27caca9d2e01c92b26d0690f065aad093fea01c7 i2c: omap: fix NACK and Arbitration Lost irq handling > 854a59425a0b9600ee974b113aae081c873163f6 i2c: omap: cleanup register definitions > 903c3859f77f9b0aace551da03267ef7a211dbc4 i2c: omap: implement workaround for handling invalid BB-bit values > 80cc361f14e8fa97119afa3324c2c913915e7252 i2c: omap: don't reset controller if Arbitration Lost detected > 39370ab406933efdedb425910f0a36c16667c45f i2c: omap: add notes related to i2c multimaster mode > ccfc866356674cb3a61829d239c685af6e85f197 i2c: omap: fix i207 errata handling > 7d168dc7ed384e50bb7bff4920b73550fd2e9fcb Merge branch 'i2c/for-3.19' into i2c/for-next > 2f769d173f0e6a2e85d75fe396f18f794fc4a615 omap: i2c: don't check bus state IP rev3.3 and earlier > 2b6f66d87b44aaf1f34f071e6f6430c3ccaa8812 i2c: omap: fix buffer overruns during RX/TX data processing > 30c52545106785405856c7e7e40b683b79c8084a i2c: omap: show that the reason of system lockup is an unhandled ISR event > > -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, Tony! > The issue I'm seeing on 2430sdp is some earlier regression that > has started happening over the past year or so. I don't have 2430sdp. And I couldn't find 2430sdp schematics. Looks, like this is NDA stuff. > Of course the issue I'm seeing could be caused by hung twl4030 > PMIC too. This could be switching twl4030 into inappropriate for 2430sdp mode. For example, SMARTREFLEX_ENABLE bit is dangerous, because it switch twl-pads from VMODE to i2c. And when you set SMARTREFLEX_ENABLE, twl automatically drop VDD1 and VDD2 to 1.2V (if VDD1_SR_CONTROL was not initialized through BYPASS register early in the boot loader). What mode is used in the 2430sdp? VMODE or Smartreflex? And, that is my assumption, may be it wrong again. I'd added trace in the twl_i2c_read_u8/twl_i2c_write_u8 functions to easy find concrete line of code, what cause problem (by register address). Hang happens on access to 0x004b address group. That could be: BACKUP_REG, INT, PM_MASTER, PM_RECEIVER, RTC or SECURED_REG. It hard to tell which one cause hang without trace. Hope this helps. Regards, Alexander. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c index 4563200..e890295 100644 --- a/drivers/i2c/busses/i2c-omap.c +++ b/drivers/i2c/busses/i2c-omap.c @@ -938,20 +938,30 @@ static int errata_omap3_i462(struct omap_i2c_dev *dev) static void omap_i2c_receive_data(struct omap_i2c_dev *dev, u8 num_bytes, bool is_rdr) { - u16 w; + u16 w; + + if (unlikely(num_bytes > dev->buf_len)) { + dev_err(dev->dev, "%s interrupt can't receive %u byte(s)\n", + is_rdr ? "RDR" : "RRDY", (num_bytes - dev->buf_len)); + num_bytes = dev->buf_len; + } - while (num_bytes--) { + while (num_bytes) { w = omap_i2c_read_reg(dev, OMAP_I2C_DATA_REG); *dev->buf++ = w; dev->buf_len--; + num_bytes--; /* * Data reg in 2430, omap3 and * omap4 is 8 bit wide */ if (dev->flags & OMAP_I2C_FLAG_16BIT_DATA_REG) { - *dev->buf++ = w >> 8; - dev->buf_len--; + if (num_bytes) { + *dev->buf++ = w >> 8; + dev->buf_len--; + num_bytes--; + } } } } @@ -959,19 +969,29 @@ static void omap_i2c_receive_data(struct omap_i2c_dev *dev, u8 num_bytes, static int omap_i2c_transmit_data(struct omap_i2c_dev *dev, u8 num_bytes, bool is_xdr) { - u16 w; + u16 w; + + if (unlikely(num_bytes > dev->buf_len)) { + dev_err(dev->dev, "%s interrupt can't transmit %u byte(s)\n", + is_xdr ? "XDR" : "XRDY", (num_bytes - dev->buf_len)); + num_bytes = dev->buf_len; + } - while (num_bytes--) { + while (num_bytes) { w = *dev->buf++; dev->buf_len--; + num_bytes--; /* * Data reg in 2430, omap3 and * omap4 is 8 bit wide */ if (dev->flags & OMAP_I2C_FLAG_16BIT_DATA_REG) { - w |= *dev->buf++ << 8; - dev->buf_len--; + if (num_bytes) { + w |= *dev->buf++ << 8; + dev->buf_len--; + num_bytes--; + } } if (dev->errata & I2C_OMAP_ERRATA_I462) {
commit dd74548ddece4b9d68e5528287a272fa552c81d0 ("i2c: omap: resize fifos before each message") dropped check for dev->buf_len. As result, data processing loop cause dev->buf overruns for devices with 16-bit data register (omap2420). In the dd74548ddece4b9d68 code, for each loop iteration if the flag OMAP_I2C_FLAG_16BIT_DATA_REG is set (omap2420), dev->buf is incremented twice, and dev->buf_len decremented twice. Also buffer overrun could happen (in theory) due to wrong ISR handling (bug). The commit fix data processing for omap2420 and add guard checks in the data processing loops do disallow accesses to the buffer, when dev->buf_len is zero. Also added warnings to unhide the bug. Found by code review. Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com> Fixes: dd74548ddece4b9d68e5528287a272fa552c81d0 "i2c: omap: resize fifos before each message" Reported-by: Tony Lindgren <tony@atomide.com> --- drivers/i2c/busses/i2c-omap.c | 36 ++++++++++++++++++++++++++++-------- 1 file changed, 28 insertions(+), 8 deletions(-)