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. > >>>> > >>>> 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. 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. [..] > > 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? 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.
