From: Christophe Leroy <christophe.leroy@csgroup.eu>
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
Date: Wed, 23 Sep 2020 10:01:03 +0200 [thread overview]
Message-ID: <2ecfe18a-61f6-bb0e-22c5-b7ab79a77d03@csgroup.eu> (raw)
In-Reply-To: <PR3P193MB0731945473A9F251C7F37608F1380@PR3P193MB0731.EURP193.PROD.OUTLOOK.COM>
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
next prev parent reply other threads:[~2020-09-23 8:01 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-22 9:04 [PATCH] i2c: cpm: Fix i2c_ram structure nico.vince
2020-09-22 11:50 ` Joakim Tjernlund
2020-09-22 12:38 ` Christophe Leroy
2020-09-23 7:18 ` Vincent Nicolas
2020-09-23 8:01 ` Christophe Leroy [this message]
2020-09-23 12:55 ` Vincent Nicolas
2020-09-23 13:04 ` Christophe Leroy
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=2ecfe18a-61f6-bb0e-22c5-b7ab79a77d03@csgroup.eu \
--to=christophe.leroy@csgroup.eu \
--cc=Nicolas.Vincent@vossloh.com \
--cc=jochen@scram.de \
--cc=linux-i2c@vger.kernel.org \
--cc=linuxppc-dev@lists.ozlabs.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).