| Message ID | 20250107141015.3367194-11-rrichter@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | cxl: Add address translation support and enable AMD Zen5 platforms | expand |
On Tue, Jan 07, 2025 at 03:09:56PM +0100, 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> > --- > drivers/cxl/core/region.c | 43 +++++++++++++++++++++++---------------- > 1 file changed, 25 insertions(+), 18 deletions(-) ... snip ... > > - cxlrd_dev = device_find_child(&iter->dev, hpa, > - match_root_decoder_by_range); > - if (!cxlrd_dev) { > + cxld = cxl_port_find_switch_decoder(iter, hpa); > + if (!cxld) { Are there scenarios where this would return a different decoder than previously? For example, is there an assumption that root decoders will be search first, as opposed to intermediate decoders? The match function was changed to check is_switch_decoder from is_root_decoder, i'm just worried about the case where we might have multiple decoders in the path and the switch decoder is hit first - resulting in the wrong decoder returned. ~Gregory
On 1/7/25 8:09 AM, 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> > --- > drivers/cxl/core/region.c | 43 +++++++++++++++++++++++---------------- > 1 file changed, 25 insertions(+), 18 deletions(-) > > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > index 5750ed2796a8..48add814924b 100644 > --- a/drivers/cxl/core/region.c > +++ b/drivers/cxl/core/region.c > @@ -3189,19 +3189,35 @@ static int devm_cxl_add_dax_region(struct cxl_region *cxlr) > return rc; > } > > -static int match_root_decoder_by_range(struct device *dev, void *data) > +static int match_decoder_by_range(struct device *dev, void *data) > { > 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() creates 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. > + */ Is this comment still true? I haven't read the rest of the series yet, but there's nothing enforcing that this function is called on a root port. If it's meant to only be used for root ports then it should probably be named that way. Also, if it is meant to be used for a general switch decoder, can we always free the reference? If so then all that needs to happen is a comment update, otherwise you'll need to keep the reference and put a comment somewhere that the function needs a matching put_device(). > + 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) > { > @@ -3209,7 +3225,6 @@ cxl_find_root_decoder(struct cxl_endpoint_decoder *cxled) > struct cxl_port *iter = cxled_to_port(cxled); > struct range *hpa = &cxled->cxld.hpa_range; > struct cxl_decoder *cxld = &cxled->cxld; > - struct device *cxlrd_dev; > > while (iter && !is_cxl_root(iter)) > iter = to_cxl_port(iter->dev.parent); > @@ -3217,9 +3232,8 @@ cxl_find_root_decoder(struct cxl_endpoint_decoder *cxled) > if (!iter) > return NULL; > > - cxlrd_dev = device_find_child(&iter->dev, hpa, > - match_root_decoder_by_range); > - if (!cxlrd_dev) { > + cxld = cxl_port_find_switch_decoder(iter, hpa); > + if (!cxld) { > dev_err(cxlmd->dev.parent, > "%s:%s no CXL window for range %#llx:%#llx\n", > dev_name(&cxlmd->dev), dev_name(&cxld->dev), > @@ -3227,16 +3241,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(&cxld->dev); > } > > static int match_region_by_range(struct device *dev, void *data)
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c index 5750ed2796a8..48add814924b 100644 --- a/drivers/cxl/core/region.c +++ b/drivers/cxl/core/region.c @@ -3189,19 +3189,35 @@ static int devm_cxl_add_dax_region(struct cxl_region *cxlr) return rc; } -static int match_root_decoder_by_range(struct device *dev, void *data) +static int match_decoder_by_range(struct device *dev, void *data) { 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() creates 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. + */ + 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) { @@ -3209,7 +3225,6 @@ cxl_find_root_decoder(struct cxl_endpoint_decoder *cxled) struct cxl_port *iter = cxled_to_port(cxled); struct range *hpa = &cxled->cxld.hpa_range; struct cxl_decoder *cxld = &cxled->cxld; - struct device *cxlrd_dev; while (iter && !is_cxl_root(iter)) iter = to_cxl_port(iter->dev.parent); @@ -3217,9 +3232,8 @@ cxl_find_root_decoder(struct cxl_endpoint_decoder *cxled) if (!iter) return NULL; - cxlrd_dev = device_find_child(&iter->dev, hpa, - match_root_decoder_by_range); - if (!cxlrd_dev) { + cxld = cxl_port_find_switch_decoder(iter, hpa); + if (!cxld) { dev_err(cxlmd->dev.parent, "%s:%s no CXL window for range %#llx:%#llx\n", dev_name(&cxlmd->dev), dev_name(&cxld->dev), @@ -3227,16 +3241,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(&cxld->dev); } static int match_region_by_range(struct device *dev, void *data)
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> --- drivers/cxl/core/region.c | 43 +++++++++++++++++++++++---------------- 1 file changed, 25 insertions(+), 18 deletions(-)