[PATCH 0/3] cxl: Misc fixups that missed v6.2

[PATCH 0/3] cxl: Misc fixups that missed v6.2

[Dan Williams]

While working on volatile region enumeration in the v6.1 timeframe I put
together these small fixups and a new attribute to help topology
walking. I overlooked them for v6.2, but will now target them for v6.3.

The most significant change is a 'parent_dport' attribute so 'cxl list'
has a simple method to identify the downstream port that routes to the
descendant cxl_port (upstream port).

---

Dan Williams (3):
      cxl/mem: Quiet port walking warning
      cxl/region: Clarify when a cxld->commit() callback is mandatory
      cxl/port: Link the 'parent_dport' in portX/ and endpointX/ sysfs


 Documentation/ABI/testing/sysfs-bus-cxl |   15 +++++++++++++++
 drivers/cxl/core/port.c                 |   31 +++++++++++++++++++++++++++++--
 drivers/cxl/core/region.c               |   19 +++++++++++++++++--
 3 files changed, 61 insertions(+), 4 deletions(-)

base-commit: f04facfb993de47e2133b2b842d72b97b1c50162

[PATCH 1/3] cxl/mem: Quiet port walking warning

[Dan Williams]

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;
 		}
 

Re: [PATCH 1/3] cxl/mem: Quiet port walking warning

[Robert Richter]

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

Re: [PATCH 1/3] cxl/mem: Quiet port walking warning

[Dan Williams]

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.

Re: [PATCH 1/3] cxl/mem: Quiet port walking warning

[Robert Richter]

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

Re: [PATCH 1/3] cxl/mem: Quiet port walking warning

[Jonathan Cameron]

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

Re: [PATCH 1/3] cxl/mem: Quiet port walking warning

[Dan Williams]

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.

[PATCH v2 1/3] cxl/mem: Quiet port walking warnings

[Dan Williams]

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.

Suppress any upper level errors by making it clear that this is merely a
probe deferral event, not a hard error.

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
Changes since v1:
* Use dev_err_probe() (Jonathan)

 drivers/cxl/core/port.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index b631a0520456..feb8f84a9281 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -1397,9 +1397,10 @@ 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));
-			return -ENXIO;
+			dev_err_probe(dev, -EPROBE_DEFER,
+				      "at %s no parent for dport: %s\n",
+				      dev_name(iter), dev_name(dport_dev));
+			return -EPROBE_DEFER;
 		}
 
 		dev_dbg(dev, "scan: iter: %s dport_dev: %s parent: %s\n",

Re: [PATCH v2 1/3] cxl/mem: Quiet port walking warnings

[Jonathan Cameron]

On Wed, 25 Jan 2023 14:11:17 -0800
Dan Williams <dan.j.williams@intel.com> 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.
> 
> Suppress any upper level errors by making it clear that this is merely a
> probe deferral event, not a hard error.
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
> Changes since v1:
> * Use dev_err_probe() (Jonathan)
> 
>  drivers/cxl/core/port.c |    7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index b631a0520456..feb8f84a9281 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -1397,9 +1397,10 @@ 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));
> -			return -ENXIO;
> +			dev_err_probe(dev, -EPROBE_DEFER,
> +				      "at %s no parent for dport: %s\n",
> +				      dev_name(iter), dev_name(dport_dev));
> +			return -EPROBE_DEFER;
>  		}
>  
>  		dev_dbg(dev, "scan: iter: %s dport_dev: %s parent: %s\n",
> 

Re: [PATCH v2 1/3] cxl/mem: Quiet port walking warnings

[Robert Richter]

On 25.01.23 14:11:17, 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.
> 
> Suppress any upper level errors by making it clear that this is merely a
> probe deferral event, not a hard error.
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Reviewed-by: Robert Richter <rrichter@amd.com>

[PATCH 2/3] cxl/region: Clarify when a cxld->commit() callback is mandatory

[Dan Williams]

Both cxl_switch_decoders() and cxl_endpoint_decoders() are considered by
cxl_region_decode_commit(). Flag cases where cxl_switch_decoders with
multiple targets, or cxl_endpoint_decoders do not have a commit callback
set. The switch case is unlikely to happen since switches are only
enumerated by the CXL core, but the endpoint case may support decoders
defined by drivers outside of drivers/cxl, like accerator drivers.

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/core/region.c |   19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index c11a6ab5e48d..60828d01972a 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -156,6 +156,22 @@ static int cxl_region_decode_reset(struct cxl_region *cxlr, int count)
 	return 0;
 }
 
+static int commit_decoder(struct cxl_decoder *cxld)
+{
+	struct cxl_switch_decoder *cxlsd = NULL;
+
+	if (cxld->commit)
+		return cxld->commit(cxld);
+
+	if (is_switch_decoder(&cxld->dev))
+		cxlsd = to_cxl_switch_decoder(&cxld->dev);
+
+	if (dev_WARN_ONCE(&cxld->dev, !cxlsd || cxlsd->nr_targets > 1,
+			  "->commit() is required\n"))
+		return -ENXIO;
+	return 0;
+}
+
 static int cxl_region_decode_commit(struct cxl_region *cxlr)
 {
 	struct cxl_region_params *p = &cxlr->params;
@@ -174,8 +190,7 @@ static int cxl_region_decode_commit(struct cxl_region *cxlr)
 		     iter = to_cxl_port(iter->dev.parent)) {
 			cxl_rr = cxl_rr_load(iter, cxlr);
 			cxld = cxl_rr->decoder;
-			if (cxld->commit)
-				rc = cxld->commit(cxld);
+			rc = commit_decoder(cxld);
 			if (rc)
 				break;
 		}

Re: [PATCH 2/3] cxl/region: Clarify when a cxld->commit() callback is mandatory

[Jonathan Cameron]

On Fri, 16 Dec 2022 17:33:38 -0800
Dan Williams <dan.j.williams@intel.com> wrote:

> Both cxl_switch_decoders() and cxl_endpoint_decoders() are considered by
> cxl_region_decode_commit(). Flag cases where cxl_switch_decoders with
> multiple targets, or cxl_endpoint_decoders do not have a commit callback
> set. The switch case is unlikely to happen since switches are only
> enumerated by the CXL core, but the endpoint case may support decoders
> defined by drivers outside of drivers/cxl, like accerator drivers.
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Seems reasonable though (just for consistency with earlier discussions)
I don't like assumption that nr->targets is the right thing to decide on.
It's fine to have decoders in single target case and if we do
they should be committed / not registered as pass through decoders etc.

Hmm. I've never tested the single downstream CXL switch port case.
Similar to HB in that case HDM decoders are optional. I've no idea
what we do about that corner currently.  One to add to my tests ;)


Anyhow, this is fine
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>




> ---
>  drivers/cxl/core/region.c |   19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index c11a6ab5e48d..60828d01972a 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -156,6 +156,22 @@ static int cxl_region_decode_reset(struct cxl_region *cxlr, int count)
>  	return 0;
>  }
>  
> +static int commit_decoder(struct cxl_decoder *cxld)
> +{
> +	struct cxl_switch_decoder *cxlsd = NULL;
> +
> +	if (cxld->commit)
> +		return cxld->commit(cxld);
> +
> +	if (is_switch_decoder(&cxld->dev))
> +		cxlsd = to_cxl_switch_decoder(&cxld->dev);
> +
> +	if (dev_WARN_ONCE(&cxld->dev, !cxlsd || cxlsd->nr_targets > 1,
> +			  "->commit() is required\n"))
> +		return -ENXIO;
> +	return 0;
> +}
> +
>  static int cxl_region_decode_commit(struct cxl_region *cxlr)
>  {
>  	struct cxl_region_params *p = &cxlr->params;
> @@ -174,8 +190,7 @@ static int cxl_region_decode_commit(struct cxl_region *cxlr)
>  		     iter = to_cxl_port(iter->dev.parent)) {
>  			cxl_rr = cxl_rr_load(iter, cxlr);
>  			cxld = cxl_rr->decoder;
> -			if (cxld->commit)
> -				rc = cxld->commit(cxld);
> +			rc = commit_decoder(cxld);
>  			if (rc)
>  				break;
>  		}
> 

