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: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?"

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;
>>  	/*
>>
>