diff mbox series

[PATCHv6,1/3] optee: use uuid for sysfs driver entry

Message ID 20200601150645.13412-2-maxim.uvarov@linaro.org (mailing list archive)
State Rejected
Headers show
Series optee: register drivers on optee bus | expand

Commit Message

Maxim Uvarov June 1, 2020, 3:06 p.m. UTC 
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>
---
 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

Comments

Jarkko Sakkinen June 4, 2020, 8:36 a.m. UTC | #1 
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?

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

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

/Jarkko
Maxim Uvarov June 4, 2020, 9:16 a.m. UTC | #2 
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.
Sumit Garg June 4, 2020, 9:21 a.m. UTC | #3 
On Thu, 4 Jun 2020 at 14:07, 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?
>

It's just about the device being added with the same name as of the
device which is already present on the TEE bus. So this leads to
duplicate sysfs entry error for device node which is created during
device_register() execution flow.

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

Is this an expected error scenario? dev_set_name() is invoked at
numerous places in the kernel without any error check.

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

As of now, there isn't any compatibility concerns with user space as
these TEE devices are meant to be used by kernel drivers only. TEE
user-space interface is quite separate (see: Documentation/tee.txt).

-Sumit

>
> /Jarkko
Jarkko Sakkinen June 16, 2020, 8:13 p.m. UTC | #4 
On Thu, Jun 04, 2020 at 12:16:21PM +0300, Maxim Uvarov wrote:
> 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.

Got it, thanks.

/Jarkko
Jarkko Sakkinen June 16, 2020, 8:15 p.m. UTC | #5 
On Thu, Jun 04, 2020 at 02:51:03PM +0530, Sumit Garg wrote:
> On Thu, 4 Jun 2020 at 14:07, 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?
> >
> 
> It's just about the device being added with the same name as of the
> device which is already present on the TEE bus. So this leads to
> duplicate sysfs entry error for device node which is created during
> device_register() execution flow.
> 
> > > ---
> > >  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.
> 
> Is this an expected error scenario? dev_set_name() is invoked at
> numerous places in the kernel without any error check.
> 
> >
> > Also, I don't get how you can just swap the name without potentially
> > breaking the backwards compatiblity towards the user space.
> 
> As of now, there isn't any compatibility concerns with user space as
> these TEE devices are meant to be used by kernel drivers only. TEE
> user-space interface is quite separate (see: Documentation/tee.txt).

OK, probably would make sense to have this in the commit message as a
remark.

/Jarkko
diff mbox series

Patch

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);
 	uuid_copy(&optee_device->id.uuid, device_uuid);
 
 	rc = device_register(&optee_device->dev);
@@ -144,7 +144,7 @@  int optee_enumerate_devices(void)
 	num_devices = shm_size / sizeof(uuid_t);
 
 	for (idx = 0; idx < num_devices; idx++) {
-		rc = optee_register_device(&device_uuid[idx], idx);
+		rc = optee_register_device(&device_uuid[idx]);
 		if (rc)
 			goto out_shm;
 	}