| Message ID | 20250306164448.3354845-11-rrichter@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | cxl: Address translation support, part 1: Cleanups and refactoring | expand |
Robert Richter wrote: > Factor out code to find the switch decoder of a port for a specific > address range. Reuse the code to search a root decoder, create the > function cxl_port_find_switch_decoder() and rework > match_root_decoder_by_range() to be usable for switch decoders too. > > Signed-off-by: Robert Richter <rrichter@amd.com> > Reviewed-by: Gregory Price <gourry@gourry.net> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > Reviewed-by: Dave Jiang <dave.jiang@intel.com> > Tested-by: Gregory Price <gourry@gourry.net> > --- > drivers/cxl/core/region.c | 48 +++++++++++++++++++++++---------------- > 1 file changed, 28 insertions(+), 20 deletions(-) > > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > index 70ff4c94fb7a..cf58ee284696 100644 > --- a/drivers/cxl/core/region.c > +++ b/drivers/cxl/core/region.c > @@ -3198,33 +3198,48 @@ static int devm_cxl_add_dax_region(struct cxl_region *cxlr) > return rc; > } > > -static int match_root_decoder_by_range(struct device *dev, > - const void *data) > +static int match_decoder_by_range(struct device *dev, const void *data) > { > const struct range *r1, *r2 = data; > - struct cxl_root_decoder *cxlrd; > + struct cxl_decoder *cxld; > > - if (!is_root_decoder(dev)) > + if (!is_switch_decoder(dev)) > return 0; > > - cxlrd = to_cxl_root_decoder(dev); > - r1 = &cxlrd->cxlsd.cxld.hpa_range; > + cxld = to_cxl_decoder(dev); > + r1 = &cxld->hpa_range; > return range_contains(r1, r2); > } > > +static struct cxl_decoder * > +cxl_port_find_switch_decoder(struct cxl_port *port, struct range *hpa) > +{ > + /* > + * device_find_child() increments the reference count of the > + * the switch decoder's parent port to protect the reference > + * to its child. The port is already a parent of the endpoint > + * decoder's port, at least indirectly in the port hierarchy. > + * Thus, the endpoint already holds a reference for the parent > + * port of the switch decoder. Free the unnecessary reference > + * here. > + */ > + struct device *cxld_dev __free(put_device) = > + device_find_child(&port->dev, hpa, match_decoder_by_range); > + > + return cxld_dev ? to_cxl_decoder(cxld_dev) : NULL; After seeing this comment block repeated, I am less enthusiastic about this approach. I am worried about cases where something wants to potentially hold the reference outside of the lifetime of the endpoint. I do not think we necessarily have those today but there is now a mix of "find" helpers that assume the caller will maintain an endpoint port reference and subtle bugs looming if that assumption is violated. Part of the reason for the "put_device early with a comment" approach was to not need to deal with some of the awkwardness of dropping the reference in all exit paths. However, the scope-based cleanup helpers now automate that awkwardness. So, I would much rather have a DEFINE_FREE(put_cxl_<object>) to pair with each "find" operation and drop all these repeated "its ok to violate typical reference expectations" comment blocks. So, that's a nak for this approach from me.
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c index 70ff4c94fb7a..cf58ee284696 100644 --- a/drivers/cxl/core/region.c +++ b/drivers/cxl/core/region.c @@ -3198,33 +3198,48 @@ static int devm_cxl_add_dax_region(struct cxl_region *cxlr) return rc; } -static int match_root_decoder_by_range(struct device *dev, - const void *data) +static int match_decoder_by_range(struct device *dev, const void *data) { const struct range *r1, *r2 = data; - struct cxl_root_decoder *cxlrd; + struct cxl_decoder *cxld; - if (!is_root_decoder(dev)) + if (!is_switch_decoder(dev)) return 0; - cxlrd = to_cxl_root_decoder(dev); - r1 = &cxlrd->cxlsd.cxld.hpa_range; + cxld = to_cxl_decoder(dev); + r1 = &cxld->hpa_range; return range_contains(r1, r2); } +static struct cxl_decoder * +cxl_port_find_switch_decoder(struct cxl_port *port, struct range *hpa) +{ + /* + * device_find_child() increments the reference count of the + * the switch decoder's parent port to protect the reference + * to its child. The port is already a parent of the endpoint + * decoder's port, at least indirectly in the port hierarchy. + * Thus, the endpoint already holds a reference for the parent + * port of the switch decoder. Free the unnecessary reference + * here. + */ + struct device *cxld_dev __free(put_device) = + device_find_child(&port->dev, hpa, match_decoder_by_range); + + return cxld_dev ? to_cxl_decoder(cxld_dev) : NULL; +} + static struct cxl_root_decoder * cxl_find_root_decoder(struct cxl_endpoint_decoder *cxled) { struct cxl_memdev *cxlmd = cxled_to_memdev(cxled); struct cxl_port *port = cxled_to_port(cxled); struct cxl_root *cxl_root __free(put_cxl_root) = find_cxl_root(port); - struct cxl_decoder *cxld = &cxled->cxld; + struct cxl_decoder *root, *cxld = &cxled->cxld; struct range *hpa = &cxld->hpa_range; - struct device *cxlrd_dev; - cxlrd_dev = device_find_child(&cxl_root->port.dev, hpa, - match_root_decoder_by_range); - if (!cxlrd_dev) { + root = cxl_port_find_switch_decoder(&cxl_root->port, hpa); + if (!root) { dev_err(cxlmd->dev.parent, "%s:%s no CXL window for range %#llx:%#llx\n", dev_name(&cxlmd->dev), dev_name(&cxld->dev), @@ -3232,16 +3247,9 @@ cxl_find_root_decoder(struct cxl_endpoint_decoder *cxled) return NULL; } - /* - * device_find_child() created a reference to the root - * decoder. Since the root decoder exists as long as the root - * port exists and the endpoint already holds a reference to - * the root port, this additional reference is not needed. - * Free it here. - */ - put_device(cxlrd_dev); - return to_cxl_root_decoder(cxlrd_dev); + + return to_cxl_root_decoder(&root->dev); } static int match_region_by_range(struct device *dev, const void *data)