usb: gadget: Fix data aborts during USB ethernet boot

PATCH New Joel Fernandes 2 participants 6 messages Aug 20, 2013 – Sep 4, 2013 USB

Share & tools

Joel Fernandes

author

2013-08-20 23:32 UTC #0 lore raw

PATCH 1/1 usb: gadget: Fix data aborts during USB ethernet boot

As seen on GCC 4.6 Linaro compiler, control_req buffer is not aligned on 4 byte boundaray causing data aborts in eth_setup -> conf_buf during dhcp boot over usb_ether. Fix the issue my aligning control_req buffer to 4-byte boundary.

Tested on am335x_evm platform (beaglebone).
Applies on 2013.10-rc1 branch.

Cc Tom Rini <[email protected]>

Signed-off-by Joel Fernandes <[email protected]>

---
 drivers/usb/gadget/ether.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/usb/gadget/ether.c +1 -1

index 579893c..251d7b2 100644
@@ -849,7 +849,7 @@ static struct usb_gadget_strings	stringtab = {
 };
 
 /*============================================================================*/
-static u8 control_req[USB_BUFSIZ];
+static u8 control_req[USB_BUFSIZ] __attribute__ ((aligned(4)));
 #if defined(CONFIG_USB_ETH_CDC) || defined(CONFIG_USB_ETH_RNDIS)
 static u8 status_req[STATUS_BYTECOUNT] __attribute__ ((aligned(4)));
 #endif

Signature · 1 line

1.7.9.5

Marek Vasut

2013-08-27 13:52 UTC #1 lore raw

PATCH 1/1 usb: gadget: Fix data aborts during USB ethernet boot

Dear Joel Fernandes,

Quoted 10 lines

As seen on GCC 4.6 Linaro compiler, control_req buffer is not aligned on 4 byte boundaray causing data aborts in eth_setup -> conf_buf during dhcp boot over usb_ether. Fix the issue my aligning control_req buffer to 4-byte boundary.
Tested on am335x_evm platform (beaglebone).
Applies on 2013.10-rc1 branch.
Cc Tom Rini <[email protected]>
Signed-off-by Joel Fernandes <[email protected]>

Please keep me in the CC next time.

Quoted 9 lines

---
 drivers/usb/gadget/ether.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/usb/gadget/ether.c +0 -0

index 579893c..251d7b2 100644

drivers/usb/gadget/ether.c static struct usb_gadget_strings stringtab = { · line 853

Quoted 5 lines

drivers/usb/gadget/ether.c +1 -0

@@ -849,7 +849,7 @@ static struct usb_gadget_strings	stringtab = {
 };
 
 /*========================================================================
 ====*/ -static u8 control_req[USB_BUFSIZ];
+static u8 control_req[USB_BUFSIZ] __attribute__ ((aligned(4)));

Please make this cacheline aligned, so we get rid of bounce buffering of the requests on stupid hardware.

drivers/usb/gadget/ether.c static struct usb_gadget_strings stringtab = { · line 854

Quoted 2 lines

drivers/usb/gadget/ether.c +0 -0

 #if defined(CONFIG_USB_ETH_CDC) || defined(CONFIG_USB_ETH_RNDIS)
 static u8 status_req[STATUS_BYTECOUNT] __attribute__ ((aligned(4)));

This could also use fixing, but the STATUS_BYTECOUNT would need to be up-aligned as well then. Some DEFINE_CACHE_ALIGN_BUFFER might help in both cases here.

Quoted 1 line

#endif

Best regards,
Marek Vasut

Joel Fernandes

author

2013-09-04 21:30 UTC #2 lore raw

PATCH 1/1 usb: gadget: Fix data aborts during USB ethernet boot

Quoted 7 lines · Marek Vasut

Dear Joel Fernandes,
Quoted 5 lines
As seen on GCC 4.6 Linaro compiler, control_req buffer is not aligned on 4 byte boundaray causing data aborts in eth_setup -> conf_buf during dhcp boot over usb_ether. Fix the issue my aligning control_req buffer to 4-byte boundary.

Quoted 7 lines

Quoted 5 lines
Tested on am335x_evm platform (beaglebone).
Applies on 2013.10-rc1 branch.
Cc Tom Rini <[email protected]>
Signed-off-by Joel Fernandes <[email protected]>
Please keep me in the CC next time.

Ok.

Quoted 17 lines

Quoted 9 lines
---
 drivers/usb/gadget/ether.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/usb/gadget/ether.c +0 -0
index 579893c..251d7b2 100644
drivers/usb/gadget/ether.c static struct usb_gadget_strings stringtab = { · line 853
Quoted 5 lines
drivers/usb/gadget/ether.c +1 -0
@@ -849,7 +849,7 @@ static struct usb_gadget_strings	stringtab = {
 };
 
 /*========================================================================
 ====*/ -static u8 control_req[USB_BUFSIZ];
+static u8 control_req[USB_BUFSIZ] __attribute__ ((aligned(4)));
Please make this cacheline aligned, so we get rid of bounce buffering of the requests on stupid hardware.

Quoted 5 lines

drivers/usb/gadget/ether.c static struct usb_gadget_strings stringtab = { · line 854
Quoted 2 lines
drivers/usb/gadget/ether.c +0 -0
 #if defined(CONFIG_USB_ETH_CDC) || defined(CONFIG_USB_ETH_RNDIS)
 static u8 status_req[STATUS_BYTECOUNT] __attribute__ ((aligned(4)));
This could also use fixing, but the STATUS_BYTECOUNT would need to be up-aligned as well then. Some DEFINE_CACHE_ALIGN_BUFFER might help in both cases here.

Ok, how about __aligned(CONFIG_SYS_CACHELINE_SIZE) instead of defining a new DEFINE_CACHE_ALIGN_BUFFER macro?

Can you explain what you meant by STATUS_BYTECOUNT up-aligned and why its needed to be up-aligned?

Regards,

-Joel

Quoted 6 lines

Quoted 2 lines
#endif
Best regards,
Marek Vasut

Marek Vasut

2013-09-04 21:38 UTC #3 lore raw

PATCH 1/1 usb: gadget: Fix data aborts during USB ethernet boot

Dear Joel Fernandes,

Quoted 43 lines · Marek Vasut

Quoted 7 lines
Dear Joel Fernandes,
Quoted 5 lines
As seen on GCC 4.6 Linaro compiler, control_req buffer is not aligned on 4 byte boundaray causing data aborts in eth_setup -> conf_buf during dhcp boot over usb_ether. Fix the issue my aligning control_req buffer to 4-byte boundary.
Quoted 7 lines
Quoted 5 lines
Tested on am335x_evm platform (beaglebone).
Applies on 2013.10-rc1 branch.
Cc Tom Rini <[email protected]>
Signed-off-by Joel Fernandes <[email protected]>
Please keep me in the CC next time.
Ok.
Quoted 25 lines
Quoted 10 lines
---
 drivers/usb/gadget/ether.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/usb/gadget/ether.c +0 -0
index 579893c..251d7b2 100644
drivers/usb/gadget/ether.c static struct usb_gadget_strings stringtab = {
Quoted 8 lines
drivers/usb/gadget/ether.c +1 -0
@@ -849,7 +849,7 @@ static struct usb_gadget_strings	stringtab = {
 
 };
 
 /*=====================================================================
 ===
 
 ====*/ -static u8 control_req[USB_BUFSIZ];
+static u8 control_req[USB_BUFSIZ] __attribute__ ((aligned(4)));
Please make this cacheline aligned, so we get rid of bounce buffering of the requests on stupid hardware.
Quoted 2 lines
#if defined(CONFIG_USB_ETH_CDC) || defined(CONFIG_USB_ETH_RNDIS) static u8 status_req[STATUS_BYTECOUNT] __attribute__ ((aligned(4)));

Quoted 6 lines

Quoted 3 lines
This could also use fixing, but the STATUS_BYTECOUNT would need to be up-aligned as well then. Some DEFINE_CACHE_ALIGN_BUFFER might help in both cases here.
Ok, how about __aligned(CONFIG_SYS_CACHELINE_SIZE) instead of defining a new DEFINE_CACHE_ALIGN_BUFFER macro?

THis is already defined and used throughout the USB code, check include/common.h IIRC.

Quoted 2 lines

Can you explain what you meant by STATUS_BYTECOUNT up-aligned and why its needed to be up-aligned?

it's 16 bytes now, no? Up-align it to cacheline size.

Best regards,
Marek Vasut

Joel Fernandes

author

2013-09-04 21:54 UTC #4 lore raw

PATCH 1/1 usb: gadget: Fix data aborts during USB ethernet boot

Quoted 52 lines · Marek Vasut

Dear Joel Fernandes,
Quoted 43 lines · Marek Vasut
Quoted 7 lines
Dear Joel Fernandes,
Quoted 5 lines
As seen on GCC 4.6 Linaro compiler, control_req buffer is not aligned on 4 byte boundaray causing data aborts in eth_setup -> conf_buf during dhcp boot over usb_ether. Fix the issue my aligning control_req buffer to 4-byte boundary.
Quoted 7 lines
Quoted 5 lines
Tested on am335x_evm platform (beaglebone).
Applies on 2013.10-rc1 branch.
Cc Tom Rini <[email protected]>
Signed-off-by Joel Fernandes <[email protected]>
Please keep me in the CC next time.
Ok.
Quoted 25 lines
Quoted 10 lines
---
 drivers/usb/gadget/ether.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/usb/gadget/ether.c +0 -0
index 579893c..251d7b2 100644
drivers/usb/gadget/ether.c static struct usb_gadget_strings stringtab = {
Quoted 8 lines
drivers/usb/gadget/ether.c +1 -0
@@ -849,7 +849,7 @@ static struct usb_gadget_strings	stringtab = {
 
 };
 
 /*=====================================================================
 ===
 
 ====*/ -static u8 control_req[USB_BUFSIZ];
+static u8 control_req[USB_BUFSIZ] __attribute__ ((aligned(4)));
Please make this cacheline aligned, so we get rid of bounce buffering of the requests on stupid hardware.
Quoted 2 lines
#if defined(CONFIG_USB_ETH_CDC) || defined(CONFIG_USB_ETH_RNDIS) static u8 status_req[STATUS_BYTECOUNT] __attribute__ ((aligned(4)));
Quoted 6 lines
Quoted 3 lines
This could also use fixing, but the STATUS_BYTECOUNT would need to be up-aligned as well then. Some DEFINE_CACHE_ALIGN_BUFFER might help in both cases here.
Ok, how about __aligned(CONFIG_SYS_CACHELINE_SIZE) instead of defining a new DEFINE_CACHE_ALIGN_BUFFER macro?

Quoted 7 lines

THis is already defined and used throughout the USB code, check include/common.h IIRC.
Quoted 2 lines
Can you explain what you meant by STATUS_BYTECOUNT up-aligned and why its needed to be up-aligned?
it's 16 bytes now, no? Up-align it to cacheline size.

Ok, that makes sense. Thanks. Hope below patch is OK, but I have to test it more once I am near a setup and can send out a proper patch later today.

If its ok can you explain technical reason for need to up-align array size to cacheline size, and why aligning only buffer location is not enough?

Also I saw the following comment before the macro definition:
* Usage of this macro shall be avoided or used with extreme care!

Regards,

-Joel

drivers/usb/gadget/ether.c +3 -2

index 579893c..700d5fb 100644
@@ -849,9 +849,10 @@ static struct usb_gadget_strings	stringtab = {
 };
 
 /*============================================================================*/
-static u8 control_req[USB_BUFSIZ];
+DEFINE_CACHE_ALIGN_BUFFER(u8, control_req, USB_BUFSIZ);
+
 #if defined(CONFIG_USB_ETH_CDC) || defined(CONFIG_USB_ETH_RNDIS)
-static u8 status_req[STATUS_BYTECOUNT] __attribute__ ((aligned(4)));
+DEFINE_CACHE_ALIGN_BUFFER(u8, status_req, STATUS_BYTECOUNT);
 #endif

Marek Vasut

2013-09-04 22:02 UTC #5 lore raw

PATCH 1/1 usb: gadget: Fix data aborts during USB ethernet boot

Dear Joel Fernandes,

Quoted 79 lines · Marek Vasut

Quoted 52 lines
Dear Joel Fernandes,
Quoted 43 lines · Marek Vasut
Quoted 7 lines
Dear Joel Fernandes,
Quoted 5 lines
As seen on GCC 4.6 Linaro compiler, control_req buffer is not aligned on 4 byte boundaray causing data aborts in eth_setup -> conf_buf during dhcp boot over usb_ether. Fix the issue my aligning control_req buffer to 4-byte boundary.
Quoted 7 lines
Quoted 5 lines
Tested on am335x_evm platform (beaglebone).
Applies on 2013.10-rc1 branch.
Cc Tom Rini <[email protected]>
Signed-off-by Joel Fernandes <[email protected]>
Please keep me in the CC next time.
Ok.
Quoted 25 lines
Quoted 10 lines
---
 drivers/usb/gadget/ether.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/usb/gadget/ether.c +0 -0
index 579893c..251d7b2 100644
drivers/usb/gadget/ether.c static struct usb_gadget_strings stringtab = {
Quoted 8 lines
drivers/usb/gadget/ether.c +1 -0
@@ -849,7 +849,7 @@ static struct usb_gadget_strings	stringtab = {
 
 };
 
 /*===================================================================
 == ===
 
 ====*/ -static u8 control_req[USB_BUFSIZ];
+static u8 control_req[USB_BUFSIZ] __attribute__ ((aligned(4)));
Please make this cacheline aligned, so we get rid of bounce buffering of the requests on stupid hardware.
Quoted 2 lines
#if defined(CONFIG_USB_ETH_CDC) || defined(CONFIG_USB_ETH_RNDIS) static u8 status_req[STATUS_BYTECOUNT] __attribute__ ((aligned(4)));
Quoted 6 lines
Quoted 3 lines
This could also use fixing, but the STATUS_BYTECOUNT would need to be up-aligned as well then. Some DEFINE_CACHE_ALIGN_BUFFER might help in both cases here.
Ok, how about __aligned(CONFIG_SYS_CACHELINE_SIZE) instead of defining a new DEFINE_CACHE_ALIGN_BUFFER macro?
Quoted 7 lines
THis is already defined and used throughout the USB code, check include/common.h IIRC.
Quoted 2 lines
Can you explain what you meant by STATUS_BYTECOUNT up-aligned and why its needed to be up-aligned?
it's 16 bytes now, no? Up-align it to cacheline size.
Ok, that makes sense. Thanks. Hope below patch is OK, but I have to test it more once I am near a setup and can send out a proper patch later today.
If its ok can you explain technical reason for need to up-align array size to cacheline size, and why aligning only buffer location is not enough?
Also I saw the following comment before the macro definition:
* Usage of this macro shall be avoided or used with extreme care!
Regards,
(quoted diff) +0 -1
-Joel
drivers/usb/gadget/ether.c +0 -0
index 579893c..700d5fb 100644

drivers/usb/gadget/ether.c static struct usb_gadget_strings stringtab = { · line 857

Quoted 10 lines

drivers/usb/gadget/ether.c +3 -1

@@ -849,9 +849,10 @@ static struct usb_gadget_strings	stringtab = {
 };
 
 /*========================================================================
 ====*/ -static u8 control_req[USB_BUFSIZ];
+DEFINE_CACHE_ALIGN_BUFFER(u8, control_req, USB_BUFSIZ);
+
 #if defined(CONFIG_USB_ETH_CDC) || defined(CONFIG_USB_ETH_RNDIS)
-static u8 status_req[STATUS_BYTECOUNT] __attribute__ ((aligned(4)));
+DEFINE_CACHE_ALIGN_BUFFER(u8, status_req, STATUS_BYTECOUNT);
 #endif

Yeah, I think so. Repost V2 and we should be cool here.

Best regards,
Marek Vasut