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. 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