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