Avoid reset work on watchdog timer function during error recovery

PATCH New Guilherme G. Piccoli 5 participants 13 messages Apr 6, 2016 – Apr 12, 2016 NVMe

Share & tools

Guilherme G. Piccoli

author

2016-04-06 20:30 UTC #0 lore raw

PATCH 1/1 Avoid reset work on watchdog timer function during error recovery

This patch adds a check on nvme_watchdog_timer() function to avoid the call to reset_work() when an error recovery process is ongoing on controller. The check is made by looking at pci_channel_offline() result.

If we don't check for this on nvme_watchdog_timer(), error recovery mechanism can't recover well, because reset_work() won't be able to do its job (since we're in the middle of an error) and so the controller is removed from the system before error recovery mechanism can perform slot reset (which would allow the adapter to recover).

Signed-off-by Guilherme G. Piccoli <gpiccoli at linux.vnet.ibm.com>

---
 drivers/nvme/host/pci.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

drivers/nvme/host/pci.c +3 -1

index 24ccda3..3cc146d 100644
@@ -1357,10 +1357,12 @@ static void nvme_watchdog_timer(unsigned long data)
 
 	/*
 	 * Skip controllers currently under reset.
+	 * Also, skip controllers going through PCI error recovery.
 	 */
 	if (!work_pending(&dev->reset_work) && !work_busy(&dev->reset_work) &&
 	    ((csts & NVME_CSTS_CFS) ||
-	     (dev->subsystem && (csts & NVME_CSTS_NSSRO)))) {
+	    (dev->subsystem && (csts & NVME_CSTS_NSSRO))) &&
+	    !pci_channel_offline(to_pci_dev(dev->dev))) {
 		if (queue_work(nvme_workq, &dev->reset_work)) {
 			dev_warn(dev->dev,
 				"Failed status: 0x%x, reset controller.\n",

Signature · 1 line

2.1.0

Keith Busch

2016-04-06 20:50 UTC #1 lore raw

PATCH 1/1 Avoid reset work on watchdog timer function during error recovery

Quoted 5 lines · Guilherme G. Piccoli

This patch adds a check on nvme_watchdog_timer() function to avoid the call to reset_work() when an error recovery process is ongoing on controller. The check is made by looking at pci_channel_offline() result.

Quoted 7 lines

If we don't check for this on nvme_watchdog_timer(), error recovery mechanism can't recover well, because reset_work() won't be able to do its job (since we're in the middle of an error) and so the controller is removed from the system before error recovery mechanism can perform slot reset (which would allow the adapter to recover).
Signed-off-by Guilherme G. Piccoli <gpiccoli at linux.vnet.ibm.com>

Reviewed-by Keith Busch <keith.busch at intel.com>

Johannes Thumshirn

2016-04-07 08:05 UTC #2 lore raw

PATCH 1/1 Avoid reset work on watchdog timer function during error recovery

In reply to Guilherme G. Piccoli

Quoted 5 lines · On Mittwoch, 6. April 2016 17:30:35 CEST Guilherme G. Piccoli wrote:

This patch adds a check on nvme_watchdog_timer() function to avoid the call to reset_work() when an error recovery process is ongoing on controller. The check is made by looking at pci_channel_offline() result.

Quoted 7 lines

If we don't check for this on nvme_watchdog_timer(), error recovery mechanism can't recover well, because reset_work() won't be able to do its job (since we're in the middle of an error) and so the controller is removed from the system before error recovery mechanism can perform slot reset (which would allow the adapter to recover).
Signed-off-by Guilherme G. Piccoli <gpiccoli at linux.vnet.ibm.com>

Reviewed-by Johannes Thumshirn <jthumshirn at suse.de>

Signature · 6 lines

Johannes Thumshirn                                          Storage
jthumshirn at suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Christoph Hellwig

2016-04-07 13:11 UTC #3 lore raw

PATCH 1/1 Avoid reset work on watchdog timer function during error recovery

In reply to Guilherme G. Piccoli

drivers/nvme/host/pci.c static void nvme_watchdog_timer(unsigned long data) · line 1368

Quoted 12 lines · Guilherme G. Piccoli

drivers/nvme/host/pci.c +3 -1

 	/*
 	 * Skip controllers currently under reset.
+	 * Also, skip controllers going through PCI error recovery.
 	 */
 	if (!work_pending(&dev->reset_work) && !work_busy(&dev->reset_work) &&
 	    ((csts & NVME_CSTS_CFS) ||
-	     (dev->subsystem && (csts & NVME_CSTS_NSSRO)))) {
+	    (dev->subsystem && (csts & NVME_CSTS_NSSRO))) &&
+	    !pci_channel_offline(to_pci_dev(dev->dev))) {
 		if (queue_work(nvme_workq, &dev->reset_work)) {
 			dev_warn(dev->dev,
 				"Failed status: 0x%x, reset controller.\n",

This looks correct to me, but the condition is getting basically unreadable. Can you factor it out into a little helper returns a boolean value if we should reset or not, and document each condition, e.g. something like the function below, just with proper comments:

static bool nvme_should_reset(struct nvme_dev *dev)

{
	u32 csts = readl(dev->bar + NVME_REG_CSTS);

	if (!(csts & NVME_CSTS_CFS) &&
	    !((dev->subsystem && (csts & NVME_CSTS_NSSRO))))
		return false;

	if (work_pending(&dev->reset_work))
		return false;
	if (work_busy(&dev->reset_work))
		return false;
	if (pci_channel_offline(to_pci_dev(dev->dev))
		return false;

	return true;
}

Guilherme G. Piccoli

author

2016-04-07 13:17 UTC #4 lore raw

PATCH 1/1 Avoid reset work on watchdog timer function during error recovery

Quoted 20 lines · Christoph Hellwig

drivers/nvme/host/pci.c static void nvme_watchdog_timer(unsigned long data) · line 1368
Quoted 12 lines · Guilherme G. Piccoli
/*
* Skip controllers currently under reset.
drivers/nvme/host/pci.c +3 -1
+	 * Also, skip controllers going through PCI error recovery.
  	 */
  	if (!work_pending(&dev->reset_work) && !work_busy(&dev->reset_work) &&
  	    ((csts & NVME_CSTS_CFS) ||
-	     (dev->subsystem && (csts & NVME_CSTS_NSSRO)))) {
+	    (dev->subsystem && (csts & NVME_CSTS_NSSRO))) &&
+	    !pci_channel_offline(to_pci_dev(dev->dev))) {
  		if (queue_work(nvme_workq, &dev->reset_work)) {
  			dev_warn(dev->dev,
  				"Failed status: 0x%x, reset controller.\n",
This looks correct to me, but the condition is getting basically unreadable. Can you factor it out into a little helper returns a boolean value if we should reset or not, and document each condition, e.g. something like the function below, just with proper comments:
static bool nvme_should_reset(struct nvme_dev *dev)

Quoted 17 lines

{
	u32 csts = readl(dev->bar + NVME_REG_CSTS);

	if (!(csts & NVME_CSTS_CFS) &&
	    !((dev->subsystem && (csts & NVME_CSTS_NSSRO))))
		return false;

	if (work_pending(&dev->reset_work))
		return false;
	if (work_busy(&dev->reset_work))
		return false;
	if (pci_channel_offline(to_pci_dev(dev->dev))
		return false;

	return true;
}

Heheh agreed, very good suggestion indeed. I found myself struggling to understand this huge condition line. I'll improve it following your suggestion, thanks.

Cheers,

Guilherme

Sagi Grimberg

maintainer

2016-04-07 13:18 UTC #5 lore raw

PATCH 1/1 Avoid reset work on watchdog timer function during error recovery

In reply to Christoph Hellwig

Quoted 17 lines · Christoph Hellwig

drivers/nvme/host/pci.c static void nvme_watchdog_timer(unsigned long data) · line 1368
Quoted 12 lines · Guilherme G. Piccoli
/*
* Skip controllers currently under reset.
drivers/nvme/host/pci.c +3 -1
+	 * Also, skip controllers going through PCI error recovery.
  	 */
  	if (!work_pending(&dev->reset_work) && !work_busy(&dev->reset_work) &&
  	    ((csts & NVME_CSTS_CFS) ||
-	     (dev->subsystem && (csts & NVME_CSTS_NSSRO)))) {
+	    (dev->subsystem && (csts & NVME_CSTS_NSSRO))) &&
+	    !pci_channel_offline(to_pci_dev(dev->dev))) {
  		if (queue_work(nvme_workq, &dev->reset_work)) {
  			dev_warn(dev->dev,
  				"Failed status: 0x%x, reset controller.\n",
This looks correct to me, but the condition is getting basically unreadable. Can you factor it out into a little helper returns a boolean value if we should reset or not, and document each condition, e.g. something like the function below, just with proper comments:

Agreed, but nvme_should_reset() sounds like you are checking if a reset is needed, maybe call it nvme_can_reset()?

Quoted 19 lines

static bool nvme_should_reset(struct nvme_dev *dev)

{
	u32 csts = readl(dev->bar + NVME_REG_CSTS);

	if (!(csts & NVME_CSTS_CFS) &&
	    !((dev->subsystem && (csts & NVME_CSTS_NSSRO))))
		return false;

	if (work_pending(&dev->reset_work))
		return false;
	if (work_busy(&dev->reset_work))
		return false;
	if (pci_channel_offline(to_pci_dev(dev->dev))
		return false;

	return true;
}

Christoph Hellwig

2016-04-07 13:26 UTC #6 lore raw

PATCH 1/1 Avoid reset work on watchdog timer function during error recovery

Quoted 2 lines · Sagi Grimberg

Agreed, but nvme_should_reset() sounds like you are checking if a reset is needed,

But that's exaxtly what it does! It reads CSTS to check for one of the two bits in there indicating that we should initiate a reset, except for various conditions why we think we better shouldn't reset at the moment..

Keith Busch

2016-04-07 14:24 UTC #7 lore raw

PATCH 1/1 Avoid reset work on watchdog timer function during error recovery

In reply to Christoph Hellwig

Quoted 12 lines · Christoph Hellwig

static bool nvme_should_reset(struct nvme_dev *dev)

{
	u32 csts = readl(dev->bar + NVME_REG_CSTS);

	if (!(csts & NVME_CSTS_CFS) &&
	    !((dev->subsystem && (csts & NVME_CSTS_NSSRO))))
		return false;

	if (work_pending(&dev->reset_work))
		return false;
	if (work_busy(&dev->reset_work))
		return false;

We could also skip checking work_pending since work_busy does that for us.

Guilherme G. Piccoli

author

2016-04-07 15:42 UTC #8 lore raw

PATCH 1/1 Avoid reset work on watchdog timer function during error recovery

Quoted 9 lines · Keith Busch

Quoted 8 lines · Christoph Hellwig
static bool nvme_should_reset(struct nvme_dev *dev)
{
	u32 csts = readl(dev->bar + NVME_REG_CSTS);

	if (!(csts & NVME_CSTS_CFS) &&
	    !((dev->subsystem && (csts & NVME_CSTS_NSSRO))))
		return false;

Quoted 7 lines

Quoted 4 lines
	if (work_pending(&dev->reset_work))
		return false;
	if (work_busy(&dev->reset_work))
		return false;
We could also skip checking work_pending since work_busy does that for us.

Great! What name should/can I choose for the function?

Thanks,

Guilherme

Keith Busch

2016-04-07 15:46 UTC #9 lore raw

PATCH 1/1 Avoid reset work on watchdog timer function during error recovery

Quoted 1 line · Guilherme G. Piccoli

Great! What name should/can I choose for the function?

I liked Christoph's 'nvme_should_reset' suggestion.

Guilherme G. Piccoli

author

2016-04-07 16:03 UTC #10 lore raw

PATCH 1/1 Avoid reset work on watchdog timer function during error recovery

Quoted 5 lines · Keith Busch

Quoted 1 line · Guilherme G. Piccoli
Great! What name should/can I choose for the function?
I liked Christoph's 'nvme_should_reset' suggestion.

Ok, cool. I'll use this name.

Thanks,

Guilherme

Christoph Hellwig

2016-04-12 19:01 UTC #11 lore raw

PATCH 1/1 Avoid reset work on watchdog timer function during error recovery

Quoted 8 lines · Guilherme G. Piccoli

Quoted 5 lines · Keith Busch
Quoted 1 line · Guilherme G. Piccoli
Great! What name should/can I choose for the function?
I liked Christoph's 'nvme_should_reset' suggestion.
Ok, cool. I'll use this name.

Did you manage to get around resending it?

Guilherme G. Piccoli

author

2016-04-12 23:33 UTC #12 lore raw

PATCH 1/1 Avoid reset work on watchdog timer function during error recovery

Quoted 2 lines · Christoph Hellwig

Did you manage to get around resending it?

Just sent the v2. Sorry for my delay.

Thanks,

Guilherme