* [PATCH v3 2/2] cxl: Add decoders_committed sysfs attribute to cxl_port
2023-10-16 17:57 ` [PATCH v3 1/2] cxl: Add cxl_decoders_committed() helper Dave Jiang
@ 2023-10-16 17:57 ` Dave Jiang
2023-10-19 14:38 ` Jonathan Cameron
2023-10-16 19:46 ` [PATCH v3 1/2] cxl: Add cxl_decoders_committed() helper Jim Harris
` (2 subsequent siblings)
3 siblings, 1 reply; 7+ messages in thread
From: Dave Jiang @ 2023-10-16 17:57 UTC (permalink / raw)
To: linux-cxl
Cc: Jim Harris, Dan Williams, dan.j.williams, Jonathan.Cameron, dave,
alison.schofield, vishal.l.verma, ira.weiny
This attribute allows cxl-cli to determine whether there are decoders
committed to a memdev. This is only a snapshot of the state, and
doesn't offer any protection or serialization against a concurrent
disable-region operation.
Reviewed-by: Jim Harris <jim.harris@samsung.com>
Suggested-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
v2:
- Use cxl_region_rwsem to protect reading of decoder info. (Dan)
---
drivers/cxl/core/port.c | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index d0ed98a6bade..0adee1e406ec 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -534,8 +534,33 @@ static void cxl_port_release(struct device *dev)
kfree(port);
}
+static ssize_t decoders_committed_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct cxl_port *port = to_cxl_port(dev);
+ int rc;
+
+ down_read(&cxl_region_rwsem);
+ rc = sysfs_emit(buf, "%d\n", cxl_num_decoders_committed(port));
+ up_read(&cxl_region_rwsem);
+
+ return rc;
+}
+
+static DEVICE_ATTR_RO(decoders_committed);
+
+static struct attribute *cxl_port_attrs[] = {
+ &dev_attr_decoders_committed.attr,
+ NULL,
+};
+
+static struct attribute_group cxl_port_attribute_group = {
+ .attrs = cxl_port_attrs,
+};
+
static const struct attribute_group *cxl_port_attribute_groups[] = {
&cxl_base_attribute_group,
+ &cxl_port_attribute_group,
NULL,
};
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH v3 2/2] cxl: Add decoders_committed sysfs attribute to cxl_port
2023-10-16 17:57 ` [PATCH v3 2/2] cxl: Add decoders_committed sysfs attribute to cxl_port Dave Jiang
@ 2023-10-19 14:38 ` Jonathan Cameron
2023-10-28 3:39 ` Dan Williams
0 siblings, 1 reply; 7+ messages in thread
From: Jonathan Cameron @ 2023-10-19 14:38 UTC (permalink / raw)
To: Dave Jiang
Cc: linux-cxl, Jim Harris, Dan Williams, dave, alison.schofield,
vishal.l.verma, ira.weiny
On Mon, 16 Oct 2023 10:57:54 -0700
Dave Jiang <dave.jiang@intel.com> wrote:
> This attribute allows cxl-cli to determine whether there are decoders
> committed to a memdev. This is only a snapshot of the state, and
> doesn't offer any protection or serialization against a concurrent
> disable-region operation.
>
> Reviewed-by: Jim Harris <jim.harris@samsung.com>
> Suggested-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
Seems reasonable, but it is adding ABI so the docs are missing.
Documentation/ABI/testing/sysfs-bus-cxl
> ---
> v2:
> - Use cxl_region_rwsem to protect reading of decoder info. (Dan)
> ---
> drivers/cxl/core/port.c | 25 +++++++++++++++++++++++++
> 1 file changed, 25 insertions(+)
>
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index d0ed98a6bade..0adee1e406ec 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -534,8 +534,33 @@ static void cxl_port_release(struct device *dev)
> kfree(port);
> }
>
> +static ssize_t decoders_committed_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct cxl_port *port = to_cxl_port(dev);
> + int rc;
> +
> + down_read(&cxl_region_rwsem);
> + rc = sysfs_emit(buf, "%d\n", cxl_num_decoders_committed(port));
> + up_read(&cxl_region_rwsem);
> +
> + return rc;
> +}
> +
> +static DEVICE_ATTR_RO(decoders_committed);
> +
> +static struct attribute *cxl_port_attrs[] = {
> + &dev_attr_decoders_committed.attr,
> + NULL,
> +};
> +
> +static struct attribute_group cxl_port_attribute_group = {
> + .attrs = cxl_port_attrs,
> +};
> +
> static const struct attribute_group *cxl_port_attribute_groups[] = {
> &cxl_base_attribute_group,
> + &cxl_port_attribute_group,
> NULL,
> };
>
>
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v3 2/2] cxl: Add decoders_committed sysfs attribute to cxl_port
2023-10-19 14:38 ` Jonathan Cameron
@ 2023-10-28 3:39 ` Dan Williams
0 siblings, 0 replies; 7+ messages in thread
From: Dan Williams @ 2023-10-28 3:39 UTC (permalink / raw)
To: Jonathan Cameron, Dave Jiang
Cc: linux-cxl, Jim Harris, Dan Williams, dave, alison.schofield,
vishal.l.verma, ira.weiny
Jonathan Cameron wrote:
> On Mon, 16 Oct 2023 10:57:54 -0700
> Dave Jiang <dave.jiang@intel.com> wrote:
>
> > This attribute allows cxl-cli to determine whether there are decoders
> > committed to a memdev. This is only a snapshot of the state, and
> > doesn't offer any protection or serialization against a concurrent
> > disable-region operation.
> >
> > Reviewed-by: Jim Harris <jim.harris@samsung.com>
> > Suggested-by: Dan Williams <dan.j.williams@intel.com>
> > Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> Seems reasonable, but it is adding ABI so the docs are missing.
> Documentation/ABI/testing/sysfs-bus-cxl
Agreed, folded in the following:
diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl
index 087f762ebfd5..432610f41aee 100644
--- a/Documentation/ABI/testing/sysfs-bus-cxl
+++ b/Documentation/ABI/testing/sysfs-bus-cxl
@@ -178,6 +178,21 @@ Description:
hardware decoder target list.
+What: /sys/bus/cxl/devices/portX/decoders_committed
+Date: October, 2023
+KernelVersion: v6.7
+Contact: linux-cxl@vger.kernel.org
+Description:
+ (RO) A memory device is considered active when any of its
+ decoders are in the "committed" state (See CXL 3.0 8.2.4.19.7
+ CXL HDM Decoder n Control Register). Hotplug and destructive
+ operations like "sanitize" are blocked while device is actively
+ decoding a Host Physical Address range. Note that this number
+ may be elevated without any regionX objects active or even
+ enumerated, as this may be due to decoders established by
+ platform firwmare or a previous kernel (kexec).
+
+
What: /sys/bus/cxl/devices/decoderX.Y
Date: June, 2021
KernelVersion: v5.14
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v3 1/2] cxl: Add cxl_decoders_committed() helper
2023-10-16 17:57 ` [PATCH v3 1/2] cxl: Add cxl_decoders_committed() helper Dave Jiang
2023-10-16 17:57 ` [PATCH v3 2/2] cxl: Add decoders_committed sysfs attribute to cxl_port Dave Jiang
@ 2023-10-16 19:46 ` Jim Harris
2023-10-19 14:34 ` Jonathan Cameron
2023-10-23 17:49 ` Alison Schofield
3 siblings, 0 replies; 7+ messages in thread
From: Jim Harris @ 2023-10-16 19:46 UTC (permalink / raw)
To: Dave Jiang
Cc: linux-cxl@vger.kernel.org, Dan Williams,
Jonathan.Cameron@huawei.com, dave@stgolabs.net,
alison.schofield@intel.com, vishal.l.verma@intel.com,
ira.weiny@intel.com
> On Oct 16, 2023, at 10:57 AM, Dave Jiang <dave.jiang@intel.com> wrote:
>
> Add a helper to retrieve the number of decoders committed for the port.
> Replace all the open coding of the calculation with the helper.
>
> Suggested-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
Reviewed-by: Jim Harris <jim.harris@samsung.com>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3 1/2] cxl: Add cxl_decoders_committed() helper
2023-10-16 17:57 ` [PATCH v3 1/2] cxl: Add cxl_decoders_committed() helper Dave Jiang
2023-10-16 17:57 ` [PATCH v3 2/2] cxl: Add decoders_committed sysfs attribute to cxl_port Dave Jiang
2023-10-16 19:46 ` [PATCH v3 1/2] cxl: Add cxl_decoders_committed() helper Jim Harris
@ 2023-10-19 14:34 ` Jonathan Cameron
2023-10-23 17:49 ` Alison Schofield
3 siblings, 0 replies; 7+ messages in thread
From: Jonathan Cameron @ 2023-10-19 14:34 UTC (permalink / raw)
To: Dave Jiang
Cc: linux-cxl, Dan Williams, dave, alison.schofield, vishal.l.verma,
ira.weiny
On Mon, 16 Oct 2023 10:57:48 -0700
Dave Jiang <dave.jiang@intel.com> wrote:
> Add a helper to retrieve the number of decoders committed for the port.
> Replace all the open coding of the calculation with the helper.
>
> Link: https://lore.kernel.org/linux-cxl/651c98472dfed_ae7e729495@dwillia2-xfh.jf.intel.com.notmuch/
> Suggested-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> ---
> v3:
> - Rename cxl_decoders_committed() to cxl_num_decoders_comitted() (Jim)
> - Compare cxl_num_decoders_committed() to 0 to improve readability (Jim)
> v2:
> - Remove EXPORT_SYMBOL() (Dan)
> - Rebase on top of Dan's Fix shutdown order series, to pick up
> cxl_region_rwsem export.
> ---
> drivers/cxl/core/hdm.c | 7 ++++---
> drivers/cxl/core/mbox.c | 2 +-
> drivers/cxl/core/memdev.c | 4 ++--
> drivers/cxl/core/port.c | 7 +++++++
> drivers/cxl/cxl.h | 1 +
> 5 files changed, 15 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 506c9e14cdf9..7d112557c939 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -643,10 +643,11 @@ static int cxl_decoder_commit(struct cxl_decoder *cxld)
> if (cxld->flags & CXL_DECODER_F_ENABLE)
> return 0;
>
> - if (port->commit_end + 1 != id) {
> + if (cxl_num_decoders_committed(port) != id) {
> dev_dbg(&port->dev,
> "%s: out of order commit, expected decoder%d.%d\n",
> - dev_name(&cxld->dev), port->id, port->commit_end + 1);
> + dev_name(&cxld->dev), port->id,
> + cxl_num_decoders_committed(port));
> return -EBUSY;
> }
>
> @@ -863,7 +864,7 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
> cxld->target_type = CXL_DECODER_HOSTONLYMEM;
> else
> cxld->target_type = CXL_DECODER_DEVMEM;
> - if (cxld->id != port->commit_end + 1) {
> + if (cxld->id != cxl_num_decoders_committed(port)) {
> dev_warn(&port->dev,
> "decoder%d.%d: Committed out of order\n",
> port->id, cxld->id);
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index 67aec57cc12f..04033b06f75f 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -1200,7 +1200,7 @@ int cxl_mem_sanitize(struct cxl_memdev *cxlmd, u16 cmd)
> * Require an endpoint to be safe otherwise the driver can not
> * be sure that the device is unmapped.
> */
> - if (endpoint && endpoint->commit_end == -1)
> + if (endpoint && cxl_num_decoders_committed(endpoint) == 0)
> rc = __cxl_mem_sanitize(mds, cmd);
> else
> rc = -EBUSY;
> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> index fed9573cf355..fc5c2b414793 100644
> --- a/drivers/cxl/core/memdev.c
> +++ b/drivers/cxl/core/memdev.c
> @@ -231,7 +231,7 @@ int cxl_trigger_poison_list(struct cxl_memdev *cxlmd)
> if (rc)
> return rc;
>
> - if (port->commit_end == -1) {
> + if (cxl_num_decoders_committed(port) == 0) {
> /* No regions mapped to this memdev */
> rc = cxl_get_poison_by_memdev(cxlmd);
> } else {
> @@ -282,7 +282,7 @@ static struct cxl_region *cxl_dpa_to_region(struct cxl_memdev *cxlmd, u64 dpa)
> .dpa = dpa,
> };
> port = cxlmd->endpoint;
> - if (port && is_cxl_endpoint(port) && port->commit_end != -1)
> + if (port && is_cxl_endpoint(port) && cxl_num_decoders_committed(port))
> device_for_each_child(&port->dev, &ctx, __cxl_dpa_to_region);
>
> return ctx.cxlr;
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 5ba606c6e03f..d0ed98a6bade 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -37,6 +37,13 @@ DECLARE_RWSEM(cxl_region_rwsem);
> static DEFINE_IDA(cxl_port_ida);
> static DEFINE_XARRAY(cxl_root_buses);
>
> +int cxl_num_decoders_committed(struct cxl_port *port)
> +{
> + lockdep_assert_held(&cxl_region_rwsem);
> +
> + return port->commit_end + 1;
> +}
> +
> static ssize_t devtype_show(struct device *dev, struct device_attribute *attr,
> char *buf)
> {
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 76d92561af29..36bd3f06dd90 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -679,6 +679,7 @@ static inline bool is_cxl_root(struct cxl_port *port)
> return port->uport_dev == port->dev.parent;
> }
>
> +int cxl_num_decoders_committed(struct cxl_port *port);
> bool is_cxl_port(const struct device *dev);
> struct cxl_port *to_cxl_port(const struct device *dev);
> struct pci_bus;
>
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v3 1/2] cxl: Add cxl_decoders_committed() helper
2023-10-16 17:57 ` [PATCH v3 1/2] cxl: Add cxl_decoders_committed() helper Dave Jiang
` (2 preceding siblings ...)
2023-10-19 14:34 ` Jonathan Cameron
@ 2023-10-23 17:49 ` Alison Schofield
3 siblings, 0 replies; 7+ messages in thread
From: Alison Schofield @ 2023-10-23 17:49 UTC (permalink / raw)
To: Dave Jiang
Cc: linux-cxl, Dan Williams, Jonathan.Cameron, dave, vishal.l.verma,
ira.weiny
On Mon, Oct 16, 2023 at 10:57:48AM -0700, Dave Jiang wrote:
> Add a helper to retrieve the number of decoders committed for the port.
> Replace all the open coding of the calculation with the helper.
Reviewed-by: Alison Schofield <alison.schofield@intel.com>
>
> Link: https://lore.kernel.org/linux-cxl/651c98472dfed_ae7e729495@dwillia2-xfh.jf.intel.com.notmuch/
> Suggested-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>
> ---
> v3:
> - Rename cxl_decoders_committed() to cxl_num_decoders_comitted() (Jim)
> - Compare cxl_num_decoders_committed() to 0 to improve readability (Jim)
> v2:
> - Remove EXPORT_SYMBOL() (Dan)
> - Rebase on top of Dan's Fix shutdown order series, to pick up
> cxl_region_rwsem export.
> ---
> drivers/cxl/core/hdm.c | 7 ++++---
> drivers/cxl/core/mbox.c | 2 +-
> drivers/cxl/core/memdev.c | 4 ++--
> drivers/cxl/core/port.c | 7 +++++++
> drivers/cxl/cxl.h | 1 +
> 5 files changed, 15 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 506c9e14cdf9..7d112557c939 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -643,10 +643,11 @@ static int cxl_decoder_commit(struct cxl_decoder *cxld)
> if (cxld->flags & CXL_DECODER_F_ENABLE)
> return 0;
>
> - if (port->commit_end + 1 != id) {
> + if (cxl_num_decoders_committed(port) != id) {
> dev_dbg(&port->dev,
> "%s: out of order commit, expected decoder%d.%d\n",
> - dev_name(&cxld->dev), port->id, port->commit_end + 1);
> + dev_name(&cxld->dev), port->id,
> + cxl_num_decoders_committed(port));
> return -EBUSY;
> }
>
> @@ -863,7 +864,7 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
> cxld->target_type = CXL_DECODER_HOSTONLYMEM;
> else
> cxld->target_type = CXL_DECODER_DEVMEM;
> - if (cxld->id != port->commit_end + 1) {
> + if (cxld->id != cxl_num_decoders_committed(port)) {
> dev_warn(&port->dev,
> "decoder%d.%d: Committed out of order\n",
> port->id, cxld->id);
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index 67aec57cc12f..04033b06f75f 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -1200,7 +1200,7 @@ int cxl_mem_sanitize(struct cxl_memdev *cxlmd, u16 cmd)
> * Require an endpoint to be safe otherwise the driver can not
> * be sure that the device is unmapped.
> */
> - if (endpoint && endpoint->commit_end == -1)
> + if (endpoint && cxl_num_decoders_committed(endpoint) == 0)
> rc = __cxl_mem_sanitize(mds, cmd);
> else
> rc = -EBUSY;
> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> index fed9573cf355..fc5c2b414793 100644
> --- a/drivers/cxl/core/memdev.c
> +++ b/drivers/cxl/core/memdev.c
> @@ -231,7 +231,7 @@ int cxl_trigger_poison_list(struct cxl_memdev *cxlmd)
> if (rc)
> return rc;
>
> - if (port->commit_end == -1) {
> + if (cxl_num_decoders_committed(port) == 0) {
> /* No regions mapped to this memdev */
> rc = cxl_get_poison_by_memdev(cxlmd);
> } else {
> @@ -282,7 +282,7 @@ static struct cxl_region *cxl_dpa_to_region(struct cxl_memdev *cxlmd, u64 dpa)
> .dpa = dpa,
> };
> port = cxlmd->endpoint;
> - if (port && is_cxl_endpoint(port) && port->commit_end != -1)
> + if (port && is_cxl_endpoint(port) && cxl_num_decoders_committed(port))
> device_for_each_child(&port->dev, &ctx, __cxl_dpa_to_region);
>
> return ctx.cxlr;
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 5ba606c6e03f..d0ed98a6bade 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -37,6 +37,13 @@ DECLARE_RWSEM(cxl_region_rwsem);
> static DEFINE_IDA(cxl_port_ida);
> static DEFINE_XARRAY(cxl_root_buses);
>
> +int cxl_num_decoders_committed(struct cxl_port *port)
> +{
> + lockdep_assert_held(&cxl_region_rwsem);
> +
> + return port->commit_end + 1;
> +}
> +
> static ssize_t devtype_show(struct device *dev, struct device_attribute *attr,
> char *buf)
> {
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 76d92561af29..36bd3f06dd90 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -679,6 +679,7 @@ static inline bool is_cxl_root(struct cxl_port *port)
> return port->uport_dev == port->dev.parent;
> }
>
> +int cxl_num_decoders_committed(struct cxl_port *port);
> bool is_cxl_port(const struct device *dev);
> struct cxl_port *to_cxl_port(const struct device *dev);
> struct pci_bus;
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread