diff mbox series

[v2,2/4] cxl/pci: Don't set up decoders for disallowed DVSEC ranges

Message ID 20240809093442.646545-3-yanfei.xu@intel.com
State Superseded
Headers show
Series Fixes for hdm docoder initialization from DVSEC ranges | expand

Commit Message

Yanfei Xu Aug. 9, 2024, 9:34 a.m. UTC 
Since it shouldn't create and configure decoders for disallowed
ranges, move the check of dvsec_range_allowed() earlier into
cxl_dvsec_rr_decode() to filter the disallowed DVSEC ranges out
at firtst.

Fixes: 34e37b4c432c ("cxl/port: Enable HDM Capability after validating DVSEC Ranges")
Signed-off-by: Yanfei Xu <yanfei.xu@intel.com>
---
 drivers/cxl/core/pci.c        | 62 +++++++++++++++++------------------
 drivers/cxl/cxl.h             |  2 +-
 drivers/cxl/port.c            |  2 +-
 tools/testing/cxl/test/mock.c |  4 +--
 4 files changed, 35 insertions(+), 35 deletions(-)

Comments

Dan Williams Aug. 9, 2024, 7:02 p.m. UTC | #1 
Yanfei Xu wrote:
> Since it shouldn't create and configure decoders for disallowed
> ranges, move the check of dvsec_range_allowed() earlier into
> cxl_dvsec_rr_decode() to filter the disallowed DVSEC ranges out
> at firtst.

"Fixes" explain the user visible side effect of the problem. There is no
problem being fixed with this. It is maybe a conceptual cleanup, but to
me the risk and thrash is not worth the reward. Unless I am missing
something this is just code movement for no value.

The "risk" is that I think this breaks cases where BIOS has enabled HDM
decoders. I.e.

        /*
         * If the HDM Decoder Capability is already enabled then assume
         * that some other agent like platform firmware set it up.
         */     
        if (global_ctrl & CXL_HDM_DECODER_ENABLE || (!hdm && info->mem_enabled))
                return devm_cxl_enable_mem(&port->dev, cxlds);

...the DVSEC range resgister values do not matter when HDM decoders are
enabled.
Yanfei Xu Aug. 10, 2024, 11:36 a.m. UTC | #2 
On 8/10/2024 3:02 AM, Dan Williams wrote:
> Yanfei Xu wrote:
>> Since it shouldn't create and configure decoders for disallowed
>> ranges, move the check of dvsec_range_allowed() earlier into
>> cxl_dvsec_rr_decode() to filter the disallowed DVSEC ranges out
>> at firtst.
> 
> "Fixes" explain the user visible side effect of the problem. There is no
> problem being fixed with this. It is maybe a conceptual cleanup, but to
> me the risk and thrash is not worth the reward. Unless I am missing
> something this is just code movement for no value.

You are right, this is much like a cleanup as creating decoders for 
disallowed ranges doesn't have side effect for user. Will drop the "Fixes".

I just was aiming to avoid to create decoders for the disallowed ranges, 
since this kind of decoders don't take effect in practice. And we 
already checked and found that, why continued to create that? :)

How about changing the dev_dbg() in below codes to dev_warn() when find 
disallowed ranges to let the user to know what happened.

         cxld_dev = device_find_child(&root->dev, &dvsec_range,
                             dvsec_range_allowed);
         if (!cxld_dev) {
             dev_dbg(dev, "DVSEC Range%d denied by platform\n", i+1);
             continue;
         }
         dev_dbg(dev, "DVSEC Range%d allowed by platform\n", i+1);
         put_device(cxld_dev);

> 
> The "risk" is that I think this breaks cases where BIOS has enabled HDM
> decoders. I.e.
> 
>          /*
>           * If the HDM Decoder Capability is already enabled then assume
>           * that some other agent like platform firmware set it up.
>           */
>          if (global_ctrl & CXL_HDM_DECODER_ENABLE || (!hdm && info->mem_enabled))
>                  return devm_cxl_enable_mem(&port->dev, cxlds);
> 
> ...the DVSEC range resgister values do not matter when HDM decoders are
> enabled.

