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

[Jens Wiklander]

On Mon, Mar 7, 2022 at 12:01 PM Clément Léger <clement.leger@xxxxxxxxxxx> wrote:
>
> 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.

Yes, it is, but perhaps there's some configuration mismatch or something.

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

OK.

Cheers,
Jens

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