| Message ID | 15db81d0-ddbd-b590-3996-51e588c5b10a@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | i2c: i801: Fix handling SMBHSTCNT_PEC_EN | expand |
On 21.06.2021 23:08, Heiner Kallweit wrote: > Bit SMBHSTCNT_PEC_EN is used only if software calculates the CRC and > uses register SMBPEC. This is not supported by the driver, it supports > hw-calculation of CRC only (using bit SMBAUXSTS_CRCE). The chip spec > states the following, therefore never set bit SMBHSTCNT_PEC_EN. > > Chapter SMBus CRC Generation and Checking > If the AAC bit is set in the Auxiliary Control register, the PCH > automatically calculates and drives CRC at the end of the transmitted > packet for write cycles, and will check the CRC for read cycles. It will > not transmit the contents of the PEC register for CRC. The PEC bit must > not be set in the Host Control register. If this bit is set, unspecified > behavior will result. > > This patch is based solely on the specification and compile-tested only, > because I have no PEC-capable devices. > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > --- > This patch may be a candidate for stable (with a little bit of fuzz) > once somebody with a PEC-capable device has tested it. > --- > drivers/i2c/busses/i2c-i801.c | 23 +++++++++-------------- > 1 file changed, 9 insertions(+), 14 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c > index 0153a21f4..161176fcd 100644 > --- a/drivers/i2c/busses/i2c-i801.c > +++ b/drivers/i2c/busses/i2c-i801.c > @@ -516,12 +516,9 @@ static int i801_transaction(struct i801_priv *priv, int xact) > > static int i801_block_transaction_by_block(struct i801_priv *priv, > union i2c_smbus_data *data, > - char read_write, int command, > - int hwpec) > + char read_write, int command) > { > - int i, len; > - int status; > - int xact = hwpec ? SMBHSTCNT_PEC_EN : 0; > + int i, len, status, xact = 0; > > switch (command) { > case I2C_SMBUS_BLOCK_PROC_CALL: > @@ -678,8 +675,7 @@ static irqreturn_t i801_isr(int irq, void *dev_id) > */ > static int i801_block_transaction_byte_by_byte(struct i801_priv *priv, > union i2c_smbus_data *data, > - char read_write, int command, > - int hwpec) > + char read_write, int command) > { > int i, len; > int smbcmd; > @@ -777,9 +773,8 @@ static int i801_set_block_buffer_mode(struct i801_priv *priv) > } > > /* Block transaction function */ > -static int i801_block_transaction(struct i801_priv *priv, > - union i2c_smbus_data *data, char read_write, > - int command, int hwpec) > +static int i801_block_transaction(struct i801_priv *priv, union i2c_smbus_data *data, > + char read_write, int command) > { > int result = 0; > unsigned char hostc; > @@ -815,11 +810,11 @@ static int i801_block_transaction(struct i801_priv *priv, > && i801_set_block_buffer_mode(priv) == 0) > result = i801_block_transaction_by_block(priv, data, > read_write, > - command, hwpec); > + command); > else > result = i801_block_transaction_byte_by_byte(priv, data, > read_write, > - command, hwpec); > + command); > > if (command == I2C_SMBUS_I2C_BLOCK_DATA > && read_write == I2C_SMBUS_WRITE) { > @@ -936,8 +931,7 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr, > SMBAUXCTL(priv)); > > if (block) > - ret = i801_block_transaction(priv, data, read_write, size, > - hwpec); > + ret = i801_block_transaction(priv, data, read_write, size); > else > ret = i801_transaction(priv, xact); > > @@ -1625,6 +1619,7 @@ static void i801_setup_hstcfg(struct i801_priv *priv) > unsigned char hstcfg = priv->original_hstcfg; > > hstcfg &= ~SMBHSTCFG_I2C_EN; /* SMBus timing */ > + hstcfg &= ~SMBHSTCNT_PEC_EN; > hstcfg |= SMBHSTCFG_HST_EN; > pci_write_config_byte(priv->pci_dev, SMBHSTCFG, hstcfg); > > Jean, do you have an opinion on this patch? It's been pending for quite some time and I have few more patches that I'd like to submit.
On Sun, 18 Jul 2021 15:51:22 +0200, Heiner Kallweit wrote: > Jean, do you have an opinion on this patch? It's been pending for quite some > time and I have few more patches that I'd like to submit. Sorry for the delay, I'll look into it today. -- Jean Delvare SUSE L3 Support
Hi Heiner, On Mon, 21 Jun 2021 23:08:40 +0200, Heiner Kallweit wrote: > Bit SMBHSTCNT_PEC_EN is used only if software calculates the CRC and > uses register SMBPEC. This is not supported by the driver, it supports > hw-calculation of CRC only (using bit SMBAUXSTS_CRCE). The chip spec > states the following, therefore never set bit SMBHSTCNT_PEC_EN. > > Chapter SMBus CRC Generation and Checking > If the AAC bit is set in the Auxiliary Control register, the PCH > automatically calculates and drives CRC at the end of the transmitted > packet for write cycles, and will check the CRC for read cycles. It will > not transmit the contents of the PEC register for CRC. The PEC bit must > not be set in the Host Control register. If this bit is set, unspecified > behavior will result. > > This patch is based solely on the specification and compile-tested only, > because I have no PEC-capable devices. > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > --- > This patch may be a candidate for stable (with a little bit of fuzz) > once somebody with a PEC-capable device has tested it. I don't think so. One of the conditions for stable branch acceptance is that the patch must fix a real problem that bothers people. Here we have an issue that is present for 20 years and nobody ever reported. I'm all for fixing it upstream, but not in stable kernels. As for testing, I also don't have a PEC-cable device at hand. However, we may still be able to test this change: * If you have a device at 0x69 on the i801 SMBus of any of your system, that would be a clock device, which almost always support PEC. * If you have EEPROMs on your i801 SMBus, you may be lucky and find a sequence of bytes where the PEC computation leads to exactly the value of the following byte. I remember doing that years ago, sadly I can no longer find the script I wrote at that time. Be careful when accessing SPD EEPROMs, you want to read from them, not write to them ;-) Incidentally i2c-tools was just improved to allow arbitrary SMBus block read commands so i2cget can be used for easy testing from user-space. > --- > drivers/i2c/busses/i2c-i801.c | 23 +++++++++-------------- > 1 file changed, 9 insertions(+), 14 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c > index 0153a21f4..161176fcd 100644 > --- a/drivers/i2c/busses/i2c-i801.c > +++ b/drivers/i2c/busses/i2c-i801.c > @@ -516,12 +516,9 @@ static int i801_transaction(struct i801_priv *priv, int xact) > > static int i801_block_transaction_by_block(struct i801_priv *priv, > union i2c_smbus_data *data, > - char read_write, int command, > - int hwpec) > + char read_write, int command) > { > - int i, len; > - int status; > - int xact = hwpec ? SMBHSTCNT_PEC_EN : 0; > + int i, len, status, xact = 0; This can be simplified further. xact no longer needs to be initialized here, you can set its value directly below instead of using bit masking. > > switch (command) { > case I2C_SMBUS_BLOCK_PROC_CALL: > @@ -678,8 +675,7 @@ static irqreturn_t i801_isr(int irq, void *dev_id) > */ > static int i801_block_transaction_byte_by_byte(struct i801_priv *priv, > union i2c_smbus_data *data, > - char read_write, int command, > - int hwpec) > + char read_write, int command) > { > int i, len; > int smbcmd; > @@ -777,9 +773,8 @@ static int i801_set_block_buffer_mode(struct i801_priv *priv) > } > > /* Block transaction function */ > -static int i801_block_transaction(struct i801_priv *priv, > - union i2c_smbus_data *data, char read_write, > - int command, int hwpec) > +static int i801_block_transaction(struct i801_priv *priv, union i2c_smbus_data *data, > + char read_write, int command) > { > int result = 0; > unsigned char hostc; > @@ -815,11 +810,11 @@ static int i801_block_transaction(struct i801_priv *priv, > && i801_set_block_buffer_mode(priv) == 0) > result = i801_block_transaction_by_block(priv, data, > read_write, > - command, hwpec); > + command); > else > result = i801_block_transaction_byte_by_byte(priv, data, > read_write, > - command, hwpec); > + command); > > if (command == I2C_SMBUS_I2C_BLOCK_DATA > && read_write == I2C_SMBUS_WRITE) { > @@ -936,8 +931,7 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr, > SMBAUXCTL(priv)); > > if (block) > - ret = i801_block_transaction(priv, data, read_write, size, > - hwpec); > + ret = i801_block_transaction(priv, data, read_write, size); > else > ret = i801_transaction(priv, xact); > > @@ -1625,6 +1619,7 @@ static void i801_setup_hstcfg(struct i801_priv *priv) > unsigned char hstcfg = priv->original_hstcfg; > > hstcfg &= ~SMBHSTCFG_I2C_EN; /* SMBus timing */ > + hstcfg &= ~SMBHSTCNT_PEC_EN; A comment like /* Disable software PEC */ could be added here. > hstcfg |= SMBHSTCFG_HST_EN; > pci_write_config_byte(priv->pci_dev, SMBHSTCFG, hstcfg); > -- Jean Delvare SUSE L3 Support
On Wed, 21 Jul 2021 14:46:20 +0200, Jean Delvare wrote: > As for testing, I also don't have a PEC-cable device at hand. However, > we may still be able to test this change: > * If you have a device at 0x69 on the i801 SMBus of any of your system, > that would be a clock device, which almost always support PEC. > * If you have EEPROMs on your i801 SMBus, you may be lucky and find a > sequence of bytes where the PEC computation leads to exactly the > value of the following byte. I remember doing that years ago, sadly I > can no longer find the script I wrote at that time. Be careful when > accessing SPD EEPROMs, you want to read from them, not write to them > ;-) Incidentally i2c-tools was just improved to allow arbitrary SMBus > block read commands so i2cget can be used for easy testing from > user-space. Well, what I wrote above wasn't accurate (bad memory I suppose). While SMBus Block Read commands are OK to test the clock devices at 0x69, they are not the best choice to poke a read-only EEPROM, as the first byte will be interpreted as the block length, and if it is not between 1 and 32, it is invalid and the transaction will fail, regardless of PEC. Which in turn dramatically decreases the chances that at least one offset in your EEPROM will work and be usable for testing purposes. Furthermore, i2cget has a safety to prevent you from messing up with your SPD EEPROMs, that will deny using PEC at all in the I2C address range 0x50-0x57. Which is exactly what I was suggesting to do. So I had to recompile i2cget without the safety in order to preform my tests. To be honest I think the safety is overkill (as far as I can see PEC would only trash data in "c" mode so we could limit the safety to that mode) but my testing being clearly a protocol abuse, I'm fine with having to modify the source code to do it. Anyway, for the record, my hackish testing protocol is as follows: # rmmod at24 # modprobe i2c-dev # for i in `seq 0 254` ; do echo $i ; ./tools/i2cget -y 4 0x50 $i bp ; sleep 1 ; done Then I basically look at commands failing (on PEC error), until I am lucky enough that the next byte in the EEPROM matches the expected PEC value. I had 2 such offsets on my first SPD EEPROM (82 and 163). Meaning that I was able to test your patch and I can confirm that it works OK (testing limited to the 8 Series/C220 Series [8086:8c22] device and SMBus Read Byte transactions, but I have no reason to believe other devices or other transaction types would behave differently). Tested-by: Jean Delvare <jdelvare@suse.de> -- Jean Delvare SUSE L3 Support
On 22.07.2021 10:34, Jean Delvare wrote: > On Wed, 21 Jul 2021 14:46:20 +0200, Jean Delvare wrote: >> As for testing, I also don't have a PEC-cable device at hand. However, >> we may still be able to test this change: >> * If you have a device at 0x69 on the i801 SMBus of any of your system, >> that would be a clock device, which almost always support PEC. >> * If you have EEPROMs on your i801 SMBus, you may be lucky and find a >> sequence of bytes where the PEC computation leads to exactly the >> value of the following byte. I remember doing that years ago, sadly I >> can no longer find the script I wrote at that time. Be careful when >> accessing SPD EEPROMs, you want to read from them, not write to them >> ;-) Incidentally i2c-tools was just improved to allow arbitrary SMBus >> block read commands so i2cget can be used for easy testing from >> user-space. > > Well, what I wrote above wasn't accurate (bad memory I suppose). While > SMBus Block Read commands are OK to test the clock devices at 0x69, > they are not the best choice to poke a read-only EEPROM, as the first > byte will be interpreted as the block length, and if it is not between > 1 and 32, it is invalid and the transaction will fail, regardless of > PEC. Which in turn dramatically decreases the chances that at least one > offset in your EEPROM will work and be usable for testing purposes. > > Furthermore, i2cget has a safety to prevent you from messing up with > your SPD EEPROMs, that will deny using PEC at all in the I2C address > range 0x50-0x57. Which is exactly what I was suggesting to do. So I had > to recompile i2cget without the safety in order to preform my tests. To > be honest I think the safety is overkill (as far as I can see PEC would > only trash data in "c" mode so we could limit the safety to that mode) > but my testing being clearly a protocol abuse, I'm fine with having to > modify the source code to do it. > > Anyway, for the record, my hackish testing protocol is as follows: > > # rmmod at24 > # modprobe i2c-dev > # for i in `seq 0 254` ; do echo $i ; ./tools/i2cget -y 4 0x50 $i bp ; sleep 1 ; done > > Then I basically look at commands failing (on PEC error), until I am > lucky enough that the next byte in the EEPROM matches the expected PEC > value. I had 2 such offsets on my first SPD EEPROM (82 and 163). > Meaning that I was able to test your patch and I can confirm that it > works OK (testing limited to the 8 Series/C220 Series [8086:8c22] > device and SMBus Read Byte transactions, but I have no reason to > believe other devices or other transaction types would behave > differently). > > Tested-by: Jean Delvare <jdelvare@suse.de> > Thanks for the comprehensive explanation, Jean. Now that you added your Tested-by: Would you prefer that I send a v2 that incorporates your two smaller comments? Or is it ok as-is?
On Thu, 22 Jul 2021 22:02:37 +0200, Heiner Kallweit wrote: > On 22.07.2021 10:34, Jean Delvare wrote: > > Tested-by: Jean Delvare <jdelvare@suse.de> > > Thanks for the comprehensive explanation, Jean. > Now that you added your Tested-by: Would you prefer that I send a v2 that > incorporates your two smaller comments? Or is it ok as-is? Please resubmit with my 2 comments addressed and including my Tested-by. Then Wolfram can pick it up. Thanks, -- Jean Delvare SUSE L3 Support
diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c index 0153a21f4..161176fcd 100644 --- a/drivers/i2c/busses/i2c-i801.c +++ b/drivers/i2c/busses/i2c-i801.c @@ -516,12 +516,9 @@ static int i801_transaction(struct i801_priv *priv, int xact) static int i801_block_transaction_by_block(struct i801_priv *priv, union i2c_smbus_data *data, - char read_write, int command, - int hwpec) + char read_write, int command) { - int i, len; - int status; - int xact = hwpec ? SMBHSTCNT_PEC_EN : 0; + int i, len, status, xact = 0; switch (command) { case I2C_SMBUS_BLOCK_PROC_CALL: @@ -678,8 +675,7 @@ static irqreturn_t i801_isr(int irq, void *dev_id) */ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv, union i2c_smbus_data *data, - char read_write, int command, - int hwpec) + char read_write, int command) { int i, len; int smbcmd; @@ -777,9 +773,8 @@ static int i801_set_block_buffer_mode(struct i801_priv *priv) } /* Block transaction function */ -static int i801_block_transaction(struct i801_priv *priv, - union i2c_smbus_data *data, char read_write, - int command, int hwpec) +static int i801_block_transaction(struct i801_priv *priv, union i2c_smbus_data *data, + char read_write, int command) { int result = 0; unsigned char hostc; @@ -815,11 +810,11 @@ static int i801_block_transaction(struct i801_priv *priv, && i801_set_block_buffer_mode(priv) == 0) result = i801_block_transaction_by_block(priv, data, read_write, - command, hwpec); + command); else result = i801_block_transaction_byte_by_byte(priv, data, read_write, - command, hwpec); + command); if (command == I2C_SMBUS_I2C_BLOCK_DATA && read_write == I2C_SMBUS_WRITE) { @@ -936,8 +931,7 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr, SMBAUXCTL(priv)); if (block) - ret = i801_block_transaction(priv, data, read_write, size, - hwpec); + ret = i801_block_transaction(priv, data, read_write, size); else ret = i801_transaction(priv, xact); @@ -1625,6 +1619,7 @@ static void i801_setup_hstcfg(struct i801_priv *priv) unsigned char hstcfg = priv->original_hstcfg; hstcfg &= ~SMBHSTCFG_I2C_EN; /* SMBus timing */ + hstcfg &= ~SMBHSTCNT_PEC_EN; hstcfg |= SMBHSTCFG_HST_EN; pci_write_config_byte(priv->pci_dev, SMBHSTCFG, hstcfg);
Bit SMBHSTCNT_PEC_EN is used only if software calculates the CRC and uses register SMBPEC. This is not supported by the driver, it supports hw-calculation of CRC only (using bit SMBAUXSTS_CRCE). The chip spec states the following, therefore never set bit SMBHSTCNT_PEC_EN. Chapter SMBus CRC Generation and Checking If the AAC bit is set in the Auxiliary Control register, the PCH automatically calculates and drives CRC at the end of the transmitted packet for write cycles, and will check the CRC for read cycles. It will not transmit the contents of the PEC register for CRC. The PEC bit must not be set in the Host Control register. If this bit is set, unspecified behavior will result. This patch is based solely on the specification and compile-tested only, because I have no PEC-capable devices. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- This patch may be a candidate for stable (with a little bit of fuzz) once somebody with a PEC-capable device has tested it. --- drivers/i2c/busses/i2c-i801.c | 23 +++++++++-------------- 1 file changed, 9 insertions(+), 14 deletions(-)