i2c-i801: Regression between 2.6.19-1 & 2.6.23.9

DISCUSSION Ivo Manca 2 participants 7 messages Jan 9, 2008 – Feb 8, 2008 6 messages not in the archive I2C subsystem host drivers

lore

Share & tools

lore

Ivo Manca

author

2008-01-09 11:08 UTC #0 lore raw

Hey all,

Trying to test interrupt support for i801 I learned that somehow between kernel version 2.6.22.14 and 2.6.23.9, something went wrong with the bus driver. Is this a known issue?
When I try to use i2cdump to block-read an EEP OM, my i2cbus hangs.

Please find my output for 2.6.19-1 (working), 2.6.22.14-72 (working) and 2.6.23.9-85 (not working).

Note: SMBus: Intel Corporation 82801EB/ER (ICH5/ICH5R)

Ivo

[root@localhost ~]# uname -r
2.6.19-1.2911.fc6

[root@localhost ~]# modprobe i2c-dev [root@localhost ~]# i2cdetect -y 0
0 1 2 3 4 5 6 7 8 9 a b c d e f
00: XX XX XX XX XX 08 XX XX XX XX XX XX XX
10: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX 20: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX 30: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX 40: XX XX XX XX 44 XX XX XX XX XX XX XX XX XX XX XX 50: 50 XX XX XX 54 55 XX XX XX XX XX XX XX XX XX XX 60: XX XX XX XX XX XX XX XX XX 69 XX XX XX XX XX XX 70: XX XX XX 73 XX XX XX XX

[root@localhost ~]# i2cdump -y 0 0x55 s

     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f    0123456789abcdef
00: 08 07 0d 0a 01 40 00 04 60 70 00 82 08 00 01 0e    ?????@.?`p.??.??
10: 04 0c 01 02 20 c0 75 70 00 00 48 30 48 2a 40 75    ???? ?up..H0H*@u

[root@localhost ~]# i2cdump -y 0 0x55 s

     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f    0123456789abcdef
00: 08 07 0d 0a 01 40 00 04 60 70 00 82 08 00 01 0e    ?????@.?`p.??.??
10: 04 0c 01 02 20 c0 75 70 00 00 48 30 48 2a 40 75    ???? ?up..H0H*@u

====

[root@localhost ~]# uname -r
2.6.22.14-72.fc6

[root@localhost ~]# modprobe i2c-dev

[root@localhost ~]# i2cdetect -y 0
0 1 2 3 4 5 6 7 8 9 a b c d e f
00: XX XX XX XX XX 08 XX XX XX XX XX XX XX
10: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX 20: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX 30: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX 40: XX XX XX XX 44 XX XX XX XX XX XX XX XX XX XX XX 50: 50 XX XX XX 54 55 XX XX XX XX XX XX XX XX XX XX 60: XX XX XX XX XX XX XX XX XX 69 XX XX XX XX XX XX 70: XX XX XX 73 XX XX XX XX

[root@localhost ~]# i2cdump -y 0 0x55 s

     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f    0123456789abcdef
00: 08 07 0d 0a 01 40 00 04 60 70 00 82 08 00 01 0e    ?????@.?`p.??.??
10: 04 0c 01 02 20 c0 75 70 00 00 48 30 48 2a 40 75    ???? ?up..H0H*@u

[root@localhost ~]# i2cdump -y 0 0x55 s

     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f    0123456789abcdef
00: 08 07 0d 0a 01 40 00 04 60 70 00 82 08 00 01 0e    ?????@.?`p.??.??
10: 04 0c 01 02 20 c0 75 70 00 00 48 30 48 2a 40 75    ???? ?up..H0H*@u

====

[root@localhost ~]# uname -r
2.6.23.9-85.fc8

[root@localhost ~]# modprobe i2c-dev

