common.h: Introduce DEFINE_CACHE_ALIGN_BUFFER

Hi Marek,

On Sun, Jul 8, 2012 at 7:08 AM, Marek Vasut <[email protected]> wrote:

[...]

drivers/usb/host/ehci-hcd.c +0 -0

Quoted 9 lines

index 04300be..5199560 100644

drivers/usb/host/ehci-hcd.c +0 -0

@@ -34,7 +34,10 @@ struct ehci_hccr *hccr;      /* R/O registers, not

need for volatile */

volatile struct ehci_hcor *hcor;

drivers/usb/host/ehci-hcd.c

Quoted 4 lines

static uint16_t portreset;

drivers/usb/host/ehci-hcd.c +1 -1

-static struct QH qh_list __attribute__((aligned(32)));
+DEFINE_CACHE_ALIGN_BUFFER(struct QH, qh_list, 1);

This will align on ARCH_DMA_MINALIGN, not USB_DMA_MINALIGN. In case of ARCH_DMA_MINALIGN < 32 we will loose the 32-byte alignment required by USB spec.

The same with other buffers. Otherwise looks great.

Regards, Ilya.

Dear Ilya Yanok,

Quoted 16 lines

Hi Marek,

On Sun, Jul 8, 2012 at 7:08 AM, Marek Vasut <[email protected]> wrote:

[...]

drivers/usb/host/ehci-hcd.c +0 -0

Quoted 9 lines

index 04300be..5199560 100644

drivers/usb/host/ehci-hcd.c +0 -0

@@ -34,7 +34,10 @@ struct ehci_hccr *hccr;      /* R/O registers, not

need for volatile */

volatile struct ehci_hcor *hcor;

Quoted 8 lines

drivers/usb/host/ehci-hcd.c

Quoted 4 lines

static uint16_t portreset;

drivers/usb/host/ehci-hcd.c +1 -1

-static struct QH qh_list __attribute__((aligned(32)));
+DEFINE_CACHE_ALIGN_BUFFER(struct QH, qh_list, 1);

This will align on ARCH_DMA_MINALIGN, not USB_DMA_MINALIGN. In case of ARCH_DMA_MINALIGN < 32 we will loose the 32-byte alignment required by USB spec.

That's true -- maybe we should create ALLOC_ALIGN_BUFFER() and ALLOC_CACHE_ALIGN_BUFFER() to be a special case of it ?

Quoted 27 lines

Dear Ilya Yanok,

Quoted 24 lines

Hi Marek,

On Sun, Jul 8, 2012 at 7:08 AM, Marek Vasut <[email protected]> wrote:

[...]

drivers/usb/host/ehci-hcd.c +0 -0

drivers/usb/host/ehci-hcd.c

Quoted 13 lines

index 04300be..5199560 100644

drivers/usb/host/ehci-hcd.c +0 -0

@@ -34,7 +34,10 @@ struct ehci_hccr *hccr;      /* R/O registers, not

need for volatile */

volatile struct ehci_hcor *hcor;

static uint16_t portreset;

drivers/usb/host/ehci-hcd.c +1 -1

-static struct QH qh_list __attribute__((aligned(32)));
+DEFINE_CACHE_ALIGN_BUFFER(struct QH, qh_list, 1);

This will align on ARCH_DMA_MINALIGN, not USB_DMA_MINALIGN. In case of ARCH_DMA_MINALIGN < 32 we will loose the 32-byte alignment required by USB spec.

Quoted 2 lines

That's true -- maybe we should create ALLOC_ALIGN_BUFFER() and ALLOC_CACHE_ALIGN_BUFFER() to be a special case of it ?

Lets think. USB says "32byte min". Do we have other buses today that apply similar constraints? If so, we should probably go about abstracting into a __ALIGN_FOO(size, what) and do a general one and a USB one that does 32 or cachline. If it's just USB that might be overkill however...

drivers/usb/host/ehci-hcd.c +4 -3

