* [PATCH] i2c: cpm: Fix i2c_ram structure @ 2020-09-22 9:04 ` nico.vince 0 siblings, 0 replies; 8+ messages in thread From: nico.vince @ 2020-09-22 9:04 UTC (permalink / raw) To: jochen; +Cc: linuxppc-dev, linux-i2c, Nicolas VINCENT 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 */ }; #define I2COM_START 0x80 -- 2.17.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH] i2c: cpm: Fix i2c_ram structure @ 2020-09-22 9:04 ` nico.vince 0 siblings, 0 replies; 8+ messages in thread From: nico.vince @ 2020-09-22 9:04 UTC (permalink / raw) To: jochen; +Cc: Nicolas VINCENT, linuxppc-dev, linux-i2c 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 */ }; #define I2COM_START 0x80 -- 2.17.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] i2c: cpm: Fix i2c_ram structure 2020-09-22 9:04 ` nico.vince (?) @ 2020-09-22 11:50 ` Joakim Tjernlund -1 siblings, 0 replies; 8+ messages in thread From: Joakim Tjernlund @ 2020-09-22 11:50 UTC (permalink / raw) To: nicolas.vincent, jochen, christophe.leroy; +Cc: linuxppc-dev, linux-i2c 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] i2c: cpm: Fix i2c_ram structure 2020-09-22 9:04 ` nico.vince (?) (?) @ 2020-09-22 12:38 ` Christophe Leroy 2020-09-23 7:18 ` Vincent Nicolas -1 siblings, 1 reply; 8+ messages in thread From: Christophe Leroy @ 2020-09-22 12:38 UTC (permalink / raw) To: nicolas.vincent, jochen; +Cc: linuxppc-dev, linux-i2c 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] i2c: cpm: Fix i2c_ram structure 2020-09-22 12:38 ` Christophe Leroy @ 2020-09-23 7:18 ` Vincent Nicolas 2020-09-23 8:01 ` Christophe Leroy 0 siblings, 1 reply; 8+ messages in thread From: Vincent Nicolas @ 2020-09-23 7:18 UTC (permalink / raw) To: Christophe Leroy, jochen; +Cc: linuxppc-dev, linux-i2c 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] i2c: cpm: Fix i2c_ram structure 2020-09-23 7:18 ` Vincent Nicolas @ 2020-09-23 8:01 ` Christophe Leroy 2020-09-23 12:55 ` Vincent Nicolas 0 siblings, 1 reply; 8+ messages in thread From: Christophe Leroy @ 2020-09-23 8:01 UTC (permalink / raw) To: Vincent Nicolas, jochen; +Cc: linuxppc-dev, linux-i2c 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] i2c: cpm: Fix i2c_ram structure 2020-09-23 8:01 ` Christophe Leroy @ 2020-09-23 12:55 ` Vincent Nicolas 2020-09-23 13:04 ` Christophe Leroy 0 siblings, 1 reply; 8+ messages in thread From: Vincent Nicolas @ 2020-09-23 12:55 UTC (permalink / raw) To: Christophe Leroy, jochen; +Cc: linuxppc-dev, linux-i2c > ________________________________________ > 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] i2c: cpm: Fix i2c_ram structure 2020-09-23 12:55 ` Vincent Nicolas @ 2020-09-23 13:04 ` Christophe Leroy 0 siblings, 0 replies; 8+ messages in thread From: Christophe Leroy @ 2020-09-23 13:04 UTC (permalink / raw) To: Vincent Nicolas, jochen; +Cc: linuxppc-dev, linux-i2c 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-09-23 13:05 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-09-22 9:04 [PATCH] i2c: cpm: Fix i2c_ram structure nico.vince 2020-09-22 9:04 ` 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 2020-09-23 12:55 ` Vincent Nicolas 2020-09-23 13:04 ` Christophe Leroy
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.