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

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

On Mon, Nov 1, 2021 at 2:45 PM Ben Widawsky <ben.widawsky@intel.com> wrote:
>
> On 21-11-01 11:56:02, Ben Widawsky wrote:
> > On 21-10-31 12:25:32, Dan Williams wrote:
> > > 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.
> >
> > Makes sense. I was trying to limit the number of rescans that took place and I
> > believe I convinced myself that the current patch does work properly. However,
> > architecturally it makes more sense for the port driver to own this and
> > realistically I assume many rescans will coalesce.
> >
>
> Oh. I assume you want this rolled into (cxl/port: Introduce a port driver),
> correct?

Yeah, otherwise the port driver is broken, right?