On Fri, Apr 5, 2024 at 9:15 AM Sumit Garg <sumit.garg@xxxxxxxxxx> wrote: > > On Wed, 3 Apr 2024 at 20:11, Jens Wiklander <jens.wiklander@xxxxxxxxxx> wrote: > > > > On Wed, Apr 3, 2024 at 2:58 PM Sumit Garg <sumit.garg@xxxxxxxxxx> wrote: > > > > > > On Thu, 28 Mar 2024 at 21:39, Jens Wiklander <jens.wiklander@xxxxxxxxxx> wrote: > > > > > > > > On Fri, Mar 1, 2024 at 11:28 AM Sumit Garg <sumit.garg@xxxxxxxxxx> wrote: > > > > > > > > > > Hi Jens, > > > > > > > > > > On Tue, 27 Feb 2024 at 21:01, Jens Wiklander <jens.wiklander@xxxxxxxxxx> wrote: > > > > > > > > > > > > Adds support in the OP-TEE drivers (both SMC and FF-A ABIs) to probe and > > > > > > use an RPMB device via the RPBM subsystem instead of passing the RPMB > > > > > > > > > > s/RPBM/RPMB/ > > > > > > > > > > Here are other places too in this patch-set. > > > > > > > > > > > frames via tee-supplicant in user space. A fallback mechanism is kept to > > > > > > route RPMB frames via tee-supplicant if the RPMB subsystem isn't > > > > > > available. > > > > > > > > > > > > The OP-TEE RPC ABI is extended to support iterating over all RPMB > > > > > > devices until one is found with the expected RPMB key already > > > > > > programmed. > > > > > > > > > > I would appreciate it if you could add a link to OP-TEE OS changes in > > > > > the cover-letter although I have found them here [1]. > > > > > > > > > > [1] https://github.com/jenswi-linaro/optee_os/commits/rpmb_probe/ > > > > > > > > OK, I'll add a link in the coverletter of the next patch set. > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Jens Wiklander <jens.wiklander@xxxxxxxxxx> > > > > > > --- > > > > > > drivers/tee/optee/core.c | 55 +++++++ > > > > > > drivers/tee/optee/ffa_abi.c | 7 + > > > > > > drivers/tee/optee/optee_private.h | 16 ++ > > > > > > drivers/tee/optee/optee_rpc_cmd.h | 35 +++++ > > > > > > drivers/tee/optee/rpc.c | 233 ++++++++++++++++++++++++++++++ > > > > > > drivers/tee/optee/smc_abi.c | 6 + > > > > > > 6 files changed, 352 insertions(+) > > > > > > > > > > > > diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c > > > > > > index 3aed554bc8d8..6b32d3e7865b 100644 > > > > > > --- a/drivers/tee/optee/core.c > > > > > > +++ b/drivers/tee/optee/core.c > > > > > > @@ -11,6 +11,7 @@ > > > > > > #include <linux/io.h> > > > > > > #include <linux/mm.h> > > > > > > #include <linux/module.h> > > > > > > +#include <linux/rpmb.h> > > > > > > #include <linux/slab.h> > > > > > > #include <linux/string.h> > > > > > > #include <linux/tee_drv.h> > > > > > > @@ -80,6 +81,57 @@ void optee_pool_op_free_helper(struct tee_shm_pool *pool, struct tee_shm *shm, > > > > > > shm->pages = NULL; > > > > > > } > > > > > > > > > > > > +static void optee_rpmb_scan(struct work_struct *work) > > > > > > +{ > > > > > > + struct optee *optee = container_of(work, struct optee, scan_rpmb_work); > > > > > > + bool scan_done = false; > > > > > > + u32 res; > > > > > > + > > > > > > + do { > > > > > > + mutex_lock(&optee->rpmb_dev_mutex); > > > > > > + /* No need to rescan if we haven't started scanning yet */ > > > > > > + optee->rpmb_dev_request_rescan = false; > > > > > > + mutex_unlock(&optee->rpmb_dev_mutex); > > > > > > + > > > > > > + res = optee_enumerate_devices(PTA_CMD_GET_DEVICES_RPMB); > > > > > > + if (res && res != TEE_ERROR_STORAGE_NOT_AVAILABLE) > > > > > > > > > > I suppose this hasn't been tested for a negative case since > > > > > optee_enumerate_devices() won't return this error code (see [2]). > > > > > However, I would prefer to use GP Client error code: > > > > > TEEC_ERROR_ITEM_NOT_FOUND here instead. > > > > > > > > > > [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/tee/optee/device.c#n43 > > > > > > > > I prefer TEE_ERROR_STORAGE_NOT_AVAILABLE since that's the code GP says > > > > a TA should get when storage is unavailable. > > > > TEEC_ERROR_ITEM_NOT_FOUND is less specific. Anyway, I'll need to > > > > translate the code in get_devices(). > > > > > > Okay, that's fair. > > > > > > > > > > > > > > > > > > > > > > + pr_info("Scanning for RPMB device: res %#x\n", res); > > > > > > + > > > > > > + mutex_lock(&optee->rpmb_dev_mutex); > > > > > > + /* > > > > > > + * If another RPMB device came online while scanning, scan one > > > > > > + * more time, unless we have already found an RPBM device. > > > > > > + */ > > > > > > + scan_done = (optee->rpmb_dev || > > > > > > > > > > I suppose we don't need to check for optee->rpmb_dev here since a > > > > > successful return from > > > > > optee_enumerate_devices(PTA_CMD_GET_DEVICES_RPMB) would ensure that > > > > > the RPMB device has been found. > > > > > > > > That makes sense, I'll check the return value instead. > > > > > > > > > > > > > > > + !optee->rpmb_dev_request_rescan); > > > > > > + optee->rpmb_dev_request_rescan = false; > > > > > > + optee->rpmb_dev_scan_in_progress = !scan_done; > > > > > > + mutex_unlock(&optee->rpmb_dev_mutex); > > > > > > + } while (!scan_done); > > > > > > +} > > > > > > + > > > > > > +void optee_rpmb_intf_add_rdev(struct rpmb_interface *intf, > > > > > > + struct rpmb_dev *rdev) > > > > > > +{ > > > > > > + struct optee *optee = container_of(intf, struct optee, rpmb_intf); > > > > > > + bool queue_work = true; > > > > > > + > > > > > > + mutex_lock(&optee->rpmb_dev_mutex); > > > > > > + if (optee->rpmb_dev || optee->rpmb_dev_scan_in_progress) { > > > > > > > > > > Can we use work_pending() instead of our custom > > > > > optee->rpmb_dev_scan_in_progress flag? > > > > > > > > That seems racy, or am I missing something? > > > > > > > > > > You are right and even work_busy() is documented to provide unreliable > > > results. So I am rather thinking about just queuing the work and > > > thereby scanning for devices unconditionally. I suppose the extra > > > logic to check if we don't try to register duplicate devices can go > > > under optee_enumerate_devices(). > > > > > > > > > > > > > > + queue_work = false; > > > > > > + if (optee->rpmb_dev_scan_in_progress) > > > > > > + optee->rpmb_dev_request_rescan = true; > > > > > > + } > > > > > > + if (queue_work) > > > > > > + optee->rpmb_dev_scan_in_progress = true; > > > > > > + mutex_unlock(&optee->rpmb_dev_mutex); > > > > > > + > > > > > > + if (queue_work) { > > > > > > + INIT_WORK(&optee->scan_rpmb_work, optee_rpmb_scan); > > > > > > + schedule_work(&optee->scan_rpmb_work); > > > > > > > > > > Can we reuse optee->scan_bus_work rather than introducing a new one here? > > > > > > > > No, both may be active at the same time. > > > > > > Actually both of them are using system_wq underneath, so it shouldn't > > > be a problem if both are active at the same time as they can be queued > > > simultaneously, right? > > > > I'm sorry, I don't get it. > > Are you suggesting to merge optee_rpmb_scan() and optee_bus_scan()? > > If so, how to tell what to do, that is, if tee-supplicant has become > > available or of we should scan for RPMB devices? > > If not, you can only have one callback at a time in a work_struct. > > You are right, keep it as it is. > > > > > > > > > > We'd have to merge > > > > optee_rpmb_scan() and optee_bus_scan(), but I'm not sure it's worth > > > > it. > > > > > > > > > > > > > > > + } > > > > > > +} > > > > > > + > > > > > > static void optee_bus_scan(struct work_struct *work) > > > > > > { > > > > > > WARN_ON(optee_enumerate_devices(PTA_CMD_GET_DEVICES_SUPP)); > > > > > > @@ -161,6 +213,7 @@ void optee_release_supp(struct tee_context *ctx) > > > > > > > > > > > > void optee_remove_common(struct optee *optee) > > > > > > { > > > > > > + rpmb_interface_unregister(&optee->rpmb_intf); > > > > > > /* Unregister OP-TEE specific client devices on TEE bus */ > > > > > > optee_unregister_devices(); > > > > > > > > > > > > @@ -177,6 +230,8 @@ void optee_remove_common(struct optee *optee) > > > > > > tee_shm_pool_free(optee->pool); > > > > > > optee_supp_uninit(&optee->supp); > > > > > > mutex_destroy(&optee->call_queue.mutex); > > > > > > + rpmb_dev_put(optee->rpmb_dev); > > > > > > + mutex_destroy(&optee->rpmb_dev_mutex); > > > > > > } > > > > > > > > > > > > static int smc_abi_rc; > > > > > > diff --git a/drivers/tee/optee/ffa_abi.c b/drivers/tee/optee/ffa_abi.c > > > > > > index ecb5eb079408..befe19ecc30a 100644 > > > > > > --- a/drivers/tee/optee/ffa_abi.c > > > > > > +++ b/drivers/tee/optee/ffa_abi.c > > > > > > @@ -7,6 +7,7 @@ > > > > > > > > > > > > #include <linux/arm_ffa.h> > > > > > > #include <linux/errno.h> > > > > > > +#include <linux/rpmb.h> > > > > > > #include <linux/scatterlist.h> > > > > > > #include <linux/sched.h> > > > > > > #include <linux/slab.h> > > > > > > @@ -934,6 +935,7 @@ static int optee_ffa_probe(struct ffa_device *ffa_dev) > > > > > > optee_cq_init(&optee->call_queue, 0); > > > > > > optee_supp_init(&optee->supp); > > > > > > 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); > > > > > > if (IS_ERR(ctx)) { > > > > > > @@ -955,6 +957,8 @@ static int optee_ffa_probe(struct ffa_device *ffa_dev) > > > > > > if (rc) > > > > > > goto err_unregister_devices; > > > > > > > > > > > > + optee->rpmb_intf.add_rdev = optee_rpmb_intf_add_rdev; > > > > > > + rpmb_interface_register(&optee->rpmb_intf); > > > > > > pr_info("initialized driver\n"); > > > > > > return 0; > > > > > > > > > > > > @@ -968,6 +972,9 @@ static int optee_ffa_probe(struct ffa_device *ffa_dev) > > > > > > teedev_close_context(ctx); > > > > > > err_rhashtable_free: > > > > > > rhashtable_free_and_destroy(&optee->ffa.global_ids, rh_free_fn, NULL); > > > > > > + rpmb_dev_put(optee->rpmb_dev); > > > > > > + mutex_destroy(&optee->rpmb_dev_mutex); > > > > > > + rpmb_interface_unregister(&optee->rpmb_intf); > > > > > > optee_supp_uninit(&optee->supp); > > > > > > mutex_destroy(&optee->call_queue.mutex); > > > > > > mutex_destroy(&optee->ffa.mutex); > > > > > > diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h > > > > > > index 7a5243c78b55..1e4c33baef43 100644 > > > > > > --- a/drivers/tee/optee/optee_private.h > > > > > > +++ b/drivers/tee/optee/optee_private.h > > > > > > @@ -8,6 +8,7 @@ > > > > > > > > > > > > #include <linux/arm-smccc.h> > > > > > > #include <linux/rhashtable.h> > > > > > > +#include <linux/rpmb.h> > > > > > > #include <linux/semaphore.h> > > > > > > #include <linux/tee_drv.h> > > > > > > #include <linux/types.h> > > > > > > @@ -20,11 +21,13 @@ > > > > > > /* Some Global Platform error codes used in this driver */ > > > > > > #define TEEC_SUCCESS 0x00000000 > > > > > > #define TEEC_ERROR_BAD_PARAMETERS 0xFFFF0006 > > > > > > +#define TEEC_ERROR_ITEM_NOT_FOUND 0xFFFF0008 > > > > > > #define TEEC_ERROR_NOT_SUPPORTED 0xFFFF000A > > > > > > #define TEEC_ERROR_COMMUNICATION 0xFFFF000E > > > > > > #define TEEC_ERROR_OUT_OF_MEMORY 0xFFFF000C > > > > > > #define TEEC_ERROR_BUSY 0xFFFF000D > > > > > > #define TEEC_ERROR_SHORT_BUFFER 0xFFFF0010 > > > > > > +#define TEE_ERROR_STORAGE_NOT_AVAILABLE 0xF0100003 > > > > > > > > > > > > #define TEEC_ORIGIN_COMMS 0x00000002 > > > > > > > > > > > > @@ -197,6 +200,8 @@ struct optee_ops { > > > > > > * @notif: notification synchronization struct > > > > > > * @supp: supplicant synchronization struct for RPC to supplicant > > > > > > * @pool: shared memory pool > > > > > > + * @mutex: mutex protecting @rpmb_dev > > > > > > + * @rpmb_dev: current RPMB device or NULL > > > > > > * @rpc_param_count: If > 0 number of RPC parameters to make room for > > > > > > * @scan_bus_done flag if device registation was already done. > > > > > > * @scan_bus_work workq to scan optee bus and register optee drivers > > > > > > @@ -215,9 +220,17 @@ struct optee { > > > > > > struct optee_notif notif; > > > > > > struct optee_supp supp; > > > > > > struct tee_shm_pool *pool; > > > > > > + /* Protects rpmb_dev pointer and rpmb_dev_* */ > > > > > > + struct mutex rpmb_dev_mutex; > > > > > > > > > > Given my comments above, do we really need this mutex? > > > > > > > > I don't see how we can do without the mutex. > > > > > > See if it is possible with the above mentioned approach. > > > > I don't follow. > > Probably my explanation above to handle complexity within > optee_enumerate_devices() wasn't clear. Let me try again: > > - Schedule optee_enumerate_devices(PTA_CMD_GET_DEVICES_RPMB) > unconditionally whenever a new RPMB device is registered. > - While registering OP-TEE devices (TA UUIDs), don't register > duplicate devices or better make the device pseudo TA to return the > UUID list only once when RPMB has been discovered successfully. > > This would allow us to drop the fragile handling via > rpmb_dev_scan_in_progress under mutex, right? I got it now. I'll do that in the v4. Thanks for your patience. :-) Cheers, Jens
