Hi Igor,
On Thu, 4 Apr 2024 at 13:18, Igor Opaniuk igor.opaniuk@gmail.com wrote:
Hi Ilias,
On Thu, Apr 4, 2024 at 10:54 AM Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Igor,
On Thu, 4 Apr 2024 at 11:40, Ilias Apalodimas ilias.apalodimas@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.opaniuk@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.opaniuk@gmail.com skype: igor.opanyuk https://www.linkedin.com/in/iopaniuk