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

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

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 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 */


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.

Christophe