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