Re: [PATCH] i2c: cpm: Fix i2c_ram structure

[Christophe Leroy <christophe.leroy@csgroup.eu>]

Le 23/09/2020 à 14:55, Vincent Nicolas a écrit :
> 
> 
>> ________________________________________
>> From: Christophe Leroy <christophe.leroy@csgroup.eu>
>> Sent: Wednesday, 23 September 2020 10:01
>> To: Vincent Nicolas; jochen@scram.de
>> Cc: linuxppc-dev@lists.ozlabs.org; linux-i2c@vger.kernel.org
>> Subject: Re: [PATCH] i2c: cpm: Fix i2c_ram structure
>>
>>
>>
>> Le 23/09/2020 à 09:18, Vincent Nicolas a écrit :
>>>
>>>
>>>
>>> From: Christophe Leroy <christophe.leroy@csgroup.eu>
>>> Sent: Tuesday, 22 September 2020 14:38
>>> To: Vincent Nicolas <Nicolas.Vincent@vossloh.com>; jochen@scram.de <jochen@scram.de>
>>> Cc: linuxppc-dev@lists.ozlabs.org <linuxppc-dev@lists.ozlabs.org>; linux-i2c@vger.kernel.org <linux-i2c@vger.kernel.org>
>>> Subject: Re: [PATCH] i2c: cpm: Fix i2c_ram structure
>>>
>>>
>>>
>>> Le 22/09/2020 à 11:04, nico.vince@gmail.com a écrit :
>>>> From: Nicolas VINCENT <nicolas.vincent@vossloh.com>
>>>>
>>>> the i2c_ram structure is missing the sdmatmp field mentionned in
>>>> datasheet for MPC8272 at paragraph 36.5. With this field missing, the
>>>> hardware would write past the allocated memory done through
>>>> cpm_muram_alloc for the i2c_ram structure and land in memory allocated
>>>> for the buffers descriptors corrupting the cbd_bufaddr field. Since this
>>>> field is only set during setup(), the first i2c transaction would work
>>>> and the following would send data read from an arbitrary memory
>>>> location.
>>>>
>>>> Signed-off-by: Nicolas VINCENT <nicolas.vincent@vossloh.com>
>>>> ---
>>>>      drivers/i2c/busses/i2c-cpm.c | 3 ++-
>>>>      1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/i2c/busses/i2c-cpm.c b/drivers/i2c/busses/i2c-cpm.c
>>>> index 1213e1932ccb..c5700addbf65 100644
>>>> --- a/drivers/i2c/busses/i2c-cpm.c
>>>> +++ b/drivers/i2c/busses/i2c-cpm.c
>>>> @@ -64,7 +64,8 @@ struct i2c_ram {
>>>>           uint    txtmp;          /* Internal */
>>>>           char    res1[4];        /* Reserved */
>>>>           ushort  rpbase;         /* Relocation pointer */
>>>> -     char    res2[2];        /* Reserved */
>>>> +     char    res2[6];        /* Reserved */
>>>> +     uint    sdmatmp;        /* Internal */
>>>
>>> On CPM1, I2C param RAM has size 0x30 (offset 0x1c80-0x1caf)
>>>
>>> Your change overlaps the miscellaneous area that contains CP Microcode
>>> Revision Number, ref MPC885 Reference Manual §18.7.3
>>>
>>> As far as I understand the mpc885 contains in the dts the compatible=fsl,cpm1-i2c which is used in cpm-i2c.c to either determine the address of the i2c_ram structure (cpm1), or dynamically allocate it with cpm_muram_alloc (cpm2).
>>> In the first case the structure will indeed overlaps with the miscellaneous section but since the sdmatmp is only used by cpm2 hardware it shall not be an issue.
>>>
>>> Please, let me know if I am mistaken. If the patch cannot be accepted as is, I would gladly accept pointers on how to address this kind of issue.
>>
>>
>> Please use a mail client that properly sets the > in front of
>> original/answered text. Here your mailer has mixed you text and mine,
>> that's unusable on the long term.
> 
> I changed my configuration, please let me know if there are still issues

Yes it is OK now.

Christophe