Re: [PATCH v3 07/18] cxl/region: Avoid duplicate call of cxl_find_decoder_early()

Jonathan Cameron
2025-02-14T16:07:45Z

On Tue, 11 Feb 2025 10:53:37 +0100
Robert Richter <rrichter@amd.com> wrote:

> Function cxl_find_decoder_early() is called twice, in
> alloc_region_ref() and cxl_rr_alloc_decoder(). Move it out there and

out where? I'd make it clear that both these calls are in
cxl_port_attach_region()

> instead pass the decoder as function argument to both.
>
> Signed-off-by: Robert Richter <rrichter@amd.com>
> Reviewed-by: Gregory Price <gourry@gourry.net>
> Tested-by: Gregory Price <gourry@gourry.net>

I think this is fine but it's not immediately obvious so a request
inline for some more details in this description.

> ---
> drivers/cxl/core/region.c | 31 +++++++++++++++----------------
> 1 file changed, 15 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 13e3ba984a53..b8201c2faa87 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -908,7 +908,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;
> @@ -922,9 +923,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);

This is buried a little deep to be obviously fine to lift out.
Seems like it should always have been done outside the xa_for_each()
loop in here. So I think this is fine but maybe some more in the
patch description is needed to make that point.

> if (auto_order_ok(port, iter->region, cxld))
> continue;
> }
> @@ -1008,17 +1006,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),
> @@ -1109,7 +1099,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",
> @@ -1118,7 +1117,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;
> }