> ________________________________________
> From: Christophe Leroy <christophe.leroy@xxxxxxxxxx>
> Sent: Wednesday, 23 September 2020 10:01
> To: Vincent Nicolas; jochen@xxxxxxxx
> Cc: linuxppc-dev@xxxxxxxxxxxxxxxx; linux-i2c@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH] i2c: cpm: Fix i2c_ram structure
>
>
>
> Le 23/09/2020 à 09:18, Vincent Nicolas a écrit :
>>
>>
>>
>> From: Christophe Leroy <christophe.leroy@xxxxxxxxxx>
>> Sent: Tuesday, 22 September 2020 14:38
>> To: Vincent Nicolas <Nicolas.Vincent@xxxxxxxxxxx>; jochen@xxxxxxxx <jochen@xxxxxxxx>
>> Cc: linuxppc-dev@xxxxxxxxxxxxxxxx <linuxppc-dev@xxxxxxxxxxxxxxxx>; linux-i2c@xxxxxxxxxxxxxxx <linux-i2c@xxxxxxxxxxxxxxx>
>> Subject: Re: [PATCH] i2c: cpm: Fix i2c_ram structure
>>
>>
>>
>> Le 22/09/2020 à 11:04, nico.vince@xxxxxxxxx a écrit :
>>> From: Nicolas VINCENT <nicolas.vincent@xxxxxxxxxxx>
>>>
>>> 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@xxxxxxxxxxx>
>>> ---
>>> 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.
