Patchwork [U-Boot] input: fix unaligned access in key_matrix_decode_fdt()

login
register
mail settings
Submitter Stephen Warren
Date May 22, 2013, 6:48 p.m.
Message ID <1369248498-1799-1-git-send-email-swarren@wwwdotorg.org>
Download mbox | patch
Permalink /patch/245693/
State Accepted
Delegated to: Tom Rini
Headers show

Comments

Stephen Warren - May 22, 2013, 6:48 p.m.
From: Stephen Warren <swarren@nvidia.com>

Initialized character arrays on the stack can cause gcc to emit code that
performs unaligned accessess. Make the data static to avoid this.

Note that the unaligned accesses are made when copying data to prefix[] on
the stack from .rodata. By making the data static, the copy is completely
avoided. All explicitly written code treats the data as u8[], so will never
cause any unaligned accesses.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 drivers/input/key_matrix.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Simon Glass - May 26, 2013, 7:28 p.m.
On Wed, May 22, 2013 at 11:48 AM, Stephen Warren <swarren@wwwdotorg.org>wrote:

> From: Stephen Warren <swarren@nvidia.com>
>
> Initialized character arrays on the stack can cause gcc to emit code that
> performs unaligned accessess. Make the data static to avoid this.
>
> Note that the unaligned accesses are made when copying data to prefix[] on
> the stack from .rodata. By making the data static, the copy is completely
> avoided. All explicitly written code treats the data as u8[], so will never
> cause any unaligned accesses.
>
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
>

Acked-by: Simon Glass <sjg@chromium.org>

Thanks for fixing.

I hit this with gcc 4.7. I wonder if previous revisions would not make this
assumption?

Another problem I have is that the 'linux' in 'linux,keymap' in the device
compile turns into '1' since gcc predefines 'linux' to 1:

I think I'm going to add a -Ulinux to dts/Makefile.

Regards.
Simon
Wolfgang Denk - May 26, 2013, 8:55 p.m.
Dear Simon Glass,

In message <CAPnjgZ1_M-7EgGade-p-TLrYnPNNXGyh8zh32w8a2+2qGVYuow@mail.gmail.com> you wrote:
>
> Another problem I have is that the 'linux' in 'linux,keymap' in the device
> compile turns into '1' since gcc predefines 'linux' to 1:

Should this not be considered a GCC bug? After all, "linux" is not a
reserved identifier.  [Defining __linux, or __linux__ would be probab-
ly OK, but "linux" or "arm" are not - IMO.]

Best regards,

Wolfgang Denk
Simon Glass - May 26, 2013, 9:29 p.m.
Hi Wolfgang,

On Sun, May 26, 2013 at 1:55 PM, Wolfgang Denk <wd@denx.de> wrote:

> Dear Simon Glass,
>
> In message <
> CAPnjgZ1_M-7EgGade-p-TLrYnPNNXGyh8zh32w8a2+2qGVYuow@mail.gmail.com> you
> wrote:
> >
> > Another problem I have is that the 'linux' in 'linux,keymap' in the
> device
> > compile turns into '1' since gcc predefines 'linux' to 1:
>
> Should this not be considered a GCC bug? After all, "linux" is not a
> reserved identifier.  [Defining __linux, or __linux__ would be probab-
> ly OK, but "linux" or "arm" are not - IMO.]
>

It certainly surprised me, but if it is a bug, then it might be too late to
fix it, since that release is widespread.


>
> Best regards,
>
> Wolfgang Denk
>
> --
> DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
> Der Horizont vieler Menschen ist ein Kreis mit Radius Null --
> und das nennen sie ihren Standpunkt.
>

That was worth translating :-)