-       /* Invalidate the memory area occupied by buffer */
-       invalidate_dcache_range(((uint32_t)buffer & ~31),
-               ((uint32_t)buffer & ~31) + roundup(length, 32));
+       /*
+        * Invalidate the memory area occupied by buffer
+        * Don't try to fix the buffer alignment, if it isn't properly
+        * aligned it's upper layer's fault so let

invalidate_dcache_range()

Hi Marek,

Quoted 8 lines · Marek Vasut

drivers/usb/host/ehci-hcd.c +4 -3

-       /* Invalidate the memory area occupied by buffer */
-       invalidate_dcache_range(((uint32_t)buffer & ~31),
-               ((uint32_t)buffer & ~31) + roundup(length, 32));
+       /*
+        * Invalidate the memory area occupied by buffer
+        * Don't try to fix the buffer alignment, if it isn't properly
+        * aligned it's upper layer's fault so let

invalidate_dcache_range()

drivers/usb/host/ehci-hcd.c ehci_submit_async(struct usb_device *dev, unsigned long pipe, void *buffer, · line 398

Quoted 7 lines

drivers/usb/host/ehci-hcd.c +7 -0

+        * vow about it. But we have to fix the length as it's actual
+        * transfer length and can be unaligned. This is potentially
+        * dangerous operation, it's responsibility of the calling
+        * code to make sure enough space is reserved.
+        */
+       invalidate_dcache_range((uint32_t)buffer,
+               ALIGN_END_ADDR(u8, buffer, length));

We shouldn't use ALIGN_END_ADDR here. This can lead to strange results on systems with ARCH_DMA_MINALIGN < 32.

Regards, Ilya.

Quoted 6 lines

drivers/usb/host/ehci-hcd.c ehci_submit_async(struct usb_device *dev, unsigned long pipe, void *buffer, · line 398

Quoted 3 lines

drivers/usb/host/ehci-hcd.c +2 -0

+       invalidate_dcache_range((uint32_t)buffer,
+               ALIGN_END_ADDR(u8, buffer, length));

We shouldn't use ALIGN_END_ADDR here. This can lead to strange results on systems with ARCH_DMA_MINALIGN < 32.

Good point, ALIGN_END_ADDR should align it to max(32, length) ?

Hm.. no, actually we should align buffer length to ARCH_DMA_MINALIGN.

Hi,

Quoted 8 lines · Marek Vasut

(quoted diff) +0 -0

@@ -207,8 +210,8 @@ static int
 
 ehci_submit_async(struct usb_device *dev, unsigned long pipe, void
 
 *buffer,
 
                   int length, struct devrequest *req)
 

drivers/usb/host/ehci-hcd.c static int · line 213

Quoted 5 lines

drivers/usb/host/ehci-hcd.c +1 -2

 {
 
-       static struct QH qh __attribute__((aligned(32)));
-       static struct qTD qtd[3] __attribute__((aligned (32)));
+       ALLOC_CACHE_ALIGN_BUFFER(struct QH, qh, 1);

Somehow this doesn't work being allocated on stack... I assumed that this was declared as static just to use __attribute__((aligned))...

but it seems
that current code doesn't like qh address being changed (I'm getting "Timed out on TD" messages). Any ideas? Changing it to DEFINE_... seems to help but it looks like hiding another bug.

Regards, Ilya.

Quoted 8 lines

Quoted 8 lines

(quoted diff) +0 -0

@@ -207,8 +210,8 @@ static int
 
 ehci_submit_async(struct usb_device *dev, unsigned long pipe, void
 
 *buffer,
 
                   int length, struct devrequest *req)
 

Quoted 8 lines

drivers/usb/host/ehci-hcd.c static int · line 213

Quoted 5 lines

drivers/usb/host/ehci-hcd.c +1 -2

 {
 
-       static struct QH qh __attribute__((aligned(32)));
-       static struct qTD qtd[3] __attribute__((aligned (32)));
+       ALLOC_CACHE_ALIGN_BUFFER(struct QH, qh, 1);

Somehow this doesn't work being allocated on stack... I assumed that this was declared as static just to use __attribute__((aligned))...

Not really ... but then, we have indeed a problem here. It was probably declared
static to preserve the contents of these variables across the the calls of this
function. Sigh, the USB code is really bloody :-/

Ilya, can you try pulling these out of the function?

Dear Marek,

Quoted 17 lines · Marek Vasut

Quoted 8 lines

Quoted 8 lines

(quoted diff) +0 -0

@@ -207,8 +210,8 @@ static int
 
 ehci_submit_async(struct usb_device *dev, unsigned long pipe, void
 
 *buffer,
 
                   int length, struct devrequest *req)
 

Quoted 8 lines

drivers/usb/host/ehci-hcd.c static int · line 213

Quoted 5 lines

drivers/usb/host/ehci-hcd.c +1 -2

 {
 
-       static struct QH qh __attribute__((aligned(32)));
-       static struct qTD qtd[3] __attribute__((aligned (32)));
+       ALLOC_CACHE_ALIGN_BUFFER(struct QH, qh, 1);

Somehow this doesn't work being allocated on stack... I assumed that this was declared as static just to use __attribute__((aligned))...

Quoted 5 lines

Not really ... but then, we have indeed a problem here. It was probably declared
static to preserve the contents of these variables across the the calls of this
function. Sigh, the USB code is really bloody :-/

Hm.. no, I don't think it's about contents... Look, there are memset() calls straight in the beginning of the function. It looks like it's about qh address: I tried debugging it a bit and it seems to work until qh has the same address and break when the address eventually changes.

Quoted 1 line

Ilya, can you try pulling these out of the function?

I can but what for? Are there any differences (except for scope) declaring statics inside or outside function? As I said declaring qh static (with DEFINE_CACHE_ALIGN_BUFFER) works but...

Regards, Ilya.

This is the out-of-function-scope counterpart of
ALLOC_CACHE_ALIGN_BUFFER.

(quoted diff) +2 -0

+#define DEFINE_CACHE_ALIGN_BUFFER(type, name, size)                    \
+       static char __##name[roundup(size * sizeof(type),

ARCH_DMA_MINALIGN)] \
+ __aligned(ARCH_DMA_MINALIGN); \

Hi Marek,

Quoted 6 lines · Marek Vasut

This is the out-of-function-scope counterpart of
ALLOC_CACHE_ALIGN_BUFFER.

(quoted diff) +2 -0

+#define DEFINE_CACHE_ALIGN_BUFFER(type, name, size)                    \
+       static char __##name[roundup(size * sizeof(type),

ARCH_DMA_MINALIGN)] \
+ __aligned(ARCH_DMA_MINALIGN); \

