| Message ID | 1341716895-31089-1-git-send-email-marex@denx.de |
|---|---|
| State | Superseded |
| Delegated to: | Marek Vasut |
| Headers | show |
Hi Marek, On Sun, Jul 8, 2012 at 7:08 AM, Marek Vasut <marex@denx.de> wrote: > This is the out-of-function-scope counterpart of > ALLOC_CACHE_ALIGN_BUFFER. > +#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.
Dear Ilya Yanok, > Hi Marek, > > On Sun, Jul 8, 2012 at 7:08 AM, Marek Vasut <marex@denx.de> wrote: > > This is the out-of-function-scope counterpart of > > ALLOC_CACHE_ALIGN_BUFFER. > > +#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. Correct, but is it a good idea to include it in common.h ? Ilya, can you re-do this patchset and repost? > Regards, Ilya. Best regards, Marek Vasut
On Saturday 07 July 2012 23:08:14 Marek Vasut wrote: > +/* > + * 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); -mike
Dear Mike Frysinger, > On Saturday 07 July 2012 23:08:14 Marek Vasut wrote: > > +/* > > + * 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); > -mike Does __aligned() align both start of the buffer downwards and end of it upwards ? Best regards, Marek Vasut
On Friday 20 July 2012 07:31:47 Marek Vasut wrote: > Dear Mike Frysinger, > > On Saturday 07 July 2012 23:08:14 Marek Vasut wrote: > > > +/* > > > 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); > > 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 ? you should probably add a const to the 2nd part so gcc is more likely to optimize away the storage: static type * const name = (type *)__##name; otherwise older gcc versions will create a pointer to go through rather than having things directly access the buffer. w/out const: $ readelf -s a.out | grep foo 11: 00000000004005d0 8 OBJECT LOCAL DEFAULT 13 foo.1592 12: 0000000000402080 96 OBJECT LOCAL DEFAULT 25 __foo.1591 w/const: $ readelf -s a.out | grep foo 11: 0000000000402080 96 OBJECT LOCAL DEFAULT 25 __foo.1591 -mike
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 07/20/2012 02:47 PM, Mike Frysinger wrote: > On Friday 20 July 2012 07:31:47 Marek Vasut wrote: >> Dear Mike Frysinger, >>> On Saturday 07 July 2012 23:08:14 Marek Vasut wrote: >>>> +/* 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); >> >> 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 ? 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. - -- Tom -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.10 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iQIcBAEBAgAGBQJQCdKoAAoJENk4IS6UOR1WuzUP/0ndUzaq6U2/8X8E7VIhLqAV AQ+5rmJy5seJAhhvzRLbNgYv7KrLJRayMd4bj4DT8WAmAyUDo1BPvIcKNuQ8Il3M un3Kjo9+qXusfyvbs1V2wOaXL1pmwcOSQe4n+/8xKlmJ3Jh9v1AIBoXIJQqXPrcL zhaN0ilxTcLeblFWVzXSOWPAWo4ufJZ1cd43lGXxtjW890GEUCRTNXod9Bpivi2P ShycGvEcvo80mR7xGKMjZYl9Zf5OQ5QP0Xcvul7X4rjGepEuOsn941wE0zrP1I+s zXfruOklqWBckiP+aBprX2lzsaBUE33hHZidfPtvuB9rTFk735snaDcrZsL/5K25 17Bczpqqb6RXP1yb/tpc1hWkzpfCZ+eqpg2pN8bIp0P8XZ5apMTvq7L9IMG90lN2 FPwOC2VH9gnOK34Est/iTkp6QnW15r/ZayD9DMBUoxelhx38VJHR8OVyu56QXk2K 10zb6lwKQ3dE8u24ki2TybycVgPVoOq35vUjMw7TWZwiwXSBwgcfHMRukpTSdHA6 wNzw+VsurehKdkqRBeG5tOeOf0tcFrKfg1tfyDQqlBJUn9E6Usj43IXfF23DOQ11 VMcMeizuHP6oTmBk571XrgOKczCaUej1UIrLhnfpXNmFsQ7YsIi9PlQcndFiMBLC yo+rfRjqLqrPxJdrP5h4 =alD6 -----END PGP SIGNATURE-----
On Friday 20 July 2012 17:50:33 Tom Rini wrote: > On 07/20/2012 02:47 PM, Mike Frysinger wrote: > > On Friday 20 July 2012 07:31:47 Marek Vasut wrote: > >> Dear Mike Frysinger, > >> > >>> On Saturday 07 July 2012 23:08:14 Marek Vasut wrote: > >>>> +/* 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); > >> > >> 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 ? > > 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. 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. -mike
On Sat, Jul 21, 2012 at 01:22:40PM -0400, Mike Frysinger wrote: > On Friday 20 July 2012 17:50:33 Tom Rini wrote: > > On 07/20/2012 02:47 PM, Mike Frysinger wrote: > > > On Friday 20 July 2012 07:31:47 Marek Vasut wrote: > > >> Dear Mike Frysinger, > > >> > > >>> On Saturday 07 July 2012 23:08:14 Marek Vasut wrote: > > >>>> +/* 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); > > >> > > >> 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 ? > > > > 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. > > 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. Er, between drivers/usb/host/ehci-hcd.c and drivers/usb/eth/smsc95xx.c the three new macros are used today.
diff --git a/include/common.h b/include/common.h index 17c64b0..06d278f 100644 --- a/include/common.h +++ b/include/common.h @@ -965,6 +965,17 @@ int cpu_release(int nr, int argc, char * const argv[]); \ type *name = (type *) ALIGN((uintptr_t)__##name, ARCH_DMA_MINALIGN) +/* + * 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; + /* Pull in stuff for the build system */ #ifdef DO_DEPS_ONLY # include <environment.h>
This is the out-of-function-scope counterpart of ALLOC_CACHE_ALIGN_BUFFER. Signed-off-by: Marek Vasut <marex@denx.de> Cc: Tom Rini <trini@ti.com> Cc: Ilya Yanok <ilya.yanok@cogentembedded.com> --- include/common.h | 11 +++++++++++ 1 file changed, 11 insertions(+)