Current function cxl_region_find_decoder() is used to find a port's
decoder during region setup. In the region creation path the function
is an allocator to find a free port. In the region assembly path, it
is recalling the decoder that platform firmware picked for validation
purposes.
Rename function to cxl_port_pick_region_decoder() that better
describes its use and update the function's description.
The result of cxl_port_pick_region_decoder() is recorded in a 'struct
cxl_region_ref' in @port for later recall when other endpoints might
also be targets of the picked decoder.
Signed-off-by: Robert Richter <rrichter@amd.com>
Reviewed-by: Gregory Price <gourry@gourry.net>
Tested-by: Gregory Price <gourry@gourry.net>
---
drivers/cxl/core/region.c | 25 ++++++++++++++++++++-----
1 file changed, 20 insertions(+), 5 deletions(-)
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index e35209168c9c..e104035e0855 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -865,10 +865,25 @@ static int match_auto_decoder(struct device *dev, const void *data)
return 0;
}
+/**
+ * cxl_port_pick_region_decoder() - assign or lookup a decoder for a region
+ * @port: a port in the ancestry of the endpoint implied by @cxled
+ * @cxled: endpoint decoder to be, or currently, mapped by @port
+ * @cxlr: region to establish, or validate, decode @port
+ *
+ * In the region creation path cxl_port_pick_region_decoder() is an
+ * allocator to find a free port. In the region assembly path, it is
+ * recalling the decoder that platform firmware picked for validation
+ * purposes.
+ *
+ * The result is recorded in a 'struct cxl_region_ref' in @port for
+ * later recall when other endpoints might also be targets of the picked
+ * decoder.
+ */
static struct cxl_decoder *
-cxl_region_find_decoder(struct cxl_port *port,
- struct cxl_endpoint_decoder *cxled,
- struct cxl_region *cxlr)
+cxl_port_pick_region_decoder(struct cxl_port *port,
+ struct cxl_endpoint_decoder *cxled,
+ struct cxl_region *cxlr)
{
struct device *dev;
@@ -932,7 +947,7 @@ alloc_region_ref(struct cxl_port *port, struct cxl_region *cxlr,
if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags)) {
struct cxl_decoder *cxld;
- cxld = cxl_region_find_decoder(port, cxled, cxlr);
+ cxld = cxl_port_pick_region_decoder(port, cxled, cxlr);
if (auto_order_ok(port, iter->region, cxld))
continue;
}
@@ -1020,7 +1035,7 @@ static int cxl_rr_alloc_decoder(struct cxl_port *port, struct cxl_region *cxlr,
{
struct cxl_decoder *cxld;
- cxld = cxl_region_find_decoder(port, cxled, cxlr);
+ cxld = cxl_port_pick_region_decoder(port, cxled, cxlr);
if (!cxld) {
dev_dbg(&cxlr->dev, "%s: no decoder available\n",
dev_name(&port->dev));
--
2.39.5On Mon, 28 Apr 2025 23:43:08 +0200
Robert Richter <rrichter@amd.com> wrote:
> Current function cxl_region_find_decoder() is used to find a port's
> decoder during region setup. In the region creation path the function
> is an allocator to find a free port. In the region assembly path, it
> is recalling the decoder that platform firmware picked for validation
> purposes.
>
> Rename function to cxl_port_pick_region_decoder() that better
> describes its use and update the function's description.
>
> The result of cxl_port_pick_region_decoder() is recorded in a 'struct
> cxl_region_ref' in @port for later recall when other endpoints might
> also be targets of the picked decoder.
I'm not convinced pick is really any clearer than find as it doesn't to me
imply 'get the one that was already allocated'. I'm also not seeing
a lot of precedence in kernel for this use of pick.
I don't feel that strongly either way though and I guess I'll
get used to the term if we go with pick.
Alternative might just be to make it an or...
cxl_region_find_or_alloc_decoder()
>
> Signed-off-by: Robert Richter <rrichter@amd.com>
> Reviewed-by: Gregory Price <gourry@gourry.net>
> Tested-by: Gregory Price <gourry@gourry.net>
> ---
> drivers/cxl/core/region.c | 25 ++++++++++++++++++++-----
> 1 file changed, 20 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index e35209168c9c..e104035e0855 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -865,10 +865,25 @@ static int match_auto_decoder(struct device *dev, const void *data)
> return 0;
> }
>
> +/**
> + * cxl_port_pick_region_decoder() - assign or lookup a decoder for a region
> + * @port: a port in the ancestry of the endpoint implied by @cxled
> + * @cxled: endpoint decoder to be, or currently, mapped by @port
> + * @cxlr: region to establish, or validate, decode @port
> + *
> + * In the region creation path cxl_port_pick_region_decoder() is an
> + * allocator to find a free port. In the region assembly path, it is
> + * recalling the decoder that platform firmware picked for validation
> + * purposes.
> + *
> + * The result is recorded in a 'struct cxl_region_ref' in @port for
> + * later recall when other endpoints might also be targets of the picked
> + * decoder.
> + */
> static struct cxl_decoder *
> -cxl_region_find_decoder(struct cxl_port *port,
> - struct cxl_endpoint_decoder *cxled,
> - struct cxl_region *cxlr)
> +cxl_port_pick_region_decoder(struct cxl_port *port,
> + struct cxl_endpoint_decoder *cxled,
> + struct cxl_region *cxlr)
> {
> struct device *dev;
>
> @@ -932,7 +947,7 @@ alloc_region_ref(struct cxl_port *port, struct cxl_region *cxlr,
> if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags)) {
> struct cxl_decoder *cxld;
>
> - cxld = cxl_region_find_decoder(port, cxled, cxlr);
> + cxld = cxl_port_pick_region_decoder(port, cxled, cxlr);
> if (auto_order_ok(port, iter->region, cxld))
> continue;
> }
> @@ -1020,7 +1035,7 @@ static int cxl_rr_alloc_decoder(struct cxl_port *port, struct cxl_region *cxlr,
> {
> struct cxl_decoder *cxld;
>
> - cxld = cxl_region_find_decoder(port, cxled, cxlr);
> + cxld = cxl_port_pick_region_decoder(port, cxled, cxlr);
> if (!cxld) {
> dev_dbg(&cxlr->dev, "%s: no decoder available\n",
> dev_name(&port->dev));
On Tue, 29 Apr 2025 16:31:19 +0100
Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:
> On Mon, 28 Apr 2025 23:43:08 +0200
> Robert Richter <rrichter@amd.com> wrote:
>
> > Current function cxl_region_find_decoder() is used to find a port's
> > decoder during region setup. In the region creation path the function
> > is an allocator to find a free port. In the region assembly path, it
> > is recalling the decoder that platform firmware picked for validation
> > purposes.
> >
> > Rename function to cxl_port_pick_region_decoder() that better
> > describes its use and update the function's description.
> >
> > The result of cxl_port_pick_region_decoder() is recorded in a 'struct
> > cxl_region_ref' in @port for later recall when other endpoints might
> > also be targets of the picked decoder.
>
> I'm not convinced pick is really any clearer than find as it doesn't to me
> imply 'get the one that was already allocated'. I'm also not seeing
> a lot of precedence in kernel for this use of pick.
>
> I don't feel that strongly either way though and I guess I'll
> get used to the term if we go with pick.
>
> Alternative might just be to make it an or...
>
> cxl_region_find_or_alloc_decoder()
>
Just taking a look at where this series stands and feel a clarification
is needed.
Note I don't care enough on this to block the series going forwards!
>
> >
> > Signed-off-by: Robert Richter <rrichter@amd.com>
> > Reviewed-by: Gregory Price <gourry@gourry.net>
> > Tested-by: Gregory Price <gourry@gourry.net>
> > ---
> > drivers/cxl/core/region.c | 25 ++++++++++++++++++++-----
> > 1 file changed, 20 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > index e35209168c9c..e104035e0855 100644
> > --- a/drivers/cxl/core/region.c
> > +++ b/drivers/cxl/core/region.c
> > @@ -865,10 +865,25 @@ static int match_auto_decoder(struct device *dev, const void *data)
> > return 0;
> > }
> >
> > +/**
> > + * cxl_port_pick_region_decoder() - assign or lookup a decoder for a region
> > + * @port: a port in the ancestry of the endpoint implied by @cxled
> > + * @cxled: endpoint decoder to be, or currently, mapped by @port
> > + * @cxlr: region to establish, or validate, decode @port
> > + *
> > + * In the region creation path cxl_port_pick_region_decoder() is an
> > + * allocator to find a free port. In the region assembly path, it is
> > + * recalling the decoder that platform firmware picked for validation
> > + * purposes.
> > + *
> > + * The result is recorded in a 'struct cxl_region_ref' in @port for
> > + * later recall when other endpoints might also be targets of the picked
> > + * decoder.
> > + */
> > static struct cxl_decoder *
> > -cxl_region_find_decoder(struct cxl_port *port,
> > - struct cxl_endpoint_decoder *cxled,
> > - struct cxl_region *cxlr)
> > +cxl_port_pick_region_decoder(struct cxl_port *port,
> > + struct cxl_endpoint_decoder *cxled,
> > + struct cxl_region *cxlr)
> > {
> > struct device *dev;
> >
> > @@ -932,7 +947,7 @@ alloc_region_ref(struct cxl_port *port, struct cxl_region *cxlr,
> > if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags)) {
> > struct cxl_decoder *cxld;
> >
> > - cxld = cxl_region_find_decoder(port, cxled, cxlr);
> > + cxld = cxl_port_pick_region_decoder(port, cxled, cxlr);
> > if (auto_order_ok(port, iter->region, cxld))
> > continue;
> > }
> > @@ -1020,7 +1035,7 @@ static int cxl_rr_alloc_decoder(struct cxl_port *port, struct cxl_region *cxlr,
> > {
> > struct cxl_decoder *cxld;
> >
> > - cxld = cxl_region_find_decoder(port, cxled, cxlr);
> > + cxld = cxl_port_pick_region_decoder(port, cxled, cxlr);
> > if (!cxld) {
> > dev_dbg(&cxlr->dev, "%s: no decoder available\n",
> > dev_name(&port->dev));
>
>
On 4/28/25 2:43 PM, Robert Richter wrote:
> Current function cxl_region_find_decoder() is used to find a port's
> decoder during region setup. In the region creation path the function
> is an allocator to find a free port. In the region assembly path, it
> is recalling the decoder that platform firmware picked for validation
> purposes.
>
> Rename function to cxl_port_pick_region_decoder() that better
> describes its use and update the function's description.
>
> The result of cxl_port_pick_region_decoder() is recorded in a 'struct
> cxl_region_ref' in @port for later recall when other endpoints might
> also be targets of the picked decoder.
>
> Signed-off-by: Robert Richter <rrichter@amd.com>
> Reviewed-by: Gregory Price <gourry@gourry.net>
> Tested-by: Gregory Price <gourry@gourry.net>
Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> ---
> drivers/cxl/core/region.c | 25 ++++++++++++++++++++-----
> 1 file changed, 20 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index e35209168c9c..e104035e0855 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -865,10 +865,25 @@ static int match_auto_decoder(struct device *dev, const void *data)
> return 0;
> }
>
> +/**
> + * cxl_port_pick_region_decoder() - assign or lookup a decoder for a region
> + * @port: a port in the ancestry of the endpoint implied by @cxled
> + * @cxled: endpoint decoder to be, or currently, mapped by @port
> + * @cxlr: region to establish, or validate, decode @port
> + *
> + * In the region creation path cxl_port_pick_region_decoder() is an
> + * allocator to find a free port. In the region assembly path, it is
> + * recalling the decoder that platform firmware picked for validation
> + * purposes.
> + *
> + * The result is recorded in a 'struct cxl_region_ref' in @port for
> + * later recall when other endpoints might also be targets of the picked
> + * decoder.
> + */
> static struct cxl_decoder *
> -cxl_region_find_decoder(struct cxl_port *port,
> - struct cxl_endpoint_decoder *cxled,
> - struct cxl_region *cxlr)
> +cxl_port_pick_region_decoder(struct cxl_port *port,
> + struct cxl_endpoint_decoder *cxled,
> + struct cxl_region *cxlr)
> {
> struct device *dev;
>
> @@ -932,7 +947,7 @@ alloc_region_ref(struct cxl_port *port, struct cxl_region *cxlr,
> if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags)) {
> struct cxl_decoder *cxld;
>
> - cxld = cxl_region_find_decoder(port, cxled, cxlr);
> + cxld = cxl_port_pick_region_decoder(port, cxled, cxlr);
> if (auto_order_ok(port, iter->region, cxld))
> continue;
> }
> @@ -1020,7 +1035,7 @@ static int cxl_rr_alloc_decoder(struct cxl_port *port, struct cxl_region *cxlr,
> {
> struct cxl_decoder *cxld;
>
> - cxld = cxl_region_find_decoder(port, cxled, cxlr);
> + cxld = cxl_port_pick_region_decoder(port, cxled, cxlr);
> if (!cxld) {
> dev_dbg(&cxlr->dev, "%s: no decoder available\n",
> dev_name(&port->dev));
On Monday, April 28, 2025 11:43:08 PM Central European Summer Time Robert Richter wrote:
> Current function cxl_region_find_decoder() is used to find a port's
> decoder during region setup. In the region creation path the function
> is an allocator to find a free port. In the region assembly path, it
> is recalling the decoder that platform firmware picked for validation
> purposes.
>
> Rename function to cxl_port_pick_region_decoder() that better
> describes its use and update the function's description.
>
> The result of cxl_port_pick_region_decoder() is recorded in a 'struct
> cxl_region_ref' in @port for later recall when other endpoints might
> also be targets of the picked decoder.
>
> Signed-off-by: Robert Richter <rrichter@amd.com>
> Reviewed-by: Gregory Price <gourry@gourry.net>
> Tested-by: Gregory Price <gourry@gourry.net>
> ---
> drivers/cxl/core/region.c | 25 ++++++++++++++++++++-----
> 1 file changed, 20 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index e35209168c9c..e104035e0855 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -865,10 +865,25 @@ static int match_auto_decoder(struct device *dev, const void *data)
> return 0;
> }
>
> +/**
> + * cxl_port_pick_region_decoder() - assign or lookup a decoder for a region
> + * @port: a port in the ancestry of the endpoint implied by @cxled
> + * @cxled: endpoint decoder to be, or currently, mapped by @port
> + * @cxlr: region to establish, or validate, decode @port
> + *
> + * In the region creation path cxl_port_pick_region_decoder() is an
> + * allocator to find a free port. In the region assembly path, it is
> + * recalling the decoder that platform firmware picked for validation
> + * purposes.
> + *
> + * The result is recorded in a 'struct cxl_region_ref' in @port for
> + * later recall when other endpoints might also be targets of the picked
> + * decoder.
I wouldn't write here about what the callers do with the value returned by
this function. Maybe that this last paragraph belongs one layer up to the
calling sites?
Other than that...
Reviewed-by: Fabio M. De Francesco <fabio.m.de.francesco@linux.intel.com>
> + */
> static struct cxl_decoder *
> -cxl_region_find_decoder(struct cxl_port *port,
> - struct cxl_endpoint_decoder *cxled,
> - struct cxl_region *cxlr)
> +cxl_port_pick_region_decoder(struct cxl_port *port,
> + struct cxl_endpoint_decoder *cxled,
> + struct cxl_region *cxlr)
> {
> struct device *dev;
>
> @@ -932,7 +947,7 @@ alloc_region_ref(struct cxl_port *port, struct cxl_region *cxlr,
> if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags)) {
> struct cxl_decoder *cxld;
>
> - cxld = cxl_region_find_decoder(port, cxled, cxlr);
> + cxld = cxl_port_pick_region_decoder(port, cxled, cxlr);
> if (auto_order_ok(port, iter->region, cxld))
> continue;
> }
> @@ -1020,7 +1035,7 @@ static int cxl_rr_alloc_decoder(struct cxl_port *port, struct cxl_region *cxlr,
> {
> struct cxl_decoder *cxld;
>
> - cxld = cxl_region_find_decoder(port, cxled, cxlr);
> + cxld = cxl_port_pick_region_decoder(port, cxled, cxlr);
> if (!cxld) {
> dev_dbg(&cxlr->dev, "%s: no decoder available\n",
> dev_name(&port->dev));
>
© 2016 - 2026 Red Hat, Inc.