Re: [PATCH] rtc: optee: add RTC driver for OP-TEE RTC PTA

[Jens Wiklander]

On Thu, Mar 3, 2022 at 9:17 AM Clément Léger <clement.leger@xxxxxxxxxxx> wrote:
>
> Le Thu, 3 Mar 2022 08:35:17 +0100,
> Jens Wiklander <jens.wiklander@xxxxxxxxxx> a écrit :
>
> Hi Jens, looks like you forgot to "Reply All".

You're right.

>
> > > +
> > > +       /* Fill invoke cmd params */
> > > +       param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT;
> > > +       param[0].u.memref.shm = priv->entropy_shm_pool;
> > > +       param[0].u.memref.size = sizeof(struct optee_rtc_time);
> > > +       param[0].u.memref.shm_offs = 0;
> > > +
> > > +       ret = tee_client_invoke_func(priv->ctx, &inv_arg, param);
> > > +       if ((ret < 0) || (inv_arg.ret != 0)) {
> >
> > if (ret < 0 || inv_arg.ret) looks easier to me. Please check the patch
> > with checkpatch --strict do catch all these and other trivial checks.
>
> Acked, the rng-optee driver might not be a good starting point :/ Will
> check that.
>
> > > +static int optee_rtc_read_info(struct device *dev, struct rtc_device *rtc,
> > > +                              u64 *features)
> > > +{
> > > +       struct optee_rtc *priv = dev_get_drvdata(dev);
> > > +       struct tee_ioctl_invoke_arg inv_arg = {0};
> > > +       struct tee_param param[4] = {0};
> > > +       struct optee_rtc_info *info;
> > > +       struct optee_rtc_time *tm;
> > > +       int ret = 0;
> > > +
> > > +       inv_arg.func = TA_CMD_RTC_GET_INFO;
> > > +       inv_arg.session = priv->session_id;
> > > +       inv_arg.num_params = 4;
> > > +
> > > +       param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT;
> > > +       param[0].u.memref.shm = priv->entropy_shm_pool;
> > > +       param[0].u.memref.size = sizeof(*info);
> > > +       param[0].u.memref.shm_offs = 0;
> > > +
> > > +       ret = tee_client_invoke_func(priv->ctx, &inv_arg, param);
> > > +       if ((ret < 0) || (inv_arg.ret != 0)) {
> > > +               dev_err(dev, "TA_CMD_RTC_GET_RNG_INFO invoke err: %x\n",
> > > +                       inv_arg.ret);
> > > +               return -EINVAL;
> > > +       }
> > > +
> > > +       info = tee_shm_get_va(priv->entropy_shm_pool, 0);
> > > +       if (IS_ERR(info)) {
> > > +               dev_err(dev, "tee_shm_get_va failed\n");
> > > +               return PTR_ERR(info);
> > > +       }
> > > +
> > > +       if (param[0].u.memref.size < sizeof(*info)) {
> > > +               dev_err(dev, "Invalid read size from OPTEE\n");
> > > +               return -EINVAL;
> > > +       }
> > > +
> > > +       if (info->version != RTC_INFO_VERSION) {
> > > +               dev_err(dev, "Unsupported information version %llu\n",
> > > +                             info->version);
> > > +               return -EINVAL;
> > > +       }
> > > +
> > > +       *features = info->features;
> > > +
> > > +       tm = &info->range_min;
> > > +       rtc->range_min = mktime64(tm->tm_year, tm->tm_mon, tm->tm_mday,
> > > +                                 tm->tm_hour, tm->tm_min, tm->tm_sec);
> > > +       tm = &info->range_max;
> > > +       rtc->range_max = mktime64(tm->tm_year, tm->tm_mon, tm->tm_mday,
> > > +                                 tm->tm_hour, tm->tm_min, tm->tm_sec);
> >
> > If this is the only thing we're going to do with the returned min and
> > max, how come this wasn't done on the OP-TEE side already? Without
> > these large structs we could have defined an ABI without the need for
> > shared memory.
>
> mktime64 converts the time range to the number of seconds elapsed since
> 1970. I thought it would be better not to assess anything about the way
> the time is stored on OP-TEE side and thus return the full time struct.
> If you think it's acceptable to suppose that, then, we can still switch
> to using timestamps as the ABI with OP-TEE RTC PTA. The same could be
> done to get/set time if needed in this case.

This is a bit more complicated than I first thought. The proposed ABI
is fine from my point of view.

Cheers,
Jens

>
> > > +
> > > +static int optee_rtc_probe(struct device *dev)
> > > +{
> > > +       struct tee_client_device *rtc_device = to_tee_client_device(dev);
> > > +       struct tee_ioctl_open_session_arg sess_arg;
> > > +       struct tee_shm *entropy_shm_pool = NULL;
> > > +       int ret = 0, err = -ENODEV;
> >
> > There is generally no need to initialize these here.
> >
> > > +       struct optee_rtc *priv;
> > > +       struct rtc_device *rtc;
> > > +
> > > +       memset(&sess_arg, 0, sizeof(sess_arg));
> > > +
> > > +       priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > > +       if (!priv)
> > > +               return -ENOMEM;
> > > +
> > > +       rtc = devm_rtc_allocate_device(dev);
> > > +       if (IS_ERR(rtc))
> > > +               return PTR_ERR(rtc);
> > > +
> > > +       /* Open context with TEE driver */
> > > +       priv->ctx = tee_client_open_context(NULL, optee_ctx_match, NULL,
> > > +                                              NULL);
> > > +       if (IS_ERR(priv->ctx))
> > > +               return -ENODEV;
> > > +
> > > +       /* Open session with rtc Trusted App */
> > > +       memcpy(sess_arg.uuid, rtc_device->id.uuid.b, TEE_IOCTL_UUID_LEN);
> >
> > Perhaps uuid_copy() would be better.
>
> Ok, I will use export_uuid() then.
>
> >
> > > +       sess_arg.clnt_login = TEE_IOCTL_LOGIN_REE_KERNEL;
> > > +       sess_arg.num_params = 0;
> > > +
> > > +       ret = tee_client_open_session(priv->ctx, &sess_arg, NULL);
> > > +       if ((ret < 0) || (sess_arg.ret != 0)) {
> > > +               dev_err(dev, "tee_client_open_session failed, err: %x\n",
> > > +                       sess_arg.ret);
> > > +               err = -EINVAL;
> > > +               goto out_ctx;
> > > +       }
> > > +       priv->session_id = sess_arg.session;
> > > +
> > > +       entropy_shm_pool = tee_shm_alloc(priv->ctx,
> > > +                                        sizeof(struct optee_rtc_info),
> > > +                                        TEE_SHM_MAPPED | TEE_SHM_DMA_BUF);
> >
> > tee_shm_alloc() is about to be removed. Please rebase on
> > https://git.linaro.org/people/jens.wiklander/linux-tee.git/tag/?h=tee-shm-for-v5.18
>
> Acked.
>
> --
> Clément Léger,
> Embedded Linux and Kernel engineer at Bootlin
> https://bootlin.com