All of lore.kernel.org
 help / color / mirror / Atom feed
From: Guixin Liu <kanie@linux.alibaba.com>
To: Alison Schofield <alison.schofield@intel.com>
Cc: Davidlohr Bueso <dave@stgolabs.net>,
	Jonathan Cameron <jic23@kernel.org>,
	Dave Jiang <dave.jiang@intel.com>,
	Vishal Verma <vishal.l.verma@intel.com>,
	Dan Williams <djbw@kernel.org>, Ira Weiny <iweiny@kernel.org>,
	Li Ming <ming.li@zohomail.com>,
	linux-cxl@vger.kernel.org
Subject: Re: [PATCH v2] cxl/region: Unregister the pmem region bridge on setup failure
Date: Mon, 31 Aug 2026 14:18:04 +0800	[thread overview]
Message-ID: <a6da695a-36a2-4b1a-a5ed-94cc42218ab2@linux.alibaba.com> (raw)
In-Reply-To: <apI1Ebyv8U6ZJWYR@aschofie-mobl2.lan>



在 2026/8/29 09:25, Alison Schofield 写道:
> 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.
Well, you are right, I test this on vm, it reached the failure branch.

I sent the v3 patch for shorter commit body and testing description.

Best Regards,
Guixin Liu
>
> 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
>>


      reply	other threads:[~2026-08-31  6:18 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
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
2026-08-29  1:25     ` Alison Schofield
2026-08-31  6:18       ` Guixin Liu [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=a6da695a-36a2-4b1a-a5ed-94cc42218ab2@linux.alibaba.com \
    --to=kanie@linux.alibaba.com \
    --cc=alison.schofield@intel.com \
    --cc=dave.jiang@intel.com \
    --cc=dave@stgolabs.net \
    --cc=djbw@kernel.org \
    --cc=iweiny@kernel.org \
    --cc=jic23@kernel.org \
    --cc=linux-cxl@vger.kernel.org \
    --cc=ming.li@zohomail.com \
    --cc=vishal.l.verma@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.