| Message ID | 1388016086-20293-1-git-send-email-marex@denx.de |
|---|---|
| State | Accepted |
| Headers | show |
Dear Marek Vasut, Marek Vasut <marex <at> denx.de> writes: > +/* > + * Warning! This array is used with the memcpy_16() function, thus > + * it must be aligned to 2 bytes. GCC can make this array unaligned > + * as the array is made of unsigned char, which memcpy16() doesn't > + * like and will cause unaligned access. > + */ This is just really a nitpick but memcpy16() -> memcpy_16(). All the best, Rommel
On Thu, 2013-12-26 at 01:01 +0100, Marek Vasut wrote: > Fix unaligned access in OneNAND core. The problem is that the ffchars[] array > is an array of "unsigned char", but in onenand_write_ops_nolock() can be passed > to the memcpy_16() function. The memcpy_16() function will treat the buffer as > an array of "unsigned short", thus triggering unaligned access if the compiler > decided ffchars[] to be not aligned. > > I managed to trigger the problem with regular ELDK 5.4 GCC compiler. > > Signed-off-by: Marek Vasut <marex@denx.de> > Cc: Albert Aribaud <albert.u.boot@aribaud.net> > Cc: Scott Wood <scottwood@freescale.com> > Cc: Tom Rini <trini@ti.com> I don't see the onenand custodian on that CC list. -Scott
On Thursday, December 26, 2013 at 01:01:24 AM, Marek Vasut wrote: > Fix unaligned access in OneNAND core. The problem is that the ffchars[] > array is an array of "unsigned char", but in onenand_write_ops_nolock() > can be passed to the memcpy_16() function. The memcpy_16() function will > treat the buffer as an array of "unsigned short", thus triggering > unaligned access if the compiler decided ffchars[] to be not aligned. > > I managed to trigger the problem with regular ELDK 5.4 GCC compiler. > > Signed-off-by: Marek Vasut <marex@denx.de> > Cc: Albert Aribaud <albert.u.boot@aribaud.net> > Cc: Scott Wood <scottwood@freescale.com> > Cc: Tom Rini <trini@ti.com> > --- > drivers/mtd/onenand/onenand_base.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/mtd/onenand/onenand_base.c > b/drivers/mtd/onenand/onenand_base.c index 979e4af..e33e8d3 100644 > --- a/drivers/mtd/onenand/onenand_base.c > +++ b/drivers/mtd/onenand/onenand_base.c > @@ -91,7 +91,13 @@ static struct nand_ecclayout onenand_oob_32 = { > .oobfree = { {2, 3}, {14, 2}, {18, 3}, {30, 2} } > }; > > -static const unsigned char ffchars[] = { > +/* > + * Warning! This array is used with the memcpy_16() function, thus > + * it must be aligned to 2 bytes. GCC can make this array unaligned > + * as the array is made of unsigned char, which memcpy16() doesn't > + * like and will cause unaligned access. > + */ > +static const unsigned char __aligned(2) ffchars[] = { > 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, > 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, /* 16 */ > 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, Lukasz, can you please review this one? Best regards, Marek Vasut
Hi Marek, > On Thursday, December 26, 2013 at 01:01:24 AM, Marek Vasut wrote: > > Fix unaligned access in OneNAND core. The problem is that the > > ffchars[] array is an array of "unsigned char", but in > > onenand_write_ops_nolock() can be passed to the memcpy_16() > > function. The memcpy_16() function will treat the buffer as an > > array of "unsigned short", thus triggering unaligned access if the > > compiler decided ffchars[] to be not aligned. > > > > I managed to trigger the problem with regular ELDK 5.4 GCC compiler. > > > > Signed-off-by: Marek Vasut <marex@denx.de> > > Cc: Albert Aribaud <albert.u.boot@aribaud.net> > > Cc: Scott Wood <scottwood@freescale.com> > > Cc: Tom Rini <trini@ti.com> > > --- > > drivers/mtd/onenand/onenand_base.c | 8 +++++++- > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/mtd/onenand/onenand_base.c > > b/drivers/mtd/onenand/onenand_base.c index 979e4af..e33e8d3 100644 > > --- a/drivers/mtd/onenand/onenand_base.c > > +++ b/drivers/mtd/onenand/onenand_base.c > > @@ -91,7 +91,13 @@ static struct nand_ecclayout onenand_oob_32 = { > > .oobfree = { {2, 3}, {14, 2}, {18, 3}, {30, 2} } > > }; > > > > -static const unsigned char ffchars[] = { > > +/* > > + * Warning! This array is used with the memcpy_16() function, thus > > + * it must be aligned to 2 bytes. GCC can make this array unaligned > > + * as the array is made of unsigned char, which memcpy16() doesn't > > + * like and will cause unaligned access. > > + */ > > +static const unsigned char __aligned(2) ffchars[] = { > > 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, > > 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, /* > > 16 */ 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, > > Lukasz, can you please review this one? No problem, I will review the patch when I come back to the office (and be able to test it on the proper HW). > > Best regards, > Marek Vasut Best regards, Lukasz
On Saturday, December 28, 2013 at 05:06:28 PM, Lukasz Majewski wrote: > Hi Marek, > > > On Thursday, December 26, 2013 at 01:01:24 AM, Marek Vasut wrote: > > > Fix unaligned access in OneNAND core. The problem is that the > > > ffchars[] array is an array of "unsigned char", but in > > > onenand_write_ops_nolock() can be passed to the memcpy_16() > > > function. The memcpy_16() function will treat the buffer as an > > > array of "unsigned short", thus triggering unaligned access if the > > > compiler decided ffchars[] to be not aligned. > > > > > > I managed to trigger the problem with regular ELDK 5.4 GCC compiler. > > > > > > Signed-off-by: Marek Vasut <marex@denx.de> > > > Cc: Albert Aribaud <albert.u.boot@aribaud.net> > > > Cc: Scott Wood <scottwood@freescale.com> > > > Cc: Tom Rini <trini@ti.com> > > > --- > > > > > > drivers/mtd/onenand/onenand_base.c | 8 +++++++- > > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/mtd/onenand/onenand_base.c > > > b/drivers/mtd/onenand/onenand_base.c index 979e4af..e33e8d3 100644 > > > --- a/drivers/mtd/onenand/onenand_base.c > > > +++ b/drivers/mtd/onenand/onenand_base.c > > > @@ -91,7 +91,13 @@ static struct nand_ecclayout onenand_oob_32 = { > > > > > > .oobfree = { {2, 3}, {14, 2}, {18, 3}, {30, 2} } > > > > > > }; > > > > > > -static const unsigned char ffchars[] = { > > > +/* > > > + * Warning! This array is used with the memcpy_16() function, thus > > > + * it must be aligned to 2 bytes. GCC can make this array unaligned > > > + * as the array is made of unsigned char, which memcpy16() doesn't > > > + * like and will cause unaligned access. > > > + */ > > > +static const unsigned char __aligned(2) ffchars[] = { > > > > > > 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, > > > 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, /* > > > > > > 16 */ 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, > > > > Lukasz, can you please review this one? > > No problem, I will review the patch when I come back to the office > (and be able to test it on the proper HW). Thanks, I'd love to get this fix into 2014.01 . Maybe the real fix here would be to fix the memcpy_16() instead tho. Best regards, Marek Vasut
Hi Marek, > On Saturday, December 28, 2013 at 05:06:28 PM, Lukasz Majewski wrote: > > Hi Marek, > > > > > On Thursday, December 26, 2013 at 01:01:24 AM, Marek Vasut wrote: > > > > Fix unaligned access in OneNAND core. The problem is that the > > > > ffchars[] array is an array of "unsigned char", but in > > > > onenand_write_ops_nolock() can be passed to the memcpy_16() > > > > function. The memcpy_16() function will treat the buffer as an > > > > array of "unsigned short", thus triggering unaligned access if > > > > the compiler decided ffchars[] to be not aligned. > > > > > > > > I managed to trigger the problem with regular ELDK 5.4 GCC > > > > compiler. > > > > > > > > Signed-off-by: Marek Vasut <marex@denx.de> > > > > Cc: Albert Aribaud <albert.u.boot@aribaud.net> > > > > Cc: Scott Wood <scottwood@freescale.com> > > > > Cc: Tom Rini <trini@ti.com> > > > > --- > > > > > > > > drivers/mtd/onenand/onenand_base.c | 8 +++++++- > > > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/mtd/onenand/onenand_base.c > > > > b/drivers/mtd/onenand/onenand_base.c index 979e4af..e33e8d3 > > > > 100644 --- a/drivers/mtd/onenand/onenand_base.c > > > > +++ b/drivers/mtd/onenand/onenand_base.c > > > > @@ -91,7 +91,13 @@ static struct nand_ecclayout onenand_oob_32 > > > > = { > > > > > > > > .oobfree = { {2, 3}, {14, 2}, {18, 3}, {30, 2} } > > > > > > > > }; > > > > > > > > -static const unsigned char ffchars[] = { > > > > +/* > > > > + * Warning! This array is used with the memcpy_16() function, > > > > thus > > > > + * it must be aligned to 2 bytes. GCC can make this array > > > > unaligned > > > > + * as the array is made of unsigned char, which memcpy16() > > > > doesn't > > > > + * like and will cause unaligned access. > > > > + */ > > > > +static const unsigned char __aligned(2) ffchars[] = { > > > > > > > > 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, > > > > 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, > > > > 0xff, /* > > > > > > > > 16 */ 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, > > > > > > Lukasz, can you please review this one? > > > > No problem, I will review the patch when I come back to the office > > (and be able to test it on the proper HW). > > Thanks, I'd love to get this fix into 2014.01 . Maybe the real fix > here would be to fix the memcpy_16() instead tho. > I've tested it with eldk-5.4 toolchain on GONI device. It seems that no regression is caused by this patch. As a side note - this problem didn't show up for GONI board. Apparently I was lucky. I will scrutinize this code in respect of code alignment. Applied to u-boot-onenand. I will send pull request to Tom ASAP. > Best regards, > Marek Vasut
On Tuesday, December 31, 2013 at 11:43:48 AM, Lukasz Majewski wrote: > Hi Marek, > > > On Saturday, December 28, 2013 at 05:06:28 PM, Lukasz Majewski wrote: > > > Hi Marek, > > > > > > > On Thursday, December 26, 2013 at 01:01:24 AM, Marek Vasut wrote: > > > > > Fix unaligned access in OneNAND core. The problem is that the > > > > > ffchars[] array is an array of "unsigned char", but in > > > > > onenand_write_ops_nolock() can be passed to the memcpy_16() > > > > > function. The memcpy_16() function will treat the buffer as an > > > > > array of "unsigned short", thus triggering unaligned access if > > > > > the compiler decided ffchars[] to be not aligned. > > > > > > > > > > I managed to trigger the problem with regular ELDK 5.4 GCC > > > > > compiler. > > > > > > > > > > Signed-off-by: Marek Vasut <marex@denx.de> > > > > > Cc: Albert Aribaud <albert.u.boot@aribaud.net> > > > > > Cc: Scott Wood <scottwood@freescale.com> > > > > > Cc: Tom Rini <trini@ti.com> > > > > > --- > > > > > > > > > > drivers/mtd/onenand/onenand_base.c | 8 +++++++- > > > > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/drivers/mtd/onenand/onenand_base.c > > > > > b/drivers/mtd/onenand/onenand_base.c index 979e4af..e33e8d3 > > > > > 100644 --- a/drivers/mtd/onenand/onenand_base.c > > > > > +++ b/drivers/mtd/onenand/onenand_base.c > > > > > @@ -91,7 +91,13 @@ static struct nand_ecclayout onenand_oob_32 > > > > > = { > > > > > > > > > > .oobfree = { {2, 3}, {14, 2}, {18, 3}, {30, 2} } > > > > > > > > > > }; > > > > > > > > > > -static const unsigned char ffchars[] = { > > > > > +/* > > > > > + * Warning! This array is used with the memcpy_16() function, > > > > > thus > > > > > + * it must be aligned to 2 bytes. GCC can make this array > > > > > unaligned > > > > > + * as the array is made of unsigned char, which memcpy16() > > > > > doesn't > > > > > + * like and will cause unaligned access. > > > > > + */ > > > > > +static const unsigned char __aligned(2) ffchars[] = { > > > > > > > > > > 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, > > > > > 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, > > > > > > > > > > 0xff, /* > > > > > > > > > > 16 */ 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, > > > > > > > > Lukasz, can you please review this one? > > > > > > No problem, I will review the patch when I come back to the office > > > (and be able to test it on the proper HW). > > > > Thanks, I'd love to get this fix into 2014.01 . Maybe the real fix > > here would be to fix the memcpy_16() instead tho. > > I've tested it with eldk-5.4 toolchain on GONI device. It seems that no > regression is caused by this patch. > > As a side note - this problem didn't show up for GONI board. Apparently > I was lucky. > > I will scrutinize this code in respect of code alignment. > > Applied to u-boot-onenand. Thank you very much! Have a happy new year, guys ! Best regards, Marek Vasut
diff --git a/drivers/mtd/onenand/onenand_base.c b/drivers/mtd/onenand/onenand_base.c index 979e4af..e33e8d3 100644 --- a/drivers/mtd/onenand/onenand_base.c +++ b/drivers/mtd/onenand/onenand_base.c @@ -91,7 +91,13 @@ static struct nand_ecclayout onenand_oob_32 = { .oobfree = { {2, 3}, {14, 2}, {18, 3}, {30, 2} } }; -static const unsigned char ffchars[] = { +/* + * Warning! This array is used with the memcpy_16() function, thus + * it must be aligned to 2 bytes. GCC can make this array unaligned + * as the array is made of unsigned char, which memcpy16() doesn't + * like and will cause unaligned access. + */ +static const unsigned char __aligned(2) ffchars[] = { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, /* 16 */ 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
Fix unaligned access in OneNAND core. The problem is that the ffchars[] array is an array of "unsigned char", but in onenand_write_ops_nolock() can be passed to the memcpy_16() function. The memcpy_16() function will treat the buffer as an array of "unsigned short", thus triggering unaligned access if the compiler decided ffchars[] to be not aligned. I managed to trigger the problem with regular ELDK 5.4 GCC compiler. Signed-off-by: Marek Vasut <marex@denx.de> Cc: Albert Aribaud <albert.u.boot@aribaud.net> Cc: Scott Wood <scottwood@freescale.com> Cc: Tom Rini <trini@ti.com> --- drivers/mtd/onenand/onenand_base.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)