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.

[...]

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?

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