[root@localhost ~]# i2cdetect -y 0
0 1 2 3 4 5 6 7 8 9 a b c d e f
00: -- -- -- -- -- 08 -- -- -- -- -- -- --
10: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 20: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 30: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 40: -- -- -- -- 44 -- -- -- -- -- -- -- -- -- -- -- 50: 50 -- -- -- 54 55 -- -- -- -- -- -- -- -- -- -- 60: -- -- -- -- -- -- -- -- -- 69 -- -- -- -- -- -- 70: -- -- -- 73 -- -- -- --

[root@localhost ~]# i2cdump -y 0 0x55 s
Error: Block read failed, return code -1

(i2c Bus hangs completely, poweroff needed.)

A message that is not in the archive (sent off-list or not yet received): dba8564e0801090308j34215f98rd758dff3702b194-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org

Jean Delvare

2008-01-09 12:53 UTC #2 lore raw

Hi Ivo,

Quoted 3 lines · Ivo Manca

Trying to test interrupt support for i801 I learned that somehow between kernel version 2.6.22.14 and 2.6.23.9, something went wrong with the bus driver. Is this a known issue?

No, you are the first person to report this problem as far as I know.

Quoted 1 line

When I try to use i2cdump to block-read an EEP OM, my i2cbus hangs.

What is an "EEP OM"?

Please note that EEPROMs typically want I2C block reads (mode "i" of i2cdump) and not SMBus block reads. Using the wrong mode shouldn't hang the bus though.

Quoted 51 lines

Please find my output for 2.6.19-1 (working), 2.6.22.14-72 (working) and 2.6.23.9-85 (not working).
Note: SMBus: Intel Corporation 82801EB/ER (ICH5/ICH5R)
Ivo
[root@localhost ~]# uname -r
2.6.19-1.2911.fc6
[root@localhost ~]# modprobe i2c-dev [root@localhost ~]# i2cdetect -y 0
0 1 2 3 4 5 6 7 8 9 a b c d e f
00: XX XX XX XX XX 08 XX XX XX XX XX XX XX
10: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX 20: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX 30: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX 40: XX XX XX XX 44 XX XX XX XX XX XX XX XX XX XX XX 50: 50 XX XX XX 54 55 XX XX XX XX XX XX XX XX XX XX 60: XX XX XX XX XX XX XX XX XX 69 XX XX XX XX XX XX 70: XX XX XX 73 XX XX XX XX
[root@localhost ~]# i2cdump -y 0 0x55 s
     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f    0123456789abcdef
00: 08 07 0d 0a 01 40 00 04 60 70 00 82 08 00 01 0e    ?????@.?`p.??.??
10: 04 0c 01 02 20 c0 75 70 00 00 48 30 48 2a 40 75    ???? ?up..H0H*@u
[root@localhost ~]# i2cdump -y 0 0x55 s
     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f    0123456789abcdef
00: 08 07 0d 0a 01 40 00 04 60 70 00 82 08 00 01 0e    ?????@.?`p.??.??
10: 04 0c 01 02 20 c0 75 70 00 00 48 30 48 2a 40 75    ???? ?up..H0H*@u
====
[root@localhost ~]# uname -r
2.6.22.14-72.fc6
[root@localhost ~]# modprobe i2c-dev
[root@localhost ~]# i2cdetect -y 0
0 1 2 3 4 5 6 7 8 9 a b c d e f
00: XX XX XX XX XX 08 XX XX XX XX XX XX XX
10: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX 20: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX 30: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX 40: XX XX XX XX 44 XX XX XX XX XX XX XX XX XX XX XX 50: 50 XX XX XX 54 55 XX XX XX XX XX XX XX XX XX XX 60: XX XX XX XX XX XX XX XX XX 69 XX XX XX XX XX XX 70: XX XX XX 73 XX XX XX XX
[root@localhost ~]# i2cdump -y 0 0x55 s

Quoted 8 lines

     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f    0123456789abcdef
