diff mbox series

[v4,06/14] cxl/region: Avoid duplicate call of cxl_find_decoder_early()

Message ID 20250306164448.3354845-7-rrichter@amd.com
State New
Headers show
Series cxl: Address translation support, part 1: Cleanups and refactoring | expand

Commit Message

Robert Richter March 6, 2025, 4:44 p.m. UTC 
Function cxl_find_decoder_early() is called twice, in
alloc_region_ref() and cxl_rr_alloc_decoder(). Both functions are
subsequently called from cxl_port_attach_region(). Make the decoder a
function argument to both which avoids a duplicate call of
cxl_find_decoder_early().

Moving the call out of alloc_region_ref() also moves it out of the
xa_for_each() loop in there. Now, cxld is determined no longer only
for each auto-generated region, but now once for all regions
regardless of auto-generated or not. This is fine as the cxld argument
is needed for all regions in cxl_rr_alloc_decoder() and an error would
be returned otherwise anyway. So it is better to determine the decoder
in front of all this and fail early if missing instead of running
through all that code with multiple calls of cxl_find_decoder_early().

Signed-off-by: Robert Richter <rrichter@amd.com>
Reviewed-by: Gregory Price <gourry@gourry.net>
Tested-by: Gregory Price <gourry@gourry.net>
---
 drivers/cxl/core/region.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

Comments

Dan Williams March 6, 2025, 8:01 p.m. UTC | #1 
Robert Richter wrote:
> Function cxl_find_decoder_early() is called twice, in
> alloc_region_ref() and cxl_rr_alloc_decoder(). Both functions are
> subsequently called from cxl_port_attach_region(). Make the decoder a
> function argument to both which avoids a duplicate call of
> cxl_find_decoder_early().
> 
> Moving the call out of alloc_region_ref() also moves it out of the
> xa_for_each() loop in there. Now, cxld is determined no longer only
> for each auto-generated region, but now once for all regions
> regardless of auto-generated or not. This is fine as the cxld argument
> is needed for all regions in cxl_rr_alloc_decoder() and an error would
> be returned otherwise anyway. So it is better to determine the decoder
> in front of all this and fail early if missing instead of running
> through all that code with multiple calls of cxl_find_decoder_early().
> 
> Signed-off-by: Robert Richter <rrichter@amd.com>
> Reviewed-by: Gregory Price <gourry@gourry.net>
> Tested-by: Gregory Price <gourry@gourry.net>
> ---
>  drivers/cxl/core/region.c | 31 +++++++++++++++----------------
>  1 file changed, 15 insertions(+), 16 deletions(-)

The change of order makes some sense, but now none of the naming makes
any sense.

So, if you pickup the cxl_port_pick_region_decoder() rename, that
implies an allocation. Then it follows that cxl_rr_alloc_decoder()
should be renamed to something like cxl_rr_assign_decoder(), because the
"alloc" was what cxl_region_find_decoder() was doing in that function.

That also would imply that cxl_rr_assign_decoder() cannot fail if all
the allocation work is done ahead of time.
diff mbox series

Patch

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index e40ae0fefddc..5a7a1dd583aa 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -924,7 +924,8 @@  static bool auto_order_ok(struct cxl_port *port, struct cxl_region *cxlr_iter,
 
 static struct cxl_region_ref *
 alloc_region_ref(struct cxl_port *port, struct cxl_region *cxlr,
-		 struct cxl_endpoint_decoder *cxled)
+		 struct cxl_endpoint_decoder *cxled,
+		 struct cxl_decoder *cxld)
 {
 	struct cxl_region_params *p = &cxlr->params;
 	struct cxl_region_ref *cxl_rr, *iter;
@@ -938,9 +939,6 @@  alloc_region_ref(struct cxl_port *port, struct cxl_region *cxlr,
 			continue;
 
 		if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags)) {
-			struct cxl_decoder *cxld;
-
-			cxld = cxl_find_decoder_early(port, cxled, cxlr);
 			if (auto_order_ok(port, iter->region, cxld))
 				continue;
 		}
@@ -1024,17 +1022,9 @@  static int cxl_rr_ep_add(struct cxl_region_ref *cxl_rr,
 
 static int cxl_rr_alloc_decoder(struct cxl_port *port, struct cxl_region *cxlr,
 				struct cxl_endpoint_decoder *cxled,
-				struct cxl_region_ref *cxl_rr)
+				struct cxl_region_ref *cxl_rr,
+				struct cxl_decoder *cxld)
 {
-	struct cxl_decoder *cxld;
-
-	cxld = cxl_find_decoder_early(port, cxled, cxlr);
-	if (!cxld) {
-		dev_dbg(&cxlr->dev, "%s: no decoder available\n",
-			dev_name(&port->dev));
-		return -EBUSY;
-	}
-
 	if (cxld->region) {
 		dev_dbg(&cxlr->dev, "%s: %s already attached to %s\n",
 			dev_name(&port->dev), dev_name(&cxld->dev),
@@ -1125,7 +1115,16 @@  static int cxl_port_attach_region(struct cxl_port *port,
 			nr_targets_inc = true;
 		}
 	} else {
-		cxl_rr = alloc_region_ref(port, cxlr, cxled);
+		struct cxl_decoder *cxld;
+
+		cxld = cxl_find_decoder_early(port, cxled, cxlr);
+		if (!cxld) {
+			dev_dbg(&cxlr->dev, "%s: no decoder available\n",
+				dev_name(&port->dev));
+			return -EBUSY;
+		}
+
+		cxl_rr = alloc_region_ref(port, cxlr, cxled, cxld);
 		if (IS_ERR(cxl_rr)) {
 			dev_dbg(&cxlr->dev,
 				"%s: failed to allocate region reference\n",
@@ -1134,7 +1133,7 @@  static int cxl_port_attach_region(struct cxl_port *port,
 		}
 		nr_targets_inc = true;
 
-		rc = cxl_rr_alloc_decoder(port, cxlr, cxled, cxl_rr);
+		rc = cxl_rr_alloc_decoder(port, cxlr, cxled, cxl_rr, cxld);
 		if (rc)
 			goto out_erase;
 	}