Regards,
Simon
Stephen Warren - May 28, 2013, 4:07 a.m.
On 05/26/2013 01:28 PM, Simon Glass wrote:
> 
> On Wed, May 22, 2013 at 11:48 AM, Stephen Warren <swarren@wwwdotorg.org
> <mailto:swarren@wwwdotorg.org>> wrote:
> 
>     From: Stephen Warren <swarren@nvidia.com <mailto:swarren@nvidia.com>>
> 
>     Initialized character arrays on the stack can cause gcc to emit code
>     that
>     performs unaligned accessess. Make the data static to avoid this.
> 
>     Note that the unaligned accesses are made when copying data to
>     prefix[] on
>     the stack from .rodata. By making the data static, the copy is
>     completely
>     avoided. All explicitly written code treats the data as u8[], so
>     will never
>     cause any unaligned accesses.
> 
>     Signed-off-by: Stephen Warren <swarren@nvidia.com
>     <mailto:swarren@nvidia.com>>
> 
> 
> Acked-by: Simon Glass <sjg@chromium.org <mailto:sjg@chromium.org>>
> 
> Thanks for fixing.
> 
> I hit this with gcc 4.7. I wonder if previous revisions would not make
> this assumption?

IIRC, gcc-4.7 introduces the emission of native unaligned accesses, and
it's been back-ported to Linaro gcc-4.6.

> Another problem I have is that the 'linux' in 'linux,keymap' in the
> device compile turns into '1' since gcc predefines 'linux' to 1:
> 
> I think I'm going to add a -Ulinux to dts/Makefile.

I forget the exact details, but if you check the Linux makefiles for
dtc+cpp, they don't suffer from this issue any more; it may have been
due to use of -x assembler-with-cpp. I do also have a bug filed
internally to NVIDIA to fix that, which is assigned to Tom. But, I'm
sure he'd be glad if you fixed it:-)
Stephen Warren - June 4, 2013, 7:29 p.m.
On 05/22/2013 12:48 PM, Stephen Warren wrote:
> From: Stephen Warren <swarren@nvidia.com>
> 
> Initialized character arrays on the stack can cause gcc to emit code that
> performs unaligned accessess. Make the data static to avoid this.
> 
> Note that the unaligned accesses are made when copying data to prefix[] on
> the stack from .rodata. By making the data static, the copy is completely
> avoided. All explicitly written code treats the data as u8[], so will never
> cause any unaligned accesses.

Tom, does this patch look good?

The discussion following it was unrelated to this patch, but rather
related to pre-processing of device-trees, so I don't think should
prevent this patch being merged.
Tom Rini - June 5, 2013, 12:34 p.m.
On Wed, May 22, 2013 at 08:48:18AM -0000, Stephen Warren wrote:

> From: Stephen Warren <swarren@nvidia.com>
> 
> Initialized character arrays on the stack can cause gcc to emit code that
> performs unaligned accessess. Make the data static to avoid this.
> 
> Note that the unaligned accesses are made when copying data to prefix[] on
> the stack from .rodata. By making the data static, the copy is completely
> avoided. All explicitly written code treats the data as u8[], so will never
> cause any unaligned accesses.
> 
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> Acked-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/master, thanks!
Tom Rini - June 6, 2013, 2:47 p.m.
On Wed, Jun 05, 2013 at 08:34:20AM -0400, Tom Rini wrote:
> On Wed, May 22, 2013 at 08:48:18AM -0000, Stephen Warren wrote:
> 
> > From: Stephen Warren <swarren@nvidia.com>
> > 
> > Initialized character arrays on the stack can cause gcc to emit code that
> > performs unaligned accessess. Make the data static to avoid this.
> > 
> > Note that the unaligned accesses are made when copying data to prefix[] on
> > the stack from .rodata. By making the data static, the copy is completely
> > avoided. All explicitly written code treats the data as u8[], so will never
> > cause any unaligned accesses.
> > 
> > Signed-off-by: Stephen Warren <swarren@nvidia.com>
> > Acked-by: Simon Glass <sjg@chromium.org>
> 
> Applied to u-boot/master, thanks!

Bah!  I see I applied v1 and not v2, I shall post and apply the delta
between them momentarily.

Patch

diff --git a/drivers/input/key_matrix.c b/drivers/input/key_matrix.c
index 946a186..c8b048e 100644
--- a/drivers/input/key_matrix.c
+++ b/drivers/input/key_matrix.c
@@ -158,7 +158,7 @@  int key_matrix_decode_fdt(struct key_matrix *config, const void *blob,
 			  int node)
 {
 	const struct fdt_property *prop;
-	const char prefix[] = "linux,";
+	static const char prefix[] = "linux,";
 	int plen = sizeof(prefix) - 1;
 	int offset;