We need linux/compiler.h (not included from common.h) for __aligned.

Regards, Ilya.

Quoted 6 lines

+/*
+ * DEFINE_CACHE_ALIGN_BUFFER() is similar to ALLOC_CACHE_ALIGN_BUFFER, but it's + * purpose is to allow allocating aligned buffers outside of function scope. + * Usage of this macro shall be avoided or used with extreme care! + */
+#define DEFINE_CACHE_ALIGN_BUFFER(type, name, size)

include/common.h int cpu_release(int nr, int argc, char * const argv[]); · line 977

Quoted 4 lines

include/common.h +4 -0

+	static char __##name[roundup(size * sizeof(type), ARCH_DMA_MINALIGN)] \
+			__aligned(ARCH_DMA_MINALIGN);			\
+									\
+	static type *name = (type *)__##name;

how is this any different from doing:

	static __u8 foo[1234] __aligned(ARCH_DMA_MINALIGN);

-mike

Dear Mike Frysinger,

Quoted 12 lines · On Saturday 07 July 2012 23:08:14 Marek Vasut wrote:

Review

Quoted 9 lines

+/*
DEFINE_CACHE_ALIGN_BUFFER() is similar to ALLOC_CACHE_ALIGN_BUFFER, but it's purpose is to allow allocating aligned buffers outside of function scope. Usage of this macro shall be avoided or used with extreme care! */

(quoted diff) +4 -0

+#define DEFINE_CACHE_ALIGN_BUFFER(type, name, size)
+	static char __##name[roundup(size * sizeof(type), ARCH_DMA_MINALIGN)]
+			__aligned(ARCH_DMA_MINALIGN);
+	static type *name = (type *)__##name;

how is this any different from doing:

Quoted 1 line

	static __u8 foo[1234] __aligned(ARCH_DMA_MINALIGN);

Does __aligned() align both start of the buffer downwards and end of it upwards ?

Quoted 14 lines · On Friday 20 July 2012 07:31:47 Marek Vasut wrote:

Dear Mike Frysinger,

Quoted 12 lines · On Saturday 07 July 2012 23:08:14 Marek Vasut wrote:

Quoted 8 lines

