Re: [PATCH v2] MM: CMA: add a simple kernel module as the helper totest CMA

[Barry Song]

hi Michal,

2012/3/5 Michal Nazarewicz <mina86@xxxxxxxxxx>:
> On Mon, 05 Mar 2012 08:13:46 +0100, Barry Song <Barry.Song@xxxxxxx> 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@xxxxxxx>
>
>
>> 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;

ok. make sense. "Identifier removed"

>
> here and then removing one indention level later:
>

agree.

>
>> + Â Â Â 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?
>

ok. anyway it is trivial.

>
>> + Â Â Â Â Â Â Â Â Â Â Â 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)
>

ok.

> (â<<â 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;
>

in original codes, "kmalloc(sizeof *alloc, GFP_KERNEL);" is not freed.
better we define a temp size_t size

        kstrtoul_from_user(buf, count, 0, &size);
        ...
        if (!size)
                return -EINVAL;

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

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

       alloc->size = size;

>
>> + Â Â Â 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@xxxxxxx>");
>> +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.

ok.

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