Re: [PATCH v3 2/3] cxl/region: Find free cxl decoder by device_for_each_child()

[Zijun Hu <zijun_hu@icloud.com>]

On 2024/8/27 19:53, Zijun Hu wrote:
> On 2024/8/27 19:30, Jonathan Cameron wrote:
>> On Sat, 24 Aug 2024 17:07:44 +0800
>> Zijun Hu <zijun_hu@icloud.com> wrote:
>>
>>> From: Zijun Hu <quic_zijuhu@quicinc.com>
>>>
>>> To prepare for constifying the following old driver core API:
>>>
>>> struct device *device_find_child(struct device *dev, void *data,
>>> 		int (*match)(struct device *dev, void *data));
>>> to new:
>>> struct device *device_find_child(struct device *dev, const void *data,
>>> 		int (*match)(struct device *dev, const void *data));
>>>
>>> The new API does not allow its match function (*match)() to modify
>>> caller's match data @*data, but match_free_decoder() as the old API's
>>> match function indeed modifies relevant match data, so it is not suitable
>>> for the new API any more, solved by using device_for_each_child() to
>>> implement relevant finding free cxl decoder function.
>>>
>>> Suggested-by: Ira Weiny <ira.weiny@intel.com>
>>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
>> This seems to functionally do the same as before.
>>
> 
> yes, this change have the same logic as previous existing logic.
> 
>> I'm not sure I like the original code though so a comment inline.
>>
>>> ---
>>>  drivers/cxl/core/region.c | 30 ++++++++++++++++++++++++------
>>>  1 file changed, 24 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
>>> index 21ad5f242875..c2068e90bf2f 100644
>>> --- a/drivers/cxl/core/region.c
>>> +++ b/drivers/cxl/core/region.c
>>> @@ -794,10 +794,15 @@ static size_t show_targetN(struct cxl_region *cxlr, char *buf, int pos)
>>>  	return rc;
>>>  }
>>>  
>>> +struct cxld_match_data {
>>> +	int id;
>>> +	struct device *target_device;
>>> +};
>>> +
>>>  static int match_free_decoder(struct device *dev, void *data)
>>>  {
>>> +	struct cxld_match_data *match_data = data;
>>>  	struct cxl_decoder *cxld;
>>> -	int *id = data;
>>>  
>>>  	if (!is_switch_decoder(dev))
>>>  		return 0;
>>> @@ -805,17 +810,31 @@ static int match_free_decoder(struct device *dev, void *data)
>>>  	cxld = to_cxl_decoder(dev);
>>>  
>>>  	/* enforce ordered allocation */
>>> -	if (cxld->id != *id)
>>> +	if (cxld->id != match_data->id)
>>
>> Why do we carry on in this case?
>> Conditions are:
>> 1. Start match_data->id == 0
>> 2. First pass cxld->id == 0 (all good) or
>>    cxld->id == 1 say (and we skip until we match
>>    on cxld->id == 0 (perhaps on the second child if they are
>>    ordered (1, 0, 2) etc. 
>>
>> If we skipped and then matched on second child but it was
>> already in use (so region set), we will increment match_data->id to 1
>> but never find that as it was the one we skipped.
>>
>> So this can only work if the children are ordered.
>> So if that's the case and the line above is just a sanity check
>> on that, it should be noisier (so an error print) and might
>> as well fail as if it doesn't match all bets are off.
>>
> 
> it seems Ira Weiny also has some concerns related to previous existing
> logic as following:
> 
> https://lore.kernel.org/all/66c4a136d9764_2ddc2429435@iweiny-mobl.notmuch/
> "Also for those working on CXL I'm questioning the use of ID here and
> the dependence on the id's being added to the parent in order.  Is that
> a guarantee?"
> 
Hi Jonathan, ira

i don't know CXL decoder at all. For your concerns about original logic:

is it okay to find a child with the minimal index as shown below ?

i would like to submit it as a separate patch if you like it.


diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 21ad5f242875..d10cca02eba8 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -797,21 +797,27 @@ 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_decoder *cxld;
-       int *id = data;
+       struct cxl_decoder *target_cxld;
+       struct device **target_device = data;

        if (!is_switch_decoder(dev))
                return 0;

        cxld = to_cxl_decoder(dev);

-       /* enforce ordered allocation */
-       if (cxld->id != *id)
+       if (cxld->region)
                return 0;

-       if (!cxld->region)
-               return 1;
+       if (!*target_device) {
+               *target_device = get_device(dev);
+               return 0;
+       }

-       (*id)++;
+       target_cxld = to_cxl_decoder(*target_device);
+       if (cxld->id < target_cxld->id) {
+               put_device(*target_device);
+               *target_device = get_device(dev);
+       }

        return 0;
 }
@@ -839,8 +845,7 @@ cxl_region_find_decoder(struct cxl_port *port,
                        struct cxl_endpoint_decoder *cxled,
                        struct cxl_region *cxlr)
 {
-       struct device *dev;
-       int id = 0;
+       struct device *dev = NULL;

        if (port == cxled_to_port(cxled))
                return &cxled->cxld;
@@ -849,7 +854,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);
+               device_for_each_child(&port->dev, &dev, match_free_decoder);
        if (!dev)
                return NULL;
        /*


> perhaps, create a new dedicated thread to discuss original design.
> 
>> Jonathan
>>  
>>>  		return 0;
>>>  
>>> -	if (!cxld->region)
>>> +	if (!cxld->region) {
>>> +		match_data->target_device = get_device(dev);
>>>  		return 1;
>>> +	}
>>>  
>>> -	(*id)++;
>>> +	match_data->id++;
>>>  
>>>  	return 0;
>>>  }
>>>  
>>> +/* NOTE: need to drop the reference with put_device() after use. */
>>> +static struct device *find_free_decoder(struct device *parent)
>>> +{
>>> +	struct cxld_match_data match_data = {
>>> +		.id = 0,
>>> +		.target_device = NULL,
>>> +	};
>>> +
>>> +	device_for_each_child(parent, &match_data, match_free_decoder);
>>> +	return match_data.target_device;
>>> +}
>>> +
>>>  static int match_auto_decoder(struct device *dev, void *data)
>>>  {
>>>  	struct cxl_region_params *p = data;
>>> @@ -840,7 +859,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;
>>> @@ -849,7 +867,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 = find_free_decoder(&port->dev);
>>>  	if (!dev)
>>>  		return NULL;
>>>  	/*
>>>
>>
>