Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) client-ip=23.128.96.32;
Date:   Fri, 27 Oct 2023 18:39:28 -0700
From:   Dan Williams <dan.j.williams@intel.com>
To:     Dan Williams <dan.j.williams@intel.com>,
        Robert Richter <rrichter@amd.com>
CC:     Davidlohr Bueso <dave@stgolabs.net>,
        Jonathan Cameron <jonathan.cameron@huawei.com>,
        Dave Jiang <dave.jiang@intel.com>,
        "Alison Schofield" <alison.schofield@intel.com>,
        Vishal Verma <vishal.l.verma@intel.com>,
        Ira Weiny <ira.weiny@intel.com>,
        Ben Widawsky <bwidawsk@kernel.org>,
        <linux-cxl@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        Bjorn Helgaas <bhelgaas@google.com>,
        "Terry Bowman" <terry.bowman@amd.com>
Subject: Re: [PATCH v12 01/20] cxl/port: Fix release of RCD endpoints
Message-ID: <653c66507bf8d_244c8f294ea@dwillia2-xfh.jf.intel.com.notmuch>
References: <20231018171713.1883517-1-rrichter@amd.com>
 <20231018171713.1883517-2-rrichter@amd.com>
 <653b3299c1a33_244c8f29449@dwillia2-xfh.jf.intel.com.notmuch>
 <ZTw_xd2_uaApAzoL@rric.localdomain>
 <653c5691a2372_780ef2949b@dwillia2-xfh.jf.intel.com.notmuch>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <653c5691a2372_780ef2949b@dwillia2-xfh.jf.intel.com.notmuch>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?Poq4eXIPL6bpTAZyaR/nIo+NvdssBWnUdx083W5Kh5+q62erZTfeLluTvM+f?=
 =?us-ascii?Q?31ke0U9wIS9mtdwz8iocTVMj/7s2HIpR2MhOpEC7P/TAcnfjL/xtGXZslS8v?=
 =?us-ascii?Q?iWt/8DkChTzKfxz5v6lQXcu5sDvH1Z87RL+pcvgVGq8sauBDDHexunqMetPU?=
 =?us-ascii?Q?xvOZvFGyFYbwmjmI8yuOv4ajRjUug2Z4sWpkhkuFeTCHq79l034fWhgG/ifQ?=
 =?us-ascii?Q?tfeyMSHf5Uxs0kP/mZklsM8OJr+L+y2OML0z/m8qe5scai6Ud8qM2eCBKTm0?=
 =?us-ascii?Q?64GIe1dSakK62jeK4FgRNELH4DUzYBmEB5QnExiVkT/bFGdJzievEjujwoS8?=
 =?us-ascii?Q?O0CV5ns9nTzC6io1dwfgheFsDLGIvJyfQhIUTdRpxkmSF+aIsu6YIAstuwgu?=
 =?us-ascii?Q?bGvW00L3uwLnvE//3Ip732hq+1MQRfp77iNcgRseSetVpml8Zq47v67v6JZS?=
 =?us-ascii?Q?v6M/3cCrKe98hgEmj6lrdWF9qMPxICDBAtou1UDM8AbYwLxaojM/JWUBbD+w?=
 =?us-ascii?Q?f9bIfGHcPhyDzA/+wo3dUdl240YVnVIj7LN4Ow83MSXNmKwQJQYh+MAhgVn7?=
 =?us-ascii?Q?t9kYf9oPx3VIAIl3LbhItb4AoehPKHIfW+W37gB5Dnubz72acvoBt4oJN6Jr?=
 =?us-ascii?Q?QNEKzAPzZzW//CmeIaCWHQluMieHFU9h4t1FeSQyGKOxI+0gS/IpZQyWgwqo?=
 =?us-ascii?Q?/AMqDgQ+lUZVZyKjrcQH0SDRclSMZKtE2vvK7gIEqktpK8goWwlNIxJvQuYi?=
 =?us-ascii?Q?dQu01d9ZIEggsaAPxzCI1TrrkvHf0S8tO7cA3Gj5l+p0C0YlFWTXyqH4QudY?=
 =?us-ascii?Q?3bK3AtRjSfIziBrxQBvGnH3cuiQyP01tjITsVM7mdVBTsKPaifijl3LmMg22?=
 =?us-ascii?Q?eoAhR/Fjx0fPHr0xkggwBOPrNDQvK2yA7ppX5iIsNlUHx++3c9ocJgSobR0M?=
 =?us-ascii?Q?9VoBQVTKxNy1rlnoyDHjY8EsrPk7nBfoomcjw6bj3UwCiFTziqAhONCNOyXp?=
 =?us-ascii?Q?9cjfSPBCYARBfaGBs79mJ6OpUFWRWFFIBBlTXsQhJ0U2BYnY5vUie5wyIRHd?=
 =?us-ascii?Q?WHTeyFuOvsHirtWVT8tebQIGLF51qJVoll47oK8Jm5tCdSwWrZgQT9SV7NyT?=
 =?us-ascii?Q?476iTDv1DuLyhGdRNm0dPzC4t2TxqMF4V6pnFqOIBaFuoo7FSkZfhM8I45aN?=
 =?us-ascii?Q?7kOlhcGzGEQslRvSQgZ8FjWre08+lpDTZwpg32ETQOm1snR7vsOFbfwtHKMd?=
 =?us-ascii?Q?73DJddCj4qtRh93U+RNylxjVr7sxW3y++Q5unYlRLVMZdN2bmKgl9tw+Ly/9?=
 =?us-ascii?Q?JslApm3GcCETozuGec/BzhSXu9QxUAdYQr4X+mbCv6n4LMepknlHYHhlZO4O?=
 =?us-ascii?Q?AWbGTPK45t4ynsgx8lW4M2hMq0x5xuR/7CAnkGooT1P7qIjyVPRE0AhAsQkx?=
 =?us-ascii?Q?FtDzZJJsc1R8nHUc4ue+mXU70g4w8U8HsE71s0C49sPhoPv1aCvGk7JGJPGF?=
 =?us-ascii?Q?Hlf+nMbQQPiFg9LCRSi0Pb9CIENhn1a4LbVT/Eb4XIQBQdRn4iRgCPQfUel6?=
 =?us-ascii?Q?Z95zf9hNHRw3j+ox8sCo9imbVsneIjNHfWGGBxeiAT1OLGfp8CPpYRl+dCZI?=
 =?us-ascii?Q?jw=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: a152baa3-2679-4b7c-b1d6-08dbd756bb28
