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@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.
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.