Re: [PATCH 2/3] cxl/region: Clarify when a cxld->commit() callback is mandatory

[Dan Williams]

Jonathan Cameron wrote:
> On Fri, 16 Dec 2022 17:33:38 -0800
> Dan Williams <dan.j.williams@intel.com> wrote:
> 
> > Both cxl_switch_decoders() and cxl_endpoint_decoders() are considered by
> > cxl_region_decode_commit(). Flag cases where cxl_switch_decoders with
> > multiple targets, or cxl_endpoint_decoders do not have a commit callback
> > set. The switch case is unlikely to happen since switches are only
> > enumerated by the CXL core, but the endpoint case may support decoders
> > defined by drivers outside of drivers/cxl, like accerator drivers.
> > 
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> 
> Seems reasonable though (just for consistency with earlier discussions)
> I don't like assumption that nr->targets is the right thing to decide on.
> It's fine to have decoders in single target case and if we do
> they should be committed / not registered as pass through decoders etc.

Thanks, yes, I can see there being cases where a decoder only has 1
target, but needs to be committed anyway just to pass the assigned HPA
through. For this patch that missed consideration it just leads to a
false negative report on a kernel bug failing to install a ->commit()
callback for such a decoder.

> Hmm. I've never tested the single downstream CXL switch port case.
> Similar to HB in that case HDM decoders are optional. I've no idea
> what we do about that corner currently.  One to add to my tests ;)

Yes, and certainly a heads up for anyone in the industry who might be
thinking of buliding such a device.

> Anyhow, this is fine
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Thanks!

[PATCH 3/3] cxl/port: Link the 'parent_dport' in portX/ and endpointX/ sysfs

[Dan Williams]

Similar to the justification in:

1b58b4cac6fc ("cxl/port: Record parent dport when adding ports")

...userspace wants to know the routing information for ports for
calculating the memdev order for region creation among other things.
Cache the information the kernel discovers at enumeration time in a
'parent_dport' attribute to save userspace the time of trawling sysfs
to recover the same information.

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 Documentation/ABI/testing/sysfs-bus-cxl |   15 +++++++++++++++
 drivers/cxl/core/port.c                 |   27 +++++++++++++++++++++++++++
 2 files changed, 42 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl
index 8494ef27e8d2..1b17c8cb48b5 100644
--- a/Documentation/ABI/testing/sysfs-bus-cxl
+++ b/Documentation/ABI/testing/sysfs-bus-cxl
@@ -90,6 +90,21 @@ Description:
 		capability.
 
 
+What:		/sys/bus/cxl/devices/{port,endpoint}X/parent_dport
+Date:		October, 2022
+KernelVersion:	v6.2
+Contact:	linux-cxl@vger.kernel.org
+Description:
+		(RO) CXL port objects are instantiated for each upstream port in
+		a CXL/PCIe switch, and for each endpoint to map the
+		corresponding memory device into the CXL port hierarchy. When a
+		descendant CXL port (switch or endpoint) is enumerated it is
+		useful to know which 'dport' object in the parent CXL port
+		routes to this descendant. The 'parent_dport' symlink points to
+		the device representing the downstream port of a CXL switch that
+		routes to {port,endpoint}X.
+
+
 What:		/sys/bus/cxl/devices/portX/dportY
 Date:		June, 2021
 KernelVersion:	v5.14
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index 6296d2bc909a..729e4aab5308 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -586,6 +586,29 @@ static int devm_cxl_link_uport(struct device *host, struct cxl_port *port)
 	return devm_add_action_or_reset(host, cxl_unlink_uport, port);
 }
 
