* [PATCH] i2c: ismt: Separate I2C block read from SMBus block read
@ 2017-10-02 12:45 Pontus Andersson
2017-10-05 12:41 ` Wolfram Sang
0 siblings, 1 reply; 6+ messages in thread
From: Pontus Andersson @ 2017-10-02 12:45 UTC (permalink / raw)
To: Seth Heasley, Neil Horman, Wolfram Sang, linux-i2c, linux-kernel
Commit b6c159a9cb69 ("i2c: ismt: Don't duplicate the receive length for
block reads") broke I2C block reads. It aimed to fix normal SMBus block
read, but changed the correct behavior of I2C block read in the process.
According to Documentation/i2c/smbus-protocol, one vital difference
between normal SMBus block read and I2C block read is that there is no
byte count prefixed in the data sent on the wire:
SMBus Block Read: i2c_smbus_read_block_data()
S Addr Wr [A] Comm [A]
S Addr Rd [A] [Count] A [Data] A [Data] A ... A [Data] NA P
I2C Block Read: i2c_smbus_read_i2c_block_data()
S Addr Wr [A] Comm [A]
S Addr Rd [A] [Data] A [Data] A ... A [Data] NA P
Therefore the two transaction types need to be processed differently in
the driver by copying of the dma_buffer as done previously for the
I2C_SMBUS_I2C_BLOCK_DATA case.
Fixes: b6c159a9cb69 ("i2c: ismt: Don't duplicate the receive length for block reads")
Signed-off-by: Pontus Andersson <epontan@gmail.com>
Cc: stable@vger.kernel.org
---
drivers/i2c/busses/i2c-ismt.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/i2c/busses/i2c-ismt.c b/drivers/i2c/busses/i2c-ismt.c
index 22ffcb73c185..b51adffa4841 100644
--- a/drivers/i2c/busses/i2c-ismt.c
+++ b/drivers/i2c/busses/i2c-ismt.c
@@ -340,12 +340,15 @@ static int ismt_process_desc(const struct ismt_desc *desc,
data->word = dma_buffer[0] | (dma_buffer[1] << 8);
break;
case I2C_SMBUS_BLOCK_DATA:
- case I2C_SMBUS_I2C_BLOCK_DATA:
if (desc->rxbytes != dma_buffer[0] + 1)
return -EMSGSIZE;
memcpy(data->block, dma_buffer, desc->rxbytes);
break;
+ case I2C_SMBUS_I2C_BLOCK_DATA:
+ memcpy(&data->block[1], dma_buffer, desc->rxbytes);
+ data->block[0] = desc->rxbytes;
+ break;
}
return 0;
}
--
2.14.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] i2c: ismt: Separate I2C block read from SMBus block read
2017-10-02 12:45 [PATCH] i2c: ismt: Separate I2C block read from SMBus block read Pontus Andersson
@ 2017-10-05 12:41 ` Wolfram Sang
2017-10-05 14:13 ` Pontus Andersson
0 siblings, 1 reply; 6+ messages in thread
From: Wolfram Sang @ 2017-10-05 12:41 UTC (permalink / raw)
To: Pontus Andersson; +Cc: Seth Heasley, Neil Horman, linux-i2c, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1279 bytes --]
On Mon, Oct 02, 2017 at 02:45:19PM +0200, Pontus Andersson wrote:
> Commit b6c159a9cb69 ("i2c: ismt: Don't duplicate the receive length for
> block reads") broke I2C block reads. It aimed to fix normal SMBus block
> read, but changed the correct behavior of I2C block read in the process.
>
> According to Documentation/i2c/smbus-protocol, one vital difference
> between normal SMBus block read and I2C block read is that there is no
> byte count prefixed in the data sent on the wire:
>
> SMBus Block Read: i2c_smbus_read_block_data()
> S Addr Wr [A] Comm [A]
> S Addr Rd [A] [Count] A [Data] A [Data] A ... A [Data] NA P
>
> I2C Block Read: i2c_smbus_read_i2c_block_data()
> S Addr Wr [A] Comm [A]
> S Addr Rd [A] [Data] A [Data] A ... A [Data] NA P
>
> Therefore the two transaction types need to be processed differently in
> the driver by copying of the dma_buffer as done previously for the
> I2C_SMBUS_I2C_BLOCK_DATA case.
>
> Fixes: b6c159a9cb69 ("i2c: ismt: Don't duplicate the receive length for block reads")
> Signed-off-by: Pontus Andersson <epontan@gmail.com>
> Cc: stable@vger.kernel.org
Applied to for-current, thanks!
Might have been good to CC the patch author of the problematic commit,
too.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] i2c: ismt: Separate I2C block read from SMBus block read
2017-10-05 12:41 ` Wolfram Sang
@ 2017-10-05 14:13 ` Pontus Andersson
2017-10-06 22:54 ` Stephen Douthit
2017-10-07 8:14 ` Wolfram Sang
0 siblings, 2 replies; 6+ messages in thread
From: Pontus Andersson @ 2017-10-05 14:13 UTC (permalink / raw)
To: Wolfram Sang
Cc: Seth Heasley, Neil Horman, Stephen Douthit, linux-i2c, linux-kernel
On Thu, Oct 05, 2017 at 02:41:33PM +0200, Wolfram Sang wrote:
> On Mon, Oct 02, 2017 at 02:45:19PM +0200, Pontus Andersson wrote:
> > Commit b6c159a9cb69 ("i2c: ismt: Don't duplicate the receive length for
> > block reads") broke I2C block reads. It aimed to fix normal SMBus block
> > read, but changed the correct behavior of I2C block read in the process.
> >
> > According to Documentation/i2c/smbus-protocol, one vital difference
> > between normal SMBus block read and I2C block read is that there is no
> > byte count prefixed in the data sent on the wire:
> >
> > SMBus Block Read: i2c_smbus_read_block_data()
> > S Addr Wr [A] Comm [A]
> > S Addr Rd [A] [Count] A [Data] A [Data] A ... A [Data] NA P
> >
> > I2C Block Read: i2c_smbus_read_i2c_block_data()
> > S Addr Wr [A] Comm [A]
> > S Addr Rd [A] [Data] A [Data] A ... A [Data] NA P
> >
> > Therefore the two transaction types need to be processed differently in
> > the driver by copying of the dma_buffer as done previously for the
> > I2C_SMBUS_I2C_BLOCK_DATA case.
> >
> > Fixes: b6c159a9cb69 ("i2c: ismt: Don't duplicate the receive length for block reads")
> > Signed-off-by: Pontus Andersson <epontan@gmail.com>
> > Cc: stable@vger.kernel.org
>
> Applied to for-current, thanks!
Thank you! Is it safe to asume it will be backported to 4.4 stable
branch and upwards as the problematic commit did (once in mainline)?
> Might have been good to CC the patch author of the problematic commit,
> too.
Yes, of course! Stephen is CC now at least (kept the commit message
quoted at his convenience).
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] i2c: ismt: Separate I2C block read from SMBus block read
2017-10-05 14:13 ` Pontus Andersson
@ 2017-10-06 22:54 ` Stephen Douthit
2017-10-07 8:14 ` Wolfram Sang
1 sibling, 0 replies; 6+ messages in thread
From: Stephen Douthit @ 2017-10-06 22:54 UTC (permalink / raw)
To: Pontus Andersson, Wolfram Sang
Cc: Seth Heasley, Neil Horman, linux-i2c, linux-kernel
On 10/05/2017 10:13 AM, Pontus Andersson wrote:
> On Thu, Oct 05, 2017 at 02:41:33PM +0200, Wolfram Sang wrote:
>> On Mon, Oct 02, 2017 at 02:45:19PM +0200, Pontus Andersson wrote:
>>> Commit b6c159a9cb69 ("i2c: ismt: Don't duplicate the receive length for
>>> block reads") broke I2C block reads. It aimed to fix normal SMBus block
>>> read, but changed the correct behavior of I2C block read in the process.
>>>
>>> According to Documentation/i2c/smbus-protocol, one vital difference
>>> between normal SMBus block read and I2C block read is that there is no
>>> byte count prefixed in the data sent on the wire:
>>>
>>> SMBus Block Read: i2c_smbus_read_block_data()
>>> S Addr Wr [A] Comm [A]
>>> S Addr Rd [A] [Count] A [Data] A [Data] A ... A [Data] NA P
>>>
>>> I2C Block Read: i2c_smbus_read_i2c_block_data()
>>> S Addr Wr [A] Comm [A]
>>> S Addr Rd [A] [Data] A [Data] A ... A [Data] NA P
>>>
>>> Therefore the two transaction types need to be processed differently in
>>> the driver by copying of the dma_buffer as done previously for the
>>> I2C_SMBUS_I2C_BLOCK_DATA case.
>>>
>>> Fixes: b6c159a9cb69 ("i2c: ismt: Don't duplicate the receive length for block reads")
>>> Signed-off-by: Pontus Andersson <epontan@gmail.com>
>>> Cc: stable@vger.kernel.org
>>
>> Applied to for-current, thanks!
>
> Thank you! Is it safe to asume it will be backported to 4.4 stable
> branch and upwards as the problematic commit did (once in mainline)?
>
>> Might have been good to CC the patch author of the problematic commit,
>> too.
>
> Yes, of course! Stephen is CC now at least (kept the commit message
> quoted at his convenience).
Doh. Looks like our regression test gave a false pass on the I2C access
case. I'd guess it's probably doing a series of byte reads instead of a
block read and missing the I2C_SMBUS_I2C_BLOCK_DATA case entirely.
Unfortunately I can't find the board that was modded with more I2C
devices on the ismt bus to verify this right now. I'll check with Dan
on Monday to see if he knows where that board is stashed.
Apologies for moving the bug instead of fixing it, and FWIW I'll have an
update Monday.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] i2c: ismt: Separate I2C block read from SMBus block read
2017-10-05 14:13 ` Pontus Andersson
2017-10-06 22:54 ` Stephen Douthit
@ 2017-10-07 8:14 ` Wolfram Sang
[not found] ` <c9a1d377-1a72-a1fa-304a-48d593d48e6b@adiengineering.com>
1 sibling, 1 reply; 6+ messages in thread
From: Wolfram Sang @ 2017-10-07 8:14 UTC (permalink / raw)
To: Pontus Andersson
Cc: Seth Heasley, Neil Horman, Stephen Douthit, linux-i2c, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 491 bytes --]
> Thank you! Is it safe to asume it will be backported to 4.4 stable
> branch and upwards as the problematic commit did (once in mainline)?
Yes, it has a Fixes: and a stable tag. That should do.
> > Might have been good to CC the patch author of the problematic commit,
> > too.
>
> Yes, of course! Stephen is CC now at least (kept the commit message
> quoted at his convenience).
So, I'll wait for his tests and then send it to Linus ASAP. I think it
should be in rc5.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] i2c: ismt: Separate I2C block read from SMBus block read
[not found] ` <c9a1d377-1a72-a1fa-304a-48d593d48e6b@adiengineering.com>
@ 2017-10-09 17:18 ` Stephen Douthit
0 siblings, 0 replies; 6+ messages in thread
From: Stephen Douthit @ 2017-10-09 17:18 UTC (permalink / raw)
To: Wolfram Sang, Pontus Andersson
Cc: Seth Heasley, Neil Horman, linux-i2c, linux-kernel
> So, I'll wait for his tests and then send it to Linus ASAP. I think it
> should be in rc5.
I've been able to confirm the regression my patch introduced, and the
fix from Pontus.
Doing a block read of the SPD EEPROM with my patch everything is shifted
by a byte:
[root@localhost ~]# i2cdump -y 0 0x50 i
0 1 2 3 4 5 6 7 8 9 a b c d e f 0123456789abcdef
00: 13 0b 02 04 21 02 01 03 11 01 08 0a 00 fe 00 69 ????!???????.?.i
10: 78 69 30 69 11 18 81 20 08 3c 3c 00 f0 83 05 80 xi0i??? ?<<.????
20: 00 00 00 00 00 00 00 00 85 00 00 00 00 00 00 00 ........?.......
30: 00 00 00 00 00 00 00 00 00 00 00 0f 01 23 00 00 ...........??#..
40: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
50: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
70: 00 00 00 00 80 2c 00 00 00 00 00 00 00 a6 dc 39 ....?,.......??9
80: 4b 53 46 35 31 32 37 32 41 5a 2d 31 47 36 45 31 KSF51272AZ-1G6E1
90: 20 45 31 80 2c 00 00 00 00 00 00 00 00 00 00 00 E1?,...........
a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 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 92 ...............?
With the patch from Pontus the I2C block read works as expected:
[root@localhost ~]# i2cdump -y 0 0x50 i
0 1 2 3 4 5 6 7 8 9 a b c d e f 0123456789abcdef
00: 92 13 0b 02 04 21 02 01 03 11 01 08 0a 00 fe 00 ?????!???????.?.
10: 69 78 69 30 69 11 18 81 20 08 3c 3c 00 f0 83 05 ixi0i??? ?<<.???
20: 80 00 00 00 00 00 00 00 00 85 00 00 00 00 00 00 ?........?......
30: 00 00 00 00 00 00 00 00 00 00 00 00 0f 01 23 00 ............??#.
40: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
50: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
70: 00 00 00 00 00 80 2c 00 00 00 00 00 00 00 a6 dc .....?,.......??
80: 39 4b 53 46 35 31 32 37 32 41 5a 2d 31 47 36 45 9KSF51272AZ-1G6E
90: 31 20 45 31 80 2c 00 00 00 00 00 00 00 00 00 00 1 E1?,..........
a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
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 ................
The ipmi_ssif driver still works with this patch, so no issues for
SMBus block reads.
Tested-by: Stephen Douthit <stephend@adiengineering.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-10-09 17:18 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-02 12:45 [PATCH] i2c: ismt: Separate I2C block read from SMBus block read Pontus Andersson
2017-10-05 12:41 ` Wolfram Sang
2017-10-05 14:13 ` Pontus Andersson
2017-10-06 22:54 ` Stephen Douthit
2017-10-07 8:14 ` Wolfram Sang
[not found] ` <c9a1d377-1a72-a1fa-304a-48d593d48e6b@adiengineering.com>
2017-10-09 17:18 ` Stephen Douthit
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.