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