+static void cxl_unlink_parent_dport(void *_port)
+{
+	struct cxl_port *port = _port;
+
+	sysfs_remove_link(&port->dev.kobj, "parent_dport");
+}
+
+static int devm_cxl_link_parent_dport(struct device *host,
+				      struct cxl_port *port,
+				      struct cxl_dport *parent_dport)
+{
+	int rc;
+
+	if (!parent_dport)
+		return 0;
+
+	rc = sysfs_create_link(&port->dev.kobj, &parent_dport->dport->kobj,
+			       "parent_dport");
+	if (rc)
+		return rc;
+	return devm_add_action_or_reset(host, cxl_unlink_parent_dport, port);
+}
+
 static struct lock_class_key cxl_port_key;
 
 static struct cxl_port *cxl_port_alloc(struct device *uport,
@@ -695,6 +718,10 @@ static struct cxl_port *__devm_cxl_add_port(struct device *host,
 	if (rc)
 		return ERR_PTR(rc);
 
+	rc = devm_cxl_link_parent_dport(host, port, parent_dport);
+	if (rc)
+		return ERR_PTR(rc);
+
 	return port;
 
 err:

Re: [PATCH 3/3] cxl/port: Link the 'parent_dport' in portX/ and endpointX/ sysfs

[Jonathan Cameron]

On Fri, 16 Dec 2022 17:33:43 -0800
Dan Williams <dan.j.williams@intel.com> wrote:

> Similar to the justification in:
> 
> 1b58b4cac6fc ("cxl/port: Record parent dport when adding ports")
> 
> ...userspace wants to know the routing information for ports for
> calculating the memdev order for region creation among other things.
> Cache the information the kernel discovers at enumeration time in a
> 'parent_dport' attribute to save userspace the time of trawling sysfs
> to recover the same information.
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

I'm not totally sold on this being worth while as opposed to building reverse
look up in userspace, but meh - seems harmless and is consistent and tiny
amount of code so

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  Documentation/ABI/testing/sysfs-bus-cxl |   15 +++++++++++++++
>  drivers/cxl/core/port.c                 |   27 +++++++++++++++++++++++++++
>  2 files changed, 42 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl
> index 8494ef27e8d2..1b17c8cb48b5 100644
> --- a/Documentation/ABI/testing/sysfs-bus-cxl
> +++ b/Documentation/ABI/testing/sysfs-bus-cxl
> @@ -90,6 +90,21 @@ Description:
>  		capability.
>  
>  
> +What:		/sys/bus/cxl/devices/{port,endpoint}X/parent_dport
> +Date:		October, 2022
> +KernelVersion:	v6.2
> +Contact:	linux-cxl@vger.kernel.org
> +Description:
> +		(RO) CXL port objects are instantiated for each upstream port in
> +		a CXL/PCIe switch, and for each endpoint to map the
> +		corresponding memory device into the CXL port hierarchy. When a
> +		descendant CXL port (switch or endpoint) is enumerated it is
> +		useful to know which 'dport' object in the parent CXL port
> +		routes to this descendant. The 'parent_dport' symlink points to
> +		the device representing the downstream port of a CXL switch that
> +		routes to {port,endpoint}X.
> +
> +
>  What:		/sys/bus/cxl/devices/portX/dportY
>  Date:		June, 2021
>  KernelVersion:	v5.14
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 6296d2bc909a..729e4aab5308 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -586,6 +586,29 @@ static int devm_cxl_link_uport(struct device *host, struct cxl_port *port)
>  	return devm_add_action_or_reset(host, cxl_unlink_uport, port);
>  }
>  
> +static void cxl_unlink_parent_dport(void *_port)
> +{
> +	struct cxl_port *port = _port;
> +
> +	sysfs_remove_link(&port->dev.kobj, "parent_dport");
> +}
> +
> +static int devm_cxl_link_parent_dport(struct device *host,
> +				      struct cxl_port *port,
> +				      struct cxl_dport *parent_dport)
> +{
> +	int rc;
> +
> +	if (!parent_dport)
> +		return 0;
> +
> +	rc = sysfs_create_link(&port->dev.kobj, &parent_dport->dport->kobj,
> +			       "parent_dport");
> +	if (rc)
> +		return rc;
> +	return devm_add_action_or_reset(host, cxl_unlink_parent_dport, port);
> +}
> +
>  static struct lock_class_key cxl_port_key;
>  
>  static struct cxl_port *cxl_port_alloc(struct device *uport,
> @@ -695,6 +718,10 @@ static struct cxl_port *__devm_cxl_add_port(struct device *host,
>  	if (rc)
>  		return ERR_PTR(rc);
>  
> +	rc = devm_cxl_link_parent_dport(host, port, parent_dport);
> +	if (rc)
> +		return ERR_PTR(rc);
> +
>  	return port;
>  
>  err:
> 

Re: [PATCH 3/3] cxl/port: Link the 'parent_dport' in portX/ and endpointX/ sysfs

[Dan Williams]

Jonathan Cameron wrote:
> On Fri, 16 Dec 2022 17:33:43 -0800
> Dan Williams <dan.j.williams@intel.com> wrote:
> 
> > Similar to the justification in:
> > 
> > 1b58b4cac6fc ("cxl/port: Record parent dport when adding ports")
> > 
> > ...userspace wants to know the routing information for ports for
> > calculating the memdev order for region creation among other things.
> > Cache the information the kernel discovers at enumeration time in a
> > 'parent_dport' attribute to save userspace the time of trawling sysfs
> > to recover the same information.
> > 
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> 
> I'm not totally sold on this being worth while as opposed to building reverse
> look up in userspace, but meh - seems harmless and is consistent and tiny
> amount of code so

Yeah, an interesting point because all of this could be done in
userspace, but I think it is useful to avoid the possibility of
mismatched expectations here, and cache something that will be
referenced often.

> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> > ---
> >  Documentation/ABI/testing/sysfs-bus-cxl |   15 +++++++++++++++
> >  drivers/cxl/core/port.c                 |   27 +++++++++++++++++++++++++++
> >  2 files changed, 42 insertions(+)
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl
> > index 8494ef27e8d2..1b17c8cb48b5 100644
> > --- a/Documentation/ABI/testing/sysfs-bus-cxl
> > +++ b/Documentation/ABI/testing/sysfs-bus-cxl
> > @@ -90,6 +90,21 @@ Description:
> >  		capability.
> >  
> >  
> > +What:		/sys/bus/cxl/devices/{port,endpoint}X/parent_dport
> > +Date:		October, 2022
> > +KernelVersion:	v6.2
> > +Contact:	linux-cxl@vger.kernel.org
> > +Description:
> > +		(RO) CXL port objects are instantiated for each upstream port in
> > +		a CXL/PCIe switch, and for each endpoint to map the
> > +		corresponding memory device into the CXL port hierarchy. When a
> > +		descendant CXL port (switch or endpoint) is enumerated it is
> > +		useful to know which 'dport' object in the parent CXL port
> > +		routes to this descendant. The 'parent_dport' symlink points to
> > +		the device representing the downstream port of a CXL switch that
> > +		routes to {port,endpoint}X.
> > +
> > +
> >  What:		/sys/bus/cxl/devices/portX/dportY
> >  Date:		June, 2021
> >  KernelVersion:	v5.14
> > diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> > index 6296d2bc909a..729e4aab5308 100644
> > --- a/drivers/cxl/core/port.c
> > +++ b/drivers/cxl/core/port.c
> > @@ -586,6 +586,29 @@ static int devm_cxl_link_uport(struct device *host, struct cxl_port *port)
> >  	return devm_add_action_or_reset(host, cxl_unlink_uport, port);
> >  }
> >  
> > +static void cxl_unlink_parent_dport(void *_port)
> > +{
> > +	struct cxl_port *port = _port;
> > +
> > +	sysfs_remove_link(&port->dev.kobj, "parent_dport");
> > +}
> > +
> > +static int devm_cxl_link_parent_dport(struct device *host,
> > +				      struct cxl_port *port,
> > +				      struct cxl_dport *parent_dport)
> > +{
> > +	int rc;
> > +
> > +	if (!parent_dport)
> > +		return 0;
> > +
> > +	rc = sysfs_create_link(&port->dev.kobj, &parent_dport->dport->kobj,
> > +			       "parent_dport");
> > +	if (rc)
> > +		return rc;
> > +	return devm_add_action_or_reset(host, cxl_unlink_parent_dport, port);
> > +}
> > +
> >  static struct lock_class_key cxl_port_key;
> >  
> >  static struct cxl_port *cxl_port_alloc(struct device *uport,
> > @@ -695,6 +718,10 @@ static struct cxl_port *__devm_cxl_add_port(struct device *host,
> >  	if (rc)
> >  		return ERR_PTR(rc);
> >  
> > +	rc = devm_cxl_link_parent_dport(host, port, parent_dport);
> > +	if (rc)
> > +		return ERR_PTR(rc);
> > +
> >  	return port;
> >  
> >  err:
> > 
> 

Re: [PATCH 3/3] cxl/port: Link the 'parent_dport' in portX/ and endpointX/ sysfs

[Dan Williams]

Dan Williams wrote:
> Jonathan Cameron wrote:
> > On Fri, 16 Dec 2022 17:33:43 -0800
> > Dan Williams <dan.j.williams@intel.com> wrote:
> > 
> > > Similar to the justification in:
> > > 
> > > 1b58b4cac6fc ("cxl/port: Record parent dport when adding ports")
> > > 
> > > ...userspace wants to know the routing information for ports for
> > > calculating the memdev order for region creation among other things.
> > > Cache the information the kernel discovers at enumeration time in a
> > > 'parent_dport' attribute to save userspace the time of trawling sysfs
> > > to recover the same information.
> > > 
> > > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > 
> > I'm not totally sold on this being worth while as opposed to building reverse
> > look up in userspace, but meh - seems harmless and is consistent and tiny
> > amount of code so
> 
> Yeah, an interesting point because all of this could be done in
> userspace, but I think it is useful to avoid the possibility of
> mismatched expectations here, and cache something that will be
> referenced often.

Note that this patch had a bug in it, and the Documenation was stale, so
I am folding in the following on applying:

diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl
index 1b17c8cb48b5..329a7e46c805 100644
--- a/Documentation/ABI/testing/sysfs-bus-cxl
+++ b/Documentation/ABI/testing/sysfs-bus-cxl
@@ -91,8 +91,8 @@ Description:
 
 
 What:		/sys/bus/cxl/devices/{port,endpoint}X/parent_dport
-Date:		October, 2022
-KernelVersion:	v6.2
+Date:		January, 2023
+KernelVersion:	v6.3
 Contact:	linux-cxl@vger.kernel.org
 Description:
 		(RO) CXL port objects are instantiated for each upstream port in
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index 8cea6df06bb5..410c036c09fa 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -1191,6 +1191,7 @@ static void delete_endpoint(void *data)
 
 	device_lock(parent);
 	if (parent->driver && !endpoint->dead) {
+		devm_release_action(parent, cxl_unlink_parent_dport, endpoint);
 		devm_release_action(parent, cxl_unlink_uport, endpoint);
 		devm_release_action(parent, unregister_port, endpoint);
 	}
@@ -1221,6 +1222,7 @@ EXPORT_SYMBOL_NS_GPL(cxl_endpoint_autoremove, CXL);
  */
 static void delete_switch_port(struct cxl_port *port)
 {
+	devm_release_action(port->dev.parent, cxl_unlink_parent_dport, port);
 	devm_release_action(port->dev.parent, cxl_unlink_uport, port);
 	devm_release_action(port->dev.parent, unregister_port, port);
 }