Jonathan Cameron wrote:
> On Thu, 01 Dec 2022 13:34:21 -0800
> Dan Williams <[email protected]> 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 <[email protected]>
> > Signed-off-by: Dan Williams <[email protected]>
>
> A few trivial things inline.
>
> Reviewed-by: Jonathan Cameron <[email protected]>
>
>
>
> > -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/[email protected]/
