diff mbox

[v7,2/3] SMAF: add CMA allocator

Message ID 1462806459-8124-3-git-send-email-benjamin.gaignard@linaro.org (mailing list archive)
State New, archived
Headers show

Commit Message

Benjamin Gaignard May 9, 2016, 3:07 p.m. UTC
SMAF CMA allocator implement helpers functions to allow SMAF
to allocate contiguous memory.

match() each if at least one of the attached devices have coherent_dma_mask
set to DMA_BIT_MASK(32).

For allocation it use dma_alloc_attrs() with DMA_ATTR_WRITE_COMBINE and not
dma_alloc_writecombine to be compatible with ARM 64bits architecture

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
---
 drivers/smaf/Kconfig    |   6 ++
 drivers/smaf/Makefile   |   1 +
 drivers/smaf/smaf-cma.c | 199 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 206 insertions(+)
 create mode 100644 drivers/smaf/smaf-cma.c

Comments

Emil Velikov May 16, 2016, 11:05 p.m. UTC | #1
Hi Benjamin,

On 9 May 2016 at 16:07, Benjamin Gaignard <benjamin.gaignard@linaro.org> wrote:
> SMAF CMA allocator implement helpers functions to allow SMAF
> to allocate contiguous memory.
>
> match() each if at least one of the attached devices have coherent_dma_mask
> set to DMA_BIT_MASK(32).
>
What is the idea behind the hardcoded 32. Wouldn't it be better to avoid that ?


