linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] drivers: optee: i2c: add bus retry configuration
@ 2020-09-16 15:27 Jorge Ramirez-Ortiz
  2020-09-22 16:38 ` Jens Wiklander
  0 siblings, 1 reply; 6+ messages in thread
From: Jorge Ramirez-Ortiz @ 2020-09-16 15:27 UTC (permalink / raw)
  To: jorge, jens.wiklander, sumit.garg; +Cc: ricardo, mike, tee-dev, linux-kernel

Allow OP-TEE to specify the number of retries in the adaptor.

Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io>
---
 drivers/tee/optee/rpc.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/tee/optee/rpc.c b/drivers/tee/optee/rpc.c
index 1e3614e4798f..2d46a9ecb1de 100644
--- a/drivers/tee/optee/rpc.c
+++ b/drivers/tee/optee/rpc.c
@@ -58,6 +58,7 @@ static void handle_rpc_func_cmd_i2c_transfer(struct tee_context *ctx,
 	struct tee_param *params;
 	size_t i;
 	int ret = -EOPNOTSUPP;
+	int retries = 0;
 	u8 attr[] = {
 		TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT,
 		TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT,
@@ -102,12 +103,17 @@ static void handle_rpc_func_cmd_i2c_transfer(struct tee_context *ctx,
 	client.addr = params[0].u.value.c;
 	snprintf(client.name, I2C_NAME_SIZE, "i2c%d", client.adapter->nr);
 
+	/* cache the current value */
+	retries = client.adapter->retries;
+
 	switch (params[0].u.value.a) {
 	case OPTEE_MSG_RPC_CMD_I2C_TRANSFER_RD:
+		client.adapter->retries = params[1].u.value.b;
 		ret = i2c_master_recv(&client, params[2].u.memref.shm->kaddr,
 				      params[2].u.memref.size);
 		break;
 	case OPTEE_MSG_RPC_CMD_I2C_TRANSFER_WR:
+		client.adapter->retries = params[1].u.value.b;
 		ret = i2c_master_send(&client, params[2].u.memref.shm->kaddr,
 				      params[2].u.memref.size);
 		break;
@@ -126,6 +132,7 @@ static void handle_rpc_func_cmd_i2c_transfer(struct tee_context *ctx,
 			arg->ret = TEEC_SUCCESS;
 	}
 
+	client.adapter->retries = retries;
 	i2c_put_adapter(client.adapter);
 	kfree(params);
 	return;
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH] drivers: optee: i2c: add bus retry configuration
  2020-09-16 15:27 [PATCH] drivers: optee: i2c: add bus retry configuration Jorge Ramirez-Ortiz
@ 2020-09-22 16:38 ` Jens Wiklander
  2020-09-23 11:18   ` Jorge Ramirez-Ortiz, Foundries
  0 siblings, 1 reply; 6+ messages in thread
From: Jens Wiklander @ 2020-09-22 16:38 UTC (permalink / raw)
  To: Jorge Ramirez-Ortiz
  Cc: sumit.garg, ricardo, mike, tee-dev, linux-kernel, op-tee

On Wed, Sep 16, 2020 at 05:27:32PM +0200, Jorge Ramirez-Ortiz wrote:
> Allow OP-TEE to specify the number of retries in the adaptor.
> 
> Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io>
> ---
>  drivers/tee/optee/rpc.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/tee/optee/rpc.c b/drivers/tee/optee/rpc.c
> index 1e3614e4798f..2d46a9ecb1de 100644
> --- a/drivers/tee/optee/rpc.c
> +++ b/drivers/tee/optee/rpc.c
> @@ -58,6 +58,7 @@ static void handle_rpc_func_cmd_i2c_transfer(struct tee_context *ctx,
>  	struct tee_param *params;
>  	size_t i;
>  	int ret = -EOPNOTSUPP;
> +	int retries = 0;
>  	u8 attr[] = {
>  		TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT,
>  		TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT,
> @@ -102,12 +103,17 @@ static void handle_rpc_func_cmd_i2c_transfer(struct tee_context *ctx,
>  	client.addr = params[0].u.value.c;
>  	snprintf(client.name, I2C_NAME_SIZE, "i2c%d", client.adapter->nr);
>  
> +	/* cache the current value */
> +	retries = client.adapter->retries;
> +
>  	switch (params[0].u.value.a) {
>  	case OPTEE_MSG_RPC_CMD_I2C_TRANSFER_RD:
> +		client.adapter->retries = params[1].u.value.b;
Do we need to take any locks befor this?

Cheers,
Jens

>  		ret = i2c_master_recv(&client, params[2].u.memref.shm->kaddr,
>  				      params[2].u.memref.size);
>  		break;
>  	case OPTEE_MSG_RPC_CMD_I2C_TRANSFER_WR:
> +		client.adapter->retries = params[1].u.value.b;
>  		ret = i2c_master_send(&client, params[2].u.memref.shm->kaddr,
>  				      params[2].u.memref.size);
>  		break;
> @@ -126,6 +132,7 @@ static void handle_rpc_func_cmd_i2c_transfer(struct tee_context *ctx,
>  			arg->ret = TEEC_SUCCESS;
>  	}
>  
> +	client.adapter->retries = retries;
>  	i2c_put_adapter(client.adapter);
>  	kfree(params);
>  	return;
> -- 
> 2.17.1
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] drivers: optee: i2c: add bus retry configuration
  2020-09-22 16:38 ` Jens Wiklander
