Message ID | 20231030155901.80673-2-sumit.garg@linaro.org |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:d641:0:b0:403:3b70:6f57 with SMTP id cy1csp2318870vqb; Mon, 30 Oct 2023 08:59:50 -0700 (PDT) X-Google-Smtp-Source: AGHT+IEQivLqFeDiMzCmGd3tFsCbwdTLqQT5IAAGBHU4+ApYP5KOn+erJCRltUevR80cJ6+TUu2K X-Received: by 2002:a05:6a20:918d:b0:17a:eddb:ac65 with SMTP id v13-20020a056a20918d00b0017aeddbac65mr9501350pzd.9.1698681590630; Mon, 30 Oct 2023 08:59:50 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1698681590; cv=none; d=google.com; s=arc-20160816; b=wtXLbOn0TPLCZd58FzOAQgHRRNABK5z/dbmHQNskkl9ntlNsvjfFMN6vqcw3cz6ZYO ENrUhzJ15K3T0TD32D1FzF+w17njwPeV6ArtD4Xd0baL4cTpC7xLmRUr32v9hNjIzwdp 8KH5Z6mtXOMsOhUBQMNwlhvFakmwv3ohZm+VsxVP7PgCnQq22e7xChuVviQlsigdlyCl GiZ3V7OA6gWOGPEqb+CqELfJHn6rwifDmBSCfRjFMdOMSPksWgEUwNAYf/+wEcPq33jZ 1CnsgH95i1Pg4ZkSMVX1fDYadb1QDpSjCEg6PCcYQFDgRnV6AxiRtbd75QrjXVXUZZK4 r1MQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=qxLGrTrVdS6twyTFJiULc4e3eYbN7qH6nv+rg3gSjBE=; fh=kek5XU/v1RmPJHz1V6icIjoJC+ErwsKZ1swtKUy8acg=; b=k6CiW0iQJrPsuDDwSk5AENqmdatkCuymis+jmqi2qWJOSDZXm/vzUGrzqKkkWrso62 r599pxGAtz38IFb5eyNbj1q6FCGrBhkMxJm+vZ2kuwBqPBhX8dm0q79IzFX78mY7d1Zi E6zkWFNJ1RRbBt7RckzVRk1151rqrSYxl31g4Qu7FMv2BGeP+xulceemkGU8EVnSEbMS lJnkEx/6YdU0KmvgtXEa7OxKWVlqtfsllhPpOyIjAsCK+bE8lzID4pWj3wDm9GVKzjyo Z4oDY78enoUwFoLmSxjggaTbbixwT0/UF3uoiCpUSfIvicCn91Cn2ii5BFfCeUg21HuT wElA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@linaro.org header.s=google header.b=jxP5rR+u; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linaro.org Received: from lipwig.vger.email (lipwig.vger.email. [23.128.96.33]) by mx.google.com with ESMTPS id bw34-20020a056a0204a200b005783367f97fsi5278777pgb.342.2023.10.30.08.59.50 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 30 Oct 2023 08:59:50 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) client-ip=23.128.96.33; Authentication-Results: mx.google.com; dkim=pass header.i=@linaro.org header.s=google header.b=jxP5rR+u; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linaro.org Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by lipwig.vger.email (Postfix) with ESMTP id 32DB9804BDEE; Mon, 30 Oct 2023 08:59:44 -0700 (PDT) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.10 at lipwig.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233791AbjJ3P72 (ORCPT <rfc822;chrisjones.unixmen@gmail.com> + 32 others); Mon, 30 Oct 2023 11:59:28 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:56444 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233789AbjJ3P7Z (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Mon, 30 Oct 2023 11:59:25 -0400 Received: from mail-pf1-x42d.google.com (mail-pf1-x42d.google.com [IPv6:2607:f8b0:4864:20::42d]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id B1847F9 for <linux-kernel@vger.kernel.org>; Mon, 30 Oct 2023 08:59:22 -0700 (PDT) Received: by mail-pf1-x42d.google.com with SMTP id d2e1a72fcca58-6c10f098a27so840582b3a.2 for <linux-kernel@vger.kernel.org>; Mon, 30 Oct 2023 08:59:22 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1698681562; x=1699286362; darn=vger.kernel.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=qxLGrTrVdS6twyTFJiULc4e3eYbN7qH6nv+rg3gSjBE=; b=jxP5rR+u79T4jWxvyDDK0ThdlJFd7ALytj+uOuH9L71bKUeS2IAsUERVTVewozUgmu LLeBgtl88vkfn/sPIygpGq+MdX7cT6s0ejt2wdbw2LiqETZE6gpSuBS9D6ljlE/HheXl FBLEceTgUz+aWkU3V/12r39Zv29YYVYur5f27arf/6VtunjIKlJoN1nE2/KEVNlzJyhE plFUeRze+p2OMg7GCGkJ60W9cekFYXU8XJ1IvMVp3JrlyzU/fKNc5Mc2AB/CFUvahbxu ozLSXeYaLJ1xDyJiEKYL5rsqHYui3jwFWhj8uRGtdF1hrKMNNRS9Z3VQvHv3tpT0oC/A eUIg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1698681562; x=1699286362; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=qxLGrTrVdS6twyTFJiULc4e3eYbN7qH6nv+rg3gSjBE=; b=BpqbKqR2i0IVth8NtQKDVWZjR4hz2Wh7dXvU0XcXB07tt9rLr7YCH3W2t2bcYtHUfU SRSojgzNiXF56G7kKjag7UT/p+qL75pdVE7K3S5lCHrnigU0SAP0+FSm40b/iSIoTkeQ ac/8H4kKx7UHtr4BjxKS+nw+tReM4N+uZnshizVYjq2ddKieSnxOFY8aoeAYJkH4Fxxg +IV0A/yJbjP6GhvRC3wdIskxODtZKtUFpk8Tw9bY/VbPp6LLrR3Zh6yB78/uuuqSlzey niU2thND1102uudoH1/ssZ8Xn/dWcNF9DHcYa33KZi4gWYp8XMdAgd9KnFvKuBH+Pqwi QODA== X-Gm-Message-State: AOJu0YxWWA8CQQb9gVQ/suJoSsZGiM4zp8iDBT9j16qcRaDdE2SYNs91 tiiEZyrqCNu9C5b+VwCl3iisuw== X-Received: by 2002:a05:6a00:24d6:b0:68e:2478:d6c9 with SMTP id d22-20020a056a0024d600b0068e2478d6c9mr8654507pfv.2.1698681562102; Mon, 30 Oct 2023 08:59:22 -0700 (PDT) Received: from sumit-X1.. ([223.178.210.245]) by smtp.gmail.com with ESMTPSA id z8-20020a056a00240800b006c148c6767bsm176632pfh.218.2023.10.30.08.59.17 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 30 Oct 2023 08:59:21 -0700 (PDT) From: Sumit Garg <sumit.garg@linaro.org> To: jens.wiklander@linaro.org, op-tee@lists.trustedfirmware.org Cc: jan.kiszka@siemens.com, arnd@linaro.org, ardb@kernel.org, jerome.forissier@linaro.org, ilias.apalodimas@linaro.org, masahisa.kojima@linaro.org, maxim.uvarov@linaro.org, jarkko.sakkinen@linux.intel.com, linux-kernel@vger.kernel.org, diogo.ivo@siemens.com, Sumit Garg <sumit.garg@linaro.org> Subject: [PATCH v3 1/2] tee: optee: Fix supplicant based device enumeration Date: Mon, 30 Oct 2023 21:29:00 +0530 Message-Id: <20231030155901.80673-2-sumit.garg@linaro.org> X-Mailer: git-send-email 2.34.1 In-Reply-To: <20231030155901.80673-1-sumit.garg@linaro.org> References: <20231030155901.80673-1-sumit.garg@linaro.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-0.8 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lipwig.vger.email Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (lipwig.vger.email [0.0.0.0]); Mon, 30 Oct 2023 08:59:44 -0700 (PDT) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1781196747691251949 X-GMAIL-MSGID: 1781196747691251949 |
Series |
tee: optee: Fixes for supplicant dependent enumeration
|
|
Commit Message
Sumit Garg
Oct. 30, 2023, 3:59 p.m. UTC
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(-)
Comments
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; > }
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; >> } >
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 >
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
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 >
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
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 >
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; }