> +static void smaf_cma_release(struct dma_buf *dmabuf)
> +{
> +       struct smaf_cma_buffer_info *info = dmabuf->priv;
> +       DEFINE_DMA_ATTRS(attrs);
> +
> +       dma_set_attr(DMA_ATTR_WRITE_COMBINE, &attrs);
> +
Imho it's worth storing the dma_attrs within smaf_cma_buffer_info.
This way it's less likely for things to go wrong, if one forgets to
update one of the three in the future.


> +static void smaf_cma_unmap(struct dma_buf_attachment *attachment,
> +                          struct sg_table *sgt,
> +                          enum dma_data_direction direction)
> +{
> +       /* do nothing */
There could/should really be a comment explaining why we "do nothing"
here, right ?

> +}
> +
> +static int smaf_cma_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma)
> +{
> +       struct smaf_cma_buffer_info *info = dmabuf->priv;
> +       int ret;
> +       DEFINE_DMA_ATTRS(attrs);
> +
> +       dma_set_attr(DMA_ATTR_WRITE_COMBINE, &attrs);
> +
> +       if (info->size < vma->vm_end - vma->vm_start)
> +               return -EINVAL;
> +
> +       vma->vm_flags |= VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP;
> +       ret = dma_mmap_attrs(info->dev, vma, info->vaddr, info->paddr,
> +                            info->size, &attrs);
> +
> +       return ret;
Kill the temporary variable 'ret' ?


> +static struct dma_buf_ops smaf_cma_ops = {
const ? Afaict the compiler would/should warn you about discarding it
as the ops are defined const.


> +static struct dma_buf *smaf_cma_allocate(struct dma_buf *dmabuf,
> +                                        size_t length, unsigned int flags)
> +{
> +       struct dma_buf_attachment *attach_obj;
> +       struct smaf_cma_buffer_info *info;
> +       struct dma_buf *cma_dmabuf;
> +       int ret;
> +
> +       DEFINE_DMA_BUF_EXPORT_INFO(export);
> +       DEFINE_DMA_ATTRS(attrs);
> +
> +       dma_set_attr(DMA_ATTR_WRITE_COMBINE, &attrs);
> +
> +       info = kzalloc(sizeof(*info), GFP_KERNEL);
> +       if (!info)
> +               return NULL;
> +
> +       info->dev = find_matching_device(dmabuf);
find_matching_device() can return NULL. We should handle that imho.

> +       info->size = length;
> +       info->vaddr = dma_alloc_attrs(info->dev, info->size, &info->paddr,
> +                                     GFP_KERNEL | __GFP_NOWARN, &attrs);
> +       if (!info->vaddr) {
> +               ret = -ENOMEM;
set-but-unused-variable 'ret' ?

> +               goto error;
> +       }
> +
> +       export.ops = &smaf_cma_ops;
> +       export.size = info->size;
> +       export.flags = flags;
> +       export.priv = info;
> +
> +       cma_dmabuf = dma_buf_export(&export);
> +       if (IS_ERR(cma_dmabuf))
Missing dma_free_attrs() ? I'd add another label in the error path and
handle it there.

> +               goto error;
> +
> +       list_for_each_entry(attach_obj, &dmabuf->attachments, node) {
> +               dma_buf_attach(cma_dmabuf, attach_obj->dev);
Imho one should error out if attach fails. Or warn at the very least ?


> +static int __init smaf_cma_init(void)
> +{
> +       INIT_LIST_HEAD(&smaf_cma.list_node);
Isn't this something that smaf_register_allocator() should be doing ?


Regards,
Emil
Benjamin Gaignard May 17, 2016, 2:55 p.m. UTC | #2
Hi Emil,

2016-05-17 1:05 GMT+02:00 Emil Velikov <emil.l.velikov@gmail.com>:
> Hi Benjamin,
>
> On 9 May 2016 at 16:07, Benjamin Gaignard <benjamin.gaignard@linaro.org> wrote:
>> SMAF CMA allocator implement helpers functions to allow SMAF
>> to allocate contiguous memory.
>>
>> match() each if at least one of the attached devices have coherent_dma_mask
>> set to DMA_BIT_MASK(32).
>>
> What is the idea behind the hardcoded 32. Wouldn't it be better to avoid that ?
>

Device dma_bit_mask field has to be set to DMA_BIT_MASK(32) to target
a CMA area.
I haven't see any other #define for that.

>
>> +static void smaf_cma_release(struct dma_buf *dmabuf)
>> +{
>> +       struct smaf_cma_buffer_info *info = dmabuf->priv;
>> +       DEFINE_DMA_ATTRS(attrs);
>> +
>> +       dma_set_attr(DMA_ATTR_WRITE_COMBINE, &attrs);
>> +
> Imho it's worth storing the dma_attrs within smaf_cma_buffer_info.
> This way it's less likely for things to go wrong, if one forgets to
> update one of the three in the future.

Here I have duplicate what is done everywhere else but I could try to
add it into
smaf_cma_buffer_info structure.

>
>> +static void smaf_cma_unmap(struct dma_buf_attachment *attachment,
>> +                          struct sg_table *sgt,
>> +                          enum dma_data_direction direction)
>> +{
>> +       /* do nothing */
> There could/should really be a comment explaining why we "do nothing"
> here, right ?
>

I haven't used DMA_ATTR_NO_KERNEL_MAPPING while allocating the buffer
so kernel mapping is set by default and I don't have to manage map refcounting.

>> +}
>> +
>> +static int smaf_cma_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma)
>> +{
>> +       struct smaf_cma_buffer_info *info = dmabuf->priv;
>> +       int ret;
>> +       DEFINE_DMA_ATTRS(attrs);
>> +
>> +       dma_set_attr(DMA_ATTR_WRITE_COMBINE, &attrs);
>> +
>> +       if (info->size < vma->vm_end - vma->vm_start)
>> +               return -EINVAL;
>> +
>> +       vma->vm_flags |= VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP;
>> +       ret = dma_mmap_attrs(info->dev, vma, info->vaddr, info->paddr,
>> +                            info->size, &attrs);
>> +
>> +       return ret;
> Kill the temporary variable 'ret' ?

sure
>
>
>> +static struct dma_buf_ops smaf_cma_ops = {
> const ? Afaict the compiler would/should warn you about discarding it
> as the ops are defined const.
>
>
>> +static struct dma_buf *smaf_cma_allocate(struct dma_buf *dmabuf,
>> +                                        size_t length, unsigned int flags)
>> +{
>> +       struct dma_buf_attachment *attach_obj;
>> +       struct smaf_cma_buffer_info *info;
>> +       struct dma_buf *cma_dmabuf;
>> +       int ret;
>> +
>> +       DEFINE_DMA_BUF_EXPORT_INFO(export);
>> +       DEFINE_DMA_ATTRS(attrs);
>> +
>> +       dma_set_attr(DMA_ATTR_WRITE_COMBINE, &attrs);
>> +
>> +       info = kzalloc(sizeof(*info), GFP_KERNEL);
>> +       if (!info)
>> +               return NULL;
>> +
>> +       info->dev = find_matching_device(dmabuf);
> find_matching_device() can return NULL. We should handle that imho.
>

If the returned device have an associated CMA area then it will use it else
if dev have not CMA area or if find_matching_device() return NULL
the default CMA area will be used

>> +       info->size = length;
>> +       info->vaddr = dma_alloc_attrs(info->dev, info->size, &info->paddr,
>> +                                     GFP_KERNEL | __GFP_NOWARN, &attrs);
>> +       if (!info->vaddr) {
>> +               ret = -ENOMEM;
> set-but-unused-variable 'ret' ?
>
I will remove it

>> +               goto error;
>> +       }
>> +
>> +       export.ops = &smaf_cma_ops;
>> +       export.size = info->size;
>> +       export.flags = flags;
>> +       export.priv = info;
>> +
>> +       cma_dmabuf = dma_buf_export(&export);
>> +       if (IS_ERR(cma_dmabuf))
> Missing dma_free_attrs() ? I'd add another label in the error path and
> handle it there.
>
OK

>> +               goto error;
>> +
>> +       list_for_each_entry(attach_obj, &dmabuf->attachments, node) {
>> +               dma_buf_attach(cma_dmabuf, attach_obj->dev);
> Imho one should error out if attach fails. Or warn at the very least ?
>
>
>> +static int __init smaf_cma_init(void)
>> +{
>> +       INIT_LIST_HEAD(&smaf_cma.list_node);
> Isn't this something that smaf_register_allocator() should be doing ?
>

Yes for sure

>
> Regards,
> Emil
diff mbox

Patch

diff --git a/drivers/smaf/Kconfig b/drivers/smaf/Kconfig
index d36651a..058ec4c 100644
--- a/drivers/smaf/Kconfig
+++ b/drivers/smaf/Kconfig
@@ -3,3 +3,9 @@  config SMAF
 	depends on DMA_SHARED_BUFFER
 	help
 	  Choose this option to enable Secure Memory Allocation Framework
+
+config SMAF_CMA
+	tristate "SMAF CMA allocator"
+	depends on SMAF && HAVE_DMA_ATTRS
+	help
+	  Choose this option to enable CMA allocation within SMAF
diff --git a/drivers/smaf/Makefile b/drivers/smaf/Makefile
index 40cd882..05bab01b 100644
--- a/drivers/smaf/Makefile
+++ b/drivers/smaf/Makefile
@@ -1 +1,2 @@ 
 obj-$(CONFIG_SMAF) += smaf-core.o
+obj-$(CONFIG_SMAF_CMA) += smaf-cma.o
diff --git a/drivers/smaf/smaf-cma.c b/drivers/smaf/smaf-cma.c
new file mode 100644
index 0000000..6366777
--- /dev/null
+++ b/drivers/smaf/smaf-cma.c
@@ -0,0 +1,199 @@ 
+/*
+ * smaf-cma.c
+ *
+ * Copyright (C) Linaro SA 2015
+ * Author: Benjamin Gaignard <benjamin.gaignard@linaro.org> for Linaro.
+ * License terms:  GNU General Public License (GPL), version 2
+ */
+
+#include <linux/dma-mapping.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/smaf-allocator.h>
+
+struct smaf_cma_buffer_info {
+	struct device *dev;
+	size_t size;
+	void *vaddr;
+	dma_addr_t paddr;
+};
+
+/**
+ * find_matching_device - iterate over the attached devices to find one
+ * with coherent_dma_mask correctly set to DMA_BIT_MASK(32).
+ * Matching device (if any) will be used to aim CMA area.
+ */
+static struct device *find_matching_device(struct dma_buf *dmabuf)
+{
+	struct dma_buf_attachment *attach_obj;
+
+	list_for_each_entry(attach_obj, &dmabuf->attachments, node) {
+		if (attach_obj->dev->coherent_dma_mask == DMA_BIT_MASK(32))
+			return attach_obj->dev;
+	}
+
+	return NULL;
+}
+
+/**
+ * smaf_cma_match - return true if at least one device has been found
+ */
+static bool smaf_cma_match(struct dma_buf *dmabuf)
+{
+	return !!find_matching_device(dmabuf);
+}
+
+static void smaf_cma_release(struct dma_buf *dmabuf)
+{
+	struct smaf_cma_buffer_info *info = dmabuf->priv;
+	DEFINE_DMA_ATTRS(attrs);
+
+	dma_set_attr(DMA_ATTR_WRITE_COMBINE, &attrs);
+
+	dma_free_attrs(info->dev, info->size, info->vaddr, info->paddr, &attrs);
+
+	kfree(info);
+}
+
+static struct sg_table *smaf_cma_map(struct dma_buf_attachment *attachment,
+				     enum dma_data_direction direction)
+{
+	struct smaf_cma_buffer_info *info = attachment->dmabuf->priv;
+	struct sg_table *sgt;
+	int ret;
+
+	sgt = kzalloc(sizeof(*sgt), GFP_KERNEL);
+	if (!sgt)
+		return NULL;
+
+	ret = dma_get_sgtable(info->dev, sgt, info->vaddr,
+			      info->paddr, info->size);
+	if (ret < 0)
+		goto out;
+
+	sg_dma_address(sgt->sgl) = info->paddr;
+	return sgt;
+
+out:
+	kfree(sgt);
+	return NULL;
+}
+
+static void smaf_cma_unmap(struct dma_buf_attachment *attachment,
+			   struct sg_table *sgt,
+			   enum dma_data_direction direction)
+{
+	/* do nothing */
+}
+
+static int smaf_cma_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma)
+{
+	struct smaf_cma_buffer_info *info = dmabuf->priv;
+	int ret;
+	DEFINE_DMA_ATTRS(attrs);
+
+	dma_set_attr(DMA_ATTR_WRITE_COMBINE, &attrs);
+
+	if (info->size < vma->vm_end - vma->vm_start)
+		return -EINVAL;
+
+	vma->vm_flags |= VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP;
+	ret = dma_mmap_attrs(info->dev, vma, info->vaddr, info->paddr,
+			     info->size, &attrs);
+
+	return ret;
+}
+
+static void *smaf_cma_vmap(struct dma_buf *dmabuf)
+{
+	struct smaf_cma_buffer_info *info = dmabuf->priv;
+
+	return info->vaddr;
+}
+
+static void *smaf_kmap_atomic(struct dma_buf *dmabuf, unsigned long offset)
+{
+	struct smaf_cma_buffer_info *info = dmabuf->priv;
+
+	return (void *)info->vaddr + offset;
+}
+
+static struct dma_buf_ops smaf_cma_ops = {
+	.map_dma_buf = smaf_cma_map,
+	.unmap_dma_buf = smaf_cma_unmap,
+	.mmap = smaf_cma_mmap,
+	.release = smaf_cma_release,
+	.kmap_atomic = smaf_kmap_atomic,
+	.kmap = smaf_kmap_atomic,
+	.vmap = smaf_cma_vmap,
+};
+
+static struct dma_buf *smaf_cma_allocate(struct dma_buf *dmabuf,
+					 size_t length, unsigned int flags)
+{
+	struct dma_buf_attachment *attach_obj;
+	struct smaf_cma_buffer_info *info;
+	struct dma_buf *cma_dmabuf;
+	int ret;
+
+	DEFINE_DMA_BUF_EXPORT_INFO(export);
+	DEFINE_DMA_ATTRS(attrs);
+
+	dma_set_attr(DMA_ATTR_WRITE_COMBINE, &attrs);
+
+	info = kzalloc(sizeof(*info), GFP_KERNEL);
+	if (!info)
+		return NULL;
+
+	info->dev = find_matching_device(dmabuf);
+	info->size = length;
+	info->vaddr = dma_alloc_attrs(info->dev, info->size, &info->paddr,
+				      GFP_KERNEL | __GFP_NOWARN, &attrs);
+	if (!info->vaddr) {
+		ret = -ENOMEM;
+		goto error;
+	}
+
+	export.ops = &smaf_cma_ops;
+	export.size = info->size;
+	export.flags = flags;
+	export.priv = info;
+
+	cma_dmabuf = dma_buf_export(&export);
+	if (IS_ERR(cma_dmabuf))
+		goto error;
+
+	list_for_each_entry(attach_obj, &dmabuf->attachments, node) {
+		dma_buf_attach(cma_dmabuf, attach_obj->dev);
+	}
+
+	return cma_dmabuf;
+
+error:
+	kfree(info);
+	return NULL;
+}
+
+static struct smaf_allocator smaf_cma = {
+	.match = smaf_cma_match,
+	.allocate = smaf_cma_allocate,
+	.name = "smaf-cma",
+	.ranking = 0,
+};
+
+static int __init smaf_cma_init(void)
+{
+	INIT_LIST_HEAD(&smaf_cma.list_node);
+	return smaf_register_allocator(&smaf_cma);
+}
+module_init(smaf_cma_init);
+
+static void __exit smaf_cma_deinit(void)
+{
+	smaf_unregister_allocator(&smaf_cma);
+}
+module_exit(smaf_cma_deinit);
+
+MODULE_DESCRIPTION("SMAF CMA module");
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Benjamin Gaignard <benjamin.gaignard@linaro.org>");