| Message ID | 20240811-const_dfc_prepare-v1-3-d67cc416b3d3@quicinc.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | driver core: Prevent device_find_child() from modifying caller's match data | expand |
On 8/11/24 02:18, Zijun Hu wrote: > From: Zijun Hu <quic_zijuhu@quicinc.com> > > It does not make sense for match_free_decoder() as device_find_child()'s > match function to modify caller's match data, match_free_decoder() is just doing that, treating caller's match data as a piece of memory to store their int. (So it is hard to tell it does not make sense "for [it] ... to"). > fixed by using > constify_device_find_child_helper() instead of device_find_child(). I don't like the constify... name, I would go with something like device_find_child_mut() or similar. > > Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com> > --- > drivers/cxl/core/region.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > index 21ad5f242875..266231d69dff 100644 > --- a/drivers/cxl/core/region.c > +++ b/drivers/cxl/core/region.c > @@ -849,7 +849,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 = constify_device_find_child_helper(&port->dev, &id, > + match_free_decoder); > if (!dev) > return NULL; > /* >
On 2024/8/12 20:54, Przemek Kitszel wrote: > On 8/11/24 02:18, Zijun Hu wrote: >> From: Zijun Hu <quic_zijuhu@quicinc.com> >> >> It does not make sense for match_free_decoder() as device_find_child()'s >> match function to modify caller's match data, > > match_free_decoder() is just doing that, treating caller's match data as > a piece of memory to store their int. > (So it is hard to tell it does not make sense "for [it] ... to"). > Thanks for reply (^^) The ultimate goal is to make device_find_child() have below prototype: struct device *device_find_child(struct device *dev, const void *data, int (*match)(struct device *dev, const void *data)); Why ? (1) It does not make sense, also does not need to, for such device finding operation to modify caller's match data which is mainly used for comparison. (2) It will make the API's match function parameter have the same signature as all other APIs (bus|class|driver)_find_device(). My idea is that: use device_find_child() for READ only accessing caller's match data. use below API if need to Modify caller's data as constify_device_find_child_helper() does. int device_for_each_child(struct device *dev, void *data, int (*fn)(struct device *dev, void *data)); So match_free_decoder() is not proper as device_find_child()'s match function. >> fixed by using >> constify_device_find_child_helper() instead of device_find_child(). > > I don't like the constify... name, I would go with something like > device_find_child_mut() or similar. > What about below alternative option i ever thought about ? Don't introduce API constify_device_find_child_helper() at all, and change in involved driver such as cxl/core/region.c directly. >> >> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com> >> --- >> drivers/cxl/core/region.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c >> index 21ad5f242875..266231d69dff 100644 >> --- a/drivers/cxl/core/region.c >> +++ b/drivers/cxl/core/region.c >> @@ -849,7 +849,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 = constify_device_find_child_helper(&port->dev, &id, >> + match_free_decoder); >> if (!dev) >> return NULL; >> /* >> >
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c index 21ad5f242875..266231d69dff 100644 --- a/drivers/cxl/core/region.c +++ b/drivers/cxl/core/region.c @@ -849,7 +849,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 = constify_device_find_child_helper(&port->dev, &id, + match_free_decoder); if (!dev) return NULL; /*