X-MS-Exchange-CrossTenant-AuthSource: PH8PR11MB8107.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 28 Oct 2023 01:39:31.2114
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 0p6LcGnMkzj9Y6aXw8NEF7+44o/dW7yX1pcXtaClHc3WAmC07RahTpWfEcgtsFZm6lxnTJIT/AWkBAoZFpWBbawWg2jdVxSXLhrjNN/1Nks=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA1PR11MB6782
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=unavailable
	autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on agentk.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (agentk.vger.email [0.0.0.0]); Fri, 27 Oct 2023 18:39:51 -0700 (PDT)

Dan Williams wrote:
> Robert Richter wrote:
> > Dan,
> [..]
> > 
> > delete_endpoint() is called here, but the uport etc. is not unbound.
> > Which means this is not true:
> > 
> > 	if (parent->driver && !endpoint->dead) {
> > 		...
> > 
> > I don't remember this with my patch. The parent is there different, so
> > that could be the reason.
> > 
> > I could not yet look into more detail but wanted to let you know. Will
> > continue.
> 
> Apologies, I didn't have that regression going, I think I see the issue.
> Thanks for the heads up.

Here is the incremental fix on top of the lifetime fix:

diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index 6230ddfc0be8..0fe915ec2cc2 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -1217,30 +1217,39 @@ static struct device *grandparent(struct device *dev)
 	return NULL;
 }
 
