smsc95xx: align buffers to cache line size

Align buffers passed to the USB code to cache line size so they can be DMAed safely.

Signed-off-by Ilya Yanok <[email protected]>

---
Depens on Marek's patch [1] for DEFINE_CACHE_ALIGN_BUFFER.

[1] http://patchwork.ozlabs.org/patch/169619/

 drivers/usb/eth/smsc95xx.c |   13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

drivers/usb/eth/smsc95xx.c +8 -3

index c7aebea..c62a8c1 100644
@@ -153,13 +153,15 @@ static int curr_eth_dev; /* index for name of next
 device detected */ static int smsc95xx_write_reg(struct ueth_data *dev,
 u32 index, u32 data) {
 	int len;
+	ALLOC_CACHE_ALIGN_BUFFER(u32, tmpbuf, 1);
 
 	cpu_to_le32s(&data);
+	tmpbuf[0] = data;
 
 	len = usb_control_msg(dev->pusb_dev, usb_sndctrlpipe(dev->pusb_dev, 0),
 		USB_VENDOR_REQUEST_WRITE_REGISTER,
 		USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
-		00, index, &data, sizeof(data), USB_CTRL_SET_TIMEOUT);
+		00, index, tmpbuf, sizeof(data), USB_CTRL_SET_TIMEOUT);
 	if (len != sizeof(data)) {
 		debug("smsc95xx_write_reg failed: index=%d, data=%d, len=%d",
 		      index, data, len);

@@ -171,11 +173,13 @@ static int smsc95xx_write_reg(struct ueth_data *dev,
 u32 index, u32 data) static int smsc95xx_read_reg(struct ueth_data *dev,
 u32 index, u32 *data) {
 	int len;
+	ALLOC_CACHE_ALIGN_BUFFER(u32, tmpbuf, 1);
 
 	len = usb_control_msg(dev->pusb_dev, usb_rcvctrlpipe(dev->pusb_dev, 0),
 		USB_VENDOR_REQUEST_READ_REGISTER,
 		USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
-		00, index, data, sizeof(data), USB_CTRL_GET_TIMEOUT);
+		00, index, tmpbuf, sizeof(data), USB_CTRL_GET_TIMEOUT);
+	*data = tmpbuf[0];
 	if (len != sizeof(data)) {
 		debug("smsc95xx_read_reg failed: index=%d, len=%d",
 		      index, len);

@@ -664,7 +668,8 @@ static int smsc95xx_send(struct eth_device *eth, void*
 packet, int length) int actual_len;
 	u32 tx_cmd_a;
 	u32 tx_cmd_b;
-	unsigned char msg[PKTSIZE + sizeof(tx_cmd_a) + sizeof(tx_cmd_b)];
+	ALLOC_CACHE_ALIGN_BUFFER(unsigned char, msg,
+				 PKTSIZE + sizeof(tx_cmd_a) + sizeof(tx_cmd_b));
 
 	debug("** %s(), len %d, buf %#x\n", __func__, length, (int)msg);
 	if (length > PKTSIZE)

btw. this will fail with cache line < 32 .

Dear Marek,

Quoted 1 line · Marek Vasut

btw. this will fail with cache line < 32 .

Hm.. I have to admit I'm not very much into USB specs and I don't have any non-ARMv7 system now to do some testing...
But it used to work without any alignment, right? (with disabled dcache, of course)
That makes me think that data buffers don't need any alignment (from USB pov, not cache) and 32-byte alignment is required for internal structs only.

Regards, Ilya.

Quoted 2 lines

non-ARMv7 system now to do some testing...
But it used to work without any alignment, right? (with disabled dcache,

of

Quoted 4 lines

course)
That makes me think that data buffers don't need any alignment (from USB pov, not cache) and 32-byte alignment is required for internal structs only.

Quoted 1 line

Hm.. I have to admit I'm not very much into USB specs and I don't have

any
See ehci-r10.pdf ... chapter 3.5 ... the buffer pointer has to be aligned too it
seems.

Dear Marek,

Quoted 11 lines · Marek Vasut

Quoted 3 lines

non-ARMv7 system now to do some testing...
But it used to work without any alignment, right? (with disabled dcache,

of

Quoted 5 lines

course)
That makes me think that data buffers don't need any alignment (from USB pov, not cache) and 32-byte alignment is required for internal structs only.

Quoted 6 lines

Quoted 1 line

Hm.. I have to admit I'm not very much into USB specs and I don't have

any
See ehci-r10.pdf ... chapter 3.5 ... the buffer pointer has to be aligned too it
seems.

But in practice it works without any alignment... ok, you made me read the spec ;)
page 55: "For the page 0 current offset interpretation, this field is the byte offset into the current page"

Regards, Ilya.

Quoted 2 lines

But in practice it works without any alignment... ok, you made me read

the

Quoted 3 lines

spec ;)
page 55: "For the page 0 current offset interpretation, this field is the byte offset into the current page"

