[U-Boot] [PATCH] usb: fix unaligned access in device_qual()

[U-Boot] [PATCH] usb: fix unaligned access in device_qual()

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

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;
-- 
1.7.11.7

[U-Boot] [PATCH] usb: fix unaligned access in device_qual()

[Albert ARIBAUD]

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

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,
-- 
Albert.

[U-Boot] [PATCH] usb: fix unaligned access in device_qual()

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

[U-Boot] [PATCH] usb: fix unaligned access in device_qual()

[Marek Vasut]

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

[U-Boot] [PATCH] usb: fix unaligned access in device_qual()

[Marek Vasut]

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

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

Best regards,
Marek Vasut

[U-Boot] [PATCH] usb: fix unaligned access in device_qual()

[Albert ARIBAUD]

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.

[U-Boot] [PATCH] usb: fix unaligned access in device_qual()

[Albert ARIBAUD]

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,
-- 
Albert.

[U-Boot] [PATCH] usb: fix unaligned access in device_qual()

[Marek Vasut]

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

[U-Boot] [PATCH] usb: fix unaligned access in device_qual()

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

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,
-- 
Albert.

[U-Boot] [PATCH] usb: fix unaligned access in device_qual()

[Marek Vasut]

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

[U-Boot] [PATCH] usb: fix unaligned access in device_qual()

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

> > 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,
-- 
Albert.

[U-Boot] [PATCH] usb: fix unaligned access in device_qual()

[Marek Vasut]

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