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

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

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.

> +	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()

> +	clear_bit(i, devsec_stream_ids);
>  }
>  
>  static const struct pci_tsm_ops devsec_pci_ops = {
> @@ -63,16 +113,44 @@ static void devsec_tsm_remove(void *tsm)
>  	tsm_unregister(tsm);
>  }
>  
> +static void set_nr_ide_streams(int nr)
> +{
> +	struct pci_dev *pdev = NULL;
> +
> +	for_each_pci_dev(pdev) {
> +		struct pci_host_bridge *hb;
> +
> +		if (pdev->sysdata != devsec_sysdata)
> +			continue;
> +		hb = pci_find_host_bridge(pdev->bus);
> +		if (hb->nr_ide_streams >= 0)
> +			continue;
> +		pci_set_nr_ide_streams(hb, nr);
> +	}
> +}
> +
> +static void devsec_tsm_ide_teardown(void *data)
> +{
> +	set_nr_ide_streams(-1);
> +}
> +
>  static int devsec_tsm_probe(struct platform_device *pdev)
>  {
>  	struct tsm_subsys *tsm;
> +	int rc;
>  
>  	tsm = tsm_register(&pdev->dev, NULL, &devsec_pci_ops);
>  	if (IS_ERR(tsm))
>  		return PTR_ERR(tsm);
>  
> -	return devm_add_action_or_reset(&pdev->dev, devsec_tsm_remove,
> -					tsm);
> +	rc = devm_add_action_or_reset(&pdev->dev, devsec_tsm_remove, tsm);
> +	if (rc)
> +		return rc;
> +
> +	set_nr_ide_streams(DEVSEC_NR_IDE_STREAMS);
> +
> +	return devm_add_action_or_reset(&pdev->dev, devsec_tsm_ide_teardown,
> +					NULL);
>  }
>  
>  static struct platform_driver devsec_tsm_driver = {
> @@ -109,5 +187,6 @@ static void __exit devsec_tsm_exit(void)
>  }
>  module_exit(devsec_tsm_exit);
>  
> +MODULE_IMPORT_NS(PCI_IDE);
>  MODULE_LICENSE("GPL");
>  MODULE_DESCRIPTION("Device Security Sample Infrastructure: Platform TSM Driver");
> 
>