On 2024/10/15 03:32, Dan Williams wrote:
> 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.
>>>>>>
[snip]
>>> 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.
>
you maybe regard my idea as a alternative proposal if Greg dislike
introducing a new core driver API. (^^)
> [..]
>>> 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.
>>
below simple solution should have same effect as your recommendation.
also have below optimizations:
1) it don't need new core API.
2) it is more efficient since it has minimal iterating.
i will submit it if you like it. (^^)
index e701e4b04032..37da42329ff3 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -796,8 +796,9 @@ static size_t show_targetN(struct cxl_region *cxlr,
char *buf, int pos)
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;
+ struct device **target_dev = data;
if (!is_switch_decoder(dev))
return 0;
@@ -805,15 +806,19 @@ static int match_free_decoder(struct device *dev,
void *data)
cxld = to_cxl_decoder(dev);
/* enforce ordered allocation */
- if (cxld->id != *id)
- return 0;
-
- if (!cxld->region)
- return 1;
-
- (*id)++;
-
- return 0;
+ if (cxld->id == port->commit_end + 1) {
+ if (!cxld->region) {
+ *target_dev = dev;
+ return 1;
+ } else {
+ dev_dbg(dev->parent,
+ "next decoder to commit is already
reserved\n",
+ dev_name(dev));
+ return -ENODEV;
+ }
+ } else {
+ return cxld->flags & CXL_DECODER_F_ENABLE ? 0 : -EBUSY;
+ }
}
static int match_auto_decoder(struct device *dev, void *data)
@@ -839,7 +844,7 @@ cxl_region_find_decoder(struct cxl_port *port,
struct cxl_endpoint_decoder *cxled,
struct cxl_region *cxlr)
{
- struct device *dev;
+ struct device *dev = NULL;
int id = 0;
if (port == cxled_to_port(cxled))
@@ -848,8 +853,8 @@ cxl_region_find_decoder(struct cxl_port *port,
if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags))
dev = device_find_child(&port->dev, &cxlr->params,
match_auto_decoder);
- else
- dev = device_find_child(&port->dev, &id,
match_free_decoder);
+ else if (device_for_each_child(&port->dev, &dev,
match_free_decoder) > 0)
+ get_device(dev);
if (!dev)
return NULL;
/*
>>> 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.