00: 08 07 0d 0a 01 40 00 04 60 70 00 82 08 00 01 0e    ?????@.?`p.??.??
10: 04 0c 01 02 20 c0 75 70 00 00 48 30 48 2a 40 75    ???? ?up..H0H*@u

[root@localhost ~]# i2cdump -y 0 0x55 s

     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f    0123456789abcdef
00: 08 07 0d 0a 01 40 00 04 60 70 00 82 08 00 01 0e    ?????@.?`p.??.??
10: 04 0c 01 02 20 c0 75 70 00 00 48 30 48 2a 40 75    ???? ?up..H0H*@u

So 2.6.22 works as good as 2.6.19 did, right?

Quoted 8 lines

====
[root@localhost ~]# uname -r
2.6.23.9-85.fc8
[root@localhost ~]# modprobe i2c-dev

Quoted 10 lines

[root@localhost ~]# i2cdetect -y 0
0 1 2 3 4 5 6 7 8 9 a b c d e f
00: -- -- -- -- -- 08 -- -- -- -- -- -- --
10: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 20: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 30: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 40: -- -- -- -- 44 -- -- -- -- -- -- -- -- -- -- -- 50: 50 -- -- -- 54 55 -- -- -- -- -- -- -- -- -- -- 60: -- -- -- -- -- -- -- -- -- 69 -- -- -- -- -- -- 70: -- -- -- 73 -- -- -- --

I note that this version of i2cdetect is more recent than the one you used when testing 2.6.19-1.2911.fc6 and 2.6.22.14-72.fc6 kernels. I suppose that this is the case for i2cdump as well... To make sure that this is really an i2c-i801 driver issue and not a user-space issue, can you please try again using the exact same version of i2cdetect and i2cdump?

Quoted 8 lines

[root@localhost ~]# i2cdump -y 0 0x55 s
Error: Block read failed, return code -1
[root@localhost ~]# i2cdump -y 0 0x55 s
Error: Block read failed, return code -1
(i2c Bus hangs completely, poweroff needed.)

Note that i2cdump relies on i2c-dev, so the problem could be in i2c-dev. There were changes to i2c-dev between 2.6.22 and 2.6.23, that affect I2C block reads, but not SMBus block reads. There were two large patches applied to i2c-i801:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=ca8b9e32a11a7cbfecbef00c8451a79fe1af392e
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=7edcb9abb594a8f3b4ca756e03d01c870aeae127

You may also want to check what fedora-specific patches your kernels include for i2c-i801 and i2c-dev, if any. Can you reproduce the problem with a vanilla 2.6.23.12 kernel?

On my side I'll check if I can reproduce the problem on one of my test systems. I don't test SMBus block reads very often so I could have missed it.

Signature · 1 line

Jean Delvare

A message that is not in the archive (sent off-list or not yet received): [email protected]

Ivo Manca

author

2008-01-09 13:47 UTC #4 lore raw

Re: i2c-i801: Regression between 2.6.22.9 & 2.6.23.9

Hey Jean,

Quoted 9 lines · Jean Delvare

Hi Ivo,
Quoted 4 lines · Ivo Manca
Trying to test interrupt support for i801 I learned that somehow between kernel version 2.6.22.14 and 2.6.23.9, something went wrong with the bus driver. Is this a known issue?

Quoted 8 lines

No, you are the first person to report this problem as far as I know.
Quoted 2 lines
When I try to use i2cdump to block-read an EEP OM, my i2cbus hangs.
What is an "EEP OM"?

Sorry, that should read "EEPROM" ofcourse.

Quoted 4 lines

Please note that EEPROMs typically want I2C block reads (mode "i" of i2cdump) and not SMBus block reads. Using the wrong mode shouldn't hang the bus though.

I know; however, i2cdump states my bus doesn't have i2c block read capabilities. That's why I used SMBus block reads, which seemed to work properly.

Quoted 55 lines

