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.
we can use similar approach as below link shown: https://lore.kernel.org/all/20240815-const_dfc_prepare-v2-2-8316b87b8ff9@qui...
No, just have a simple starting point parameter. I understand that more logic can be placed around device_for_each_child_reverse() to achieve the same effect, but the core helpers should be removing logic from consumers, not forcing them to add more.
If bloat is a concern, then after your const cleanups go through device_for_each_child_reverse() can be rewritten in terms of device_for_each_child_reverse_from() as (untested):
bloat is one aspect, the other aspect is that there are redundant between both driver core APIs, namely, there are a question:
why to still need device_for_each_child_reverse() if it is same as _from(..., NULL, ...) ?
To allow access to the new functionality without burdening existing callers. With device_for_each_child_reverse() re-written to internally use device_for_each_child_reverse_from() Linux gets more functionality with no disruption to existing users and no bloat. This is typical API evolution.
So i suggest use existing API now.
No, I am failing to understand your concern.
if there are more users who have such starting point requirement, then add the parameter into device_for_each_child_reverse() which is consistent with other existing *_for_each_*() core APIs such as (class|driver|bus)_for_each_device() and bus_for_each_drv(), that may need much efforts.
There are ~370 existing device_for_each* callers. Let's not thrash them.
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.
could you please contains your proposal "fixing this allocation order validation" of below link into this patch series with below reason? and Cc me (^^)
https://lore.kernel.org/all/670835f5a2887_964f229474@dwillia2-xfh.jf.intel.c...
A) the proposal depends on this patch series. B) one of the issues the proposal fix is match_free_decoder() error logic which is also relevant issues this patch series fix, the proposal also can fix the other device_find_child()'s match() issue i care about.
C) Actually, it is a bit difficult for me to understand the proposal since i don't have any basic knowledge about CXL. (^^)
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.
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?
- if (!cxld->region)
return 1;- (*id)++;
- if (cxld->region) {
dev_dbg(dev->parent,"next decoder to commit is already reserved\n",dev_name(dev));return 0;- }
- return 0;
- rc = device_for_each_child_reverse_from(dev->parent, dev, NULL,
check_commit_order);- if (rc) {
dev_dbg(dev->parent,"unable to allocate %s due to out of order shutdown\n",dev_name(dev));return 0;- }
- return 1;
} static int match_auto_decoder(struct device *dev, void *data) @@ -824,7 +848,6 @@ cxl_region_find_decoder(struct cxl_port *port, struct cxl_region *cxlr) { struct device *dev;
- int id = 0;
if (port == cxled_to_port(cxled)) return &cxled->cxld; @@ -833,7 +856,7 @@ 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);
dev = device_find_child(&port->dev, NULL, match_free_decoder);