+/* DEFINE_CACHE_ALIGN_BUFFER() is similar to ALLOC_CACHE_ALIGN_BUFFER, but it's purpose is to allow allocating aligned buffers outside of function scope. Usage of this macro shall be avoided or used with extreme care! */
+#define DEFINE_CACHE_ALIGN_BUFFER(type, name, size) + static
char __##name[roundup(size * sizeof(type),
ARCH_DMA_MINALIGN)] + __aligned(ARCH_DMA_MINALIGN); +
static type *name = (type *)__##name;

how is this any different from doing: static __u8 foo[1234] __aligned(ARCH_DMA_MINALIGN);

Quoted 2 lines

Does __aligned() align both start of the buffer downwards and end of it upwards ?

it guarantees the start is aligned. i don't believe it does any tail padding.

that said, you've added just 1 consumer, but it uses in function scope, so i don't see why you had to define a new helper in the first place. the existing one would work fine shouldn't it ?

Quoted 19 lines · Mike Frysinger

Quoted 15 lines · On Friday 20 July 2012 07:31:47 Marek Vasut wrote:

Dear Mike Frysinger,

Quoted 12 lines · On Saturday 07 July 2012 23:08:14 Marek Vasut wrote:

Quoted 8 lines

+/* DEFINE_CACHE_ALIGN_BUFFER() is similar to ALLOC_CACHE_ALIGN_BUFFER, but it's purpose is to allow allocating aligned buffers outside of function scope. Usage of this macro shall be avoided or used with extreme care! */
+#define DEFINE_CACHE_ALIGN_BUFFER(type, name, size) + static
char __##name[roundup(size * sizeof(type),
ARCH_DMA_MINALIGN)] + __aligned(ARCH_DMA_MINALIGN); +
static type *name = (type *)__##name;

how is this any different from doing: static __u8 foo[1234] __aligned(ARCH_DMA_MINALIGN);

Quoted 2 lines

Does __aligned() align both start of the buffer downwards and end of it upwards ?

Quoted 6 lines

it guarantees the start is aligned. i don't believe it does any tail padding.

that said, you've added just 1 consumer, but it uses in function scope, so i don't see why you had to define a new helper in the first place. the existing one would work fine shouldn't it ?

The rough outline of the problems are:
- We need to have buffers that are multiple of cache size, for clearing.
- Today we know ehci-hcd had a problem. We also know other drivers / layers have problems, but they aren't as readily breakable.

That's why we put the macro in <common.h> rather than a USB header.

Quoted 32 lines · On Friday 20 July 2012 17:50:33 Tom Rini wrote:

Quoted 19 lines · Mike Frysinger

Quoted 15 lines · On Friday 20 July 2012 07:31:47 Marek Vasut wrote:

Dear Mike Frysinger,

Quoted 12 lines · On Saturday 07 July 2012 23:08:14 Marek Vasut wrote:

Quoted 8 lines

+/* DEFINE_CACHE_ALIGN_BUFFER() is similar to ALLOC_CACHE_ALIGN_BUFFER, but it's purpose is to allow allocating aligned buffers outside of function scope. Usage of this macro shall be avoided or used with extreme care! */
+#define DEFINE_CACHE_ALIGN_BUFFER(type, name, size) + static
char __##name[roundup(size * sizeof(type),
ARCH_DMA_MINALIGN)] + __aligned(ARCH_DMA_MINALIGN); +
static type *name = (type *)__##name;

how is this any different from doing: static __u8 foo[1234] __aligned(ARCH_DMA_MINALIGN);

Quoted 2 lines

Does __aligned() align both start of the buffer downwards and end of it upwards ?

Quoted 6 lines

it guarantees the start is aligned. i don't believe it does any tail padding.

that said, you've added just 1 consumer, but it uses in function scope, so i don't see why you had to define a new helper in the first place. the existing one would work fine shouldn't it ?

The rough outline of the problems are:
- We need to have buffers that are multiple of cache size, for clearing.
- Today we know ehci-hcd had a problem. We also know other drivers / layers have problems, but they aren't as readily breakable.

Quoted 1 line

That's why we put the macro in <common.h> rather than a USB header.

that wasn't the question. no one in the tree needs the new macro at all, regardless of what header it lives in. i guess the answer is that some code in the future (which hasn't been merged) might use it.