* [PATCH] nvme: nvme_remove should cancel the reset_work before setting the ctrl state
@ 2017-12-05 19:01 Thomas Tai
2017-12-05 20:09 ` Keith Busch
0 siblings, 1 reply; 3+ messages in thread
From: Thomas Tai @ 2017-12-05 19:01 UTC (permalink / raw)
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
Signed-off-by: Thomas Tai <thomas.tai at oracle.com>
Reviewed-by: Kyle Fortin <kyle.fortin at oracle.com>
---
drivers/nvme/host/pci.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index a11cfd4..e2d10f9 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -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)) {
--
1.8.3.1
^ permalink raw reply related [flat|nested] 3+ messages in thread* [PATCH] nvme: nvme_remove should cancel the reset_work before setting the ctrl state
2017-12-05 19:01 [PATCH] nvme: nvme_remove should cancel the reset_work before setting the ctrl state Thomas Tai
@ 2017-12-05 20:09 ` Keith Busch
2017-12-05 20:43 ` Thomas Tai
0 siblings, 1 reply; 3+ messages in thread
From: Keith Busch @ 2017-12-05 20:09 UTC (permalink / raw)
On Tue, Dec 05, 2017@12:01:35PM -0700, Thomas Tai wrote:
> 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".
The message seems to make sense. You requested to unbind the driver
before the controller went live.
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index a11cfd4..e2d10f9 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -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.
^ permalink raw reply [flat|nested] 3+ messages in thread* [PATCH] nvme: nvme_remove should cancel the reset_work before setting the ctrl state
2017-12-05 20:09 ` Keith Busch
@ 2017-12-05 20:43 ` Thomas Tai
0 siblings, 0 replies; 3+ messages in thread
From: Thomas Tai @ 2017-12-05 20:43 UTC (permalink / raw)
On 2017-12-05 03:09 PM, Keith Busch wrote:
> On Tue, Dec 05, 2017@12:01:35PM -0700, Thomas Tai wrote:
>> 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".
>
> The message seems to make sense. You requested to unbind the driver
> before the controller went live.
>
>> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
>> index a11cfd4..e2d10f9 100644
>> --- a/drivers/nvme/host/pci.c
>> +++ b/drivers/nvme/host/pci.c
>> @@ -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.
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
>
> _______________________________________________
> Linux-nvme mailing list
> Linux-nvme at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-nvme
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-12-05 20:43 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-12-05 19:01 [PATCH] nvme: nvme_remove should cancel the reset_work before setting the ctrl state Thomas Tai
2017-12-05 20:09 ` Keith Busch
2017-12-05 20:43 ` Thomas Tai
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.