| Message ID | 165074883800.4116052.10737040861825806582.stgit@dwillia2-desk3.amr.corp.intel.com |
|---|---|
| State | Accepted |
| Commit | e6829d1bd3c4b58296ee9e412f7ed4d6cb390192 |
| Headers | show |
| Series | None | expand |
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 <dan.j.williams@intel.com> Reviewed-by: Ira Weiny <ira.weiny@intel.com> > --- > 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; > >
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;
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 <dan.j.williams@intel.com> --- Changes since v3: - Remove nvdimm_bus_lock() from all ->capability() invocations (Ira) drivers/nvdimm/core.c | 9 --------- 1 file changed, 9 deletions(-)