On Wed, 3 Dec 2014, Sebastian Andrzej Siewior wrote:
> the following error pops up during "testusb -a -t 10"
> | musb-hdrc musb-hdrc.1.auto: dma_pool_free buffer-128, f134e000/be842000 (bad dma)
> hcd_buffer_create() creates a few buffers, the smallest has 32 bytes of
> size. ARCH_KMALLOC_MINALIGN is set to 64 bytes. This combo results in
> hcd_buffer_alloc() returning memory which is 32 bytes aligned and it
> might by identified by buffer_offset() as another buffer. This means the
> buffer which is on a 32 byte boundary will not get freed, instead it
> tries to free another buffer with the error message.
>
> This patch fixes the issue by creating the smallest DMA buffer with the
> size of ARCH_KMALLOC_MINALIGN (or 32 in case ARCH_KMALLOC_MINALIGN is smaller).
> This might be 32, 64 or even 128 bytes. The next three pools will have
> the size 128, 512 and 2048.
> In case the smallest pool is 128 bytes then we have only three pools
> instead of four.
> The last pool size is always 2048 bytes which is the assumed PAGE_SIZE /
> 2 of 4096. I doubt it makes sense to continue using PAGE_SIZE / 2 where
> we would end up with 8KiB buffer in case we have 16KiB pages.
> Instead I think it makes sense to have a common size(s) and extend them
> if there is need to.
> There is a BUILD_BUG_ON() now in case someone has a minalign of more than
> 128 bytes.
>
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx>
> ---
> v2…v3:
> - rewrite and use usb_init_pool_max() instead. Albeit the value
> __alignof__(x) is known at compile time it can't be used in #if
> statement by the CPP. The max_t and if statment in this patch is
> optimized away by the compiler
> - replace PAGE_SIZE / 2 by 2048.
>
> v1…v2: rewrite pool_max so it is less confusing.
>
> drivers/usb/core/buffer.c | 31 ++++++++++++++++++++++---------
> drivers/usb/core/usb.c | 1 +
> include/linux/usb/hcd.h | 1 +
> 3 files changed, 24 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/usb/core/buffer.c b/drivers/usb/core/buffer.c
> index 684ef70dc09d..8f58b88e9c6b 100644
> --- a/drivers/usb/core/buffer.c
> +++ b/drivers/usb/core/buffer.c
> @@ -22,17 +22,30 @@
> */
>
> /* FIXME tune these based on pool statistics ... */
> -static const size_t pool_max[HCD_BUFFER_POOLS] = {
> - /* platforms without dma-friendly caches might need to
> +static size_t pool_max[HCD_BUFFER_POOLS];
> +
> +void __init usb_init_pool_max(void)
> +{
> + unsigned pool_size = max_t(unsigned, ARCH_KMALLOC_MINALIGN, 32);
> + int i = 0;
> +
> + /*
> + * platforms without dma-friendly caches might need to
> * prevent cacheline sharing...
> + * MIN [1] [2] [3]
> + * 32 128 512 2048
> + * 64 128 512 2048
> + * 128 512 2048 -
> */
The stuff you added to this comment won't mean anything to somebody
unless they already know what's going on. The code below is also
confusing. How does this look instead?
static size_t pool_max[HCD_BUFFER_POOLS] = {
32, 128, 512, 2048;
}
...
/*
* The pool_max values must never be smaller than
* ARCH_KMALLOC_MINALIGN.
*/
if (ARCH_KMALLOC_MINALIGN <= 32)
; /* Original value is okay */
else if (ARCH_KMALLOC_MINALIGN <= 64)
pool_max[0] = 64;
else if (ARCH_KMALLOC_MINALIGN <= 128)
pool_max[0] = 0; /* Don't use this pool */
else
BUILD_BUG(); /* We don't allow this */
The compiler should eliminate most of that.
> - 32,
> - 128,
> - 512,
> - PAGE_SIZE / 2
> - /* bigger --> allocate pages */
> -};
> -
> + pool_max[i++] = pool_size;
> + if (pool_size < 128)
> + pool_max[i++] = 128;
> + pool_max[i++] = 512;
> + pool_max[i++] = 2048;
> +
> + /* MINALIGN > 128 hasn't been considered yet */
> + BUILD_BUG_ON(ARCH_KMALLOC_MINALIGN > 128);
> +}
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
