NVMe: Lock out shutdown during pci init
PATCH 1/2 NVMe: Lock out shutdown during pci init
An unexpected driver unbind needs to be prevented from unmapping the pci nvme registers while they're being initialized.
--- drivers/nvme/host/pci.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)
drivers/nvme/host/pci.c +10 -2
@@ -1927,13 +1927,19 @@ static void nvme_reset_work(struct work_struct *work)set_bit (NVME_CTRL_RESETTING , &dev->flags); +/* +* Lock out shutdown so an unexpected driver removal won't unmap +* registers while reset work is setting them up. +*/ +mutex_lock (&dev->shutdown_lock); result =nvme_dev_map (dev);if (result) - goto out; + goto out_unlock; result =nvme_configure_admin_queue (dev);if (result) - goto out; + goto out_unlock; +mutex_unlock (&dev->shutdown_lock);nvme_init_queue (dev->queues[0 ],0 ); result =nvme_alloc_admin_tags (dev);
@@ -1970,6 +1976,8 @@ static void nvme_reset_work(struct work_struct *work)clear_bit (NVME_CTRL_RESETTING , &dev->flags);return ; + out_unlock: +mutex_unlock (&dev->shutdown_lock); out:nvme_remove_dead_ctrl (dev, result); }
Signature · 1 line
2.6.2.307.g37023ba
PATCH 2/2 NVMe: NULL pointer check for admin queues
Admin commands don't have a namespace, so don't check the state when there isn't one.
An admin command that does happen to make it to a disabled queue can failed immediatly: Initialization commands should not be requeued across resets as they would mess up the controller if/when it is reinitialized. If a user command made hits this condition, it should be able to handle this error as that's the existing result for commands when the controller is reset with outstanding admin commands.
--- drivers/nvme/host/pci.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
drivers/nvme/host/pci.c +6 -1
@@ -678,7 +678,12 @@ static int nvme_queue_rq(struct blk_mq_hw_ctx *hctx,spin_lock_irq (&nvmeq->q_lock);if (unlikely (nvmeq->cq_vector <0 )) { - ret =test_bit (NVME_NS_DEAD , &ns->flags) ? +/* +* Admin commands need to fail immediatly when device is +* disabled: initialization commands should not requeue +* across resets. +*/ + ret = !ns ||test_bit (NVME_NS_DEAD , &ns->flags) ?BLK_MQ_RQ_QUEUE_ERROR :BLK_MQ_RQ_QUEUE_BUSY ;spin_unlock_irq (&nvmeq->q_lock);
Signature · 1 line
2.6.2.307.g37023ba
PATCH 2/2 NVMe: NULL pointer check for admin queues
Quoted 9 lines · Keith Busch
Admin commands don't have a namespace, so don't check the state when there isn't one.
An admin command that does happen to make it to a disabled queue can failed immediatly: Initialization commands should not be requeued across resets as they would mess up the controller if/when it is reinitialized. If a user command made hits this condition, it should be able to handle this error as that's the existing result for commands when the controller is reset with outstanding admin commands.
Looks fine, but I'd say fold this directly into the respin of the patch introducing the check.
PATCH 2/2 NVMe: NULL pointer check for admin queues
In reply to Keith BuschLooks good,
PATCH 1/2 NVMe: Lock out shutdown during pci init
In reply to Keith BuschQuoted 21 lines · Keith Busch
An unexpected driver unbind needs to be prevented from unmapping the pci nvme registers while they're being initialized.
Signed-off-by Keith Busch <keith.busch at intel.com>--- drivers/nvme/host/pci.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)drivers/nvme/host/pci.c +5
-0@@ -1927,13 +1927,19 @@ static void nvme_reset_work(struct work_struct *work)set_bit (NVME_CTRL_RESETTING , &dev->flags); +/* +* Lock out shutdown so an unexpected driver removal won't unmap +* registers while reset work is setting them up. +*/ +mutex_lock (&dev->shutdown_lock);
drivers/nvme/host/pci.c static void nvme_reset_work(struct work_struct *work) · line 1942
Quoted 10 lines
drivers/nvme/host/pci.c +3
-2result =nvme_dev_map (dev);if (result) - goto out; + goto out_unlock; result =nvme_configure_admin_queue (dev);if (result) - goto out; + goto out_unlock; +mutex_unlock (&dev->shutdown_lock);
Seems like we should be holding the lock over the call to nvme_dev_disable to get protection for the whole execution, e.g. by adding a __nvme_dev_disable variant that expects the lock to be held.
Maybe it's also worth renaming shutdown_lock to register_mutex or similar to better document what it protects.
Quoted 20 lines
drivers/nvme/host/pci.c +2
-0nvme_init_queue (dev->queues[0 ],0 ); result =nvme_alloc_admin_tags (dev);@@ -1970,6 +1976,8 @@ static void nvme_reset_work(struct work_struct *work)clear_bit (NVME_CTRL_RESETTING , &dev->flags);return ; + out_unlock: +mutex_unlock (&dev->shutdown_lock); out:nvme_remove_dead_ctrl (dev, result); }Signature · 7 lines
2.6.2.307.g37023ba _______________________________________________ Linux-nvme mailing list Linux-nvme at lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme
---end quoted text---
PATCH 1/2 NVMe: Lock out shutdown during pci init
Quoted 7 lines · Christoph Hellwig
Seems like we should be holding the lock over the call to
nvme_dev_disableto get protection for the whole execution, e.g. by adding a__nvme_dev_disablevariant that expects the lock to be held.Maybe it's also worth renaming
shutdown_locktoregister_mutexor similar to better document what it protects.
Perhaps we shouldn't unmap the registers on a reset. That'd fix several problems, including this one and writing queue doorbells.
PATCH 1/2 NVMe: Lock out shutdown during pci init
Quoted 11 lines · Keith Busch
Quoted 7 lines · Christoph Hellwig
Seems like we should be holding the lock over the call to
nvme_dev_disableto get protection for the whole execution, e.g. by adding a__nvme_dev_disablevariant that expects the lock to be held.Maybe it's also worth renaming
shutdown_locktoregister_mutexor similar to better document what it protects.Perhaps we shouldn't unmap the registers on a reset. That'd fix several problems, including this one and writing queue doorbells.
I always thought there was a deep reason we'd need to unmap it. But if there isn't getting rid of the unmap should make the driver a lot more robust.
PATCH 1/2 NVMe: Lock out shutdown during pci init
Quoted 3 lines · Christoph Hellwig
I always thought there was a deep reason we'd need to unmap it. But if there isn't getting rid of the unmap should make the driver a lot more robust.
Remapping after each reset was prepping for a non-existent feature. We'd like to create domains safe for the PCI core to reassign bus numbers and memory windows on a live system for various hot plug scenarios, but I don't see that happening soon.