From: Zijun Hu quic_zijuhu@quicinc.com
It is bad for current match_free_decoder()'s logic to find a free switch cxl decoder as explained below:
- If all child decoders are sorted by ID in ascending order, then current logic can be simplified as below one:
static int match_free_decoder(struct device *dev, void *data) { struct cxl_decoder *cxld;
if (!is_switch_decoder(dev)) return 0;
cxld = to_cxl_decoder(dev); return cxld->region ? 0 : 1; } dev = device_find_child(&port->dev, NULL, match_free_decoder);
which does not also need to modify device_find_child()'s match data.
- If all child decoders are NOT sorted by ID in ascending order, then current logic are wrong as explained below:
F: free, (cxld->region == NULL) B: busy, (cxld->region != NULL) S(n)F : State of switch cxl_decoder with ID n is Free S(n)B : State of switch cxl_decoder with ID n is Busy
Provided there are 2 child decoders: S(1)F -> S(0)B, then current logic will fail to find a free decoder even if there are a free one with ID 1
Anyway, current logic is not good, fixed by finding a free switch cxl decoder with minimal ID.
Fixes: 384e624bb211 ("cxl/region: Attach endpoint decoders") Closes: https://lore.kernel.org/all/cdfc6f98-1aa0-4cb5-bd7d-93256552c39b@icloud.com/ Cc: stable@vger.kernel.org Signed-off-by: Zijun Hu quic_zijuhu@quicinc.com --- Changes in v2: - Correct title and commit message - Link to v1: https://lore.kernel.org/r/20240903-fix_cxld-v1-1-61acba7198ae@quicinc.com --- drivers/cxl/core/region.c | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-)
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c index 21ad5f242875..b9607b4fc40b 100644 --- a/drivers/cxl/core/region.c +++ b/drivers/cxl/core/region.c @@ -797,21 +797,26 @@ static size_t show_targetN(struct cxl_region *cxlr, char *buf, int pos) static int match_free_decoder(struct device *dev, void *data) { struct cxl_decoder *cxld; - int *id = data; + struct cxl_decoder *target_cxld; + struct device **target_device = data;
if (!is_switch_decoder(dev)) return 0;
cxld = to_cxl_decoder(dev); - - /* enforce ordered allocation */ - if (cxld->id != *id) + if (cxld->region) return 0;
- if (!cxld->region) - return 1; - - (*id)++; + if (!*target_device) { + *target_device = get_device(dev); + return 0; + } + /* enforce ordered allocation */ + target_cxld = to_cxl_decoder(*target_device); + if (cxld->id < target_cxld->id) { + put_device(*target_device); + *target_device = get_device(dev); + }
return 0; } @@ -839,8 +844,7 @@ cxl_region_find_decoder(struct cxl_port *port, struct cxl_endpoint_decoder *cxled, struct cxl_region *cxlr) { - struct device *dev; - int id = 0; + struct device *dev = NULL;
if (port == cxled_to_port(cxled)) return &cxled->cxld; @@ -849,7 +853,8 @@ cxl_region_find_decoder(struct cxl_port *port, dev = device_find_child(&port->dev, &cxlr->params, match_auto_decoder); else - dev = device_find_child(&port->dev, &id, match_free_decoder); + /* Need to put_device(@dev) after use */ + device_for_each_child(&port->dev, &dev, match_free_decoder); if (!dev) return NULL; /*
--- base-commit: 67784a74e258a467225f0e68335df77acd67b7ab change-id: 20240903-fix_cxld-4f6575a90619
Best regards,