* Re: [PATCH v2] cxl/region: Unregister the pmem region bridge on setup failure
2026-08-12 6:10 [PATCH v2] cxl/region: Unregister the pmem region bridge on setup failure Guixin Liu
@ 2026-08-12 7:58 ` Richard Cheng
2026-08-12 11:57 ` Li Ming
2026-08-12 21:42 ` Alison Schofield
2 siblings, 0 replies; 6+ messages in thread
From: Richard Cheng @ 2026-08-12 7:58 UTC (permalink / raw)
To: Guixin Liu
Cc: Davidlohr Bueso, Jonathan Cameron, Dave Jiang, Alison Schofield,
Vishal Verma, Dan Williams, Ira Weiny, Li Ming, linux-cxl
On Wed, Aug 12, 2026 at 02:10:43PM +0800, Guixin Liu wrote:
> devm_cxl_add_pmem_region() publishes the cxl_pmem_region with device_add()
> and only afterwards, under the nvdimm bridge's device lock, arranges for
> its removal - and only if the bridge has a driver bound. If it does not,
> the function sets -ENXIO and leaves through err_bridge, which drops the
> reference this function took on the bridge and returns. The device that
> was just added has no owner at that point: no device_del(), no
> put_device(), and no devm action to do either later. The sibling failure,
> a devm_add_action_or_reset() that cannot allocate, is already covered,
> because devm_add_action_or_reset() runs cxlr_pmem_unregister() itself on
> that path.
>
> An unbound bridge is a normal state, not an error state. The bridge is
> unbound whenever cxl_pmem is unloaded or its device is detached through
> sysfs, and a region can be probed in that window.
>
> The added device then stays in sysfs, along with the reference it holds on
> the region, until the module is unloaded. cxlr->cxlr_pmem still points at
> it, and worse, the name is still taken: a later probe of the same region
> allocates a second cxl_pmem_region and fails in device_add() on the
> duplicate "pmem_region%d", so once this has happened the region can no
> longer be brought up at all.
>
> Call cxlr_pmem_unregister() on that branch. It is invoked from inside the
> scoped_guard() that holds the bridge's device lock, which is what its
> device_lock_assert() requires, and it performs the same teardown the devm
> action would have performed, including clearing cxlr->cxlr_pmem, so
> err_bridge is left with only the bridge reference to drop.
>
> Fixes: f17b558d6663 ("cxl/pmem: Refactor nvdimm device registration, delete the workqueue")
> Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
> ---
> This was patch 8/8 of the "cxl: Assorted fixes" series [1]. Per review
> feedback that series is not being reworked as a whole; the fixes are resent
> individually instead. Patches 1, 2 and 7 of the series are dropped, as those
> issues are already fixed in cxl/next.
>
> v1->v2:
> - rebase onto cxl/next
> - rewrite the commit message to describe the behaviour rather than narrate
> the code change (Alison Schofield)
>
> [1] https://lore.kernel.org/linux-cxl/20260811113608.2815625-1-kanie@linux.alibaba.com/
>
> drivers/cxl/core/region_pmem.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/cxl/core/region_pmem.c b/drivers/cxl/core/region_pmem.c
> index 23d97e3d78b6..7ab1373a95e0 100644
> --- a/drivers/cxl/core/region_pmem.c
> +++ b/drivers/cxl/core/region_pmem.c
> @@ -168,12 +168,14 @@ int devm_cxl_add_pmem_region(struct cxl_region *cxlr)
> dev_name(dev));
>
> scoped_guard(device, &cxl_nvb->dev) {
> - if (cxl_nvb->dev.driver)
> + if (cxl_nvb->dev.driver) {
> rc = devm_add_action_or_reset(&cxl_nvb->dev,
> cxlr_pmem_unregister,
> cxlr_pmem);
> - else
> + } else {
> rc = -ENXIO;
> + cxlr_pmem_unregister(cxlr_pmem);
> + }
> }
>
> if (rc)
>
> base-commit: 7098e9cd98a05c0c5de2fae0c2465f9d966fdd07
> --
> 2.43.7
>
>
Reviewed-by: Richard Cheng <icheng@nvidia.com>
Best regards,
Richard Cheng
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] cxl/region: Unregister the pmem region bridge on setup failure
2026-08-12 6:10 [PATCH v2] cxl/region: Unregister the pmem region bridge on setup failure Guixin Liu
2026-08-12 7:58 ` Richard Cheng
@ 2026-08-12 11:57 ` Li Ming
2026-08-12 21:42 ` Alison Schofield
2 siblings, 0 replies; 6+ messages in thread
From: Li Ming @ 2026-08-12 11:57 UTC (permalink / raw)
To: Guixin Liu, Davidlohr Bueso, Jonathan Cameron, Dave Jiang,
Alison Schofield, Vishal Verma, Dan Williams, Ira Weiny
Cc: linux-cxl
在 2026/8/12 14:10, Guixin Liu 写道:
> devm_cxl_add_pmem_region() publishes the cxl_pmem_region with device_add()
> and only afterwards, under the nvdimm bridge's device lock, arranges for
> its removal - and only if the bridge has a driver bound. If it does not,
> the function sets -ENXIO and leaves through err_bridge, which drops the
> reference this function took on the bridge and returns. The device that
> was just added has no owner at that point: no device_del(), no
> put_device(), and no devm action to do either later. The sibling failure,
> a devm_add_action_or_reset() that cannot allocate, is already covered,
> because devm_add_action_or_reset() runs cxlr_pmem_unregister() itself on
> that path.
>
> An unbound bridge is a normal state, not an error state. The bridge is
> unbound whenever cxl_pmem is unloaded or its device is detached through
> sysfs, and a region can be probed in that window.
>
> The added device then stays in sysfs, along with the reference it holds on
> the region, until the module is unloaded. cxlr->cxlr_pmem still points at
> it, and worse, the name is still taken: a later probe of the same region
> allocates a second cxl_pmem_region and fails in device_add() on the
> duplicate "pmem_region%d", so once this has happened the region can no
> longer be brought up at all.
>
> Call cxlr_pmem_unregister() on that branch. It is invoked from inside the
> scoped_guard() that holds the bridge's device lock, which is what its
> device_lock_assert() requires, and it performs the same teardown the devm
> action would have performed, including clearing cxlr->cxlr_pmem, so
> err_bridge is left with only the bridge reference to drop.
>
> Fixes: f17b558d6663 ("cxl/pmem: Refactor nvdimm device registration, delete the workqueue")
> Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
Reviewed-by: Li Ming <ming.li@zohomail.com>
> ---
> This was patch 8/8 of the "cxl: Assorted fixes" series [1]. Per review
> feedback that series is not being reworked as a whole; the fixes are resent
> individually instead. Patches 1, 2 and 7 of the series are dropped, as those
> issues are already fixed in cxl/next.
>
> v1->v2:
> - rebase onto cxl/next
> - rewrite the commit message to describe the behaviour rather than narrate
> the code change (Alison Schofield)
>
> [1] https://lore.kernel.org/linux-cxl/20260811113608.2815625-1-kanie@linux.alibaba.com/
>
> drivers/cxl/core/region_pmem.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/cxl/core/region_pmem.c b/drivers/cxl/core/region_pmem.c
> index 23d97e3d78b6..7ab1373a95e0 100644
> --- a/drivers/cxl/core/region_pmem.c
> +++ b/drivers/cxl/core/region_pmem.c
> @@ -168,12 +168,14 @@ int devm_cxl_add_pmem_region(struct cxl_region *cxlr)
> dev_name(dev));
>
> scoped_guard(device, &cxl_nvb->dev) {
> - if (cxl_nvb->dev.driver)
> + if (cxl_nvb->dev.driver) {
> rc = devm_add_action_or_reset(&cxl_nvb->dev,
> cxlr_pmem_unregister,
> cxlr_pmem);
> - else
> + } else {
> rc = -ENXIO;
> + cxlr_pmem_unregister(cxlr_pmem);
> + }
> }
>
> if (rc)
>
> base-commit: 7098e9cd98a05c0c5de2fae0c2465f9d966fdd07
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] cxl/region: Unregister the pmem region bridge on setup failure
2026-08-12 6:10 [PATCH v2] cxl/region: Unregister the pmem region bridge on setup failure Guixin Liu
2026-08-12 7:58 ` Richard Cheng
2026-08-12 11:57 ` Li Ming
@ 2026-08-12 21:42 ` Alison Schofield
2026-08-28 9:06 ` Guixin Liu
2 siblings, 1 reply; 6+ messages in thread
From: Alison Schofield @ 2026-08-12 21:42 UTC (permalink / raw)
To: Guixin Liu
Cc: Davidlohr Bueso, Jonathan Cameron, Dave Jiang, Vishal Verma,
Dan Williams, Ira Weiny, Li Ming, linux-cxl
On Wed, Aug 12, 2026 at 02:10:43PM +0800, Guixin Liu wrote:
About the subject line:
Do we unregister the bridge or do we unregister the pmem
region when the bridge is unboound?
> devm_cxl_add_pmem_region() publishes the cxl_pmem_region with device_add()
> and only afterwards, under the nvdimm bridge's device lock, arranges for
> its removal - and only if the bridge has a driver bound. If it does not,
> the function sets -ENXIO and leaves through err_bridge, which drops the
> reference this function took on the bridge and returns. The device that
> was just added has no owner at that point: no device_del(), no
> put_device(), and no devm action to do either later. The sibling failure,
> a devm_add_action_or_reset() that cannot allocate, is already covered,
> because devm_add_action_or_reset() runs cxlr_pmem_unregister() itself on
> that path.
>
> An unbound bridge is a normal state, not an error state. The bridge is
> unbound whenever cxl_pmem is unloaded or its device is detached through
> sysfs, and a region can be probed in that window.
>
> The added device then stays in sysfs, along with the reference it holds on
> the region, until the module is unloaded. cxlr->cxlr_pmem still points at
> it, and worse, the name is still taken: a later probe of the same region
> allocates a second cxl_pmem_region and fails in device_add() on the
> duplicate "pmem_region%d", so once this has happened the region can no
> longer be brought up at all.
Is above an observed behavior? ie can you reproduce this, apply
the fix, see it go away? That would be far more valuable than this
excessive narration as proof of repair.
More below...
>
> Call cxlr_pmem_unregister() on that branch. It is invoked from inside the
> scoped_guard() that holds the bridge's device lock, which is what its
> device_lock_assert() requires, and it performs the same teardown the devm
> action would have performed, including clearing cxlr->cxlr_pmem, so
> err_bridge is left with only the bridge reference to drop.
>
> Fixes: f17b558d6663 ("cxl/pmem: Refactor nvdimm device registration, delete the workqueue")
> Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
> ---
> This was patch 8/8 of the "cxl: Assorted fixes" series [1]. Per review
> feedback that series is not being reworked as a whole; the fixes are resent
> individually instead. Patches 1, 2 and 7 of the series are dropped, as those
> issues are already fixed in cxl/next.
>
> v1->v2:
> - rebase onto cxl/next
> - rewrite the commit message to describe the behaviour rather than narrate
> the code change (Alison Schofield)
This v2 misses the spirit of my v1 feedback. Spirit, meaning I guess
I was not clear enough.
It is more behavior-oriented than v1, but it is still far too much
investigation in the permanent commit log. The first paragraph walks
the reader through control flow almost statement by statement:
device_add(), lock acquisition, driver check, -ENXIO, err_bridge,
reference drop, missing device_del(), missing put_device(), and
then the sibling devm_add_action_or_reset() case. That is essentially
code narration with more behavioral vocabulary around it.
I was hoping the outcome would simply state the failing condition,
the consequence, and the fix, maybe like:
>> cxl/region: Unregister pmem region when bridge is unbound
>>
>> Probing a pmem region while the nvdimm bridge is unbound leaves the
>> newly registered cxl_pmem_region in sysfs. A later probe attempts to
>> register another device with the same name and fails with -EEXIST,
>> preventing the region from being brought up.
>>
>> Unregister the cxl_pmem_region when the bridge is unbound.
Please consider that in future patches to CXL.
You can v3 if you wish or not.
>
> [1] https://lore.kernel.org/linux-cxl/20260811113608.2815625-1-kanie@linux.alibaba.com/
>
> drivers/cxl/core/region_pmem.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/cxl/core/region_pmem.c b/drivers/cxl/core/region_pmem.c
> index 23d97e3d78b6..7ab1373a95e0 100644
> --- a/drivers/cxl/core/region_pmem.c
> +++ b/drivers/cxl/core/region_pmem.c
> @@ -168,12 +168,14 @@ int devm_cxl_add_pmem_region(struct cxl_region *cxlr)
> dev_name(dev));
>
> scoped_guard(device, &cxl_nvb->dev) {
> - if (cxl_nvb->dev.driver)
> + if (cxl_nvb->dev.driver) {
> rc = devm_add_action_or_reset(&cxl_nvb->dev,
> cxlr_pmem_unregister,
> cxlr_pmem);
> - else
> + } else {
> rc = -ENXIO;
> + cxlr_pmem_unregister(cxlr_pmem);
> + }
> }
>
> if (rc)
>
> base-commit: 7098e9cd98a05c0c5de2fae0c2465f9d966fdd07
> --
> 2.43.7
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] cxl/region: Unregister the pmem region bridge on setup failure
2026-08-12 21:42 ` Alison Schofield
@ 2026-08-28 9:06 ` Guixin Liu
2026-08-29 1:25 ` Alison Schofield
0 siblings, 1 reply; 6+ messages in thread
From: Guixin Liu @ 2026-08-28 9:06 UTC (permalink / raw)
To: Alison Schofield
Cc: Davidlohr Bueso, Jonathan Cameron, Dave Jiang, Vishal Verma,
Dan Williams, Ira Weiny, Li Ming, linux-cxl
在 2026/8/13 05:42, Alison Schofield 写道:
> On Wed, Aug 12, 2026 at 02:10:43PM +0800, Guixin Liu wrote:
>
> About the subject line:
>
> Do we unregister the bridge or do we unregister the pmem
> region when the bridge is unboound?
>
Unregister the pmem region, sorry for the confusing description.
>> devm_cxl_add_pmem_region() publishes the cxl_pmem_region with device_add()
>> and only afterwards, under the nvdimm bridge's device lock, arranges for
>> its removal - and only if the bridge has a driver bound. If it does not,
>> the function sets -ENXIO and leaves through err_bridge, which drops the
>> reference this function took on the bridge and returns. The device that
>> was just added has no owner at that point: no device_del(), no
>> put_device(), and no devm action to do either later. The sibling failure,
>> a devm_add_action_or_reset() that cannot allocate, is already covered,
>> because devm_add_action_or_reset() runs cxlr_pmem_unregister() itself on
>> that path.
>>
>> An unbound bridge is a normal state, not an error state. The bridge is
>> unbound whenever cxl_pmem is unloaded or its device is detached through
>> sysfs, and a region can be probed in that window.
>>
>> The added device then stays in sysfs, along with the reference it holds on
>> the region, until the module is unloaded. cxlr->cxlr_pmem still points at
>> it, and worse, the name is still taken: a later probe of the same region
>> allocates a second cxl_pmem_region and fails in device_add() on the
>> duplicate "pmem_region%d", so once this has happened the region can no
>> longer be brought up at all.
> Is above an observed behavior? ie can you reproduce this, apply
> the fix, see it go away? That would be far more valuable than this
> excessive narration as proof of repair.
>
> More below...
I'm so sorry, I thought that we should add cleanup in
failure branch, but now I realize that this branch is never actually
reached.
Please ignore this patch, sorry again.
Best Regards,
Guixin Liu
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] cxl/region: Unregister the pmem region bridge on setup failure
2026-08-28 9:06 ` Guixin Liu
@ 2026-08-29 1:25 ` Alison Schofield
0 siblings, 0 replies; 6+ messages in thread
From: Alison Schofield @ 2026-08-29 1:25 UTC (permalink / raw)
To: Guixin Liu
Cc: Davidlohr Bueso, Jonathan Cameron, Dave Jiang, Vishal Verma,
Dan Williams, Ira Weiny, Li Ming, linux-cxl
On Fri, Aug 28, 2026 at 05:06:55PM +0800, Guixin Liu wrote:
>
>
> 在 2026/8/13 05:42, Alison Schofield 写道:
> > On Wed, Aug 12, 2026 at 02:10:43PM +0800, Guixin Liu wrote:
> >
> > About the subject line:
> >
> > Do we unregister the bridge or do we unregister the pmem
> > region when the bridge is unboound?
> >
> Unregister the pmem region, sorry for the confusing description.
> > > devm_cxl_add_pmem_region() publishes the cxl_pmem_region with device_add()
> > > and only afterwards, under the nvdimm bridge's device lock, arranges for
> > > its removal - and only if the bridge has a driver bound. If it does not,
> > > the function sets -ENXIO and leaves through err_bridge, which drops the
> > > reference this function took on the bridge and returns. The device that
> > > was just added has no owner at that point: no device_del(), no
> > > put_device(), and no devm action to do either later. The sibling failure,
> > > a devm_add_action_or_reset() that cannot allocate, is already covered,
> > > because devm_add_action_or_reset() runs cxlr_pmem_unregister() itself on
> > > that path.
> > >
> > > An unbound bridge is a normal state, not an error state. The bridge is
> > > unbound whenever cxl_pmem is unloaded or its device is detached through
> > > sysfs, and a region can be probed in that window.
> > >
> > > The added device then stays in sysfs, along with the reference it holds on
> > > the region, until the module is unloaded. cxlr->cxlr_pmem still points at
> > > it, and worse, the name is still taken: a later probe of the same region
> > > allocates a second cxl_pmem_region and fails in device_add() on the
> > > duplicate "pmem_region%d", so once this has happened the region can no
> > > longer be brought up at all.
> > Is above an observed behavior? ie can you reproduce this, apply
> > the fix, see it go away? That would be far more valuable than this
> > excessive narration as proof of repair.
> >
> > More below...
> I'm so sorry, I thought that we should add cleanup in
> failure branch, but now I realize that this branch is never actually
> reached.
>
I think the patch may still apply as-is, so before dropping it, please take a
look at this from the teardown side rather than the "cxl_pmem is not loaded" side.
cxl_acpi removal unbinds and deletes the bridge before the regions it serves. So,
a region probe that already found the bridge can reach that check after the bridge
is gone, set -ENXIO, and return leaving the just-added device with no owner.
If that is true (I'm not sure it is) then the fix stands and the changelog needs to
tell that story.
-- Alison
> Please ignore this patch, sorry again.
>
> Best Regards,
> Guixin Liu
>
^ permalink raw reply [flat|nested] 6+ messages in thread