Message-ID: <bb7b0895-bdcf-44dc-ac15-b528ebbc7e25@quicinc.com>
Date: Fri, 29 Nov 2024 08:28:43 +1100
From: Amirreza Zarrabi <quic_azarrabi@...cinc.com>
To: Sumit Garg <sumit.garg@...aro.org>
CC: Jens Wiklander <jens.wiklander@...aro.org>,
        <op-tee@...ts.trustedfirmware.org>, <linux-kernel@...r.kernel.org>,
        <linux-arm-msm@...r.kernel.org>
Subject: Re: [PATCH RFC 3/3] tee: introduce orphan tee_shm and default context



On 11/28/2024 11:44 PM, Sumit Garg wrote:
> On Thu, 28 Nov 2024 at 02:29, Amirreza Zarrabi
> <quic_azarrabi@...cinc.com> wrote:
>>
>>
>>
>> On 11/27/2024 5:01 PM, Sumit Garg wrote:
>>> On Tue, 26 Nov 2024 at 20:52, Jens Wiklander <jens.wiklander@...aro.org> wrote:
>>>>
>>>> On Tue, Nov 26, 2024 at 1:27 PM Sumit Garg <sumit.garg@...aro.org> wrote:
>>>>>
>>>>> On Tue, 26 Nov 2024 at 14:03, Jens Wiklander <jens.wiklander@...aro.org> wrote:
>>>>>>
>>>>>> On Mon, Nov 25, 2024 at 9:55 PM Amirreza Zarrabi
>>>>>> <quic_azarrabi@...cinc.com> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 11/25/2024 6:51 PM, Sumit Garg wrote:
>>>>>>>> On Mon, 25 Nov 2024 at 12:53, Jens Wiklander <jens.wiklander@...aro.org> wrote:
>>>>>>>>>
>>>>>>>>> On Mon, Nov 25, 2024 at 7:14 AM Sumit Garg <sumit.garg@...aro.org> wrote:
>>>>>>>>>>
>>>>>>>>>> On Mon, 25 Nov 2024 at 03:00, Amirreza Zarrabi
>>>>>>>>>> <quic_azarrabi@...cinc.com> wrote:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Hi Sumit,
>>>>>>>>>>>
>>>>>>>>>>> Thank you so much for the comemnts :).
>>>>>>>>>>>
>>>>>>>>>>> On 11/23/2024 9:32 PM, Sumit Garg wrote:
>>>>>>>>>>>> Hi Amirreza,
>>>>>>>>>>>>
>>>>>>>>>>>> Thanks for proposing this.
>>>>>>>>>>>>
>>>>>>>>>>>> On Fri, 22 Nov 2024 at 06:38, Amirreza Zarrabi
>>>>>>>>>>>> <quic_azarrabi@...cinc.com> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 11/21/2024 11:08 PM, Jens Wiklander wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>> Hi Jens,
>>>>>>>>>>>>>
>>>>>>>>>>>>>> Hi Amirreza,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On Thu, Nov 21, 2024 at 2:37 AM Amirreza Zarrabi
>>>>>>>>>>>>>> <quic_azarrabi@...cinc.com> wrote:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> The default context has a lifespan similar to the tee_device.
>>>>>>>>>>>>
>>>>>>>>>>>> Since it's associated with tee_device context, let's call it obvious
>>>>>>>>>>>> via renaming it as device context instead (s/def_ctx/dev_ctx/ in this
>>>>>>>>>>>> patch).
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Make sense, I'll rename it.
>>>>>>>>>>>
>>>>>>>>>>>>>>> It is used as a context for shared memory if the context to which the
>>>>>>>>>>>>>>> shared memory belongs is released, making the tee_shm an orphan.
>>>>>>>>>>>>>>> This allows the driver implementing shm_unregister to safely make
>>>>>>>>>>>>>>> subsequent calls, such as to a supplicant if needed.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> It also enables users to free the shared memory while the driver is
>>>>>>>>>>>>>>> blocked on unregister_tee_device safely.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Preferably, this should be used for all driver internal uses, using
>>>>>>>>>>>>>>> teedev_get_def_context rather than calling teedev_open.
>>>>>>>>>>>>
>>>>>>>>>>>> Makes sense to me.
>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Signed-off-by: Amirreza Zarrabi <quic_azarrabi@...cinc.com>
>>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>>  drivers/tee/optee/core.c    |  2 +-
>>>>>>>>>>>>>>>  drivers/tee/optee/ffa_abi.c |  2 +-
>>>>>>>>>>>>>>>  drivers/tee/optee/smc_abi.c |  2 +-
>>>>>>>>>>>>>>>  drivers/tee/tee_core.c      | 83 +++++++++++++++++++++++++++++----------------
>>>>>>>>>>>>>>>  drivers/tee/tee_private.h   |  3 --
>>>>>>>>>>>>>>>  drivers/tee/tee_shm.c       | 18 ++--------
>>>>>>>>>>>>>>>  include/linux/tee_core.h    | 15 ++++++++
>>>>>>>>>>>>>>>  include/linux/tee_drv.h     |  7 ----
>>>>>>>>>>>>>>>  8 files changed, 73 insertions(+), 59 deletions(-)
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
>>>>>>>>>>>>>>> index c75fddc83576..78d43d0c8014 100644
>>>>>>>>>>>>>>> --- a/drivers/tee/optee/core.c
>>>>>>>>>>>>>>> +++ b/drivers/tee/optee/core.c
>>>>>>>>>>>>>>> @@ -173,7 +173,7 @@ void optee_remove_common(struct optee *optee)
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>         optee_notif_uninit(optee);
>>>>>>>>>>>>>>>         optee_shm_arg_cache_uninit(optee);
>>>>>>>>>>>>>>> -       teedev_close_context(optee->ctx);
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>         /*
>>>>>>>>>>>>>>>          * The two devices have to be unregistered before we can free the
>>>>>>>>>>>>>>>          * other resources.
>>>>>>>>>>>>>>> diff --git a/drivers/tee/optee/ffa_abi.c b/drivers/tee/optee/ffa_abi.c
>>>>>>>>>>>>>>> index f3af5666bb11..6ad94f0788ad 100644
>>>>>>>>>>>>>>> --- a/drivers/tee/optee/ffa_abi.c
>>>>>>>>>>>>>>> +++ b/drivers/tee/optee/ffa_abi.c
>>>>>>>>>>>>>>> @@ -949,7 +949,7 @@ static int optee_ffa_probe(struct ffa_device *ffa_dev)
>>>>>>>>>>>>>>>         optee_shm_arg_cache_init(optee, arg_cache_flags);
>>>>>>>>>>>>>>>         mutex_init(&optee->rpmb_dev_mutex);
>>>>>>>>>>>>>>>         ffa_dev_set_drvdata(ffa_dev, optee);
>>>>>>>>>>>>>>> -       ctx = teedev_open(optee->teedev);
>>>>>>>>>>>>>>> +       ctx = teedev_get_def_context(optee->teedev);
>>>>>>>>>>>>>>>         if (IS_ERR(ctx)) {
>>>>>>>>>>>>>>>                 rc = PTR_ERR(ctx);
>>>>>>>>>>>>>>>                 goto err_rhashtable_free;
>>>>>>>>>>>>>>> diff --git a/drivers/tee/optee/smc_abi.c b/drivers/tee/optee/smc_abi.c
>>>>>>>>>>>>>>> index e9456e3e74cc..c77a3e631d04 100644
>>>>>>>>>>>>>>> --- a/drivers/tee/optee/smc_abi.c
>>>>>>>>>>>>>>> +++ b/drivers/tee/optee/smc_abi.c
>>>>>>>>>>>>>>> @@ -1722,7 +1722,7 @@ static int optee_probe(struct platform_device *pdev)
>>>>>>>>>>>>>>>         mutex_init(&optee->rpmb_dev_mutex);
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>         platform_set_drvdata(pdev, optee);
>>>>>>>>>>>>>>> -       ctx = teedev_open(optee->teedev);
>>>>>>>>>>>>>>> +       ctx = teedev_get_def_context(optee->teedev);
>>>>>>>>>>>>>>>         if (IS_ERR(ctx)) {
>>>>>>>>>>>>>>>                 rc = PTR_ERR(ctx);
>>>>>>>>>>>>>>>                 goto err_supp_uninit;
>>>>>>>>>>>>>>> diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c
>>>>>>>>>>>>>>> index 93f3b330aec8..805e1336089d 100644
>>>>>>>>>>>>>>> --- a/drivers/tee/tee_core.c
>>>>>>>>>>>>>>> +++ b/drivers/tee/tee_core.c
>>>>>>>>>>>>>>> @@ -57,7 +57,6 @@ struct tee_context *teedev_open(struct tee_device *teedev)
>>>>>>>>>>>>>>>                 goto err;
>>>>>>>>>>>>>>>         }
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> -       kref_init(&ctx->refcount);
>>>>>>>>>>>>>>>         ctx->teedev = teedev;
>>>>>>>>>>>>>>>         INIT_LIST_HEAD(&ctx->list_shm);
>>>>>>>>>>>>>>>         rc = teedev->desc->ops->open(ctx);
>>>>>>>>>>>>>>> @@ -73,36 +72,43 @@ struct tee_context *teedev_open(struct tee_device *teedev)
>>>>>>>>>>>>>>>  }
>>>>>>>>>>>>>>>  EXPORT_SYMBOL_GPL(teedev_open);
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> -void teedev_ctx_get(struct tee_context *ctx)
>>>>>>>>>>>>>>> +struct tee_context *teedev_get_def_context(struct tee_device *teedev)
>>>>>>>>>>>>>>>  {
>>>>>>>>>>>>>>> -       if (ctx->releasing)
>>>>>>>>>>>>>>> -               return;
>>>>>>>>>>>>>>> +       int rc;
>>>>>>>>>>>>>>> +       struct tee_context *ctx = &teedev->def_ctx;
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> -       kref_get(&ctx->refcount);
>>>>>>>>>>>>>>> -}
>>>>>>>>>>>>>>> +       ctx->teedev = teedev;
>>>>>>>>>>>>>>> +       INIT_LIST_HEAD(&ctx->list_shm);
>>>>>>>>>>>>>>> +       rc = teedev->desc->ops->open(ctx);
>>>>>>>>>>>>>>> +       if (rc)
>>>>>>>>>>>>>>> +               return ERR_PTR(rc);
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I think ctx->teedev and ctx->list_shm must always be initialized or
>>>>>>>>>>>>>> &teedev->def_ctx can't be used in teedev_close_context().
>>>>>>>>>>>>>
>>>>>>>>>>>>> True, but &teedev->def_ctx is never used in teedev_close_context().
>>>>>>>>>>>>> The closing of the &teedev->def_ctx simply ignored. So once opened,
>>>>>>>>>>>>> &teedev->def_ctx will always remain open until the tee_device is alive.
>>>>>>>>>>>>>
>>>>>>>>>>>>>> We could initialize teedev->def_ctx on the first call to teedev_open()
>>>>>>>>>>>>>> on that tee_device. We need a way to tell the
>>>>>>>>>>>>>> teedev->desc->ops->open() to the backed driver that it's initializing
>>>>>>>>>>>>>> the default context though, or optee_open() can't handle the
>>>>>>>>>>>>>> tee-supplicant case properly.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> That's a good point. This way, it is guaranteed that there is one def_ctx
>>>>>>>>>>>>> per teedev. There should be a way to tell the open() callback that it is
>>>>>>>>>>>>> a def_ctx, so it is not registered as a supplicant context.
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>> Should we allow this function to be called more than once for each teedev?
>>>>>>>>>>>>>
>>>>>>>>>>>>> Yes, moving to teedev_open() will fix the issue.
>>>>>>>>>>>>>
>>>>>>>>>>>>>> Do we need serialization in this function if it's called after the
>>>>>>>>>>>>>> driver is probed?
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> True. I'll make sure there is no race.
>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> -static void teedev_ctx_release(struct kref *ref)
>>>>>>>>>>>>>>> -{
>>>>>>>>>>>>>>> -       struct tee_context *ctx = container_of(ref, struct tee_context,
>>>>>>>>>>>>>>> -                                              refcount);
>>>>>>>>>>>>>>> -       ctx->releasing = true;
>>>>>>>>>>>>>>> -       ctx->teedev->desc->ops->release(ctx);
>>>>>>>>>>>>>>> -       kfree(ctx);
>>>>>>>>>>>>>>> +       return ctx;
>>>>>>>>>>>>>>>  }
>>>>>>>>>>>>>>> +EXPORT_SYMBOL_GPL(teedev_get_def_context);
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> -void teedev_ctx_put(struct tee_context *ctx)
>>>>>>>>>>>>>>> +void teedev_close_context(struct tee_context *ctx)
>>>>>>>>>>>>>>>  {
>>>>>>>>>>>>>>> -       if (ctx->releasing)
>>>>>>>>>>>>>>> +       struct tee_device *teedev = ctx->teedev;
>>>>>>>>>>>>>>> +       struct tee_shm *shm;
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> +       if (ctx == &teedev->def_ctx)
>>>>>>>>>>>>>>>                 return;
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> -       kref_put(&ctx->refcount, teedev_ctx_release);
>>>>>>>>>>>>>>> -}
>>>>>>>>>>>>>>> +       teedev->desc->ops->release(ctx);
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> -void teedev_close_context(struct tee_context *ctx)
>>>>>>>>>>>>>>> -{
>>>>>>>>>>>>>>> -       struct tee_device *teedev = ctx->teedev;
>>>>>>>>>>>>>>> +       mutex_lock(&teedev->mutex);
>>>>>>>>>>>>>>> +       list_for_each_entry(shm, &ctx->list_shm, link) {
>>>>>>>>>>>>>>> +               /* Context released. However, shm still holding a teedev reference.
>>>>>>>>>>>>>>> +                * Replace shm->ctx with the default context so that tee_shm_get_from_id()
>>>>>>>>>>>>>>> +                * fails (i.e. it is not accessible from userspace) but shm still
>>>>>>>>>>>>>>> +                * holds a valid context for further clean up, e.g. shm_unregister().
>>>>>>>>>>>>>>> +                */
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> /*
>>>>>>>>>>>>>>  * Please format
>>>>>>>>>>>>>>  * multiline comments
>>>>>>>>>>>>>>  * like this. Please
>>>>>>>>>>>>>>  * keep the lines at
>>>>>>>>>>>>>>  * max 80 columns
>>>>>>>>>>>>>>  * here and at other
>>>>>>>>>>>>>>  * places in the patch-
>>>>>>>>>>>>>>  * set.
>>>>>>>>>>>>>>  */
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Ack.
>>>>>>>>>>>>>
>>>>>>>>>>>>>>> +               shm->ctx = &teedev->def_ctx;
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> shm->ctx will always point to a valid context, even if it is the
>>>>>>>>>>>>>> default context. It seems that we can always get hold of the correct
>>>>>>>>>>>>>> teedev via shm->ctx->teedev. Do we need "tee: revert removal of
>>>>>>>>>>>>>> redundant teedev in struct tee_shm"?
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> It was there in case we wanted to use NULL, but with def_ctx, it is not
>>>>>>>>>>>>> necessary. I am withdrawing that commit. :).
>>>>>>>>>>>>>
>>>>>>>>>>>>>> Shouldn't the shm be removed from the ctx->list_shm and be moved to
>>>>>>>>>>>>>> teedev->def_ctx.list_shm?
>>>>>>>>>>>>
>>>>>>>>>>>> +1
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Ack.
>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Not really. If we put shm in the teedev->def_ctx.list_shm, by the time
>>>>>>>>>>>>> we are closing the def_ctx, the list is guaranteed to be empty.
>>>>>>>>>>>>>
>>>>>>>>>>>>> However, I understand it is cleaner and more consistent to do that rather
>>>>>>>>>>>>> than making changes to tee_shm_put().
>>>>>>>>>>>>>
>>>>>>>>>>>>> I'll do it.
>>>>>>>>>>>>>
>>>>>>>>>>>>>>> +       }
>>>>>>>>>>>>>>> +       mutex_unlock(&teedev->mutex);
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> -       teedev_ctx_put(ctx);
>>>>>>>>>>>>>>> +       kfree(ctx);
>>>>>>>>>>>>>>>         tee_device_put(teedev);
>>>>>>>>>>>>>>>  }
>>>>>>>>>>>>>>>  EXPORT_SYMBOL_GPL(teedev_close_context);
>>>>>>>>>>>>>>> @@ -946,6 +952,8 @@ struct tee_device *tee_device_alloc(const struct tee_desc *teedesc,
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>         teedev->desc = teedesc;
>>>>>>>>>>>>>>>         teedev->pool = pool;
>>>>>>>>>>>>>>> +       /* Only open default context when teedev_get_def_context() called. */
>>>>>>>>>>>>>>> +       teedev->def_ctx.teedev = NULL;
>>>>>>>>>>>>
>>>>>>>>>>>> Why don't you open the device context here only? This will associate
>>>>>>>>>>>> it automatically with teedev lifespan and then
>>>>>>>>>>>> teedev_get_def_context() will just return a reference to that.
>>>>>>>>>>>>
>>>>>>>>>>>> -Sumit
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> So my assumption is that the tee_devic_alloc() is called as part of
>>>>>>>>>>> the driver initialization; there is no guarantee that at this time the
>>>>>>>>>>> driver is actually ready to accept any open() callback.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> The drivers should be able to handle open() callback since we already
>>>>>>>>>> check for !teedesc->ops->open in the beginning of tee_devic_alloc().
>>>>>>>>>> Also, we need to open a device context for !TEE_DESC_PRIVILEGED such
>>>>>>>>>> that we don't open a supplicant device context there.
>>>>>>>>>
>>>>>>>>> It would be nice to have the device context fully initialized when the
>>>>>>>>> probe function returns. How about adding a "bool is_dev_ctx" to struct
>>>>>>>>> tee_context so the open() callback can tell that this is a special
>>>>>>>>> tee_contex?
>>>>>>>>
>>>>>>>> Sure, that will be useful to distinguish the device context from
>>>>>>>> normal client context.
>>>>>>>>
>>>>>>>> -Sumit
>>>>>>>>
>>>>>>>
>>>>>>> So, as far as the open() callback, I do not believe checking if it is not null
>>>>>>> is reasonable for calling it here. Most drivers allocate resources and then
>>>>>>> initialize them. So, assume these steps for a TEE driver:
>>>>>>>  (1) allocate internal data structures,
>>>>>>>  (2) allocate the device,
>>>>>>>  (3) initialize the internal data structurse and then
>>>>>>>  (4) register the device.
>>>>>>>
>>>>>>> Having these steps for a backend driver means that if you call open() at
>>>>>>> step (2), the internal data structures are not ready.
>>>>>
>>>>> As part of tee_device_alloc(), every driver has to pass "const struct
>>>>> tee_desc *teedesc" fully initialized. Which internal data structures
>>>>> are you referring too? Is there any upstream example?
>>>>
>>>> It's reasonable to wait with the open() callback until step 4 above,
>>>> which should correspond with the tee_device_register() call. Data
>>>> written only once doesn't need serialized access if the fields are
>>>> only accessed after they have been fully initialized.
>>>
>>> Fair enough, I can live with the device context opened after registering it.
>>>
>>>>
>>>>>
>>>>>>>
>>>>>>> I was originally thinking of going with Jens' suggestion to open dev_ctx in
>>>>>>> the teedev_open(), and use a flag to distinguish the type of context for
>>>>>>> the open() callback
>>>>>>>
>>>>>>> What about this:
>>>>>>> Open the dev_ctx in the tee_device_register(), at the last step before
>>>>>>> setting the TEE_DEVICE_FLAG_REGISTERED flag. Then the open() callback can
>>>>>>> check for this flag to determine if it is a normal context or dev_ctx.
>>>>>>> If the open() is called while the device has not been registered, it should
>>>>>>> handle it differently
>>>>>>
>>>>>> That makes sense, the driver should be prepared to handle open() calls
>>>>>> after tee_device_register() anyway.
>>>>>> However, there is no serialization of the flags field in struct
>>>>>> tee_device. Hmm, would it be too hacky for the open() callback to
>>>>>> check if &ctx->teedev.dev_ctx == ctx? We could add a helper function
>>>>>> to wrap that check.
>>>>>>
>>>>>
>>>>> Your suggested change requires every driver to update open() callback
>>>>> and later other callbacks may have to support it too. IMHO, only
>>>>> teedev_get_dev_ctx() should be able to return a reference to device
>>>>> context for usage within the TEE and the implementation driver.
>>>>
>>>> Yes, but it's only the OP-TEE driver that needs anything special. It
>>>> looks like the others can be left unchanged.
>>>
>>> I suppose it's most likely the upcoming QTEE driver requiring it.
>>>
>>
>> I don't believe this is correct. This requirement is implicitly imposed
>> by the TEE subsystem API. If calling open() is acceptable in
>> tee_device_alloc(), then I could argue that tee_device_register() and
>> tee_device_alloc() should be merged into a single function. If a driver
>> is ready to handle requests, why delay its exposure by postponing the
>> registration?
> 
> You can't mix in device alloc/init with device registration. As soon
> as you register a device, it's available for user-space to issue
> IOCTLs. So if there is any race between device init and it's usage
> then it's going to cause hard to debug issues.
> 

That's exactly my point :). Whoever alloc the device, knows that the
device is not being available untill the regsiteration is done. So they
may make decisions based on that, e.g reorder some init steps.

> There can be an argument that dev_ctx being a member of "struct
> tee_device" so it should get initialized alongside other bits in
> tee_device_alloc() (we can rename
> s/tee_device_alloc()/tee_device_init()/ if that makes it obvious). But
> I can live with the device context getting initialized as the first
> thing in tee_device_register() avoiding the race window mentioned
> above.
> 
>>
>> By calling open() in tee_device_alloc(), you indirectly impose an unspoken
>> requirement on developers regarding how they should write their drivers,
>> such as the steps they should take to probe the device.
> 
> Still not sure why it isn't an explicit requirement when you are
> already passing an initialized ops structure to tee_device_alloc().
> 
> -Sumit

Whoeever, passed the ops to the alloc interface may assume that it is
not being called untill the register being finished as you mentioned
above. I assume it is fair assumption.

Regards,
Amir

> 
>>
>> Regards,
>> Amir
>>
>>>>
>>>>>
>>>>> I am still not able to understand why the following won't work with a
>>>>> clear lifetime for the device context?
>>>>>
>>>>> tee_device_alloc()
>>>>>   -> if (!(teedesc->flags & TEE_DESC_PRIVILEGED))
>>>>> desc->ops->open(&teedev->dev_ctx);
>>>>
>>>> We must also have a fully initialized dev_ctx for the supplicant
>>>> device.
>>>
>>> Currently I only see following for OP-TEE driver:
>>>
>>> ctx = teedev_open(optee->teedev);
>>>
>>> And I can't see anything like below:
>>>
>>> ctx = teedev_open(optee->supp_teedev);
>>>
>>> Where do you think that the dev_ctx is required for a supplicant
>>> device? AFAICS, currently opening a context with the supplicant device
>>> means that the supplicant daemon is available to handle RPCs which
>>> won't be possible during OP-TEE driver probe. Am I missing something?
>>>
>>>> I'd rather delay the open() callback until
>>>> tee_device_register() since the dev_ctx is guaranteed not to be needed
>>>> before that.
>>>
>>> Okay, the updated call chain can look like:
>>>
>>> tee_device_register()
>>>   -> if (!(teedev->desc->flags & TEE_DESC_PRIVILEGED))
>>>          desc->ops->open(&teedev->dev_ctx);
>>>>
>>>>>
>>>>> tee_device_put()
>>>>>   -> if (teedev->dev_ctx) desc->ops->release(&teedev->dev_ctx);
>>>>
>>>> teedev->dev_ctx is supposed to be embedded in struct tee_device, so
>>>> the if isn't needed.
>>>
>>> I added "if" to cover the case when dev_ctx is not initialized for the
>>> supplicant device.
>>>
>>> -Sumit
>>>
>>>>
>>>> Cheers,
>>>> Jens
>>>>
>>>>>
>>>>> -Sumit
>>>>>
>>>>>> Cheers,
>>>>>> Jens
>>>>>>
>>>>>>>
>>>>>>> - Amir
>>>>>>>
>>>>>>>>>
>>>>>>>>> Cheers,
>>>>>>>>> Jens
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> -Sumit

Powered by blists - more mailing lists