Re: [PATCHv6] drivers: optee: allow op-tee to access devices on the i2c bus

[Jorge Ramirez-Ortiz, Foundries]

On 05/08/20, Jens Wiklander wrote:
> On Wed, Aug 05, 2020 at 03:35:01PM +0200, Jorge Ramirez-Ortiz, Foundries wrote:
> > On 22/07/20, Jorge Ramirez-Ortiz wrote:
> > > Some secure elements like NXP's SE050 sit on I2C buses. For OP-TEE to
> > > control this type of cryptographic devices it needs coordinated access
> > > to the bus, so collisions and RUNTIME_PM dont get in the way.
> > > 
> > > This trampoline driver allow OP-TEE to access them.
> > > Tested on imx8mm LPDDR4
> > > 
> > > Signed-off-by: Jorge Ramirez-Ortiz <jorge@xxxxxxxxxxxx>
> > > ---
> > >  v6: compile out if CONFIG_I2C not enabled
> > >  v5: alphabetic order of includes
> > >  v4: remove unnecessary extra line in optee_msg.h
> > >  v3: use from/to msg param to support all types of memory
> > >      modify OPTEE_MSG_RPC_CMD_I2C_TRANSFER message id
> > >      
> > >  drivers/tee/optee/optee_msg.h | 16 +++++++
> > >  drivers/tee/optee/rpc.c       | 88 +++++++++++++++++++++++++++++++++++
> > >  2 files changed, 104 insertions(+)
> > > 
> > > diff --git a/drivers/tee/optee/optee_msg.h b/drivers/tee/optee/optee_msg.h
> > > index 795bc19ae17a..14b580f55356 100644
> > > --- a/drivers/tee/optee/optee_msg.h
> > > +++ b/drivers/tee/optee/optee_msg.h
> > > @@ -419,4 +419,20 @@ struct optee_msg_arg {
> > >   */
> > >  #define OPTEE_MSG_RPC_CMD_SHM_FREE	7
> > >  
> > > +/*
> > > + * Access a device on an i2c bus
> > > + *
> > > + * [in]  param[0].u.value.a		mode: RD(0), WR(1)
> > > + * [in]  param[0].u.value.b		i2c adapter
> > > + * [in]  param[0].u.value.c		i2c chip
> > > + *
> > > + * [in/out] memref[1]			buffer to exchange the transfer data
> > > + *					with the secure world
> > > + *
> > > + * [out]  param[0].u.value.a		bytes transferred by the driver
> > > + */
> > > +#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER 21
> > > +#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER_RD 0
> > > +#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER_WR 1
> > > +
> > >  #endif /* _OPTEE_MSG_H */
> > > diff --git a/drivers/tee/optee/rpc.c b/drivers/tee/optee/rpc.c
> > > index b4ade54d1f28..5fd5c6c93896 100644
> > > --- a/drivers/tee/optee/rpc.c
> > > +++ b/drivers/tee/optee/rpc.c
> > > @@ -7,6 +7,7 @@
> > >  
> > >  #include <linux/delay.h>
> > >  #include <linux/device.h>
> > > +#include <linux/i2c.h>
> > >  #include <linux/slab.h>
> > >  #include <linux/tee_drv.h>
> > >  #include "optee_private.h"
> > > @@ -49,6 +50,90 @@ static void handle_rpc_func_cmd_get_time(struct optee_msg_arg *arg)
> > >  	arg->ret = TEEC_ERROR_BAD_PARAMETERS;
> > >  }
> > >  
> > > +#if IS_ENABLED(CONFIG_I2C)
> > > +static void handle_rpc_func_cmd_i2c_transfer(struct tee_context *ctx,
> > > +					     struct optee_msg_arg *arg)
> > > +{
> > > +	struct i2c_client client;
> > > +	struct tee_param *params;
> > > +	uint32_t type;
> > > +	int i, ret;
> > > +	size_t len;
> > > +	char *buf;
> > > +	uint32_t attr[] = {
> > > +		TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT,
> > > +		TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT,
> > > +		TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT,
> > > +	};
> > > +
> > > +	if (arg->num_params != ARRAY_SIZE(attr)) {
> > > +		arg->ret = TEEC_ERROR_BAD_PARAMETERS;
> > > +		return;
> > > +	}
> > > +
> > > +	params = kmalloc_array(arg->num_params, sizeof(struct tee_param),
> > > +			       GFP_KERNEL);
> > > +	if (!params) {
> > > +		arg->ret = TEEC_ERROR_OUT_OF_MEMORY;
> > > +		return;
> > > +	}
> > > +
> > > +	if (optee_from_msg_param(params, arg->num_params, arg->params))
> > > +		goto bad;
> > > +
> > > +	for (i = 0; i < arg->num_params; i++) {
> > > +		type = params[i].attr & TEE_IOCTL_PARAM_ATTR_TYPE_MASK;
> > > +		if (type != attr[i])
> > > +			goto bad;
> > > +	}
> > > +
> > > +	client.addr = params[0].u.value.c;
> > > +	client.adapter = i2c_get_adapter(params[0].u.value.b);
> > > +	if (!client.adapter)
> > > +		goto bad;
> > > +
> > > +	snprintf(client.name, I2C_NAME_SIZE, "i2c%d", client.adapter->nr);
> > > +
> > > +	buf = params[1].u.memref.shm->kaddr;
> > > +	len = params[1].u.memref.size;
> > > +
> > > +	switch (params[0].u.value.a) {
> > > +	case OPTEE_MSG_RPC_CMD_I2C_TRANSFER_RD:
> > > +		ret = i2c_master_recv(&client, buf, len);
> > > +		break;
> > > +	case OPTEE_MSG_RPC_CMD_I2C_TRANSFER_WR:
> > > +		ret = i2c_master_send(&client, buf, len);
> > > +		break;
> > > +	default:
> > > +		i2c_put_adapter(client.adapter);
> > > +		goto bad;
> > > +	}
> > > +
> > > +	if (ret >= 0) {
> > > +		params[2].u.value.a = ret;
> > > +		arg->ret = TEEC_SUCCESS;
> > > +	} else {
> > > +		arg->ret = TEEC_ERROR_COMMUNICATION;
> > > +	}
> > > +
> > > +	if (optee_to_msg_param(arg->params, arg->num_params, params))
> > > +		arg->ret = TEEC_ERROR_BAD_PARAMETERS;
> > > +
> > > +	i2c_put_adapter(client.adapter);
> > > +	kfree(params);
> > > +	return;
> > > +bad:
> > > +	kfree(params);
> > > +	arg->ret = TEEC_ERROR_BAD_PARAMETERS;
> > > +}
> > > +#else
> > > +static void handle_rpc_func_cmd_i2c_transfer(struct tee_context *ctx,
> > > +					     struct optee_msg_arg *arg)
> > > +{
> > > +	arg->ret = TEEC_ERROR_COMMUNICATION;
> > > +}
> > > +#endif
> > > +
> > >  static struct wq_entry *wq_entry_get(struct optee_wait_queue *wq, u32 key)
> > >  {
> > >  	struct wq_entry *w;
> > > @@ -382,6 +467,9 @@ static void handle_rpc_func_cmd(struct tee_context *ctx, struct optee *optee,
> > >  	case OPTEE_MSG_RPC_CMD_SHM_FREE:
> > >  		handle_rpc_func_cmd_shm_free(ctx, arg);
> > >  		break;
> > > +	case OPTEE_MSG_RPC_CMD_I2C_TRANSFER:
> > > +		handle_rpc_func_cmd_i2c_transfer(ctx, arg);
> > > +		break;
> > >  	default:
> > >  		handle_rpc_supp_cmd(ctx, arg);
> > >  	}
> > 
> > 
> > any comments please?
> 
> As you know we're still reviewing the secure world counterpart at
> https://github.com/OP-TEE/optee_os/pull/3905

yep

> Where we're sorting out the ABI. Thanks for your patience.

after the initial comments on the first patch it was not clear why
there were none on the follow up ones. but sure, thanks for taking the
time.


> 
> Cheers,
> Jens