| Message ID | 20240824-const_dfc_prepare-v3-2-32127ea32bba@quicinc.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | driver core: Prevent device_find_child() from modifying caller's match data | expand |
On 2024/8/24 17:07, 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, 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> > --- > 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) > return 0; > > - if (!cxld->region) > + if (!cxld->region) { > + match_data->target_device = get_device(dev); get_device() must != NULL since @dev != NULL for the function parameter of both device_for_each_child() and device_find_child(). so this change's logic is same as previous existing logic. > 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; > /* >
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. 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. 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; > /* >
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; >> /* >> >
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; >>> /* >>> >> >
Jonathan Cameron wrote: > On Sat, 24 Aug 2024 17:07:44 +0800 > Zijun Hu <zijun_hu@icloud.com> wrote: > [snip] > > > > +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. > I've worked through this with Dan and the devices are added in order in devm_cxl_enumerate_decoders(). So I don't think there is an issue with converting the code directly. Sorry for the noise Jijun, Ira
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) 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; /*