Comment 1 Kubilay Kocak 2021-10-08 02:30:57 UTC

^Triage: Set Version to earliest confirmed (12.2-R)

Comment 2 Vladimir Kondratyev 2021-10-08 16:30:11 UTC

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);

Comment 6 Vladimir Kondratyev 2021-10-13 07:13:26 UTC

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

Comment 9 Andriy Gapon 2021-10-22 08:38:42 UTC

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

Comment 11 Vladimir Kondratyev 2021-11-07 21:39:32 UTC

(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.

Comment 12 Andriy Gapon 2021-11-10 14:11:09 UTC

(In reply to Vladimir Kondratyev from comment #11)
Yes, I am trying as much as my spare time allows.

Comment 13 Andriy Gapon 2021-11-10 14:12:48 UTC

(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

Comment 15 Andriy Gapon 2021-11-11 16:26:48 UTC

(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

Comment 17 commit-hook 2021-11-26 14:25:01 UTC

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(-)

Comment 18 commit-hook 2021-11-26 14:25:02 UTC

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(-)

Comment 19 commit-hook 2021-12-17 07:32:16 UTC

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(-)

Comment 20 commit-hook 2021-12-17 07:32:17 UTC

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(-)

Comment 21 commit-hook 2021-12-17 07:32:18 UTC

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(-)

Comment 22 commit-hook 2021-12-17 07:32:19 UTC

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(-)

Comment 23 Andriy Gapon 2021-12-17 07:34:16 UTC

Should be fixed in the stable branches now.
Please re-test.