usb: fix unaligned access in device_qual()

PATCH New Heiko Schocher 4 participants 12 messages Jun 27, 2013 – Jul 8, 2013 USB

Share & tools

Heiko Schocher

author

2013-06-27 08:04 UTC #0 lore raw

PATCH 1/1 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 <[email protected]>

Cc Marek Vasut <[email protected]>

Cc Samuel Egli <[email protected]>

---
 include/linux/usb/composite.h | 2 +-

1 Datei ge?ndert, 1 Zeile hinzugef?gt(+), 1 Zeile entfernt(-)

include/linux/usb/composite.h +1 -1

index 53cb095..4f76f88 100644
@@ -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;

Signature · 1 line

1.7.11.7

Albert ARIBAUD

2013-06-27 10:11 UTC #1 lore raw

PATCH 1/1 usb: fix unaligned access in device_qual()

Hi Heiko,

Quoted 17 lines · 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:
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 ...

Quoted 7 lines

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).

Quoted 12 lines

Signed-off-by Heiko Schocher <[email protected]>
Cc Marek Vasut <[email protected]>
Cc Samuel Egli <[email protected]>
---
 include/linux/usb/composite.h | 2 +-
1 Datei ge?ndert, 1 Zeile hinzugef?gt(+), 1 Zeile entfernt(-)
include/linux/usb/composite.h +0 -0
index 53cb095..4f76f88 100644

include/linux/usb/composite.h struct usb_composite_dev { · line 337

Quoted 8 lines

include/linux/usb/composite.h +1 -1

@@ -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,

Signature · 1 line

Albert.

Stefan Roese

2013-06-27 10:26 UTC #2 lore raw

PATCH 1/1 usb: fix unaligned access in device_qual()

Hi Albert,

Quoted 22 lines · Albert ARIBAUD

Quoted 22 lines
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()

Quoted 6 lines

Quoted 2 lines
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

Marek Vasut

2013-06-27 11:25 UTC #3 lore raw

PATCH 1/1 usb: fix unaligned access in device_qual()

Dear Stefan Roese,

Quoted 28 lines

Hi Albert,
Quoted 25 lines · Albert ARIBAUD
Quoted 24 lines
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.

Quoted 7 lines

Quoted 3 lines
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

Albert ARIBAUD

2013-06-27 12:06 UTC #4 lore raw

PATCH 1/1 usb: fix unaligned access in device_qual()

In reply to Stefan Roese

Hi Stefan,

Quoted 27 lines · Stefan Roese

Hi Albert,
Quoted 24 lines · Albert ARIBAUD
Quoted 24 lines
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.

Quoted 7 lines

Quoted 3 lines
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>]

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

Quoted 2 lines

Thanks,
Stefan

Amicalement,

Signature · 1 line

Albert.

Marek Vasut

Applied

2013-06-27 11:26 UTC #5 lore raw

PATCH 1/1 usb: fix unaligned access in device_qual()

In reply to Heiko Schocher

Dear Heiko Schocher,

Quoted 3 lines

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

btw you can send this stuff to my @denx address, it's much more convenient for me.

Best regards,
Marek Vasut

Albert ARIBAUD

2013-06-27 12:06 UTC #6 lore raw

PATCH 1/1 usb: fix unaligned access in device_qual()

Hi Marek,

Quoted 7 lines · Marek Vasut

Dear Heiko Schocher,
Quoted 3 lines
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,

Signature · 1 line

Albert.

Marek Vasut

2013-06-27 13:23 UTC #7 lore raw

PATCH 1/1 usb: fix unaligned access in device_qual()

Hello Albert,

Quoted 5 lines

Hi Marek,
Quoted 2 lines · Marek Vasut
Dear Heiko Schocher,

Quoted 7 lines

Quoted 5 lines
Quoted 4 lines
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

Albert ARIBAUD

2013-06-27 16:52 UTC #8 lore raw

PATCH 1/1 usb: fix unaligned access in device_qual()

Hi Marek,

Quoted 10 lines · Marek Vasut

Hello Albert,
Quoted 8 lines
Hi Marek,
Quoted 5 lines · Marek Vasut
Dear Heiko Schocher,
Quoted 2 lines
while playing with dfu, I tapped in an unaligned access when doing on the host side a "lsusb -d [vendornr]: -v"

Quoted 6 lines

Quoted 4 lines
Quoted 2 lines
Quoted 1 line
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).

Quoted 2 lines

Best regards,
Marek Vasut

Amicalement,

Signature · 1 line

Albert.

Marek Vasut

2013-07-03 13:30 UTC #9 lore raw

PATCH 1/1 usb: fix unaligned access in device_qual()

Dear Albert ARIBAUD,

Quoted 16 lines

Hi Marek,
Quoted 13 lines · Marek Vasut
Hello Albert,
Quoted 11 lines
Hi Marek,
Quoted 7 lines · Marek Vasut
Dear Heiko Schocher,
Quoted 4 lines
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

Quoted 8 lines

Quoted 3 lines
Quoted 1 line
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.

Quoted 2 lines

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

Albert ARIBAUD

2013-07-04 11:50 UTC #10 lore raw

PATCH 1/1 usb: fix unaligned access in device_qual()

Hi Marek,

Quoted 22 lines · Marek Vasut

Dear Albert ARIBAUD,
Quoted 20 lines
Hi Marek,
Quoted 16 lines · Marek Vasut
Hello Albert,
Quoted 5 lines
Hi Marek,
Quoted 2 lines · Marek Vasut
Dear Heiko Schocher,
Quoted 7 lines
Quoted 5 lines
Quoted 4 lines
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 ;)

Quoted 7 lines

Quoted 4 lines
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...

Quoted 4 lines

Quoted 2 lines
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.

Quoted 2 lines

Best regards,
Marek Vasut

Amicalement,

Signature · 1 line

Albert.

Marek Vasut

2013-07-08 13:06 UTC #11 lore raw

PATCH 1/1 usb: fix unaligned access in device_qual()

Dear Albert ARIBAUD,

Quoted 30 lines

Hi Marek,
Quoted 27 lines · Marek Vasut
Dear Albert ARIBAUD,
Quoted 24 lines
Hi Marek,
Quoted 10 lines · Marek Vasut
Hello Albert,
Quoted 8 lines
Hi Marek,
Quoted 5 lines · Marek Vasut
Dear Heiko Schocher,
Quoted 2 lines
while playing with dfu, I tapped in an unaligned access when doing on the host side a "lsusb -d [vendornr]: -v"
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
Quoted 1 line
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.

Quoted 6 lines

Quoted 2 lines
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