Quoted 55 lines
Please find my output for 2.6.19-1 (working), 2.6.22.14-72 (working) and 2.6.23.9-85 (not working).
Note: SMBus: Intel Corporation 82801EB/ER (ICH5/ICH5R)
Ivo
[root@localhost ~]# uname -r
2.6.19-1.2911.fc6
[root@localhost ~]# modprobe i2c-dev [root@localhost ~]# i2cdetect -y 0
0 1 2 3 4 5 6 7 8 9 a b c d e f
00: XX XX XX XX XX 08 XX XX XX XX XX XX XX
10: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX 20: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX 30: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX 40: XX XX XX XX 44 XX XX XX XX XX XX XX XX XX XX XX 50: 50 XX XX XX 54 55 XX XX XX XX XX XX XX XX XX XX 60: XX XX XX XX XX XX XX XX XX 69 XX XX XX XX XX XX 70: XX XX XX 73 XX XX XX XX
[root@localhost ~]# i2cdump -y 0 0x55 s
     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f    0123456789abcdef
00: 08 07 0d 0a 01 40 00 04 60 70 00 82 08 00 01 0e    ?????@.?`p.??.??
10: 04 0c 01 02 20 c0 75 70 00 00 48 30 48 2a 40 75    ???? ?up..H0H*@u
[root@localhost ~]# i2cdump -y 0 0x55 s
     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f    0123456789abcdef
00: 08 07 0d 0a 01 40 00 04 60 70 00 82 08 00 01 0e    ?????@.?`p.??.??
10: 04 0c 01 02 20 c0 75 70 00 00 48 30 48 2a 40 75    ???? ?up..H0H*@u
====
[root@localhost ~]# uname -r
2.6.22.14-72.fc6
[root@localhost ~]# modprobe i2c-dev
[root@localhost ~]# i2cdetect -y 0
0 1 2 3 4 5 6 7 8 9 a b c d e f
00: XX XX XX XX XX 08 XX XX XX XX XX XX XX
10: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX 20: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX 30: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX 40: XX XX XX XX 44 XX XX XX XX XX XX XX XX XX XX XX 50: 50 XX XX XX 54 55 XX XX XX XX XX XX XX XX XX XX 60: XX XX XX XX XX XX XX XX XX 69 XX XX XX XX XX XX 70: XX XX XX 73 XX XX XX XX
[root@localhost ~]# i2cdump -y 0 0x55 s
     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f    0123456789abcdef
00: 08 07 0d 0a 01 40 00 04 60 70 00 82 08 00 01 0e    ?????@.?`p.??.??
10: 04 0c 01 02 20 c0 75 70 00 00 48 30 48 2a 40 75    ???? ?up..H0H*@u

Quoted 8 lines

Quoted 5 lines

[root@localhost ~]# i2cdump -y 0 0x55 s

     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f    0123456789abcdef
00: 08 07 0d 0a 01 40 00 04 60 70 00 82 08 00 01 0e    ?????@.?`p.??.??
10: 04 0c 01 02 20 c0 75 70 00 00 48 30 48 2a 40 75    ???? ?up..H0H*@u

So 2.6.22 works as good as 2.6.19 did, right?

Correct. I had to test this at university, since I don't have to proper hardware at home. Because they machine's there are running FC6 with 2.6.19 kernels, I just wanted to be sure the problem didn't start between 2.6.19 and 2.6.22.9. Being in a hurry, I forgot to change the subject of this mail (and of course, only noticed just after pushing the send button).

Quoted 19 lines

Quoted 7 lines
====
[root@localhost ~]# uname -r
2.6.23.9-85.fc8
[root@localhost ~]# modprobe i2c-dev
Quoted 11 lines
[root@localhost ~]# i2cdetect -y 0
0 1 2 3 4 5 6 7 8 9 a b c d e f
00: -- -- -- -- -- 08 -- -- -- -- -- -- --
10: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 20: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 30: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 40: -- -- -- -- 44 -- -- -- -- -- -- -- -- -- -- -- 50: 50 -- -- -- 54 55 -- -- -- -- -- -- -- -- -- -- 60: -- -- -- -- -- -- -- -- -- 69 -- -- -- -- -- -- 70: -- -- -- 73 -- -- -- --

