Re: [PATCH 11/11] samples/devsec: Add sample IDE establishment

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

Jonathan Cameron wrote:
> On Thu, 05 Dec 2024 14:24:19 -0800
> Dan Williams <dan.j.williams@intel.com> wrote:
> 
> > Exercise common setup and teardown flows for a sample platform TSM
> > driver that implements the TSM 'connect' and 'disconnect' flows.
> > 
> > This is both a template for platform specific implementations and a test
> > case for the shared infrastructure.
> > 
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: Lukas Wunner <lukas@wunner.de>
> > Cc: Samuel Ortiz <sameo@rivosinc.com>
> > Cc: Alexey Kardashevskiy <aik@amd.com>
> > Cc: Xu Yilun <yilun.xu@linux.intel.com>
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> Trivial comments inline.
> 
> >  static int devsec_tsm_connect(struct pci_dev *pdev)
> >  {
> > -	return -ENXIO;
> > +	struct pci_ide *ide;
> > +	int rc, stream_id;
> > +
> > +	stream_id =
> > +		find_first_zero_bit(devsec_stream_ids, DEVSEC_NR_IDE_STREAMS);
> > +	if (stream_id == DEVSEC_NR_IDE_STREAMS)
> > +		return -EBUSY;
> > +	set_bit(stream_id, devsec_stream_ids);
> > +	ide = &devsec_streams[stream_id].ide;
> > +	pci_ide_stream_probe(pdev, ide);
> > +
> > +	ide->stream_id = stream_id;
> > +	rc = pci_ide_stream_setup(pdev, ide, PCI_IDE_SETUP_ROOT_PORT);
> > +	if (rc)
> > +		return rc;
> > +	rc = tsm_register_ide_stream(pdev, ide);
> > +	if (rc)
> > +		goto err;
> > +
> > +	devsec_streams[stream_id].pdev = pdev;
> > +	pci_ide_enable_stream(pdev, ide);
> > +	return 0;
> > +err:
> > +	pci_ide_stream_teardown(pdev, ide, PCI_IDE_SETUP_ROOT_PORT);
> 
> I'd kind of expect to see more of what we have in disconnect here.
> Like clearing the bit.

Yeah, that was a leak, now fixed and symmetric.

> > +	return rc;
> >  }
> >  
> >  static void devsec_tsm_disconnect(struct pci_dev *pdev)
> >  {
> > +	struct pci_ide *ide;
> > +	int i;
> > +
> > +	for_each_set_bit(i, devsec_stream_ids, DEVSEC_NR_IDE_STREAMS)
> > +		if (devsec_streams[i].pdev == pdev)
> > +			break;
> > +
> > +	if (i >= DEVSEC_NR_IDE_STREAMS)
> > +		return;
> > +
> > +	ide = &devsec_streams[i].ide;
> > +	pci_ide_disable_stream(pdev, ide);
> > +	tsm_unregister_ide_stream(ide);
> > +	pci_ide_stream_teardown(pdev, ide, PCI_IDE_SETUP_ROOT_PORT);
> > +	devsec_streams[i].pdev = NULL;
> If this setting to NULL needs to be out of order wrt to what happens
> in probe, add a comment.  If not move it to after pci_ide_disable_steram()

Also fixed with the same symmetry as the connect unwind case.