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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