Quoted 7 lines

I note that this version of i2cdetect is more recent than the one you used when testing 2.6.19-1.2911.fc6 and 2.6.22.14-72.fc6 kernels. I suppose that this is the case for i2cdump as well... To make sure that this is really an i2c-i801 driver issue and not a user-space issue, can you please try again using the exact same version of i2cdetect and i2cdump?

True. I used an old version of the fedora lm_sensors package. I updated both installations to the newest version available @ ATrpms.net. This did not make any difference.

Quoted 6 lines

Quoted 6 lines
[root@localhost ~]# i2cdump -y 0 0x55 s
Error: Block read failed, return code -1
[root@localhost ~]# i2cdump -y 0 0x55 s
Error: Block read failed, return code -1

Quoted 6 lines

Quoted 2 lines
(i2c Bus hangs completely, poweroff needed.)
Note that i2cdump relies on i2c-dev, so the problem could be in i2c-dev. There were changes to i2c-dev between 2.6.22 and 2.6.23, that affect I2C block reads, but not SMBus block reads.

Not sure if it helps, but the 2.6.23.9 i2c_i801 module loaded on a 2.6.22.9 kernel gave the same error message and problems. Yes: I'm aware that I'm not supposed to test modules like that ;)

Quoted 5 lines

There were two large
patches applied to i2c-i801:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=ca8b9e32a11a7cbfecbef00c8451a79fe1af392e
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=7edcb9abb594a8f3b4ca756e03d01c870aeae127

I already noticed these patches while trying to convert my patch, which adds interrupt support, to 2.6.23.9. I was happy to see them, since I was always curious why defines were not used for statusses and why "/* set 32 byte buffer */" was there without doing anything ;)

Since I was using an ICH5/ICH5R, which is not listed in the switch statement at the probe function, I should be defaulting to isich = 0 and therefor using i801_block_transaction_byte_by_byte. I'll look into the exact changes made here.

Quoted 8 lines

You may also want to check what fedora-specific patches your kernels include for i2c-i801 and i2c-dev, if any. Can you reproduce the problem with a vanilla 2.6.23.12 kernel?
On my side I'll check if I can reproduce the problem on one of my test systems. I don't test SMBus block reads very often so I could have missed it.

I'll check with Hans for fedora specific patches and install a vanilla kernel as soon as time permits; I hope tomorrow.

Thanks,

Ivo

A message that is not in the archive (sent off-list or not yet received): [email protected]

Jean Delvare

2008-01-09 14:42 UTC #6 lore raw

Re: i2c-i801: Regression between 2.6.22.9 & 2.6.23.9

Quoted 4 lines · Ivo Manca

Quoted 1 line · Jean Delvare
What is an "EEP OM"?
Sorry, that should read "EEPROM" ofcourse.

Err, of course. I'm not very smart today it seems...

Quoted 7 lines

Quoted 3 lines
Please note that EEPROMs typically want I2C block reads (mode "i" of i2cdump) and not SMBus block reads. Using the wrong mode shouldn't hang the bus though.
I know; however, i2cdump states my bus doesn't have i2c block read capabilities. That's why I used SMBus block reads, which seemed to work properly.

OK. This is an important fact when tracking this regression. I guess that the 1st byte returned by your EEPROM is 0x80, which is NOT a valid SMBus block length. This means that it is expected that an error is returned. It wasn't the case before (the invalid length was adjust to the nearest valid valid length, i.e. 32) but that was changed in 2.6.23 to return an error instead, and I think this is the right thing to do:

Review

Quoted 12 lines

                if (i == 1 && read_write == I2C_SMBUS_READ) {
                        len = inb_p(SMBHSTDAT0);

(quoted diff) +4 -4

-                       if (len < 1)
-                               len = 1;
-                       if (len > 32)
-                               len = 32;
+                       if (len < 1 || len > I2C_SMBUS_BLOCK_MAX) {
+                               result = -1;
+                               goto END;
+                       }
                        data->block[0] = len;
                }

