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

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

On Thu, 6 Nov 2025 13:18:41 +0800
Xu Yilun <yilun.xu@linux.intel.com> wrote:

> 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;
> }
Nice. That looks good to me.

J