It won't. Even no allowed range, it only keep 0 in info->ranges first. 
And the check for whether info->ranges is 0 is performed after the check 
you pasted.

     if (global_ctrl & CXL_HDM_DECODER_ENABLE || (!hdm && 
info->mem_enabled))
         return devm_cxl_enable_mem(&port->dev, cxlds);
     else if (!hdm)
         return -ENODEV;
     ......

     if (!info->ranges) {
         dev_err(dev, "No available DVSEC register ranges.\n");
         return -ENXIO;
     }


Thanks,
Yanfei
diff mbox series

Patch

diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index 2d69340134da..0915fc9e6d70 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -322,11 +322,14 @@  static int devm_cxl_enable_hdm(struct device *host, struct cxl_hdm *cxlhdm)
 	return devm_add_action_or_reset(host, disable_hdm, cxlhdm);
 }
 
-int cxl_dvsec_rr_decode(struct device *dev, int d,
+int cxl_dvsec_rr_decode(struct device *dev, struct cxl_port *port,
 			struct cxl_endpoint_dvsec_info *info)
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
+	struct cxl_dev_state *cxlds = pci_get_drvdata(pdev);
 	int hdm_count, rc, i, ranges = 0;
+	int d = cxlds->cxl_dvsec;
+	struct cxl_port *root;
 	u16 cap, ctrl;
 
 	if (!d) {
@@ -359,6 +362,14 @@  int cxl_dvsec_rr_decode(struct device *dev, int d,
 		return rc;
 	}
 
+	root = to_cxl_port(port->dev.parent);
+	while (!is_cxl_root(root) && is_cxl_port(root->dev.parent))
+		root = to_cxl_port(root->dev.parent);
+	if (!is_cxl_root(root)) {
+		dev_err(dev, "Failed to acquire root port for HDM enable\n");
+		return -ENODEV;
+	}
+
 	/*
 	 * The current DVSEC values are moot if the memory capability is
 	 * disabled, and they will remain moot after the HDM Decoder
@@ -373,6 +384,8 @@  int cxl_dvsec_rr_decode(struct device *dev, int d,
 		return 0;
 
 	for (i = 0; i < hdm_count; i++) {
+		struct device *cxld_dev;
+		struct range dvsec_range;
 		u64 base, size;
 		u32 temp;
 
@@ -389,9 +402,8 @@  int cxl_dvsec_rr_decode(struct device *dev, int d,
 			return rc;
 
 		size |= temp & CXL_DVSEC_MEM_SIZE_LOW_MASK;
-		if (!size) {
+		if (!size)
 			continue;
-		}
 
 		rc = pci_read_config_dword(
 			pdev, d + CXL_DVSEC_RANGE_BASE_HIGH(i), &temp);
@@ -407,10 +419,21 @@  int cxl_dvsec_rr_decode(struct device *dev, int d,
 
 		base |= temp & CXL_DVSEC_MEM_BASE_LOW_MASK;
 
-		info->dvsec_range[ranges++] = (struct range) {
+		dvsec_range = (struct range) {
 			.start = base,
-			.end = base + size - 1
+			.end = base + size - 1,
 		};
+
+		cxld_dev = device_find_child(&root->dev, &dvsec_range,
+							dvsec_range_allowed);
+		if (!cxld_dev) {
+			dev_dbg(dev, "DVSEC Range%d denied by platform\n", i+1);
+			continue;
+		}
+		dev_dbg(dev, "DVSEC Range%d allowed by platform\n", i+1);
+		put_device(cxld_dev);
+
+		info->dvsec_range[ranges++] = dvsec_range;
 	}
 
 	info->ranges = ranges;
@@ -433,9 +456,8 @@  int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm,
 	void __iomem *hdm = cxlhdm->regs.hdm_decoder;
 	struct cxl_port *port = cxlhdm->port;
 	struct device *dev = cxlds->dev;
-	struct cxl_port *root;
-	int i, rc, allowed;
 	u32 global_ctrl = 0;
+	int rc;
 
 	if (hdm)
 		global_ctrl = readl(hdm + CXL_HDM_DECODER_CTRL_OFFSET);
@@ -449,30 +471,8 @@  int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm,
 	else if (!hdm)
 		return -ENODEV;
 
-	root = to_cxl_port(port->dev.parent);
-	while (!is_cxl_root(root) && is_cxl_port(root->dev.parent))
-		root = to_cxl_port(root->dev.parent);
-	if (!is_cxl_root(root)) {
-		dev_err(dev, "Failed to acquire root port for HDM enable\n");
-		return -ENODEV;
-	}
-
-	for (i = 0, allowed = 0; info->mem_enabled && i < info->ranges; i++) {
-		struct device *cxld_dev;
-
-		cxld_dev = device_find_child(&root->dev, &info->dvsec_range[i],
-					     dvsec_range_allowed);
-		if (!cxld_dev) {
-			dev_dbg(dev, "DVSEC Range%d denied by platform\n", i);
-			continue;
-		}
-		dev_dbg(dev, "DVSEC Range%d allowed by platform\n", i);
-		put_device(cxld_dev);
-		allowed++;
-	}
-
-	if (!allowed && info->mem_enabled) {
-		dev_err(dev, "Range register decodes outside platform defined CXL ranges.\n");
+	if (!info->ranges && info->mem_enabled) {
+		dev_err(dev, "No available DVSEC register ranges.\n");
 		return -ENXIO;
 	}
 
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 9afb407d438f..e2e277463794 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -809,7 +809,7 @@  struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port,
 int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
 				struct cxl_endpoint_dvsec_info *info);
 int devm_cxl_add_passthrough_decoder(struct cxl_port *port);
-int cxl_dvsec_rr_decode(struct device *dev, int dvsec,
+int cxl_dvsec_rr_decode(struct device *dev, struct cxl_port *port,
 			struct cxl_endpoint_dvsec_info *info);
 
 bool is_cxl_region(struct device *dev);
diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
index d7d5d982ce69..861dde65768f 100644
--- a/drivers/cxl/port.c
+++ b/drivers/cxl/port.c
@@ -98,7 +98,7 @@  static int cxl_endpoint_port_probe(struct cxl_port *port)
 	struct cxl_port *root;
 	int rc;
 
-	rc = cxl_dvsec_rr_decode(cxlds->dev, cxlds->cxl_dvsec, &info);
+	rc = cxl_dvsec_rr_decode(cxlds->dev, port, &info);
 	if (rc < 0)
 		return rc;
 
diff --git a/tools/testing/cxl/test/mock.c b/tools/testing/cxl/test/mock.c
index 6f737941dc0e..79fdfaad49e8 100644
--- a/tools/testing/cxl/test/mock.c
+++ b/tools/testing/cxl/test/mock.c
@@ -228,7 +228,7 @@  int __wrap_cxl_hdm_decode_init(struct cxl_dev_state *cxlds,
 }
 EXPORT_SYMBOL_NS_GPL(__wrap_cxl_hdm_decode_init, CXL);
 
-int __wrap_cxl_dvsec_rr_decode(struct device *dev, int dvsec,
+int __wrap_cxl_dvsec_rr_decode(struct device *dev, struct cxl_port *port,
 			       struct cxl_endpoint_dvsec_info *info)
 {
 	int rc = 0, index;
@@ -237,7 +237,7 @@  int __wrap_cxl_dvsec_rr_decode(struct device *dev, int dvsec,
 	if (ops && ops->is_mock_dev(dev))
 		rc = 0;
 	else
-		rc = cxl_dvsec_rr_decode(dev, dvsec, info);
+		rc = cxl_dvsec_rr_decode(dev, port, info);
 	put_cxl_mock_ops(index);
 
 	return rc;