Re: [PATCH v7 1/9] coco/tsm: Introduce a core device for TEE Security Managers

[Jonathan Cameron <jonathan.cameron@huawei.com>]

On Thu, 23 Oct 2025 19:04:10 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> A "TSM" is a platform component that provides an API for securely
> provisioning resources for a confidential guest (TVM) to consume. The
> name originates from the PCI specification for platform agent that
> carries out operations for PCIe TDISP (TEE Device Interface Security
> Protocol).
> 
> Instances of this core device are parented by a device representing the
> platform security function like CONFIG_CRYPTO_DEV_CCP or
> CONFIG_INTEL_TDX_HOST.
> 
> This device interface is a frontend to the aspects of a TSM and TEE I/O
> that are cross-architecture common. This includes mechanisms like
> enumerating available platform TEE I/O capabilities and provisioning
> connections between the platform TSM and device DSMs (Device Security
> Manager (TDISP)).
> 
> For now this is just the scaffolding for registering a TSM device sysfs
> interface.
> 
> Cc: Alexey Kardashevskiy <aik@amd.com>
> Cc: Xu Yilun <yilun.xu@linux.intel.com>
> Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
> Co-developed-by: Aneesh Kumar K.V (Arm) <aneesh.kumar@kernel.org>
> Signed-off-by: Aneesh Kumar K.V (Arm) <aneesh.kumar@kernel.org>
> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Dan,

My usual problem of having forgotten all the details since I last
looked applies, so I'll take another look at the lot.

One trivial comment below.

> diff --git a/drivers/virt/coco/tsm-core.c b/drivers/virt/coco/tsm-core.c
> new file mode 100644
> index 000000000000..a64b776642cf
> --- /dev/null
> +++ b/drivers/virt/coco/tsm-core.c
> @@ -0,0 +1,109 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Copyright(c) 2024 Intel Corporation. All rights reserved. */
Maybe worth updating as in general this has evolved a bit this year
I think.

> +
> +static void put_tsm_dev(struct tsm_dev *tsm_dev)
> +{
> +	if (!IS_ERR_OR_NULL(tsm_dev))
> +		put_device(&tsm_dev->dev);
> +}
> +
> +DEFINE_FREE(put_tsm_dev, struct tsm_dev *,
> +	    if (!IS_ERR_OR_NULL(_T)) put_tsm_dev(_T))

I'm entirely on board with the normal argument behind the !IS_ERR_OR_NULL()
check and the fact it lets the compiler remove an indirect call in some
cases.  However, here you have the protection here and in put_tsm_dev()
that is only called via this path.  That seems excessive.