@ 2020-09-23 11:18   ` Jorge Ramirez-Ortiz, Foundries
  2020-09-23 11:26     ` Jorge Ramirez-Ortiz, Foundries
  0 siblings, 1 reply; 6+ messages in thread
From: Jorge Ramirez-Ortiz, Foundries @ 2020-09-23 11:18 UTC (permalink / raw)
  To: Jens Wiklander
  Cc: Jorge Ramirez-Ortiz, sumit.garg, ricardo, mike, tee-dev,
	linux-kernel, op-tee

On 22/09/20, Jens Wiklander wrote:
> On Wed, Sep 16, 2020 at 05:27:32PM +0200, Jorge Ramirez-Ortiz wrote:
> > Allow OP-TEE to specify the number of retries in the adaptor.
> > 
> > Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io>
> > ---
> >  drivers/tee/optee/rpc.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/drivers/tee/optee/rpc.c b/drivers/tee/optee/rpc.c
> > index 1e3614e4798f..2d46a9ecb1de 100644
> > --- a/drivers/tee/optee/rpc.c
> > +++ b/drivers/tee/optee/rpc.c
> > @@ -58,6 +58,7 @@ static void handle_rpc_func_cmd_i2c_transfer(struct tee_context *ctx,
> >  	struct tee_param *params;
> >  	size_t i;
> >  	int ret = -EOPNOTSUPP;
> > +	int retries = 0;
> >  	u8 attr[] = {
> >  		TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT,
> >  		TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT,
> > @@ -102,12 +103,17 @@ static void handle_rpc_func_cmd_i2c_transfer(struct tee_context *ctx,
> >  	client.addr = params[0].u.value.c;
> >  	snprintf(client.name, I2C_NAME_SIZE, "i2c%d", client.adapter->nr);
> >  
> > +	/* cache the current value */
> > +	retries = client.adapter->retries;
> > +
> >  	switch (params[0].u.value.a) {
> >  	case OPTEE_MSG_RPC_CMD_I2C_TRANSFER_RD:
> > +		client.adapter->retries = params[1].u.value.b;
> Do we need to take any locks befor this?

no I dont think so: there is no need for bus locks when requesting a
transfer via i2c_master_recv/send; the lock for the bus segment gets
taken later on, when the actual transfer hppens ( __i2c_transfer())

the functionality implemented in this function pretty much mimicks
what is done in the normal world via /dev/i2c-X
(drivers/i2c/i2c_dev.c)

 - i2cdev_read --> i2c_master_send
 - i2cdev->write -->i2c_master_recv

and now the retry count setup on the adaptor with this commit.

 - i2cdev_ioctl I2C_RETRIES

> 
> Cheers,
> Jens
> 
> >  		ret = i2c_master_recv(&client, params[2].u.memref.shm->kaddr,
> >  				      params[2].u.memref.size);
> >  		break;
> >  	case OPTEE_MSG_RPC_CMD_I2C_TRANSFER_WR:
> > +		client.adapter->retries = params[1].u.value.b;
> >  		ret = i2c_master_send(&client, params[2].u.memref.shm->kaddr,
> >  				      params[2].u.memref.size);
> >  		break;
> > @@ -126,6 +132,7 @@ static void handle_rpc_func_cmd_i2c_transfer(struct tee_context *ctx,
> >  			arg->ret = TEEC_SUCCESS;
> >  	}
> >  
> > +	client.adapter->retries = retries;
> >  	i2c_put_adapter(client.adapter);
> >  	kfree(params);
> >  	return;
> > -- 
> > 2.17.1
> > 

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] drivers: optee: i2c: add bus retry configuration
  2020-09-23 11:18   ` Jorge Ramirez-Ortiz, Foundries
@ 2020-09-23 11:26     ` Jorge Ramirez-Ortiz, Foundries
  2020-09-23 12:13       ` Jens Wiklander
  0 siblings, 1 reply; 6+ messages in thread
From: Jorge Ramirez-Ortiz, Foundries @ 2020-09-23 11:26 UTC (permalink / raw)
  To: Jorge Ramirez-Ortiz, Foundries
  Cc: Jens Wiklander, sumit.garg, ricardo, mike, tee-dev, linux-kernel, op-tee

On 23/09/20, Jorge Ramirez-Ortiz, Foundries wrote:
> On 22/09/20, Jens Wiklander wrote:
> > On Wed, Sep 16, 2020 at 05:27:32PM +0200, Jorge Ramirez-Ortiz wrote:
> > > Allow OP-TEE to specify the number of retries in the adaptor.
> > > 
> > > Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io>
> > > ---
> > >  drivers/tee/optee/rpc.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > > 
> > > diff --git a/drivers/tee/optee/rpc.c b/drivers/tee/optee/rpc.c
> > > index 1e3614e4798f..2d46a9ecb1de 100644
> > > --- a/drivers/tee/optee/rpc.c
> > > +++ b/drivers/tee/optee/rpc.c
> > > @@ -58,6 +58,7 @@ static void handle_rpc_func_cmd_i2c_transfer(struct tee_context *ctx,
> > >  	struct tee_param *params;
> > >  	size_t i;
> > >  	int ret = -EOPNOTSUPP;
> > > +	int retries = 0;
> > >  	u8 attr[] = {
> > >  		TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT,
> > >  		TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT,
> > > @@ -102,12 +103,17 @@ static void handle_rpc_func_cmd_i2c_transfer(struct tee_context *ctx,
> > >  	client.addr = params[0].u.value.c;
> > >  	snprintf(client.name, I2C_NAME_SIZE, "i2c%d", client.adapter->nr);
> > >  
> > > +	/* cache the current value */
> > > +	retries = client.adapter->retries;
> > > +
> > >  	switch (params[0].u.value.a) {
> > >  	case OPTEE_MSG_RPC_CMD_I2C_TRANSFER_RD:
> > > +		client.adapter->retries = params[1].u.value.b;
> > Do we need to take any locks befor this?
> 
> no I dont think so: there is no need for bus locks when requesting a
> transfer via i2c_master_recv/send; the lock for the bus segment gets
> taken later on, when the actual transfer hppens ( __i2c_transfer())
> 
> the functionality implemented in this function pretty much mimicks
> what is done in the normal world via /dev/i2c-X
> (drivers/i2c/i2c_dev.c)
>

correction (of course)
  - i2cdev_read --> i2c_master_recv
  - i2cdev->write -->i2c_master_send
> 
> and now the retry count setup on the adaptor with this commit.
> 
>  - i2cdev_ioctl I2C_RETRIES
> 
> > 
> > Cheers,
> > Jens
> > 
> > >  		ret = i2c_master_recv(&client, params[2].u.memref.shm->kaddr,
> > >  				      params[2].u.memref.size);
> > >  		break;
> > >  	case OPTEE_MSG_RPC_CMD_I2C_TRANSFER_WR:
> > > +		client.adapter->retries = params[1].u.value.b;
> > >  		ret = i2c_master_send(&client, params[2].u.memref.shm->kaddr,
> > >  				      params[2].u.memref.size);
> > >  		break;
> > > @@ -126,6 +132,7 @@ static void handle_rpc_func_cmd_i2c_transfer(struct tee_context *ctx,
> > >  			arg->ret = TEEC_SUCCESS;
> > >  	}
> > >  
> > > +	client.adapter->retries = retries;
> > >  	i2c_put_adapter(client.adapter);
> > >  	kfree(params);
> > >  	return;
> > > -- 
> > > 2.17.1
> > > 

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] drivers: optee: i2c: add bus retry configuration
  2020-09-23 11:26     ` Jorge Ramirez-Ortiz, Foundries
@ 2020-09-23 12:13       ` Jens Wiklander
  2020-09-23 13:51         ` Jorge Ramirez-Ortiz, Foundries
  0 siblings, 1 reply; 6+ messages in thread
From: Jens Wiklander @ 2020-09-23 12:13 UTC (permalink / raw)
  To: Jorge Ramirez-Ortiz, Foundries
  Cc: sumit.garg, ricardo, mike, tee-dev, linux-kernel, op-tee

On Wed, Sep 23, 2020 at 01:26:31PM +0200, Jorge Ramirez-Ortiz, Foundries wrote:
> On 23/09/20, Jorge Ramirez-Ortiz, Foundries wrote:
> > On 22/09/20, Jens Wiklander wrote:
> > > On Wed, Sep 16, 2020 at 05:27:32PM +0200, Jorge Ramirez-Ortiz wrote:
> > > > Allow OP-TEE to specify the number of retries in the adaptor.
> > > > 
> > > > Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io>
> > > > ---
> > > >  drivers/tee/optee/rpc.c | 7 +++++++
> > > >  1 file changed, 7 insertions(+)
> > > > 
> > > > diff --git a/drivers/tee/optee/rpc.c b/drivers/tee/optee/rpc.c
> > > > index 1e3614e4798f..2d46a9ecb1de 100644
> > > > --- a/drivers/tee/optee/rpc.c
> > > > +++ b/drivers/tee/optee/rpc.c
> > > > @@ -58,6 +58,7 @@ static void handle_rpc_func_cmd_i2c_transfer(struct tee_context *ctx,
> > > >  	struct tee_param *params;
> > > >  	size_t i;
> > > >  	int ret = -EOPNOTSUPP;
> > > > +	int retries = 0;
> > > >  	u8 attr[] = {
> > > >  		TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT,
> > > >  		TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT,
> > > > @@ -102,12 +103,17 @@ static void handle_rpc_func_cmd_i2c_transfer(struct tee_context *ctx,
> > > >  	client.addr = params[0].u.value.c;
> > > >  	snprintf(client.name, I2C_NAME_SIZE, "i2c%d", client.adapter->nr);
> > > >  
> > > > +	/* cache the current value */
> > > > +	retries = client.adapter->retries;
> > > > +
> > > >  	switch (params[0].u.value.a) {
> > > >  	case OPTEE_MSG_RPC_CMD_I2C_TRANSFER_RD:
> > > > +		client.adapter->retries = params[1].u.value.b;
> > > Do we need to take any locks befor this?
> > 
> > no I dont think so: there is no need for bus locks when requesting a
> > transfer via i2c_master_recv/send; the lock for the bus segment gets
> > taken later on, when the actual transfer hppens ( __i2c_transfer())
> > 
> > the functionality implemented in this function pretty much mimicks
> > what is done in the normal world via /dev/i2c-X
> > (drivers/i2c/i2c_dev.c)
> >
> 
> correction (of course)
>   - i2cdev_read --> i2c_master_recv
>   - i2cdev->write -->i2c_master_send
> > 
> > and now the retry count setup on the adaptor with this commit.
> > 
> >  - i2cdev_ioctl I2C_RETRIES

I don't understand. Do you mean that client.adapter->retries doesn't
need to be protected from concurrent updates? Or is it already protected
by some other mechanism?

Cheers,
Jens

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] drivers: optee: i2c: add bus retry configuration
  2020-09-23 12:13       ` Jens Wiklander
@ 2020-09-23 13:51         ` Jorge Ramirez-Ortiz, Foundries
  0 siblings, 0 replies; 6+ messages in thread
From: Jorge Ramirez-Ortiz, Foundries @ 2020-09-23 13:51 UTC (permalink / raw)
  To: Jens Wiklander
  Cc: Jorge Ramirez-Ortiz, Foundries, sumit.garg, ricardo, mike,
	tee-dev, linux-kernel, op-tee

On 23/09/20, Jens Wiklander wrote:
> On Wed, Sep 23, 2020 at 01:26:31PM +0200, Jorge Ramirez-Ortiz, Foundries wrote:
> > On 23/09/20, Jorge Ramirez-Ortiz, Foundries wrote:
> > > On 22/09/20, Jens Wiklander wrote:
> > > > On Wed, Sep 16, 2020 at 05:27:32PM +0200, Jorge Ramirez-Ortiz wrote:
> > > > > Allow OP-TEE to specify the number of retries in the adaptor.
> > > > > 
> > > > > Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io>
> > > > > ---
> > > > >  drivers/tee/optee/rpc.c | 7 +++++++
> > > > >  1 file changed, 7 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/tee/optee/rpc.c b/drivers/tee/optee/rpc.c
> > > > > index 1e3614e4798f..2d46a9ecb1de 100644
> > > > > --- a/drivers/tee/optee/rpc.c
> > > > > +++ b/drivers/tee/optee/rpc.c
> > > > > @@ -58,6 +58,7 @@ static void handle_rpc_func_cmd_i2c_transfer(struct tee_context *ctx,
> > > > >  	struct tee_param *params;
> > > > >  	size_t i;
> > > > >  	int ret = -EOPNOTSUPP;
> > > > > +	int retries = 0;
> > > > >  	u8 attr[] = {
> > > > >  		TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT,
> > > > >  		TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT,
> > > > > @@ -102,12 +103,17 @@ static void handle_rpc_func_cmd_i2c_transfer(struct tee_context *ctx,
> > > > >  	client.addr = params[0].u.value.c;
> > > > >  	snprintf(client.name, I2C_NAME_SIZE, "i2c%d", client.adapter->nr);
> > > > >  
> > > > > +	/* cache the current value */
> > > > > +	retries = client.adapter->retries;
> > > > > +
> > > > >  	switch (params[0].u.value.a) {
> > > > >  	case OPTEE_MSG_RPC_CMD_I2C_TRANSFER_RD:
> > > > > +		client.adapter->retries = params[1].u.value.b;
> > > > Do we need to take any locks befor this?
> > > 
> > > no I dont think so: there is no need for bus locks when requesting a
> > > transfer via i2c_master_recv/send; the lock for the bus segment gets
> > > taken later on, when the actual transfer hppens ( __i2c_transfer())
> > > 
> > > the functionality implemented in this function pretty much mimicks
> > > what is done in the normal world via /dev/i2c-X
> > > (drivers/i2c/i2c_dev.c)
> > >
> > 
> > correction (of course)
> >   - i2cdev_read --> i2c_master_recv
> >   - i2cdev->write -->i2c_master_send
> > > 
> > > and now the retry count setup on the adaptor with this commit.
> > > 
> > >  - i2cdev_ioctl I2C_RETRIES
> 
> I don't understand. Do you mean that client.adapter->retries doesn't
> need to be protected from concurrent updates? Or is it already protected
> by some other mechanism?

yeah I probably misunderstood your comment. my bad.

um I thought that upon getting the adaptor there would be some
protection mechanism in place until it is put back; but that is not
the case. looking a bit into it I see no simple way of protecting
changes to the adaptor (at any given time any thread could get a
pointer to it) so it seems that setting the retry field is not a
guarantee that it will be applied.

> 
> Cheers,
> Jens

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-09-23 13:51 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-16 15:27 [PATCH] drivers: optee: i2c: add bus retry configuration Jorge Ramirez-Ortiz
2020-09-22 16:38 ` Jens Wiklander
2020-09-23 11:18   ` Jorge Ramirez-Ortiz, Foundries
2020-09-23 11:26     ` Jorge Ramirez-Ortiz, Foundries
2020-09-23 12:13       ` Jens Wiklander
2020-09-23 13:51         ` Jorge Ramirez-Ortiz, Foundries

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).