From patchwork Sat Apr 23 21:05:20 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Dan Williams X-Patchwork-Id: 12824682 Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) (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 4CF867C for ; Sat, 23 Apr 2022 21:05:21 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1650747921; x=1682283921; h=subject:from:to:cc:date:message-id:in-reply-to: references:mime-version:content-transfer-encoding; bh=u2uujpIWafLuqRQHmmYfUJuJS1w46wzg590lAQIVpiY=; b=W3jq0zX/txDa0FFCKYob99yIHrkMsSY+yg3eCZK4X+WhfNmnw5Z2QJqA sVm3aFg1kN01f0qGlmuOBY/rBM3SxlWKVRThnk/yKXa9FOlNmRJ+zEIHN lLOdY4f4wemWVp0UzcSCXLlzWuFYq9bFUqfErZEDcjpLWT3Rs1hSgrW9C 0ZqrPn9Pwh0hVOrcIz+lMG6ojFeAv19+zVHWP0c4pgnguZOcwppPknRQ9 +OEKBbvO4gSG6fSK+9TP8jcJbh4OYUIe17pukFLYvDO/Xh95MDE+u13M4 Ka4zmpdC2dQK6tv6wLE66++G40ioLEuUbcGtXNsGQxlW0oU4esKnetOcf A==; X-IronPort-AV: E=McAfee;i="6400,9594,10326"; a="252296279" X-IronPort-AV: E=Sophos;i="5.90,285,1643702400"; d="scan'208";a="252296279" Received: from orsmga005.jf.intel.com ([10.7.209.41]) by orsmga101.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 23 Apr 2022 14:05:20 -0700 X-IronPort-AV: E=Sophos;i="5.90,285,1643702400"; d="scan'208";a="729057744" Received: from dwillia2-desk3.jf.intel.com (HELO dwillia2-desk3.amr.corp.intel.com) ([10.54.39.25]) by orsmga005-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 23 Apr 2022 14:05:20 -0700 Subject: [PATCH v4 2/8] cxl/acpi: Add root device lockdep validation From: Dan Williams To: linux-cxl@vger.kernel.org Cc: Peter Zijlstra , "Rafael J. Wysocki" , Ingo Molnar , Will Deacon , Waiman Long , Boqun Feng , Alison Schofield , Vishal Verma , Ben Widawsky , Jonathan Cameron , Greg Kroah-Hartman , Ira Weiny , nvdimm@lists.linux.dev Date: Sat, 23 Apr 2022 14:05:20 -0700 Message-ID: <165074768280.4047093.15803516065715087623.stgit@dwillia2-desk3.amr.corp.intel.com> In-Reply-To: <165055519869.3745911.10162603933337340370.stgit@dwillia2-desk3.amr.corp.intel.com> References: <165055519869.3745911.10162603933337340370.stgit@dwillia2-desk3.amr.corp.intel.com> User-Agent: StGit/0.18-3-g996c Precedence: bulk X-Mailing-List: nvdimm@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 The CXL "root" device, ACPI0017, is an attach point for coordinating platform level CXL resources and is the parent device for a CXL port topology tree. As such it has distinct locking rules relative to other CXL subsystem objects, but because it is an ACPI device the lock class is established well before it is given to the cxl_acpi driver. However, the lockdep API does support changing the lock class "live" for situations like this. Add a device_lock_set_class() helper that a driver can use in ->probe() to set a custom lock class, and device_lock_reset_class() to return to the default "no validate" class before the custom lock class key goes out of scope after ->remove(). Note the helpers are all macros to support dead code elimination in the CONFIG_PROVE_LOCKING=n case. Suggested-by: Peter Zijlstra Cc: "Rafael J. Wysocki" Cc: Ingo Molnar Cc: Will Deacon Cc: Waiman Long Cc: Boqun Feng Cc: Alison Schofield Cc: Vishal Verma Cc: Ben Widawsky Cc: Jonathan Cameron Reviewed-by: Greg Kroah-Hartman Reviewed-by: Ira Weiny Signed-off-by: Dan Williams --- Changes since v3: - Add a comment about expected usage of device_lock_set_class() to only override the "no validate" class (Ira) - Clang-format the macros - Fix device_lock_reset_class() to reset the lock class name to "&dev->mutex". drivers/cxl/acpi.c | 15 +++++++++++++++ include/linux/device.h | 25 +++++++++++++++++++++++++ 2 files changed, 40 insertions(+) diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c index d15a6aec0331..e19cea27387e 100644 --- a/drivers/cxl/acpi.c +++ b/drivers/cxl/acpi.c @@ -275,6 +275,15 @@ static int add_root_nvdimm_bridge(struct device *match, void *data) return 1; } +static struct lock_class_key cxl_root_key; + +static void cxl_acpi_lock_reset_class(void *_dev) +{ + struct device *dev = _dev; + + device_lock_reset_class(dev); +} + static int cxl_acpi_probe(struct platform_device *pdev) { int rc; @@ -283,6 +292,12 @@ static int cxl_acpi_probe(struct platform_device *pdev) struct acpi_device *adev = ACPI_COMPANION(host); struct cxl_cfmws_context ctx; + device_lock_set_class(&pdev->dev, &cxl_root_key); + rc = devm_add_action_or_reset(&pdev->dev, cxl_acpi_lock_reset_class, + &pdev->dev); + if (rc) + return rc; + root_port = devm_cxl_add_port(host, host, CXL_RESOURCE_NONE, NULL); if (IS_ERR(root_port)) return PTR_ERR(root_port); diff --git a/include/linux/device.h b/include/linux/device.h index 93459724dcde..a4a7c1bf3b72 100644 --- a/include/linux/device.h +++ b/include/linux/device.h @@ -850,6 +850,31 @@ static inline bool device_supports_offline(struct device *dev) return dev->bus && dev->bus->offline && dev->bus->online; } +#define __device_lock_set_class(dev, name, key) \ + lock_set_class(&(dev)->mutex.dep_map, name, key, 0, _THIS_IP_) + +/** + * device_lock_set_class - Specify a temporary lock class while a device + * is attached to a driver + * @dev: device to modify + * @key: lock class key data + * + * This must be called with the device_lock() already held, for example + * from driver ->probe(). Take care to only override the default + * lockdep_no_validate class. + */ +#define device_lock_set_class(dev, key) __device_lock_set_class(dev, #key, key) + +/** + * device_lock_reset_class - Return a device to the default lockdep novalidate state + * @dev: device to modify + * + * This must be called with the device_lock() already held, for example + * from driver ->remove(). + */ +#define device_lock_reset_class(dev) \ + __device_lock_set_class(dev, "&dev->mutex", &__lockdep_no_validate__) + void lock_device_hotplug(void); void unlock_device_hotplug(void); int lock_device_hotplug_sysfs(void); From patchwork Sat Apr 23 21:22:18 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Dan Williams X-Patchwork-Id: 12824684 Received: from mga06.intel.com (mga06b.intel.com [134.134.136.31]) (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 5FF757C for ; Sat, 23 Apr 2022 21:22:19 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1650748939; x=1682284939; h=subject:from:to:cc:date:message-id:in-reply-to: references:mime-version:content-transfer-encoding; bh=tWkequjSYEh30bCG4fPlkdokflxW/XwwcXiDxXk3wWw=; b=ZLkL8bd7zu/z9JmRsQMuuejFr7DMXDUAZBV3v+PSQVY8Z8Ws81rtEqKw /B+ebigwfeppDi6Y4Ijw+sI1bgdoyQDrmnQV+tbR3sq7V/mVkzrqGHIDZ nOFyVrtpJAFW8jjjnxHHztcjmXyzG6AB/TTlG/9H2RyQUmI1/Lvyd5vCQ 6pEPc9athS88K3rjuUMY6hdZy8TQlA8pS3S38dA/6UZhIeKXQpyfb3GJG EVRyjaDb41oGbALWUJLhU3vXMcGwAN96OX3GJAaUowW55p99y5NIOa9up S26U7qVS223yBWUN0s5HQg5Z+eO1cdON6X1GFRu9isVxCdiS1u6PWVfMH w==; X-IronPort-AV: E=McAfee;i="6400,9594,10326"; a="325417699" X-IronPort-AV: E=Sophos;i="5.90,285,1643702400"; d="scan'208";a="325417699" Received: from orsmga007.jf.intel.com ([10.7.209.58]) by orsmga104.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 23 Apr 2022 14:22:18 -0700 X-IronPort-AV: E=Sophos;i="5.90,285,1643702400"; d="scan'208";a="557009140" Received: from dwillia2-desk3.jf.intel.com (HELO dwillia2-desk3.amr.corp.intel.com) ([10.54.39.25]) by orsmga007-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 23 Apr 2022 14:22:18 -0700 Subject: [PATCH v4 8/8] nvdimm: Fix firmware activation deadlock scenarios From: Dan Williams To: linux-cxl@vger.kernel.org Cc: nvdimm@lists.linux.dev Date: Sat, 23 Apr 2022 14:22:18 -0700 Message-ID: <165074883800.4116052.10737040861825806582.stgit@dwillia2-desk3.amr.corp.intel.com> In-Reply-To: <165055523099.3745911.9091010720291846249.stgit@dwillia2-desk3.amr.corp.intel.com> References: <165055523099.3745911.9091010720291846249.stgit@dwillia2-desk3.amr.corp.intel.com> User-Agent: StGit/0.18-3-g996c Precedence: bulk X-Mailing-List: nvdimm@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Lockdep reports the following deadlock scenarios for CXL root device power-management, device_prepare(), operations, and device_shutdown() operations for 'nd_region' devices: Reviewed-by: Ira Weiny --- Chain exists of: &nvdimm_region_key --> &nvdimm_bus->reconfig_mutex --> system_transition_mutex Possible unsafe locking scenario: CPU0 CPU1 ---- ---- lock(system_transition_mutex); lock(&nvdimm_bus->reconfig_mutex); lock(system_transition_mutex); lock(&nvdimm_region_key); -- Chain exists of: &cxl_nvdimm_bridge_key --> acpi_scan_lock --> &cxl_root_key Possible unsafe locking scenario: CPU0 CPU1 ---- ---- lock(&cxl_root_key); lock(acpi_scan_lock); lock(&cxl_root_key); lock(&cxl_nvdimm_bridge_key); --- These stem from holding nvdimm_bus_lock() over hibernate_quiet_exec() which walks the entire system device topology taking device_lock() along the way. The nvdimm_bus_lock() is protecting against unregistration, multiple simultaneous ops callers, and preventing activate_show() from racing activate_store(). For the first 2, the lock is redundant. Unregistration already flushes all ops users, and sysfs already prevents multiple threads to be active in an ops handler at the same time. For the last userspace should already be waiting for its last activate_store() to complete, and does not need activate_show() to flush the write side, so this lock usage can be deleted in these attributes. Fixes: 48001ea50d17 ("PM, libnvdimm: Add runtime firmware activation support") Signed-off-by: Dan Williams --- Changes since v3: - Remove nvdimm_bus_lock() from all ->capability() invocations (Ira) drivers/nvdimm/core.c | 9 --------- 1 file changed, 9 deletions(-) diff --git a/drivers/nvdimm/core.c b/drivers/nvdimm/core.c index 144926b7451c..d91799b71d23 100644 --- a/drivers/nvdimm/core.c +++ b/drivers/nvdimm/core.c @@ -368,9 +368,7 @@ static ssize_t capability_show(struct device *dev, if (!nd_desc->fw_ops) return -EOPNOTSUPP; - nvdimm_bus_lock(dev); cap = nd_desc->fw_ops->capability(nd_desc); - nvdimm_bus_unlock(dev); switch (cap) { case NVDIMM_FWA_CAP_QUIESCE: @@ -395,10 +393,8 @@ static ssize_t activate_show(struct device *dev, if (!nd_desc->fw_ops) return -EOPNOTSUPP; - nvdimm_bus_lock(dev); cap = nd_desc->fw_ops->capability(nd_desc); state = nd_desc->fw_ops->activate_state(nd_desc); - nvdimm_bus_unlock(dev); if (cap < NVDIMM_FWA_CAP_QUIESCE) return -EOPNOTSUPP; @@ -443,7 +439,6 @@ static ssize_t activate_store(struct device *dev, else return -EINVAL; - nvdimm_bus_lock(dev); state = nd_desc->fw_ops->activate_state(nd_desc); switch (state) { @@ -461,7 +456,6 @@ static ssize_t activate_store(struct device *dev, default: rc = -ENXIO; } - nvdimm_bus_unlock(dev); if (rc == 0) rc = len; @@ -484,10 +478,7 @@ static umode_t nvdimm_bus_firmware_visible(struct kobject *kobj, struct attribut if (!nd_desc->fw_ops) return 0; - nvdimm_bus_lock(dev); cap = nd_desc->fw_ops->capability(nd_desc); - nvdimm_bus_unlock(dev); - if (cap < NVDIMM_FWA_CAP_QUIESCE) return 0;