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@xxxxxxxxxxx/ > > > > 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. > 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@xxxxxxxxxxxxxxxxxxxxxxxxx.notmuch/ > > 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? 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; - 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); if (!dev) return NULL; /*
