On 10/05/2015 03:11 AM, Benjamin Gaignard wrote: > diff --git a/drivers/smaf/smaf-core.c b/drivers/smaf/smaf-core.c > new file mode 100644 > index 0000000..37914e7 > --- /dev/null > +++ b/drivers/smaf/smaf-core.c > @@ -0,0 +1,736 @@ > +/* > + * smaf.c > + * > + * Copyright (C) Linaro SA 2015 > + * Author: Benjamin Gaignard <benjamin.gaignard at linaro.org> for Linaro. > + * License terms: GNU General Public License (GPL), version 2 > + */ > + > +#include <linux/device.h> > +#include <linux/dma-buf.h> > +#include <linux/dma-mapping.h> > +#include <linux/fs.h> > +#include <linux/ioctl.h> > +#include <linux/list_sort.h> > +#include <linux/miscdevice.h> > +#include <linux/module.h> > +#include <linux/slab.h> > +#include <linux/smaf.h> > +#include <linux/smaf-allocator.h> > +#include <linux/smaf-secure.h> > +#include <linux/uaccess.h> > + > +struct smaf_handle { > + struct dma_buf *dmabuf; > + struct smaf_allocator *allocator; > + struct dma_buf *db_alloc; > + size_t length; > + unsigned int flags; > + int fd; > + bool is_secure; > + void *secure_ctx; > +}; > + > +/** > + * struct smaf_device - smaf device node private data > + * @misc_dev: the misc device > + * @head: list of allocator > + * @lock: list and secure pointer mutex > + * @secure: pointer to secure functions helpers > + */ > +struct smaf_device { > + struct miscdevice misc_dev; > + struct list_head head; > + /* list and secure pointer lock*/ > + struct mutex lock; > + struct smaf_secure *secure; > +}; > + > +static struct smaf_device smaf_dev; > + > +/** > + * smaf_allow_cpu_access return true if CPU can access to memory > + * if their is no secure module associated to SMAF assume that CPU can get > + * access to the memory. > + */ > +static bool smaf_allow_cpu_access(struct smaf_handle *handle, > + unsigned long flags) > +{ > + if (!handle->is_secure) > + return true; > + > + if (!smaf_dev.secure) > + return true; > + > + if (!smaf_dev.secure->allow_cpu_access) > + return true; > + > + return smaf_dev.secure->allow_cpu_access(handle->secure_ctx, flags); > +} > + > +static int smaf_grant_access(struct smaf_handle *handle, struct device *dev, > + dma_addr_t addr, size_t size, > + enum dma_data_direction dir) > +{ > + if (!handle->is_secure) > + return 0; > + > + if (!smaf_dev.secure) > + return -EINVAL; > + > + if (!smaf_dev.secure->grant_access) > + return -EINVAL; > + > + return smaf_dev.secure->grant_access(handle->secure_ctx, > + dev, addr, size, dir); > +} > + > +static void smaf_revoke_access(struct smaf_handle *handle, struct device > *dev, > + dma_addr_t addr, size_t size, > + enum dma_data_direction dir) > +{ > + if (!handle->is_secure) > + return; > + > + if (!smaf_dev.secure) > + return; > + > + if (!smaf_dev.secure->revoke_access) > + return; > + > + smaf_dev.secure->revoke_access(handle->secure_ctx, > + dev, addr, size, dir); > +} > + > +static int smaf_secure_handle(struct smaf_handle *handle) > +{ > + if (handle->is_secure) > + return 0; > + > + if (!smaf_dev.secure) > + return -EINVAL; > + > + if (!smaf_dev.secure->create_context) > + return -EINVAL; > + > + handle->secure_ctx = smaf_dev.secure->create_context(); > + > + if (!handle->secure_ctx) > + return -EINVAL; > + > + handle->is_secure = true; > + return 0; > +} > + > +static int smaf_unsecure_handle(struct smaf_handle *handle) > +{ > + if (!handle->is_secure) > + return 0; > + > + if (!smaf_dev.secure) > + return -EINVAL; > + > + if (!smaf_dev.secure->destroy_context) > + return -EINVAL; > + > + if (smaf_dev.secure->destroy_context(handle->secure_ctx)) > + return -EINVAL; > + > + handle->secure_ctx = NULL; > + handle->is_secure = false; > + return 0; > +}
All these functions need to be protected by a lock, otherwise the secure state could change. For that matter, I think the smaf_handle needs a lock to protect its state as well for places like map_dma_buf > <snip> > +static long smaf_ioctl(struct file *file, unsigned int cmd, unsigned long > arg) > +{ > + switch (cmd) { > + case SMAF_IOC_CREATE: > + { > + struct smaf_create_data data; > + struct smaf_handle *handle; > + > + if (copy_from_user(&data, (void __user *)arg, _IOC_SIZE(cmd))) > + return -EFAULT; > + > + handle = smaf_create_handle(data.length, data.flags); > + if (!handle) > + return -EINVAL; > + > + if (data.name[0]) { > + /* user force allocator selection */ > + if (smaf_select_allocator_by_name(handle->dmabuf, > + data.name)) { > + dma_buf_put(handle->dmabuf); > + return -EINVAL; > + } > + } > + > + handle->fd = dma_buf_fd(handle->dmabuf, data.flags); > + if (handle->fd < 0) { > + dma_buf_put(handle->dmabuf); > + return -EINVAL; > + } > + > + data.fd = handle->fd; > + if (copy_to_user((void __user *)arg, &data, _IOC_SIZE(cmd))) { > + dma_buf_put(handle->dmabuf); > + return -EFAULT; > + } > + break; > + } > + case SMAF_IOC_GET_SECURE_FLAG: > + { > + struct smaf_secure_flag data; > + struct dma_buf *dmabuf; > + > + if (copy_from_user(&data, (void __user *)arg, _IOC_SIZE(cmd))) > + return -EFAULT; > + > + dmabuf = dma_buf_get(data.fd); > + if (!dmabuf) > + return -EINVAL; > + > + data.secure = smaf_is_secure(dmabuf); > + dma_buf_put(dmabuf); > + > + if (copy_to_user((void __user *)arg, &data, _IOC_SIZE(cmd))) > + return -EFAULT; > + break; > + } > + case SMAF_IOC_SET_SECURE_FLAG: > + { > + struct smaf_secure_flag data; > + struct dma_buf *dmabuf; > + int ret; > + > + if (!smaf_dev.secure) > + return -EINVAL; > + > + if (copy_from_user(&data, (void __user *)arg, _IOC_SIZE(cmd))) > + return -EFAULT; > + > + dmabuf = dma_buf_get(data.fd); > + if (!dmabuf) > + return -EINVAL; > + > + ret = smaf_set_secure(dmabuf, data.secure); > + > + dma_buf_put(dmabuf); > + > + if (ret) > + return -EINVAL; > + > + break; > + } > + default: > + return -EINVAL; > + } > + > + return 0; > +} How would you see this tying into something like Ion? It seems like Ion and SMAF have non-zero over lapping functionality for some things or that SMAF could be implemented as a heap type. I think my biggest concern here is that it seems like either Ion or SMAF is going to feel redundant as an interface. Thanks, Laura