| Message ID | 20240214155939.728155bc@endymion.delvare |
|---|---|
| State | Accepted |
| Delegated to: | Andi Shyti |
| Headers | show |
| Series | i2c: i801: Fix block process call transactions | expand |
Hi Jean, thanks for looking into this! On Wed, 2024-02-14 at 15:59 +0100, Jean Delvare wrote: > According to the Intel datasheets, software must reset the block > buffer index twice for block process call transactions: once before > writing the outgoing data to the buffer, and once again before > reading the incoming data from the buffer. > > The driver is currently missing the second reset, causing the wrong > portion of the block buffer to be read. > > Signed-off-by: Jean Delvare <jdelvare@suse.de> > Reported-by: Piotr Zakowski <piotr.zakowski@intel.com> > Closes: https://lore.kernel.org/linux-i2c/20240213120553.7b0ab120@endymion.delvare/ > Fixes: 315cd67c9453 ("i2c: i801: Add Block Write-Block Read Process Call support") According to all the datasheets I've been able to find, the patch makes sense to me. Reviewed-by: Alexander Sverdlin <alexander.sverdlin@gmail.com> > --- > Piotr, does this change make your tests succeed? > > Alexander, I don't suppose you still have access to the hardware on > which you were using block process call transactions back in 2019, but > maybe you remember having to do the same change to make it work? Nope, I cannot explain how this went unnoticed... I don't think we had a downstream patch on top back than. > drivers/i2c/busses/i2c-i801.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > --- linux-6.6.orig/drivers/i2c/busses/i2c-i801.c > +++ linux-6.6/drivers/i2c/busses/i2c-i801.c > @@ -500,11 +500,10 @@ static int i801_block_transaction_by_blo > /* Set block buffer mode */ > outb_p(inb_p(SMBAUXCTL(priv)) | SMBAUXCTL_E32B, SMBAUXCTL(priv)); > > - inb_p(SMBHSTCNT(priv)); /* reset the data buffer index */ > - > if (read_write == I2C_SMBUS_WRITE) { > len = data->block[0]; > outb_p(len, SMBHSTDAT0(priv)); > + inb_p(SMBHSTCNT(priv)); /* reset the data buffer index */ > for (i = 0; i < len; i++) > outb_p(data->block[i+1], SMBBLKDAT(priv)); > } > @@ -522,6 +521,7 @@ static int i801_block_transaction_by_blo > } > > data->block[0] = len; > + inb_p(SMBHSTCNT(priv)); /* reset the data buffer index */ > for (i = 0; i < len; i++) > data->block[i + 1] = inb_p(SMBBLKDAT(priv)); > } >
Hi Jean, On Wed, Feb 14, 2024 at 03:59:39PM +0100, Jean Delvare wrote: > According to the Intel datasheets, software must reset the block > buffer index twice for block process call transactions: once before > writing the outgoing data to the buffer, and once again before > reading the incoming data from the buffer. > > The driver is currently missing the second reset, causing the wrong > portion of the block buffer to be read. > > Signed-off-by: Jean Delvare <jdelvare@suse.de> > Reported-by: Piotr Zakowski <piotr.zakowski@intel.com> > Closes: https://lore.kernel.org/linux-i2c/20240213120553.7b0ab120@endymion.delvare/ > Fixes: 315cd67c9453 ("i2c: i801: Add Block Write-Block Read Process Call support") > --- applied to i2c/i2c-host-fixes. b4 ty failed to send the thank you letter because there are some addresses that were not accepted (Kozlowski@imap1.dmz-prg2.suse.org?) Anyway, it's applied :-) Andi
On Wed, Feb 14, 2024 at 10:44:17PM +0100, Andi Shyti wrote: > On Wed, Feb 14, 2024 at 03:59:39PM +0100, Jean Delvare wrote: > > According to the Intel datasheets, software must reset the block > > buffer index twice for block process call transactions: once before > > writing the outgoing data to the buffer, and once again before > > reading the incoming data from the buffer. > > > > The driver is currently missing the second reset, causing the wrong > > portion of the block buffer to be read. > > > > Signed-off-by: Jean Delvare <jdelvare@suse.de> > > Reported-by: Piotr Zakowski <piotr.zakowski@intel.com> > > Closes: https://lore.kernel.org/linux-i2c/20240213120553.7b0ab120@endymion.delvare/ > > Fixes: 315cd67c9453 ("i2c: i801: Add Block Write-Block Read Process Call support") > > --- > > applied to i2c/i2c-host-fixes. > > b4 ty failed to send the thank you letter because there are some > addresses that were not accepted > (Kozlowski@imap1.dmz-prg2.suse.org?) > > Anyway, it's applied :-) BTW, thanks for the fast action on this bug report! Andi
Hi Andi, On Wed, 14 Feb 2024 22:44:12 +0100, Andi Shyti wrote: > b4 ty failed to send the thank you letter because there are some > addresses that were not accepted > (Kozlowski@imap1.dmz-prg2.suse.org?) Sorry for the mess, this seems to be a bug in my email client (Claws Mail 4.1.1) when the recipient names are quoted and/or include a comma. Ideally my SMTP server should have rejected the message as invalid, but it tried its best to still process it. I'll report the issue to the Claws Mail developers.
--- linux-6.6.orig/drivers/i2c/busses/i2c-i801.c +++ linux-6.6/drivers/i2c/busses/i2c-i801.c @@ -500,11 +500,10 @@ static int i801_block_transaction_by_blo /* Set block buffer mode */ outb_p(inb_p(SMBAUXCTL(priv)) | SMBAUXCTL_E32B, SMBAUXCTL(priv)); - inb_p(SMBHSTCNT(priv)); /* reset the data buffer index */ - if (read_write == I2C_SMBUS_WRITE) { len = data->block[0]; outb_p(len, SMBHSTDAT0(priv)); + inb_p(SMBHSTCNT(priv)); /* reset the data buffer index */ for (i = 0; i < len; i++) outb_p(data->block[i+1], SMBBLKDAT(priv)); } @@ -522,6 +521,7 @@ static int i801_block_transaction_by_blo } data->block[0] = len; + inb_p(SMBHSTCNT(priv)); /* reset the data buffer index */ for (i = 0; i < len; i++) data->block[i + 1] = inb_p(SMBBLKDAT(priv)); }
According to the Intel datasheets, software must reset the block buffer index twice for block process call transactions: once before writing the outgoing data to the buffer, and once again before reading the incoming data from the buffer. The driver is currently missing the second reset, causing the wrong portion of the block buffer to be read. Signed-off-by: Jean Delvare <jdelvare@suse.de> Reported-by: Piotr Zakowski <piotr.zakowski@intel.com> Closes: https://lore.kernel.org/linux-i2c/20240213120553.7b0ab120@endymion.delvare/ Fixes: 315cd67c9453 ("i2c: i801: Add Block Write-Block Read Process Call support") --- Piotr, does this change make your tests succeed? Alexander, I don't suppose you still have access to the hardware on which you were using block process call transactions back in 2019, but maybe you remember having to do the same change to make it work? drivers/i2c/busses/i2c-i801.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)