[U-Boot] [PATCH] usb: fix unaligned access in device_qual()
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.
Signed-off-by: Heiko Schocher hs@denx.de Cc: Marek Vasut marek.vasut@gmail.com Cc: Samuel Egli samuel.egli@siemens.com --- include/linux/usb/composite.h | 2 +- 1 Datei geändert, 1 Zeile hinzugefügt(+), 1 Zeile entfernt(-)
diff --git a/include/linux/usb/composite.h b/include/linux/usb/composite.h index 53cb095..4f76f88 100644 --- a/include/linux/usb/composite.h +++ b/include/linux/usb/composite.h @@ -331,7 +331,7 @@ struct usb_composite_dev { /* private: */ /* internals */ unsigned int suspended:1; - struct usb_device_descriptor desc; + struct usb_device_descriptor __aligned(CONFIG_SYS_CACHELINE_SIZE) desc; struct list_head configs; struct usb_composite_driver *driver; u8 next_string_id;
Hi Heiko,
On Thu, 27 Jun 2013 10:04:57 +0200, Heiko Schocher hs@denx.de 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.
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).
Signed-off-by: Heiko Schocher hs@denx.de Cc: Marek Vasut marek.vasut@gmail.com Cc: Samuel Egli samuel.egli@siemens.com
include/linux/usb/composite.h | 2 +- 1 Datei geändert, 1 Zeile hinzugefügt(+), 1 Zeile entfernt(-)
diff --git a/include/linux/usb/composite.h b/include/linux/usb/composite.h index 53cb095..4f76f88 100644 --- a/include/linux/usb/composite.h +++ b/include/linux/usb/composite.h @@ -331,7 +331,7 @@ struct usb_composite_dev { /* private: */ /* internals */ unsigned int suspended:1;
- struct usb_device_descriptor desc;
- struct usb_device_descriptor __aligned(CONFIG_SYS_CACHELINE_SIZE) desc; struct list_head configs; struct usb_composite_driver *driver; u8 next_string_id;
Amicalement,
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.
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.
Thanks, Stefan
Dear Stefan Roese,
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.
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.
Agreed
Best regards, Marek Vasut
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,
Hi Marek,
On Thu, 27 Jun 2013 13:26:39 +0200, Marek Vasut marex@denx.de wrote:
Dear Heiko Schocher,
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:
Applied, thanks
Now we have console log output in commit messages. :(
Amicalement,
Hello Albert,
Hi Marek,
On Thu, 27 Jun 2013 13:26:39 +0200, Marek Vasut marex@denx.de wrote:
Dear Heiko Schocher,
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:
Applied, thanks
Now we have console log output in commit messages. :(
Don't be sad, I will buy you a tartelette ;)
Best regards, Marek Vasut
Hi Marek,
On Thu, 27 Jun 2013 15:23:33 +0200, Marek Vasut marex@denx.de wrote:
Hello Albert,
Hi Marek,
On Thu, 27 Jun 2013 13:26:39 +0200, Marek Vasut marex@denx.de wrote:
Dear Heiko Schocher,
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:
Applied, thanks
Now we have console log output in commit messages. :(
Don't be sad, I will buy you a tartelette ;)
Actually the sad part is that the patch in itself is bad: the actual alignment boundary should have been 16 bit, not one cacheline. Also, the issue could / should have been solved by reordering the fields rather than using an attribute.
And I'm not going to eat a tartelette when I'm about 30 minutes from going to a restaurant (admittedly small, but undoubtedly near).
Best regards, Marek Vasut
Amicalement,
Dear Albert ARIBAUD,
Hi Marek,
On Thu, 27 Jun 2013 15:23:33 +0200, Marek Vasut marex@denx.de wrote:
Hello Albert,
Hi Marek,
On Thu, 27 Jun 2013 13:26:39 +0200, Marek Vasut marex@denx.de wrote:
Dear Heiko Schocher,
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:
Applied, thanks
Now we have console log output in commit messages. :(
Don't be sad, I will buy you a tartelette ;)
Actually the sad part is that the patch in itself is bad: the actual alignment boundary should have been 16 bit, not one cacheline. Also, the issue could / should have been solved by reordering the fields rather than using an attribute.
Why 16 bit? I think cacheline alignment here makes sense if the descriptor is to be flush()'d from dcache.
And I'm not going to eat a tartelette when I'm about 30 minutes from going to a restaurant (admittedly small, but undoubtedly near).
You should have one to survive the journey ;-)
Best regards, Marek Vasut
Hi Marek,
On Wed, 3 Jul 2013 15:30:20 +0200, Marek Vasut marex@denx.de wrote:
Dear Albert ARIBAUD,
Hi Marek,
On Thu, 27 Jun 2013 15:23:33 +0200, Marek Vasut marex@denx.de wrote:
Hello Albert,
Hi Marek,
On Thu, 27 Jun 2013 13:26:39 +0200, Marek Vasut marex@denx.de wrote:
Dear Heiko Schocher,
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:
Applied, thanks
Now we have console log output in commit messages. :(
Don't be sad, I will buy you a tartelette ;)
Actually the sad part is that the patch in itself is bad: the actual alignment boundary should have been 16 bit, not one cacheline. Also, the issue could / should have been solved by reordering the fields rather than using an attribute.
Why 16 bit? I think cacheline alignment here makes sense if the descriptor is to be flush()'d from dcache.
If it is then it should be moved out of the struct it currently lives in, in order to avoid a big alignment gap, and replaced with a pointer. Also, the commit message would then be wrong...
And I'm not going to eat a tartelette when I'm about 30 minutes from going to a restaurant (admittedly small, but undoubtedly near).
You should have one to survive the journey ;-)
I'm more addicted to the raw stuff. No one should need anything beyond 90+%-cocoa chocolate.
Best regards, Marek Vasut
Amicalement,
Dear Albert ARIBAUD,
Hi Marek,
On Wed, 3 Jul 2013 15:30:20 +0200, Marek Vasut marex@denx.de wrote:
Dear Albert ARIBAUD,
Hi Marek,
On Thu, 27 Jun 2013 15:23:33 +0200, Marek Vasut marex@denx.de wrote:
Hello Albert,
Hi Marek,
On Thu, 27 Jun 2013 13:26:39 +0200, Marek Vasut marex@denx.de wrote:
Dear Heiko Schocher,
> 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: Applied, thanks
Now we have console log output in commit messages. :(
Don't be sad, I will buy you a tartelette ;)
Actually the sad part is that the patch in itself is bad: the actual alignment boundary should have been 16 bit, not one cacheline. Also, the issue could / should have been solved by reordering the fields rather than using an attribute.
Why 16 bit? I think cacheline alignment here makes sense if the descriptor is to be flush()'d from dcache.
If it is then it should be moved out of the struct it currently lives in, in order to avoid a big alignment gap, and replaced with a pointer. Also, the commit message would then be wrong...
Good point, Heiko, can you comment?
Best regards, Marek Vasut
participants (4)
-
Albert ARIBAUD -
Heiko Schocher -
Marek Vasut -
Stefan Roese