PCI/portdrv: do not disable device on reboot/shutdown
Subsystems (5)
PATCH v2 1/1 PCI/portdrv: do not disable device on reboot/shutdown
'Commit cc27b735ad3a ("PCI/portdrv: Turn off PCIe services during shutdown")' has been added to kernel to shutdown pending PCIe port service interrupts during reboot so that a newly started kexec kernel wouldn't observe pending interrupts.
pcie_port_device_remove() is disabling the root port and switches by calling pci_disable_device() after all PCIe service drivers are shutdown.
pci_disable_device() has a much wider impact then port service itself and it prevents all inbound transactions to reach to the system and impacts the entire PCI traffic behind the bridge.
Issue is that pcie_port_device_remove() doesn't maintain any coordination with the rest of the PCI device drivers in the system before clearing the bus master bit.
This has been found to cause crashes on HP DL360 Gen9 machines during reboot. Besides, kexec is already clearing the bus master bit in pci_device_shutdown() after all PCI drivers are removed.
Just remove the extra clear in shutdown path by seperating the remove and shutdown APIs in the PORTDRV.
--- drivers/pci/pcie/portdrv.h | 2 +- drivers/pci/pcie/portdrv_core.c | 5 +++-- drivers/pci/pcie/portdrv_pci.c | 16 +++++++++++++--- 3 files changed, 17 insertions(+), 6 deletions(-)
drivers/pci/pcie/portdrv.h +1 -1
@@ -86,7 +86,7 @@ int pcie_port_device_register(struct pci_dev *dev);int pcie_port_device_suspend (struct device *dev);int pcie_port_device_resume (struct device *dev);#endif -void pcie_port_device_remove (struct pci_dev *dev); +void pcie_port_device_remove (struct pci_dev *dev,bool disable);int __must_check pcie_port_bus_register (void );void pcie_port_bus_unregister (void );
drivers/pci/pcie/portdrv_core.c +3 -2
@@ -405,11 +405,12 @@ static int remove_iter(struct device *dev, void *data)* Remove PCI Express port service devices associated with given port and * disable MSI-X or MSI for the port. */ -void pcie_port_device_remove (struct pci_dev *dev) +void pcie_port_device_remove (struct pci_dev *dev,bool disable) {device_for_each_child (&dev->dev,NULL , remove_iter);pci_free_irq_vectors (dev); -pci_disable_device (dev); +if (disable) +pci_disable_device (dev); }/**
drivers/pci/pcie/portdrv_pci.c +13 -3
@@ -137,7 +137,7 @@ static int pcie_portdrv_probe(struct pci_dev *dev,return 0 ; } -static void pcie_portdrv_remove (struct pci_dev *dev) +static void __pcie_portdrv_remove (struct pci_dev *dev,bool disable) {if (pci_bridge_d3_possible (dev)) {pm_runtime_forbid (&dev->dev);
@@ -145,7 +145,17 @@ static void pcie_portdrv_remove(struct pci_dev *dev)pm_runtime_dont_use_autosuspend (&dev->dev); } -pcie_port_device_remove (dev); +pcie_port_device_remove (dev, disable); +} + +static void pcie_portdrv_remove (struct pci_dev *dev) +{ +return __pcie_portdrv_remove (dev, true); +} + +static void pcie_portdrv_shutdown (struct pci_dev *dev) +{ +return __pcie_portdrv_remove (dev, false); } static pci_ers_result_t pcie_portdrv_error_detected(struct pci_dev *dev,
@@ -218,7 +228,7 @@ static struct pci_driver pcie_portdriver = { .probe = pcie_portdrv_probe, .remove = pcie_portdrv_remove, - .shutdown = pcie_portdrv_remove, + .shutdown = pcie_portdrv_shutdown, .err_handler = &pcie_portdrv_err_handler,
Signature · 1 line
2.7.4
[-cc Gabriele (invalid email address)]
[+cc Don, esc.storagedev, linux-scsi since hpsa is involved]
Background for newcomers:
Ryan reported a panic on shutdown/reboot [1] on DL360 Gen9. I think the problem is that the shutdown path clears PCI_COMMAND_MASTER on the Root Port leading to an hpsa device, the hpsa device attempts a DMA, the Root Port treats the DMA as an Unsupported Request (a fatal error), and that leads to a panic.
This patch avoids the problem by changing the Root Port shutdown path so it doesn't clear PCI_COMMAND_MASTER.
[1] https://bugzilla.kernel.org/show_bug.cgi?id=199779
Quoted 7 lines · Sinan Kaya
'Commit
cc27b735ad3a("PCI/portdrv: Turn off PCIe services during shutdown")' has been added to kernel to shutdown pending PCIe port service interrupts during reboot so that a newly started kexec kernel wouldn't observe pending interrupts.
pcie_port_device_remove()is disabling the root port and switches by callingpci_disable_device()after all PCIe service drivers are shutdown.
It's interesting and annoying that pci_enable_device() and pci_disable_device() sound like they should be inverses, or at least related, but they really aren't.
pci_enable_device() basically turns on PCI_COMMAND_MEMORY and/or PCI_COMMAND_IO so the device will respond to programmed I/O. But pci_disable_device() leaves programmed I/O enabled and turns off PCI_COMMAND_MASTER, which keeps the device from performing DMA or forwarding transactions upstream (in the case of bridges).
Not an issue for this patch, just an observation :)
Quoted 7 lines
pci_disable_device()has a much wider impact then port service itself and it prevents all inbound transactions to reach to the system and impacts the entire PCI traffic behind the bridge.Issue is that
pcie_port_device_remove()doesn't maintain any coordination with the rest of the PCI device drivers in the system before clearing the bus master bit.
I am interested in this part because if there really isn't any coordination, that's a good argument for getting rid of portdrv, which I want to do anyway. But I haven't dug into this enough to verify it.
The portdrv does register as a regular PCI driver (pcie_portdriver), and I'm pretty sure the driver core is smart enough to call the driver entry points in a postorder traversal (children before parents), which means the hpsa .shutdown() should be called before the portdrv .shutdown().
The crash seems to indicate that the hpsa device attempted a DMA after we cleared the Root Port's PCI_COMMAND_MASTER, which means hpsa_shutdown() didn't stop DMA from the device (it looks like *most* shutdown methods don't disable device DMA, so it's in good company).
Quoted 3 lines
This has been found to cause crashes on HP DL360 Gen9 machines during reboot. Besides, kexec is already clearing the bus master bit in
pci_device_shutdown()after all PCI drivers are removed.
The original path was:
pci_device_shutdown(hpsa)
drv->shutdown
hpsa_shutdown # hpsa_pci_driver.shutdown
...
pci_device_shutdown(RP) # root port
drv->shutdown
pcie_portdrv_remove # pcie_portdriver.shutdown
pcie_port_device_remove
pci_disable_device
do_pci_disable_device
# clear RP PCI_COMMAND_MASTER
if (kexec)
pci_clear_master(RP)
# clear RP PCI_COMMAND_MASTER
If I understand correctly, the new path after this patch is:
pci_device_shutdown(hpsa)
drv->shutdown
hpsa_shutdown # hpsa_pci_driver.shutdown
...
pci_device_shutdown(RP) # root port
drv->shutdown
pcie_portdrv_shutdown # pcie_portdriver.shutdown
__pcie_portdrv_remove(RP, false)
pcie_port_device_remove(RP, false)
# do NOT clear RP PCI_COMMAND_MASTER
if (kexec)
pci_clear_master(RP)
# clear RP PCI_COMMAND_MASTER
I guess this patch avoids the panic during reboot because we're not in the kexec path, so we never clear PCI_COMMAND_MASTER for the Root Port, so the hpsa device can DMA happily until the lights go out.
But DMA continuing for some random amount of time before the reboot or shutdown happens makes me a little queasy. That doesn't sound safe. The more I think about this, the more confused I get. What am I missing?
Quoted 79 lines
Just remove the extra clear in shutdown path by seperating the remove and shutdown APIs in the PORTDRV.
Signed-off-by Sinan Kaya <[email protected]>Fixes cc27b735ad3a ("PCI/portdrv: Turn off PCIe services during shutdown")Cc stable at vger.kernel.orgReported-by Ryan Finnie <[email protected]>--- drivers/pci/pcie/portdrv.h | 2 +- drivers/pci/pcie/portdrv_core.c | 5 +++-- drivers/pci/pcie/portdrv_pci.c | 16 +++++++++++++--- 3 files changed, 17 insertions(+), 6 deletions(-)drivers/pci/pcie/portdrv.h +1
-1@@ -86,7 +86,7 @@ int pcie_port_device_register(struct pci_dev *dev);int pcie_port_device_suspend (struct device *dev);int pcie_port_device_resume (struct device *dev);#endif -void pcie_port_device_remove (struct pci_dev *dev); +void pcie_port_device_remove (struct pci_dev *dev,bool disable);int __must_check pcie_port_bus_register (void );void pcie_port_bus_unregister (void );drivers/pci/pcie/portdrv_core.c +3
-2@@ -405,11 +405,12 @@ static int remove_iter(struct device *dev, void *data)* Remove PCI Express port service devices associated with given port and * disable MSI-X or MSI for the port. */ -void pcie_port_device_remove (struct pci_dev *dev) +void pcie_port_device_remove (struct pci_dev *dev,bool disable) {device_for_each_child (&dev->dev,NULL , remove_iter);pci_free_irq_vectors (dev); -pci_disable_device (dev); +if (disable) +pci_disable_device (dev); }/** drivers/pci/pcie/portdrv_pci.c +12
-2@@ -137,7 +137,7 @@ static int pcie_portdrv_probe(struct pci_dev *dev,return 0 ; } -static void pcie_portdrv_remove (struct pci_dev *dev) +static void __pcie_portdrv_remove (struct pci_dev *dev,bool disable) {if (pci_bridge_d3_possible (dev)) {pm_runtime_forbid (&dev->dev);@@ -145,7 +145,17 @@ static void pcie_portdrv_remove(struct pci_dev *dev)pm_runtime_dont_use_autosuspend (&dev->dev); } -pcie_port_device_remove (dev); +pcie_port_device_remove (dev, disable); +} + +static void pcie_portdrv_remove (struct pci_dev *dev) +{ +return __pcie_portdrv_remove (dev, true); +} + +static void pcie_portdrv_shutdown (struct pci_dev *dev) +{ +return __pcie_portdrv_remove (dev, false); } static pci_ers_result_t pcie_portdrv_error_detected(struct pci_dev *dev,
drivers/pci/pcie/portdrv_pci.c static struct pci_driver pcie_portdriver = { · line 231
Quoted 5 lines
drivers/pci/pcie/portdrv_pci.c +1
-1@@ -218,7 +228,7 @@ static struct pci_driver pcie_portdriver = { .probe = pcie_portdrv_probe, .remove = pcie_portdrv_remove, - .shutdown = pcie_portdrv_remove, + .shutdown = pcie_portdrv_shutdown,
What are the circumstances when we call .remove() vs .shutdown()?
I guess the main (maybe only) way to call .remove() is to hot-remove the port? And .shutdown() is basically used in the reboot and kexec paths?
Quoted 5 lines
drivers/pci/pcie/portdrv_pci.c +0
-0.err_handler = &pcie_portdrv_err_handler,Signature · 1 line
2.7.4
Quoted 5 lines · Bjorn Helgaas
The crash seems to indicate that the hpsa device attempted a DMA after we cleared the Root Port's
PCI_COMMAND_MASTER, which meanshpsa_shutdown()didn't stop DMA from the device (it looks like *most* shutdown methods don't disable device DMA, so it's in good company).
All drivers are expected to shutdown DMA and interrupts in their shutdown() routines. They can skip removing threads, data structures etc. but DMA and interrupt disabling are required. This is the difference between shutdown() and remove() callbacks.
If you see that this is not being done in HPSA, then that is where the bugfix should be.
Counter argument is that if shutdown() is not implemented, at least remove() should be called. Expecting all drivers to implement shutdown() callbacks is just bad by design in my opinion.
Code should have fallen back to remove() if shutdown() doesn't exist. I can propose a patch for this but this is yet another story to chase.
Quoted 24 lines
Quoted 4 lines
This has been found to cause crashes on HP DL360 Gen9 machines during reboot. Besides, kexec is already clearing the bus master bit in
pci_device_shutdown()after all PCI drivers are removed.The original path was:
pci_device_shutdown(hpsa)
drv->shutdown
hpsa_shutdown# hpsa_pci_driver.shutdown
...
pci_device_shutdown(RP)# root port
drv->shutdown
pcie_portdrv_remove# pcie_portdriver.shutdown
pcie_port_device_remove
pci_disable_device
do_pci_disable_device
# clear RPPCI_COMMAND_MASTER
if (kexec)
pci_clear_master(RP)
# clear RPPCI_COMMAND_MASTERIf I understand correctly, the new path after this patch is:
Quoted 10 lines
pci_device_shutdown(hpsa)
drv->shutdown
hpsa_shutdown# hpsa_pci_driver.shutdown
...
pci_device_shutdown(RP)# root port
drv->shutdown
pcie_portdrv_shutdown# pcie_portdriver.shutdown
__pcie_portdrv_remove(RP, false)
pcie_port_device_remove(RP, false)
# do NOT clear RPPCI_COMMAND_MASTER
yup
Quoted 4 lines
if (kexec)
pci_clear_master(RP)
# clear RPPCI_COMMAND_MASTER
Quoted 8 lines
I guess this patch avoids the panic during reboot because we're not in the kexec path, so we never clear
PCI_COMMAND_MASTERfor the Root Port, so the hpsa device can DMA happily until the lights go out.But DMA continuing for some random amount of time before the reboot or shutdown happens makes me a little queasy. That doesn't sound safe. The more I think about this, the more confused I get. What am I missing?
see above.
Quoted 14 lines
drivers/pci/pcie/portdrv_pci.c static struct pci_driver pcie_portdriver = { · line 231
Quoted 11 lines
Just remove the extra clear in shutdown path by seperating the remove and shutdown APIs in the PORTDRV.
static
pci_ers_result_tpcie_portdrv_error_detected(structpci_dev*dev,drivers/pci/pcie/portdrv_pci.c +1
-1@@ -218,7 +228,7 @@ static struct pci_driver pcie_portdriver = { .probe = pcie_portdrv_probe, .remove = pcie_portdrv_remove, - .shutdown = pcie_portdrv_remove, + .shutdown = pcie_portdrv_shutdown,What are the circumstances when we call .remove() vs .shutdown()?
Quoted 3 lines
I guess the main (maybe only) way to call .remove() is to hot-remove the port? And .shutdown() is basically used in the reboot and kexec paths?
Correct. shutdown() is only called during reboot/shutdown calls. If you echo 1 into the remove file, remove() gets called. Handy for hotplug use cases. It needs to be the exact opposite of the probe. It needs to clean up resources etc. and have the HW in a state where it can be reinitialized via probe again.
Quoted 7 lines
Quoted 6 lines
drivers/pci/pcie/portdrv_pci.c +0
-0.err_handler = &pcie_portdrv_err_handler,Signature · 1 line
2.7.4
Signature · 3 lines
Sinan Kaya Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Quoted 8 lines · Sinan Kaya
Quoted 4 lines
The crash seems to indicate that the hpsa device attempted a DMA after we cleared the Root Port's
PCI_COMMAND_MASTER, which meanshpsa_shutdown()didn't stop DMA from the device (it looks like *most* shutdown methods don't disable device DMA, so it's in good company).All drivers are expected to shutdown DMA and interrupts in their
shutdown()routines. They can skip removing threads, data structures etc. but DMA and interrupt disabling are required. This is the difference betweenshutdown()andremove()callbacks.
I found this note yesterday to see why we are not disabling the devices in the PCI core itself.
pci_device_remove()
/*
* We would love to complain here if pci_dev->is_enabled is set, that
* the driver should have called pci_disable_device(), but the
* unfortunate fact is there are too many odd BIOS and bridge setups
* that don't like drivers doing that all of the time.
* Oh well, we can dream of sane hardware when we sleep, no matter how
* horrible the crap we have to deal with is when we are awake... */
Ryan, can you discard the previous patch and test this one instead? remove() path in hpsa driver seems to call pci_disable_device() via
hpsa_remove_one()
hpsa_free_pci_init()
but nothing on the shutdown path.
drivers/scsi/hpsa.c +1 -0
@@ -8651,6 +8651,7 @@ static void hpsa_shutdown(struct pci_dev *pdev) h->access.set_intr_mask (h,HPSA_INTR_OFF );hpsa_free_irqs (h);/* init_one 4 */ hpsa_disable_interrupt_mode (h);/* pci_init 2 */ +pci_disable_device (h->pdev); }
Signature · 3 lines
Sinan Kaya Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Quoted 37 lines · Sinan Kaya
Quoted 8 lines · Sinan Kaya
Quoted 4 lines
The crash seems to indicate that the hpsa device attempted a DMA after we cleared the Root Port's
PCI_COMMAND_MASTER, which meanshpsa_shutdown()didn't stop DMA from the device (it looks like *most* shutdown methods don't disable device DMA, so it's in good company).All drivers are expected to shutdown DMA and interrupts in their
shutdown()routines. They can skip removing threads, data structures etc. but DMA and interrupt disabling are required. This is the difference betweenshutdown()andremove()callbacks.I found this note yesterday to see why we are not disabling the devices in the PCI core itself.
pci_device_remove()/*
* We would love to complain here ifpci_dev->is_enabledis set, that
* the driver should have calledpci_disable_device(), but the
* unfortunate fact is there are too many odd BIOS and bridge setups
* that don't like drivers doing that all of the time.
* Oh well, we can dream of sane hardware when we sleep, no matter how
* horrible the crap we have to deal with is when we are awake... */Ryan, can you discard the previous patch and test this one instead?
remove()path in hpsa driver seems to callpci_disable_device()via
hpsa_remove_one()
hpsa_free_pci_init()but nothing on the shutdown path.
drivers/scsi/hpsa.c +0
-0
drivers/scsi/hpsa.c static void hpsa_shutdown(struct pci_dev *pdev) · line 8655
Quoted 5 lines
drivers/scsi/hpsa.c +1
-0@@ -8651,6 +8651,7 @@ static void hpsa_shutdown(struct pci_dev *pdev) h->access.set_intr_mask (h,HPSA_INTR_OFF );hpsa_free_irqs (h);/* init_one 4 */ hpsa_disable_interrupt_mode (h);/* pci_init 2 */ +pci_disable_device (h->pdev); }
I suspect this will make things "work" (if the device can't attempt DMA, no Unsupported Request error will occur).
But I'm concerned that the reason for the DMA might that hpsa is transferring buffers from system memory to the hpsa device, and if we arbitrarily terminate those transfers with pci_disable_device(), we may leave the hpsa device in an inconsistent state, e.g., with a dirty filesystem.
But we really need guidance from an hpsa expert. I don't know the filesystem/SCSI/hpsa details.
Bjorn
Quoted 48 lines · Bjorn Helgaas
Quoted 38 lines · Sinan Kaya
Quoted 8 lines · Sinan Kaya
Quoted 4 lines
The crash seems to indicate that the hpsa device attempted a DMA after we cleared the Root Port's
PCI_COMMAND_MASTER, which meanshpsa_shutdown()didn't stop DMA from the device (it looks like *most* shutdown methods don't disable device DMA, so it's in good company).All drivers are expected to shutdown DMA and interrupts in their
shutdown()routines. They can skip removing threads, data structures etc. but DMA and interrupt disabling are required. This is the difference betweenshutdown()andremove()callbacks.I found this note yesterday to see why we are not disabling the devices in the PCI core itself.
pci_device_remove()/*
* We would love to complain here ifpci_dev->is_enabledis set, that
* the driver should have calledpci_disable_device(), but the
* unfortunate fact is there are too many odd BIOS and bridge setups
* that don't like drivers doing that all of the time.
* Oh well, we can dream of sane hardware when we sleep, no matter
how* horrible the crap we have to deal with is when we are awake... */Ryan, can you discard the previous patch and test this one instead?
remove()path in hpsa driver seems to callpci_disable_device()via
hpsa_remove_one()
hpsa_free_pci_init()but nothing on the shutdown path.
drivers/scsi/hpsa.c +0
-0drivers/scsi/hpsa.c static void hpsa_shutdown(struct pci_dev *pdev) · line 8655
Quoted 5 lines
drivers/scsi/hpsa.c +1
-0@@ -8651,6 +8651,7 @@ static void hpsa_shutdown(struct pci_dev *pdev) h->access.set_intr_mask (h,HPSA_INTR_OFF );hpsa_free_irqs (h);/* init_one 4 */ hpsa_disable_interrupt_mode (h);/* pci_init 2 */ +pci_disable_device (h->pdev); }I suspect this will make things "work" (if the device can't attempt DMA, no Unsupported Request error will occur).
Quoted 8 lines
But I'm concerned that the reason for the DMA might that hpsa is transferring buffers from system memory to the hpsa device, and if we arbitrarily terminate those transfers with
pci_disable_device(), we may leave the hpsa device in an inconsistent state, e.g., with a dirty filesystem.But we really need guidance from an hpsa expert. I don't know the filesystem/SCSI/hpsa details.
Agreed,
We can drop shutdown and use the remove callback. Remove is supposed to do a safe cleanup.
Quoted 2 lines
Bjorn
PATCH v2 1/1 PCI/portdrv: do not disable device on reboot/shutdown
In reply to Bjorn HelgaasQuoted 72 lines
-----Original Message-----
From: Bjorn Helgaas [mailto:helgaas at kernel.org]
Sent: Thursday, May 24, 2018 8:07 AM
To: Sinan Kaya <[email protected]>
Cc: linux-pci at vger.kernel.org; timur at codeaurora.org; ryan at finnie.org; linux- arm-msm at vger.kernel.org; linux-arm-kernel at lists.infradead.org; stable at vger.kernel.org; Bjorn Helgaas <[email protected]>; Rafael J. Wysocki <[email protected]>; Greg Kroah-Hartman <[email protected]>; Thomas Gleixner <[email protected]>; Kate Stewart <[email protected]>; Frederick Lawler <[email protected]>; Dongdong Liu <[email protected]>; Mika Westerberg <[email protected]>; open list <linux-
kernel at vger.kernel.org>; Don Brace <[email protected]>; esc.storagedev <[email protected]>; linux-scsi at vger.kernel.org
Subject: Re: [PATCH V2] PCI/portdrv: do not disable device on reboot/shutdownEXTERNAL EMAIL
Quoted 37 lines · Sinan Kaya
Quoted 8 lines · Sinan Kaya
Quoted 4 lines
The crash seems to indicate that the hpsa device attempted a DMA after we cleared the Root Port's
PCI_COMMAND_MASTER, which meanshpsa_shutdown()didn't stop DMA from the device (it looks like *most* shutdown methods don't disable device DMA, so it's in good company).All drivers are expected to shutdown DMA and interrupts in their
shutdown()routines. They can skip removing threads, data structures etc. but DMA and interrupt disabling are required. This is the difference betweenshutdown()andremove()callbacks.I found this note yesterday to see why we are not disabling the devices in the PCI core itself.
pci_device_remove()/*
* We would love to complain here ifpci_dev->is_enabledis set, that
* the driver should have calledpci_disable_device(), but the
* unfortunate fact is there are too many odd BIOS and bridge setups
* that don't like drivers doing that all of the time.
* Oh well, we can dream of sane hardware when we sleep, no matter how
* horrible the crap we have to deal with is when we are awake... */Ryan, can you discard the previous patch and test this one instead?
remove()path in hpsa driver seems to callpci_disable_device()via
hpsa_remove_one()
hpsa_free_pci_init()but nothing on the shutdown path.
drivers/scsi/hpsa.c +0
-0drivers/scsi/hpsa.c static void hpsa_shutdown(struct pci_dev *pdev) · line 8655
Quoted 5 lines
drivers/scsi/hpsa.c +1
-0@@ -8651,6 +8651,7 @@ static void hpsa_shutdown(struct pci_dev *pdev) h->access.set_intr_mask (h,HPSA_INTR_OFF );hpsa_free_irqs (h);/* init_one 4 */ hpsa_disable_interrupt_mode (h);/* pci_init 2 */ +pci_disable_device (h->pdev); }I suspect this will make things "work" (if the device can't attempt DMA, no Unsupported Request error will occur).
But I'm concerned that the reason for the DMA might that hpsa is transferring buffers from system memory to the hpsa device, and if we arbitrarily terminate those transfers with
pci_disable_device(), we may leave the hpsa device in an inconsistent state, e.g., with a dirty filesystem.
Quoted 4 lines
But we really need guidance from an hpsa expert. I don't know the filesystem/SCSI/hpsa details.
Bjorn
It's most likely OCSD traffic that will stop when bus mastering is turned off. So, I'll run some tests on my end before ACKing your patch.
Thanks,
Don Brace
ESC - Smart Storage
Microsemi Corporation
Quoted 6 lines · Don Brace
Quoted 4 lines
But we really need guidance from an hpsa expert. I don't know the filesystem/SCSI/hpsa details.
Bjorn
It's most likely OCSD traffic that will stop when bus mastering is turned off. So, I'll run some tests on my end before ACKing your patch.
Can you test V3 instead of this?
I don't think adding pci_disable_device() to shutdown() is enough to put the HW into safe state. I moved clean up responsibility to remove() instead of shutdown().
Signature · 3 lines
Sinan Kaya Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Quoted 15 lines · Sinan Kaya
Quoted 5 lines · Bjorn Helgaas
The crash seems to indicate that the hpsa device attempted a DMA after we cleared the Root Port's
PCI_COMMAND_MASTER, which meanshpsa_shutdown()didn't stop DMA from the device (it looks like *most* shutdown methods don't disable device DMA, so it's in good company).All drivers are expected to shutdown DMA and interrupts in their
shutdown()routines. They can skip removing threads, data structures etc. but DMA and interrupt disabling are required. This is the difference betweenshutdown()andremove()callbacks.If you see that this is not being done in HPSA, then that is where the bugfix should be.
Quoted 6 lines
Counter argument is that if
shutdown()is not implemented, at leastremove()should be called. Expecting all drivers to implementshutdown()callbacks is just bad by design in my opinion.Code should have fallen back to
remove()ifshutdown()doesn't exist. I can propose a patch for this but this is yet another story to chase.
That sounds like a reasonable idea, and it is definitely another can of worms. I looked briefly at some of the .shutdown() cases:
- device_shutdown() doesn't fall back to remove().
- It looks like most bus_types don't implement .shutdown() at all (I didn't look at them all).
- Of the bus_types that do implement .shutdown(), most do not fall back to .remove(). ps3_system_bus_type() is an example of one that *does* fall back to a driver's .remove() if there is no .shutdown().
Implement shutdown (no fallback unless indicated):
ecard_bus_type
gio_bus_type
ps3_system_bus_type # does fallback to remove
ibmebus_bus_type
isa_bus_type
platform_bus_type # not direct implementation
fmc_bus_type # fmc_shutdown() looks spurious
mipi_dsi_bus_type
hv_bus
Quoted 87 lines
Quoted 23 lines
Quoted 3 lines
This has been found to cause crashes on HP DL360 Gen9 machines during reboot. Besides, kexec is already clearing the bus master bit in
pci_device_shutdown()after all PCI drivers are removed.The original path was:
pci_device_shutdown(hpsa)
drv->shutdown
hpsa_shutdown# hpsa_pci_driver.shutdown
...
pci_device_shutdown(RP)# root port
drv->shutdown
pcie_portdrv_remove# pcie_portdriver.shutdown
pcie_port_device_remove
pci_disable_device
do_pci_disable_device
# clear RPPCI_COMMAND_MASTER
if (kexec)
pci_clear_master(RP)
# clear RPPCI_COMMAND_MASTERIf I understand correctly, the new path after this patch is:
Quoted 10 lines
pci_device_shutdown(hpsa)
drv->shutdown
hpsa_shutdown# hpsa_pci_driver.shutdown
...
pci_device_shutdown(RP)# root port
drv->shutdown
pcie_portdrv_shutdown# pcie_portdriver.shutdown
__pcie_portdrv_remove(RP, false)
pcie_port_device_remove(RP, false)
# do NOT clear RPPCI_COMMAND_MASTERyup
Quoted 4 lines
if (kexec)
pci_clear_master(RP)
# clear RPPCI_COMMAND_MASTERQuoted 8 lines
I guess this patch avoids the panic during reboot because we're not in the kexec path, so we never clear
PCI_COMMAND_MASTERfor the Root Port, so the hpsa device can DMA happily until the lights go out.But DMA continuing for some random amount of time before the reboot or shutdown happens makes me a little queasy. That doesn't sound safe. The more I think about this, the more confused I get. What am I missing?
see above.
Quoted 14 lines
drivers/pci/pcie/portdrv_pci.c static struct pci_driver pcie_portdriver = { · line 231
Quoted 11 lines
Just remove the extra clear in shutdown path by seperating the remove and shutdown APIs in the PORTDRV.
static
pci_ers_result_tpcie_portdrv_error_detected(structpci_dev*dev,drivers/pci/pcie/portdrv_pci.c +1
-1@@ -218,7 +228,7 @@ static struct pci_driver pcie_portdriver = { .probe = pcie_portdrv_probe, .remove = pcie_portdrv_remove, - .shutdown = pcie_portdrv_remove, + .shutdown = pcie_portdrv_shutdown,What are the circumstances when we call .remove() vs .shutdown()?
Quoted 3 lines
I guess the main (maybe only) way to call .remove() is to hot-remove the port? And .shutdown() is basically used in the reboot and kexec paths?
Correct.
shutdown()is only called during reboot/shutdown calls. If you echo 1 into the remove file,remove()gets called. Handy for hotplug use cases. It needs to be the exact opposite of the probe. It needs to clean up resources etc. and have the HW in a state where it can be reinitialized via probe again.Quoted 7 lines
Quoted 6 lines
drivers/pci/pcie/portdrv_pci.c +0
-0.err_handler = &pcie_portdrv_err_handler,Signature · 1 line
2.7.4Signature · 3 lines
Sinan Kaya Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Quoted 2 lines · Bjorn Helgaas
That sounds like a reasonable idea, and it is definitely another can of worms. I looked briefly at some of the .shutdown() cases:
should we throw it into 4.18 and see what happens?
Signature · 3 lines
Sinan Kaya Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Quoted 5 lines · Sinan Kaya
Quoted 2 lines · Bjorn Helgaas
That sounds like a reasonable idea, and it is definitely another can of worms. I looked briefly at some of the .shutdown() cases:
should we throw it into 4.18 and see what happens?
It wouldn't solve this particular problem because hpsa *does* have a .shutdown() method. The problem is that it doesn't work -- it's supposed to stop DMA and interrupts but it apparently doesn't.
I think we need to fix hpsa first.
Quoted 5 lines · Bjorn Helgaas
Quoted 4 lines · Sinan Kaya
Quoted 2 lines · Bjorn Helgaas
That sounds like a reasonable idea, and it is definitely another can of worms. I looked briefly at some of the .shutdown() cases:
Quoted 7 lines
Quoted 1 line
should we throw it into 4.18 and see what happens?
It wouldn't solve this particular problem because hpsa *does* have a .shutdown() method. The problem is that it doesn't work -- it's supposed to stop DMA and interrupts but it apparently doesn't.
I think we need to fix hpsa first.
Absolutely, the othet patch is a parallel issue.
Bjorn,
Quoted 5 lines · Bjorn Helgaas
Quoted 4 lines · Sinan Kaya
Quoted 2 lines · Bjorn Helgaas
That sounds like a reasonable idea, and it is definitely another can of worms. I looked briefly at some of the .shutdown() cases:
Quoted 8 lines
Quoted 1 line
should we throw it into 4.18 and see what happens?
It wouldn't solve this particular problem because hpsa *does* have a .shutdown() method. The problem is that it doesn't work -- it's supposed to stop DMA and interrupts but it apparently doesn't.
I think we need to fix hpsa first.
I don't know if you followed my latest V3 patch but I found a way to put these two together to reach to an, IMO, more acceptable solution.
Sinan
Signature · 3 lines
Sinan Kaya Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.