| Message ID | 20211022183709.1199701-12-ben.widawsky@intel.com |
|---|---|
| State | New, archived |
| Headers | show |
| Series | CXL Region Creation / HDM decoder programming | expand |
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.
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. > > In any event this is missing flushing to prevent races between .text > removal at module_exit() and pending work.
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? > > > > In any event this is missing flushing to prevent races between .text > > removal at module_exit() and pending work. >
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?
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); out: acpi_put_table(acpi_cedt);
Ensure that devices being probed before cxl_acpi has completed will get a second chance. 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(-)