On Sat, Apr 23, 2022 at 02:22:18PM -0700, Dan Williams wrote:
> Lockdep reports the following deadlock scenarios for CXL root device
> power-management, device_prepare(), operations, and device_shutdown()
> operations for 'nd_region' devices:
>
> ---
> 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 <[email protected]>
Reviewed-by: Ira Weiny <[email protected]>
> ---
> 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;
>
>
