[PATCH] i2c: cpm: Fix i2c_ram structure

[Vincent Nicolas]

> ________________________________________
> From: Christophe Leroy <christophe.leroy at csgroup.eu>
> Sent: Wednesday, 23 September 2020 10:01
> To: Vincent Nicolas; jochen at scram.de
> Cc: linuxppc-dev at lists.ozlabs.org; linux-i2c at 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 at csgroup.eu>
>> Sent: Tuesday, 22 September 2020 14:38
>> To: Vincent Nicolas <Nicolas.Vincent at vossloh.com>; jochen at scram.de <jochen at scram.de>
>> Cc: linuxppc-dev at lists.ozlabs.org <linuxppc-dev at lists.ozlabs.org>; linux-i2c at vger.kernel.org <linux-i2c at vger.kernel.org>
>> Subject: Re: [PATCH] i2c: cpm: Fix i2c_ram structure
>>
>>
>>
>> Le 22/09/2020 à 11:04, nico.vince at gmail.com a écrit :
>>> From: Nicolas VINCENT <nicolas.vincent at 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 at 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

>
>
> I think you are right on the fact that it doesn't seem to be an issue.
> Nevertheless, that's confusing.
>
> What I would suggest is to leave res2[2] as is, and add something like:
>
>         /* The following elements are only for CPM2 */
>         char res3[4];   /* Reserved */
>         uint sdmatmp;   /* Internal */

I'll repost the patch like this then.

>
>
> Other solution (not sure that's the best solution thought) would be to
> do as in spi-fsl-cpm: use iic_t structure from asm/cpm1.h when
> CONFIG_CPM1 is selected and use iic_t from asm/cpm2.h when CONFIG_CPM2
> is selected, taking into account that CONFIG_CPM1 and CONFIG_CPM2 are
> mutually exclusive at the time being.

Unless someone argues for this solution I will go with the first one.

Thank again for your time and quick responses.

Nicolas.