Re: [Tee-dev] [PATCH v4 2/3] tee: generic TEE subsystem

Hi,

On Wed, Mar 25, 2015 at 12:01:40PM +0100, Jens Wiklander wrote:

+static long tee_ioctl_shm_alloc(struct tee_filp *teefilp,

• struct tee_ioctl_shm_alloc_data __user *udata)
  

+{

• long ret;
• struct tee_ioctl_shm_alloc_data data;
• struct tee_shm *shm;
• if (copy_from_user(&data, udata, sizeof(data)))

• return -EFAULT;
  

• /* Currently no input flags are supported */
• if (data.flags)

• return -EINVAL;
  

• shm = tee_shm_alloc(teefilp->teedev, teefilp, data.size,

• 	    TEE_SHM_MAPPED | TEE_SHM_DMA_BUF);
  

• if (IS_ERR(shm))

• return PTR_ERR(shm);
  

• ret = teefilp->teedev->desc->ops->shm_share(shm);

I was thinking a bit when it comes to this function. Now we start by allocating memory (tee_shm_alloc) and then we let the specific driver get a chance to do something with the shared memory we just allocated.

Wouldn't it be nicer to turn things around? I.e, first ask the specific driver if it wants to do something and then if needed call the tee_shm_alloc function? I.e, let the specific driver have a chance to override the default implementation?

For example, lets say that a certain specific driver needs to deal with some shared memory on it's own. So use cases could be, for example: a) The specific driver handles shared memory entirely on its own. b) The specific driver give hints (by settings some flags) to the generic driver. I.e, something like this.

Pseudo... /* This call updates "some_flag" in shm */ ret = teefilp->teedev->desc->ops->shm_share(shm);

u32 flags = TEE_SHM_MAPPED | TEE_SHM_DMA_BUF;

switch (shm.some_flag) { case HANDLED_BY_SPECIFIC: /* * Don't call tee_shm_alloc(), since the specific driver * already has allocated shared memory and don't want the * generic driver to do the same. */ break;

case FEATURE_X_REQUIRED: flags |= DO_SOME_MORE_MAGIC; /* Fall through ... */

case NO_FLAG_SET: default: /* Simply call the generic shm alloc function */ shm = tee_shm_alloc(teefilp->teedev, teefilp, data.size, flags); }

Comments? Thoughts?

• if (ret)

• goto err;
  

• data.flags = shm->flags;
• data.size = shm->size;
• data.fd = tee_shm_fd(shm);
• if (data.fd < 0) {

• ret = data.fd;
  

• goto err;
  

• }
• if (copy_to_user(udata, &data, sizeof(data))) {

• ret = -EFAULT;
  

• goto err;
  

• }
• return 0;

+err:

• tee_shm_free(shm);
• return ret;

+}