On 2024/10/15 03:32, Dan Williams wrote:
Zijun Hu wrote:
On 2024/10/13 06:16, Dan Williams wrote:
Zijun Hu wrote: [..]
it does NOT deserve, also does NOT need to introduce a new core driver API device_for_each_child_reverse_from(). existing device_for_each_child_reverse() can do what the _from() wants to do.
[snip]
Introduce new superset calls with the additional parameter and then rewrite the old routines to just have a simple wrapper that passes a NULL @start parameter.
Now, if Greg has the appetite to go touch all ~370 existing callers, so be it, but introducing a superset-iterator-helper and a compat-wrapper for legacy is the path I would take.
current kernel tree ONLY has 15 usages of device_for_each_child_reverse(), i would like to add an extra parameter @start as existing (class|driver)_for_each_device() and bus_for_each_(dev|drv)() do if it is required.
A new parameter to a new wrapper symbol sounds fine to me. Otherwise, please do not go thrash all the call sites to pass an unused NULL @start parameter. Just accept that device_for_each_* did not follow the {class,driver,bus}_for_each_* example and instead introduce a new symbol to wrap the new functionality that so far only has the single CXL user.
you maybe regard my idea as a alternative proposal if Greg dislike introducing a new core driver API. (^^)
[..]
If I understand your question correctly you are asking how does device_for_each_child_reverse_from() get used in cxl_region_find_decoder() to enforce in-order allocation?
yes. your recommendation may help me understand it.
below simple solution should have same effect as your recommendation. also have below optimizations:
1) it don't need new core API. 2) it is more efficient since it has minimal iterating.
i will submit it if you like it. (^^)
index e701e4b04032..37da42329ff3 100644 --- a/drivers/cxl/core/region.c +++ b/drivers/cxl/core/region.c @@ -796,8 +796,9 @@ 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_port *port = to_cxl_port(dev->parent); struct cxl_decoder *cxld; - int *id = data; + struct device **target_dev = data;
if (!is_switch_decoder(dev)) return 0; @@ -805,15 +806,19 @@ static int match_free_decoder(struct device *dev, void *data) cxld = to_cxl_decoder(dev);
/* enforce ordered allocation */ - if (cxld->id != *id) - return 0; - - if (!cxld->region) - return 1; - - (*id)++; - - return 0; + if (cxld->id == port->commit_end + 1) { + if (!cxld->region) { + *target_dev = dev; + return 1; + } else { + dev_dbg(dev->parent, + "next decoder to commit is already reserved\n", + dev_name(dev)); + return -ENODEV; + } + } else { + return cxld->flags & CXL_DECODER_F_ENABLE ? 0 : -EBUSY; + } }
static int match_auto_decoder(struct device *dev, void *data) @@ -839,7 +844,7 @@ cxl_region_find_decoder(struct cxl_port *port, struct cxl_endpoint_decoder *cxled, struct cxl_region *cxlr) { - struct device *dev; + struct device *dev = NULL; int id = 0;
if (port == cxled_to_port(cxled)) @@ -848,8 +853,8 @@ cxl_region_find_decoder(struct cxl_port *port, if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags)) dev = device_find_child(&port->dev, &cxlr->params, match_auto_decoder); - else - dev = device_find_child(&port->dev, &id, match_free_decoder); + else if (device_for_each_child(&port->dev, &dev, match_free_decoder) > 0) + get_device(dev); if (!dev) return NULL; /*
The recommendation is the following:
-- 8< -- diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c index 3478d2058303..32cde18ff31b 100644 --- a/drivers/cxl/core/region.c +++ b/drivers/cxl/core/region.c @@ -778,26 +778,50 @@ static size_t show_targetN(struct cxl_region *cxlr, char *buf, int pos) return rc; } +static int check_commit_order(struct device *dev, const void *data) +{
- struct cxl_decoder *cxld = to_cxl_decoder(dev);
- /*
* if port->commit_end is not the only free decoder, then out of* order shutdown has occurred, block further allocations until* that is resolved*/- if (((cxld->flags & CXL_DECODER_F_ENABLE) == 0))
return -EBUSY;- return 0;
+}
static int match_free_decoder(struct device *dev, void *data) {
- struct cxl_port *port = to_cxl_port(dev->parent); struct cxl_decoder *cxld;
- int *id = data;
- int rc;
if (!is_switch_decoder(dev)) return 0; cxld = to_cxl_decoder(dev);
- /* enforce ordered allocation */
- if (cxld->id != *id)
- if (cxld->id != port->commit_end + 1) return 0;
for a port, is it possible that there are multiple CXLDs with same IDs?
Not possible, that is also enforced by the driver core, all kernel object names must be unique (at least if they have the same parent), and the local subsystem convention is that all decoders are registered in id-order.