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