lkml.org 
[lkml]   [2012]   [Mar]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2] MM: CMA: add a simple kernel module as the helper to test CMA
Date
From
On Mon, 05 Mar 2012 08:13:46 +0100, Barry Song <Barry.Song@csr.com> wrote:
> Any write request to /dev/cma_test will let the module to allocate memory from
> CMA, for example:
>
> 1st time
> $ echo 1024 > /dev/cma_test
> will require cma_test to request 1MB(1024KB)
> 2nd time
> $ echo 2048 > /dev/cma_test
> will require cma_test to request 2MB(2048KB)
>
> Any read request to /dev/cma_test will let the module to free the 1st valid
> memory from CMA, for example:
>
> 1st time
> $ cat /dev/cma_test
> will require cma_test to free the 1MB allocated in the first write request
> 2nd time
> $ cat /dev/cma_test
> will require cma_test to free the 2MB allocated in the second write request
>
> Signed-off-by: Barry Song <Baohua.Song@csr.com>

> diff --git a/tools/cma/cma_test.c b/tools/cma/cma_test.c
> new file mode 100644
> index 0000000..46af250
> --- /dev/null
> +++ b/tools/cma/cma_test.c
> @@ -0,0 +1,140 @@
> +/*
> + * kernel module helper for testing CMA
> + *
> + * Copyright (c) 2011 Cambridge Silicon Radio Limited, a CSR plc group company.

It's 2012.

> + *
> + * Licensed under GPLv2 or later.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/fs.h>
> +#include <linux/miscdevice.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +
> +struct cma_allocation {
> + struct list_head list;
> + unsigned long size;
> + dma_addr_t dma;
> + void *virt;
> +};
> +
> +static struct device *cma_dev;
> +static LIST_HEAD(cma_allocations);
> +static DEFINE_SPINLOCK(cma_lock);
> +
> +/*
> + * any read request will free the 1st allocated coherent memory, eg.
> + * cat /dev/cma_test
> + */
> +static ssize_t
> +cma_test_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
> +{
> + struct cma_allocation *alloc = NULL;
> +
> + spin_lock(&cma_lock);
> + if (!list_empty(&cma_allocations)) {
> + alloc = list_first_entry(&cma_allocations,
> + struct cma_allocation, list);
> + list_del(&alloc->list);
> + }
> + spin_unlock(&cma_lock);
> +

Come to think of it, how about putting:

if (!alloc)
return -EIDRM;

here and then removing one indention level later:

> + if (alloc) {
> + dma_free_coherent(cma_dev, alloc->size, alloc->virt,
> + alloc->dma);
> +
> + _dev_info(cma_dev, "free CM at virtual address: 0x%p dma address: 0x%p size:%luKiB\n",

This message seem overly long and “0x” seem redundant. How about something like:

_dev_info(cma_dev, "free: virt:%p dma:%p size:%luK\n",

which has all the same information but is shorter?

> + alloc->virt, (void *)alloc->dma, alloc->size / SZ_1K);
> + kfree(alloc);
> + }
> +
> + return 0;
> +}
> +
> +/*
> + * any write request will alloc a new coherent memory, eg.
> + * echo 1024 > /dev/cma_test
> + * will request 1024KiB by CMA
> + */
> +static ssize_t
> +cma_test_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
> +{
> + struct cma_allocation *alloc;
> + int ret;
> +
> + alloc = kmalloc(sizeof *alloc, GFP_KERNEL);
> + if (!alloc)
> + return -ENOMEM;
> +
> + ret = kstrtoul_from_user(buf, count, 0, &alloc->size);
> + if (ret)
> + return ret;
> +
> + if (!alloc->size)
> + return -EINVAL;
> +
> + if (alloc->size > (ULONG_MAX << PAGE_SHIFT))

You've changed units from pages to KiBs and, I've just realised that, since
dma_alloc_coherent accepts “size_t” as argument, this should read:

if (alloc->size > ~(size_t)0 / SZ_1K)

(“<<” was in fact a bug in my code from the beginning.)

> + return -EOVERFLOW;

Most importantly, there was a memory leak in my original suggestion, and
it got carried over to this patch. The above code should look
something like that:

ret = kstrtoul_from_user(buf, count, 0, &alloc->size);
if (ret)
return ret;

if (!alloc->size)
return -EINVAL;

if (alloc->size > ~(size_t)0 / SZ_1K)
return -EOVERFLOW;

alloc = kmalloc(sizeof *alloc, GFP_KERNEL);
if (!alloc)
return -ENOMEM;

> + alloc->size *= SZ_1K;
> + alloc->virt = dma_alloc_coherent(cma_dev, alloc->size,
> + &alloc->dma, GFP_KERNEL);
> +
> + if (alloc->virt) {
> + _dev_info(cma_dev, "allocate CM at virtual address: 0x%p"
> + "address: 0x%p size:%luKiB\n", alloc->virt,

Similarly to message earlier, how about:

_dev_info(cma_dev, "alloc: virt:%p dma:%p size:%luK\n",

> + (void *)alloc->dma, alloc->size / SZ_1K);
> +
> + spin_lock(&cma_lock);
> + list_add_tail(&alloc->list, &cma_allocations);
> + spin_unlock(&cma_lock);
> +
> + return count;
> + } else {
> + dev_err(cma_dev, "no mem in CMA area\n");
> + kfree(alloc);
> + return -ENOSPC;
> + }
> +}
> +
> +static const struct file_operations cma_test_fops = {
> + .owner = THIS_MODULE,
> + .read = cma_test_read,
> + .write = cma_test_write,
> +};
> +
> +static struct miscdevice cma_test_misc = {
> + .name = "cma_test",
> + .fops = &cma_test_fops,
> +};
> +
> +static int __init cma_test_init(void)
> +{
> + int ret = 0;

Drop “= 0”, or better yet, combain this declaration with the next line.

> +
> + ret = misc_register(&cma_test_misc);
> + if (unlikely(ret)) {
> + pr_err("failed to register cma test misc device!\n");
> + return ret;
> + }
> + cma_dev = cma_test_misc.this_device;
> + cma_dev->coherent_dma_mask = ~0;
> + _dev_info(cma_dev, "registered.\n");
> +
> + return ret;

return 0;

> +}
> +module_init(cma_test_init);
> +
> +static void __exit cma_test_exit(void)
> +{
> + misc_deregister(&cma_test_misc);
> +}
> +module_exit(cma_test_exit);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Barry Song <Baohua.Song@csr.com>");
> +MODULE_DESCRIPTION("kernel module to help the test of CMA");
> +MODULE_ALIAS("CMA test");

Can module alias contain spaces? I don't think this declaration even
adds anything useful. “cma_test” as module name should be good enough.

--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michał “mina86” Nazarewicz (o o)
ooo +----<email/xmpp: mpn@google.com>--------------ooO--(_)--Ooo--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2012-03-05 10:43    [W:0.210 / U:0.064 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site