Hi Igor, On Thu, 4 Apr 2024 at 13:18, Igor Opaniuk <igor.opan...@gmail.com> wrote: > > Hi Ilias, > > On Thu, Apr 4, 2024 at 10:54 AM Ilias Apalodimas > <ilias.apalodi...@linaro.org> wrote: >> >> Hi Igor, >> >> On Thu, 4 Apr 2024 at 11:40, Ilias Apalodimas >> <ilias.apalodi...@linaro.org> wrote: >> > >> > Hi Igor, >> > >> > I was about to apply the series, but noticed neither me or Jens were cc'ed >> > on this. Adding Jens to the party >> > >> > On Thu, Apr 04, 2024 at 10:13:49AM +0200, Igor Opaniuk wrote: >> > > Add calls for closing tee session after every read/write operation. >> > >> > What the patch is pretty obvious, but I am missing an explanation of why >> > this is needed >> > >> > > >> > > Signed-off-by: Igor Opaniuk <igor.opan...@gmail.com> >> > > --- >> > > >> > > (no changes since v1) >> > > >> > > cmd/optee_rpmb.c | 23 +++++++++++++++++------ >> > > 1 file changed, 17 insertions(+), 6 deletions(-) >> > > >> > > diff --git a/cmd/optee_rpmb.c b/cmd/optee_rpmb.c >> > > index e0e44bbed04..b3cafd92410 100644 >> > > --- a/cmd/optee_rpmb.c >> > > +++ b/cmd/optee_rpmb.c >> > > @@ -87,8 +87,10 @@ static int read_persistent_value(const char *name, >> > > >> > > rc = tee_shm_alloc(tee, name_size, >> > > TEE_SHM_ALLOC, &shm_name); >> > > - if (rc) >> > > - return -ENOMEM; >> > > + if (rc) { >> > > + rc = -ENOMEM; >> > > + goto close_session; >> > > + } >> > >> > I don't think you need the session to be opened to allocate memory. >> > So instead of of doing this, why don't we just move the alloc call before >> > opening the session? >> >> Looking at it again, we do need tee != NULL, but I think it's cleaner >> to add something like >> if (!dev) >> return -ENODEV >> to __tee_shm_add() instead. > > Do you mind if I address that in a separate patch series, as tbh I > don't want to add additional patches to the existing series?
We still completely change the functionality. So, the patchset is not a 'tiny cleanup', it instead closes the session every time instead of keeping it open. There are a few things going on, that aren't explained clearly in the commit message - Why is this needed? Looking at the code it is an actual problem since sessions tied to the AVB uuid will remain open - Is there any side effect by always closing the session? I don't mind merging it as is with a proper commit message, but I think the alternative is just easier. Thanks /Ilias > >> >> >> Cheers >> /Ilias >> > >> > > >> > > rc = tee_shm_alloc(tee, buffer_size, >> > > TEE_SHM_ALLOC, &shm_buf); >> > > @@ -125,6 +127,9 @@ out: >> > > tee_shm_free(shm_buf); >> > > free_name: >> > > tee_shm_free(shm_name); >> > > +close_session: >> > > + tee_close_session(tee, session); >> > > + tee = NULL; >> > > >> > > return rc; >> > > } >> > > @@ -139,17 +144,20 @@ static int write_persistent_value(const char *name, >> > > struct tee_param param[2]; >> > > size_t name_size = strlen(name) + 1; >> > > >> > > + if (!value_size) >> > > + return -EINVAL; >> > > + >> > > if (!tee) { >> > > if (avb_ta_open_session()) >> > > return -ENODEV; >> > > } >> > > - if (!value_size) >> > > - return -EINVAL; >> > > >> > > rc = tee_shm_alloc(tee, name_size, >> > > TEE_SHM_ALLOC, &shm_name); >> > > - if (rc) >> > > - return -ENOMEM; >> > > + if (rc) { >> > > + rc = -ENOMEM; >> > > + goto close_session; >> > > + } >> > >> > ditto >> > >> > > >> > > rc = tee_shm_alloc(tee, value_size, >> > > TEE_SHM_ALLOC, &shm_buf); >> > > @@ -178,6 +186,9 @@ out: >> > > tee_shm_free(shm_buf); >> > > free_name: >> > > tee_shm_free(shm_name); >> > > +close_session: >> > > + tee_close_session(tee, session); >> > > + tee = NULL; >> > > >> > > return rc; >> > > } >> > > -- >> > > 2.34.1 >> > > >> > >> > Thanks >> > /Ilias > > > > > -- > Best regards - Atentamente - Meilleures salutations > > Igor Opaniuk > > mailto: igor.opan...@gmail.com > skype: igor.opanyuk > https://www.linkedin.com/in/iopaniuk