On Thu, Jan 19, 2023 at 04:17:11PM +0000, Jonathan Cameron wrote:
>
> Whilst I still have no idea if this is the same problem, I have identified
> what goes wrong if there is a module probe ordering issue.
> https://elixir.bootlin.com/linux/v6.2-rc4/source/drivers/cxl/core/pmem.c#L306
>
> /*
> * The two actions below arrange for @cxl_nvd to be deleted when either
> * the top-level PMEM bridge goes down, or the endpoint device goes
> * through ->remove().
> */
> device_lock(&cxl_nvb->dev);
> if (cxl_nvb->dev.driver)
> rc = devm_add_action_or_reset(&cxl_nvb->dev, cxl_nvd_unregister,
> cxl_nvd);
> else
> // bridge driver not loaded, so we hit this path.
> rc = -ENXIO;
> device_unlock(&cxl_nvb->dev);
>
> if (rc)
> /// and this one
> goto err_alloc;
>
> /* @cxlmd carries a reference on @cxl_nvb until cxlmd_release_nvdimm */
> return devm_add_action_or_reset(&cxlmd->dev, cxlmd_release_nvdimm,
> cxlmd);
>
> err:
> put_device(dev);
> err_alloc:
> cxlmd->cxl_nvb = NULL;
> cxlmd->cxl_nvd = NULL;
> put_device(&cxl_nvb->dev);
> // whilst we scrub the pointers we don't actually get rid of the
> // cxl_nvd that we registered. Hence later load of the driver tries to
> // attach to that and boom because we've scrubbed these pointers here.
> // A quick hack is to just call device_del(&cxl_nvd->dev) if rc = -ENXIO here.
> // There may well be a races though....
> return rc;
> }
> EXPORT_SYMBOL_NS_GPL(devm_cxl_add_nvdimm, CXL);
>
>
> Of course this "fix" just stops things blowing up, it doesn't leave things
> in a remotely useful state. If it's triggered because someone
> is messing with the load order that's fine. If the same issue
> is occurring for Gregory, not so much.
>
> Jonathan
>
mild hint in the dev_cxl_add_nvdimm_bridge path
driver/cxl/acpi.c
static int cxl_acpi_probe(struct platform_device *pdev)
{
... snip ...
if (IS_ENABLED(CONFIG_CXL_PMEM))
rc = device_for_each_child(&root_port->dev, root_port,
add_root_nvdimm_bridge);
if (rc < 0)
return rc;
/* In case PCI is scanned before ACPI re-trigger memdev attach */
cxl_bus_rescan();
return 0;
}
if PCI is presently written in a way that it's expecting nvdimm_bridge
to be present (via acpi_probe), then clearly this would break.
>From the other discussion here... that seems to be the issue? If that's
an issue, I also imagine there are other parts that may be subject to
the same problem.
static int cxl_pmem_region_probe(struct device *dev)
{
struct nd_mapping_desc mappings[CXL_DECODER_MAX_INTERLEAVE];
struct cxl_pmem_region *cxlr_pmem = to_cxl_pmem_region(dev);
struct cxl_region *cxlr = cxlr_pmem->cxlr;
struct cxl_nvdimm_bridge *cxl_nvb = cxlr->cxl_nvb;
this may be unreachable due to prior stack traces, but you get the
point.
Reiterating my confusion a bit: I don't have an nvdimm, why am i getting
an nvdimm_bridge? The reason it no longer appears to trigger on my
memexp example is because it doesnt go down this path:
static int cxl_mem_probe(struct device *dev)
{
... snip ...
// resource size is 0 here due to type3dev->persistent_capacity=0
if (resource_size(&cxlds->pmem_res) && IS_ENABLED(CONFIG_CXL_PMEM)) {
rc = devm_cxl_add_nvdimm(cxlmd);
if (rc == -ENODEV)
dev_info(dev, "PMEM disabled by platform\n");
else
return rc;
}
... snip ...
}
This seems like more than an ordering issue.
