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

[Xu Yilun <yilun.xu@linux.intel.com>]

On Mon, Nov 03, 2025 at 03:34:15PM -0800, dan.j.williams@intel.com wrote:
> Jonathan Cameron wrote:
> > 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.
> 
> Yeah this needs a rethink. The session and the stream are independent
> resources. It can be a composite object that encapsulates both
> resources, but not tlink directly.
> 
> ...chalk this up to RFC expediency.
> 
> > 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. 
> 
> I would not go that far, but certainly I can see that being preferable
> than reusing the existing base 'struct tdx_link *' as the cleanup
> variable.

The latest implementation internally is as follows. tlink_spdm &
tlink_ide represent independent resources though they point to the same
instance. I'm already comfortable about this code:

static int tdx_link_connect(struct pci_dev *pdev)
{
	struct tdx_link *tlink = to_tdx_link(pdev->tsm);

	struct tdx_link *tlink_spdm __free(tdx_spdm_session_teardown) =
		tdx_spdm_session_setup(tlink);
	if (IS_ERR(tlink_spdm)) {
		pci_err(pdev, "fail to setup spdm session\n");
		return PTR_ERR(tlink_spdm);
	}

	struct tdx_link *tlink_ide __free(tdx_ide_stream_teardown) =
		tdx_ide_stream_setup(tlink);
	if (IS_ERR(tlink_ide)) {
		pci_err(pdev, "fail to setup ide stream\n");
		return PTR_ERR(tlink_ide);
	}

	retain_and_null_ptr(tlink_spdm);
	retain_and_null_ptr(tlink_ide);

	return 0;
}