nvme/pci: Log PCI_STATUS when the controller dies
PATCH v2 1/1 nvme/pci: Log PCI_STATUS when the controller dies
When debugging nvme controller crashes, it's nice to know whether the controller died cleanly so that the failure is just reflected in CSTS, it died and put an error in PCI_STATUS, or whether it died so badly that it stopped responding to PCI configuration space reads.
--- drivers/nvme/host/pci.c | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-)
drivers/nvme/host/pci.c +16 -4
@@ -1292,10 +1292,22 @@ static void nvme_watchdog_timer(unsigned long data)/* Skip controllers under certain specific conditions. */ if (nvme_should_reset (dev, csts)) { -if (!nvme_reset (dev)) -dev_warn (dev->dev, -"Failed status: 0x%x, reset controller.\n" , - csts); +if (!nvme_reset (dev)) { +/* Read a config register to help see what died. */ +u16 pci_status; +int result; + + result =pci_read_config_word (to_pci_dev (dev->dev), +PCI_STATUS , &pci_status); +if (result ==PCIBIOS_SUCCESSFUL ) +dev_warn (dev->dev, +"controller is down; will reset: CSTS=0x%x, PCI_STATUS=0x%hx\n" , + csts, pci_status); +else +dev_warn (dev->dev, +"controller is down; will reset: CSTS=0x%x, PCI_STATUS read failed (%d)\n" , + csts, result); + }return ; }
Signature · 1 line
2.9.3
PATCH v2 1/1 nvme/pci: Log PCI_STATUS when the controller dies
Quoted 4 lines · Andy Lutomirski
When debugging nvme controller crashes, it's nice to know whether the controller died cleanly so that the failure is just reflected in CSTS, it died and put an error in
PCI_STATUS, or whether it died so badly that it stopped responding to PCI configuration space reads.
Just curious: what controller did this happen with?
Quoted 5 lines
drivers/nvme/host/pci.c +5
-0+/* Read a config register to help see what died. */ +u16 pci_status; +int result; + + result = pci_read_config_word(to_pci_dev(dev->dev),
drivers/nvme/host/pci.c static void nvme_watchdog_timer(unsigned long data) · line 1310
Quoted 10 lines
drivers/nvme/host/pci.c +10
-0+ PCI_STATUS, &pci_status); +if (result ==PCIBIOS_SUCCESSFUL ) +dev_warn (dev->dev, +"controller is down; will reset: CSTS=0x%x, PCI_STATUS=0x%hx\n" , + csts, pci_status); +else +dev_warn (dev->dev, +"controller is down; will reset: CSTS=0x%x, PCI_STATUS read failed (%d)\n" , + csts, result); + }
Can you factor all this debug code into a separate function to keep the main flow easier to read?
Except for that this patch looks fine to me:
PATCH v2 1/1 nvme/pci: Log PCI_STATUS when the controller dies
Quoted 7 lines · Christoph Hellwig
Quoted 4 lines · Andy Lutomirski
When debugging nvme controller crashes, it's nice to know whether the controller died cleanly so that the failure is just reflected in CSTS, it died and put an error in
PCI_STATUS, or whether it died so badly that it stopped responding to PCI configuration space reads.Just curious: what controller did this happen with?
I've seen a failure that gives 0xffff in PCI_STATUS on a Samsung "SM951 NVMe SAMSUNG 256GB" with firmware "BXW75D0Q".
I'll add that to the v3 changelog.
Quoted 17 lines
Quoted 6 lines
drivers/nvme/host/pci.c +5
-0+/* Read a config register to help see what died. */ +u16 pci_status; +int result; + + result = pci_read_config_word(to_pci_dev(dev->dev),drivers/nvme/host/pci.c static void nvme_watchdog_timer(unsigned long data) · line 1310
Quoted 10 lines
drivers/nvme/host/pci.c +10
-0+ PCI_STATUS, &pci_status); +if (result ==PCIBIOS_SUCCESSFUL ) +dev_warn (dev->dev, +"controller is down; will reset: CSTS=0x%x, PCI_STATUS=0x%hx\n" , + csts, pci_status); +else +dev_warn (dev->dev, +"controller is down; will reset: CSTS=0x%x, PCI_STATUS read failed (%d)\n" , + csts, result); + }
Quoted 6 lines
Can you factor all this debug code into a separate function to keep the main flow easier to read?
Except for that this patch looks fine to me:
Reviewed-by Christoph Hellwig <hch at lst.de>
Done. v3 coming.
Signature · 2 lines
Andy Lutomirski AMA Capital Management, LLC
PATCH v2 1/1 nvme/pci: Log PCI_STATUS when the controller dies
In reply to Andy LutomirskiQuoted 45 lines · Andy Lutomirski
When debugging nvme controller crashes, it's nice to know whether the controller died cleanly so that the failure is just reflected in CSTS, it died and put an error in
PCI_STATUS, or whether it died so badly that it stopped responding to PCI configuration space reads.Signed-off-by Andy Lutomirski <luto at kernel.org>---
?drivers/nvme/host/pci.c | 20 ++++++++++++++++---- ?1 file changed, 16 insertions(+), 4 deletions(-)drivers/nvme/host/pci.c +0
-0@@ -1292,10 +1292,22 @@ static void nvme_watchdog_timer(unsigned long data)?
? /* Skip controllers under certain specific conditions. */
? if (nvme_should_reset(dev, csts)) {drivers/nvme/host/pci.c +0
-3-if (!nvme_reset (dev)) - dev_warn(dev->dev, - "Failed status: 0x%x, resetcontroller.\n",
drivers/nvme/host/pci.c +2
-1- csts); +if (!nvme_reset (dev)) { +/* Read a config register to help see what died. */
drivers/nvme/host/pci.c +4
-0+u16 pci_status; +int result; + + result =pci_read_config_word(to_pci_dev(dev->dev),
drivers/nvme/host/pci.c +1
-0+ ??????PCI_STATUS,
&pci_status);drivers/nvme/host/pci.c +3
-0+if (result ==PCIBIOS_SUCCESSFUL ) + dev_warn(dev->dev, + ?"controller is down; willreset: CSTS=0x%x, PCI_STATUS=0x%hx\n",
drivers/nvme/host/pci.c +4
-0+ ?csts, pci_status); + else + dev_warn(dev->dev, + ?"controller is down; willreset: CSTS=0x%x,
PCI_STATUSread failed (%d)\n",drivers/nvme/host/pci.c +2
-0+ ?csts, result); + }
Quoted 1 line
? return;
Looks a lot more like what Keith suggested.