From mboxrd@z Thu Jan  1 00:00:00 1970
From: Albert ARIBAUD <albert.u.boot@aribaud.net>
Date: Thu, 27 Jun 2013 14:06:00 +0200
Subject: [U-Boot] [PATCH] usb: fix unaligned access in device_qual()
In-Reply-To: <51CC1350.3030505@denx.de>
References: <1372320297-16275-1-git-send-email-hs@denx.de>
	<20130627121136.0411b3f1@lilith> <51CC1350.3030505@denx.de>
Message-ID: <20130627140600.32d9182e@lilith>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Hi Stefan,

On Thu, 27 Jun 2013 12:26:24 +0200, Stefan Roese <sr@denx.de> wrote:

> Hi Albert,
> 
> On 06/27/2013 12:11 PM, Albert ARIBAUD wrote:
> >> while playing with dfu, I tapped in an unaligned access
> >> when doing on the host side a "lsusb -d [vendornr]: -v"
> >> I get on the board:
> >>
> >> GADGET DRIVER: usb_dnl_dfu
> >> data abort
> >>
> >>     MAYBE you should read doc/README.arm-unaligned-accesses
> >>
> >> pc : [<8ff71db8>]          lr : [<8ff75aec>]
> >> sp : 8ef40d18  ip : 00000005     fp : 00000000
> >> r10: 00000000  r9 : 47401410     r8 : 8ef40f38
> >> r7 : 8ef4aae8  r6 : 0000000a     r5 : 8ef4ab28  r4 : 8ef4ab80
> >> r3 : 0000000a  r2 : 00000006     r1 : 00000006  r0 : 8ef4aae8
> >> Flags: Nzcv  IRQs off  FIQs on  Mode SVC_32
> >> Resetting CPU ...
> >>
> >> reason is that in the "struct usb_composite_dev" the
> >> "struct usb_device_descriptor desc;" is on an odd address,
> >> and this struct gets accessed in
> >> drivers/usb/gadget/composite.c device_qual()
> >>
> >> Fix it, by align this var "struct desc" fix to an aligned
> >> address.

Slow thinking: if the issue is "odd address", why align using
cache-related constant? Alignment should just be to even address --
maybe even 4-byte alignment, to be checked.

> > Please keep the commit message to a minimum -- what is wrong and how it
> > is fixed -- and move the rest (context and additional details) after
> > the commit message separator ('---' below).
> 
> I personally find this expanded description quite helpful. Everything
> below the "---" line is removed from the git history. So +1 for this
> expanded description from me.

I am not against an expanded explanation of the why and how of the
patch; but here, the console output copy, as well as the anecdote about
the circumstances which led to discovering the bug, only add noise to
the commit -- they're good for discussion the patch, hence my
suggestion. The commit message is just as informative when spelled out
thus:

----- 8< -----
usb: fix unaligned access in device_qual()

File include/linux/usb/composite.h contains struct usb_composite_dev in
which field 'desc' is misaligned, causing an alignment data abort in
function device_qual() in file drivers/usb/gadget/composite.c.

Fixed by forcing 'desc' field alignment to {even,4-byte} address.

[...]
---
When doing on the host side a "lsusb -d [vendornr]: -v"
I get on the board:

GADGET DRIVER: usb_dnl_dfu
data abort

    MAYBE you should read doc/README.arm-unaligned-accesses

pc : [<8ff71db8>]          lr : [<8ff75aec>]
sp : 8ef40d18  ip : 00000005     fp : 00000000
r10: 00000000  r9 : 47401410     r8 : 8ef40f38
r7 : 8ef4aae8  r6 : 0000000a     r5 : 8ef4ab28  r4 : 8ef4ab80
r3 : 0000000a  r2 : 00000006     r1 : 00000006  r0 : 8ef4aae8
Flags: Nzcv  IRQs off  FIQs on  Mode SVC_32
Resetting CPU ...

[...]
----- 8< -----

> Thanks,
> Stefan

Amicalement,
-- 
Albert.