+static struct device *endpoint_host(struct cxl_port *endpoint)
+{
+	struct cxl_port *port = to_cxl_port(endpoint->dev.parent);
+
+	if (is_cxl_root(port))
+		return port->uport_dev;
+	return &port->dev;
+}
+
 static void delete_endpoint(void *data)
 {
 	struct cxl_memdev *cxlmd = data;
 	struct cxl_port *endpoint = cxlmd->endpoint;
-	struct device *parent = endpoint->dev.parent;
+	struct device *host = endpoint_host(endpoint);
 
-	device_lock(parent);
-	if (parent->driver && !endpoint->dead) {
-		devm_release_action(parent, cxl_unlink_parent_dport, endpoint);
-		devm_release_action(parent, cxl_unlink_uport, endpoint);
-		devm_release_action(parent, unregister_port, endpoint);
+	device_lock(host);
+	if (host->driver && !endpoint->dead) {
+		devm_release_action(host, cxl_unlink_parent_dport, endpoint);
+		devm_release_action(host, cxl_unlink_uport, endpoint);
+		devm_release_action(host, unregister_port, endpoint);
 	}
 	cxlmd->endpoint = NULL;
-	device_unlock(parent);
+	device_unlock(host);
 	put_device(&endpoint->dev);
-	put_device(parent);
+	put_device(host);
 }
 
 int cxl_endpoint_autoremove(struct cxl_memdev *cxlmd, struct cxl_port *endpoint)
 {
-	struct device *parent = endpoint->dev.parent;
+	struct device *host = endpoint_host(endpoint);
 	struct device *dev = &cxlmd->dev;
 
-	get_device(parent);
+	get_device(host);
 	get_device(&endpoint->dev);
 	cxlmd->endpoint = endpoint;
 	cxlmd->depth = endpoint->depth;


---

...and here is the new regression test so I don't mess up and miss this
again:

diff --git a/cxl/memdev.c b/cxl/memdev.c
index d76a4d86a40a..81dfd4c25b25 100644
--- a/cxl/memdev.c
+++ b/cxl/memdev.c
@@ -752,6 +752,8 @@ static int memdev_action(int argc, const char **argv, struct cxl_ctx *ctx,
                        if (end[0] == 0)
                                continue;
                } else {
+                       unsigned long domain, bus, dev, func;
+
                        if (strcmp(argv[i], "all") == 0) {
                                argc = 1;
                                break;
@@ -760,6 +762,12 @@ static int memdev_action(int argc, const char **argv, struct cxl_ctx *ctx,
                                continue;
                        if (sscanf(argv[i], "%lu", &id) == 1)
                                continue;
+                       if (sscanf(argv[i], "%lx:%lx:%lx.%lx", &domain, &bus, &dev, &func))
+                               continue;
+                       if (sscanf(argv[i], "cxl_mem.%lu", &id))
+                               continue;
+                       if (sscanf(argv[i], "cxl_rcd.%lu", &id))
+                               continue;
                }
 
                log_err(&ml, "'%s' is not a valid memdev %s\n", argv[i],
diff --git a/test/cxl-topology.sh b/test/cxl-topology.sh
index 89d01a89ccb1..0320887a953b 100644
--- a/test/cxl-topology.sh
+++ b/test/cxl-topology.sh
@@ -120,6 +120,13 @@ count=$(jq "map(select(.pmem_size == $pmem_size)) | length" <<< $json)
 ((bridges == 2 && count == 8 || bridges == 3 && count == 10 ||
   bridges == 4 && count == 11)) || err "$LINENO"
 
+# check rcd endpoints disappear when disabling the memdev
+m=$($CXL list -M -b cxl_test | jq -r ".[].host" | grep rcd)
+ep=$($CXL list -E -m $m | jq -r ".[].endpoint")
+$CXL disable-memdev $m --force
+check=$($CXL list -E -e $ep | jq -r ".[].endpoint")
+[ -z "$check" ] || err "$LINENO"
+$CXL enable-memdev $m