Hi Ilias, On Thu, Apr 4, 2024 at 10:40 AM 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 > I usually rely on automatic CC-list creation by patman. Looks like there is no entry in MAINTAINERS for this specific file, that's why only Tom was added. I'll send a patch for that if you don't mind. > 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? > That's correct, but avb_ta_open_session() wrapper calls both tee_find_device()/ tee_open_session(), and tee_shm_alloc() expects valid tee device as param, which is initialized by tee_find_device(). My intention was to address the only one particular issue that was catched by CI and explained in [1] instead of doing overhaul of the whole optee_rpmb cmd implementation. > > > > 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 > [1] https://lore.kernel.org/all/20240302220108.720637-5-igor.opan...@gmail.com/T/ -- Best regards - Atentamente - Meilleures salutations Igor Opaniuk mailto: igor.opan...@gmail.com skype: igor.opanyuk https://www.linkedin.com/in/iopaniuk <http://ua.linkedin.com/in/iopaniuk>