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

[Vincent Nicolas <Nicolas.Vincent@vossloh.com>]

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.

Thanks,
Nicolas.

>   };
>   
>   #define I2COM_START 0x80
> 


Christophe