From patchwork Fri Oct 18 01:10:17 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Dan Williams X-Patchwork-Id: 13840943 Received: from mgamail.intel.com (mgamail.intel.com [192.198.163.18]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id BE7384E1C4 for ; Fri, 18 Oct 2024 01:10:23 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=192.198.163.18 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1729213825; cv=none; b=ojWEbimyYyfsqbV7ciUpfKNC7ozuYXgE90eFyPH9tcHtpiRfGwVkorfumLWJBfMXqawvYS5+AeoAGPSYGudGkVKMRAR4/5NoFtBnyqkuktC9nmxn3cg1sWPVk8cOhQrX96b0sT4/K3If/WX3tQGyPbkmdnR4LVhpkOK96AXHUKE= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1729213825; c=relaxed/simple; bh=E3UHSvZvgSX6hOL/3CPLxJ2XK2lLvnfBXphzuY2jUjM=; h=Subject:From:To:Cc:Date:Message-ID:In-Reply-To:References: MIME-Version:Content-Type; b=YnXZR4d99h1ZPibcFmz7d5suPKduN1fPzEv8xIfN1OqM4VWIrt4M2WO4XoE3gBsn6Ygbr1I3BbktMOREPYHe69bCMhG2aNO9M8q/YvI3G0P/cdh/oonzW199gpK6Rtg6niEp/hnCtIKLlr12McUzngAzEo3yVPrfhqkqEeAQ/ZQ= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=intel.com; spf=pass smtp.mailfrom=intel.com; dkim=pass (2048-bit key) header.d=intel.com header.i=@intel.com header.b=Cmvf84Z5; arc=none smtp.client-ip=192.198.163.18 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=intel.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=intel.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=intel.com header.i=@intel.com header.b="Cmvf84Z5" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1729213824; x=1760749824; h=subject:from:to:cc:date:message-id:in-reply-to: references:mime-version:content-transfer-encoding; bh=E3UHSvZvgSX6hOL/3CPLxJ2XK2lLvnfBXphzuY2jUjM=; b=Cmvf84Z50KdJYHtj4ImauM4ZN+5VIsYdTWxkRLQovYfuew/P5qxlkYJP CHyYFnfeMpQXsVuC5ugxwi77H3d5BFhmPsEnAziF4NVnXO2wZRluJZTG4 Vmh/dwWTZid8R+zvfDXhYHa/ZcX/nfKVaIHrgRJI2LACPm0LGMeGh12LT ICKSbyrE3VNrfKMUu38STPI+KAjIiUT/+MMDA4xUIWvU1ZIck2XGzZ281 qf9vm2elnmSpNUS8gUUn5MZ6LgRHS3gZ2JGOvO5ZkzVlFIKMllKmlqxBQ 6JECIj1QQCLECupvJzIfzeuBQ2r+jAy0GUjgizoTzr3IvE1vpxhXtrrF2 w==; X-CSE-ConnectionGUID: MwLK4LWvR9GT0rleXFWoBw== X-CSE-MsgGUID: 4x2Rs2GKRzuMFv0kmUu1DQ== X-IronPort-AV: E=McAfee;i="6700,10204,11228"; a="28168403" X-IronPort-AV: E=Sophos;i="6.11,212,1725346800"; d="scan'208";a="28168403" Received: from orviesa002.jf.intel.com ([10.64.159.142]) by fmvoesa112.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 17 Oct 2024 18:10:19 -0700 X-CSE-ConnectionGUID: JsrNMH3hR+KjLWzhmPR/hw== X-CSE-MsgGUID: 9Gj+iNiKRpea+csLbSW4mw== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.11,212,1725346800"; d="scan'208";a="109535218" Received: from rfrazer-mobl3.amr.corp.intel.com (HELO dwillia2-xfh.jf.intel.com) ([10.125.109.118]) by orviesa002-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 17 Oct 2024 18:10:20 -0700 Subject: [PATCH v2 1/3] cxl/port: Revert usage of scoped_guard() From: Dan Williams To: ira.weiny@intel.com, dave.jiang@intel.com Cc: Dan Carpenter , Li Ming , linux-cxl@vger.kernel.org Date: Thu, 17 Oct 2024 18:10:17 -0700 Message-ID: <172921381577.2133624.10091366656126564045.stgit@dwillia2-xfh.jf.intel.com> In-Reply-To: <172921380683.2133624.1708770961640157143.stgit@dwillia2-xfh.jf.intel.com> References: <172921380683.2133624.1708770961640157143.stgit@dwillia2-xfh.jf.intel.com> User-Agent: StGit/0.18-3-g996c Precedence: bulk X-Mailing-List: linux-cxl@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Dan points out via smatch [1] that the conversion of add_port_attach_ep() to use cleanup helpers gives the appearance of not correctly handling a pointer reassignment case. drivers/cxl/core/port.c:1591 add_port_attach_ep() warn: re-assigning __cleanup__ ptr 'port' The conversion to scoped_guard() complicates rather than simplifies code readability. It turns out that there is no bug in practice because the reassignment still results in correct reference accounting, but that deserves a separate fix. For now, just revert the scoped_guard() conversions in preparation for refactoring the subtlety out of this routine. Reported-by: Dan Carpenter Link: http://lore.kernel.org/2a19289b-0bcf-42c4-82a9-268a922535f2@stanley.mountain [1] Cc: Li Ming Signed-off-by: Dan Williams Reviewed-by: Ira Weiny --- drivers/cxl/core/port.c | 72 +++++++++++++++++++++++---------------------- drivers/cxl/core/region.c | 25 ++++++++-------- drivers/cxl/mem.c | 22 ++++++++------ drivers/cxl/pmem.c | 16 ++++++---- 4 files changed, 70 insertions(+), 65 deletions(-) diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c index b7828b6c7826..54dd2cd450ca 100644 --- a/drivers/cxl/core/port.c +++ b/drivers/cxl/core/port.c @@ -1399,14 +1399,14 @@ static void delete_endpoint(void *data) struct cxl_port *endpoint = cxlmd->endpoint; struct device *host = endpoint_host(endpoint); - scoped_guard(device, 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_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(host); put_device(&endpoint->dev); put_device(host); } @@ -1571,38 +1571,40 @@ static int add_port_attach_ep(struct cxl_memdev *cxlmd, * dereferencing the device of the port before the parent_port releasing. */ struct cxl_port *port __free(put_cxl_port) = NULL; - scoped_guard(device, &parent_port->dev) { - if (!parent_port->dev.driver) { - dev_warn(&cxlmd->dev, - "port %s:%s disabled, failed to enumerate CXL.mem\n", - dev_name(&parent_port->dev), dev_name(uport_dev)); - return -ENXIO; - } - - port = find_cxl_port_at(parent_port, dport_dev, &dport); - if (!port) { - component_reg_phys = find_component_registers(uport_dev); - port = devm_cxl_add_port(&parent_port->dev, uport_dev, - component_reg_phys, parent_dport); - if (IS_ERR(port)) - return PTR_ERR(port); + device_lock(&parent_port->dev); + if (!parent_port->dev.driver) { + dev_warn(&cxlmd->dev, + "port %s:%s disabled, failed to enumerate CXL.mem\n", + dev_name(&parent_port->dev), dev_name(uport_dev)); + port = ERR_PTR(-ENXIO); + goto out; + } - /* retry find to pick up the new dport information */ + port = find_cxl_port_at(parent_port, dport_dev, &dport); + if (!port) { + component_reg_phys = find_component_registers(uport_dev); + port = devm_cxl_add_port(&parent_port->dev, uport_dev, + component_reg_phys, parent_dport); + /* retry find to pick up the new dport information */ + if (!IS_ERR(port)) port = find_cxl_port_at(parent_port, dport_dev, &dport); - if (!port) - return -ENXIO; - } } +out: + device_unlock(&parent_port->dev); - dev_dbg(&cxlmd->dev, "add to new port %s:%s\n", - dev_name(&port->dev), dev_name(port->uport_dev)); - rc = cxl_add_ep(dport, &cxlmd->dev); - if (rc == -EBUSY) { - /* - * "can't" happen, but this error code means - * something to the caller, so translate it. - */ - rc = -ENXIO; + if (IS_ERR(port)) + rc = PTR_ERR(port); + else { + dev_dbg(&cxlmd->dev, "add to new port %s:%s\n", + dev_name(&port->dev), dev_name(port->uport_dev)); + rc = cxl_add_ep(dport, &cxlmd->dev); + if (rc == -EBUSY) { + /* + * "can't" happen, but this error code means + * something to the caller, so translate it. + */ + rc = -ENXIO; + } } return rc; diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c index dff618c708dc..77b301b0d269 100644 --- a/drivers/cxl/core/region.c +++ b/drivers/cxl/core/region.c @@ -3083,11 +3083,11 @@ static void cxlr_release_nvdimm(void *_cxlr) struct cxl_region *cxlr = _cxlr; struct cxl_nvdimm_bridge *cxl_nvb = cxlr->cxl_nvb; - scoped_guard(device, &cxl_nvb->dev) { - if (cxlr->cxlr_pmem) - devm_release_action(&cxl_nvb->dev, cxlr_pmem_unregister, - cxlr->cxlr_pmem); - } + device_lock(&cxl_nvb->dev); + if (cxlr->cxlr_pmem) + devm_release_action(&cxl_nvb->dev, cxlr_pmem_unregister, + cxlr->cxlr_pmem); + device_unlock(&cxl_nvb->dev); cxlr->cxl_nvb = NULL; put_device(&cxl_nvb->dev); } @@ -3123,14 +3123,13 @@ static int devm_cxl_add_pmem_region(struct cxl_region *cxlr) dev_dbg(&cxlr->dev, "%s: register %s\n", dev_name(dev->parent), dev_name(dev)); - scoped_guard(device, &cxl_nvb->dev) { - if (cxl_nvb->dev.driver) - rc = devm_add_action_or_reset(&cxl_nvb->dev, - cxlr_pmem_unregister, - cxlr_pmem); - else - rc = -ENXIO; - } + device_lock(&cxl_nvb->dev); + if (cxl_nvb->dev.driver) + rc = devm_add_action_or_reset(&cxl_nvb->dev, + cxlr_pmem_unregister, cxlr_pmem); + else + rc = -ENXIO; + device_unlock(&cxl_nvb->dev); if (rc) goto err_bridge; diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c index a9fd5cd5a0d2..6daca88845ca 100644 --- a/drivers/cxl/mem.c +++ b/drivers/cxl/mem.c @@ -168,18 +168,20 @@ static int cxl_mem_probe(struct device *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", - dev_name(endpoint_parent)); - return -ENXIO; - } - - rc = devm_cxl_add_endpoint(endpoint_parent, cxlmd, dport); - if (rc) - return rc; + device_lock(endpoint_parent); + if (!endpoint_parent->driver) { + dev_err(dev, "CXL port topology %s not enabled\n", + dev_name(endpoint_parent)); + rc = -ENXIO; + goto unlock; } + rc = devm_cxl_add_endpoint(endpoint_parent, cxlmd, dport); +unlock: + device_unlock(endpoint_parent); + if (rc) + return rc; + /* * The kernel may be operating out of CXL memory on this device, * there is no spec defined way to determine whether this device diff --git a/drivers/cxl/pmem.c b/drivers/cxl/pmem.c index d2d43a4fc053..94ce71952447 100644 --- a/drivers/cxl/pmem.c +++ b/drivers/cxl/pmem.c @@ -237,13 +237,15 @@ static int detach_nvdimm(struct device *dev, void *data) if (!is_cxl_nvdimm(dev)) return 0; - scoped_guard(device, dev) { - if (dev->driver) { - cxl_nvd = to_cxl_nvdimm(dev); - if (cxl_nvd->cxlmd && cxl_nvd->cxlmd->cxl_nvb == data) - release = true; - } - } + device_lock(dev); + if (!dev->driver) + goto out; + + cxl_nvd = to_cxl_nvdimm(dev); + if (cxl_nvd->cxlmd && cxl_nvd->cxlmd->cxl_nvb == data) + release = true; +out: + device_unlock(dev); if (release) device_release_driver(dev); return 0; From patchwork Fri Oct 18 01:10:25 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Dan Williams X-Patchwork-Id: 13840944 Received: from mgamail.intel.com (mgamail.intel.com [192.198.163.18]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 4E18620E31E for ; Fri, 18 Oct 2024 01:10:37 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=192.198.163.18 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1729213839; cv=none; b=lKWKhLG/wUoI+lU6VtsJxJihyJnBAAaIFHmBRzi38byGI0MNjCQplI5VxV0PpEhJiBarBAtAQSTm8X8ADjeUhksXYXs7mapPauWDtMZsIiR/3ykXI70qJq5yB0gLVUHXusBnfpnDAvXu5s/qMjjrKA2UHGcuP9Zf64kJ2owYeBQ= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1729213839; c=relaxed/simple; bh=L8pZ6+V41pof13b5gixaLksIjInuyvkMAXvESD6nvPE=; h=Subject:From:To:Cc:Date:Message-ID:In-Reply-To:References: MIME-Version:Content-Type; b=azeBwpDWtWMWmN4atLdGfph0NCeHMAiwfCtj0+XOAKE/LTdnjXCGnB2+WuXZ79T1kHwW8THvTz5M7VVmkXAbl5lAHr1Lg3wIJuJZ43WHfkGhA4Tou8G/nM2/Ro/kJfYguQj4DhS+cD8VK2K0KhDQwVZS67tSeDFF5dSdXytsOBA= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=intel.com; spf=pass smtp.mailfrom=intel.com; dkim=pass (2048-bit key) header.d=intel.com header.i=@intel.com header.b=nygvghDc; arc=none smtp.client-ip=192.198.163.18 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=intel.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=intel.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=intel.com header.i=@intel.com header.b="nygvghDc" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1729213837; x=1760749837; h=subject:from:to:cc:date:message-id:in-reply-to: references:mime-version:content-transfer-encoding; bh=L8pZ6+V41pof13b5gixaLksIjInuyvkMAXvESD6nvPE=; b=nygvghDcNn6ULtWP4NFv9bJhYMU79uCrKUfdrDaIQrIYvJX0+73PURGm Xc3HuqmY1Z7tqJ73PEkyHewoksMmBOzRmz/7hnyTljs3njOITg5FAT4Si Dfb8+6+tmnjpnSDuFq3V3KmKPSPSpa9Z7L0OHYRIp9gbFai9QSy77/Kw4 TeqUL0dACbnwqkjiBJToVJjYSJiKg+n1qB/kRJLg8ANVDuzFPe2Sl1LZF 50CuRaHKGRk37CMLMc7ryvthnwFtXkHNansb3Ptw39kwYs/OVqzkHseHu 6YFTyFzmKCGaNkvtJz0WbaoeI6pir8hEobmzG6yRJ/ZwWud0oLi+JjOJG Q==; X-CSE-ConnectionGUID: 36QXn4HHRUOSqqVLhw//LQ== X-CSE-MsgGUID: J6v592BaRiiIHlEUVgxF3w== X-IronPort-AV: E=McAfee;i="6700,10204,11228"; a="28168462" X-IronPort-AV: E=Sophos;i="6.11,212,1725346800"; d="scan'208";a="28168462" Received: from orviesa002.jf.intel.com ([10.64.159.142]) by fmvoesa112.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 17 Oct 2024 18:10:27 -0700 X-CSE-ConnectionGUID: pDH82L2TQ0mWDTgtwOqenQ== X-CSE-MsgGUID: eihjvqXMRcW9z7EL3+8DKA== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.11,212,1725346800"; d="scan'208";a="109535282" Received: from rfrazer-mobl3.amr.corp.intel.com (HELO dwillia2-xfh.jf.intel.com) ([10.125.109.118]) by orviesa002-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 17 Oct 2024 18:10:27 -0700 Subject: [PATCH v2 2/3] cxl/port: Revert __free() conversion of add_port_attach_ep() From: Dan Williams To: ira.weiny@intel.com, dave.jiang@intel.com Cc: Dan Carpenter , Li Ming , linux-cxl@vger.kernel.org Date: Thu, 17 Oct 2024 18:10:25 -0700 Message-ID: <172921382410.2133624.570103049126168259.stgit@dwillia2-xfh.jf.intel.com> In-Reply-To: <172921380683.2133624.1708770961640157143.stgit@dwillia2-xfh.jf.intel.com> References: <172921380683.2133624.1708770961640157143.stgit@dwillia2-xfh.jf.intel.com> User-Agent: StGit/0.18-3-g996c Precedence: bulk X-Mailing-List: linux-cxl@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Dan points out via smatch [1] that the conversion of add_port_attach_ep() to use cleanup helpers does appears to not correctly handle a pointer reassignment case. drivers/cxl/core/port.c:1591 add_port_attach_ep() warn: re-assigning __cleanup__ ptr 'port' The reassignment still results in the correct cleanup and reference counting, but it is not immediately obvious from the code. Revert the __free() conversion in preparation for refactoring the subtlety out of this function. Cc: Dan Carpenter Link: http://lore.kernel.org/2a19289b-0bcf-42c4-82a9-268a922535f2@stanley.mountain [1] Cc: Li Ming Signed-off-by: Dan Williams Reviewed-by: Ira Weiny --- drivers/cxl/core/port.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c index 54dd2cd450ca..8e6596e147b3 100644 --- a/drivers/cxl/core/port.c +++ b/drivers/cxl/core/port.c @@ -1544,6 +1544,7 @@ static int add_port_attach_ep(struct cxl_memdev *cxlmd, struct device *dport_dev) { struct device *dparent = grandparent(dport_dev); + struct cxl_port *port, *parent_port = NULL; struct cxl_dport *dport, *parent_dport; resource_size_t component_reg_phys; int rc; @@ -1559,18 +1560,12 @@ static int add_port_attach_ep(struct cxl_memdev *cxlmd, return -ENXIO; } - struct cxl_port *parent_port __free(put_cxl_port) = - find_cxl_port(dparent, &parent_dport); + parent_port = find_cxl_port(dparent, &parent_dport); if (!parent_port) { /* iterate to create this parent_port */ return -EAGAIN; } - /* - * Definition with __free() here to keep the sequence of - * dereferencing the device of the port before the parent_port releasing. - */ - struct cxl_port *port __free(put_cxl_port) = NULL; device_lock(&parent_port->dev); if (!parent_port->dev.driver) { dev_warn(&cxlmd->dev, @@ -1605,8 +1600,10 @@ static int add_port_attach_ep(struct cxl_memdev *cxlmd, */ rc = -ENXIO; } + put_device(&port->dev); } + put_device(&parent_port->dev); return rc; } From patchwork Fri Oct 18 01:10:33 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Dan Williams X-Patchwork-Id: 13840945 Received: from mgamail.intel.com (mgamail.intel.com [192.198.163.18]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 4FC242C859 for ; Fri, 18 Oct 2024 01:10:38 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=192.198.163.18 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1729213840; cv=none; b=IkioS6ofhaQjztcR3J72SyVGYPPCcu1H9Z65oTD9/deRbqzc3emims+KjZ1dr4yZVEpJbqDjeO1sY6TDnnCgMFTwdXQiO6jCDv3UIEEVntLM2402W1aPINNRusAE+C7kCqzXDXItP21meBKKLmY2TCsA63ZGI21xhr4470Yj3MU= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1729213840; c=relaxed/simple; bh=DlwFjROwVpvtL3AixKA+5jVRuBuItMbFqNJvW+kkm0E=; h=Subject:From:To:Cc:Date:Message-ID:In-Reply-To:References: MIME-Version:Content-Type; b=HHFzVOidznPjwbeuuh6EXXYQBFrQoSWtkLXYTPtBmLUMpDIcSQow7JXCfUQz8dOf7JdLsVXIGKWjtF65kf/7GGG1rAwQq0fnTxvB9dXY0+Usu4Gf9NfdWD+yhx8fh1TbJQGPx/6ommQLX+Eu+vSgdi1VIn6uJE4x52VfBO5d1UU= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=intel.com; spf=pass smtp.mailfrom=intel.com; dkim=pass (2048-bit key) header.d=intel.com header.i=@intel.com header.b=IU7t2L4R; arc=none smtp.client-ip=192.198.163.18 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=intel.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=intel.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=intel.com header.i=@intel.com header.b="IU7t2L4R" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1729213838; x=1760749838; h=subject:from:to:cc:date:message-id:in-reply-to: references:mime-version:content-transfer-encoding; bh=DlwFjROwVpvtL3AixKA+5jVRuBuItMbFqNJvW+kkm0E=; b=IU7t2L4Ryfns3z0DhGnjTYzx1NWoJ8Z5jDyg35aEKtovMz9DHpLEr1m4 lLCg8DopQ+pQZsj5H60c7+B4OoRVLFWzmhP2qaFoiUb13j+0CEciyLGpQ 9I7xXBPNzNzPHfhW/mm16wJWAl0uZR9Vi8axbrgLR0259/6n7AK06c2gu WbmIIKiT0sUxXMBiD8Tv2dYIVqOAoGmmEazxHynX0ecuSzb2ES+2GeESq 3GnF8y4g1nTWdJ2QM0CeTylkkrmPESzfp34MPSFtBQObr/IBUkjfVZ2PP 3MrjtTxpbabi0q2xAkeKOh2zD6KNAHLBHf51qI+PgZF3ODZfTFKTQiF96 A==; X-CSE-ConnectionGUID: fVdeUim5TpWWUxOr0/wsSg== X-CSE-MsgGUID: BkR+s7F3Q3uMJ/pXXamVhg== X-IronPort-AV: E=McAfee;i="6700,10204,11228"; a="28168500" X-IronPort-AV: E=Sophos;i="6.11,212,1725346800"; d="scan'208";a="28168500" Received: from orviesa002.jf.intel.com ([10.64.159.142]) by fmvoesa112.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 17 Oct 2024 18:10:35 -0700 X-CSE-ConnectionGUID: MNPoNwRhQsuSx2eTCCO70g== X-CSE-MsgGUID: 5dXVxnOvQwKEyuqjk1tvjA== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.11,212,1725346800"; d="scan'208";a="109535317" Received: from rfrazer-mobl3.amr.corp.intel.com (HELO dwillia2-xfh.jf.intel.com) ([10.125.109.118]) by orviesa002-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 17 Oct 2024 18:10:36 -0700 Subject: [PATCH v2 3/3] cxl/port: Clarify add_port_attach_ep() From: Dan Williams To: ira.weiny@intel.com, dave.jiang@intel.com Cc: Dan Carpenter , Jonathan Cameron , Li Ming , linux-cxl@vger.kernel.org Date: Thu, 17 Oct 2024 18:10:33 -0700 Message-ID: <172921383206.2133624.16072155083340676420.stgit@dwillia2-xfh.jf.intel.com> In-Reply-To: <172921380683.2133624.1708770961640157143.stgit@dwillia2-xfh.jf.intel.com> References: <172921380683.2133624.1708770961640157143.stgit@dwillia2-xfh.jf.intel.com> User-Agent: StGit/0.18-3-g996c Precedence: bulk X-Mailing-List: linux-cxl@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 The subtlety in add_port_attach_ep() is too high as even static analysis checkers had trouble following the error exit logic [1]. Jonathan points out that at a minimum the 2 acquisitions of @port should use 2 variables [2]. This patch goes a step further and refactors the code to: - drop the redundant uport_dev argument - move all device_lock dependent code to a new find_add_dport() helper - clarify why the @port reference needs to held - clarify that @port is not devm released on failure - create a new __free(put_cxl_dport) helper for this case of passing around an implied @port refernce Reported-by: Dan Carpenter Closes: http://lore.kernel.org/2a19289b-0bcf-42c4-82a9-268a922535f2@stanley.mountain [1] Cc: Jonathan Cameron Link: http://lore.kernel.org/20241017181244.00003e1f@Huawei.com [2] Cc: Li Ming Signed-off-by: Dan Williams --- drivers/cxl/core/port.c | 112 ++++++++++++++++++++++++++++------------------- drivers/cxl/cxl.h | 1 2 files changed, 68 insertions(+), 45 deletions(-) diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c index 8e6596e147b3..599b1a4caa70 100644 --- a/drivers/cxl/core/port.c +++ b/drivers/cxl/core/port.c @@ -1539,14 +1539,63 @@ static resource_size_t find_component_registers(struct device *dev) return map.resource; } +/* + * Check if @parent_port has a child cxl_port that hosts @dport_dev. + * and if not create a new port attached via the @parent_dport + * downstream of @parent_port + * + * Caller is responsible for dropping the port reference after using + * @dport + */ +static struct cxl_dport *find_add_dport(struct cxl_port *parent_port, + struct device *dport_dev, + struct cxl_dport *parent_dport) +{ + struct device *uport_dev = dport_dev->parent; + resource_size_t component_reg_phys; + struct cxl_dport *dport; + struct cxl_port *port; + + /* + * lock serves 2 purposes: + * - Resolve races to find/create a new port + * - Prevent found / created ports from being freed before a + * reference can be taken + */ + guard(device)(&parent_port->dev); + if (!parent_port->dev.driver) { + dev_warn(&parent_port->dev, + "disabled, failed to enumerate CXL.mem\n"); + return ERR_PTR(-ENXIO); + } + + port = find_cxl_port_at(parent_port, dport_dev, &dport); + if (port) + return dport; + + /* + * Note that this port is only unregistered when @parent_port + * is unbound / removed, not if this routine returns an error. + * It is a shared object across multiple downstream endpoints. + */ + component_reg_phys = find_component_registers(uport_dev); + port = devm_cxl_add_port(&parent_port->dev, uport_dev, + component_reg_phys, parent_dport); + if (IS_ERR(port)) + return ERR_CAST(port); + + dport = cxl_find_dport_by_dev(port, dport_dev); + if (!dport) + return ERR_PTR(-ENXIO); + get_device(&port->dev); + return dport; +} + static int add_port_attach_ep(struct cxl_memdev *cxlmd, - struct device *uport_dev, struct device *dport_dev) { struct device *dparent = grandparent(dport_dev); - struct cxl_port *port, *parent_port = NULL; - struct cxl_dport *dport, *parent_dport; - resource_size_t component_reg_phys; + struct cxl_dport *parent_dport; int rc; if (!dparent) { @@ -1560,50 +1609,23 @@ static int add_port_attach_ep(struct cxl_memdev *cxlmd, return -ENXIO; } - parent_port = find_cxl_port(dparent, &parent_dport); - if (!parent_port) { - /* iterate to create this parent_port */ - return -EAGAIN; - } + struct cxl_port *parent_port __free(put_cxl_port) = + find_cxl_port(dparent, &parent_dport); - device_lock(&parent_port->dev); - if (!parent_port->dev.driver) { - dev_warn(&cxlmd->dev, - "port %s:%s disabled, failed to enumerate CXL.mem\n", - dev_name(&parent_port->dev), dev_name(uport_dev)); - port = ERR_PTR(-ENXIO); - goto out; - } + /* iterate to create this parent_port? */ + if (!parent_port) + return -EAGAIN; - port = find_cxl_port_at(parent_port, dport_dev, &dport); - if (!port) { - component_reg_phys = find_component_registers(uport_dev); - port = devm_cxl_add_port(&parent_port->dev, uport_dev, - component_reg_phys, parent_dport); - /* retry find to pick up the new dport information */ - if (!IS_ERR(port)) - port = find_cxl_port_at(parent_port, dport_dev, &dport); - } -out: - device_unlock(&parent_port->dev); + struct cxl_dport *dport __free(put_cxl_dport) = + find_add_dport(parent_port, dport_dev, parent_dport); + if (IS_ERR(dport)) + return PTR_ERR(dport); - if (IS_ERR(port)) - rc = PTR_ERR(port); - else { - dev_dbg(&cxlmd->dev, "add to new port %s:%s\n", - dev_name(&port->dev), dev_name(port->uport_dev)); - rc = cxl_add_ep(dport, &cxlmd->dev); - if (rc == -EBUSY) { - /* - * "can't" happen, but this error code means - * something to the caller, so translate it. - */ - rc = -ENXIO; - } - put_device(&port->dev); - } + rc = cxl_add_ep(dport, &cxlmd->dev); - put_device(&parent_port->dev); + /* translate EBUSY as fatal */ + if (rc == -EBUSY) + rc = -ENXIO; return rc; } @@ -1678,7 +1700,7 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd) return 0; } - rc = add_port_attach_ep(cxlmd, uport_dev, dport_dev); + rc = add_port_attach_ep(cxlmd, dport_dev); /* port missing, try to add parent */ if (rc == -EAGAIN) continue; diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h index 5406e3ab3d4a..31dadc128b4e 100644 --- a/drivers/cxl/cxl.h +++ b/drivers/cxl/cxl.h @@ -746,6 +746,7 @@ void put_cxl_root(struct cxl_root *cxl_root); DEFINE_FREE(put_cxl_root, struct cxl_root *, if (_T) put_cxl_root(_T)) DEFINE_FREE(put_cxl_port, struct cxl_port *, if (!IS_ERR_OR_NULL(_T)) put_device(&_T->dev)) +DEFINE_FREE(put_cxl_dport, struct cxl_dport *, if (!IS_ERR_OR_NULL(_T)) put_device(&_T->port->dev)) int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd); void cxl_bus_rescan(void); void cxl_bus_drain(void);