See the thread
"[U-Boot] [PATCH] usb_storage: fix ehci driver max transfer size"

So, if we compute the size over there correctly (I hope Stefan will send an updated patch soon), we'll still need it to be aligned to ARCH_DMA_MINALIGN so
we can flush it. Bah, this is getting quite crazy, the deeper we go, the more
bugs we meet!

So, what I'd like to do is to see a patch from Stefan, it was really a good finding! Next up, we should finish this patchset for proper EHCI QH and qTD alignment. And finally, we need generic bounce buffer to use as a protection
against crazy people who might want to load stuff to unaligned address.

Dear Marek,

Quoted 10 lines · Marek Vasut

Quoted 1 line

But in practice it works without any alignment... ok, you made me read

the

Quoted 3 lines

spec ;)
page 55: "For the page 0 current offset interpretation, this field is the byte offset into the current page"

See the thread
"[U-Boot] [PATCH] usb_storage: fix ehci driver max transfer size"

Interesting... but that's a different issue...

Quoted 6 lines

So, if we compute the size over there correctly (I hope Stefan will send an updated patch soon), we'll still need it to be aligned to ARCH_DMA_MINALIGN so
we can flush it. Bah, this is getting quite crazy, the deeper we go, the more
bugs we meet!

Well, of course we need proper alignment for cache stuff (well, actually we can skip this alignment thing for the buffer we will flush as long as all buffers we are going to invalidate are properly aligned/sized... but that's too tricky, personally I'd prefer every DMAed buffer to be cache-line aligned/sized).

And this patch actually adds the alignment for the smsc95xx driver's buffers. In your initial reply you said it will be broken on systems with ARCH_DMA_MINALIGN < 32, so I'm asking what makes you think so?

Quoted 5 lines

So, what I'd like to do is to see a patch from Stefan, it was really a good finding! Next up, we should finish this patchset for proper EHCI QH and qTD alignment. And finally, we need generic bounce buffer to use as a protection
against crazy people who might want to load stuff to unaligned address.

Good plan.

Regards, Ilya.

Quoted 1 line

Well, of course we need proper alignment for cache stuff (well, actually

we

Quoted 2 lines

can skip this alignment thing for the buffer we will flush as long as all buffers we are going to invalidate are properly aligned/sized... but

that's

Quoted 3 lines

too tricky, personally I'd prefer every DMAed buffer to be cache-line aligned/sized).

Quoted 3 lines

And this patch actually adds the alignment for the smsc95xx driver's buffers. In your initial reply you said it will be broken on systems with ARCH_DMA_MINALIGN < 32, so I'm asking what makes you think so?

ALLOC_CACHE_ALIGN_BUFFER(u32, tmpbuf, 1); this stuff maybe? It'll be aligned to
16 bytes for arch with 16 byte cachelines.

Hi Marek,

Quoted 18 lines · Marek Vasut

Quoted 2 lines

Well, of course we need proper alignment for cache stuff (well, actually

we

Quoted 3 lines

can skip this alignment thing for the buffer we will flush as long as all buffers we are going to invalidate are properly aligned/sized... but

that's

Quoted 6 lines

too tricky, personally I'd prefer every DMAed buffer to be cache-line aligned/sized).

And this patch actually adds the alignment for the smsc95xx driver's buffers. In your initial reply you said it will be broken on systems with ARCH_DMA_MINALIGN < 32, so I'm asking what makes you think so?

Quoted 3 lines

ALLOC_CACHE_ALIGN_BUFFER(u32, tmpbuf, 1); this stuff maybe? It'll be aligned to
16 bytes for arch with 16 byte cachelines.

Yes, and this is exactly what we need.

Regards, Ilya.

Quoted 6 lines

Quoted 2 lines

aligned to
16 bytes for arch with 16 byte cachelines.

Yes, and this is exactly what we need.

Quoted 1 line

ALLOC_CACHE_ALIGN_BUFFER(u32, tmpbuf, 1); this stuff maybe? It'll be

It isn't, EHCI needs it aligned on 32 byte boundary.

Quoted 5 lines

Quoted 5 lines

Quoted 2 lines

aligned to
16 bytes for arch with 16 byte cachelines.

Yes, and this is exactly what we need.

Quoted 3 lines

Quoted 1 line

Quoted 1 line

ALLOC_CACHE_ALIGN_BUFFER(u32, tmpbuf, 1); this stuff maybe? It'll be

It isn't, EHCI needs it aligned on 32 byte boundary.

It doesn't. I posted the page number with data buffer description in EHCI spec upper in this thread. There is no alignment requirement at all. Controller is fine with any address. _We_ need the buffer to be cache line aligned to perform cache operations safely.

Regards, Ilya.