Re: [PATCH v6 11/12] tools/testing/cxl: Add an RCH topology

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

Jonathan Cameron wrote:
> On Thu, 01 Dec 2022 13:34:21 -0800
> Dan Williams <dan.j.williams@intel.com> wrote:
> 
> > In an RCH topology a CXL host-bridge as Root Complex Integrated Endpoint
> > the represents the memory expander. Unlike a VH topology there is no
> > CXL/PCIE Root Port that host the endpoint. The CXL subsystem maps this
> > as the CXL root object (ACPI0017 on ACPI based systems) targeting the
> > host-bridge as a dport, per usual, but then that dport directly hosts
> > the endpoint port.
> > 
> > Mock up that configuration with a 4th host-bridge that has a 'cxl_rcd'
> > device instance as its immediate child.
> > 
> > Reviewed-by: Alison Schofield <alison.schofield@intel.com>
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> 
> A few trivial things inline.  
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> 
> 
> > -static struct pci_bus mock_pci_bus[NR_CXL_HOST_BRIDGES + NR_CXL_SINGLE_HOST];
> > +static struct pci_bus mock_pci_bus[NR_BRIDGES];
> >  static struct acpi_pci_root mock_pci_root[ARRAY_SIZE(mock_pci_bus)] = {
> >  	[0] = {
> >  		.bus = &mock_pci_bus[0],
> > @@ -452,7 +493,9 @@ static struct acpi_pci_root mock_pci_root[ARRAY_SIZE(mock_pci_bus)] = {
> >  	[2] = {
> >  		.bus = &mock_pci_bus[2],
> >  	},
> > -
> 
> I guess fixing this stray space here is fine to avoid a rebase to tidy it up
> in original patch which you have on your next branch.

In fact it's already upstream in v6.1-rc4 unfortunately, but it has no
business being in this patch.

So the hunk is now:

@@ -452,6 +493,9 @@ static struct acpi_pci_root mock_pci_root[ARRAY_SIZE(mock_pci_bus)] = {
        [2] = {
                .bus = &mock_pci_bus[2],
        },
+       [3] = {
+               .bus = &mock_pci_bus[3],
+       },
 
 };
 
> 
> > +	[3] = {
> > +		.bus = &mock_pci_bus[3],
> > +	},
> >  };
> >  
> >  static bool is_mock_bus(struct pci_bus *bus)
> > @@ -738,6 +781,87 @@ static void mock_companion(struct acpi_device *adev, struct device *dev)
> >  #define SZ_512G (SZ_64G * 8)
> >  #endif
> >  
> > +static __init int cxl_rch_init(void)
> > +{
> > +	int rc, i;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(cxl_rch); i++) {
> > +		int idx = NR_CXL_HOST_BRIDGES + NR_CXL_SINGLE_HOST + i;
> > +		struct acpi_device *adev = &host_bridge[idx];
> > +		struct platform_device *pdev;
> > +
> > +		pdev = platform_device_alloc("cxl_host_bridge", idx);
> > +		if (!pdev)
> > +			goto err_bridge;
> > +
> > +		mock_companion(adev, &pdev->dev);
> > +		rc = platform_device_add(pdev);
> > +		if (rc) {
> > +			platform_device_put(pdev);
> > +			goto err_bridge;
> > +		}
> > +
> > +		cxl_rch[i] = pdev;
> 
> Reason for this suggestion is below.
> Move down cxl_rch[i] = pdev;...
> 
> > +		mock_pci_bus[idx].bridge = &pdev->dev;
> > +		rc = sysfs_create_link(&pdev->dev.kobj, &pdev->dev.kobj,
> > +				       "firmware_node");
> > +		if (rc)
> > +			goto err_bridge;
> 
> to here, and clean up this single loop iteration by having a 
> platform_device_unregister in the error path above.

Ok, makes sense.

> 
> > +	}
> > +
> > +	for (i = 0; i < ARRAY_SIZE(cxl_rcd); i++) {
> > +		int idx = NR_MEM_MULTI + NR_MEM_SINGLE + i;
> > +		struct platform_device *rch = cxl_rch[i];
> > +		struct platform_device *pdev;
> > +
> > +		pdev = platform_device_alloc("cxl_rcd", idx);
> > +		if (!pdev)
> > +			goto err_mem;
> > +		pdev->dev.parent = &rch->dev;
> > +		set_dev_node(&pdev->dev, i % 2);
> > +
> > +		rc = platform_device_add(pdev);
> > +		if (rc) {
> > +			platform_device_put(pdev);
> > +			goto err_mem;
> > +		}
> > +		cxl_rcd[i] = pdev;
> > +	}
> > +
> > +	return 0;
> > +
> > +err_mem:
> > +	for (i = ARRAY_SIZE(cxl_rcd) - 1; i >= 0; i--)
> > +		platform_device_unregister(cxl_rcd[i]);
> > +err_bridge:
> > +	for (i = ARRAY_SIZE(cxl_rch) - 1; i >= 0; i--) {
> > +		struct platform_device *pdev = cxl_rch[i];
> > +
> > +		if (!pdev)
> > +			continue;
> > +		sysfs_remove_link(&pdev->dev.kobj, "firmware_node");
> 
> Had to look up that this was safe if the file doesn't exist (it is)
> I'd rather not have to check, so maybe make the sysfs path
> above clean up the device in the loop iteration and only set
> cxl_rch[i] once the loop iteration can't fail?  See above.
> 
> To my mind doing it that way is more 'obviously correct'
> which is never a bad thing.

So this also dovetails with Robert's feedback to move this error exit
block to just call cxl_rch_exit() [1]. In turn both of these feedbacks
are applicable to the other setup loops in this file. So I am thinking
this calls for a follow-on patch to cleanup all the instances of these 2
patterns in this file.

[1]: https://lore.kernel.org/all/Y4ZwXNpwt83puF4W@rric.localdomain/