diff mbox series

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

Message ID 20240815-const_dfc_prepare-v2-2-8316b87b8ff9@quicinc.com
State Superseded
Headers show
Series driver core: Prevent device_find_child() from modifying caller's match data | expand

Commit Message

Zijun Hu Aug. 15, 2024, 2:58 p.m. UTC
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.

Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 drivers/cxl/core/region.c | 36 +++++++++++++++++++++++++++++++++++-
 1 file changed, 35 insertions(+), 1 deletion(-)

Comments

Ira Weiny Aug. 20, 2024, 1:59 p.m. UTC | #1
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.

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.

> +	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?

> +{
> +	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.

> +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).

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)) {                                 
+               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;                                        
+}                                                                              
+                                                                               
 static int match_auto_decoder(struct device *dev, void *data)                  
 {                                                                              
        struct cxl_region_params *p = data;                                     
@@ -840,7 +858,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 +866,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 = find_free_decoder(&port->dev);                            
+                                                                               
        if (!dev)                                                               
                return NULL;                                                    
        /*
Zijun Hu Aug. 21, 2024, 12:46 p.m. UTC | #2
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.

>> +{
>> +	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.

>> +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 ?

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.

> +               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;                                        
> +}                                                                              
> +                                                                               
>  static int match_auto_decoder(struct device *dev, void *data)                  
>  {                                                                              
>         struct cxl_region_params *p = data;                                     
> @@ -840,7 +858,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 +866,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 = find_free_decoder(&port->dev);                            
> +                                                                               
>         if (!dev)                                                               
>                 return NULL;                                                    
>         /*
Ira Weiny Aug. 23, 2024, 6:10 p.m. UTC | #3
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?

> 
> >> +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:

> 
> 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()?

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]
Zijun Hu Aug. 23, 2024, 10:18 p.m. UTC | #4
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]
Greg KH Aug. 24, 2024, 3:29 a.m. UTC | #5
On Tue, Aug 20, 2024 at 08:59:18AM -0500, 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.
> 
> 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.
> 
> > +	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?
> 
> > +{
> > +	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.
> 
> > +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).
> 
> 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)) {                                 
> +               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;                                        
> +}                                                                              

I like this type of re-write much better, thanks!

greg k-h
diff mbox series

Patch

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 {
+	int (*match)(struct device *dev, void *data);
+	void *data;
+	struct device *target_device;
+};
+
+static int cxl_dfc_match_modify(struct device *dev, void *data)
+{
+	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.
+ */
+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);
 	if (!dev)
 		return NULL;
 	/*