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

[<dan.j.williams@intel.com>]

Jonathan Cameron wrote:
> 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.

Too late, you already added a review tag. </joking>

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

Sure.

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

I think if there were open coded callers of put_tsm_dev() I would keep
the excessive form, but since this only for __free() no need for the
explicit helper.

range-diff result:

 1:  c2ad31ce3803 !  1:  448addc31b86 coco/tsm: Introduce a core device for TEE Security Managers
    @@ include/linux/tsm.h: struct tsm_report_ops {
      ## drivers/virt/coco/tsm-core.c (new) ##
     @@
     +// SPDX-License-Identifier: GPL-2.0-only
    -+/* Copyright(c) 2024 Intel Corporation. All rights reserved. */
    ++/* Copyright(c) 2024-2025 Intel Corporation. All rights reserved. */
     +
     +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
     +
    @@ drivers/virt/coco/tsm-core.c (new)
     +  return no_free_ptr(tsm_dev);
     +}
     +
    -+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))
    ++      if (!IS_ERR_OR_NULL(_T)) put_device(&_T->dev))
     +
     +struct tsm_dev *tsm_register(struct device *parent)
     +{