Re: cxl nvdimm Potential probe ordering issues.

[Gregory Price]

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.