* [PATCH 1/9] cxl/port: Cleanup handling of the nr_dports 0 -> 1 transition
2026-01-22 3:33 [PATCH 0/9] cxl/port: Unify RAS setup across port types Dan Williams
@ 2026-01-22 3:33 ` Dan Williams
2026-01-22 11:32 ` Jonathan Cameron
2026-01-22 16:45 ` Dave Jiang
2026-01-22 3:33 ` [PATCH 2/9] cxl/port: Reduce number of @dport variables in cxl_port_add_dport() Dan Williams
` (8 subsequent siblings)
9 siblings, 2 replies; 36+ messages in thread
From: Dan Williams @ 2026-01-22 3:33 UTC (permalink / raw)
To: linux-cxl
Cc: jonathan.cameron, dave, dave.jiang, alison.schofield, ira.weiny,
terry.bowman
There are multiple setup actions that can occur for a switch port after it
is known that it has at least one active downstream link. That work is
currently split between __devm_cxl_add_dport(), the add_dport() helper, and
cxl_port_add_dport() where decoder setup occurs.
Clean this up by moving all @dport object setup responsibilities into
add_dport() and all port effects into cxl_port_add_dport().
add_dport() handles taking a reference on @dport->dport_dev, and
cxl_port_add_dport() grows the awareness to setup the port component
registers. This removes an awkward open-coded xa_erase() from the middle of
__devm_cxl_add_dport() and instead tasks cxl_port_add_dport() with calling
the common @dport destruction path if anything goes wrong.
After this @port->nr_dports is always the count of @dports in the
@port->dports xarray, and cxl_dport_remove() is symmetric with add_dport().
With ->nr_dports now reliably tracking the number of dports the use of
ida_is_empty() can be dropped. Recall that the ida is only cleared on
"release" of decoder objects, and release can be arbitrarily delayed past
unregistration.
Lastly port->component_reg_phys is no longer reset to CXL_RESOURCE_NONE
post setup, no reason is seen to carry that forward.
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
drivers/cxl/core/port.c | 31 +++++++++++++++----------------
1 file changed, 15 insertions(+), 16 deletions(-)
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index fef3aa0c6680..ff899c690d85 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -1066,11 +1066,15 @@ static int add_dport(struct cxl_port *port, struct cxl_dport *dport)
return -EBUSY;
}
+ /* Arrange for dport_dev to be valid through remove_dport() */
+ struct device *dev __free(put_device) = get_device(dport->dport_dev);
+
rc = xa_insert(&port->dports, (unsigned long)dport->dport_dev, dport,
GFP_KERNEL);
if (rc)
return rc;
+ retain_and_null_ptr(dev);
port->nr_dports++;
return 0;
}
@@ -1099,6 +1103,7 @@ static void cxl_dport_remove(void *data)
struct cxl_dport *dport = data;
struct cxl_port *port = dport->port;
+ port->nr_dports--;
xa_erase(&port->dports, (unsigned long) dport->dport_dev);
put_device(dport->dport_dev);
}
@@ -1181,21 +1186,6 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
if (rc)
return ERR_PTR(rc);
- /*
- * Setup port register if this is the first dport showed up. Having
- * a dport also means that there is at least 1 active link.
- */
- if (port->nr_dports == 1 &&
- port->component_reg_phys != CXL_RESOURCE_NONE) {
- rc = cxl_port_setup_regs(port, port->component_reg_phys);
- if (rc) {
- xa_erase(&port->dports, (unsigned long)dport->dport_dev);
- return ERR_PTR(rc);
- }
- port->component_reg_phys = CXL_RESOURCE_NONE;
- }
-
- get_device(dport_dev);
rc = devm_add_action_or_reset(host, cxl_dport_remove, dport);
if (rc)
return ERR_PTR(rc);
@@ -1622,7 +1612,16 @@ static struct cxl_dport *cxl_port_add_dport(struct cxl_port *port,
cxl_switch_parse_cdat(new_dport);
- if (ida_is_empty(&port->decoder_ida)) {
+ if (port->nr_dports == 1) {
+ /*
+ * Some host bridges are known to not have component regsisters
+ * available until a root port has trained CXL. Perform that
+ * setup now.
+ */
+ rc = cxl_port_setup_regs(port, port->component_reg_phys);
+ if (rc)
+ return ERR_PTR(rc);
+
rc = devm_cxl_switch_port_decoders_setup(port);
if (rc)
return ERR_PTR(rc);
--
2.52.0
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH 1/9] cxl/port: Cleanup handling of the nr_dports 0 -> 1 transition
2026-01-22 3:33 ` [PATCH 1/9] cxl/port: Cleanup handling of the nr_dports 0 -> 1 transition Dan Williams
@ 2026-01-22 11:32 ` Jonathan Cameron
2026-01-22 19:58 ` dan.j.williams
2026-01-22 16:45 ` Dave Jiang
1 sibling, 1 reply; 36+ messages in thread
From: Jonathan Cameron @ 2026-01-22 11:32 UTC (permalink / raw)
To: Dan Williams
Cc: linux-cxl, dave, dave.jiang, alison.schofield, ira.weiny,
terry.bowman
On Wed, 21 Jan 2026 19:33:22 -0800
Dan Williams <dan.j.williams@intel.com> wrote:
> There are multiple setup actions that can occur for a switch port after it
> is known that it has at least one active downstream link. That work is
> currently split between __devm_cxl_add_dport(), the add_dport() helper, and
> cxl_port_add_dport() where decoder setup occurs.
>
> Clean this up by moving all @dport object setup responsibilities into
> add_dport() and all port effects into cxl_port_add_dport().
>
> add_dport() handles taking a reference on @dport->dport_dev, and
> cxl_port_add_dport() grows the awareness to setup the port component
> registers. This removes an awkward open-coded xa_erase() from the middle of
> __devm_cxl_add_dport() and instead tasks cxl_port_add_dport() with calling
> the common @dport destruction path if anything goes wrong.
>
> After this @port->nr_dports is always the count of @dports in the
> @port->dports xarray, and cxl_dport_remove() is symmetric with add_dport().
> With ->nr_dports now reliably tracking the number of dports the use of
> ida_is_empty() can be dropped. Recall that the ida is only cleared on
> "release" of decoder objects, and release can be arbitrarily delayed past
> unregistration.
Given we only care about nr_dports going to and and from 0, why do
we need nr_dports at all? Why not use xa_empty() as the condition.
Does this now guarantee we don't need the extra nr_dports reset in
cxl_switch_port_probe()? That also made me suspicious of something
funny going on as the count should have dropped to zero if we
are in state to rebind.
Other than those this looks good to me and both of those are perhaps
topics for other patches even if maybe they could be done in here.
Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 1/9] cxl/port: Cleanup handling of the nr_dports 0 -> 1 transition
2026-01-22 11:32 ` Jonathan Cameron
@ 2026-01-22 19:58 ` dan.j.williams
0 siblings, 0 replies; 36+ messages in thread
From: dan.j.williams @ 2026-01-22 19:58 UTC (permalink / raw)
To: Jonathan Cameron, Dan Williams
Cc: linux-cxl, dave, dave.jiang, alison.schofield, ira.weiny,
terry.bowman
Jonathan Cameron wrote:
> On Wed, 21 Jan 2026 19:33:22 -0800
> Dan Williams <dan.j.williams@intel.com> wrote:
>
> > There are multiple setup actions that can occur for a switch port after it
> > is known that it has at least one active downstream link. That work is
> > currently split between __devm_cxl_add_dport(), the add_dport() helper, and
> > cxl_port_add_dport() where decoder setup occurs.
> >
> > Clean this up by moving all @dport object setup responsibilities into
> > add_dport() and all port effects into cxl_port_add_dport().
> >
> > add_dport() handles taking a reference on @dport->dport_dev, and
> > cxl_port_add_dport() grows the awareness to setup the port component
> > registers. This removes an awkward open-coded xa_erase() from the middle of
> > __devm_cxl_add_dport() and instead tasks cxl_port_add_dport() with calling
> > the common @dport destruction path if anything goes wrong.
> >
> > After this @port->nr_dports is always the count of @dports in the
> > @port->dports xarray, and cxl_dport_remove() is symmetric with add_dport().
> > With ->nr_dports now reliably tracking the number of dports the use of
> > ida_is_empty() can be dropped. Recall that the ida is only cleared on
> > "release" of decoder objects, and release can be arbitrarily delayed past
> > unregistration.
>
> Given we only care about nr_dports going to and and from 0, why do
> we need nr_dports at all? Why not use xa_empty() as the condition.
>
> Does this now guarantee we don't need the extra nr_dports reset in
> cxl_switch_port_probe()? That also made me suspicious of something
> funny going on as the count should have dropped to zero if we
> are in state to rebind.
>
> Other than those this looks good to me and both of those are perhaps
> topics for other patches even if maybe they could be done in here.
True. I had just been assuming that nr_dports was still needed for other
reasons. It turns out that the original need for nr_dports:
e4f6dfa9ef75 cxl/region: Fix 'distance' calculation with passthrough ports
...was obviated here:
711442e29f16 cxl/region: Fix passthrough-decoder detection
So, yes, I would be on board with switching to xa_empty() as a follow-on
cleanup.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 1/9] cxl/port: Cleanup handling of the nr_dports 0 -> 1 transition
2026-01-22 3:33 ` [PATCH 1/9] cxl/port: Cleanup handling of the nr_dports 0 -> 1 transition Dan Williams
2026-01-22 11:32 ` Jonathan Cameron
@ 2026-01-22 16:45 ` Dave Jiang
1 sibling, 0 replies; 36+ messages in thread
From: Dave Jiang @ 2026-01-22 16:45 UTC (permalink / raw)
To: Dan Williams, linux-cxl
Cc: jonathan.cameron, dave, alison.schofield, ira.weiny, terry.bowman
On 1/21/26 8:33 PM, Dan Williams wrote:
> There are multiple setup actions that can occur for a switch port after it
> is known that it has at least one active downstream link. That work is
> currently split between __devm_cxl_add_dport(), the add_dport() helper, and
> cxl_port_add_dport() where decoder setup occurs.
>
> Clean this up by moving all @dport object setup responsibilities into
> add_dport() and all port effects into cxl_port_add_dport().
>
> add_dport() handles taking a reference on @dport->dport_dev, and
> cxl_port_add_dport() grows the awareness to setup the port component
> registers. This removes an awkward open-coded xa_erase() from the middle of
> __devm_cxl_add_dport() and instead tasks cxl_port_add_dport() with calling
> the common @dport destruction path if anything goes wrong.
>
> After this @port->nr_dports is always the count of @dports in the
> @port->dports xarray, and cxl_dport_remove() is symmetric with add_dport().
> With ->nr_dports now reliably tracking the number of dports the use of
> ida_is_empty() can be dropped. Recall that the ida is only cleared on
> "release" of decoder objects, and release can be arbitrarily delayed past
> unregistration.
>
> Lastly port->component_reg_phys is no longer reset to CXL_RESOURCE_NONE
> post setup, no reason is seen to carry that forward.
>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> ---
> drivers/cxl/core/port.c | 31 +++++++++++++++----------------
> 1 file changed, 15 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index fef3aa0c6680..ff899c690d85 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -1066,11 +1066,15 @@ static int add_dport(struct cxl_port *port, struct cxl_dport *dport)
> return -EBUSY;
> }
>
> + /* Arrange for dport_dev to be valid through remove_dport() */
> + struct device *dev __free(put_device) = get_device(dport->dport_dev);
> +
> rc = xa_insert(&port->dports, (unsigned long)dport->dport_dev, dport,
> GFP_KERNEL);
> if (rc)
> return rc;
>
> + retain_and_null_ptr(dev);
> port->nr_dports++;
> return 0;
> }
> @@ -1099,6 +1103,7 @@ static void cxl_dport_remove(void *data)
> struct cxl_dport *dport = data;
> struct cxl_port *port = dport->port;
>
> + port->nr_dports--;
> xa_erase(&port->dports, (unsigned long) dport->dport_dev);
> put_device(dport->dport_dev);
> }
> @@ -1181,21 +1186,6 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
> if (rc)
> return ERR_PTR(rc);
>
> - /*
> - * Setup port register if this is the first dport showed up. Having
> - * a dport also means that there is at least 1 active link.
> - */
> - if (port->nr_dports == 1 &&
> - port->component_reg_phys != CXL_RESOURCE_NONE) {
> - rc = cxl_port_setup_regs(port, port->component_reg_phys);
> - if (rc) {
> - xa_erase(&port->dports, (unsigned long)dport->dport_dev);
> - return ERR_PTR(rc);
> - }
> - port->component_reg_phys = CXL_RESOURCE_NONE;
> - }
> -
> - get_device(dport_dev);
> rc = devm_add_action_or_reset(host, cxl_dport_remove, dport);
> if (rc)
> return ERR_PTR(rc);
> @@ -1622,7 +1612,16 @@ static struct cxl_dport *cxl_port_add_dport(struct cxl_port *port,
>
> cxl_switch_parse_cdat(new_dport);
>
> - if (ida_is_empty(&port->decoder_ida)) {
> + if (port->nr_dports == 1) {
> + /*
> + * Some host bridges are known to not have component regsisters
> + * available until a root port has trained CXL. Perform that
> + * setup now.
> + */
> + rc = cxl_port_setup_regs(port, port->component_reg_phys);
> + if (rc)
> + return ERR_PTR(rc);
> +
> rc = devm_cxl_switch_port_decoders_setup(port);
> if (rc)
> return ERR_PTR(rc);
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 2/9] cxl/port: Reduce number of @dport variables in cxl_port_add_dport()
2026-01-22 3:33 [PATCH 0/9] cxl/port: Unify RAS setup across port types Dan Williams
2026-01-22 3:33 ` [PATCH 1/9] cxl/port: Cleanup handling of the nr_dports 0 -> 1 transition Dan Williams
@ 2026-01-22 3:33 ` Dan Williams
2026-01-22 11:39 ` Jonathan Cameron
2026-01-22 16:54 ` Dave Jiang
2026-01-22 3:33 ` [PATCH 3/9] cxl/port: Cleanup dport removal with a devres group Dan Williams
` (7 subsequent siblings)
9 siblings, 2 replies; 36+ messages in thread
From: Dan Williams @ 2026-01-22 3:33 UTC (permalink / raw)
To: linux-cxl
Cc: jonathan.cameron, dave, dave.jiang, alison.schofield, ira.weiny,
terry.bowman
In preparation for refactoring cxl_port_add_dport() to add RAS register
setup, cleanup the number of dport variables with a dport_exists() helper.
Kill the @dport needed to check for duplicates, rename @new_dport to
@dport.
Reported-by: Jonathan Cameron <jonathan.cameron@huawei.com>
Closes: http://lore.kernel.org/20260116150119.00003bbd@huawei.com
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
drivers/cxl/core/port.c | 43 +++++++++++++++++++++++++----------------
1 file changed, 26 insertions(+), 17 deletions(-)
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index ff899c690d85..1637e97f6805 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -1587,30 +1587,39 @@ static int update_decoder_targets(struct device *dev, void *data)
return 0;
}
+static bool dport_exists(struct cxl_port *port, struct device *dport_dev)
+{
+ struct cxl_dport *dport = cxl_find_dport_by_dev(port, dport_dev);
+
+ if (dport) {
+ dev_dbg(&port->dev, "dport%d:%s already exists\n",
+ dport->port_id, dev_name(dport_dev));
+ return true;
+ }
+
+ return false;
+}
+
DEFINE_FREE(del_cxl_dport, struct cxl_dport *, if (!IS_ERR_OR_NULL(_T)) del_dport(_T))
static struct cxl_dport *cxl_port_add_dport(struct cxl_port *port,
struct device *dport_dev)
{
- struct cxl_dport *dport;
int rc;
device_lock_assert(&port->dev);
- if (!port->dev.driver)
- return ERR_PTR(-ENXIO);
- dport = cxl_find_dport_by_dev(port, dport_dev);
- if (dport) {
- dev_dbg(&port->dev, "dport%d:%s already exists\n",
- dport->port_id, dev_name(dport_dev));
+ if (dport_exists(port, dport_dev))
return ERR_PTR(-EBUSY);
- }
- struct cxl_dport *new_dport __free(del_cxl_dport) =
+ if (!port->dev.driver)
+ return ERR_PTR(-ENXIO);
+
+ struct cxl_dport *dport __free(del_cxl_dport) =
devm_cxl_add_dport_by_dev(port, dport_dev);
- if (IS_ERR(new_dport))
- return new_dport;
+ if (IS_ERR(dport))
+ return dport;
- cxl_switch_parse_cdat(new_dport);
+ cxl_switch_parse_cdat(dport);
if (port->nr_dports == 1) {
/*
@@ -1626,17 +1635,17 @@ static struct cxl_dport *cxl_port_add_dport(struct cxl_port *port,
if (rc)
return ERR_PTR(rc);
dev_dbg(&port->dev, "first dport%d:%s added with decoders\n",
- new_dport->port_id, dev_name(dport_dev));
- return no_free_ptr(new_dport);
+ dport->port_id, dev_name(dport_dev));
+ return no_free_ptr(dport);
}
/* New dport added, update the decoder targets */
- device_for_each_child(&port->dev, new_dport, update_decoder_targets);
+ device_for_each_child(&port->dev, dport, update_decoder_targets);
- dev_dbg(&port->dev, "dport%d:%s added\n", new_dport->port_id,
+ dev_dbg(&port->dev, "dport%d:%s added\n", dport->port_id,
dev_name(dport_dev));
- return no_free_ptr(new_dport);
+ return no_free_ptr(dport);
}
static struct cxl_dport *devm_cxl_create_port(struct device *ep_dev,
--
2.52.0
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH 2/9] cxl/port: Reduce number of @dport variables in cxl_port_add_dport()
2026-01-22 3:33 ` [PATCH 2/9] cxl/port: Reduce number of @dport variables in cxl_port_add_dport() Dan Williams
@ 2026-01-22 11:39 ` Jonathan Cameron
2026-01-22 20:02 ` dan.j.williams
2026-01-22 16:54 ` Dave Jiang
1 sibling, 1 reply; 36+ messages in thread
From: Jonathan Cameron @ 2026-01-22 11:39 UTC (permalink / raw)
To: Dan Williams
Cc: linux-cxl, dave, dave.jiang, alison.schofield, ira.weiny,
terry.bowman
On Wed, 21 Jan 2026 19:33:23 -0800
Dan Williams <dan.j.williams@intel.com> wrote:
> In preparation for refactoring cxl_port_add_dport() to add RAS register
> setup, cleanup the number of dport variables with a dport_exists() helper.
Maybe mention why the driver check is now after the duplicate check.
If there isn't a reason for that, I'd put them back to reduce churn a tiny
bit.
>
> Kill the @dport needed to check for duplicates, rename @new_dport to
> @dport.
In ideal world I think the helper introduction and the rename
would be two patches. Still real world, this is fine.
Assuming you'll address the reorder of checks by comment or
putting them back,
Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
>
> Reported-by: Jonathan Cameron <jonathan.cameron@huawei.com>
> Closes: http://lore.kernel.org/20260116150119.00003bbd@huawei.com
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
> drivers/cxl/core/port.c | 43 +++++++++++++++++++++++++----------------
> 1 file changed, 26 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index ff899c690d85..1637e97f6805 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -1587,30 +1587,39 @@ static int update_decoder_targets(struct device *dev, void *data)
> return 0;
> }
>
> +static bool dport_exists(struct cxl_port *port, struct device *dport_dev)
> +{
> + struct cxl_dport *dport = cxl_find_dport_by_dev(port, dport_dev);
> +
> + if (dport) {
> + dev_dbg(&port->dev, "dport%d:%s already exists\n",
> + dport->port_id, dev_name(dport_dev));
> + return true;
> + }
> +
> + return false;
> +}
> +
> DEFINE_FREE(del_cxl_dport, struct cxl_dport *, if (!IS_ERR_OR_NULL(_T)) del_dport(_T))
> static struct cxl_dport *cxl_port_add_dport(struct cxl_port *port,
> struct device *dport_dev)
> {
> - struct cxl_dport *dport;
> int rc;
>
> device_lock_assert(&port->dev);
> - if (!port->dev.driver)
> - return ERR_PTR(-ENXIO);
Diff is a bit of a mess due to the checks (originally driver bind
then duplicate test) being swapped.
Is there a reason to do that?
>
> - dport = cxl_find_dport_by_dev(port, dport_dev);
> - if (dport) {
> - dev_dbg(&port->dev, "dport%d:%s already exists\n",
> - dport->port_id, dev_name(dport_dev));
> + if (dport_exists(port, dport_dev))
> return ERR_PTR(-EBUSY);
> - }
>
> - struct cxl_dport *new_dport __free(del_cxl_dport) =
> + if (!port->dev.driver)
> + return ERR_PTR(-ENXIO);
> +
> + struct cxl_dport *dport __free(del_cxl_dport) =
> devm_cxl_add_dport_by_dev(port, dport_dev);
> - if (IS_ERR(new_dport))
> - return new_dport;
> + if (IS_ERR(dport))
> + return dport;
>
> - cxl_switch_parse_cdat(new_dport);
> + cxl_switch_parse_cdat(dport);
>
> if (port->nr_dports == 1) {
> /*
> @@ -1626,17 +1635,17 @@ static struct cxl_dport *cxl_port_add_dport(struct cxl_port *port,
> if (rc)
> return ERR_PTR(rc);
> dev_dbg(&port->dev, "first dport%d:%s added with decoders\n",
> - new_dport->port_id, dev_name(dport_dev));
> - return no_free_ptr(new_dport);
> + dport->port_id, dev_name(dport_dev));
> + return no_free_ptr(dport);
> }
>
> /* New dport added, update the decoder targets */
> - device_for_each_child(&port->dev, new_dport, update_decoder_targets);
> + device_for_each_child(&port->dev, dport, update_decoder_targets);
>
> - dev_dbg(&port->dev, "dport%d:%s added\n", new_dport->port_id,
> + dev_dbg(&port->dev, "dport%d:%s added\n", dport->port_id,
> dev_name(dport_dev));
>
> - return no_free_ptr(new_dport);
> + return no_free_ptr(dport);
> }
>
> static struct cxl_dport *devm_cxl_create_port(struct device *ep_dev,
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH 2/9] cxl/port: Reduce number of @dport variables in cxl_port_add_dport()
2026-01-22 11:39 ` Jonathan Cameron
@ 2026-01-22 20:02 ` dan.j.williams
0 siblings, 0 replies; 36+ messages in thread
From: dan.j.williams @ 2026-01-22 20:02 UTC (permalink / raw)
To: Jonathan Cameron, Dan Williams
Cc: linux-cxl, dave, dave.jiang, alison.schofield, ira.weiny,
terry.bowman
Jonathan Cameron wrote:
> On Wed, 21 Jan 2026 19:33:23 -0800
> Dan Williams <dan.j.williams@intel.com> wrote:
>
> > In preparation for refactoring cxl_port_add_dport() to add RAS register
> > setup, cleanup the number of dport variables with a dport_exists() helper.
> Maybe mention why the driver check is now after the duplicate check.
> If there isn't a reason for that, I'd put them back to reduce churn a tiny
> bit.
>
> >
> > Kill the @dport needed to check for duplicates, rename @new_dport to
> > @dport.
> In ideal world I think the helper introduction and the rename
> would be two patches. Still real world, this is fine.
>
> Assuming you'll address the reorder of checks by comment or
> putting them back,
>
> Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
[..]
> > static struct cxl_dport *cxl_port_add_dport(struct cxl_port *port,
> > struct device *dport_dev)
> > {
> > - struct cxl_dport *dport;
> > int rc;
> >
> > device_lock_assert(&port->dev);
> > - if (!port->dev.driver)
> > - return ERR_PTR(-ENXIO);
>
> Diff is a bit of a mess due to the checks (originally driver bind
> then duplicate test) being swapped.
> Is there a reason to do that?
Nope, no reason. Flipped it back.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 2/9] cxl/port: Reduce number of @dport variables in cxl_port_add_dport()
2026-01-22 3:33 ` [PATCH 2/9] cxl/port: Reduce number of @dport variables in cxl_port_add_dport() Dan Williams
2026-01-22 11:39 ` Jonathan Cameron
@ 2026-01-22 16:54 ` Dave Jiang
1 sibling, 0 replies; 36+ messages in thread
From: Dave Jiang @ 2026-01-22 16:54 UTC (permalink / raw)
To: Dan Williams, linux-cxl
Cc: jonathan.cameron, dave, alison.schofield, ira.weiny, terry.bowman
On 1/21/26 8:33 PM, Dan Williams wrote:
> In preparation for refactoring cxl_port_add_dport() to add RAS register
> setup, cleanup the number of dport variables with a dport_exists() helper.
>
> Kill the @dport needed to check for duplicates, rename @new_dport to
> @dport.
>
> Reported-by: Jonathan Cameron <jonathan.cameron@huawei.com>
> Closes: http://lore.kernel.org/20260116150119.00003bbd@huawei.com
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> ---
> drivers/cxl/core/port.c | 43 +++++++++++++++++++++++++----------------
> 1 file changed, 26 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index ff899c690d85..1637e97f6805 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -1587,30 +1587,39 @@ static int update_decoder_targets(struct device *dev, void *data)
> return 0;
> }
>
> +static bool dport_exists(struct cxl_port *port, struct device *dport_dev)
> +{
> + struct cxl_dport *dport = cxl_find_dport_by_dev(port, dport_dev);
> +
> + if (dport) {
> + dev_dbg(&port->dev, "dport%d:%s already exists\n",
> + dport->port_id, dev_name(dport_dev));
> + return true;
> + }
> +
> + return false;
> +}
> +
> DEFINE_FREE(del_cxl_dport, struct cxl_dport *, if (!IS_ERR_OR_NULL(_T)) del_dport(_T))
> static struct cxl_dport *cxl_port_add_dport(struct cxl_port *port,
> struct device *dport_dev)
> {
> - struct cxl_dport *dport;
> int rc;
>
> device_lock_assert(&port->dev);
> - if (!port->dev.driver)
> - return ERR_PTR(-ENXIO);
>
> - dport = cxl_find_dport_by_dev(port, dport_dev);
> - if (dport) {
> - dev_dbg(&port->dev, "dport%d:%s already exists\n",
> - dport->port_id, dev_name(dport_dev));
> + if (dport_exists(port, dport_dev))
> return ERR_PTR(-EBUSY);
> - }
>
> - struct cxl_dport *new_dport __free(del_cxl_dport) =
> + if (!port->dev.driver)
> + return ERR_PTR(-ENXIO);
> +
> + struct cxl_dport *dport __free(del_cxl_dport) =
> devm_cxl_add_dport_by_dev(port, dport_dev);
> - if (IS_ERR(new_dport))
> - return new_dport;
> + if (IS_ERR(dport))
> + return dport;
>
> - cxl_switch_parse_cdat(new_dport);
> + cxl_switch_parse_cdat(dport);
>
> if (port->nr_dports == 1) {
> /*
> @@ -1626,17 +1635,17 @@ static struct cxl_dport *cxl_port_add_dport(struct cxl_port *port,
> if (rc)
> return ERR_PTR(rc);
> dev_dbg(&port->dev, "first dport%d:%s added with decoders\n",
> - new_dport->port_id, dev_name(dport_dev));
> - return no_free_ptr(new_dport);
> + dport->port_id, dev_name(dport_dev));
> + return no_free_ptr(dport);
> }
>
> /* New dport added, update the decoder targets */
> - device_for_each_child(&port->dev, new_dport, update_decoder_targets);
> + device_for_each_child(&port->dev, dport, update_decoder_targets);
>
> - dev_dbg(&port->dev, "dport%d:%s added\n", new_dport->port_id,
> + dev_dbg(&port->dev, "dport%d:%s added\n", dport->port_id,
> dev_name(dport_dev));
>
> - return no_free_ptr(new_dport);
> + return no_free_ptr(dport);
> }
>
> static struct cxl_dport *devm_cxl_create_port(struct device *ep_dev,
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 3/9] cxl/port: Cleanup dport removal with a devres group
2026-01-22 3:33 [PATCH 0/9] cxl/port: Unify RAS setup across port types Dan Williams
2026-01-22 3:33 ` [PATCH 1/9] cxl/port: Cleanup handling of the nr_dports 0 -> 1 transition Dan Williams
2026-01-22 3:33 ` [PATCH 2/9] cxl/port: Reduce number of @dport variables in cxl_port_add_dport() Dan Williams
@ 2026-01-22 3:33 ` Dan Williams
2026-01-22 11:59 ` Jonathan Cameron
2026-01-22 3:33 ` [PATCH 4/9] cxl/port: Move decoder setup before dport creation Dan Williams
` (6 subsequent siblings)
9 siblings, 1 reply; 36+ messages in thread
From: Dan Williams @ 2026-01-22 3:33 UTC (permalink / raw)
To: linux-cxl
Cc: jonathan.cameron, dave, dave.jiang, alison.schofield, ira.weiny,
terry.bowman
In preparation for adding more setup actions like RAS register mapping,
introduce a devres group to collect all the dport creation / registration
actions. This replaces the maintenance tedium of open coding several
devm_release_action() calls in del_dport().
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
drivers/cxl/core/port.c | 62 +++++++++++++++++++++++++++++++++++++----
1 file changed, 56 insertions(+), 6 deletions(-)
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index 1637e97f6805..f2723bf948e2 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -1118,6 +1118,51 @@ static void cxl_dport_unlink(void *data)
sysfs_remove_link(&port->dev.kobj, link_name);
}
+static struct device *dport_to_host(struct cxl_dport *dport)
+{
+ if (is_cxl_root(dport->port))
+ return dport->port->uport_dev;
+ return &dport->port->dev;
+}
+
+static void free_dport(void *dport)
+{
+ kfree(dport);
+}
+
+/*
+ * Upon return either a group is established with one action (free_dport()), or
+ * no group established and @dport is freed.
+ */
+static void *cxl_dport_open_group_or_free(struct cxl_dport *dport)
+{
+ int rc;
+ struct device *host = dport_to_host(dport);
+ void *group = devres_open_group(host, dport, GFP_KERNEL);
+
+ if (!group) {
+ kfree(dport);
+ return NULL;
+ }
+
+ rc = devm_add_action_or_reset(host, free_dport, dport);
+ if (rc) {
+ devres_release_group(host, group);
+ return NULL;
+ }
+
+ return group;
+}
+
+static void cxl_dport_close_group(struct cxl_dport *dport, void *group)
+{
+ devres_close_group(dport_to_host(dport), group);
+}
+
+/* The dport group id is the dport */
+DEFINE_FREE(cxl_dport_release_group, void *,
+ if (_T) devres_release_group(dport_to_host(_T), _T))
+
static struct cxl_dport *
__devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
int port_id, resource_size_t component_reg_phys,
@@ -1143,14 +1188,20 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
CXL_TARGET_STRLEN)
return ERR_PTR(-EINVAL);
- dport = devm_kzalloc(host, sizeof(*dport), GFP_KERNEL);
+ dport = kzalloc(sizeof(*dport), GFP_KERNEL);
if (!dport)
return ERR_PTR(-ENOMEM);
+ /* Just enough init to manage the devres group */
dport->dport_dev = dport_dev;
dport->port_id = port_id;
dport->port = port;
+ void *dport_group __free(cxl_dport_release_group) =
+ cxl_dport_open_group_or_free(dport);
+ if (!dport_group)
+ return ERR_PTR(-ENOMEM);
+
if (rcrb == CXL_RESOURCE_NONE) {
rc = cxl_dport_setup_regs(&port->dev, dport,
component_reg_phys);
@@ -1203,6 +1254,9 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
cxl_debugfs_create_dport_dir(dport);
+ /* keep the group, and mark the end of devm actions */
+ cxl_dport_close_group(dport, no_free_ptr(dport_group));
+
return dport;
}
@@ -1431,11 +1485,7 @@ static void delete_switch_port(struct cxl_port *port)
static void del_dport(struct cxl_dport *dport)
{
- struct cxl_port *port = dport->port;
-
- devm_release_action(&port->dev, cxl_dport_unlink, dport);
- devm_release_action(&port->dev, cxl_dport_remove, dport);
- devm_kfree(&port->dev, dport);
+ devres_release_group(dport_to_host(dport), dport);
}
static void del_dports(struct cxl_port *port)
--
2.52.0
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH 3/9] cxl/port: Cleanup dport removal with a devres group
2026-01-22 3:33 ` [PATCH 3/9] cxl/port: Cleanup dport removal with a devres group Dan Williams
@ 2026-01-22 11:59 ` Jonathan Cameron
2026-01-22 20:43 ` dan.j.williams
0 siblings, 1 reply; 36+ messages in thread
From: Jonathan Cameron @ 2026-01-22 11:59 UTC (permalink / raw)
To: Dan Williams
Cc: linux-cxl, dave, dave.jiang, alison.schofield, ira.weiny,
terry.bowman
On Wed, 21 Jan 2026 19:33:24 -0800
Dan Williams <dan.j.williams@intel.com> wrote:
> In preparation for adding more setup actions like RAS register mapping,
> introduce a devres group to collect all the dport creation / registration
> actions. This replaces the maintenance tedium of open coding several
> devm_release_action() calls in del_dport().
>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Whilst nice, there is some logic buried deep enough that it might surprise
anyone trying to grasp flow in __devm_cxl_add_dport.
I like the cleanup.h stuff but here I'm wondering if it is appropriate.
Maybe just use a goto in __devm_cxl_add_dport()
Minimum I think is we need a breadcrumb in the function naming that
it has anything to do with devres. _dr_ is used in a few places where
devres naming is too long.
Otherwise one minor thing inline around reusing del_dport in
the DEFINE_FREE()
If we can't because the diverge later, maybe add a comment (or just
ignore this feedback).
> ---
> drivers/cxl/core/port.c | 62 +++++++++++++++++++++++++++++++++++++----
> 1 file changed, 56 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 1637e97f6805..f2723bf948e2 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -1118,6 +1118,51 @@ static void cxl_dport_unlink(void *data)
> sysfs_remove_link(&port->dev.kobj, link_name);
> }
>
> +static struct device *dport_to_host(struct cxl_dport *dport)
> +{
> + if (is_cxl_root(dport->port))
> + return dport->port->uport_dev;
> + return &dport->port->dev;
> +}
> +
> +static void free_dport(void *dport)
> +{
> + kfree(dport);
> +}
> +
> +/*
> + * Upon return either a group is established with one action (free_dport()), or
> + * no group established and @dport is freed.
> + */
> +static void *cxl_dport_open_group_or_free(struct cxl_dport *dport)
Can we put something in the name to hint this is devres stuff?
Group could mean too many things :( Even
cxl_dport_open_dr_group_or_free() avoids sounding too generic.
> +{
> + int rc;
> + struct device *host = dport_to_host(dport);
> + void *group = devres_open_group(host, dport, GFP_KERNEL);
> +
> + if (!group) {
> + kfree(dport);
> + return NULL;
> + }
> +
> + rc = devm_add_action_or_reset(host, free_dport, dport);
> + if (rc) {
> + devres_release_group(host, group);
> + return NULL;
> + }
> +
> + return group;
> +}
> +
> +static void cxl_dport_close_group(struct cxl_dport *dport, void *group)
> +{
> + devres_close_group(dport_to_host(dport), group);
> +}
> +
> +/* The dport group id is the dport */
> +DEFINE_FREE(cxl_dport_release_group, void *,
> + if (_T) devres_release_group(dport_to_host(_T), _T))
Reorder so this can use the typed del_dport()?
> +
> static struct cxl_dport *
> __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
> int port_id, resource_size_t component_reg_phys,
> @@ -1143,14 +1188,20 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
> CXL_TARGET_STRLEN)
> return ERR_PTR(-EINVAL);
>
> - dport = devm_kzalloc(host, sizeof(*dport), GFP_KERNEL);
> + dport = kzalloc(sizeof(*dport), GFP_KERNEL);
> if (!dport)
> return ERR_PTR(-ENOMEM);
>
> + /* Just enough init to manage the devres group */
> dport->dport_dev = dport_dev;
> dport->port_id = port_id;
> dport->port = port;
>
> + void *dport_group __free(cxl_dport_release_group) =
> + cxl_dport_open_group_or_free(dport);
> + if (!dport_group)
> + return ERR_PTR(-ENOMEM);
> +
> if (rcrb == CXL_RESOURCE_NONE) {
> rc = cxl_dport_setup_regs(&port->dev, dport,
> component_reg_phys);
> @@ -1203,6 +1254,9 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
>
> cxl_debugfs_create_dport_dir(dport);
>
> + /* keep the group, and mark the end of devm actions */
> + cxl_dport_close_group(dport, no_free_ptr(dport_group));
> +
> return dport;
> }
>
> @@ -1431,11 +1485,7 @@ static void delete_switch_port(struct cxl_port *port)
>
> static void del_dport(struct cxl_dport *dport)
> {
> - struct cxl_port *port = dport->port;
> -
> - devm_release_action(&port->dev, cxl_dport_unlink, dport);
> - devm_release_action(&port->dev, cxl_dport_remove, dport);
> - devm_kfree(&port->dev, dport);
> + devres_release_group(dport_to_host(dport), dport);
> }
>
> static void del_dports(struct cxl_port *port)
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH 3/9] cxl/port: Cleanup dport removal with a devres group
2026-01-22 11:59 ` Jonathan Cameron
@ 2026-01-22 20:43 ` dan.j.williams
2026-01-23 12:14 ` Jonathan Cameron
0 siblings, 1 reply; 36+ messages in thread
From: dan.j.williams @ 2026-01-22 20:43 UTC (permalink / raw)
To: Jonathan Cameron, Dan Williams
Cc: linux-cxl, dave, dave.jiang, alison.schofield, ira.weiny,
terry.bowman
Jonathan Cameron wrote:
> On Wed, 21 Jan 2026 19:33:24 -0800
> Dan Williams <dan.j.williams@intel.com> wrote:
>
> > In preparation for adding more setup actions like RAS register mapping,
> > introduce a devres group to collect all the dport creation / registration
> > actions. This replaces the maintenance tedium of open coding several
> > devm_release_action() calls in del_dport().
> >
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> Whilst nice, there is some logic buried deep enough that it might surprise
> anyone trying to grasp flow in __devm_cxl_add_dport.
>
> I like the cleanup.h stuff but here I'm wondering if it is appropriate.
> Maybe just use a goto in __devm_cxl_add_dport()
>
It is several gotos, I have a hard time ever writing goto again.
Maybe if you can clarify your "inappropriate" feeling. To be clear I
have heard this from other maintainers that are not ready to let go of
goto, but I feel this is rapidly approaching the reverse-xmas-tree level
of local maintainer preferences.
[..]
> > + * Upon return either a group is established with one action (free_dport()), or
> > + * no group established and @dport is freed.
> > + */
> > +static void *cxl_dport_open_group_or_free(struct cxl_dport *dport)
>
> Can we put something in the name to hint this is devres stuff?
> Group could mean too many things :( Even
> cxl_dport_open_dr_group_or_free() avoids sounding too generic.
I was on the fence with making it more clear it was devres, was just
waiting for a tie breaking shove. Shove received, "dr_group" it is.
> > +{
> > + int rc;
> > + struct device *host = dport_to_host(dport);
> > + void *group = devres_open_group(host, dport, GFP_KERNEL);
> > +
> > + if (!group) {
> > + kfree(dport);
> > + return NULL;
> > + }
> > +
> > + rc = devm_add_action_or_reset(host, free_dport, dport);
> > + if (rc) {
> > + devres_release_group(host, group);
> > + return NULL;
> > + }
> > +
> > + return group;
> > +}
> > +
> > +static void cxl_dport_close_group(struct cxl_dport *dport, void *group)
> > +{
> > + devres_close_group(dport_to_host(dport), group);
> > +}
> > +
> > +/* The dport group id is the dport */
> > +DEFINE_FREE(cxl_dport_release_group, void *,
> > + if (_T) devres_release_group(dport_to_host(_T), _T))
>
> Reorder so this can use the typed del_dport()?
Yeah, that is cleaner.
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH 3/9] cxl/port: Cleanup dport removal with a devres group
2026-01-22 20:43 ` dan.j.williams
@ 2026-01-23 12:14 ` Jonathan Cameron
2026-01-23 12:24 ` Jonathan Cameron
2026-01-30 23:58 ` dan.j.williams
0 siblings, 2 replies; 36+ messages in thread
From: Jonathan Cameron @ 2026-01-23 12:14 UTC (permalink / raw)
To: dan.j.williams
Cc: linux-cxl, dave, dave.jiang, alison.schofield, ira.weiny,
terry.bowman
On Thu, 22 Jan 2026 12:43:36 -0800
dan.j.williams@intel.com wrote:
> Jonathan Cameron wrote:
> > On Wed, 21 Jan 2026 19:33:24 -0800
> > Dan Williams <dan.j.williams@intel.com> wrote:
> >
> > > In preparation for adding more setup actions like RAS register mapping,
> > > introduce a devres group to collect all the dport creation / registration
> > > actions. This replaces the maintenance tedium of open coding several
> > > devm_release_action() calls in del_dport().
> > >
> > > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > Whilst nice, there is some logic buried deep enough that it might surprise
> > anyone trying to grasp flow in __devm_cxl_add_dport.
> >
> > I like the cleanup.h stuff but here I'm wondering if it is appropriate.
> > Maybe just use a goto in __devm_cxl_add_dport()
> >
>
> It is several gotos, I have a hard time ever writing goto again.
>
> Maybe if you can clarify your "inappropriate" feeling. To be clear I
> have heard this from other maintainers that are not ready to let go of
> goto, but I feel this is rapidly approaching the reverse-xmas-tree level
> of local maintainer preferences.
I'm an enthusiast for the cleanup.h stuff. This was very much specific
to this case. I thought I wrote more on this in original mail, but seems
I deleted the comments before sending! Sorry about that.
Main thing I was a bit dubious about in this very specific case was about
overlapping semantic meaning of the group and the the dport (which are
the same address, but we only pretend that in some paths).
That is necessary so there is 'one' thing for:
DEFINE_FREE(cxl_dport_release_group, void *,
if (_T) devres_release_group(dport_to_host(_T), _T))
Which is fine but then the meaning is broken out in
static void cxl_dport_close_group(struct cxl_dport *dport, void *group)
+ I'd have preferred we were explicit in the group being temporary and
hence passed NULL as ID which we can't do if group and dport need
to be the same pointer.
That in combination made me think perhaps it wasn't worth applying here.
>
> [..]
> > > + * Upon return either a group is established with one action (free_dport()), or
> > > + * no group established and @dport is freed.
> > > + */
> > > +static void *cxl_dport_open_group_or_free(struct cxl_dport *dport)
> >
> > Can we put something in the name to hint this is devres stuff?
> > Group could mean too many things :( Even
> > cxl_dport_open_dr_group_or_free() avoids sounding too generic.
>
> I was on the fence with making it more clear it was devres, was just
> waiting for a tie breaking shove. Shove received, "dr_group" it is.
>
> > > +{
> > > + int rc;
> > > + struct device *host = dport_to_host(dport);
> > > + void *group = devres_open_group(host, dport, GFP_KERNEL);
> > > +
> > > + if (!group) {
> > > + kfree(dport);
> > > + return NULL;
> > > + }
> > > +
> > > + rc = devm_add_action_or_reset(host, free_dport, dport);
> > > + if (rc) {
> > > + devres_release_group(host, group);
> > > + return NULL;
> > > + }
> > > +
> > > + return group;
> > > +}
> > > +
> > > +static void cxl_dport_close_group(struct cxl_dport *dport, void *group)
> > > +{
> > > + devres_close_group(dport_to_host(dport), group);
> > > +}
> > > +
> > > +/* The dport group id is the dport */
> > > +DEFINE_FREE(cxl_dport_release_group, void *,
> > > + if (_T) devres_release_group(dport_to_host(_T), _T))
> >
> > Reorder so this can use the typed del_dport()?
>
> Yeah, that is cleaner.
>
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH 3/9] cxl/port: Cleanup dport removal with a devres group
2026-01-23 12:14 ` Jonathan Cameron
@ 2026-01-23 12:24 ` Jonathan Cameron
2026-01-30 23:58 ` dan.j.williams
1 sibling, 0 replies; 36+ messages in thread
From: Jonathan Cameron @ 2026-01-23 12:24 UTC (permalink / raw)
To: dan.j.williams
Cc: linux-cxl, dave, dave.jiang, alison.schofield, ira.weiny,
terry.bowman
On Fri, 23 Jan 2026 12:14:41 +0000
Jonathan Cameron <jonathan.cameron@huawei.com> wrote:
> On Thu, 22 Jan 2026 12:43:36 -0800
> dan.j.williams@intel.com wrote:
>
> > Jonathan Cameron wrote:
> > > On Wed, 21 Jan 2026 19:33:24 -0800
> > > Dan Williams <dan.j.williams@intel.com> wrote:
> > >
> > > > In preparation for adding more setup actions like RAS register mapping,
> > > > introduce a devres group to collect all the dport creation / registration
> > > > actions. This replaces the maintenance tedium of open coding several
> > > > devm_release_action() calls in del_dport().
> > > >
> > > > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > > Whilst nice, there is some logic buried deep enough that it might surprise
> > > anyone trying to grasp flow in __devm_cxl_add_dport.
> > >
> > > I like the cleanup.h stuff but here I'm wondering if it is appropriate.
> > > Maybe just use a goto in __devm_cxl_add_dport()
> > >
> >
> > It is several gotos, I have a hard time ever writing goto again.
> >
> > Maybe if you can clarify your "inappropriate" feeling. To be clear I
> > have heard this from other maintainers that are not ready to let go of
> > goto, but I feel this is rapidly approaching the reverse-xmas-tree level
> > of local maintainer preferences.
>
> I'm an enthusiast for the cleanup.h stuff. This was very much specific
> to this case. I thought I wrote more on this in original mail, but seems
> I deleted the comments before sending! Sorry about that.
>
> Main thing I was a bit dubious about in this very specific case was about
> overlapping semantic meaning of the group and the the dport (which are
> the same address, but we only pretend that in some paths).
>
> That is necessary so there is 'one' thing for:
>
> DEFINE_FREE(cxl_dport_release_group, void *,
> if (_T) devres_release_group(dport_to_host(_T), _T))
>
> Which is fine but then the meaning is broken out in
> static void cxl_dport_close_group(struct cxl_dport *dport, void *group)
>
> + I'd have preferred we were explicit in the group being temporary and
> hence passed NULL as ID which we can't do if group and dport need
> to be the same pointer.
>
> That in combination made me think perhaps it wasn't worth applying here.
Ah. That discussion was in the next patch. Temporal slip ;)
Anyhow, it was a bit of musing rather than a strong request to not use
cleanup stuff here.
J
>
> >
> > [..]
> > > > + * Upon return either a group is established with one action (free_dport()), or
> > > > + * no group established and @dport is freed.
> > > > + */
> > > > +static void *cxl_dport_open_group_or_free(struct cxl_dport *dport)
> > >
> > > Can we put something in the name to hint this is devres stuff?
> > > Group could mean too many things :( Even
> > > cxl_dport_open_dr_group_or_free() avoids sounding too generic.
> >
> > I was on the fence with making it more clear it was devres, was just
> > waiting for a tie breaking shove. Shove received, "dr_group" it is.
> >
> > > > +{
> > > > + int rc;
> > > > + struct device *host = dport_to_host(dport);
> > > > + void *group = devres_open_group(host, dport, GFP_KERNEL);
> > > > +
> > > > + if (!group) {
> > > > + kfree(dport);
> > > > + return NULL;
> > > > + }
> > > > +
> > > > + rc = devm_add_action_or_reset(host, free_dport, dport);
> > > > + if (rc) {
> > > > + devres_release_group(host, group);
> > > > + return NULL;
> > > > + }
> > > > +
> > > > + return group;
> > > > +}
> > > > +
> > > > +static void cxl_dport_close_group(struct cxl_dport *dport, void *group)
> > > > +{
> > > > + devres_close_group(dport_to_host(dport), group);
> > > > +}
> > > > +
> > > > +/* The dport group id is the dport */
> > > > +DEFINE_FREE(cxl_dport_release_group, void *,
> > > > + if (_T) devres_release_group(dport_to_host(_T), _T))
> > >
> > > Reorder so this can use the typed del_dport()?
> >
> > Yeah, that is cleaner.
> >
>
>
>
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH 3/9] cxl/port: Cleanup dport removal with a devres group
2026-01-23 12:14 ` Jonathan Cameron
2026-01-23 12:24 ` Jonathan Cameron
@ 2026-01-30 23:58 ` dan.j.williams
1 sibling, 0 replies; 36+ messages in thread
From: dan.j.williams @ 2026-01-30 23:58 UTC (permalink / raw)
To: Jonathan Cameron, dan.j.williams
Cc: linux-cxl, dave, dave.jiang, alison.schofield, ira.weiny,
terry.bowman
Jonathan Cameron wrote:
> On Thu, 22 Jan 2026 12:43:36 -0800
> dan.j.williams@intel.com wrote:
>
> > Jonathan Cameron wrote:
> > > On Wed, 21 Jan 2026 19:33:24 -0800
> > > Dan Williams <dan.j.williams@intel.com> wrote:
> > >
> > > > In preparation for adding more setup actions like RAS register mapping,
> > > > introduce a devres group to collect all the dport creation / registration
> > > > actions. This replaces the maintenance tedium of open coding several
> > > > devm_release_action() calls in del_dport().
> > > >
> > > > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > > Whilst nice, there is some logic buried deep enough that it might surprise
> > > anyone trying to grasp flow in __devm_cxl_add_dport.
> > >
> > > I like the cleanup.h stuff but here I'm wondering if it is appropriate.
> > > Maybe just use a goto in __devm_cxl_add_dport()
> > >
> >
> > It is several gotos, I have a hard time ever writing goto again.
> >
> > Maybe if you can clarify your "inappropriate" feeling. To be clear I
> > have heard this from other maintainers that are not ready to let go of
> > goto, but I feel this is rapidly approaching the reverse-xmas-tree level
> > of local maintainer preferences.
>
> I'm an enthusiast for the cleanup.h stuff. This was very much specific
> to this case. I thought I wrote more on this in original mail, but seems
> I deleted the comments before sending! Sorry about that.
>
> Main thing I was a bit dubious about in this very specific case was about
> overlapping semantic meaning of the group and the the dport (which are
> the same address, but we only pretend that in some paths).
>
> That is necessary so there is 'one' thing for:
>
> DEFINE_FREE(cxl_dport_release_group, void *,
> if (_T) devres_release_group(dport_to_host(_T), _T))
>
> Which is fine but then the meaning is broken out in
> static void cxl_dport_close_group(struct cxl_dport *dport, void *group)
It is a fair point, but not sure how to square it outside of the comment
in the one place that needs the alias. These things are separate, but
for the convenience of DEFINE_FREE(), aliasing the pointer saves a bunch
of other boilerplate.
> + I'd have preferred we were explicit in the group being temporary and
> hence passed NULL as ID which we can't do if group and dport need
> to be the same pointer.
This group is the one that needs a long lived ID. The port group is
temporary. It was not immediately clear to me that NULL could be used
for that ID given the need to also create the dport group, but that is a
potential follow-on cleanup.
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 4/9] cxl/port: Move decoder setup before dport creation
2026-01-22 3:33 [PATCH 0/9] cxl/port: Unify RAS setup across port types Dan Williams
` (2 preceding siblings ...)
2026-01-22 3:33 ` [PATCH 3/9] cxl/port: Cleanup dport removal with a devres group Dan Williams
@ 2026-01-22 3:33 ` Dan Williams
2026-01-22 13:07 ` Jonathan Cameron
2026-01-22 20:38 ` Dave Jiang
2026-01-22 3:33 ` [PATCH 5/9] cxl/port: Move dport probe operations to a driver event Dan Williams
` (5 subsequent siblings)
9 siblings, 2 replies; 36+ messages in thread
From: Dan Williams @ 2026-01-22 3:33 UTC (permalink / raw)
To: linux-cxl
Cc: jonathan.cameron, dave, dave.jiang, alison.schofield, ira.weiny,
terry.bowman
There are port setup actions that run on first dport arrival, and there are
setup actions that run per dport.
RAS register setup is a future additional setup action to run per-port
(once the first dport arrives), and each dport also has RAS registers to
map.
Before adding that, flip the order of "first dport" and "per-dport"
actions. This makes allocation symmetric with teardown, "first dport"
actions unwind after last dport removed. It also allows for using a devres
group to collect the unrelated decoder, RAS, and dport setup actions into
one group release action.
The new cxl_port_open_group() collects "first dport" and "per-dport" into
one group that can be released on any failure. This group's lifetime only
needs to span the short duration of cxl_port_add_dport() to cleanup all
potential damage from failing to add a dport. Contrast that to the "dport"
devres group that is called upon to destruct fully formed dport objects.
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
drivers/cxl/core/port.c | 43 +++++++++++++++++++++++++++++------------
1 file changed, 31 insertions(+), 12 deletions(-)
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index f2723bf948e2..f69395ea0c14 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -1650,10 +1650,24 @@ static bool dport_exists(struct cxl_port *port, struct device *dport_dev)
return false;
}
-DEFINE_FREE(del_cxl_dport, struct cxl_dport *, if (!IS_ERR_OR_NULL(_T)) del_dport(_T))
+static void *cxl_port_open_group(struct cxl_port *port)
+{
+ return devres_open_group(&port->dev, port, GFP_KERNEL);
+}
+
+/* note this implicitly casts @port_group back to its @port */
+DEFINE_FREE(cxl_port_release_group, struct cxl_port *,
+ if (_T) devres_release_group(&_T->dev, _T))
+
+static void cxl_port_remove_group(struct cxl_port *port, void *port_group)
+{
+ devres_remove_group(&port->dev, port_group);
+}
+
static struct cxl_dport *cxl_port_add_dport(struct cxl_port *port,
struct device *dport_dev)
{
+ struct cxl_dport *dport;
int rc;
device_lock_assert(&port->dev);
@@ -1664,14 +1678,13 @@ static struct cxl_dport *cxl_port_add_dport(struct cxl_port *port,
if (!port->dev.driver)
return ERR_PTR(-ENXIO);
- struct cxl_dport *dport __free(del_cxl_dport) =
- devm_cxl_add_dport_by_dev(port, dport_dev);
- if (IS_ERR(dport))
- return dport;
-
- cxl_switch_parse_cdat(dport);
+ /* Temp group for all "first dport" and "per dport" setup actions */
+ void *port_group __free(cxl_port_release_group) =
+ cxl_port_open_group(port);
+ if (!port_group)
+ return ERR_PTR(-ENOMEM);
- if (port->nr_dports == 1) {
+ if (port->nr_dports == 0) {
/*
* Some host bridges are known to not have component regsisters
* available until a root port has trained CXL. Perform that
@@ -1684,18 +1697,24 @@ static struct cxl_dport *cxl_port_add_dport(struct cxl_port *port,
rc = devm_cxl_switch_port_decoders_setup(port);
if (rc)
return ERR_PTR(rc);
- dev_dbg(&port->dev, "first dport%d:%s added with decoders\n",
- dport->port_id, dev_name(dport_dev));
- return no_free_ptr(dport);
}
+ dport = devm_cxl_add_dport_by_dev(port, dport_dev);
+ if (IS_ERR(dport))
+ return dport;
+
+ /* This group was only needed for early exit above */
+ cxl_port_remove_group(port, no_free_ptr(port_group));
+
+ cxl_switch_parse_cdat(dport);
+
/* New dport added, update the decoder targets */
device_for_each_child(&port->dev, dport, update_decoder_targets);
dev_dbg(&port->dev, "dport%d:%s added\n", dport->port_id,
dev_name(dport_dev));
- return no_free_ptr(dport);
+ return dport;
}
static struct cxl_dport *devm_cxl_create_port(struct device *ep_dev,
--
2.52.0
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH 4/9] cxl/port: Move decoder setup before dport creation
2026-01-22 3:33 ` [PATCH 4/9] cxl/port: Move decoder setup before dport creation Dan Williams
@ 2026-01-22 13:07 ` Jonathan Cameron
2026-01-22 21:42 ` dan.j.williams
2026-01-22 20:38 ` Dave Jiang
1 sibling, 1 reply; 36+ messages in thread
From: Jonathan Cameron @ 2026-01-22 13:07 UTC (permalink / raw)
To: Dan Williams
Cc: linux-cxl, dave, dave.jiang, alison.schofield, ira.weiny,
terry.bowman
On Wed, 21 Jan 2026 19:33:25 -0800
Dan Williams <dan.j.williams@intel.com> wrote:
> There are port setup actions that run on first dport arrival, and there are
> setup actions that run per dport.
>
> RAS register setup is a future additional setup action to run per-port
> (once the first dport arrives), and each dport also has RAS registers to
> map.
>
> Before adding that, flip the order of "first dport" and "per-dport"
> actions. This makes allocation symmetric with teardown, "first dport"
> actions unwind after last dport removed. It also allows for using a devres
> group to collect the unrelated decoder, RAS, and dport setup actions into
> one group release action.
>
> The new cxl_port_open_group() collects "first dport" and "per-dport" into
> one group that can be released on any failure. This group's lifetime only
> needs to span the short duration of cxl_port_add_dport() to cleanup all
> potential damage from failing to add a dport. Contrast that to the "dport"
> devres group that is called upon to destruct fully formed dport objects.
>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Trivial stuff only. Took me a while to get my head around the temporary
group usage, but having done so it seems correct to me. I poked the
various paths fairly heavily to be sure they all worked out after
thinking there was a bug due to a misread :(
Either way on suggestions below.
Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
> ---
> drivers/cxl/core/port.c | 43 +++++++++++++++++++++++++++++------------
> 1 file changed, 31 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index f2723bf948e2..f69395ea0c14 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -1650,10 +1650,24 @@ static bool dport_exists(struct cxl_port *port, struct device *dport_dev)
> return false;
> }
>
> -DEFINE_FREE(del_cxl_dport, struct cxl_dport *, if (!IS_ERR_OR_NULL(_T)) del_dport(_T))
> +static void *cxl_port_open_group(struct cxl_port *port)
> +{
> + return devres_open_group(&port->dev, port, GFP_KERNEL);
So only reason you are using port as the ID is so there is just one thing
to pass to the DEFINE_FREE() callback. Fair enough, but...
> +}
> +
> +/* note this implicitly casts @port_group back to its @port */
> +DEFINE_FREE(cxl_port_release_group, struct cxl_port *,
> + if (_T) devres_release_group(&_T->dev, _T))
> +
> +static void cxl_port_remove_group(struct cxl_port *port, void *port_group)
> +{
> + devres_remove_group(&port->dev, port_group);
To keep this inline with the DEFINE_FREE(), I'd pass in only one parameter.
Can in theory be either of them but to me port_group is more
consistent. Then cast that to get the struct cxl_port *
> +}
> +
> static struct cxl_dport *cxl_port_add_dport(struct cxl_port *port,
> struct device *dport_dev)
> {
> + struct cxl_dport *dport;
> int rc;
>
> device_lock_assert(&port->dev);
> @@ -1664,14 +1678,13 @@ static struct cxl_dport *cxl_port_add_dport(struct cxl_port *port,
> if (!port->dev.driver)
> return ERR_PTR(-ENXIO);
>
> - struct cxl_dport *dport __free(del_cxl_dport) =
> - devm_cxl_add_dport_by_dev(port, dport_dev);
> - if (IS_ERR(dport))
> - return dport;
> -
> - cxl_switch_parse_cdat(dport);
> + /* Temp group for all "first dport" and "per dport" setup actions */
> + void *port_group __free(cxl_port_release_group) =
> + cxl_port_open_group(port);
> + if (!port_group)
> + return ERR_PTR(-ENOMEM);
>
> - if (port->nr_dports == 1) {
> + if (port->nr_dports == 0) {
> /*
> * Some host bridges are known to not have component regsisters
> * available until a root port has trained CXL. Perform that
> @@ -1684,18 +1697,24 @@ static struct cxl_dport *cxl_port_add_dport(struct cxl_port *port,
> rc = devm_cxl_switch_port_decoders_setup(port);
> if (rc)
> return ERR_PTR(rc);
> - dev_dbg(&port->dev, "first dport%d:%s added with decoders\n",
> - dport->port_id, dev_name(dport_dev));
> - return no_free_ptr(dport);
> }
>
> + dport = devm_cxl_add_dport_by_dev(port, dport_dev);
> + if (IS_ERR(dport))
> + return dport;
> +
> + /* This group was only needed for early exit above */
> + cxl_port_remove_group(port, no_free_ptr(port_group));
> +
> + cxl_switch_parse_cdat(dport);
> +
> /* New dport added, update the decoder targets */
> device_for_each_child(&port->dev, dport, update_decoder_targets);
>
> dev_dbg(&port->dev, "dport%d:%s added\n", dport->port_id,
> - return no_free_ptr(dport);
> + return dport;
> }
>
> static struct cxl_dport *devm_cxl_create_port(struct device *ep_dev,
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH 4/9] cxl/port: Move decoder setup before dport creation
2026-01-22 13:07 ` Jonathan Cameron
@ 2026-01-22 21:42 ` dan.j.williams
0 siblings, 0 replies; 36+ messages in thread
From: dan.j.williams @ 2026-01-22 21:42 UTC (permalink / raw)
To: Jonathan Cameron, Dan Williams
Cc: linux-cxl, dave, dave.jiang, alison.schofield, ira.weiny,
terry.bowman
Jonathan Cameron wrote:
> On Wed, 21 Jan 2026 19:33:25 -0800
> Dan Williams <dan.j.williams@intel.com> wrote:
>
> > There are port setup actions that run on first dport arrival, and there are
> > setup actions that run per dport.
> >
> > RAS register setup is a future additional setup action to run per-port
> > (once the first dport arrives), and each dport also has RAS registers to
> > map.
> >
> > Before adding that, flip the order of "first dport" and "per-dport"
> > actions. This makes allocation symmetric with teardown, "first dport"
> > actions unwind after last dport removed. It also allows for using a devres
> > group to collect the unrelated decoder, RAS, and dport setup actions into
> > one group release action.
> >
> > The new cxl_port_open_group() collects "first dport" and "per-dport" into
> > one group that can be released on any failure. This group's lifetime only
> > needs to span the short duration of cxl_port_add_dport() to cleanup all
> > potential damage from failing to add a dport. Contrast that to the "dport"
> > devres group that is called upon to destruct fully formed dport objects.
> >
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>
> Trivial stuff only. Took me a while to get my head around the temporary
> group usage, but having done so it seems correct to me. I poked the
> various paths fairly heavily to be sure they all worked out after
> thinking there was a bug due to a misread :(
>
> Either way on suggestions below.
>
> Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
> > ---
> > drivers/cxl/core/port.c | 43 +++++++++++++++++++++++++++++------------
> > 1 file changed, 31 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> > index f2723bf948e2..f69395ea0c14 100644
> > --- a/drivers/cxl/core/port.c
> > +++ b/drivers/cxl/core/port.c
> > @@ -1650,10 +1650,24 @@ static bool dport_exists(struct cxl_port *port, struct device *dport_dev)
> > return false;
> > }
> >
> > -DEFINE_FREE(del_cxl_dport, struct cxl_dport *, if (!IS_ERR_OR_NULL(_T)) del_dport(_T))
> > +static void *cxl_port_open_group(struct cxl_port *port)
> > +{
> > + return devres_open_group(&port->dev, port, GFP_KERNEL);
> So only reason you are using port as the ID is so there is just one thing
> to pass to the DEFINE_FREE() callback. Fair enough, but...
Right, and it avoids needing to wade through the cleverness of defining
a new CLASS() with a 'fat' pointer that contains the devm host and the
group. I.e. similar to DEFINE_LOCK_GUARD_0.
> > +}
> > +
> > +/* note this implicitly casts @port_group back to its @port */
> > +DEFINE_FREE(cxl_port_release_group, struct cxl_port *,
> > + if (_T) devres_release_group(&_T->dev, _T))
> > +
> > +static void cxl_port_remove_group(struct cxl_port *port, void *port_group)
> > +{
> > + devres_remove_group(&port->dev, port_group);
>
> To keep this inline with the DEFINE_FREE(), I'd pass in only one parameter.
> Can in theory be either of them but to me port_group is more
> consistent. Then cast that to get the struct cxl_port *
I would sooner go the other way and skip the open/remove wrappers
altogether. Because it just adds to the confusion of what is happening.
The DEFINE_FREE() needs a comment for its cleverness, but unlike the
dport case this port group can just be idiomatic
devres_{open,remove}_group().
I will do the "dr_group" rename of the local pointer though which also
fits better with not wrapping devres_{open,remove}_group() for this case.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 4/9] cxl/port: Move decoder setup before dport creation
2026-01-22 3:33 ` [PATCH 4/9] cxl/port: Move decoder setup before dport creation Dan Williams
2026-01-22 13:07 ` Jonathan Cameron
@ 2026-01-22 20:38 ` Dave Jiang
1 sibling, 0 replies; 36+ messages in thread
From: Dave Jiang @ 2026-01-22 20:38 UTC (permalink / raw)
To: Dan Williams, linux-cxl
Cc: jonathan.cameron, dave, alison.schofield, ira.weiny, terry.bowman
On 1/21/26 8:33 PM, Dan Williams wrote:
> There are port setup actions that run on first dport arrival, and there are
> setup actions that run per dport.
>
> RAS register setup is a future additional setup action to run per-port
> (once the first dport arrives), and each dport also has RAS registers to
> map.
>
> Before adding that, flip the order of "first dport" and "per-dport"
> actions. This makes allocation symmetric with teardown, "first dport"
> actions unwind after last dport removed. It also allows for using a devres
> group to collect the unrelated decoder, RAS, and dport setup actions into
> one group release action.
>
> The new cxl_port_open_group() collects "first dport" and "per-dport" into
> one group that can be released on any failure. This group's lifetime only
> needs to span the short duration of cxl_port_add_dport() to cleanup all
> potential damage from failing to add a dport. Contrast that to the "dport"
> devres group that is called upon to destruct fully formed dport objects.
>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> ---
> drivers/cxl/core/port.c | 43 +++++++++++++++++++++++++++++------------
> 1 file changed, 31 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index f2723bf948e2..f69395ea0c14 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -1650,10 +1650,24 @@ static bool dport_exists(struct cxl_port *port, struct device *dport_dev)
> return false;
> }
>
> -DEFINE_FREE(del_cxl_dport, struct cxl_dport *, if (!IS_ERR_OR_NULL(_T)) del_dport(_T))
> +static void *cxl_port_open_group(struct cxl_port *port)
> +{
> + return devres_open_group(&port->dev, port, GFP_KERNEL);
> +}
> +
> +/* note this implicitly casts @port_group back to its @port */
> +DEFINE_FREE(cxl_port_release_group, struct cxl_port *,
> + if (_T) devres_release_group(&_T->dev, _T))
> +
> +static void cxl_port_remove_group(struct cxl_port *port, void *port_group)
> +{
> + devres_remove_group(&port->dev, port_group);
> +}
> +
> static struct cxl_dport *cxl_port_add_dport(struct cxl_port *port,
> struct device *dport_dev)
> {
> + struct cxl_dport *dport;
> int rc;
>
> device_lock_assert(&port->dev);
> @@ -1664,14 +1678,13 @@ static struct cxl_dport *cxl_port_add_dport(struct cxl_port *port,
> if (!port->dev.driver)
> return ERR_PTR(-ENXIO);
>
> - struct cxl_dport *dport __free(del_cxl_dport) =
> - devm_cxl_add_dport_by_dev(port, dport_dev);
> - if (IS_ERR(dport))
> - return dport;
> -
> - cxl_switch_parse_cdat(dport);
> + /* Temp group for all "first dport" and "per dport" setup actions */
> + void *port_group __free(cxl_port_release_group) =
> + cxl_port_open_group(port);
> + if (!port_group)
> + return ERR_PTR(-ENOMEM);
>
> - if (port->nr_dports == 1) {
> + if (port->nr_dports == 0) {
> /*
> * Some host bridges are known to not have component regsisters
> * available until a root port has trained CXL. Perform that
> @@ -1684,18 +1697,24 @@ static struct cxl_dport *cxl_port_add_dport(struct cxl_port *port,
> rc = devm_cxl_switch_port_decoders_setup(port);
> if (rc)
> return ERR_PTR(rc);
> - dev_dbg(&port->dev, "first dport%d:%s added with decoders\n",
> - dport->port_id, dev_name(dport_dev));
> - return no_free_ptr(dport);
> }
>
> + dport = devm_cxl_add_dport_by_dev(port, dport_dev);
> + if (IS_ERR(dport))
> + return dport;
> +
> + /* This group was only needed for early exit above */
> + cxl_port_remove_group(port, no_free_ptr(port_group));
> +
> + cxl_switch_parse_cdat(dport);
> +
> /* New dport added, update the decoder targets */
> device_for_each_child(&port->dev, dport, update_decoder_targets);
>
> dev_dbg(&port->dev, "dport%d:%s added\n", dport->port_id,
> dev_name(dport_dev));
>
> - return no_free_ptr(dport);
> + return dport;
> }
>
> static struct cxl_dport *devm_cxl_create_port(struct device *ep_dev,
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 5/9] cxl/port: Move dport probe operations to a driver event
2026-01-22 3:33 [PATCH 0/9] cxl/port: Unify RAS setup across port types Dan Williams
` (3 preceding siblings ...)
2026-01-22 3:33 ` [PATCH 4/9] cxl/port: Move decoder setup before dport creation Dan Williams
@ 2026-01-22 3:33 ` Dan Williams
2026-01-22 14:44 ` Jonathan Cameron
2026-01-22 3:33 ` [PATCH 6/9] cxl/port: Move dport RAS setup to dport add time Dan Williams
` (4 subsequent siblings)
9 siblings, 1 reply; 36+ messages in thread
From: Dan Williams @ 2026-01-22 3:33 UTC (permalink / raw)
To: linux-cxl
Cc: jonathan.cameron, dave, dave.jiang, alison.schofield, ira.weiny,
terry.bowman
In preparation for adding more register setup to the cxl_port_add_dport()
path (for RAS register mapping), move the dport creation event to a driver
callback. This achieves two goals, it puts driver operations logically
where they belong, in a driver, and it obviates the gymnastics of
DECLARE_TESTABLE() which just makes a mess of grepping for CXL symbols.
In other words, a driver callback is less of an ongoing maintenance burden
than this DECLARE_TESTABLE arrangement that does not scale and diminishes
the grep-ability of the codebase.
cxl_port_add_dport() moves mostly unmodified from drivers/cxl/core/port.c.
The only deliberate change is that it now assumes that the device_lock is
held on entry and the driver is attached (just like cxl_port_probe()).
Reviewed-by: Terry Bowman <terry.bowman@amd.com>
Reviewed-by: Dave Jiang <dave.jiang@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
drivers/cxl/cxl.h | 25 +++------
tools/testing/cxl/exports.h | 13 -----
drivers/cxl/core/hdm.c | 6 +--
drivers/cxl/core/pci.c | 8 +--
drivers/cxl/core/port.c | 78 +++++++---------------------
drivers/cxl/port.c | 60 +++++++++++++++++++++
tools/testing/cxl/cxl_core_exports.c | 22 --------
tools/testing/cxl/test/mock.c | 24 +++------
tools/testing/cxl/Kbuild | 2 +
9 files changed, 102 insertions(+), 136 deletions(-)
delete mode 100644 tools/testing/cxl/exports.h
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 6f3741a57932..75ff5f055f7f 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -840,8 +840,11 @@ struct cxl_endpoint_dvsec_info {
};
int devm_cxl_switch_port_decoders_setup(struct cxl_port *port);
-int __devm_cxl_switch_port_decoders_setup(struct cxl_port *port);
int devm_cxl_endpoint_decoders_setup(struct cxl_port *port);
+void cxl_port_update_decoder_targets(struct cxl_port *port,
+ struct cxl_dport *dport);
+int cxl_port_setup_regs(struct cxl_port *port,
+ resource_size_t component_reg_phys);
struct cxl_dev_state;
int cxl_dvsec_rr_decode(struct cxl_dev_state *cxlds,
@@ -855,6 +858,8 @@ struct cxl_driver {
const char *name;
int (*probe)(struct device *dev);
void (*remove)(struct device *dev);
+ struct cxl_dport *(*add_dport)(struct cxl_port *port,
+ struct device *dport_dev);
struct device_driver drv;
int id;
};
@@ -939,8 +944,6 @@ void cxl_coordinates_combine(struct access_coordinate *out,
bool cxl_endpoint_decoder_reset_detected(struct cxl_port *port);
struct cxl_dport *devm_cxl_add_dport_by_dev(struct cxl_port *port,
struct device *dport_dev);
-struct cxl_dport *__devm_cxl_add_dport_by_dev(struct cxl_port *port,
- struct device *dport_dev);
/*
* Unit test builds overrides this to __weak, find the 'strong' version
@@ -952,20 +955,4 @@ struct cxl_dport *__devm_cxl_add_dport_by_dev(struct cxl_port *port,
u16 cxl_gpf_get_dvsec(struct device *dev);
-/*
- * Declaration for functions that are mocked by cxl_test that are called by
- * cxl_core. The respective functions are defined as __foo() and called by
- * cxl_core as foo(). The macros below ensures that those functions would
- * exist as foo(). See tools/testing/cxl/cxl_core_exports.c and
- * tools/testing/cxl/exports.h for setting up the mock functions. The dance
- * is done to avoid a circular dependency where cxl_core calls a function that
- * ends up being a mock function and goes to * cxl_test where it calls a
- * cxl_core function.
- */
-#ifndef CXL_TEST_ENABLE
-#define DECLARE_TESTABLE(x) __##x
-#define devm_cxl_add_dport_by_dev DECLARE_TESTABLE(devm_cxl_add_dport_by_dev)
-#define devm_cxl_switch_port_decoders_setup DECLARE_TESTABLE(devm_cxl_switch_port_decoders_setup)
-#endif
-
#endif /* __CXL_H__ */
diff --git a/tools/testing/cxl/exports.h b/tools/testing/cxl/exports.h
deleted file mode 100644
index 7ebee7c0bd67..000000000000
--- a/tools/testing/cxl/exports.h
+++ /dev/null
@@ -1,13 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 */
-/* Copyright(c) 2025 Intel Corporation */
-#ifndef __MOCK_CXL_EXPORTS_H_
-#define __MOCK_CXL_EXPORTS_H_
-
-typedef struct cxl_dport *(*cxl_add_dport_by_dev_fn)(struct cxl_port *port,
- struct device *dport_dev);
-extern cxl_add_dport_by_dev_fn _devm_cxl_add_dport_by_dev;
-
-typedef int(*cxl_switch_decoders_setup_fn)(struct cxl_port *port);
-extern cxl_switch_decoders_setup_fn _devm_cxl_switch_port_decoders_setup;
-
-#endif
diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
index 1c5d2022c87a..365b02b7a241 100644
--- a/drivers/cxl/core/hdm.c
+++ b/drivers/cxl/core/hdm.c
@@ -1219,12 +1219,12 @@ static int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
}
/**
- * __devm_cxl_switch_port_decoders_setup - allocate and setup switch decoders
+ * devm_cxl_switch_port_decoders_setup - allocate and setup switch decoders
* @port: CXL port context
*
* Return 0 or -errno on error
*/
-int __devm_cxl_switch_port_decoders_setup(struct cxl_port *port)
+int devm_cxl_switch_port_decoders_setup(struct cxl_port *port)
{
struct cxl_hdm *cxlhdm;
@@ -1248,7 +1248,7 @@ int __devm_cxl_switch_port_decoders_setup(struct cxl_port *port)
dev_err(&port->dev, "HDM decoder capability not found\n");
return -ENXIO;
}
-EXPORT_SYMBOL_NS_GPL(__devm_cxl_switch_port_decoders_setup, "CXL");
+EXPORT_SYMBOL_NS_GPL(devm_cxl_switch_port_decoders_setup, "CXL");
/**
* devm_cxl_endpoint_decoders_setup - allocate and setup endpoint decoders
diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index b838c59d7a3c..f96ce884a213 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -41,14 +41,14 @@ static int pci_get_port_num(struct pci_dev *pdev)
}
/**
- * __devm_cxl_add_dport_by_dev - allocate a dport by dport device
+ * devm_cxl_add_dport_by_dev - allocate a dport by dport device
* @port: cxl_port that hosts the dport
* @dport_dev: 'struct device' of the dport
*
* Returns the allocated dport on success or ERR_PTR() of -errno on error
*/
-struct cxl_dport *__devm_cxl_add_dport_by_dev(struct cxl_port *port,
- struct device *dport_dev)
+struct cxl_dport *devm_cxl_add_dport_by_dev(struct cxl_port *port,
+ struct device *dport_dev)
{
struct cxl_register_map map;
struct pci_dev *pdev;
@@ -69,7 +69,7 @@ struct cxl_dport *__devm_cxl_add_dport_by_dev(struct cxl_port *port,
device_lock_assert(&port->dev);
return devm_cxl_add_dport(port, dport_dev, port_num, map.resource);
}
-EXPORT_SYMBOL_NS_GPL(__devm_cxl_add_dport_by_dev, "CXL");
+EXPORT_SYMBOL_NS_GPL(devm_cxl_add_dport_by_dev, "CXL");
static int cxl_dvsec_mem_range_valid(struct cxl_dev_state *cxlds, int id)
{
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index f69395ea0c14..b87b90e33b0d 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -778,7 +778,7 @@ static int cxl_setup_comp_regs(struct device *host, struct cxl_register_map *map
return cxl_setup_regs(map);
}
-static int cxl_port_setup_regs(struct cxl_port *port,
+int cxl_port_setup_regs(struct cxl_port *port,
resource_size_t component_reg_phys)
{
if (dev_is_platform(port->uport_dev))
@@ -786,6 +786,7 @@ static int cxl_port_setup_regs(struct cxl_port *port,
return cxl_setup_comp_regs(&port->dev, &port->reg_map,
component_reg_phys);
}
+EXPORT_SYMBOL_NS_GPL(cxl_port_setup_regs, "CXL");
static int cxl_dport_setup_regs(struct device *host, struct cxl_dport *dport,
resource_size_t component_reg_phys)
@@ -1637,6 +1638,13 @@ static int update_decoder_targets(struct device *dev, void *data)
return 0;
}
+void cxl_port_update_decoder_targets(struct cxl_port *port,
+ struct cxl_dport *dport)
+{
+ device_for_each_child(&port->dev, dport, update_decoder_targets);
+}
+EXPORT_SYMBOL_NS_GPL(cxl_port_update_decoder_targets, "CXL");
+
static bool dport_exists(struct cxl_port *port, struct device *dport_dev)
{
struct cxl_dport *dport = cxl_find_dport_by_dev(port, dport_dev);
@@ -1650,25 +1658,10 @@ static bool dport_exists(struct cxl_port *port, struct device *dport_dev)
return false;
}
-static void *cxl_port_open_group(struct cxl_port *port)
-{
- return devres_open_group(&port->dev, port, GFP_KERNEL);
-}
-
-/* note this implicitly casts @port_group back to its @port */
-DEFINE_FREE(cxl_port_release_group, struct cxl_port *,
- if (_T) devres_release_group(&_T->dev, _T))
-
-static void cxl_port_remove_group(struct cxl_port *port, void *port_group)
-{
- devres_remove_group(&port->dev, port_group);
-}
-
-static struct cxl_dport *cxl_port_add_dport(struct cxl_port *port,
- struct device *dport_dev)
+static struct cxl_dport *probe_dport(struct cxl_port *port,
+ struct device *dport_dev)
{
- struct cxl_dport *dport;
- int rc;
+ struct cxl_driver *drv;
device_lock_assert(&port->dev);
@@ -1678,43 +1671,12 @@ static struct cxl_dport *cxl_port_add_dport(struct cxl_port *port,
if (!port->dev.driver)
return ERR_PTR(-ENXIO);
- /* Temp group for all "first dport" and "per dport" setup actions */
- void *port_group __free(cxl_port_release_group) =
- cxl_port_open_group(port);
- if (!port_group)
- return ERR_PTR(-ENOMEM);
-
- if (port->nr_dports == 0) {
- /*
- * Some host bridges are known to not have component regsisters
- * available until a root port has trained CXL. Perform that
- * setup now.
- */
- rc = cxl_port_setup_regs(port, port->component_reg_phys);
- if (rc)
- return ERR_PTR(rc);
-
- rc = devm_cxl_switch_port_decoders_setup(port);
- if (rc)
- return ERR_PTR(rc);
- }
-
- dport = devm_cxl_add_dport_by_dev(port, dport_dev);
- if (IS_ERR(dport))
- return dport;
-
- /* This group was only needed for early exit above */
- cxl_port_remove_group(port, no_free_ptr(port_group));
-
- cxl_switch_parse_cdat(dport);
-
- /* New dport added, update the decoder targets */
- device_for_each_child(&port->dev, dport, update_decoder_targets);
-
- dev_dbg(&port->dev, "dport%d:%s added\n", dport->port_id,
- dev_name(dport_dev));
+ drv = container_of(port->dev.driver, struct cxl_driver, drv);
+ if (!drv->add_dport)
+ return ERR_PTR(-ENXIO);
- return dport;
+ /* see cxl_port_add_dport() */
+ return drv->add_dport(port, dport_dev);
}
static struct cxl_dport *devm_cxl_create_port(struct device *ep_dev,
@@ -1761,7 +1723,7 @@ static struct cxl_dport *devm_cxl_create_port(struct device *ep_dev,
}
guard(device)(&port->dev);
- return cxl_port_add_dport(port, dport_dev);
+ return probe_dport(port, dport_dev);
}
static int add_port_attach_ep(struct cxl_memdev *cxlmd,
@@ -1793,7 +1755,7 @@ static int add_port_attach_ep(struct cxl_memdev *cxlmd,
scoped_guard(device, &parent_port->dev) {
parent_dport = cxl_find_dport_by_dev(parent_port, dparent);
if (!parent_dport) {
- parent_dport = cxl_port_add_dport(parent_port, dparent);
+ parent_dport = probe_dport(parent_port, dparent);
if (IS_ERR(parent_dport))
return PTR_ERR(parent_dport);
}
@@ -1829,7 +1791,7 @@ static struct cxl_dport *find_or_add_dport(struct cxl_port *port,
device_lock_assert(&port->dev);
dport = cxl_find_dport_by_dev(port, dport_dev);
if (!dport) {
- dport = cxl_port_add_dport(port, dport_dev);
+ dport = probe_dport(port, dport_dev);
if (IS_ERR(dport))
return dport;
diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
index 51c8f2f84717..70815c41883e 100644
--- a/drivers/cxl/port.c
+++ b/drivers/cxl/port.c
@@ -151,9 +151,69 @@ static const struct attribute_group *cxl_port_attribute_groups[] = {
NULL,
};
+static void *cxl_port_open_group(struct cxl_port *port)
+{
+ return devres_open_group(&port->dev, port, GFP_KERNEL);
+}
+
+/* note this implicitly casts @port_group back to its @port */
+DEFINE_FREE(cxl_port_release_group, struct cxl_port *,
+ if (_T) devres_release_group(&_T->dev, _T))
+
+static void cxl_port_remove_group(struct cxl_port *port, void *port_group)
+{
+ devres_remove_group(&port->dev, port_group);
+}
+
+static struct cxl_dport *cxl_port_add_dport(struct cxl_port *port,
+ struct device *dport_dev)
+{
+ struct cxl_dport *dport;
+ int rc;
+
+ /* Temp group for all "first dport" and "per dport" setup actions */
+ void *port_group __free(cxl_port_release_group) =
+ cxl_port_open_group(port);
+ if (!port_group)
+ return ERR_PTR(-ENOMEM);
+
+ if (port->nr_dports == 0) {
+ /*
+ * Some host bridges are known to not have component regsisters
+ * available until a root port has trained CXL. Perform that
+ * setup now.
+ */
+ rc = cxl_port_setup_regs(port, port->component_reg_phys);
+ if (rc)
+ return ERR_PTR(rc);
+
+ rc = devm_cxl_switch_port_decoders_setup(port);
+ if (rc)
+ return ERR_PTR(rc);
+ }
+
+ dport = devm_cxl_add_dport_by_dev(port, dport_dev);
+ if (IS_ERR(dport))
+ return dport;
+
+ /* This group was only needed for early exit above */
+ cxl_port_remove_group(port, no_free_ptr(port_group));
+
+ cxl_switch_parse_cdat(dport);
+
+ /* New dport added, update the decoder targets */
+ cxl_port_update_decoder_targets(port, dport);
+
+ dev_dbg(&port->dev, "dport%d:%s added\n", dport->port_id,
+ dev_name(dport_dev));
+
+ return dport;
+}
+
static struct cxl_driver cxl_port_driver = {
.name = "cxl_port",
.probe = cxl_port_probe,
+ .add_dport = cxl_port_add_dport,
.id = CXL_DEVICE_PORT,
.drv = {
.dev_groups = cxl_port_attribute_groups,
diff --git a/tools/testing/cxl/cxl_core_exports.c b/tools/testing/cxl/cxl_core_exports.c
index 6754de35598d..f088792a8925 100644
--- a/tools/testing/cxl/cxl_core_exports.c
+++ b/tools/testing/cxl/cxl_core_exports.c
@@ -2,28 +2,6 @@
/* Copyright(c) 2022 Intel Corporation. All rights reserved. */
#include "cxl.h"
-#include "exports.h"
/* Exporting of cxl_core symbols that are only used by cxl_test */
EXPORT_SYMBOL_NS_GPL(cxl_num_decoders_committed, "CXL");
-
-cxl_add_dport_by_dev_fn _devm_cxl_add_dport_by_dev =
- __devm_cxl_add_dport_by_dev;
-EXPORT_SYMBOL_NS_GPL(_devm_cxl_add_dport_by_dev, "CXL");
-
-struct cxl_dport *devm_cxl_add_dport_by_dev(struct cxl_port *port,
- struct device *dport_dev)
-{
- return _devm_cxl_add_dport_by_dev(port, dport_dev);
-}
-EXPORT_SYMBOL_NS_GPL(devm_cxl_add_dport_by_dev, "CXL");
-
-cxl_switch_decoders_setup_fn _devm_cxl_switch_port_decoders_setup =
- __devm_cxl_switch_port_decoders_setup;
-EXPORT_SYMBOL_NS_GPL(_devm_cxl_switch_port_decoders_setup, "CXL");
-
-int devm_cxl_switch_port_decoders_setup(struct cxl_port *port)
-{
- return _devm_cxl_switch_port_decoders_setup(port);
-}
-EXPORT_SYMBOL_NS_GPL(devm_cxl_switch_port_decoders_setup, "CXL");
diff --git a/tools/testing/cxl/test/mock.c b/tools/testing/cxl/test/mock.c
index 44bce80ef3ff..f307c5b39184 100644
--- a/tools/testing/cxl/test/mock.c
+++ b/tools/testing/cxl/test/mock.c
@@ -10,21 +10,12 @@
#include <cxlmem.h>
#include <cxlpci.h>
#include "mock.h"
-#include "../exports.h"
static LIST_HEAD(mock);
-static struct cxl_dport *
-redirect_devm_cxl_add_dport_by_dev(struct cxl_port *port,
- struct device *dport_dev);
-static int redirect_devm_cxl_switch_port_decoders_setup(struct cxl_port *port);
-
void register_cxl_mock_ops(struct cxl_mock_ops *ops)
{
list_add_rcu(&ops->list, &mock);
- _devm_cxl_add_dport_by_dev = redirect_devm_cxl_add_dport_by_dev;
- _devm_cxl_switch_port_decoders_setup =
- redirect_devm_cxl_switch_port_decoders_setup;
}
EXPORT_SYMBOL_GPL(register_cxl_mock_ops);
@@ -32,9 +23,6 @@ DEFINE_STATIC_SRCU(cxl_mock_srcu);
void unregister_cxl_mock_ops(struct cxl_mock_ops *ops)
{
- _devm_cxl_switch_port_decoders_setup =
- __devm_cxl_switch_port_decoders_setup;
- _devm_cxl_add_dport_by_dev = __devm_cxl_add_dport_by_dev;
list_del_rcu(&ops->list);
synchronize_srcu(&cxl_mock_srcu);
}
@@ -163,7 +151,7 @@ __wrap_nvdimm_bus_register(struct device *dev,
}
EXPORT_SYMBOL_GPL(__wrap_nvdimm_bus_register);
-int redirect_devm_cxl_switch_port_decoders_setup(struct cxl_port *port)
+int __wrap_devm_cxl_switch_port_decoders_setup(struct cxl_port *port)
{
int rc, index;
struct cxl_mock_ops *ops = get_cxl_mock_ops(&index);
@@ -171,11 +159,12 @@ int redirect_devm_cxl_switch_port_decoders_setup(struct cxl_port *port)
if (ops && ops->is_mock_port(port->uport_dev))
rc = ops->devm_cxl_switch_port_decoders_setup(port);
else
- rc = __devm_cxl_switch_port_decoders_setup(port);
+ rc = devm_cxl_switch_port_decoders_setup(port);
put_cxl_mock_ops(index);
return rc;
}
+EXPORT_SYMBOL_NS_GPL(__wrap_devm_cxl_switch_port_decoders_setup, "CXL");
int __wrap_devm_cxl_endpoint_decoders_setup(struct cxl_port *port)
{
@@ -257,8 +246,8 @@ void __wrap_cxl_dport_init_ras_reporting(struct cxl_dport *dport, struct device
}
EXPORT_SYMBOL_NS_GPL(__wrap_cxl_dport_init_ras_reporting, "CXL");
-struct cxl_dport *redirect_devm_cxl_add_dport_by_dev(struct cxl_port *port,
- struct device *dport_dev)
+struct cxl_dport *__wrap_devm_cxl_add_dport_by_dev(struct cxl_port *port,
+ struct device *dport_dev)
{
int index;
struct cxl_mock_ops *ops = get_cxl_mock_ops(&index);
@@ -267,11 +256,12 @@ struct cxl_dport *redirect_devm_cxl_add_dport_by_dev(struct cxl_port *port,
if (ops && ops->is_mock_port(port->uport_dev))
dport = ops->devm_cxl_add_dport_by_dev(port, dport_dev);
else
- dport = __devm_cxl_add_dport_by_dev(port, dport_dev);
+ dport = devm_cxl_add_dport_by_dev(port, dport_dev);
put_cxl_mock_ops(index);
return dport;
}
+EXPORT_SYMBOL_NS_GPL(__wrap_devm_cxl_add_dport_by_dev, "CXL");
MODULE_LICENSE("GPL v2");
MODULE_DESCRIPTION("cxl_test: emulation module");
diff --git a/tools/testing/cxl/Kbuild b/tools/testing/cxl/Kbuild
index 6eceefefb0e0..9b2d514a867e 100644
--- a/tools/testing/cxl/Kbuild
+++ b/tools/testing/cxl/Kbuild
@@ -10,6 +10,8 @@ ldflags-y += --wrap=cxl_endpoint_parse_cdat
ldflags-y += --wrap=cxl_dport_init_ras_reporting
ldflags-y += --wrap=devm_cxl_endpoint_decoders_setup
ldflags-y += --wrap=hmat_get_extended_linear_cache_size
+ldflags-y += --wrap=devm_cxl_add_dport_by_dev
+ldflags-y += --wrap=devm_cxl_switch_port_decoders_setup
DRIVERS := ../../../drivers
CXL_SRC := $(DRIVERS)/cxl
--
2.52.0
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH 5/9] cxl/port: Move dport probe operations to a driver event
2026-01-22 3:33 ` [PATCH 5/9] cxl/port: Move dport probe operations to a driver event Dan Williams
@ 2026-01-22 14:44 ` Jonathan Cameron
2026-01-22 21:53 ` dan.j.williams
0 siblings, 1 reply; 36+ messages in thread
From: Jonathan Cameron @ 2026-01-22 14:44 UTC (permalink / raw)
To: Dan Williams
Cc: linux-cxl, dave, dave.jiang, alison.schofield, ira.weiny,
terry.bowman
On Wed, 21 Jan 2026 19:33:26 -0800
Dan Williams <dan.j.williams@intel.com> wrote:
> In preparation for adding more register setup to the cxl_port_add_dport()
> path (for RAS register mapping), move the dport creation event to a driver
> callback. This achieves two goals, it puts driver operations logically
> where they belong, in a driver, and it obviates the gymnastics of
> DECLARE_TESTABLE() which just makes a mess of grepping for CXL symbols.
>
> In other words, a driver callback is less of an ongoing maintenance burden
> than this DECLARE_TESTABLE arrangement that does not scale and diminishes
> the grep-ability of the codebase.
>
> cxl_port_add_dport() moves mostly unmodified from drivers/cxl/core/port.c.
> The only deliberate change is that it now assumes that the device_lock is
> held on entry and the driver is attached (just like cxl_port_probe()).
>
> Reviewed-by: Terry Bowman <terry.bowman@amd.com>
> Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Just a small comment on maybe adding some scary sounding docs, or
a struct wrapper to deal with the very specific callback inside
struct cxl_driver.
With at least a comment,
Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
> ---
> drivers/cxl/cxl.h | 25 +++------
> tools/testing/cxl/exports.h | 13 -----
> drivers/cxl/core/hdm.c | 6 +--
> drivers/cxl/core/pci.c | 8 +--
> drivers/cxl/core/port.c | 78 +++++++---------------------
> drivers/cxl/port.c | 60 +++++++++++++++++++++
> tools/testing/cxl/cxl_core_exports.c | 22 --------
> tools/testing/cxl/test/mock.c | 24 +++------
> tools/testing/cxl/Kbuild | 2 +
> 9 files changed, 102 insertions(+), 136 deletions(-)
> delete mode 100644 tools/testing/cxl/exports.h
>
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 6f3741a57932..75ff5f055f7f 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -840,8 +840,11 @@ struct cxl_endpoint_dvsec_info {
> };
>
> int devm_cxl_switch_port_decoders_setup(struct cxl_port *port);
> -int __devm_cxl_switch_port_decoders_setup(struct cxl_port *port);
> int devm_cxl_endpoint_decoders_setup(struct cxl_port *port);
> +void cxl_port_update_decoder_targets(struct cxl_port *port,
> + struct cxl_dport *dport);
> +int cxl_port_setup_regs(struct cxl_port *port,
> + resource_size_t component_reg_phys);
>
> struct cxl_dev_state;
> int cxl_dvsec_rr_decode(struct cxl_dev_state *cxlds,
> @@ -855,6 +858,8 @@ struct cxl_driver {
> const char *name;
> int (*probe)(struct device *dev);
> void (*remove)(struct device *dev);
> + struct cxl_dport *(*add_dport)(struct cxl_port *port,
> + struct device *dport_dev);
Having a single callback in a generic driver struct seems unusual.
Maybe wrap the structure or add some documentation to set expectations.
Probably not the time for it, but why do we have our own probe and remove rather
than using the ones in device_driver? Maybe we talked about this long
ago, but I can't remember the answer :(
> struct device_driver drv;
> int id;
> };
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH 5/9] cxl/port: Move dport probe operations to a driver event
2026-01-22 14:44 ` Jonathan Cameron
@ 2026-01-22 21:53 ` dan.j.williams
0 siblings, 0 replies; 36+ messages in thread
From: dan.j.williams @ 2026-01-22 21:53 UTC (permalink / raw)
To: Jonathan Cameron, Dan Williams
Cc: linux-cxl, dave, dave.jiang, alison.schofield, ira.weiny,
terry.bowman
Jonathan Cameron wrote:
> On Wed, 21 Jan 2026 19:33:26 -0800
> Dan Williams <dan.j.williams@intel.com> wrote:
>
> > In preparation for adding more register setup to the cxl_port_add_dport()
> > path (for RAS register mapping), move the dport creation event to a driver
> > callback. This achieves two goals, it puts driver operations logically
> > where they belong, in a driver, and it obviates the gymnastics of
> > DECLARE_TESTABLE() which just makes a mess of grepping for CXL symbols.
> >
> > In other words, a driver callback is less of an ongoing maintenance burden
> > than this DECLARE_TESTABLE arrangement that does not scale and diminishes
> > the grep-ability of the codebase.
> >
> > cxl_port_add_dport() moves mostly unmodified from drivers/cxl/core/port.c.
> > The only deliberate change is that it now assumes that the device_lock is
> > held on entry and the driver is attached (just like cxl_port_probe()).
> >
> > Reviewed-by: Terry Bowman <terry.bowman@amd.com>
> > Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> Just a small comment on maybe adding some scary sounding docs, or
> a struct wrapper to deal with the very specific callback inside
> struct cxl_driver.
>
> With at least a comment,
> Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
>
> > ---
> > drivers/cxl/cxl.h | 25 +++------
> > tools/testing/cxl/exports.h | 13 -----
> > drivers/cxl/core/hdm.c | 6 +--
> > drivers/cxl/core/pci.c | 8 +--
> > drivers/cxl/core/port.c | 78 +++++++---------------------
> > drivers/cxl/port.c | 60 +++++++++++++++++++++
> > tools/testing/cxl/cxl_core_exports.c | 22 --------
> > tools/testing/cxl/test/mock.c | 24 +++------
> > tools/testing/cxl/Kbuild | 2 +
> > 9 files changed, 102 insertions(+), 136 deletions(-)
> > delete mode 100644 tools/testing/cxl/exports.h
> >
> > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> > index 6f3741a57932..75ff5f055f7f 100644
> > --- a/drivers/cxl/cxl.h
> > +++ b/drivers/cxl/cxl.h
> > @@ -840,8 +840,11 @@ struct cxl_endpoint_dvsec_info {
> > };
> >
> > int devm_cxl_switch_port_decoders_setup(struct cxl_port *port);
> > -int __devm_cxl_switch_port_decoders_setup(struct cxl_port *port);
> > int devm_cxl_endpoint_decoders_setup(struct cxl_port *port);
> > +void cxl_port_update_decoder_targets(struct cxl_port *port,
> > + struct cxl_dport *dport);
> > +int cxl_port_setup_regs(struct cxl_port *port,
> > + resource_size_t component_reg_phys);
> >
> > struct cxl_dev_state;
> > int cxl_dvsec_rr_decode(struct cxl_dev_state *cxlds,
> > @@ -855,6 +858,8 @@ struct cxl_driver {
> > const char *name;
> > int (*probe)(struct device *dev);
> > void (*remove)(struct device *dev);
> > + struct cxl_dport *(*add_dport)(struct cxl_port *port,
> > + struct device *dport_dev);
>
> Having a single callback in a generic driver struct seems unusual.
> Maybe wrap the structure or add some documentation to set expectations.
>
> Probably not the time for it, but why do we have our own probe and remove rather
> than using the ones in device_driver? Maybe we talked about this long
> ago, but I can't remember the answer :(
I think there were thoughts on a common 'struct cxl_device' indirection,
but fell through and never came back to cleanup these definitions.
I think for now add a comment about this being needed for the most
prevalent CXL object 'struct cxl_port', but that longer term we may want
to consider separate 'struct cxl_port_driver' and 'struct
cxl_memdev_driver' wrappers.
That would save needing to up-cast the 'struct device *' argument as the
first step of these probe routines.
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 6/9] cxl/port: Move dport RAS setup to dport add time
2026-01-22 3:33 [PATCH 0/9] cxl/port: Unify RAS setup across port types Dan Williams
` (4 preceding siblings ...)
2026-01-22 3:33 ` [PATCH 5/9] cxl/port: Move dport probe operations to a driver event Dan Williams
@ 2026-01-22 3:33 ` Dan Williams
2026-01-22 15:00 ` Jonathan Cameron
2026-01-22 21:06 ` Dave Jiang
2026-01-22 3:33 ` [PATCH 7/9] cxl/port: Map CXL Endpoint Port and CXL Switch Port RAS registers Dan Williams
` (3 subsequent siblings)
9 siblings, 2 replies; 36+ messages in thread
From: Dan Williams @ 2026-01-22 3:33 UTC (permalink / raw)
To: linux-cxl
Cc: jonathan.cameron, dave, dave.jiang, alison.schofield, ira.weiny,
terry.bowman
Towards the end goal of making all CXL RAS capability handling uniform
across host bridge ports, upstream switch ports, and endpoint ports, move
dport RAS setup. Move it to cxl_switch_port_probe() context for switch / VH
dports (via cxl_port_add_dport()) and cxl_endpoint_port_probe() context for
an RCH dport. Rename the RAS setup helper to devm_cxl_dport_ras_setup() for
symmetry with devm_cxl_switch_port_decoders_setup().
Only the RCH version needs to be exported and the cxl_test mocking can be
deleted with a dev_is_pci() check on the dport_dev.
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
drivers/cxl/core/core.h | 8 ++++++++
drivers/cxl/cxlpci.h | 8 ++++----
drivers/cxl/core/port.c | 10 +++-------
drivers/cxl/core/ras.c | 30 ++++++++++++++++++------------
drivers/cxl/mem.c | 2 --
drivers/cxl/port.c | 12 ++++++++++++
tools/testing/cxl/test/mock.c | 12 ------------
tools/testing/cxl/Kbuild | 1 -
8 files changed, 45 insertions(+), 38 deletions(-)
diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
index 422531799af2..fb1461c07648 100644
--- a/drivers/cxl/core/core.h
+++ b/drivers/cxl/core/core.h
@@ -144,6 +144,12 @@ int cxl_pci_get_bandwidth(struct pci_dev *pdev, struct access_coordinate *c);
int cxl_port_get_switch_dport_bandwidth(struct cxl_port *port,
struct access_coordinate *c);
+static inline struct device *dport_to_host(struct cxl_dport *dport)
+{
+ if (is_cxl_root(dport->port))
+ return dport->port->uport_dev;
+ return &dport->port->dev;
+}
#ifdef CONFIG_CXL_RAS
int cxl_ras_init(void);
void cxl_ras_exit(void);
@@ -152,6 +158,7 @@ void cxl_handle_cor_ras(struct device *dev, void __iomem *ras_base);
void cxl_dport_map_rch_aer(struct cxl_dport *dport);
void cxl_disable_rch_root_ints(struct cxl_dport *dport);
void cxl_handle_rdport_errors(struct cxl_dev_state *cxlds);
+void devm_cxl_dport_ras_setup(struct cxl_dport *dport);
#else
static inline int cxl_ras_init(void)
{
@@ -166,6 +173,7 @@ static inline void cxl_handle_cor_ras(struct device *dev, void __iomem *ras_base
static inline void cxl_dport_map_rch_aer(struct cxl_dport *dport) { }
static inline void cxl_disable_rch_root_ints(struct cxl_dport *dport) { }
static inline void cxl_handle_rdport_errors(struct cxl_dev_state *cxlds) { }
+static inline void devm_cxl_dport_ras_setup(struct cxl_dport *dport) { }
#endif /* CONFIG_CXL_RAS */
int cxl_gpf_port_setup(struct cxl_dport *dport);
diff --git a/drivers/cxl/cxlpci.h b/drivers/cxl/cxlpci.h
index 6f9c78886fd9..0db3d73548aa 100644
--- a/drivers/cxl/cxlpci.h
+++ b/drivers/cxl/cxlpci.h
@@ -81,7 +81,7 @@ void read_cdat_data(struct cxl_port *port);
void cxl_cor_error_detected(struct pci_dev *pdev);
pci_ers_result_t cxl_error_detected(struct pci_dev *pdev,
pci_channel_state_t state);
-void cxl_dport_init_ras_reporting(struct cxl_dport *dport, struct device *host);
+void devm_cxl_dport_rch_ras_setup(struct cxl_dport *dport);
#else
static inline void cxl_cor_error_detected(struct pci_dev *pdev) { }
@@ -90,9 +90,9 @@ static inline pci_ers_result_t cxl_error_detected(struct pci_dev *pdev,
{
return PCI_ERS_RESULT_NONE;
}
-
-static inline void cxl_dport_init_ras_reporting(struct cxl_dport *dport,
- struct device *host) { }
+static inline void devm_cxl_dport_rch_ras_setup(struct cxl_dport *dport)
+{
+}
#endif
#endif /* __CXL_PCI_H__ */
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index b87b90e33b0d..436d9f8f65cb 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -1119,13 +1119,6 @@ static void cxl_dport_unlink(void *data)
sysfs_remove_link(&port->dev.kobj, link_name);
}
-static struct device *dport_to_host(struct cxl_dport *dport)
-{
- if (is_cxl_root(dport->port))
- return dport->port->uport_dev;
- return &dport->port->dev;
-}
-
static void free_dport(void *dport)
{
kfree(dport);
@@ -1255,6 +1248,9 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
cxl_debugfs_create_dport_dir(dport);
+ if (!dport->rch)
+ devm_cxl_dport_ras_setup(dport);
+
/* keep the group, and mark the end of devm actions */
cxl_dport_close_group(dport, no_free_ptr(dport_group));
diff --git a/drivers/cxl/core/ras.c b/drivers/cxl/core/ras.c
index 72908f3ced77..e90b7a91bf5d 100644
--- a/drivers/cxl/core/ras.c
+++ b/drivers/cxl/core/ras.c
@@ -139,26 +139,32 @@ static void cxl_dport_map_ras(struct cxl_dport *dport)
}
/**
- * cxl_dport_init_ras_reporting - Setup CXL RAS report on this dport
+ * devm_cxl_dport_ras_setup - Setup CXL RAS report on this dport
* @dport: the cxl_dport that needs to be initialized
- * @host: host device for devm operations
*/
-void cxl_dport_init_ras_reporting(struct cxl_dport *dport, struct device *host)
+void devm_cxl_dport_ras_setup(struct cxl_dport *dport)
{
- dport->reg_map.host = host;
+ dport->reg_map.host = dport_to_host(dport);
cxl_dport_map_ras(dport);
+}
- if (dport->rch) {
- struct pci_host_bridge *host_bridge = to_pci_host_bridge(dport->dport_dev);
+void devm_cxl_dport_rch_ras_setup(struct cxl_dport *dport)
+{
+ struct pci_host_bridge *host_bridge;
- if (!host_bridge->native_aer)
- return;
+ if (!dev_is_pci(dport->dport_dev))
+ return;
- cxl_dport_map_rch_aer(dport);
- cxl_disable_rch_root_ints(dport);
- }
+ devm_cxl_dport_ras_setup(dport);
+
+ host_bridge = to_pci_host_bridge(dport->dport_dev);
+ if (!host_bridge->native_aer)
+ return;
+
+ cxl_dport_map_rch_aer(dport);
+ cxl_disable_rch_root_ints(dport);
}
-EXPORT_SYMBOL_NS_GPL(cxl_dport_init_ras_reporting, "CXL");
+EXPORT_SYMBOL_NS_GPL(devm_cxl_dport_rch_ras_setup, "CXL");
void cxl_handle_cor_ras(struct device *dev, void __iomem *ras_base)
{
diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
index c2ee7f7f6320..e25c33f8c6cf 100644
--- a/drivers/cxl/mem.c
+++ b/drivers/cxl/mem.c
@@ -166,8 +166,6 @@ static int cxl_mem_probe(struct device *dev)
else
endpoint_parent = &parent_port->dev;
- cxl_dport_init_ras_reporting(dport, dev);
-
scoped_guard(device, endpoint_parent) {
if (!endpoint_parent->driver) {
dev_err(dev, "CXL port topology %s not enabled\n",
diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
index 70815c41883e..2988533fb0a2 100644
--- a/drivers/cxl/port.c
+++ b/drivers/cxl/port.c
@@ -71,6 +71,7 @@ static int cxl_switch_port_probe(struct cxl_port *port)
static int cxl_endpoint_port_probe(struct cxl_port *port)
{
struct cxl_memdev *cxlmd = to_cxl_memdev(port->uport_dev);
+ struct cxl_dport *dport = port->parent_dport;
int rc;
/* Cache the data early to ensure is_visible() works */
@@ -86,6 +87,17 @@ static int cxl_endpoint_port_probe(struct cxl_port *port)
if (rc)
return rc;
+ /*
+ * With VH (CXL Virtual Host) topology the cxl_port::add_dport() method
+ * handles RAS setup for downstream ports. With RCH (CXL Restricted CXL
+ * Host) topologies the downstream port is enumerated early by platform
+ * firmware, but the RCRB (root complex register block) is not mapped
+ * until after the cxl_pci driver attaches to the RCIeP (root complex
+ * integrated endpoint).
+ */
+ if (dport->rch)
+ devm_cxl_dport_rch_ras_setup(dport);
+
/*
* Now that all endpoint decoders are successfully enumerated, try to
* assemble regions from committed decoders
diff --git a/tools/testing/cxl/test/mock.c b/tools/testing/cxl/test/mock.c
index f307c5b39184..b8fcb50c1027 100644
--- a/tools/testing/cxl/test/mock.c
+++ b/tools/testing/cxl/test/mock.c
@@ -234,18 +234,6 @@ void __wrap_cxl_endpoint_parse_cdat(struct cxl_port *port)
}
EXPORT_SYMBOL_NS_GPL(__wrap_cxl_endpoint_parse_cdat, "CXL");
-void __wrap_cxl_dport_init_ras_reporting(struct cxl_dport *dport, struct device *host)
-{
- int index;
- struct cxl_mock_ops *ops = get_cxl_mock_ops(&index);
-
- if (!ops || !ops->is_mock_port(dport->dport_dev))
- cxl_dport_init_ras_reporting(dport, host);
-
- put_cxl_mock_ops(index);
-}
-EXPORT_SYMBOL_NS_GPL(__wrap_cxl_dport_init_ras_reporting, "CXL");
-
struct cxl_dport *__wrap_devm_cxl_add_dport_by_dev(struct cxl_port *port,
struct device *dport_dev)
{
diff --git a/tools/testing/cxl/Kbuild b/tools/testing/cxl/Kbuild
index 9b2d514a867e..982e8ea28b92 100644
--- a/tools/testing/cxl/Kbuild
+++ b/tools/testing/cxl/Kbuild
@@ -7,7 +7,6 @@ ldflags-y += --wrap=nvdimm_bus_register
ldflags-y += --wrap=cxl_await_media_ready
ldflags-y += --wrap=devm_cxl_add_rch_dport
ldflags-y += --wrap=cxl_endpoint_parse_cdat
-ldflags-y += --wrap=cxl_dport_init_ras_reporting
ldflags-y += --wrap=devm_cxl_endpoint_decoders_setup
ldflags-y += --wrap=hmat_get_extended_linear_cache_size
ldflags-y += --wrap=devm_cxl_add_dport_by_dev
--
2.52.0
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH 6/9] cxl/port: Move dport RAS setup to dport add time
2026-01-22 3:33 ` [PATCH 6/9] cxl/port: Move dport RAS setup to dport add time Dan Williams
@ 2026-01-22 15:00 ` Jonathan Cameron
2026-01-22 21:56 ` dan.j.williams
2026-01-22 21:06 ` Dave Jiang
1 sibling, 1 reply; 36+ messages in thread
From: Jonathan Cameron @ 2026-01-22 15:00 UTC (permalink / raw)
To: Dan Williams
Cc: linux-cxl, dave, dave.jiang, alison.schofield, ira.weiny,
terry.bowman
On Wed, 21 Jan 2026 19:33:27 -0800
Dan Williams <dan.j.williams@intel.com> wrote:
> Towards the end goal of making all CXL RAS capability handling uniform
> across host bridge ports, upstream switch ports, and endpoint ports, move
> dport RAS setup. Move it to cxl_switch_port_probe() context for switch / VH
> dports (via cxl_port_add_dport()) and cxl_endpoint_port_probe() context for
> an RCH dport. Rename the RAS setup helper to devm_cxl_dport_ras_setup() for
> symmetry with devm_cxl_switch_port_decoders_setup().
>
> Only the RCH version needs to be exported and the cxl_test mocking can be
> deleted with a dev_is_pci() check on the dport_dev.
>
Comment below that doesn't really have anything to do with this patch
and can be ignored.
Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
> drivers/cxl/core/core.h | 8 ++++++++
> drivers/cxl/cxlpci.h | 8 ++++----
> drivers/cxl/core/port.c | 10 +++-------
> drivers/cxl/core/ras.c | 30 ++++++++++++++++++------------
> drivers/cxl/mem.c | 2 --
> drivers/cxl/port.c | 12 ++++++++++++
> tools/testing/cxl/test/mock.c | 12 ------------
> tools/testing/cxl/Kbuild | 1 -
> 8 files changed, 45 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
> index 422531799af2..fb1461c07648 100644
> --- a/drivers/cxl/core/core.h
> +++ b/drivers/cxl/core/core.h
> @@ -144,6 +144,12 @@ int cxl_pci_get_bandwidth(struct pci_dev *pdev, struct access_coordinate *c);
> int cxl_port_get_switch_dport_bandwidth(struct cxl_port *port,
> struct access_coordinate *c);
>
> +static inline struct device *dport_to_host(struct cxl_dport *dport)
> +{
> + if (is_cxl_root(dport->port))
> + return dport->port->uport_dev;
> + return &dport->port->dev;
Obviously really applies to earlier patch but maybe it's worth
struct cxl_port *port = &dport->port;
if (is_cxl_root(port))
return port->uport_dev;
return port->dev;
I'm not sure it really adds that much but is the sort of thing
someone will post a patch 'tidying up' later.
> +}
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH 6/9] cxl/port: Move dport RAS setup to dport add time
2026-01-22 15:00 ` Jonathan Cameron
@ 2026-01-22 21:56 ` dan.j.williams
0 siblings, 0 replies; 36+ messages in thread
From: dan.j.williams @ 2026-01-22 21:56 UTC (permalink / raw)
To: Jonathan Cameron, Dan Williams
Cc: linux-cxl, dave, dave.jiang, alison.schofield, ira.weiny,
terry.bowman
Jonathan Cameron wrote:
[..]
> Obviously really applies to earlier patch but maybe it's worth
>
> struct cxl_port *port = &dport->port;
>
> if (is_cxl_root(port))
> return port->uport_dev;
> return port->dev;
>
> I'm not sure it really adds that much but is the sort of thing
> someone will post a patch 'tidying up' later.
Sure, folded.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 6/9] cxl/port: Move dport RAS setup to dport add time
2026-01-22 3:33 ` [PATCH 6/9] cxl/port: Move dport RAS setup to dport add time Dan Williams
2026-01-22 15:00 ` Jonathan Cameron
@ 2026-01-22 21:06 ` Dave Jiang
1 sibling, 0 replies; 36+ messages in thread
From: Dave Jiang @ 2026-01-22 21:06 UTC (permalink / raw)
To: Dan Williams, linux-cxl
Cc: jonathan.cameron, dave, alison.schofield, ira.weiny, terry.bowman
On 1/21/26 8:33 PM, Dan Williams wrote:
> Towards the end goal of making all CXL RAS capability handling uniform
> across host bridge ports, upstream switch ports, and endpoint ports, move
> dport RAS setup. Move it to cxl_switch_port_probe() context for switch / VH
> dports (via cxl_port_add_dport()) and cxl_endpoint_port_probe() context for
> an RCH dport. Rename the RAS setup helper to devm_cxl_dport_ras_setup() for
> symmetry with devm_cxl_switch_port_decoders_setup().
>
> Only the RCH version needs to be exported and the cxl_test mocking can be
> deleted with a dev_is_pci() check on the dport_dev.
>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> ---
> drivers/cxl/core/core.h | 8 ++++++++
> drivers/cxl/cxlpci.h | 8 ++++----
> drivers/cxl/core/port.c | 10 +++-------
> drivers/cxl/core/ras.c | 30 ++++++++++++++++++------------
> drivers/cxl/mem.c | 2 --
> drivers/cxl/port.c | 12 ++++++++++++
> tools/testing/cxl/test/mock.c | 12 ------------
> tools/testing/cxl/Kbuild | 1 -
> 8 files changed, 45 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
> index 422531799af2..fb1461c07648 100644
> --- a/drivers/cxl/core/core.h
> +++ b/drivers/cxl/core/core.h
> @@ -144,6 +144,12 @@ int cxl_pci_get_bandwidth(struct pci_dev *pdev, struct access_coordinate *c);
> int cxl_port_get_switch_dport_bandwidth(struct cxl_port *port,
> struct access_coordinate *c);
>
> +static inline struct device *dport_to_host(struct cxl_dport *dport)
> +{
> + if (is_cxl_root(dport->port))
> + return dport->port->uport_dev;
> + return &dport->port->dev;
> +}
> #ifdef CONFIG_CXL_RAS
> int cxl_ras_init(void);
> void cxl_ras_exit(void);
> @@ -152,6 +158,7 @@ void cxl_handle_cor_ras(struct device *dev, void __iomem *ras_base);
> void cxl_dport_map_rch_aer(struct cxl_dport *dport);
> void cxl_disable_rch_root_ints(struct cxl_dport *dport);
> void cxl_handle_rdport_errors(struct cxl_dev_state *cxlds);
> +void devm_cxl_dport_ras_setup(struct cxl_dport *dport);
> #else
> static inline int cxl_ras_init(void)
> {
> @@ -166,6 +173,7 @@ static inline void cxl_handle_cor_ras(struct device *dev, void __iomem *ras_base
> static inline void cxl_dport_map_rch_aer(struct cxl_dport *dport) { }
> static inline void cxl_disable_rch_root_ints(struct cxl_dport *dport) { }
> static inline void cxl_handle_rdport_errors(struct cxl_dev_state *cxlds) { }
> +static inline void devm_cxl_dport_ras_setup(struct cxl_dport *dport) { }
> #endif /* CONFIG_CXL_RAS */
>
> int cxl_gpf_port_setup(struct cxl_dport *dport);
> diff --git a/drivers/cxl/cxlpci.h b/drivers/cxl/cxlpci.h
> index 6f9c78886fd9..0db3d73548aa 100644
> --- a/drivers/cxl/cxlpci.h
> +++ b/drivers/cxl/cxlpci.h
> @@ -81,7 +81,7 @@ void read_cdat_data(struct cxl_port *port);
> void cxl_cor_error_detected(struct pci_dev *pdev);
> pci_ers_result_t cxl_error_detected(struct pci_dev *pdev,
> pci_channel_state_t state);
> -void cxl_dport_init_ras_reporting(struct cxl_dport *dport, struct device *host);
> +void devm_cxl_dport_rch_ras_setup(struct cxl_dport *dport);
> #else
> static inline void cxl_cor_error_detected(struct pci_dev *pdev) { }
>
> @@ -90,9 +90,9 @@ static inline pci_ers_result_t cxl_error_detected(struct pci_dev *pdev,
> {
> return PCI_ERS_RESULT_NONE;
> }
> -
> -static inline void cxl_dport_init_ras_reporting(struct cxl_dport *dport,
> - struct device *host) { }
> +static inline void devm_cxl_dport_rch_ras_setup(struct cxl_dport *dport)
> +{
> +}
> #endif
>
> #endif /* __CXL_PCI_H__ */
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index b87b90e33b0d..436d9f8f65cb 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -1119,13 +1119,6 @@ static void cxl_dport_unlink(void *data)
> sysfs_remove_link(&port->dev.kobj, link_name);
> }
>
> -static struct device *dport_to_host(struct cxl_dport *dport)
> -{
> - if (is_cxl_root(dport->port))
> - return dport->port->uport_dev;
> - return &dport->port->dev;
> -}
> -
> static void free_dport(void *dport)
> {
> kfree(dport);
> @@ -1255,6 +1248,9 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
>
> cxl_debugfs_create_dport_dir(dport);
>
> + if (!dport->rch)
> + devm_cxl_dport_ras_setup(dport);
> +
> /* keep the group, and mark the end of devm actions */
> cxl_dport_close_group(dport, no_free_ptr(dport_group));
>
> diff --git a/drivers/cxl/core/ras.c b/drivers/cxl/core/ras.c
> index 72908f3ced77..e90b7a91bf5d 100644
> --- a/drivers/cxl/core/ras.c
> +++ b/drivers/cxl/core/ras.c
> @@ -139,26 +139,32 @@ static void cxl_dport_map_ras(struct cxl_dport *dport)
> }
>
> /**
> - * cxl_dport_init_ras_reporting - Setup CXL RAS report on this dport
> + * devm_cxl_dport_ras_setup - Setup CXL RAS report on this dport
> * @dport: the cxl_dport that needs to be initialized
> - * @host: host device for devm operations
> */
> -void cxl_dport_init_ras_reporting(struct cxl_dport *dport, struct device *host)
> +void devm_cxl_dport_ras_setup(struct cxl_dport *dport)
> {
> - dport->reg_map.host = host;
> + dport->reg_map.host = dport_to_host(dport);
> cxl_dport_map_ras(dport);
> +}
>
> - if (dport->rch) {
> - struct pci_host_bridge *host_bridge = to_pci_host_bridge(dport->dport_dev);
> +void devm_cxl_dport_rch_ras_setup(struct cxl_dport *dport)
> +{
> + struct pci_host_bridge *host_bridge;
>
> - if (!host_bridge->native_aer)
> - return;
> + if (!dev_is_pci(dport->dport_dev))
> + return;
>
> - cxl_dport_map_rch_aer(dport);
> - cxl_disable_rch_root_ints(dport);
> - }
> + devm_cxl_dport_ras_setup(dport);
> +
> + host_bridge = to_pci_host_bridge(dport->dport_dev);
> + if (!host_bridge->native_aer)
> + return;
> +
> + cxl_dport_map_rch_aer(dport);
> + cxl_disable_rch_root_ints(dport);
> }
> -EXPORT_SYMBOL_NS_GPL(cxl_dport_init_ras_reporting, "CXL");
> +EXPORT_SYMBOL_NS_GPL(devm_cxl_dport_rch_ras_setup, "CXL");
>
> void cxl_handle_cor_ras(struct device *dev, void __iomem *ras_base)
> {
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index c2ee7f7f6320..e25c33f8c6cf 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
> @@ -166,8 +166,6 @@ static int cxl_mem_probe(struct device *dev)
> else
> endpoint_parent = &parent_port->dev;
>
> - cxl_dport_init_ras_reporting(dport, dev);
> -
> scoped_guard(device, endpoint_parent) {
> if (!endpoint_parent->driver) {
> dev_err(dev, "CXL port topology %s not enabled\n",
> diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
> index 70815c41883e..2988533fb0a2 100644
> --- a/drivers/cxl/port.c
> +++ b/drivers/cxl/port.c
> @@ -71,6 +71,7 @@ static int cxl_switch_port_probe(struct cxl_port *port)
> static int cxl_endpoint_port_probe(struct cxl_port *port)
> {
> struct cxl_memdev *cxlmd = to_cxl_memdev(port->uport_dev);
> + struct cxl_dport *dport = port->parent_dport;
> int rc;
>
> /* Cache the data early to ensure is_visible() works */
> @@ -86,6 +87,17 @@ static int cxl_endpoint_port_probe(struct cxl_port *port)
> if (rc)
> return rc;
>
> + /*
> + * With VH (CXL Virtual Host) topology the cxl_port::add_dport() method
> + * handles RAS setup for downstream ports. With RCH (CXL Restricted CXL
> + * Host) topologies the downstream port is enumerated early by platform
> + * firmware, but the RCRB (root complex register block) is not mapped
> + * until after the cxl_pci driver attaches to the RCIeP (root complex
> + * integrated endpoint).
> + */
> + if (dport->rch)
> + devm_cxl_dport_rch_ras_setup(dport);
> +
> /*
> * Now that all endpoint decoders are successfully enumerated, try to
> * assemble regions from committed decoders
> diff --git a/tools/testing/cxl/test/mock.c b/tools/testing/cxl/test/mock.c
> index f307c5b39184..b8fcb50c1027 100644
> --- a/tools/testing/cxl/test/mock.c
> +++ b/tools/testing/cxl/test/mock.c
> @@ -234,18 +234,6 @@ void __wrap_cxl_endpoint_parse_cdat(struct cxl_port *port)
> }
> EXPORT_SYMBOL_NS_GPL(__wrap_cxl_endpoint_parse_cdat, "CXL");
>
> -void __wrap_cxl_dport_init_ras_reporting(struct cxl_dport *dport, struct device *host)
> -{
> - int index;
> - struct cxl_mock_ops *ops = get_cxl_mock_ops(&index);
> -
> - if (!ops || !ops->is_mock_port(dport->dport_dev))
> - cxl_dport_init_ras_reporting(dport, host);
> -
> - put_cxl_mock_ops(index);
> -}
> -EXPORT_SYMBOL_NS_GPL(__wrap_cxl_dport_init_ras_reporting, "CXL");
> -
> struct cxl_dport *__wrap_devm_cxl_add_dport_by_dev(struct cxl_port *port,
> struct device *dport_dev)
> {
> diff --git a/tools/testing/cxl/Kbuild b/tools/testing/cxl/Kbuild
> index 9b2d514a867e..982e8ea28b92 100644
> --- a/tools/testing/cxl/Kbuild
> +++ b/tools/testing/cxl/Kbuild
> @@ -7,7 +7,6 @@ ldflags-y += --wrap=nvdimm_bus_register
> ldflags-y += --wrap=cxl_await_media_ready
> ldflags-y += --wrap=devm_cxl_add_rch_dport
> ldflags-y += --wrap=cxl_endpoint_parse_cdat
> -ldflags-y += --wrap=cxl_dport_init_ras_reporting
> ldflags-y += --wrap=devm_cxl_endpoint_decoders_setup
> ldflags-y += --wrap=hmat_get_extended_linear_cache_size
> ldflags-y += --wrap=devm_cxl_add_dport_by_dev
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 7/9] cxl/port: Map CXL Endpoint Port and CXL Switch Port RAS registers
2026-01-22 3:33 [PATCH 0/9] cxl/port: Unify RAS setup across port types Dan Williams
` (5 preceding siblings ...)
2026-01-22 3:33 ` [PATCH 6/9] cxl/port: Move dport RAS setup to dport add time Dan Williams
@ 2026-01-22 3:33 ` Dan Williams
2026-01-22 15:25 ` Jonathan Cameron
2026-01-22 3:33 ` [PATCH 8/9] cxl/port: Move endpoint component register management to cxl_port Dan Williams
` (2 subsequent siblings)
9 siblings, 1 reply; 36+ messages in thread
From: Dan Williams @ 2026-01-22 3:33 UTC (permalink / raw)
To: linux-cxl
Cc: jonathan.cameron, dave, dave.jiang, alison.schofield, ira.weiny,
terry.bowman
From: Terry Bowman <terry.bowman@amd.com>
In preparation for CXL VH (Virtual Host) topology protocol error handling,
add RAS capability registered mapping for all ports in a CXL VH topology.
This includes the RAS capabilities of Switch Upstream Ports, Switch
Downstream Ports, Host Bridge Ports ("upstream"), and Root Ports
("downstream")
Update cxl_port_add_dport() to map the upstream RAS capability on first
'dport' attach.
Signed-off-by: Terry Bowman <terry.bowman@amd.com>
Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
Reviewed-by: Dave Jiang <dave.jiang@intel.com>
Co-developed-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
drivers/cxl/cxl.h | 2 ++
drivers/cxl/cxlpci.h | 4 ++++
drivers/cxl/core/ras.c | 16 ++++++++++++++++
drivers/cxl/port.c | 6 ++++++
4 files changed, 28 insertions(+)
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 75ff5f055f7f..3e0e82523bfd 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -607,6 +607,7 @@ struct cxl_dax_region {
* @parent_dport: dport that points to this port in the parent
* @decoder_ida: allocator for decoder ids
* @reg_map: component and ras register mapping parameters
+ * @regs: mapped component registers
* @nr_dports: number of entries in @dports
* @hdm_end: track last allocated HDM decoder instance for allocation ordering
* @commit_end: cursor to track highest committed decoder for commit ordering
@@ -628,6 +629,7 @@ struct cxl_port {
struct cxl_dport *parent_dport;
struct ida decoder_ida;
struct cxl_register_map reg_map;
+ struct cxl_component_regs regs;
int nr_dports;
int hdm_end;
int commit_end;
diff --git a/drivers/cxl/cxlpci.h b/drivers/cxl/cxlpci.h
index 0db3d73548aa..970add0256e9 100644
--- a/drivers/cxl/cxlpci.h
+++ b/drivers/cxl/cxlpci.h
@@ -82,6 +82,7 @@ void cxl_cor_error_detected(struct pci_dev *pdev);
pci_ers_result_t cxl_error_detected(struct pci_dev *pdev,
pci_channel_state_t state);
void devm_cxl_dport_rch_ras_setup(struct cxl_dport *dport);
+void devm_cxl_port_ras_setup(struct cxl_port *port);
#else
static inline void cxl_cor_error_detected(struct pci_dev *pdev) { }
@@ -93,6 +94,9 @@ static inline pci_ers_result_t cxl_error_detected(struct pci_dev *pdev,
static inline void devm_cxl_dport_rch_ras_setup(struct cxl_dport *dport)
{
}
+static inline void devm_cxl_port_ras_setup(struct cxl_port *port)
+{
+}
#endif
#endif /* __CXL_PCI_H__ */
diff --git a/drivers/cxl/core/ras.c b/drivers/cxl/core/ras.c
index e90b7a91bf5d..b4be9c5715a6 100644
--- a/drivers/cxl/core/ras.c
+++ b/drivers/cxl/core/ras.c
@@ -166,6 +166,22 @@ void devm_cxl_dport_rch_ras_setup(struct cxl_dport *dport)
}
EXPORT_SYMBOL_NS_GPL(devm_cxl_dport_rch_ras_setup, "CXL");
+void devm_cxl_port_ras_setup(struct cxl_port *port)
+{
+ struct cxl_register_map *map = &port->reg_map;
+
+ if (!map->component_map.ras.valid) {
+ dev_dbg(&port->dev, "RAS registers not found\n");
+ return;
+ }
+
+ map->host = &port->dev;
+ if (cxl_map_component_regs(map, &port->regs,
+ BIT(CXL_CM_CAP_CAP_ID_RAS)))
+ dev_dbg(&port->dev, "Failed to map RAS capability\n");
+}
+EXPORT_SYMBOL_NS_GPL(devm_cxl_port_ras_setup, "CXL");
+
void cxl_handle_cor_ras(struct device *dev, void __iomem *ras_base)
{
void __iomem *addr;
diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
index 2988533fb0a2..50a643cb3e04 100644
--- a/drivers/cxl/port.c
+++ b/drivers/cxl/port.c
@@ -202,6 +202,12 @@ static struct cxl_dport *cxl_port_add_dport(struct cxl_port *port,
rc = devm_cxl_switch_port_decoders_setup(port);
if (rc)
return ERR_PTR(rc);
+
+ /*
+ * RAS setup is optional, either driver operation can continue
+ * on failure, or the device does not implement RAS registers.
+ */
+ devm_cxl_port_ras_setup(port);
}
dport = devm_cxl_add_dport_by_dev(port, dport_dev);
--
2.52.0
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH 7/9] cxl/port: Map CXL Endpoint Port and CXL Switch Port RAS registers
2026-01-22 3:33 ` [PATCH 7/9] cxl/port: Map CXL Endpoint Port and CXL Switch Port RAS registers Dan Williams
@ 2026-01-22 15:25 ` Jonathan Cameron
2026-01-22 22:11 ` dan.j.williams
0 siblings, 1 reply; 36+ messages in thread
From: Jonathan Cameron @ 2026-01-22 15:25 UTC (permalink / raw)
To: Dan Williams
Cc: linux-cxl, dave, dave.jiang, alison.schofield, ira.weiny,
terry.bowman
On Wed, 21 Jan 2026 19:33:28 -0800
Dan Williams <dan.j.williams@intel.com> wrote:
> From: Terry Bowman <terry.bowman@amd.com>
>
> In preparation for CXL VH (Virtual Host) topology protocol error handling,
> add RAS capability registered mapping for all ports in a CXL VH topology.
> This includes the RAS capabilities of Switch Upstream Ports, Switch
> Downstream Ports, Host Bridge Ports ("upstream"), and Root Ports
> ("downstream")
>
> Update cxl_port_add_dport() to map the upstream RAS capability on first
> 'dport' attach.
I'm struggling to see how this maps the endpoint registers (or at least
how it changes that). So maybe the patch title has become incorrect?
Other than that seems fine to me.
>
> Signed-off-by: Terry Bowman <terry.bowman@amd.com>
> Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
> Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> Co-developed-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
> drivers/cxl/cxl.h | 2 ++
> drivers/cxl/cxlpci.h | 4 ++++
> drivers/cxl/core/ras.c | 16 ++++++++++++++++
> drivers/cxl/port.c | 6 ++++++
> 4 files changed, 28 insertions(+)
>
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 75ff5f055f7f..3e0e82523bfd 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -607,6 +607,7 @@ struct cxl_dax_region {
> * @parent_dport: dport that points to this port in the parent
> * @decoder_ida: allocator for decoder ids
> * @reg_map: component and ras register mapping parameters
> + * @regs: mapped component registers
> * @nr_dports: number of entries in @dports
> * @hdm_end: track last allocated HDM decoder instance for allocation ordering
> * @commit_end: cursor to track highest committed decoder for commit ordering
> @@ -628,6 +629,7 @@ struct cxl_port {
> struct cxl_dport *parent_dport;
> struct ida decoder_ida;
> struct cxl_register_map reg_map;
> + struct cxl_component_regs regs;
> int nr_dports;
> int hdm_end;
> int commit_end;
> diff --git a/drivers/cxl/cxlpci.h b/drivers/cxl/cxlpci.h
> index 0db3d73548aa..970add0256e9 100644
> --- a/drivers/cxl/cxlpci.h
> +++ b/drivers/cxl/cxlpci.h
> @@ -82,6 +82,7 @@ void cxl_cor_error_detected(struct pci_dev *pdev);
> pci_ers_result_t cxl_error_detected(struct pci_dev *pdev,
> pci_channel_state_t state);
> void devm_cxl_dport_rch_ras_setup(struct cxl_dport *dport);
> +void devm_cxl_port_ras_setup(struct cxl_port *port);
> #else
> static inline void cxl_cor_error_detected(struct pci_dev *pdev) { }
>
> @@ -93,6 +94,9 @@ static inline pci_ers_result_t cxl_error_detected(struct pci_dev *pdev,
> static inline void devm_cxl_dport_rch_ras_setup(struct cxl_dport *dport)
> {
> }
> +static inline void devm_cxl_port_ras_setup(struct cxl_port *port)
> +{
> +}
> #endif
>
> #endif /* __CXL_PCI_H__ */
> diff --git a/drivers/cxl/core/ras.c b/drivers/cxl/core/ras.c
> index e90b7a91bf5d..b4be9c5715a6 100644
> --- a/drivers/cxl/core/ras.c
> +++ b/drivers/cxl/core/ras.c
> @@ -166,6 +166,22 @@ void devm_cxl_dport_rch_ras_setup(struct cxl_dport *dport)
> }
> EXPORT_SYMBOL_NS_GPL(devm_cxl_dport_rch_ras_setup, "CXL");
>
> +void devm_cxl_port_ras_setup(struct cxl_port *port)
> +{
> + struct cxl_register_map *map = &port->reg_map;
> +
> + if (!map->component_map.ras.valid) {
> + dev_dbg(&port->dev, "RAS registers not found\n");
> + return;
> + }
> +
> + map->host = &port->dev;
> + if (cxl_map_component_regs(map, &port->regs,
> + BIT(CXL_CM_CAP_CAP_ID_RAS)))
> + dev_dbg(&port->dev, "Failed to map RAS capability\n");
> +}
> +EXPORT_SYMBOL_NS_GPL(devm_cxl_port_ras_setup, "CXL");
> +
> void cxl_handle_cor_ras(struct device *dev, void __iomem *ras_base)
> {
> void __iomem *addr;
> diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
> index 2988533fb0a2..50a643cb3e04 100644
> --- a/drivers/cxl/port.c
> +++ b/drivers/cxl/port.c
> @@ -202,6 +202,12 @@ static struct cxl_dport *cxl_port_add_dport(struct cxl_port *port,
> rc = devm_cxl_switch_port_decoders_setup(port);
> if (rc)
> return ERR_PTR(rc);
> +
> + /*
> + * RAS setup is optional, either driver operation can continue
> + * on failure, or the device does not implement RAS registers.
> + */
> + devm_cxl_port_ras_setup(port);
> }
>
> dport = devm_cxl_add_dport_by_dev(port, dport_dev);
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH 7/9] cxl/port: Map CXL Endpoint Port and CXL Switch Port RAS registers
2026-01-22 15:25 ` Jonathan Cameron
@ 2026-01-22 22:11 ` dan.j.williams
0 siblings, 0 replies; 36+ messages in thread
From: dan.j.williams @ 2026-01-22 22:11 UTC (permalink / raw)
To: Jonathan Cameron, Dan Williams
Cc: linux-cxl, dave, dave.jiang, alison.schofield, ira.weiny,
terry.bowman
Jonathan Cameron wrote:
> On Wed, 21 Jan 2026 19:33:28 -0800
> Dan Williams <dan.j.williams@intel.com> wrote:
>
> > From: Terry Bowman <terry.bowman@amd.com>
> >
> > In preparation for CXL VH (Virtual Host) topology protocol error handling,
> > add RAS capability registered mapping for all ports in a CXL VH topology.
> > This includes the RAS capabilities of Switch Upstream Ports, Switch
> > Downstream Ports, Host Bridge Ports ("upstream"), and Root Ports
> > ("downstream")
> >
> > Update cxl_port_add_dport() to map the upstream RAS capability on first
> > 'dport' attach.
>
> I'm struggling to see how this maps the endpoint registers (or at least
> how it changes that). So maybe the patch title has become incorrect?
It is time travelling a bit. The new / common devm_cxl_port_ras_setup() helper is
going to be reused for endpoint RAS register mapping in a subsequent
patch. So this can be renamed:
cxl/port: Map Port RAS registers
...and clarify that Switch Ports are covered in this patch and a later
patch adds Endpoint support.
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 8/9] cxl/port: Move endpoint component register management to cxl_port
2026-01-22 3:33 [PATCH 0/9] cxl/port: Unify RAS setup across port types Dan Williams
` (6 preceding siblings ...)
2026-01-22 3:33 ` [PATCH 7/9] cxl/port: Map CXL Endpoint Port and CXL Switch Port RAS registers Dan Williams
@ 2026-01-22 3:33 ` Dan Williams
2026-01-22 15:27 ` Jonathan Cameron
2026-01-22 21:24 ` Dave Jiang
2026-01-22 3:33 ` [PATCH 9/9] cxl/port: Unify endpoint and switch port lookup Dan Williams
2026-01-22 21:42 ` [PATCH 0/9] cxl/port: Unify RAS setup across port types Bowman, Terry
9 siblings, 2 replies; 36+ messages in thread
From: Dan Williams @ 2026-01-22 3:33 UTC (permalink / raw)
To: linux-cxl
Cc: jonathan.cameron, dave, dave.jiang, alison.schofield, ira.weiny,
terry.bowman
In preparation for generic protocol error handling across CXL endpoints,
whether they be memory expander class devices or accelerators, drop the
endpoint component management from cxl_dev_state.
Organize all CXL port component management through the common cxl_port
driver.
Note that the end game is that drivers/cxl/core/ras.c loses all
dependencies on a 'struct cxl_dev_state' parameter and operates only on
port resources. The removal of component register mapping from cxl_pci is
an incremental step towards that.
Reviewed-by: Terry Bowman <terry.bowman@amd.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
drivers/cxl/cxlmem.h | 4 +--
drivers/cxl/core/ras.c | 6 ++--
drivers/cxl/pci.c | 63 +-----------------------------------------
drivers/cxl/port.c | 54 ++++++++++++++++++++++++++++++++++++
4 files changed, 60 insertions(+), 67 deletions(-)
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index 434031a0c1f7..ab7201ef3ea6 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -415,7 +415,7 @@ struct cxl_dpa_partition {
* @dev: The device associated with this CXL state
* @cxlmd: The device representing the CXL.mem capabilities of @dev
* @reg_map: component and ras register mapping parameters
- * @regs: Parsed register blocks
+ * @regs: Class device "Device" registers
* @cxl_dvsec: Offset to the PCIe device DVSEC
* @rcd: operating in RCD mode (CXL 3.0 9.11.8 CXL Devices Attached to an RCH)
* @media_ready: Indicate whether the device media is usable
@@ -431,7 +431,7 @@ struct cxl_dev_state {
struct device *dev;
struct cxl_memdev *cxlmd;
struct cxl_register_map reg_map;
- struct cxl_regs regs;
+ struct cxl_device_regs regs;
int cxl_dvsec;
bool rcd;
bool media_ready;
diff --git a/drivers/cxl/core/ras.c b/drivers/cxl/core/ras.c
index b4be9c5715a6..f6a8f4a355f1 100644
--- a/drivers/cxl/core/ras.c
+++ b/drivers/cxl/core/ras.c
@@ -255,6 +255,7 @@ bool cxl_handle_ras(struct device *dev, void __iomem *ras_base)
void cxl_cor_error_detected(struct pci_dev *pdev)
{
struct cxl_dev_state *cxlds = pci_get_drvdata(pdev);
+ struct cxl_memdev *cxlmd = cxlds->cxlmd;
struct device *dev = &cxlds->cxlmd->dev;
scoped_guard(device, dev) {
@@ -268,7 +269,7 @@ void cxl_cor_error_detected(struct pci_dev *pdev)
if (cxlds->rcd)
cxl_handle_rdport_errors(cxlds);
- cxl_handle_cor_ras(&cxlds->cxlmd->dev, cxlds->regs.ras);
+ cxl_handle_cor_ras(&cxlds->cxlmd->dev, cxlmd->endpoint->regs.ras);
}
}
EXPORT_SYMBOL_NS_GPL(cxl_cor_error_detected, "CXL");
@@ -297,10 +298,9 @@ pci_ers_result_t cxl_error_detected(struct pci_dev *pdev,
* chance the situation is recoverable dump the status of the RAS
* capability registers and bounce the active state of the memdev.
*/
- ue = cxl_handle_ras(&cxlds->cxlmd->dev, cxlds->regs.ras);
+ ue = cxl_handle_ras(&cxlds->cxlmd->dev, cxlmd->endpoint->regs.ras);
}
-
switch (state) {
case pci_channel_io_normal:
if (ue) {
diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index b7f694bda913..acb0eb2a13c3 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -535,52 +535,6 @@ static int cxl_pci_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type type,
return cxl_setup_regs(map);
}
-static int cxl_pci_ras_unmask(struct pci_dev *pdev)
-{
- struct cxl_dev_state *cxlds = pci_get_drvdata(pdev);
- void __iomem *addr;
- u32 orig_val, val, mask;
- u16 cap;
- int rc;
-
- if (!cxlds->regs.ras) {
- dev_dbg(&pdev->dev, "No RAS registers.\n");
- return 0;
- }
-
- /* BIOS has PCIe AER error control */
- if (!pcie_aer_is_native(pdev))
- return 0;
-
- rc = pcie_capability_read_word(pdev, PCI_EXP_DEVCTL, &cap);
- if (rc)
- return rc;
-
- if (cap & PCI_EXP_DEVCTL_URRE) {
- addr = cxlds->regs.ras + CXL_RAS_UNCORRECTABLE_MASK_OFFSET;
- orig_val = readl(addr);
-
- mask = CXL_RAS_UNCORRECTABLE_MASK_MASK |
- CXL_RAS_UNCORRECTABLE_MASK_F256B_MASK;
- val = orig_val & ~mask;
- writel(val, addr);
- dev_dbg(&pdev->dev,
- "Uncorrectable RAS Errors Mask: %#x -> %#x\n",
- orig_val, val);
- }
-
- if (cap & PCI_EXP_DEVCTL_CERE) {
- addr = cxlds->regs.ras + CXL_RAS_CORRECTABLE_MASK_OFFSET;
- orig_val = readl(addr);
- val = orig_val & ~CXL_RAS_CORRECTABLE_MASK_MASK;
- writel(val, addr);
- dev_dbg(&pdev->dev, "Correctable RAS Errors Mask: %#x -> %#x\n",
- orig_val, val);
- }
-
- return 0;
-}
-
static void free_event_buf(void *buf)
{
kvfree(buf);
@@ -912,13 +866,6 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
unsigned int i;
bool irq_avail;
- /*
- * Double check the anonymous union trickery in struct cxl_regs
- * FIXME switch to struct_group()
- */
- BUILD_BUG_ON(offsetof(struct cxl_regs, memdev) !=
- offsetof(struct cxl_regs, device_regs.memdev));
-
rc = pcim_enable_device(pdev);
if (rc)
return rc;
@@ -942,7 +889,7 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
if (rc)
return rc;
- rc = cxl_map_device_regs(&map, &cxlds->regs.device_regs);
+ rc = cxl_map_device_regs(&map, &cxlds->regs);
if (rc)
return rc;
@@ -957,11 +904,6 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
else if (!cxlds->reg_map.component_map.ras.valid)
dev_dbg(&pdev->dev, "RAS registers not found\n");
- rc = cxl_map_component_regs(&cxlds->reg_map, &cxlds->regs.component,
- BIT(CXL_CM_CAP_CAP_ID_RAS));
- if (rc)
- dev_dbg(&pdev->dev, "Failed to map RAS capability.\n");
-
rc = cxl_pci_type3_init_mailbox(cxlds);
if (rc)
return rc;
@@ -1052,9 +994,6 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
if (rc)
return rc;
- if (cxl_pci_ras_unmask(pdev))
- dev_dbg(&pdev->dev, "No RAS reporting unmasked\n");
-
pci_save_state(pdev);
return rc;
diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
index 50a643cb3e04..57b01f3bb750 100644
--- a/drivers/cxl/port.c
+++ b/drivers/cxl/port.c
@@ -1,5 +1,6 @@
// SPDX-License-Identifier: GPL-2.0-only
/* Copyright(c) 2022 Intel Corporation. All rights reserved. */
+#include <linux/aer.h>
#include <linux/device.h>
#include <linux/module.h>
#include <linux/slab.h>
@@ -68,6 +69,55 @@ static int cxl_switch_port_probe(struct cxl_port *port)
return 0;
}
+static int cxl_ras_unmask(struct cxl_port *port)
+{
+ struct pci_dev *pdev;
+ void __iomem *addr;
+ u32 orig_val, val, mask;
+ u16 cap;
+ int rc;
+
+ if (!dev_is_pci(port->uport_dev))
+ return 0;
+ pdev = to_pci_dev(port->uport_dev);
+
+ if (!port->regs.ras) {
+ pci_dbg(pdev, "No RAS registers.\n");
+ return 0;
+ }
+
+ /* BIOS has PCIe AER error control */
+ if (!pcie_aer_is_native(pdev))
+ return 0;
+
+ rc = pcie_capability_read_word(pdev, PCI_EXP_DEVCTL, &cap);
+ if (rc)
+ return rc;
+
+ if (cap & PCI_EXP_DEVCTL_URRE) {
+ addr = port->regs.ras + CXL_RAS_UNCORRECTABLE_MASK_OFFSET;
+ orig_val = readl(addr);
+
+ mask = CXL_RAS_UNCORRECTABLE_MASK_MASK |
+ CXL_RAS_UNCORRECTABLE_MASK_F256B_MASK;
+ val = orig_val & ~mask;
+ writel(val, addr);
+ pci_dbg(pdev, "Uncorrectable RAS Errors Mask: %#x -> %#x\n",
+ orig_val, val);
+ }
+
+ if (cap & PCI_EXP_DEVCTL_CERE) {
+ addr = port->regs.ras + CXL_RAS_CORRECTABLE_MASK_OFFSET;
+ orig_val = readl(addr);
+ val = orig_val & ~CXL_RAS_CORRECTABLE_MASK_MASK;
+ writel(val, addr);
+ pci_dbg(pdev, "Correctable RAS Errors Mask: %#x -> %#x\n",
+ orig_val, val);
+ }
+
+ return 0;
+}
+
static int cxl_endpoint_port_probe(struct cxl_port *port)
{
struct cxl_memdev *cxlmd = to_cxl_memdev(port->uport_dev);
@@ -98,6 +148,10 @@ static int cxl_endpoint_port_probe(struct cxl_port *port)
if (dport->rch)
devm_cxl_dport_rch_ras_setup(dport);
+ devm_cxl_port_ras_setup(port);
+ if (cxl_ras_unmask(port))
+ dev_dbg(&port->dev, "failed to unmask RAS interrupts\n");
+
/*
* Now that all endpoint decoders are successfully enumerated, try to
* assemble regions from committed decoders
--
2.52.0
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH 8/9] cxl/port: Move endpoint component register management to cxl_port
2026-01-22 3:33 ` [PATCH 8/9] cxl/port: Move endpoint component register management to cxl_port Dan Williams
@ 2026-01-22 15:27 ` Jonathan Cameron
2026-01-22 21:24 ` Dave Jiang
1 sibling, 0 replies; 36+ messages in thread
From: Jonathan Cameron @ 2026-01-22 15:27 UTC (permalink / raw)
To: Dan Williams
Cc: linux-cxl, dave, dave.jiang, alison.schofield, ira.weiny,
terry.bowman
On Wed, 21 Jan 2026 19:33:29 -0800
Dan Williams <dan.j.williams@intel.com> wrote:
> In preparation for generic protocol error handling across CXL endpoints,
> whether they be memory expander class devices or accelerators, drop the
> endpoint component management from cxl_dev_state.
>
> Organize all CXL port component management through the common cxl_port
> driver.
>
> Note that the end game is that drivers/cxl/core/ras.c loses all
> dependencies on a 'struct cxl_dev_state' parameter and operates only on
> port resources. The removal of component register mapping from cxl_pci is
> an incremental step towards that.
>
> Reviewed-by: Terry Bowman <terry.bowman@amd.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 8/9] cxl/port: Move endpoint component register management to cxl_port
2026-01-22 3:33 ` [PATCH 8/9] cxl/port: Move endpoint component register management to cxl_port Dan Williams
2026-01-22 15:27 ` Jonathan Cameron
@ 2026-01-22 21:24 ` Dave Jiang
1 sibling, 0 replies; 36+ messages in thread
From: Dave Jiang @ 2026-01-22 21:24 UTC (permalink / raw)
To: Dan Williams, linux-cxl
Cc: jonathan.cameron, dave, alison.schofield, ira.weiny, terry.bowman
On 1/21/26 8:33 PM, Dan Williams wrote:
> In preparation for generic protocol error handling across CXL endpoints,
> whether they be memory expander class devices or accelerators, drop the
> endpoint component management from cxl_dev_state.
>
> Organize all CXL port component management through the common cxl_port
> driver.
>
> Note that the end game is that drivers/cxl/core/ras.c loses all
> dependencies on a 'struct cxl_dev_state' parameter and operates only on
> port resources. The removal of component register mapping from cxl_pci is
> an incremental step towards that.
>
> Reviewed-by: Terry Bowman <terry.bowman@amd.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> ---
> drivers/cxl/cxlmem.h | 4 +--
> drivers/cxl/core/ras.c | 6 ++--
> drivers/cxl/pci.c | 63 +-----------------------------------------
> drivers/cxl/port.c | 54 ++++++++++++++++++++++++++++++++++++
> 4 files changed, 60 insertions(+), 67 deletions(-)
>
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index 434031a0c1f7..ab7201ef3ea6 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -415,7 +415,7 @@ struct cxl_dpa_partition {
> * @dev: The device associated with this CXL state
> * @cxlmd: The device representing the CXL.mem capabilities of @dev
> * @reg_map: component and ras register mapping parameters
> - * @regs: Parsed register blocks
> + * @regs: Class device "Device" registers
> * @cxl_dvsec: Offset to the PCIe device DVSEC
> * @rcd: operating in RCD mode (CXL 3.0 9.11.8 CXL Devices Attached to an RCH)
> * @media_ready: Indicate whether the device media is usable
> @@ -431,7 +431,7 @@ struct cxl_dev_state {
> struct device *dev;
> struct cxl_memdev *cxlmd;
> struct cxl_register_map reg_map;
> - struct cxl_regs regs;
> + struct cxl_device_regs regs;
> int cxl_dvsec;
> bool rcd;
> bool media_ready;
> diff --git a/drivers/cxl/core/ras.c b/drivers/cxl/core/ras.c
> index b4be9c5715a6..f6a8f4a355f1 100644
> --- a/drivers/cxl/core/ras.c
> +++ b/drivers/cxl/core/ras.c
> @@ -255,6 +255,7 @@ bool cxl_handle_ras(struct device *dev, void __iomem *ras_base)
> void cxl_cor_error_detected(struct pci_dev *pdev)
> {
> struct cxl_dev_state *cxlds = pci_get_drvdata(pdev);
> + struct cxl_memdev *cxlmd = cxlds->cxlmd;
> struct device *dev = &cxlds->cxlmd->dev;
>
> scoped_guard(device, dev) {
> @@ -268,7 +269,7 @@ void cxl_cor_error_detected(struct pci_dev *pdev)
> if (cxlds->rcd)
> cxl_handle_rdport_errors(cxlds);
>
> - cxl_handle_cor_ras(&cxlds->cxlmd->dev, cxlds->regs.ras);
> + cxl_handle_cor_ras(&cxlds->cxlmd->dev, cxlmd->endpoint->regs.ras);
> }
> }
> EXPORT_SYMBOL_NS_GPL(cxl_cor_error_detected, "CXL");
> @@ -297,10 +298,9 @@ pci_ers_result_t cxl_error_detected(struct pci_dev *pdev,
> * chance the situation is recoverable dump the status of the RAS
> * capability registers and bounce the active state of the memdev.
> */
> - ue = cxl_handle_ras(&cxlds->cxlmd->dev, cxlds->regs.ras);
> + ue = cxl_handle_ras(&cxlds->cxlmd->dev, cxlmd->endpoint->regs.ras);
> }
>
> -
> switch (state) {
> case pci_channel_io_normal:
> if (ue) {
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index b7f694bda913..acb0eb2a13c3 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -535,52 +535,6 @@ static int cxl_pci_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type type,
> return cxl_setup_regs(map);
> }
>
> -static int cxl_pci_ras_unmask(struct pci_dev *pdev)
> -{
> - struct cxl_dev_state *cxlds = pci_get_drvdata(pdev);
> - void __iomem *addr;
> - u32 orig_val, val, mask;
> - u16 cap;
> - int rc;
> -
> - if (!cxlds->regs.ras) {
> - dev_dbg(&pdev->dev, "No RAS registers.\n");
> - return 0;
> - }
> -
> - /* BIOS has PCIe AER error control */
> - if (!pcie_aer_is_native(pdev))
> - return 0;
> -
> - rc = pcie_capability_read_word(pdev, PCI_EXP_DEVCTL, &cap);
> - if (rc)
> - return rc;
> -
> - if (cap & PCI_EXP_DEVCTL_URRE) {
> - addr = cxlds->regs.ras + CXL_RAS_UNCORRECTABLE_MASK_OFFSET;
> - orig_val = readl(addr);
> -
> - mask = CXL_RAS_UNCORRECTABLE_MASK_MASK |
> - CXL_RAS_UNCORRECTABLE_MASK_F256B_MASK;
> - val = orig_val & ~mask;
> - writel(val, addr);
> - dev_dbg(&pdev->dev,
> - "Uncorrectable RAS Errors Mask: %#x -> %#x\n",
> - orig_val, val);
> - }
> -
> - if (cap & PCI_EXP_DEVCTL_CERE) {
> - addr = cxlds->regs.ras + CXL_RAS_CORRECTABLE_MASK_OFFSET;
> - orig_val = readl(addr);
> - val = orig_val & ~CXL_RAS_CORRECTABLE_MASK_MASK;
> - writel(val, addr);
> - dev_dbg(&pdev->dev, "Correctable RAS Errors Mask: %#x -> %#x\n",
> - orig_val, val);
> - }
> -
> - return 0;
> -}
> -
> static void free_event_buf(void *buf)
> {
> kvfree(buf);
> @@ -912,13 +866,6 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> unsigned int i;
> bool irq_avail;
>
> - /*
> - * Double check the anonymous union trickery in struct cxl_regs
> - * FIXME switch to struct_group()
> - */
> - BUILD_BUG_ON(offsetof(struct cxl_regs, memdev) !=
> - offsetof(struct cxl_regs, device_regs.memdev));
> -
> rc = pcim_enable_device(pdev);
> if (rc)
> return rc;
> @@ -942,7 +889,7 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> if (rc)
> return rc;
>
> - rc = cxl_map_device_regs(&map, &cxlds->regs.device_regs);
> + rc = cxl_map_device_regs(&map, &cxlds->regs);
> if (rc)
> return rc;
>
> @@ -957,11 +904,6 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> else if (!cxlds->reg_map.component_map.ras.valid)
> dev_dbg(&pdev->dev, "RAS registers not found\n");
>
> - rc = cxl_map_component_regs(&cxlds->reg_map, &cxlds->regs.component,
> - BIT(CXL_CM_CAP_CAP_ID_RAS));
> - if (rc)
> - dev_dbg(&pdev->dev, "Failed to map RAS capability.\n");
> -
> rc = cxl_pci_type3_init_mailbox(cxlds);
> if (rc)
> return rc;
> @@ -1052,9 +994,6 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> if (rc)
> return rc;
>
> - if (cxl_pci_ras_unmask(pdev))
> - dev_dbg(&pdev->dev, "No RAS reporting unmasked\n");
> -
> pci_save_state(pdev);
>
> return rc;
> diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
> index 50a643cb3e04..57b01f3bb750 100644
> --- a/drivers/cxl/port.c
> +++ b/drivers/cxl/port.c
> @@ -1,5 +1,6 @@
> // SPDX-License-Identifier: GPL-2.0-only
> /* Copyright(c) 2022 Intel Corporation. All rights reserved. */
> +#include <linux/aer.h>
> #include <linux/device.h>
> #include <linux/module.h>
> #include <linux/slab.h>
> @@ -68,6 +69,55 @@ static int cxl_switch_port_probe(struct cxl_port *port)
> return 0;
> }
>
> +static int cxl_ras_unmask(struct cxl_port *port)
> +{
> + struct pci_dev *pdev;
> + void __iomem *addr;
> + u32 orig_val, val, mask;
> + u16 cap;
> + int rc;
> +
> + if (!dev_is_pci(port->uport_dev))
> + return 0;
> + pdev = to_pci_dev(port->uport_dev);
> +
> + if (!port->regs.ras) {
> + pci_dbg(pdev, "No RAS registers.\n");
> + return 0;
> + }
> +
> + /* BIOS has PCIe AER error control */
> + if (!pcie_aer_is_native(pdev))
> + return 0;
> +
> + rc = pcie_capability_read_word(pdev, PCI_EXP_DEVCTL, &cap);
> + if (rc)
> + return rc;
> +
> + if (cap & PCI_EXP_DEVCTL_URRE) {
> + addr = port->regs.ras + CXL_RAS_UNCORRECTABLE_MASK_OFFSET;
> + orig_val = readl(addr);
> +
> + mask = CXL_RAS_UNCORRECTABLE_MASK_MASK |
> + CXL_RAS_UNCORRECTABLE_MASK_F256B_MASK;
> + val = orig_val & ~mask;
> + writel(val, addr);
> + pci_dbg(pdev, "Uncorrectable RAS Errors Mask: %#x -> %#x\n",
> + orig_val, val);
> + }
> +
> + if (cap & PCI_EXP_DEVCTL_CERE) {
> + addr = port->regs.ras + CXL_RAS_CORRECTABLE_MASK_OFFSET;
> + orig_val = readl(addr);
> + val = orig_val & ~CXL_RAS_CORRECTABLE_MASK_MASK;
> + writel(val, addr);
> + pci_dbg(pdev, "Correctable RAS Errors Mask: %#x -> %#x\n",
> + orig_val, val);
> + }
> +
> + return 0;
> +}
> +
> static int cxl_endpoint_port_probe(struct cxl_port *port)
> {
> struct cxl_memdev *cxlmd = to_cxl_memdev(port->uport_dev);
> @@ -98,6 +148,10 @@ static int cxl_endpoint_port_probe(struct cxl_port *port)
> if (dport->rch)
> devm_cxl_dport_rch_ras_setup(dport);
>
> + devm_cxl_port_ras_setup(port);
> + if (cxl_ras_unmask(port))
> + dev_dbg(&port->dev, "failed to unmask RAS interrupts\n");
> +
> /*
> * Now that all endpoint decoders are successfully enumerated, try to
> * assemble regions from committed decoders
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 9/9] cxl/port: Unify endpoint and switch port lookup
2026-01-22 3:33 [PATCH 0/9] cxl/port: Unify RAS setup across port types Dan Williams
` (7 preceding siblings ...)
2026-01-22 3:33 ` [PATCH 8/9] cxl/port: Move endpoint component register management to cxl_port Dan Williams
@ 2026-01-22 3:33 ` Dan Williams
2026-01-22 15:32 ` Jonathan Cameron
2026-01-22 21:24 ` Dave Jiang
2026-01-22 21:42 ` [PATCH 0/9] cxl/port: Unify RAS setup across port types Bowman, Terry
9 siblings, 2 replies; 36+ messages in thread
From: Dan Williams @ 2026-01-22 3:33 UTC (permalink / raw)
To: linux-cxl
Cc: jonathan.cameron, dave, dave.jiang, alison.schofield, ira.weiny,
terry.bowman
In support of generic CXL protocol error handling across various 'struct
cxl_port' types, update find_cxl_port_by_uport() to retrieve endpoint CXL
port companions from endpoint PCIe device instances.
The end result is that upstream switch ports and endpoint ports can share
error handling and eventually delete the misplaced cxl_error_handlers from
the cxl_pci class driver.
Reviewed-by: Terry Bowman <terry.bowman@amd.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
drivers/cxl/core/port.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index 436d9f8f65cb..446a97d48423 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -1591,10 +1591,20 @@ static int match_port_by_uport(struct device *dev, const void *data)
return 0;
port = to_cxl_port(dev);
+ /* Endpoint ports are hosted by memdevs */
+ if (is_cxl_memdev(port->uport_dev))
+ return uport_dev == port->uport_dev->parent;
return uport_dev == port->uport_dev;
}
-/*
+/**
+ * find_cxl_port_by_uport - Find a CXL port device companion
+ * @uport_dev: Device that acts as a switch or endpoint in the CXL hierarchy
+ *
+ * In the case of endpoint ports recall that port->uport_dev points to a 'struct
+ * cxl_memdev' device. So, the @uport_dev argument is the parent device of the
+ * 'struct cxl_memdev' in that case.
+ *
* Function takes a device reference on the port device. Caller should do a
* put_device() when done.
*/
--
2.52.0
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH 9/9] cxl/port: Unify endpoint and switch port lookup
2026-01-22 3:33 ` [PATCH 9/9] cxl/port: Unify endpoint and switch port lookup Dan Williams
@ 2026-01-22 15:32 ` Jonathan Cameron
2026-01-22 21:24 ` Dave Jiang
1 sibling, 0 replies; 36+ messages in thread
From: Jonathan Cameron @ 2026-01-22 15:32 UTC (permalink / raw)
To: Dan Williams
Cc: linux-cxl, dave, dave.jiang, alison.schofield, ira.weiny,
terry.bowman
On Wed, 21 Jan 2026 19:33:30 -0800
Dan Williams <dan.j.williams@intel.com> wrote:
> In support of generic CXL protocol error handling across various 'struct
> cxl_port' types, update find_cxl_port_by_uport() to retrieve endpoint CXL
> port companions from endpoint PCIe device instances.
>
> The end result is that upstream switch ports and endpoint ports can share
> error handling and eventually delete the misplaced cxl_error_handlers from
> the cxl_pci class driver.
>
> Reviewed-by: Terry Bowman <terry.bowman@amd.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 9/9] cxl/port: Unify endpoint and switch port lookup
2026-01-22 3:33 ` [PATCH 9/9] cxl/port: Unify endpoint and switch port lookup Dan Williams
2026-01-22 15:32 ` Jonathan Cameron
@ 2026-01-22 21:24 ` Dave Jiang
1 sibling, 0 replies; 36+ messages in thread
From: Dave Jiang @ 2026-01-22 21:24 UTC (permalink / raw)
To: Dan Williams, linux-cxl
Cc: jonathan.cameron, dave, alison.schofield, ira.weiny, terry.bowman
On 1/21/26 8:33 PM, Dan Williams wrote:
> In support of generic CXL protocol error handling across various 'struct
> cxl_port' types, update find_cxl_port_by_uport() to retrieve endpoint CXL
> port companions from endpoint PCIe device instances.
>
> The end result is that upstream switch ports and endpoint ports can share
> error handling and eventually delete the misplaced cxl_error_handlers from
> the cxl_pci class driver.
>
> Reviewed-by: Terry Bowman <terry.bowman@amd.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> ---
> drivers/cxl/core/port.c | 12 +++++++++++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 436d9f8f65cb..446a97d48423 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -1591,10 +1591,20 @@ static int match_port_by_uport(struct device *dev, const void *data)
> return 0;
>
> port = to_cxl_port(dev);
> + /* Endpoint ports are hosted by memdevs */
> + if (is_cxl_memdev(port->uport_dev))
> + return uport_dev == port->uport_dev->parent;
> return uport_dev == port->uport_dev;
> }
>
> -/*
> +/**
> + * find_cxl_port_by_uport - Find a CXL port device companion
> + * @uport_dev: Device that acts as a switch or endpoint in the CXL hierarchy
> + *
> + * In the case of endpoint ports recall that port->uport_dev points to a 'struct
> + * cxl_memdev' device. So, the @uport_dev argument is the parent device of the
> + * 'struct cxl_memdev' in that case.
> + *
> * Function takes a device reference on the port device. Caller should do a
> * put_device() when done.
> */
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 0/9] cxl/port: Unify RAS setup across port types
2026-01-22 3:33 [PATCH 0/9] cxl/port: Unify RAS setup across port types Dan Williams
` (8 preceding siblings ...)
2026-01-22 3:33 ` [PATCH 9/9] cxl/port: Unify endpoint and switch port lookup Dan Williams
@ 2026-01-22 21:42 ` Bowman, Terry
9 siblings, 0 replies; 36+ messages in thread
From: Bowman, Terry @ 2026-01-22 21:42 UTC (permalink / raw)
To: Dan Williams, linux-cxl
Cc: jonathan.cameron, dave, dave.jiang, alison.schofield, ira.weiny
On 1/21/2026 9:33 PM, Dan Williams wrote:
> The CXL Port Protocol error handling series grew to be over 30 patches
> which is too much to handle at once given the various topics involved.
> One of the sub-threads of the v14 review was confusion about the new
> devres groups to manage port setup unwind failures [1].
>
> [1]: http://lore.kernel.org/20260115144605.00000666@huawei.com
>
> Given that review indicated a need to break up and better explain the
> conversion, do that in a separate patch set. Build on top of the first
> 18 patches of that series [2] that are ready to merge (save one missing
> ack from Bjorn).
>
> [2]: https://git.kernel.org/pub/scm/linux/kernel/git/cxl/cxl.git/log/?h=for-7.0/cxl-aer-prep
>
> The wider goals of the port protocol handling series are:
>
> 1/ Be minimally invasive to the ongoing maintenance burden of PCIe error
> handling. Just do the minimal enlightenment to forward "internal"
> errors for device with active CXL links to the CXL core.
>
> 2/ Build a framework for any driver that registers a 'struct cxl_memdev'
> (or in the future a 'struct cxl_cachedev') gets protocol error
> handling support.
>
> This "Unify RAS setup across port types" set supports goal 2/. It
> enables a model where all CXL error handling is relative to the common
> 'struct cxl_port' and 'struct cxl_dport' objects and is agnostic to
> whether those objects are in support of the memory expansion class
> device (driven by cxl_pci) or any other CXL endpoint in the system that
> supports CXL.cachemem operation.
>
> In support of that unification, the setup of RAS registers needs to be
> centralized. That in turn requires new handling for early exit setup
> failures and additional teardown support for resources optionally
> acquired at port / dport creation time.
>
> The devres group mechanism is deployed to cleanup some open coded
> devm_release_action() calls. The devres group facility also comes in
> handy for unwinding conditional setup steps in the port creation
> process. Recall that ports defer probing their CXL resources until after
> they are known to have a downstream CXL connection. So, early exit during
> setup of a new dport may have more or less work to do depending on
> whether the first or subsequent dport is being added.
>
> Given probing port resources is a 'probe' action it fits more naturally
> as a driver operation. If cxl_port_add_dport() then moves to cxl_port
> driver operation alongside ->probe(), it enables a cxl_test cleanup. The
> cxl_test approach has a hard time mocking interfaces that are internal
> to the cxl_core.
>
> The rest of the patches in this set finish off the conversion of 'struct
> cxl_port' and 'struct cxl_dport' to be the only CXL objects that
> interact with the CXL RAS.
>
> Dan Williams (8):
> cxl/port: Cleanup handling of the nr_dports 0 -> 1 transition
> cxl/port: Reduce number of @dport variables in cxl_port_add_dport()
> cxl/port: Cleanup dport removal with a devres group
> cxl/port: Move decoder setup before dport creation
> cxl/port: Move dport probe operations to a driver event
> cxl/port: Move dport RAS setup to dport add time
> cxl/port: Move endpoint component register management to cxl_port
> cxl/port: Unify endpoint and switch port lookup
>
> Terry Bowman (1):
> cxl/port: Map CXL Endpoint Port and CXL Switch Port RAS registers
>
> drivers/cxl/core/core.h | 8 ++
> drivers/cxl/cxl.h | 27 ++---
> drivers/cxl/cxlmem.h | 4 +-
> drivers/cxl/cxlpci.h | 12 ++-
> tools/testing/cxl/exports.h | 13 ---
> drivers/cxl/core/hdm.c | 6 +-
> drivers/cxl/core/pci.c | 8 +-
> drivers/cxl/core/port.c | 153 +++++++++++++++++----------
> drivers/cxl/core/ras.c | 50 ++++++---
> drivers/cxl/mem.c | 2 -
> drivers/cxl/pci.c | 63 +----------
> drivers/cxl/port.c | 132 +++++++++++++++++++++++
> tools/testing/cxl/cxl_core_exports.c | 22 ----
> tools/testing/cxl/test/mock.c | 36 ++-----
> tools/testing/cxl/Kbuild | 3 +-
> 15 files changed, 310 insertions(+), 229 deletions(-)
> delete mode 100644 tools/testing/cxl/exports.h
>
>
> base-commit: c61e99a20e7390bf8727a3b2cacbc00931b05d0b
I rebased this series onto the remaining CXL error handling patches to test.
I injected CXL protocol errors using aer inject for AER internal errors and
emulated RAS errors. The tested devices include CXL RP, USP, DSP, and EPs.
Error injection testing passed with no issues. You can add my tested by if
you like:
Tested-by: Terry Bowman <terry.bowman@amd.com>
-Terry
^ permalink raw reply [flat|nested] 36+ messages in thread