| Message ID | 1372320297-16275-1-git-send-email-hs@denx.de |
|---|---|
| State | Accepted |
| Delegated to: | Marek Vasut |
| Headers | show |
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,
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
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
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
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,
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
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;
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(-)