Summary: | iic(4): Panics on Nanopi Neo when using 16-bit device addressing width: panic: Assertion strlen(description) < MAX_W_NAME failed at ... sys/kern/subr_witness.c:1914 | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | Base System | Reporter: | alydiomc | ||||||
Component: | kern | Assignee: | Andriy Gapon <avg> | ||||||
Status: | Closed FIXED | ||||||||
Severity: | Affects Some People | CC: | alydiomc, avg, freebsd-arm, manu, wulf | ||||||
Priority: | --- | Keywords: | crash, needs-qa | ||||||
Version: | 12.2-RELEASE | Flags: | koobs:
mfc-stable13?
koobs: mfc-stable12? |
||||||
Hardware: | arm | ||||||||
OS: | Any | ||||||||
Attachments: |
|
Description
alydiomc
2021-10-07 23:11:02 UTC
^Triage: Set Version to earliest confirmed (12.2-R) 4 byte constant string "iic" did not pass length check for 64 byte buffer. That is strange. You definitely have some sort of memory corruption. Through local source modification, out of buffer write or hardware failure. Created attachment 228624 [details]
twsi.c driver
Comment on attachment 228624 [details]
twsi.c driver
Seems the driver does not handle 16bit wide addressing properly.
I made some modification. Looks to be working.
root@nanopi-neo:~/prog/I2C # ./i2c -s -f /dev/iic0
Scanning I2C devices on /dev/iic0: 57 68
root@nanopi-neo:~/prog/I2C # ./i2c -a 0x57 -f /dev/iic0 -d r -o 0 -w 16 -c 16 -m tr
00 01 02 03 04 05 06 07 08 09 0a 0b 0c 0d 0e 0f
root@nanopi-neo:~/prog/I2C # echo -n "test" | ./i2c -a 0x57 -f /dev/iic0 -d w -o 0x6 -w 16 -c 4 -m tr
root@nanopi-neo:~/prog/I2C # ./i2c -a 0x57 -f /dev/iic0 -d r -o 0 -w 16 -c 16 -m tr
00 01 02 03 04 05 74 65 73 74 0a 0b 0c 0d 0e 0f
root@nanopi-neo:~/prog/I2C # date;./ds1307 -s;./ds1307 -r
Tue Oct 12 19:45:35 PST 2021
19:45:35 12/10/2021
root@nanopi-neo:~/prog/I2C # ./i2c -a 0x68 -f /dev/iic0 -d r -o 0 -w 8 -c 1 -m tr
15
root@nanopi-neo:~/prog/I2C # ./i2c -a 0x68 -f /dev/iic0 -d r -o 0 -w 8 -c 1 -m tr
16
on i2c(8), i also adjusted the parsing of offset. Seems LSB is being sent first. I need MSB first. off.off16 = (((uint16_t)i2c_opt.off << 8) & 0xff00) | (((uint16_t)i2c_opt.off >> 8) & 0x00ff); Could upload it as a diff against base version rather than entire content (In reply to Vladimir Kondratyev from comment #6) Yeah! Sorry, this is my first. :) Will do next time. Thank you! Created attachment 228827 [details]
Added some improvements.
root@nanopi-neo:~/i2c # i2c -s /dev/iic0
Scanning I2C devices on /dev/iic0: 40 57 68
[8bit single write]
root@nanopi-neo:~/i2c # i2c -a 40 -f /dev/iic0 -d w -o 0 -w 8 -c 1 -m tr < 00.hex
root@nanopi-neo:~/i2c # i2c -a 40 -f /dev/iic0 -d r -o 0 -w 8 -c 1 -m tr
00
[8bit single write]
root@nanopi-neo:~/i2c # i2c -a 40 -f /dev/iic0 -d w -o 0 -w 8 -c 1 -m tr < 20.hex
root@nanopi-neo:~/i2c # i2c -a 40 -f /dev/iic0 -d r -o 0 -w 8 -c 1 -m tr
20
[8bit multiple write] Clear Registers from offset 5 to 1f [27 bytes]
root@nanopi-neo:~/i2c # i2c -a 40 -f /dev/iic0 -d w -o 5 -w 8 -c 27 -m tr < null.txt
[8bit multiple Read] Read Registers from offset 0 [48 bytes]
root@nanopi-neo:~/i2c # i2c -a 40 -f /dev/iic0 -d r -o 0 -w 8 -c 48 -m tr
20 13 44 4c 46 00 00 00 00 00 00 00 00 00 00 00
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
ff 10 00 00 00 10 00 00 00 10 00 00 00 10 00 00
[8bit multiple write] Fill Register from offset 5 to 1f [27 bytes]
root@nanopi-neo:~/i2c # i2c -a 40 -f /dev/iic0 -d w -o 5 -w 8 -c 27 -m tr < random.txt
root@nanopi-neo:~/i2c # i2c -a 40 -f /dev/iic0 -d r -o 0 -w 8 -c 48 -m tr
20 13 44 4c 46 7e 77 14 67 01 58 1c 2b 06 0e 17
69 10 1e 1f 2e 00 c6 16 0c 04 a2 04 3c 06 1e 11
ff 10 00 00 00 10 00 00 00 10 00 00 00 10 00 00
[16bit write] Clear eeprom data from offset 0 to 1F [32 bytes]
root@nanopi-neo:~/i2c # i2c -a 57 -f /dev/iic0 -d w -o 0 -w 16 -c 32 -m tr < null.txt
[16bit Read] Read eeprom data from offset 0 [48 bytes]
root@nanopi-neo:~/i2c # i2c -a 57 -f /dev/iic0 -d r -o 0 -w 16 -c 48 -m tr
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
f8 0c 00 00 02 04 00 05 34 00 20 00 08 00 28 00
[16bit write] Fill eeprom from offset 0, 32bytes
root@nanopi-neo:~/i2c # i2c -a 57 -f /dev/iic0 -d w -o 0 -w 16 -c 32 -m tr < random.txt
root@nanopi-neo:~/i2c # i2c -a 57 -f /dev/iic0 -d r -o 0 -w 16 -c 48 -m tr
7e 77 b4 67 e1 58 1c 2b a6 0e d7 69 50 1e 9f 2e
a0 c6 76 0c 84 a2 a4 3c 66 1e 51 b4 10 f1 d8 60
f8 0c 00 00 02 04 00 05 34 00 20 00 08 00 28 00
FWIW, I've been also working on twsi a little bit. I used a bunch of I2C peripherals and had to write new drivers for a couple of devices. I found that twsi was either too limiting or buggy with some valid protocol sequences needed for those peripherals. My work can be found here: https://github.com/avg-I/freebsd/commit/1c6777dc1d2ee5ba9e8ff887e9517c58340ce457 https://github.com/avg-I/freebsd/commit/35f319546b594a6094a86bcb18e415d4de2643e2 https://github.com/avg-I/freebsd/commit/5f2d7e7c13f53bfe51644fd0e1757623ca0d7990 https://github.com/avg-I/freebsd/commit/4fc6103770cf48b12c6de275b8c200df0080210a https://github.com/avg-I/freebsd/commit/0075e711e23cc20d7a4d31cf97648c20df8c9ef3 https://github.com/avg-I/freebsd/commit/937a86158404fe84d8135110e01dd60b1e6df81b (In reply to Andriy Gapon from comment #9) Hi Andriy, Been searching a for a patch on twsi.c but couldn't find before. Thank you for sharing your works! I tested some of your commits, below are the results: 1c6777dc1d2ee5ba9e8ff887e9517c58340ce457 - 8bit single byte write not working - eeprom write not working 35f319546b594a6094a86bcb18e415d4de2643e2 -eeprom (16bit addressing) single byte read/write working -eeprom (16bit addressing) continues read/write working -8bit single byte read/write working -8bit continues read/write working 937a86158404fe84d8135110e01dd60b1e6df81b -can't write eeprom (In reply to Andriy Gapon from comment #9) Hi Andriy, Could you handle this PR? I do not have any twsi(4) hardware or driver internals knowledge. (In reply to Vladimir Kondratyev from comment #11) Yes, I am trying as much as my spare time allows. (In reply to alydiomc from comment #10) Did you try each of those commits individually (in isolation) ? Or did you apply them sequentially (accumulated the changes) ? Or something else? Each commit has its own purpose as described in their commit messages. (In reply to Andriy Gapon from comment #13) Hi Andriy, I downloaded the "raw" file individually and then recompiled the kernel. Regards, Alyd (In reply to alydiomc from comment #14) So, you did all of them one by one in isolation from each other? (In reply to Andriy Gapon from comment #15) Hi Andriy, Yes, I tested them separately but only the three commits below: 1c6777dc1d2ee5ba9e8ff887e9517c58340ce457 35f319546b594a6094a86bcb18e415d4de2643e2 937a86158404fe84d8135110e01dd60b1e6df81b Steps: 1. download raw file. 2. copied to ${SRCROOT}/sys/dev/iicbus/twsi/twsi.c 3. rebuild 4. test A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=04622a7f21157827fe75276a4520a52d3a571a86 commit 04622a7f21157827fe75276a4520a52d3a571a86 Author: Andriy Gapon <avg@FreeBSD.org> AuthorDate: 2021-11-26 08:07:27 +0000 Commit: Andriy Gapon <avg@FreeBSD.org> CommitDate: 2021-11-26 14:17:24 +0000 twsi: move handling of TWSI_CONTROL_ACK into the state machine Previously the code set TWSI_CONTROL_ACK in twsi_transfer() based on whether the first message had a length of one. That was done regardless of whether the message was a read or write and what kind of messages followed it. Now the bit is set or cleared while handling TWSI_STATUS_ADDR_R_ACK state transition based on the current (read) message. The old code did not correctly work in a scenario where a single byte was read from an EEPROM device with two byte addressing. For example: i2c -m tr -a 0x50 -d r -w 16 -o 0 -c 1 -v The reason is that the first message (a write) has two bytes, so TWSI_CONTROL_ACK was set and never cleared. Since the controller did not send NACK the EEPROM sent more data resulting in a buffer overrun. While working on TWSI_STATUS_ADDR_R_ACK I also added support for the zero-length read access and then I did the same for zero-length write access. While rare, those types of I2C transactions are completely valid and are used by some devices. PR: 258994 MFC after: 3 weeks sys/dev/iicbus/twsi/twsi.c | 40 ++++++++++++++++++++++++++-------------- 1 file changed, 26 insertions(+), 14 deletions(-) A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=ff1e8581806f70e54fecddf8a6a23488dc7b968a commit ff1e8581806f70e54fecddf8a6a23488dc7b968a Author: Andriy Gapon <avg@FreeBSD.org> AuthorDate: 2021-11-26 09:48:21 +0000 Commit: Andriy Gapon <avg@FreeBSD.org> CommitDate: 2021-11-26 14:20:27 +0000 twsi: support more message combinations in transfers Most prominently, add support for a transfer where a write with no-stop flag is followed by a write with no-start flag. Logically, it's a single larger write, but consumers may want to split it like that because one part can be a register ID and the other part can be data to be written to (or starting at) that register. Such a transfer can be created by i2c tool and iic(4) driver, e.g., for an EEPROM write at specific offset: i2c -m tr -a 0x50 -d w -w 16 -o 0 -c 8 -v < /dev/random This should be fixed by new code that handles the end of data transfer for both reads and writes. It handles two existing conditions and one new. Namely: - the last message has been completed -- end of transfer; - a message has been completed and the next one requires the start condition; - a message has been completed and the next one should be sent without the start condition. In the last case we simply switch to the next message and start sending its data. Reads without the start condition are not supported yet, though. That's because we NACK the last byte of the previous message, so the device stops sending data. To fix this we will need to add a look-ahead at the next message when handling the penultimate byte of the current one. This change also fixed a bug where msg_idx was not incremented after a read message. Apparently, typically a read message is a last message in a transfer, so the bug did not cause much trouble. PR: 258994 MFC after: 3 weeks sys/dev/iicbus/twsi/twsi.c | 85 +++++++++++++++++++++++++++++----------------- 1 file changed, 53 insertions(+), 32 deletions(-) A commit in branch stable/13 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=c7bf34cfe52c09abf813c784a60d7d9677afb603 commit c7bf34cfe52c09abf813c784a60d7d9677afb603 Author: Andriy Gapon <avg@FreeBSD.org> AuthorDate: 2021-11-26 08:07:27 +0000 Commit: Andriy Gapon <avg@FreeBSD.org> CommitDate: 2021-12-17 07:30:41 +0000 twsi: move handling of TWSI_CONTROL_ACK into the state machine Previously the code set TWSI_CONTROL_ACK in twsi_transfer() based on whether the first message had a length of one. That was done regardless of whether the message was a read or write and what kind of messages followed it. Now the bit is set or cleared while handling TWSI_STATUS_ADDR_R_ACK state transition based on the current (read) message. The old code did not correctly work in a scenario where a single byte was read from an EEPROM device with two byte addressing. For example: i2c -m tr -a 0x50 -d r -w 16 -o 0 -c 1 -v The reason is that the first message (a write) has two bytes, so TWSI_CONTROL_ACK was set and never cleared. Since the controller did not send NACK the EEPROM sent more data resulting in a buffer overrun. While working on TWSI_STATUS_ADDR_R_ACK I also added support for the zero-length read access and then I did the same for zero-length write access. While rare, those types of I2C transactions are completely valid and are used by some devices. PR: 258994 (cherry picked from commit 04622a7f21157827fe75276a4520a52d3a571a86) sys/dev/iicbus/twsi/twsi.c | 40 ++++++++++++++++++++++++++-------------- 1 file changed, 26 insertions(+), 14 deletions(-) A commit in branch stable/13 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=6820d578bb1fb8d0a8e781a7f3d59311a35d453f commit 6820d578bb1fb8d0a8e781a7f3d59311a35d453f Author: Andriy Gapon <avg@FreeBSD.org> AuthorDate: 2021-11-26 09:48:21 +0000 Commit: Andriy Gapon <avg@FreeBSD.org> CommitDate: 2021-12-17 07:30:58 +0000 twsi: support more message combinations in transfers Most prominently, add support for a transfer where a write with no-stop flag is followed by a write with no-start flag. Logically, it's a single larger write, but consumers may want to split it like that because one part can be a register ID and the other part can be data to be written to (or starting at) that register. Such a transfer can be created by i2c tool and iic(4) driver, e.g., for an EEPROM write at specific offset: i2c -m tr -a 0x50 -d w -w 16 -o 0 -c 8 -v < /dev/random This should be fixed by new code that handles the end of data transfer for both reads and writes. It handles two existing conditions and one new. Namely: - the last message has been completed -- end of transfer; - a message has been completed and the next one requires the start condition; - a message has been completed and the next one should be sent without the start condition. In the last case we simply switch to the next message and start sending its data. Reads without the start condition are not supported yet, though. That's because we NACK the last byte of the previous message, so the device stops sending data. To fix this we will need to add a look-ahead at the next message when handling the penultimate byte of the current one. This change also fixed a bug where msg_idx was not incremented after a read message. Apparently, typically a read message is a last message in a transfer, so the bug did not cause much trouble. PR: 258994 (cherry picked from commit ff1e8581806f70e54fecddf8a6a23488dc7b968a) sys/dev/iicbus/twsi/twsi.c | 85 +++++++++++++++++++++++++++++----------------- 1 file changed, 53 insertions(+), 32 deletions(-) A commit in branch stable/12 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=074248b948b6fab3ec3a84e2573201fe82190cf3 commit 074248b948b6fab3ec3a84e2573201fe82190cf3 Author: Andriy Gapon <avg@FreeBSD.org> AuthorDate: 2021-11-26 08:07:27 +0000 Commit: Andriy Gapon <avg@FreeBSD.org> CommitDate: 2021-12-17 07:31:21 +0000 twsi: move handling of TWSI_CONTROL_ACK into the state machine Previously the code set TWSI_CONTROL_ACK in twsi_transfer() based on whether the first message had a length of one. That was done regardless of whether the message was a read or write and what kind of messages followed it. Now the bit is set or cleared while handling TWSI_STATUS_ADDR_R_ACK state transition based on the current (read) message. The old code did not correctly work in a scenario where a single byte was read from an EEPROM device with two byte addressing. For example: i2c -m tr -a 0x50 -d r -w 16 -o 0 -c 1 -v The reason is that the first message (a write) has two bytes, so TWSI_CONTROL_ACK was set and never cleared. Since the controller did not send NACK the EEPROM sent more data resulting in a buffer overrun. While working on TWSI_STATUS_ADDR_R_ACK I also added support for the zero-length read access and then I did the same for zero-length write access. While rare, those types of I2C transactions are completely valid and are used by some devices. PR: 258994 (cherry picked from commit 04622a7f21157827fe75276a4520a52d3a571a86) sys/dev/iicbus/twsi/twsi.c | 40 ++++++++++++++++++++++++++-------------- 1 file changed, 26 insertions(+), 14 deletions(-) A commit in branch stable/12 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=e8dbe7bdcf5d9dfbe40b326001b7751e3d42c635 commit e8dbe7bdcf5d9dfbe40b326001b7751e3d42c635 Author: Andriy Gapon <avg@FreeBSD.org> AuthorDate: 2021-11-26 09:48:21 +0000 Commit: Andriy Gapon <avg@FreeBSD.org> CommitDate: 2021-12-17 07:31:39 +0000 twsi: support more message combinations in transfers Most prominently, add support for a transfer where a write with no-stop flag is followed by a write with no-start flag. Logically, it's a single larger write, but consumers may want to split it like that because one part can be a register ID and the other part can be data to be written to (or starting at) that register. Such a transfer can be created by i2c tool and iic(4) driver, e.g., for an EEPROM write at specific offset: i2c -m tr -a 0x50 -d w -w 16 -o 0 -c 8 -v < /dev/random This should be fixed by new code that handles the end of data transfer for both reads and writes. It handles two existing conditions and one new. Namely: - the last message has been completed -- end of transfer; - a message has been completed and the next one requires the start condition; - a message has been completed and the next one should be sent without the start condition. In the last case we simply switch to the next message and start sending its data. Reads without the start condition are not supported yet, though. That's because we NACK the last byte of the previous message, so the device stops sending data. To fix this we will need to add a look-ahead at the next message when handling the penultimate byte of the current one. This change also fixed a bug where msg_idx was not incremented after a read message. Apparently, typically a read message is a last message in a transfer, so the bug did not cause much trouble. PR: 258994 (cherry picked from commit ff1e8581806f70e54fecddf8a6a23488dc7b968a) sys/dev/iicbus/twsi/twsi.c | 85 +++++++++++++++++++++++++++++----------------- 1 file changed, 53 insertions(+), 32 deletions(-) Should be fixed in the stable branches now. Please re-test. |