So, the fact that you get an error with 2.6.23 when you did not in 2.6.22 is expected, this is not a regression. I get the same error on my test system. The only difference is that it does NOT hang the bus for me.

BTW, I2C block read is now supported in -mm:
http://khali.linux-fr.org/devel/linux-2.6/jdelvare-i2c/i2c-i801-04-add-support-for-i2c-block-read.patch

Note that the chip you have at 0x69 is most certainly a clock chip and many clock chip do support the SMBus block read transaction. This is how I test it, maybe you can try this and see if it hangs as well or not.

Quoted 4 lines

Since I was using an ICH5/ICH5R, which is not listed in the switch statement at the probe function, I should be defaulting to isich = 0 and therefor using i801_block_transaction_byte_by_byte. I'll look into the exact changes made here.

The ICH5 _is_ listed in the switch statement:

Quoted 1 line

	case PCI_DEVICE_ID_INTEL_82801EB_3:

So it should use i801_block_transaction_by_block(). You might want to test removing this case from the switch statement to force the driver to use i801_block_transaction_byte_by_byte() instead, and see if it makes a difference.

Quoted 3 lines

Quoted 3 lines
On my side I'll check if I can reproduce the problem on one of my test systems. I don't test SMBus block reads very often so I could have missed it.

I did that, with an ICH5 as well, I get the error when using an SMBus block read on an EEPROM, as expected, but the bus never hangs. I have no idea why it hangs for you and not for me. If you figure it out, please let me know.

Signature · 1 line

Jean Delvare

A message that is not in the archive (sent off-list or not yet received): [email protected]

Ivo Manca

author

2008-01-09 18:09 UTC #8 lore raw

Re: i2c-i801: Regression between 2.6.22.9 & 2.6.23.9

Quoted 21 lines · Jean Delvare

Quoted 6 lines · Ivo Manca
Quoted 2 lines · Jean Delvare
What is an "EEP OM"?
Sorry, that should read "EEPROM" ofcourse.
Err, of course. I'm not very smart today it seems...
Quoted 8 lines
Quoted 4 lines
Please note that EEPROMs typically want I2C block reads (mode "i" of i2cdump) and not SMBus block reads. Using the wrong mode shouldn't hang the bus though.
I know; however, i2cdump states my bus doesn't have i2c block read capabilities. That's why I used SMBus block reads, which seemed to work properly.

Quoted 7 lines

OK. This is an important fact when tracking this regression. I guess that the 1st byte returned by your EEPROM is 0x80, which is NOT a valid SMBus block length. This means that it is expected that an error is returned. It wasn't the case before (the invalid length was adjust to the nearest valid valid length, i.e. 32) but that was changed in 2.6.23 to return an error instead, and I think this is the right thing to do:

Ahhhh, seeing the code this way makes perfect sense. Funny, that actual correcting something makes me feel like it's an error.

Quoted 14 lines

Review

Quoted 13 lines

                if (i == 1 && read_write == I2C_SMBUS_READ) {
                        len = inb_p(SMBHSTDAT0);

(quoted diff) +4 -4

-                       if (len < 1)
-                               len = 1;
-                       if (len > 32)
-                               len = 32;
+                       if (len < 1 || len > I2C_SMBUS_BLOCK_MAX) {
+                               result = -1;
+                               goto END;
+                       }
                        data->block[0] = len;
                }

Quoted 5 lines

So, the fact that you get an error with 2.6.23 when you did not in 2.6.22 is expected, this is not a regression. I get the same error on my test system. The only difference is that it does NOT hang the bus for me.

Thanks for showing me; I agree it's not a regression. However, the bus normally did not hang and now it does, which kinda feels like an awful downside. Maybe I'm just too lazy to unplug the power cord every time I test something ;).

Quoted 3 lines

BTW, I2C block read is now supported in -mm:
http://khali.linux-fr.org/devel/linux-2.6/jdelvare-i2c/i2c-i801-04-add-support-for-i2c-block-read.patch

Some more to apply to my patch then :-). Thanks for noticing me, now I can properly test interrupt support tomorrow.

Quoted 6 lines

Note that the chip you have at 0x69 is most certainly a clock chip and many clock chip do support the SMBus block read transaction. This is how I test it, maybe you can try this and see if it hangs as well or not.

Quoted 11 lines

Quoted 5 lines
Since I was using an ICH5/ICH5R, which is not listed in the switch statement at the probe function, I should be defaulting to isich = 0 and therefor using i801_block_transaction_byte_by_byte. I'll look into the exact changes made here.
The ICH5 _is_ listed in the switch statement:
Quoted 2 lines
	case PCI_DEVICE_ID_INTEL_82801EB_3:

Doh. Not my day either.

Quoted 5 lines

So it should use i801_block_transaction_by_block(). You might want to test removing this case from the switch statement to force the driver to use i801_block_transaction_byte_by_byte() instead, and see if it makes a difference.

Will try soon.

Quoted 11 lines

Quoted 4 lines
Quoted 4 lines
On my side I'll check if I can reproduce the problem on one of my test systems. I don't test SMBus block reads very often so I could have missed it.
I did that, with an ICH5 as well, I get the error when using an SMBus block read on an EEPROM, as expected, but the bus never hangs. I have no idea why it hangs for you and not for me. If you figure it out, please let me know.

I'll still have to do quite a lot of testing, so if I stumble acros the caus, I'll let you know.

Just a quick update:
Interrupt support seemed to work well for both block and byte reads in 2.6.22.9. However, the code was too ugly and full with awful hacks, so I've converted it to a proper patch for both 2.6.22.9 and (later) 2.6.23.9. Since I do not have the proper hardware at home, I had to wait for today & tomorrow to test.

I'm very curious whether it will work or not, and especially, how fast it'll be with i2c block reads.

Last test results shows:

Old driver:
(time 25x i2cdump -s)

real    0m27.375s
user    0m0.008s
sys     0m0.023s

--
I2C_SMBUS_QUICK(nodev) 0.00222
I2C_SMBUS_QUICK 0.00238
I2C_SMBUS_BYTE 0.00203
I2C_SMBUS_BYTE_DATA 0.00203
I2C_SMBUS_WORD_DATA 0.00216

New driver:
(time 25x i2cdump -s)
./bla (i2c-dump 25x)

real    0m24.215s
user    0m0.013s
sys     0m0.175s

--
I2C_SMBUS_QUICK(nodev) 0.00112
I2C_SMBUS_QUICK 0.00112
I2C_SMBUS_BYTE 0.00110
I2C_SMBUS_BYTE_DATA 0.00113
I2C_SMBUS_WORD_DATA 0.00108

A message that is not in the archive (sent off-list or not yet received): [email protected]

Jean Delvare

2008-01-10 14:09 UTC #10 lore raw

Re: i2c-i801: Regression between 2.6.22.9 & 2.6.23.9

Hi Ivo,

Quoted 2 lines · Ivo Manca

I'll still have to do quite a lot of testing, so if I stumble acros the caus, I'll let you know.

OK, great.

Quoted 9 lines

Just a quick update:
Interrupt support seemed to work well for both block and byte reads in 2.6.22.9. However, the code was too ugly and full with awful hacks, so I've converted it to a proper patch for both 2.6.22.9 and (later) 2.6.23.9. Since I do not have the proper hardware at home, I had to wait for today & tomorrow to test.
I'm very curious whether it will work or not, and especially, how fast it'll be with i2c block reads.

I am very interested in this as well. I have several ICH3-M, ICH5 and ICH7-based systems here for testing as soon as you have something good enough to be published.

For devices which support the block buffer (i.e. ICH4 and later), Oleg's patch improved speed dramatically for block transactions already. I don't think that you will win much with interrupts here, except maybe for HZ=100. I think that you will win a lot more on short transactions, where polled mode has to wait for at least one jiffie (and more like 2 on practice) and interrupt mode could be one or two orders of magnitude faster than this. For hardware monitoring chips with a lot of registers (e.g. LM85 or ADM1026) the speedup should be very visible.

Quoted 7 lines

Last test results shows:
Old driver:
(time 25x i2cdump -s)
real    0m27.375s
user    0m0.008s
sys     0m0.023s    

Quoted 6 lines

--
I2C_SMBUS_QUICK(nodev) 0.00222
I2C_SMBUS_QUICK 0.00238
I2C_SMBUS_BYTE 0.00203
I2C_SMBUS_BYTE_DATA 0.00203
I2C_SMBUS_WORD_DATA 0.00216

What value of HZ are you using? It matters for polled mode. From the figures above I'd guess HZ=1000. Try again with HZ=250 or even HZ=100 if you want more impressive figures ;)

Quoted 6 lines

New driver:
(time 25x i2cdump -s)
./bla (i2c-dump 25x)
real    0m24.215s
user    0m0.013s
sys     0m0.175s

Quoted 6 lines

--
I2C_SMBUS_QUICK(nodev) 0.00112
I2C_SMBUS_QUICK 0.00112
I2C_SMBUS_BYTE 0.00110
I2C_SMBUS_BYTE_DATA 0.00113
I2C_SMBUS_WORD_DATA 0.00108

So that would be a 2x improvement for short transactions at HZ=1000... and 20x at HZ=100. Very nice :)

Signature · 1 line

Jean Delvare

A message that is not in the archive (sent off-list or not yet received): [email protected]

Ivo Manca

author

2008-02-08 16:22 UTC #12 lore raw

I2c-i801 interrupt support (was: Re: i2c-i801: Regression between 2.6.22.9 & 2.6.23.9)

Hey Jean,

Quoted 7 lines · Jean Delvare

Hi Ivo,
I am very interested in this as well. I have several ICH3-M, ICH5 and ICH7-based systems here for testing as soon as you have something good enough to be published.

Quoted 11 lines

For devices which support the block buffer (i.e. ICH4 and later), Oleg's patch improved speed dramatically for block transactions already. I don't think that you will win much with interrupts here, except maybe for HZ=100. I think that you will win a lot more on short transactions, where polled mode has to wait for at least one jiffie (and more like 2 on practice) and interrupt mode could be one or two orders of magnitude faster than this. For hardware monitoring chips with a lot of registers (e.g. LM85 or ADM1026) the speedup should be very visible.

Sadly I haven't been able to test this improvement due to lack of (available) hardware. The last test showed a bug which crashes the controller but I think I've already fixed it. Luckily, Hans de Goede arranged a spare PC which I can use for testing and development. I hope to pick it up soon and development should speed up then!

So, thanks to the HHS (Hague University) for making a system available for testing and thanks to Hans for arranging this!

Quoted 4 lines

What value of HZ are you using? It matters for polled mode. From the figures above I'd guess HZ=1000. Try again with HZ=250 or even HZ=100 if you want more impressive figures ;)

Default, so 1000 I suppose.

Quoted 14 lines

Quoted 6 lines
New driver:
(time 25x i2cdump -s)
./bla (i2c-dump 25x)
real    0m24.215s
user    0m0.013s
sys     0m0.175s
Quoted 7 lines
--
I2C_SMBUS_QUICK(nodev) 0.00112
I2C_SMBUS_QUICK 0.00112
I2C_SMBUS_BYTE 0.00110
I2C_SMBUS_BYTE_DATA 0.00113
I2C_SMBUS_WORD_DATA 0.00108

Quoted 3 lines

So that would be a 2x improvement for short transactions at HZ=1000... and 20x at HZ=100. Very nice :)

Seems quite nice yes, let's just hope I/we can get it stable enough!

Ivo

_______________________________________________
i2c mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/i2c