[PATCH v3 0/2] tee: optee: Fixes for supplicant dependent enumeration

[PATCH v3 0/2] tee: optee: Fixes for supplicant dependent enumeration

[Sumit Garg]

Currently supplicant dependent optee device enumeration only registers
devices whenever tee-supplicant is invoked for the first time. But it
forgets to remove devices when tee-supplicant daemon stops running and
closes its context gracefully. This leads to following error for fTPM
driver during reboot/shutdown:

[   73.466791] tpm tpm0: ftpm_tee_tpm_op_send: SUBMIT_COMMAND invoke error: 0xffff3024

Fix this by separating supplicant dependent devices so that the
user-space service can detach supplicant devices before closing the
supplicant. While at it use the global system workqueue for OP-TEE bus
scanning work rather than our own custom one.

Changes in v3:

- Split patch into 2 separate ones, one for supplicant fix and other for
  the workqueue. 

Changes in v2:

- Use device names to separate out tee-supplicant dependent devices via
  this patch.
- Since user-space service is aware about tee-supplicant lifespan, so
  allow the user-space service to unbind tee-supplicant dependent
  devices before killing the supplicant. Following command has to be
  added to the tee-supplicant service file.

  $ for dev in /sys/bus/tee/devices/*; do if [[ "$dev" == *"optee-ta-supp-"* ]]; \
        then echo $(basename "$dev") > $dev/driver/unbind; fi done

Sumit Garg (2):
  tee: optee: Fix supplicant based device enumeration
  tee: optee: Remove redundant custom workqueue

 drivers/tee/optee/core.c          | 13 ++-----------
 drivers/tee/optee/device.c        | 13 ++++++++++---
 drivers/tee/optee/optee_private.h |  2 --
 3 files changed, 12 insertions(+), 16 deletions(-)

-- 
2.34.1

[PATCH v3 1/2] tee: optee: Fix supplicant based device enumeration

[Sumit Garg]

Currently supplicant dependent optee device enumeration only registers
devices whenever tee-supplicant is invoked for the first time. But it
forgets to remove devices when tee-supplicant daemon stops running and
closes its context gracefully. This leads to following error for fTPM
driver during reboot/shutdown:

[   73.466791] tpm tpm0: ftpm_tee_tpm_op_send: SUBMIT_COMMAND invoke error: 0xffff3024

Fix this by separating supplicant dependent devices so that the
user-space service can detach supplicant devices before closing the
supplicant.

Reported-by: Jan Kiszka <jan.kiszka@siemens.com>
Link: https://github.com/OP-TEE/optee_os/issues/6094
Fixes: 5f178bb71e3a ("optee: enable support for multi-stage bus enumeration")
Tested-by: Jan Kiszka <jan.kiszka@siemens.com>
Tested-by: Masahisa Kojima <masahisa.kojima@linaro.org>
Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
---
 drivers/tee/optee/device.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/tee/optee/device.c b/drivers/tee/optee/device.c
index 64f0e047c23d..78fc0a15c463 100644
--- a/drivers/tee/optee/device.c
+++ b/drivers/tee/optee/device.c
@@ -60,9 +60,10 @@ static void optee_release_device(struct device *dev)
 	kfree(optee_device);
 }
 
-static int optee_register_device(const uuid_t *device_uuid)
+static int optee_register_device(const uuid_t *device_uuid, u32 func)
 {
 	struct tee_client_device *optee_device = NULL;
+	const char *dev_name_fmt = NULL;
 	int rc;
 
 	optee_device = kzalloc(sizeof(*optee_device), GFP_KERNEL);
@@ -71,7 +72,13 @@ static int optee_register_device(const uuid_t *device_uuid)
 
 	optee_device->dev.bus = &tee_bus_type;
 	optee_device->dev.release = optee_release_device;
-	if (dev_set_name(&optee_device->dev, "optee-ta-%pUb", device_uuid)) {
+
+	if (func == PTA_CMD_GET_DEVICES_SUPP)
+		dev_name_fmt = "optee-ta-supp-%pUb";
+	else
+		dev_name_fmt = "optee-ta-%pUb";
+
+	if (dev_set_name(&optee_device->dev, dev_name_fmt, device_uuid)) {
 		kfree(optee_device);
 		return -ENOMEM;
 	}
@@ -142,7 +149,7 @@ static int __optee_enumerate_devices(u32 func)
 	num_devices = shm_size / sizeof(uuid_t);
 
 	for (idx = 0; idx < num_devices; idx++) {
-		rc = optee_register_device(&device_uuid[idx]);
+		rc = optee_register_device(&device_uuid[idx], func);
 		if (rc)
 			goto out_shm;
 	}
-- 
2.34.1

Re: [PATCH v3 1/2] tee: optee: Fix supplicant based device enumeration

[Jerome Forissier]

On 10/30/23 16:59, Sumit Garg wrote:
> Currently supplicant dependent optee device enumeration only registers
> devices whenever tee-supplicant is invoked for the first time. But it
> forgets to remove devices when tee-supplicant daemon stops running and
> closes its context gracefully. This leads to following error for fTPM
> driver during reboot/shutdown:
> 
> [   73.466791] tpm tpm0: ftpm_tee_tpm_op_send: SUBMIT_COMMAND invoke error: 0xffff3024
> 
> Fix this by separating supplicant dependent devices so that the
> user-space service can detach supplicant devices before closing the
> supplicant.
> 
> Reported-by: Jan Kiszka <jan.kiszka@siemens.com>
> Link: https://github.com/OP-TEE/optee_os/issues/6094
> Fixes: 5f178bb71e3a ("optee: enable support for multi-stage bus enumeration")
> Tested-by: Jan Kiszka <jan.kiszka@siemens.com>
> Tested-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> ---
>  drivers/tee/optee/device.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/tee/optee/device.c b/drivers/tee/optee/device.c
> index 64f0e047c23d..78fc0a15c463 100644
> --- a/drivers/tee/optee/device.c
> +++ b/drivers/tee/optee/device.c
> @@ -60,9 +60,10 @@ static void optee_release_device(struct device *dev)
>  	kfree(optee_device);
>  }
>  
> -static int optee_register_device(const uuid_t *device_uuid)
> +static int optee_register_device(const uuid_t *device_uuid, u32 func)
>  {
>  	struct tee_client_device *optee_device = NULL;
> +	const char *dev_name_fmt = NULL;
>  	int rc;
>  
>  	optee_device = kzalloc(sizeof(*optee_device), GFP_KERNEL);
> @@ -71,7 +72,13 @@ static int optee_register_device(const uuid_t *device_uuid)
>  
>  	optee_device->dev.bus = &tee_bus_type;
>  	optee_device->dev.release = optee_release_device;
> -	if (dev_set_name(&optee_device->dev, "optee-ta-%pUb", device_uuid)) {
> +
> +	if (func == PTA_CMD_GET_DEVICES_SUPP)
> +		dev_name_fmt = "optee-ta-supp-%pUb";
> +	else
> +		dev_name_fmt = "optee-ta-%pUb";

That's an ABI change, isn't it?

> +
> +	if (dev_set_name(&optee_device->dev, dev_name_fmt, device_uuid)) {
>  		kfree(optee_device);
>  		return -ENOMEM;
>  	}
> @@ -142,7 +149,7 @@ static int __optee_enumerate_devices(u32 func)
>  	num_devices = shm_size / sizeof(uuid_t);
>  
>  	for (idx = 0; idx < num_devices; idx++) {
> -		rc = optee_register_device(&device_uuid[idx]);
> +		rc = optee_register_device(&device_uuid[idx], func);
>  		if (rc)
>  			goto out_shm;
>  	}

-- 
Jerome

Re: [PATCH v3 1/2] tee: optee: Fix supplicant based device enumeration

[Jan Kiszka]

On 31.10.23 12:04, Jerome Forissier wrote:
> 
> 
> On 10/30/23 16:59, Sumit Garg wrote:
>> Currently supplicant dependent optee device enumeration only registers
>> devices whenever tee-supplicant is invoked for the first time. But it
>> forgets to remove devices when tee-supplicant daemon stops running and
>> closes its context gracefully. This leads to following error for fTPM
>> driver during reboot/shutdown:
>>
>> [   73.466791] tpm tpm0: ftpm_tee_tpm_op_send: SUBMIT_COMMAND invoke error: 0xffff3024
>>
>> Fix this by separating supplicant dependent devices so that the
>> user-space service can detach supplicant devices before closing the
>> supplicant.
>>
>> Reported-by: Jan Kiszka <jan.kiszka@siemens.com>
>> Link: https://github.com/OP-TEE/optee_os/issues/6094
>> Fixes: 5f178bb71e3a ("optee: enable support for multi-stage bus enumeration")
>> Tested-by: Jan Kiszka <jan.kiszka@siemens.com>
>> Tested-by: Masahisa Kojima <masahisa.kojima@linaro.org>
>> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
>> ---
>>  drivers/tee/optee/device.c | 13 ++++++++++---
>>  1 file changed, 10 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/tee/optee/device.c b/drivers/tee/optee/device.c
>> index 64f0e047c23d..78fc0a15c463 100644
>> --- a/drivers/tee/optee/device.c
>> +++ b/drivers/tee/optee/device.c
>> @@ -60,9 +60,10 @@ static void optee_release_device(struct device *dev)
>>  	kfree(optee_device);
>>  }
>>  
>> -static int optee_register_device(const uuid_t *device_uuid)
>> +static int optee_register_device(const uuid_t *device_uuid, u32 func)
>>  {
>>  	struct tee_client_device *optee_device = NULL;
>> +	const char *dev_name_fmt = NULL;
>>  	int rc;
>>  
>>  	optee_device = kzalloc(sizeof(*optee_device), GFP_KERNEL);
>> @@ -71,7 +72,13 @@ static int optee_register_device(const uuid_t *device_uuid)
>>  
>>  	optee_device->dev.bus = &tee_bus_type;
>>  	optee_device->dev.release = optee_release_device;
>> -	if (dev_set_name(&optee_device->dev, "optee-ta-%pUb", device_uuid)) {
>> +
>> +	if (func == PTA_CMD_GET_DEVICES_SUPP)
>> +		dev_name_fmt = "optee-ta-supp-%pUb";
>> +	else
>> +		dev_name_fmt = "optee-ta-%pUb";
> 
> That's an ABI change, isn't it?

Oh, here did this come from! Yes, I recently had to adjust some systemd
service due to carrying this patch but looking for the change only in 
upstream:

https://github.com/ilbers/isar/commit/83644ddf694e51f11793e6107e4aaf68dc0043a5

Jan

> 
>> +
>> +	if (dev_set_name(&optee_device->dev, dev_name_fmt, device_uuid)) {
>>  		kfree(optee_device);
>>  		return -ENOMEM;
>>  	}
>> @@ -142,7 +149,7 @@ static int __optee_enumerate_devices(u32 func)
>>  	num_devices = shm_size / sizeof(uuid_t);
>>  
>>  	for (idx = 0; idx < num_devices; idx++) {
>> -		rc = optee_register_device(&device_uuid[idx]);
>> +		rc = optee_register_device(&device_uuid[idx], func);
>>  		if (rc)
>>  			goto out_shm;
>>  	}
> 

-- 
Siemens AG, Technology
Linux Expert Center

Re: [PATCH v3 1/2] tee: optee: Fix supplicant based device enumeration

[Sumit Garg]

On Tue, 31 Oct 2023 at 17:14, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>
> On 31.10.23 12:04, Jerome Forissier wrote:
> >
> >
> > On 10/30/23 16:59, Sumit Garg wrote:
> >> Currently supplicant dependent optee device enumeration only registers
> >> devices whenever tee-supplicant is invoked for the first time. But it
> >> forgets to remove devices when tee-supplicant daemon stops running and
> >> closes its context gracefully. This leads to following error for fTPM
> >> driver during reboot/shutdown:
> >>
> >> [   73.466791] tpm tpm0: ftpm_tee_tpm_op_send: SUBMIT_COMMAND invoke error: 0xffff3024
> >>
> >> Fix this by separating supplicant dependent devices so that the
> >> user-space service can detach supplicant devices before closing the
> >> supplicant.
> >>
> >> Reported-by: Jan Kiszka <jan.kiszka@siemens.com>
> >> Link: https://github.com/OP-TEE/optee_os/issues/6094
> >> Fixes: 5f178bb71e3a ("optee: enable support for multi-stage bus enumeration")
> >> Tested-by: Jan Kiszka <jan.kiszka@siemens.com>
> >> Tested-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> >> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> >> ---
> >>  drivers/tee/optee/device.c | 13 ++++++++++---
> >>  1 file changed, 10 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/tee/optee/device.c b/drivers/tee/optee/device.c
> >> index 64f0e047c23d..78fc0a15c463 100644
> >> --- a/drivers/tee/optee/device.c
> >> +++ b/drivers/tee/optee/device.c
> >> @@ -60,9 +60,10 @@ static void optee_release_device(struct device *dev)
> >>      kfree(optee_device);
> >>  }
> >>
> >> -static int optee_register_device(const uuid_t *device_uuid)
> >> +static int optee_register_device(const uuid_t *device_uuid, u32 func)
> >>  {
> >>      struct tee_client_device *optee_device = NULL;
> >> +    const char *dev_name_fmt = NULL;
> >>      int rc;
> >>
> >>      optee_device = kzalloc(sizeof(*optee_device), GFP_KERNEL);
> >> @@ -71,7 +72,13 @@ static int optee_register_device(const uuid_t *device_uuid)
> >>
> >>      optee_device->dev.bus = &tee_bus_type;
> >>      optee_device->dev.release = optee_release_device;
> >> -    if (dev_set_name(&optee_device->dev, "optee-ta-%pUb", device_uuid)) {
> >> +
> >> +    if (func == PTA_CMD_GET_DEVICES_SUPP)
> >> +            dev_name_fmt = "optee-ta-supp-%pUb";
> >> +    else
> >> +            dev_name_fmt = "optee-ta-%pUb";
> >
> > That's an ABI change, isn't it?
>

Indeed it is an ABI break although we would like this to be backported
but don't want to break existing users. So I brainstormed on it and
came up with an alternative fix via device attribute in v4. Please
have a look.

> Oh, here did this come from! Yes, I recently had to adjust some systemd
> service due to carrying this patch but looking for the change only in
> upstream:
>
> https://github.com/ilbers/isar/commit/83644ddf694e51f11793e6107e4aaf68dc0043a5
>

You don't need to unbind all of the optee devices. v4 would help you
to maintain backwards compatibility, can you retest it?

-Sumit

> Jan
>
> >
> >> +
> >> +    if (dev_set_name(&optee_device->dev, dev_name_fmt, device_uuid)) {
> >>              kfree(optee_device);
> >>              return -ENOMEM;
> >>      }
> >> @@ -142,7 +149,7 @@ static int __optee_enumerate_devices(u32 func)
> >>      num_devices = shm_size / sizeof(uuid_t);
> >>
> >>      for (idx = 0; idx < num_devices; idx++) {
> >> -            rc = optee_register_device(&device_uuid[idx]);
> >> +            rc = optee_register_device(&device_uuid[idx], func);
> >>              if (rc)
> >>                      goto out_shm;
> >>      }
> >
>
> --
> Siemens AG, Technology
> Linux Expert Center
>

Re: [PATCH v3 1/2] tee: optee: Fix supplicant based device enumeration

[Jan Kiszka]

On 02.11.23 08:37, Sumit Garg wrote:
> On Tue, 31 Oct 2023 at 17:14, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>
>> On 31.10.23 12:04, Jerome Forissier wrote:
>>>
>>>
>>> On 10/30/23 16:59, Sumit Garg wrote:
>>>> Currently supplicant dependent optee device enumeration only registers
>>>> devices whenever tee-supplicant is invoked for the first time. But it
>>>> forgets to remove devices when tee-supplicant daemon stops running and
>>>> closes its context gracefully. This leads to following error for fTPM
>>>> driver during reboot/shutdown:
>>>>
>>>> [   73.466791] tpm tpm0: ftpm_tee_tpm_op_send: SUBMIT_COMMAND invoke error: 0xffff3024
>>>>
>>>> Fix this by separating supplicant dependent devices so that the
>>>> user-space service can detach supplicant devices before closing the
>>>> supplicant.
>>>>
>>>> Reported-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>> Link: https://github.com/OP-TEE/optee_os/issues/6094
>>>> Fixes: 5f178bb71e3a ("optee: enable support for multi-stage bus enumeration")
>>>> Tested-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>> Tested-by: Masahisa Kojima <masahisa.kojima@linaro.org>
>>>> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
>>>> ---
>>>>  drivers/tee/optee/device.c | 13 ++++++++++---
>>>>  1 file changed, 10 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/tee/optee/device.c b/drivers/tee/optee/device.c
>>>> index 64f0e047c23d..78fc0a15c463 100644
>>>> --- a/drivers/tee/optee/device.c
>>>> +++ b/drivers/tee/optee/device.c
>>>> @@ -60,9 +60,10 @@ static void optee_release_device(struct device *dev)
>>>>      kfree(optee_device);
>>>>  }
>>>>
>>>> -static int optee_register_device(const uuid_t *device_uuid)
>>>> +static int optee_register_device(const uuid_t *device_uuid, u32 func)
>>>>  {
>>>>      struct tee_client_device *optee_device = NULL;
>>>> +    const char *dev_name_fmt = NULL;
>>>>      int rc;
>>>>
>>>>      optee_device = kzalloc(sizeof(*optee_device), GFP_KERNEL);
>>>> @@ -71,7 +72,13 @@ static int optee_register_device(const uuid_t *device_uuid)
>>>>
>>>>      optee_device->dev.bus = &tee_bus_type;
>>>>      optee_device->dev.release = optee_release_device;
>>>> -    if (dev_set_name(&optee_device->dev, "optee-ta-%pUb", device_uuid)) {
>>>> +
>>>> +    if (func == PTA_CMD_GET_DEVICES_SUPP)
>>>> +            dev_name_fmt = "optee-ta-supp-%pUb";
>>>> +    else
>>>> +            dev_name_fmt = "optee-ta-%pUb";
>>>
>>> That's an ABI change, isn't it?
>>
> 
> Indeed it is an ABI break although we would like this to be backported
> but don't want to break existing users. So I brainstormed on it and
> came up with an alternative fix via device attribute in v4. Please
> have a look.
> 
>> Oh, here did this come from! Yes, I recently had to adjust some systemd
>> service due to carrying this patch but looking for the change only in
>> upstream:
>>
>> https://github.com/ilbers/isar/commit/83644ddf694e51f11793e6107e4aaf68dc0043a5
>>
> 
> You don't need to unbind all of the optee devices. v4 would help you
> to maintain backwards compatibility, can you retest it?

How do I know from tee-supplicant perspective which devices I need to
unbind? There could be one in the future that will also use storage and
will therefore also fail once the supplicant is gone.

Jan

-- 
Siemens AG, Technology
Linux Expert Center

Re: [PATCH v3 1/2] tee: optee: Fix supplicant based device enumeration

[Sumit Garg]

On Thu, 2 Nov 2023 at 13:28, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>
> On 02.11.23 08:37, Sumit Garg wrote:
> > On Tue, 31 Oct 2023 at 17:14, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> >>
> >> On 31.10.23 12:04, Jerome Forissier wrote:
> >>>
> >>>
> >>> On 10/30/23 16:59, Sumit Garg wrote:
> >>>> Currently supplicant dependent optee device enumeration only registers
> >>>> devices whenever tee-supplicant is invoked for the first time. But it
> >>>> forgets to remove devices when tee-supplicant daemon stops running and
> >>>> closes its context gracefully. This leads to following error for fTPM
> >>>> driver during reboot/shutdown:
> >>>>
> >>>> [   73.466791] tpm tpm0: ftpm_tee_tpm_op_send: SUBMIT_COMMAND invoke error: 0xffff3024
> >>>>
> >>>> Fix this by separating supplicant dependent devices so that the
> >>>> user-space service can detach supplicant devices before closing the
> >>>> supplicant.
> >>>>
> >>>> Reported-by: Jan Kiszka <jan.kiszka@siemens.com>
> >>>> Link: https://github.com/OP-TEE/optee_os/issues/6094
> >>>> Fixes: 5f178bb71e3a ("optee: enable support for multi-stage bus enumeration")
> >>>> Tested-by: Jan Kiszka <jan.kiszka@siemens.com>
> >>>> Tested-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> >>>> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> >>>> ---
> >>>>  drivers/tee/optee/device.c | 13 ++++++++++---
> >>>>  1 file changed, 10 insertions(+), 3 deletions(-)
> >>>>
> >>>> diff --git a/drivers/tee/optee/device.c b/drivers/tee/optee/device.c
> >>>> index 64f0e047c23d..78fc0a15c463 100644
> >>>> --- a/drivers/tee/optee/device.c
> >>>> +++ b/drivers/tee/optee/device.c
> >>>> @@ -60,9 +60,10 @@ static void optee_release_device(struct device *dev)
> >>>>      kfree(optee_device);
> >>>>  }
> >>>>
> >>>> -static int optee_register_device(const uuid_t *device_uuid)
> >>>> +static int optee_register_device(const uuid_t *device_uuid, u32 func)
> >>>>  {
> >>>>      struct tee_client_device *optee_device = NULL;
> >>>> +    const char *dev_name_fmt = NULL;
> >>>>      int rc;
> >>>>
> >>>>      optee_device = kzalloc(sizeof(*optee_device), GFP_KERNEL);
> >>>> @@ -71,7 +72,13 @@ static int optee_register_device(const uuid_t *device_uuid)
> >>>>
> >>>>      optee_device->dev.bus = &tee_bus_type;
> >>>>      optee_device->dev.release = optee_release_device;
> >>>> -    if (dev_set_name(&optee_device->dev, "optee-ta-%pUb", device_uuid)) {
> >>>> +
> >>>> +    if (func == PTA_CMD_GET_DEVICES_SUPP)
> >>>> +            dev_name_fmt = "optee-ta-supp-%pUb";
> >>>> +    else
> >>>> +            dev_name_fmt = "optee-ta-%pUb";
> >>>
> >>> That's an ABI change, isn't it?
> >>
> >
> > Indeed it is an ABI break although we would like this to be backported
> > but don't want to break existing users. So I brainstormed on it and
> > came up with an alternative fix via device attribute in v4. Please
> > have a look.
> >
> >> Oh, here did this come from! Yes, I recently had to adjust some systemd
> >> service due to carrying this patch but looking for the change only in
> >> upstream:
> >>
> >> https://github.com/ilbers/isar/commit/83644ddf694e51f11793e6107e4aaf68dc0043a5
> >>
> >
> > You don't need to unbind all of the optee devices. v4 would help you
> > to maintain backwards compatibility, can you retest it?
>
> How do I know from tee-supplicant perspective which devices I need to
> unbind? There could be one in the future that will also use storage and
> will therefore also fail once the supplicant is gone.
>

With v4, the devices where the below attribute is present need to
unbind before closing tee-supplicant.

/sys/bus/tee/devices/optee-ta-<uuid>/need_supplicant

-Sumit

> Jan
>
> --
> Siemens AG, Technology
> Linux Expert Center
>

Re: [PATCH v3 1/2] tee: optee: Fix supplicant based device enumeration

[Jan Kiszka]

On 02.11.23 09:02, Sumit Garg wrote:
> On Thu, 2 Nov 2023 at 13:28, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>
>> On 02.11.23 08:37, Sumit Garg wrote:
>>> On Tue, 31 Oct 2023 at 17:14, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>>
>>>> On 31.10.23 12:04, Jerome Forissier wrote:
>>>>>
>>>>>
>>>>> On 10/30/23 16:59, Sumit Garg wrote:
>>>>>> Currently supplicant dependent optee device enumeration only registers
>>>>>> devices whenever tee-supplicant is invoked for the first time. But it
>>>>>> forgets to remove devices when tee-supplicant daemon stops running and
>>>>>> closes its context gracefully. This leads to following error for fTPM
>>>>>> driver during reboot/shutdown:
>>>>>>
>>>>>> [   73.466791] tpm tpm0: ftpm_tee_tpm_op_send: SUBMIT_COMMAND invoke error: 0xffff3024
>>>>>>
>>>>>> Fix this by separating supplicant dependent devices so that the
>>>>>> user-space service can detach supplicant devices before closing the
>>>>>> supplicant.
>>>>>>
>>>>>> Reported-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>> Link: https://github.com/OP-TEE/optee_os/issues/6094
>>>>>> Fixes: 5f178bb71e3a ("optee: enable support for multi-stage bus enumeration")
>>>>>> Tested-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>> Tested-by: Masahisa Kojima <masahisa.kojima@linaro.org>
>>>>>> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
>>>>>> ---
>>>>>>  drivers/tee/optee/device.c | 13 ++++++++++---
>>>>>>  1 file changed, 10 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/tee/optee/device.c b/drivers/tee/optee/device.c
>>>>>> index 64f0e047c23d..78fc0a15c463 100644
>>>>>> --- a/drivers/tee/optee/device.c
>>>>>> +++ b/drivers/tee/optee/device.c
>>>>>> @@ -60,9 +60,10 @@ static void optee_release_device(struct device *dev)
>>>>>>      kfree(optee_device);
>>>>>>  }
>>>>>>
>>>>>> -static int optee_register_device(const uuid_t *device_uuid)
>>>>>> +static int optee_register_device(const uuid_t *device_uuid, u32 func)
>>>>>>  {
>>>>>>      struct tee_client_device *optee_device = NULL;
>>>>>> +    const char *dev_name_fmt = NULL;
>>>>>>      int rc;
>>>>>>
>>>>>>      optee_device = kzalloc(sizeof(*optee_device), GFP_KERNEL);
>>>>>> @@ -71,7 +72,13 @@ static int optee_register_device(const uuid_t *device_uuid)
>>>>>>
>>>>>>      optee_device->dev.bus = &tee_bus_type;
>>>>>>      optee_device->dev.release = optee_release_device;
>>>>>> -    if (dev_set_name(&optee_device->dev, "optee-ta-%pUb", device_uuid)) {
>>>>>> +
>>>>>> +    if (func == PTA_CMD_GET_DEVICES_SUPP)
>>>>>> +            dev_name_fmt = "optee-ta-supp-%pUb";
>>>>>> +    else
>>>>>> +            dev_name_fmt = "optee-ta-%pUb";
>>>>>
>>>>> That's an ABI change, isn't it?
>>>>
>>>
>>> Indeed it is an ABI break although we would like this to be backported
>>> but don't want to break existing users. So I brainstormed on it and
>>> came up with an alternative fix via device attribute in v4. Please
>>> have a look.
>>>
>>>> Oh, here did this come from! Yes, I recently had to adjust some systemd
>>>> service due to carrying this patch but looking for the change only in
>>>> upstream:
>>>>
>>>> https://github.com/ilbers/isar/commit/83644ddf694e51f11793e6107e4aaf68dc0043a5
>>>>
>>>
>>> You don't need to unbind all of the optee devices. v4 would help you
>>> to maintain backwards compatibility, can you retest it?
>>
>> How do I know from tee-supplicant perspective which devices I need to
>> unbind? There could be one in the future that will also use storage and
>> will therefore also fail once the supplicant is gone.
>>
> 
> With v4, the devices where the below attribute is present need to
> unbind before closing tee-supplicant.
> 
> /sys/bus/tee/devices/optee-ta-<uuid>/need_supplicant

OK - but that will only help in future kernels, nothing we have today.
Thus, the shutdown script cannot assume to alone kill those devices
unless it find a certain upcoming kernel release.

Jan

-- 
Siemens AG, Technology
Linux Expert Center

Re: [PATCH v3 1/2] tee: optee: Fix supplicant based device enumeration

[Sumit Garg]

On Thu, 2 Nov 2023 at 13:35, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>
> On 02.11.23 09:02, Sumit Garg wrote:
> > On Thu, 2 Nov 2023 at 13:28, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> >>
> >> On 02.11.23 08:37, Sumit Garg wrote:
> >>> On Tue, 31 Oct 2023 at 17:14, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> >>>>
> >>>> On 31.10.23 12:04, Jerome Forissier wrote:
> >>>>>
> >>>>>
> >>>>> On 10/30/23 16:59, Sumit Garg wrote:
> >>>>>> Currently supplicant dependent optee device enumeration only registers
> >>>>>> devices whenever tee-supplicant is invoked for the first time. But it
> >>>>>> forgets to remove devices when tee-supplicant daemon stops running and
> >>>>>> closes its context gracefully. This leads to following error for fTPM
> >>>>>> driver during reboot/shutdown:
> >>>>>>
> >>>>>> [   73.466791] tpm tpm0: ftpm_tee_tpm_op_send: SUBMIT_COMMAND invoke error: 0xffff3024
> >>>>>>
> >>>>>> Fix this by separating supplicant dependent devices so that the
> >>>>>> user-space service can detach supplicant devices before closing the
> >>>>>> supplicant.
> >>>>>>
> >>>>>> Reported-by: Jan Kiszka <jan.kiszka@siemens.com>
> >>>>>> Link: https://github.com/OP-TEE/optee_os/issues/6094
> >>>>>> Fixes: 5f178bb71e3a ("optee: enable support for multi-stage bus enumeration")
> >>>>>> Tested-by: Jan Kiszka <jan.kiszka@siemens.com>
> >>>>>> Tested-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> >>>>>> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> >>>>>> ---
> >>>>>>  drivers/tee/optee/device.c | 13 ++++++++++---
> >>>>>>  1 file changed, 10 insertions(+), 3 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/tee/optee/device.c b/drivers/tee/optee/device.c
> >>>>>> index 64f0e047c23d..78fc0a15c463 100644
> >>>>>> --- a/drivers/tee/optee/device.c
> >>>>>> +++ b/drivers/tee/optee/device.c
> >>>>>> @@ -60,9 +60,10 @@ static void optee_release_device(struct device *dev)
> >>>>>>      kfree(optee_device);
> >>>>>>  }
> >>>>>>
> >>>>>> -static int optee_register_device(const uuid_t *device_uuid)
> >>>>>> +static int optee_register_device(const uuid_t *device_uuid, u32 func)
> >>>>>>  {
> >>>>>>      struct tee_client_device *optee_device = NULL;
> >>>>>> +    const char *dev_name_fmt = NULL;
> >>>>>>      int rc;
> >>>>>>
> >>>>>>      optee_device = kzalloc(sizeof(*optee_device), GFP_KERNEL);
> >>>>>> @@ -71,7 +72,13 @@ static int optee_register_device(const uuid_t *device_uuid)
> >>>>>>
> >>>>>>      optee_device->dev.bus = &tee_bus_type;
> >>>>>>      optee_device->dev.release = optee_release_device;
> >>>>>> -    if (dev_set_name(&optee_device->dev, "optee-ta-%pUb", device_uuid)) {
> >>>>>> +
> >>>>>> +    if (func == PTA_CMD_GET_DEVICES_SUPP)
> >>>>>> +            dev_name_fmt = "optee-ta-supp-%pUb";
> >>>>>> +    else
> >>>>>> +            dev_name_fmt = "optee-ta-%pUb";
> >>>>>
> >>>>> That's an ABI change, isn't it?
> >>>>
> >>>
> >>> Indeed it is an ABI break although we would like this to be backported
> >>> but don't want to break existing users. So I brainstormed on it and
> >>> came up with an alternative fix via device attribute in v4. Please
> >>> have a look.
> >>>
> >>>> Oh, here did this come from! Yes, I recently had to adjust some systemd
> >>>> service due to carrying this patch but looking for the change only in
> >>>> upstream:
> >>>>
> >>>> https://github.com/ilbers/isar/commit/83644ddf694e51f11793e6107e4aaf68dc0043a5
> >>>>
> >>>
> >>> You don't need to unbind all of the optee devices. v4 would help you
> >>> to maintain backwards compatibility, can you retest it?
> >>
> >> How do I know from tee-supplicant perspective which devices I need to
> >> unbind? There could be one in the future that will also use storage and
> >> will therefore also fail once the supplicant is gone.
> >>
> >
> > With v4, the devices where the below attribute is present need to
> > unbind before closing tee-supplicant.
> >
> > /sys/bus/tee/devices/optee-ta-<uuid>/need_supplicant
>
> OK - but that will only help in future kernels, nothing we have today.
> Thus, the shutdown script cannot assume to alone kill those devices
> unless it find a certain upcoming kernel release.
>

This v4 fix will be backported to stable kernels. So you can update
your scripts once it lands into your stable tree.

-Sumit

> Jan
>
> --
> Siemens AG, Technology
> Linux Expert Center
>

[PATCH v3 2/2] tee: optee: Remove redundant custom workqueue

[Sumit Garg]

Global system workqueue is sufficient to suffice OP-TEE bus scanning work
needs. So drop redundant usage of the custom workqueue.

Tested-by: Jan Kiszka <jan.kiszka@siemens.com>
Tested-by: Masahisa Kojima <masahisa.kojima@linaro.org>
Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
---
 drivers/tee/optee/core.c          | 13 ++-----------
 drivers/tee/optee/optee_private.h |  2 --
 2 files changed, 2 insertions(+), 13 deletions(-)

diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
index 2a258bd3b6b5..1eaa191b6ff6 100644
--- a/drivers/tee/optee/core.c
+++ b/drivers/tee/optee/core.c
@@ -15,7 +15,6 @@
 #include <linux/string.h>
 #include <linux/tee_drv.h>
 #include <linux/types.h>
-#include <linux/workqueue.h>
 #include "optee_private.h"
 
 int optee_pool_op_alloc_helper(struct tee_shm_pool *pool, struct tee_shm *shm,
@@ -110,12 +109,7 @@ int optee_open(struct tee_context *ctx, bool cap_memref_null)
 
 		if (!optee->scan_bus_done) {
 			INIT_WORK(&optee->scan_bus_work, optee_bus_scan);
-			optee->scan_bus_wq = create_workqueue("optee_bus_scan");
-			if (!optee->scan_bus_wq) {
-				kfree(ctxdata);
-				return -ECHILD;
-			}
-			queue_work(optee->scan_bus_wq, &optee->scan_bus_work);
+			schedule_work(&optee->scan_bus_work);
 			optee->scan_bus_done = true;
 		}
 	}
@@ -158,10 +152,7 @@ void optee_release_supp(struct tee_context *ctx)
 	struct optee *optee = tee_get_drvdata(ctx->teedev);
 
 	optee_release_helper(ctx, optee_close_session_helper);
-	if (optee->scan_bus_wq) {
-		destroy_workqueue(optee->scan_bus_wq);
-		optee->scan_bus_wq = NULL;
-	}
+
 	optee_supp_release(&optee->supp);
 }
 
diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h
index 6bb5cae09688..94c0ee381894 100644
--- a/drivers/tee/optee/optee_private.h
+++ b/drivers/tee/optee/optee_private.h
@@ -178,7 +178,6 @@ struct optee_ops {
  * @pool:		shared memory pool
  * @rpc_param_count:	If > 0 number of RPC parameters to make room for
  * @scan_bus_done	flag if device registation was already done.
- * @scan_bus_wq		workqueue to scan optee bus and register optee drivers
  * @scan_bus_work	workq to scan optee bus and register optee drivers
  */
 struct optee {
@@ -197,7 +196,6 @@ struct optee {
 	struct tee_shm_pool *pool;
 	unsigned int rpc_param_count;
 	bool   scan_bus_done;
-	struct workqueue_struct *scan_bus_wq;
 	struct work_struct scan_bus_work;
 };
 
-- 
2.34.1