nvme: nvme_remove should cancel the reset_work before setting the ctrl state
PATCH 1/1 nvme: nvme_remove should cancel the reset_work before setting the ctrl state
During nvme_probe, a reset_work task is queued up for execution. When removing the nvme during unbinding, the remove function set the ctrl state to NVME_CTRL_DELETING and then cancel the reset_work. The correct sequence should have been cancel the reset_work first then change the state.
Otherwise, if the reset_work happens to schedule to work, the NVME_CTRL_DELETING causes the reset_work function to fail with "failed to make controller live".
Steps to reproduce the problem:
drive="0000:14:00.0"
for i in {1..20};
do
echo iteration $i
driverctl set-override $drive nvme
done
--- drivers/nvme/host/pci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/nvme/host/pci.c +1 -1
@@ -2534,9 +2534,9 @@ static void nvme_remove(struct pci_dev *pdev) {struct nvme_dev *dev =pci_get_drvdata (pdev); -nvme_change_ctrl_state (&dev->ctrl,NVME_CTRL_DELETING );cancel_work_sync (&dev->ctrl.reset_work); +nvme_change_ctrl_state (&dev->ctrl,NVME_CTRL_DELETING );pci_set_drvdata (pdev,NULL );if (!pci_device_is_present (pdev)) {
Signature · 1 line
1.8.3.1
PATCH 1/1 nvme: nvme_remove should cancel the reset_work before setting the ctrl state
Quoted 9 lines · Thomas Tai
During
nvme_probe, areset_worktask is queued up for execution. When removing the nvme during unbinding, the remove function set the ctrl state toNVME_CTRL_DELETINGand then cancel thereset_work. The correct sequence should have been cancel thereset_workfirst then change the state.Otherwise, if the
reset_workhappens to schedule to work, theNVME_CTRL_DELETINGcauses thereset_workfunction to fail with "failed to make controller live".
The message seems to make sense. You requested to unbind the driver before the controller went live.
drivers/nvme/host/pci.c static void nvme_remove(struct pci_dev *pdev) · line 2540
Quoted 13 lines
drivers/nvme/host/pci.c +1
-1@@ -2534,9 +2534,9 @@ static void nvme_remove(struct pci_dev *pdev) {struct nvme_dev *dev =pci_get_drvdata (pdev); -nvme_change_ctrl_state (&dev->ctrl,NVME_CTRL_DELETING );cancel_work_sync (&dev->ctrl.reset_work); +nvme_change_ctrl_state (&dev->ctrl,NVME_CTRL_DELETING );pci_set_drvdata (pdev,NULL );
The state is changed before synchronously cancelling the reset work so that nvme_remove is guaranteed another reset can't possibly occur while we're deleting the controller. This change provies a small window for which that could happen, and bad things will happen if it does.
PATCH 1/1 nvme: nvme_remove should cancel the reset_work before setting the ctrl state
Quoted 24 lines · Keith Busch
Quoted 9 lines · Thomas Tai
During
nvme_probe, areset_worktask is queued up for execution. When removing the nvme during unbinding, the remove function set the ctrl state toNVME_CTRL_DELETINGand then cancel thereset_work. The correct sequence should have been cancel thereset_workfirst then change the state.Otherwise, if the
reset_workhappens to schedule to work, theNVME_CTRL_DELETINGcauses thereset_workfunction to fail with "failed to make controller live".The message seems to make sense. You requested to unbind the driver before the controller went live.
Quoted 10 lines
drivers/nvme/host/pci.c +0
-1@@ -2534,9 +2534,9 @@ static void nvme_remove(struct pci_dev *pdev) {struct nvme_dev *dev =pci_get_drvdata (pdev); -nvme_change_ctrl_state (&dev->ctrl,NVME_CTRL_DELETING );
Quoted 8 lines
drivers/nvme/host/pci.c static void nvme_remove(struct pci_dev *pdev) · line 2540
Quoted 3 lines
drivers/nvme/host/pci.c +1
-0cancel_work_sync (&dev->ctrl.reset_work); +nvme_change_ctrl_state (&dev->ctrl,NVME_CTRL_DELETING );pci_set_drvdata (pdev,NULL );The state is changed before synchronously cancelling the reset work so that
nvme_removeis guaranteed another reset can't possibly occur while we're deleting the controller. This change provies a small window for which that could happen, and bad things will happen if it does.
Hi Keith,
Thank you for your explanation. So changing the state before cancel_work_sync() is the right way to go then.
Thank you,
Thomas
Quoted 6 lines
_______________________________________________
Linux-nvme mailing list
Linux-nvme at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme