Re: [PATCH 4/8] i2c: i801: enable FEATURE_IRQ and FEATURE_I2C_BLOCK_READ on all chip versions

[Heiner Kallweit <hkallweit1@gmail.com>]

On 14.06.2022 14:59, Jean Delvare wrote:
> Hi Heiner,
> 
> On Mon, 13 Jun 2022 19:08:41 +0200, Jean Delvare wrote:
>> I was able to resurrect my Sony Vaio GR214EP laptop to test this patch
>> on an Intel 82801CAM (ICH3-M) chipset. And as I suspected, it does not
>> work. I get the following error in the kernel log:
>>
>> [13563.411101] i801_smbus 0000:00:1f.3: SMBus using PCI interrupt
>> [13565.434712] irq 9: nobody cared (try booting with the "irqpoll" option)
> 
> And now it's all coming back to me. The reason why we did not enable
> interrupts on chipsets older than the ICH5 is because they lack bit
> INTS in PCI register PCISTS. When the IRQ line is shared, we don't know
> whether the interrupt was caused by our device or by another device.
> Specifically, the following piece of code fails (it returns IRQ_NONE
> unconditionally):
> 
> static irqreturn_t i801_isr(int irq, void *dev_id)
> {
> 	(...)
> 	/* Confirm this is our interrupt */
> 	pci_read_config_word(priv->pci_dev, PCI_STATUS, &pcists);
> 	if (!(pcists & PCI_STATUS_INTERRUPT))
> 		return IRQ_NONE;
> 
> I tried replacing the code above by a check on SMBHSTSTS instead:
> 
> 	status = inb_p(SMBHSTSTS(priv));
> 	if (!(status & STATUS_FLAGS))
> 		return IRQ_NONE;
> 
> It seems to work, however my testing is limited and I don't know how
> reliable that would be if the other devices sharing the interrupt line
> were used heavily at the same time (the laptop is idling in text mode
> at the moment so the fact that the interrupt line is heavily shared
> does not really get exercised).
> 
> Then I loaded the driver with interrupts disabled
> (disable_features=0x10) to see if I2C block read was working. It is NOT
> working, as can be seen from the following dumps from the memory SPD
> EEPROM:
> 
> linux-w6i1:/home/jdelvare/src/i2c-i801 # i2cdump -y 4 0x50 b
>      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f    0123456789abcdef
> 00: 80 08 04 0d 09 02 40 00 01 75 54 00 82 10 00 01    ??????@.?uT.??.?
> 10: 8f 04 06 01 01 00 0e a0 60 00 00 14 0f 14 2c 20    ?????.??`..???, 
> 20: 15 08 15 08 00 00 00 00 00 00 00 00 00 00 00 00    ????............
> 30: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 12 b9    ..............??
> 40: 7f da 00 00 00 00 00 00 53 53 50 31 33 33 2d 30    ??......SSP133-0
> 50: 36 34 33 32 33 4a 00 00 00 00 00 00 00 01 02 00    64323J.......??.
> 60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
> 70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 64 cd    ..............d?
> 80: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
> 90: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
> a0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
> b0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
> c0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
> d0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
> e0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
> f0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
> 
> linux-w6i1:/home/jdelvare/src/i2c-i801 # i2cdump -y 4 0x50 i
>      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f    0123456789abcdef
> 00: 08 04 0d 09 02 40 00 01 75 54 00 82 10 00 01 8f    ?????@.?uT.??.??
> 10: 04 06 01 01 00 0e a0 60 00 00 14 0f 14 2c 20 15    ????.??`..???, ?
> 20: 08 04 0d 09 02 40 00 01 75 54 00 82 10 00 01 8f    ?????@.?uT.??.??
> 30: 04 06 01 01 00 0e a0 60 00 00 14 0f 14 2c 20 15    ????.??`..???, ?
> 40: 08 04 0d 09 02 40 00 01 75 54 00 82 10 00 01 8f    ?????@.?uT.??.??
> 50: 04 06 01 01 00 0e a0 60 00 00 14 0f 14 2c 20 15    ????.??`..???, ?
> 60: 08 04 0d 09 02 40 00 01 75 54 00 82 10 00 01 8f    ?????@.?uT.??.??
> 70: 04 06 01 01 00 0e a0 60 00 00 14 0f 14 2c 20 15    ????.??`..???, ?
> 80: 08 04 0d 09 02 40 00 01 75 54 00 82 10 00 01 8f    ?????@.?uT.??.??
> 90: 04 06 01 01 00 0e a0 60 00 00 14 0f 14 2c 20 15    ????.??`..???, ?
> a0: 08 04 0d 09 02 40 00 01 75 54 00 82 10 00 01 8f    ?????@.?uT.??.??
> b0: 04 06 01 01 00 0e a0 60 00 00 14 0f 14 2c 20 15    ????.??`..???, ?
> c0: 08 04 0d 09 02 40 00 01 75 54 00 82 10 00 01 8f    ?????@.?uT.??.??
> d0: 04 06 01 01 00 0e a0 60 00 00 14 0f 14 2c 20 15    ????.??`..???, ?
> e0: 08 04 0d 09 02 40 00 01 75 54 00 82 10 00 01 8f    ?????@.?uT.??.??
> f0: 04 06 01 01 00 0e a0 60 00 00 14 0f 14 2c 20 15    ????.??`..???, ?
> 
> As you can see, the requested offset of the I2C block read (0x00, then
> 0x20, then 0x40 etc.) is being ignored, and instead every I2C block
> read starts at EEPROM offset 0x01.
> 
> If you compare the datasheets of the ICH5 (Intel doc 252516-001) and
> ICH3-M (Intel doc 290716-001), section "I2C Read", you will see that
> the description of the command is different. The format described for
> the ICH5 (table 114 in the datasheet) does match what the Linux i2c
> stack calls an I2C block read, while the format described for the
> ICH3-M (table 5-87 in the datasheet) does NOT. Apparently the original
> implementation was aimed at specific devices using 10-bit I2C
> addressing. As no other SMBus host device implemented that, we did not
> even allocate an SMBus command constant to it (and the fact that Intel
> changed the format in later hardware iterations proves that this was the
> right thing to do).
> 
> Looking at the ICH3-M implementation of the "I2C Block Read" transfer,
> I feel very lucky that I did not trash my memory SPD EEPROM while
> running the command. Because the format really starts with a WRITE of 2
> bytes to the EEPROM before reading from it.
> 
> So at least the I2C block read part of the patch is never going to
> work. The interrupt part could work if we change the test as described
> above, however I would question the relevance of making that change
> considering that it is no longer the obvious clean-up you originally
> proposed, and would then impact recent hardware too. I would hate to
> introduce a regression for the sole purpose of enabling interrupts on
> 20-year old hardware which I doubt many people are still using.
> 
> I am available to perform any test you want me to on this old laptop.
> As long as it runs...
> 
Thanks for testing! So I'll drop this patch from the series.