Robert Richter wrote:
> On 24.11.22 10:35:38, Dan Williams 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.
> >
> > Signed-off-by: Dan Williams <dan.j.willi...@intel.com>
> > ---
> > tools/testing/cxl/test/cxl.c | 151
> > +++++++++++++++++++++++++++++++++++++++---
> > tools/testing/cxl/test/mem.c | 37 ++++++++++
> > 2 files changed, 176 insertions(+), 12 deletions(-)
>
> One comment below.
>
> > @@ -736,6 +779,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;
> > + mock_pci_bus[idx].bridge = &pdev->dev;
> > + rc = sysfs_create_link(&pdev->dev.kobj, &pdev->dev.kobj,
> > + "firmware_node");
> > + if (rc)
> > + goto err_bridge;
> > + }
> > +
> > + 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:
>
> platform_device_unregister() is safe to be used with NULL, so we can
> have a single entry of this unregister code ...
>
> > + 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");
> > + platform_device_unregister(cxl_rch[i]);
> > + }
> > +
> > + return rc;
> > +}
> > +
> > +static void cxl_rch_exit(void)
> > +{
> > + int i;
> > +
> > + for (i = ARRAY_SIZE(cxl_rcd) - 1; i >= 0; i--)
> > + platform_device_unregister(cxl_rcd[i]);
> > + 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");
> > + platform_device_unregister(cxl_rch[i]);
> > + }
> > +}
>
> ... and have a single function for both. This reduces code
> duplication here.
>
That's true. This was cargo culted from the other parts of cxl_test, but
those can be cleaned up too. I will do this with a follow-on patch and
clean up both.
