| 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)
On Tue, Jan 07, 2025 at 01:38:33PM -0500, Gregory Price wrote: > 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. Intermediate decoders never share the port with a root decoder, both decoders always have different parents. So depending on the direction of walking the tree, the same port is always found first. That is, starting at the endpoint, an intermediate switch decoder would be found first. Search is always deterministic. Note the "root_decoder" is a subset of "switch_decoder" here. -Robert > > ~Gregory
On Fri, Jan 17, 2025 at 03:31:34PM -0600, Ben Cheatham wrote: > 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(). In general, the assumption is true and all ports in the hierarchy exist as long as the endpoint exists. The reference can be freed. I have updated the comment: /* * 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. */ Thanks for catching this. -Robert
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(-)