diff mbox series

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

Message ID 167124082375.1626103.6047000000121298560.stgit@dwillia2-xfh.jf.intel.com
State Accepted
Commit 172738bbccdb4ef76bdd72fc72a315c741c39161
Headers show
Series cxl: Misc fixups that missed v6.2 | expand

Commit Message

Dan Williams Dec. 17, 2022, 1:33 a.m. UTC 
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(+)

Comments

Jonathan Cameron Jan. 13, 2023, 11:39 a.m. UTC | #1 
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:
>
Dan Williams Jan. 25, 2023, 10:46 p.m. UTC | #2 
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:
> > 
>
Dan Williams Jan. 25, 2023, 11:32 p.m. UTC | #3 
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);
 }
diff mbox series

Patch

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: