Re: [RFC PATCH v2 11/28] cxl/acpi: Rescan bus at probe completion

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

On Fri, Oct 22, 2021 at 11:37 AM Ben Widawsky <ben.widawsky@intel.com> wrote:
>
> Ensure that devices being probed before cxl_acpi has completed will get
> a second chance.

I think this is at the wrong level... more below.
>
> CXL drivers are brought up through two enumerable, asynchronous
> mechanism. The leaf nodes in the CXL topology, endpoints, are enumerated
> via PCI headers. The root node's enumeration is platform specific. The
> current defacto mechanism for enumerating the root node is through the
> presence of an ACPI device, ACPI0017.
>
> The primary job of a cxl_mem driver is to determine if CXL.mem traffic
> can be routed to/from the PCIe device that it is being probed. A
> prerequisite in this determination is that all CXL components in the
> path from root to leaf are capable of routing CXL.mem traffic. If the
> cxl_mem driver is probed before cxl_acpi is complete the driver will be
> unable to make this determination. To address this, cxl_acpi (or in the
> future, another platform specific driver) will rescan all devices to
> make sure the ordering is correct.
>
> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> ---
>  drivers/cxl/acpi.c | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index 625c5d95b83f..1cc3a74c16bd 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -451,6 +451,14 @@ static u32 cedt_instance(struct platform_device *pdev)
>         return U32_MAX;
>  }
>
> +static void bus_rescan(struct work_struct *work)
> +{
> +       if (bus_rescan_devices(&cxl_bus_type))
> +               pr_err("Failed to rescan CXL bus\n");
> +}
> +
> +static DECLARE_WORK(deferred_bus_rescan, bus_rescan);
> +
>  static int cxl_acpi_probe(struct platform_device *pdev)
>  {
>         int rc;
> @@ -484,9 +492,19 @@ static int cxl_acpi_probe(struct platform_device *pdev)
>         if (rc)
>                 goto out;
>
> -       if (IS_ENABLED(CONFIG_CXL_PMEM))
> +       if (IS_ENABLED(CONFIG_CXL_PMEM)) {
>                 rc = device_for_each_child(&root_port->dev, root_port,
>                                            add_root_nvdimm_bridge);
> +               if (rc)
> +                       goto out;
> +       }
> +
> +       /*
> +        * While ACPI is scanning hostbridge ports, switches and memory devices
> +        * may have been probed. Those devices will need to know whether the
> +        * hostbridge is CXL capable.
> +        */
> +       schedule_work(&deferred_bus_rescan);

I think this belongs in port driver similar to the one in
drivers/cxl/pmem.c, because any port online event might mean that a
downstream port can now attach to the port driver. I prefer a local
ordered workqueue via queue_work() rather than the global unordered
workqueue primarily because it can be flushed without getting
entangled with other random work in the system, but also because the
ordered property is useful for comprehended hierarchical topology
events.

In any event this is missing flushing to prevent races between .text
removal at module_exit() and pending work.