On 06/01/2016 07:41 AM, Jens Wiklander wrote: few minor comments below. I see the patch generated (with --strict): > CHECK: Alignment should match open parenthesis > #512: FILE: drivers/tee/tee.c:375: > +static int tee_ioctl_close_session(struct tee_context *ctx, > + struct tee_ioctl_close_session_arg __user *uarg) > CHECK: Alignment should match open parenthesis > #1607: FILE: drivers/tee/tee_shm_pool.c:103: > +struct tee_shm_pool *tee_shm_pool_alloc_res_mem(struct device *dev, > + struct tee_shm_pool_mem_info *priv_info, > CHECK: Alignment should match open parenthesis > #1789: FILE: include/linux/tee_drv.h:124: > +struct tee_device *tee_device_alloc(const struct tee_desc *teedesc, > + struct device *dev, struct tee_shm_pool *pool, > WARNING: line over 80 characters > #1814: FILE: include/linux/tee_drv.h:149: > + * struct tee_shm_pool_mem_info - holds information needed to create a > shared memory pool > WARNING: line over 80 characters > #1826: FILE: include/linux/tee_drv.h:161: > + * tee_shm_pool_alloc_res_mem() - Create a shared memory pool from reserved > memory range > CHECK: Alignment should match open parenthesis > #1839: FILE: include/linux/tee_drv.h:174: > +struct tee_shm_pool *tee_shm_pool_alloc_res_mem(struct device *dev, > > + struct tee_shm_pool_mem_info *priv_info, > CHECK: Prefer using the BIT macro > #1995: FILE: include/uapi/linux/tee.h:51: > +#define TEE_GEN_CAP_GP (1 << 0)/* GlobalPlatform compliant > TEE */ > CHECK: Prefer using the BIT macro > #2005: FILE: include/uapi/linux/tee.h:61: > +#define TEE_OPTEE_CAP_TZ (1 << 0) > WARNING: line over 80 characters > #2205: FILE: include/uapi/linux/tee.h:261: > + * struct tee_ioctl_invoke_func_arg - Invokes a function in a Trusted > Application > mechanically convert to the typical style using --fix or --fix-inplace. > them to the maintainer, see CHECKPATCH in MAINTAINERS.
Might be nice to fix them up. [...] > diff --git a/drivers/tee/Kconfig b/drivers/tee/Kconfig > new file mode 100644 > index 0000000..f3ba154 > --- /dev/null > +++ b/drivers/tee/Kconfig > @@ -0,0 +1,9 @@ > +# Generic Trusted Execution Environment Configuration > +config TEE > + bool "Trusted Execution Environment support" Why could not this be tristate? we would like to enforce subsystem init? > + default n You should not need this. (default is n) > + select DMA_SHARED_BUFFER > + select GENERIC_ALLOCATOR select or depends? > + help > + This implements a generic interface towards a Trusted Execution > + Environment (TEE). > diff --git a/drivers/tee/Makefile b/drivers/tee/Makefile > new file mode 100644 > index 0000000..60d2dab > --- /dev/null > +++ b/drivers/tee/Makefile > @@ -0,0 +1,3 @@ > +obj-y += tee.o > +obj-y += tee_shm.o > +obj-y += tee_shm_pool.o > diff --git a/drivers/tee/tee.c b/drivers/tee/tee.c > new file mode 100644 > index 0000000..119e18e > --- /dev/null > +++ b/drivers/tee/tee.c > @@ -0,0 +1,877 @@ > +/* > + * Copyright (c) 2015-2016, Linaro Limited > + * > + * This software is licensed under the terms of the GNU General Public > + * License version 2, as published by the Free Software Foundation, and > + * may be copied, distributed, and modified under those terms. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + */ Adding a #define pr_fmt(fmt) "%s: " fmt, __func__ might help might help give reasonable errors where pr_* is used. > +#include <linux/cdev.h> > +#include <linux/device.h> > +#include <linux/fs.h> > +#include <linux/idr.h> > +#include <linux/slab.h> > +#include <linux/tee_drv.h> > +#include <linux/uaccess.h> > +#include "tee_private.h" > + > +#define TEE_NUM_DEVICES 32 I have a personal allergy to MAX_* macros, so I wonder if idr can help us get rid of fixed size tables? I wonder if the use should be limited to tee_shm.c ? static DEFINE_IDR(tee_id); .... teedev->id = idr_alloc(&tee_id, teedev, 0, 0, GFP_KERNEL); or something similar? > + > +#define TEE_IOCTL_PARAM_SIZE(x) (sizeof(struct tee_param) * (x)) > + > +/* > + * Unprivileged devices in the in the lower half range and privileged > + * devices in the upper half range. > + */ > +static DECLARE_BITMAP(dev_mask, TEE_NUM_DEVICES); > +static DEFINE_SPINLOCK(driver_lock); I think you might be able to get rid of the above two with idr usage. > + > +static struct class *tee_class; > +static dev_t tee_devt; > + > +static int tee_open(struct inode *inode, struct file *filp) > +{ > + int rc; > + struct tee_device *teedev; > + struct tee_context *ctx; > + > + teedev = container_of(inode->i_cdev, struct tee_device, cdev); > + if (!tee_device_get(teedev)) > + return -EINVAL; > + > + ctx = kzalloc(sizeof(*ctx), GFP_KERNEL); > + if (!ctx) { > + rc = -ENOMEM; > + goto err; > + } > + > + ctx->teedev = teedev; > + filp->private_data = ctx; I wonder if the teedev was a module, could it be removed / unregistered after tee_open was invoked? > +static int tee_ioctl_invoke(struct tee_context *ctx, > + struct tee_ioctl_buf_data __user *ubuf) > +{ > + int rc; > + size_t n; > + struct tee_ioctl_buf_data buf; > + struct tee_ioctl_invoke_arg __user *uarg; > + struct tee_ioctl_invoke_arg arg; > + struct tee_ioctl_param __user *uparams = NULL; > + struct tee_param *params = NULL; > + > + if (!ctx->teedev->desc->ops->invoke_func) > + return -EINVAL; > + > + rc = copy_from_user(&buf, ubuf, sizeof(buf)); > + if (rc) > + return rc; > + > + if (buf.buf_len > TEE_MAX_ARG_SIZE || > + buf.buf_len < sizeof(struct tee_ioctl_invoke_arg)) > + return -EINVAL; > + > + uarg = (struct tee_ioctl_invoke_arg __user *)(unsigned long)buf.buf_ptr; > + if (copy_from_user(&arg, uarg, sizeof(arg))) > + return -EFAULT; > + > + if (sizeof(arg) + TEE_IOCTL_PARAM_SIZE(arg.num_params) != buf.buf_len) > + return -EINVAL; > + > + if (arg.num_params) { > + params = kcalloc(arg.num_params, sizeof(struct tee_param), > + GFP_KERNEL); > + if (!params) > + return -ENOMEM; > + uparams = (struct tee_ioctl_param __user *)(uarg + 1); > + rc = params_from_user(ctx, params, arg.num_params, uparams); > + if (rc) > + goto out; > + } > + > + rc = ctx->teedev->desc->ops->invoke_func(ctx, &arg, params); > + if (rc) > + goto out; Hmm.. I wonder if the teedev drivers should get subsystem level lock protection for ops invocation or should they implement locking themselves? [...] -- Regards, Nishanth Menon