Re: [RFC PATCH 20/27] coco/tdx-host: Add connect()/disconnect() handlers prototype

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

On Fri, 19 Sep 2025 07:22:29 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> From: Xu Yilun <yilun.xu@linux.intel.com>
> 
> Add basic skeleton for connect()/disconnect() handlers. The major steps
> are SPDM setup first and then IDE selective stream setup.
> 
> No detailed TDX Connect implementation.
> 
> Signed-off-by: Xu Yilun <yilun.xu@linux.intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Feels like use of __free() in here is inappropriate to me.

> ---
>  drivers/virt/coco/tdx-host/tdx-host.c | 49 ++++++++++++++++++++++++++-
>  1 file changed, 48 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/virt/coco/tdx-host/tdx-host.c b/drivers/virt/coco/tdx-host/tdx-host.c
> index f5a869443b15..0d052a1acf62 100644
> --- a/drivers/virt/coco/tdx-host/tdx-host.c
> +++ b/drivers/virt/coco/tdx-host/tdx-host.c
> @@ -104,13 +104,60 @@ static int __maybe_unused tdx_spdm_msg_exchange(struct tdx_link *tlink,
>  	return ret;

> +
> +static void __tdx_link_disconnect(struct tdx_link *tlink)
> +{
> +	tdx_ide_stream_teardown(tlink);
> +	tdx_spdm_session_teardown(tlink);
> +}
> +
> +DEFINE_FREE(__tdx_link_disconnect, struct tdx_link *, if (_T) __tdx_link_disconnect(_T))
> +
>  static int tdx_link_connect(struct pci_dev *pdev)
>  {
> -	return -ENXIO;
> +	struct tdx_link *tlink = to_tdx_link(pdev->tsm);
> +	int ret;
> +
> +	struct tdx_link *__tlink __free(__tdx_link_disconnect) = tlink;
I'm not a fan on an ownership pass like this just for purposes of cleaning up.

I'd be a bit happier if you could make it
	struct tdx_link *tlink __free(__tdx_link_disconnect) = to_tdx_link(pdev->dsm);

but I still don't really like it.  I think I'd just not use __free and stick
to traditional cleanup in via a goto. 

> +	ret = tdx_spdm_session_setup(tlink);
> +	if (ret) {
> +		pci_err(pdev, "fail to setup spdm session\n");
> +		return ret;
> +	}
> +
> +	ret = tdx_ide_stream_setup(tlink);
> +	if (ret) {
> +		pci_err(pdev, "fail to setup ide stream\n");

If this fails we still call tdx_ide_stream_teardown().  Why does that make sense
given I'd expect this to have no side effects on failure?  Definitely needs
a comment. I also definitely don't think __free is appropriate here as it
is hiding this detail somewhat

> +		return ret;
> +	}
> +
> +	tlink = no_free_ptr(__tlink);
> +
> +	return 0;
>  }
>  
>  static void tdx_link_disconnect(struct pci_dev *pdev)
>  {
> +	struct tdx_link *tlink = to_tdx_link(pdev->tsm);
> +
> +	__tdx_link_disconnect(tlink);
>  }
>  
>  static struct pci_tsm_ops tdx_link_ops;