| Message ID | 20200922090400.6282-1-nicolas.vincent@vossloh.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | i2c: cpm: Fix i2c_ram structure | expand |
On Tue, 2020-09-22 at 11:04 +0200, nico.vince@gmail.com wrote: > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe. > > > 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 */ > }; Not sure if this will fit on 8xx CPUs, Leroy ? Jocke
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 > }; > > #define I2COM_START 0x80 > Christophe
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
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
> ________________________________________ > From: Christophe Leroy <christophe.leroy@csgroup.eu> > Sent: Wednesday, 23 September 2020 10:01 > To: Vincent Nicolas; jochen@scram.de > Cc: linuxppc-dev@lists.ozlabs.org; linux-i2c@vger.kernel.org > Subject: Re: [PATCH] i2c: cpm: Fix i2c_ram structure > > > > 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 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.
Le 23/09/2020 à 14:55, Vincent Nicolas a écrit : > > >> ________________________________________ >> From: Christophe Leroy <christophe.leroy@csgroup.eu> >> Sent: Wednesday, 23 September 2020 10:01 >> To: Vincent Nicolas; jochen@scram.de >> Cc: linuxppc-dev@lists.ozlabs.org; linux-i2c@vger.kernel.org >> Subject: Re: [PATCH] i2c: cpm: Fix i2c_ram structure >> >> >> >> 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 changed my configuration, please let me know if there are still issues Yes it is OK now. Christophe
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 */ }; #define I2COM_START 0x80