Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200601150645.13412-1-maxim.uvarov@linaro.org>
 <20200601150645.13412-2-maxim.uvarov@linaro.org> <20200604083655.GA4026@linux.intel.com>
In-Reply-To: <20200604083655.GA4026@linux.intel.com>
From:   Maxim Uvarov <maxim.uvarov@linaro.org>
Date:   Thu, 4 Jun 2020 12:16:21 +0300
Message-ID: <CAD8XO3b4K6ozHwKf2NDJ482JQY6CPXKKxRuZgo5snP16i_5E=Q@mail.gmail.com>
Subject: Re: [PATCHv6 1/3] optee: use uuid for sysfs driver entry
To:     Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Cc:     Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        "tee-dev @ lists . linaro . org" <tee-dev@lists.linaro.org>,
        peterhuewe@gmx.de, Jason Gunthorpe <jgg@ziepe.ca>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Jens Wiklander <jens.wiklander@linaro.org>,
        linux-integrity@vger.kernel.org, Arnd Bergmann <arnd@linaro.org>,
        Sumit Garg <sumit.garg@linaro.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, 4 Jun 2020 at 11:37, Jarkko Sakkinen
<jarkko.sakkinen@linux.intel.com> wrote:
>
> On Mon, Jun 01, 2020 at 06:06:43PM +0300, Maxim Uvarov wrote:
> > With the evolving use-cases for TEE bus, now it's required to support
> > multi-stage enumeration process. But using a simple index doesn't
> > suffice this requirement and instead leads to duplicate sysfs entries.
> > So instead switch to use more informative device UUID for sysfs entry
> > like:
> > /sys/bus/tee/devices/optee-ta-<uuid>
> >
> > Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
> > Reviewed-by: Sumit Garg <sumit.garg@linaro.org>
>
> Why do you mean by duplicate sysfs entries?
>

Without this change it will try to register something like that:
first stage w/o tee-supplicant:
/sys/bus/tee/devices/optee-clnt0
/sys/bus/tee/devices/optee-clnt1
/sys/bus/tee/devices/optee-clnt3
Then with tee-supplicant also index started with 0. I.e.
/sys/bus/tee/devices/optee-clnt0
/sys/bus/tee/devices/optee-clnt1
...

So we need to increase global index or use some other unique names.


> > ---
> >  Documentation/ABI/testing/sysfs-bus-optee-devices | 8 ++++++++
> >  MAINTAINERS                                       | 1 +
> >  drivers/tee/optee/device.c                        | 6 +++---
> >  3 files changed, 12 insertions(+), 3 deletions(-)
> >  create mode 100644 Documentation/ABI/testing/sysfs-bus-optee-devices
> >
> > diff --git a/Documentation/ABI/testing/sysfs-bus-optee-devices b/Documentation/ABI/testing/sysfs-bus-optee-devices
> > new file mode 100644
> > index 000000000000..0ae04ae5374a
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-bus-optee-devices
> > @@ -0,0 +1,8 @@
> > +What:                /sys/bus/tee/devices/optee-ta-<uuid>/
> > +Date:           May 2020
> > +KernelVersion   5.7
> > +Contact:        tee-dev@lists.linaro.org
> > +Description:
> > +             OP-TEE bus provides reference to registered drivers under this directory. The <uuid>
> > +             matches Trusted Application (TA) driver and corresponding TA in secure OS. Drivers
> > +             are free to create needed API under optee-ta-<uuid> directory.
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index ecc0749810b0..6717afef2de3 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -12516,6 +12516,7 @@ OP-TEE DRIVER
> >  M:   Jens Wiklander <jens.wiklander@linaro.org>
> >  L:   tee-dev@lists.linaro.org
> >  S:   Maintained
> > +F:   Documentation/ABI/testing/sysfs-bus-optee-devices
> >  F:   drivers/tee/optee/
> >
> >  OP-TEE RANDOM NUMBER GENERATOR (RNG) DRIVER
> > diff --git a/drivers/tee/optee/device.c b/drivers/tee/optee/device.c
> > index e3a148521ec1..ed3d1ddfa52b 100644
> > --- a/drivers/tee/optee/device.c
> > +++ b/drivers/tee/optee/device.c
> > @@ -65,7 +65,7 @@ static int get_devices(struct tee_context *ctx, u32 session,
> >       return 0;
> >  }
> >
> > -static int optee_register_device(const uuid_t *device_uuid, u32 device_id)
> > +static int optee_register_device(const uuid_t *device_uuid)
> >  {
> >       struct tee_client_device *optee_device = NULL;
> >       int rc;
> > @@ -75,7 +75,7 @@ static int optee_register_device(const uuid_t *device_uuid, u32 device_id)
> >               return -ENOMEM;
> >
> >       optee_device->dev.bus = &tee_bus_type;
> > -     dev_set_name(&optee_device->dev, "optee-clnt%u", device_id);
> > +     dev_set_name(&optee_device->dev, "optee-ta-%pUl", device_uuid);
>
> This code is and already was broken. If dev_set_name() returns -ENOMEM,
> the name will be a null pointer.
>

Well in the current code base only 1% of drivers check the return code
of dev_set_name(). But I don't  mind adding this check.


> Also, I don't get how you can just swap the name without potentially
> breaking the backwards compatiblity towards the user space.
>
> /Jarkko

Backwards compatibility to what? API (stable, testing) are defined in
Documentation. There was no doc for that. And user get notifications
on uevent, so it's udev deal how to find proper sysfs entry.

Regards,
Maxim.