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-accessespc : [<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,