Re: [PATCH v2 2/4] cxl/region: Prevent device_find_child() from modifying caller's match data

[Zijun Hu <zijun_hu@icloud.com>]

On 2024/8/24 02:10, Ira Weiny wrote:
> Zijun Hu wrote:
>> On 2024/8/20 21:59, Ira Weiny wrote:
>>> Zijun Hu 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, fixed by implementing a equivalent
>>>> cxl_device_find_child() instead of the old API usage.
>>>
>>> Generally it seems ok but I think some name changes will make this more
>>> clear.  See below.
>>>
>>
>> okay.
>>
>>> 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?
>>>
>>>>
>>>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
>>>> ---
>>>>  drivers/cxl/core/region.c | 36 +++++++++++++++++++++++++++++++++++-
>>>>  1 file changed, 35 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
>>>> index 21ad5f242875..8d8f0637f7ac 100644
>>>> --- a/drivers/cxl/core/region.c
>>>> +++ b/drivers/cxl/core/region.c
>>>> @@ -134,6 +134,39 @@ static const struct attribute_group *get_cxl_region_access1_group(void)
>>>>  	return &cxl_region_access1_coordinate_group;
>>>>  }
>>>>  
>>>> +struct cxl_dfc_data {
>>>
>>> struct cxld_match_data
>>>
>>> 'cxld' == cxl decoder in our world.
>>>
>>
>> make sense.
>>
>>>> +	int (*match)(struct device *dev, void *data);
>>>> +	void *data;
>>>> +	struct device *target_device;
>>>> +};
>>>> +
>>>> +static int cxl_dfc_match_modify(struct device *dev, void *data)
>>>
>>> Why not just put this logic into match_free_decoder?
>>>
>>
>> Actually, i ever considered solution B as you suggested in the end.
>>
>> For this change, namely, solution A:
>> 1) this change is clearer and easier to understand.
>> 2) this change does not touch any existing cxld logic
>>
>> For solution B:
>> it is more reasonable
>>
>> i finally select A since it can express my concern and relevant solution
>> clearly.
> 
> Understood.
> 
>>
>>>> +{
>>>> +	struct cxl_dfc_data *dfc_data = data;
>>>> +	int res;
>>>> +
>>>> +	res = dfc_data->match(dev, dfc_data->data);
>>>> +	if (res && get_device(dev)) {
>>>> +		dfc_data->target_device = dev;
>>>> +		return res;
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +/*
>>>> + * I have the same function as device_find_child() but allow to modify
>>>> + * caller's match data @*data.
>>>> + */
>>>
>>> No need for this comment after the new API is established.
>>>
>>
>> i have given up the idea within v1 to introduce a new API which *should
>> ONLY* be used by this patch series, so it is not worthy of a new API
>> even if it can bring convenient for this patch series.
> 
> I'm not clear on this.  Are you still proposing to change the parameter to
> const?
> 
yes.
>>
>>>> +static struct device *cxl_device_find_child(struct device *parent, void *data,
>>>> +					    int (*match)(struct device *dev, void *data))
>>>> +{
>>>> +	struct cxl_dfc_data dfc_data = {match, data, NULL};
>>>> +
>>>> +	device_for_each_child(parent, &dfc_data, cxl_dfc_match_modify);
>>>> +	return dfc_data.target_device;
>>>> +}
>>>> +
>>>>  static ssize_t uuid_show(struct device *dev, struct device_attribute *attr,
>>>>  			 char *buf)
>>>>  {
>>>> @@ -849,7 +882,8 @@ 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 = cxl_device_find_child(&port->dev, &id,
>>>> +					    match_free_decoder);
>>>
>>> This is too literal.  How about the following (passes basic cxl-tests).
>>>
>>
>> it is reasonable.
>>
>> do you need me to submit that you suggest in the end and add you as
>> co-developer ?
> 
> You can submit it with Suggested-by:
> 
okay.
>>
>> OR
>>
>> you submit it by yourself ?
>>
>> either is okay for me.
>>
>>> Ira
>>>
>>> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c              
>>> index 21ad5f242875..c1e46254efb8 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,30 @@ 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)                                         
>>>                 return 0;                                                       
>>>                                                                                 
>>> -       if (!cxld->region)                                                      
>>> +       if (!cxld->region && get_device(dev)) {                                 
>>
>> get_device(dev) failure may cause different logic against existing
>> but i think it should be impossible to happen normally.
> 
> Indeed this is slightly different.  :-/
> 
> Move the get_device() to find_free_decoder()?
> 
i think we can keep your change. so ignore this slight difference.
i also notice that you have done some verification for this change.
> Ira
> 
>>
>>> +               match_data->target_device = dev;                                
>>>                 return 1;                                                       
>>> +       }                                                                       
>>>                                                                                 
>>> -       (*id)++;                                                                
>>> +       match_data->id++;                                                       
>>>                                                                                 
>>>         return 0;                                                               
>>>  }                                                                              
>>>                                                                                 
>>> +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;                                        
>>> +}                                                                              
>>> +                                                                               
> 
> [snip]