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

[Clément Léger]

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

> > +
> > +       /* 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.

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