| Message ID | 167124081278.1626103.4792472728150764118.stgit@dwillia2-xfh.jf.intel.com |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cxl: Misc fixups that missed v6.2 | expand |
On 16.12.22 17:33:32, Dan Williams wrote: > The cxl_mem driver attempts to establish, or revalidate, the cxl_port > hierarcy to attach a cxl_memdev to a CXL platform topology. There is a > natural race (on ACPI platforms) between when the cxl_mem driver > attempts to attach and when the cxl_acpi driver establishes the root of > the topology. > > If cxl_mem_probe() runs first it will iterate to the top of the device > topology without finding the CXL platform root. That situation is benign > / expected, so stop warning about it. The cxl_acpi driver will poke > cxl_mem_probe() to try again once the CXL platform root is established. > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > --- > drivers/cxl/core/port.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c > index 810e60cc331c..6296d2bc909a 100644 > --- a/drivers/cxl/core/port.c > +++ b/drivers/cxl/core/port.c > @@ -1400,8 +1400,8 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd) > > uport_dev = dport_dev->parent; > if (!uport_dev) { > - dev_warn(dev, "at %s no parent for dport: %s\n", > - dev_name(iter), dev_name(dport_dev)); > + dev_dbg(dev, "at %s no parent for dport: %s\n", > + dev_name(iter), dev_name(dport_dev)); > return -ENXIO; Maybe we should also change the return code to the common -EAGAIN for this case here too? It looks like it is just passed to cxl_mem_probe(), so there are probably no side effects of this change. The probe is triggered then again by the base driver. -Robert
Robert Richter wrote: > On 16.12.22 17:33:32, Dan Williams wrote: > > The cxl_mem driver attempts to establish, or revalidate, the cxl_port > > hierarcy to attach a cxl_memdev to a CXL platform topology. There is a > > natural race (on ACPI platforms) between when the cxl_mem driver > > attempts to attach and when the cxl_acpi driver establishes the root of > > the topology. > > > > If cxl_mem_probe() runs first it will iterate to the top of the device > > topology without finding the CXL platform root. That situation is benign > > / expected, so stop warning about it. The cxl_acpi driver will poke > > cxl_mem_probe() to try again once the CXL platform root is established. > > > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > > --- > > drivers/cxl/core/port.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c > > index 810e60cc331c..6296d2bc909a 100644 > > --- a/drivers/cxl/core/port.c > > +++ b/drivers/cxl/core/port.c > > @@ -1400,8 +1400,8 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd) > > > > uport_dev = dport_dev->parent; > > if (!uport_dev) { > > - dev_warn(dev, "at %s no parent for dport: %s\n", > > - dev_name(iter), dev_name(dport_dev)); > > + dev_dbg(dev, "at %s no parent for dport: %s\n", > > + dev_name(iter), dev_name(dport_dev)); > > return -ENXIO; > > Maybe we should also change the return code to the common -EAGAIN for > this case here too? It looks like it is just passed to > cxl_mem_probe(), so there are probably no side effects of this change. > The probe is triggered then again by the base driver. Good point, might as well explicitly return EPROBE_DEFER rather than let the driver core turn EAGAIN into EPROBE_DEFER. Tests seem to pass with that change as well.
On 03.01.23 13:07:18, Dan Williams wrote: > Robert Richter wrote: > > On 16.12.22 17:33:32, Dan Williams wrote: > > > diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c > > > index 810e60cc331c..6296d2bc909a 100644 > > > --- a/drivers/cxl/core/port.c > > > +++ b/drivers/cxl/core/port.c > > > @@ -1400,8 +1400,8 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd) > > > > > > uport_dev = dport_dev->parent; > > > if (!uport_dev) { > > > - dev_warn(dev, "at %s no parent for dport: %s\n", > > > - dev_name(iter), dev_name(dport_dev)); > > > + dev_dbg(dev, "at %s no parent for dport: %s\n", > > > + dev_name(iter), dev_name(dport_dev)); > > > return -ENXIO; > > > > Maybe we should also change the return code to the common -EAGAIN for > > this case here too? It looks like it is just passed to > > cxl_mem_probe(), so there are probably no side effects of this change. > > The probe is triggered then again by the base driver. > > Good point, might as well explicitly return EPROBE_DEFER rather than let > the driver core turn EAGAIN into EPROBE_DEFER. Tests seem to pass with > that change as well. Yes, EPROBE_DEFER is the one used in the Deferred Probe infrastructure. Thanks, -Robert
On Wed, 4 Jan 2023 10:36:13 +0100 Robert Richter <rrichter@amd.com> wrote: > On 03.01.23 13:07:18, Dan Williams wrote: > > Robert Richter wrote: > > > On 16.12.22 17:33:32, Dan Williams wrote: > > > > > diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c > > > > index 810e60cc331c..6296d2bc909a 100644 > > > > --- a/drivers/cxl/core/port.c > > > > +++ b/drivers/cxl/core/port.c > > > > @@ -1400,8 +1400,8 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd) > > > > > > > > uport_dev = dport_dev->parent; > > > > if (!uport_dev) { > > > > - dev_warn(dev, "at %s no parent for dport: %s\n", > > > > - dev_name(iter), dev_name(dport_dev)); > > > > + dev_dbg(dev, "at %s no parent for dport: %s\n", > > > > + dev_name(iter), dev_name(dport_dev)); > > > > return -ENXIO; > > > > > > Maybe we should also change the return code to the common -EAGAIN for > > > this case here too? It looks like it is just passed to > > > cxl_mem_probe(), so there are probably no side effects of this change. > > > The probe is triggered then again by the base driver. > > > > Good point, might as well explicitly return EPROBE_DEFER rather than let > > the driver core turn EAGAIN into EPROBE_DEFER. Tests seem to pass with > > that change as well. > > Yes, EPROBE_DEFER is the one used in the Deferred Probe > infrastructure. If doing that, can we add a dev_err_probe() call so that the deferred probing infrastructure gets a nice error message for anyone wondering why this deferred. That calls the device_set_deferred_probe_reason() in the -EPROBE_DEFER call and deals with dev_dbg print for this case for us. > > Thanks, > > -Robert
Jonathan Cameron wrote: > On Wed, 4 Jan 2023 10:36:13 +0100 > Robert Richter <rrichter@amd.com> wrote: > > > On 03.01.23 13:07:18, Dan Williams wrote: > > > Robert Richter wrote: > > > > On 16.12.22 17:33:32, Dan Williams wrote: > > > > > > > diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c > > > > > index 810e60cc331c..6296d2bc909a 100644 > > > > > --- a/drivers/cxl/core/port.c > > > > > +++ b/drivers/cxl/core/port.c > > > > > @@ -1400,8 +1400,8 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd) > > > > > > > > > > uport_dev = dport_dev->parent; > > > > > if (!uport_dev) { > > > > > - dev_warn(dev, "at %s no parent for dport: %s\n", > > > > > - dev_name(iter), dev_name(dport_dev)); > > > > > + dev_dbg(dev, "at %s no parent for dport: %s\n", > > > > > + dev_name(iter), dev_name(dport_dev)); > > > > > return -ENXIO; > > > > > > > > Maybe we should also change the return code to the common -EAGAIN for > > > > this case here too? It looks like it is just passed to > > > > cxl_mem_probe(), so there are probably no side effects of this change. > > > > The probe is triggered then again by the base driver. > > > > > > Good point, might as well explicitly return EPROBE_DEFER rather than let > > > the driver core turn EAGAIN into EPROBE_DEFER. Tests seem to pass with > > > that change as well. > > > > Yes, EPROBE_DEFER is the one used in the Deferred Probe > > infrastructure. > > If doing that, can we add a dev_err_probe() call so that the deferred probing > infrastructure gets a nice error message for anyone wondering why this > deferred. That calls the device_set_deferred_probe_reason() in the > -EPROBE_DEFER call and deals with dev_dbg print for this case for us. Ah, sure that's better.
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c index 810e60cc331c..6296d2bc909a 100644 --- a/drivers/cxl/core/port.c +++ b/drivers/cxl/core/port.c @@ -1400,8 +1400,8 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd) uport_dev = dport_dev->parent; if (!uport_dev) { - dev_warn(dev, "at %s no parent for dport: %s\n", - dev_name(iter), dev_name(dport_dev)); + dev_dbg(dev, "at %s no parent for dport: %s\n", + dev_name(iter), dev_name(dport_dev)); return -ENXIO; }
The cxl_mem driver attempts to establish, or revalidate, the cxl_port hierarcy to attach a cxl_memdev to a CXL platform topology. There is a natural race (on ACPI platforms) between when the cxl_mem driver attempts to attach and when the cxl_acpi driver establishes the root of the topology. If cxl_mem_probe() runs first it will iterate to the top of the device topology without finding the CXL platform root. That situation is benign / expected, so stop warning about it. The cxl_acpi driver will poke cxl_mem_probe() to try again once the CXL platform root is established. Signed-off-by: Dan Williams <dan.j.williams@intel.com> --- drivers/cxl/core/port.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)