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

[Clément Léger]

Le Mon, 7 Mar 2022 11:40:38 +0100,
Jens Wiklander <jens.wiklander@xxxxxxxxxx> a écrit :

> On Mon, Mar 7, 2022 at 10:42 AM Clément Léger <clement.leger@xxxxxxxxxxx> wrote:
> >
> > This drivers allows to communicate with a RTC PTA handled by OP-TEE [1].
> > This PTA allows to query RTC information, set/get time and set/get
> > offset depending on the supported features.
> >
> > [1] https://github.com/OP-TEE/optee_os/pull/5179
> >
> > Signed-off-by: Clément Léger <clement.leger@xxxxxxxxxxx>
> > ---
> >
> > Changes in v2:  
> 
> Hmm, this seems to be a second v2.

Hmpf, I answered to Etienne questions on V1 and forgot I already sent a
V2.

> 
> >  - Rebased over tee-shm-for-v5.18
> >  - Switch to tee_shm_alloc_kernel_buf()
> >  - Use export_uuid() to copy uuid
> >  - Fix warnings reported by checkpatch
> >  - Free SHM in error exit path
> >  - Fix error messages to include ret value and fix wrong IOCTL names
> >  - Use 100 columns char limit  
> 
> From bdc48fa11e46 ("checkpatch/coding-style: deprecate 80-column warning"):
>     Yes, staying withing 80 columns is certainly still _preferred_.  But
>     it's not the hard limit that the checkpatch warnings imply, and other
>     concerns can most certainly dominate.
> 
>     Increase the default limit to 100 characters.  Not because 100
>     characters is some hard limit either, but that's certainly a "what are
>     you doing" kind of value and less likely to be about the occasional
>     slightly longer lines.

Ok.

> > +
> > +       if (param[0].u.memref.size != sizeof(*optee_tm)) {
> > +               dev_err(dev, "Invalid read size from OPTEE\n");
> > +               return -EPROTO;
> > +       }  
> 
> The dev_err() prints above are basically covering "can't happen"
> cases. Robust code should certainly do the checks, but I'm not so sure
> about how useful the prints are.

Agreed, if it fails, this is likely to be due to protocol changes and
thus, the developer will probably know where to search for the error.

[...]

> > +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 optee_rtc *priv;
> > +       struct rtc_device *rtc;
> > +       struct tee_shm *shm;
> > +       int ret, err;
> > +
> > +       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 */
> > +       export_uuid(sess_arg.uuid, &rtc_device->id.uuid);
> > +       sess_arg.clnt_login = TEE_IOCTL_LOGIN_REE_KERNEL;
> > +
> > +       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);  
> 
> This print is the most useful print in the driver. This is typically
> reached if the PTA doesn't exist.

If the PTA does not exists, is the driver even probed ? I thought it
was based on the UUID matching.

> 
> > +               err = -EINVAL;
> > +               goto out_ctx;
> > +       }
> > +       priv->session_id = sess_arg.session;
> > +
> > +       shm = tee_shm_alloc_kernel_buf(priv->ctx, sizeof(struct optee_rtc_info));
> > +       if (IS_ERR(shm)) {
> > +               dev_err(priv->dev, "tee_shm_alloc_kernel_buf failed\n");
> > +               err = PTR_ERR(shm);
> > +               goto out_sess;
> > +       }
> > +
> > +       priv->shm = shm;
> > +       priv->dev = dev;
> > +       dev_set_drvdata(dev, priv);
> > +
> > +       rtc->ops = &optee_rtc_ops;
> > +
> > +       err = optee_rtc_read_info(dev, rtc, &priv->features);
> > +       if (err) {
> > +               dev_err(dev, "Failed to get RTC features from OP-TEE\n");  
> 
> This print could also be worth keeping, but the rest are in my opinion
> of limited interest.
> 
> It's a tradeoff with the prints, no big deal if you'd like to keep more.

I'm ok with that statement. The runtime errors are less likely (if not
totally unlikely) to happen. I'll sent a new version (V4 this time...)
with theses modifications.

Thanks,

-- 
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com