* [RFC PATCH] PCI/portdrv: No need to call pci_disable_device() during shutdown
@ 2020-09-05 8:33 Tiezhu Yang
2020-09-10 19:41 ` Bjorn Helgaas
0 siblings, 1 reply; 6+ messages in thread
From: Tiezhu Yang @ 2020-09-05 8:33 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: linux-pci, linux-kernel, Rafael J. Wysocki, Xuefeng Li
After commit 745be2e700cd ("PCIe: portdrv: call pci_disable_device
during remove") and commit cc27b735ad3a ("PCI/portdrv: Turn off PCIe
services during shutdown"), it also calls pci_disable_device() during
shutdown, this seems unnecessary, so just remove it.
Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
---
drivers/pci/pcie/portdrv_core.c | 1 -
drivers/pci/pcie/portdrv_pci.c | 14 +++++++++++++-
2 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
index 50a9522..1991aca 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -491,7 +491,6 @@ void pcie_port_device_remove(struct pci_dev *dev)
{
device_for_each_child(&dev->dev, NULL, remove_iter);
pci_free_irq_vectors(dev);
- pci_disable_device(dev);
}
/**
diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
index 3a3ce40..cab37a8 100644
--- a/drivers/pci/pcie/portdrv_pci.c
+++ b/drivers/pci/pcie/portdrv_pci.c
@@ -143,6 +143,18 @@ static void pcie_portdrv_remove(struct pci_dev *dev)
}
pcie_port_device_remove(dev);
+ pci_disable_device(dev);
+}
+
+static void pcie_portdrv_shutdown(struct pci_dev *dev)
+{
+ if (pci_bridge_d3_possible(dev)) {
+ pm_runtime_forbid(&dev->dev);
+ pm_runtime_get_noresume(&dev->dev);
+ pm_runtime_dont_use_autosuspend(&dev->dev);
+ }
+
+ pcie_port_device_remove(dev);
}
static pci_ers_result_t pcie_portdrv_error_detected(struct pci_dev *dev,
@@ -211,7 +223,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,
--
2.1.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [RFC PATCH] PCI/portdrv: No need to call pci_disable_device() during shutdown 2020-09-05 8:33 [RFC PATCH] PCI/portdrv: No need to call pci_disable_device() during shutdown Tiezhu Yang @ 2020-09-10 19:41 ` Bjorn Helgaas 2020-09-10 20:21 ` Bjorn Helgaas 0 siblings, 1 reply; 6+ messages in thread From: Bjorn Helgaas @ 2020-09-10 19:41 UTC (permalink / raw) To: Tiezhu Yang Cc: Bjorn Helgaas, linux-pci, linux-kernel, Rafael J. Wysocki, Xuefeng Li On Sat, Sep 05, 2020 at 04:33:26PM +0800, Tiezhu Yang wrote: > After commit 745be2e700cd ("PCIe: portdrv: call pci_disable_device > during remove") and commit cc27b735ad3a ("PCI/portdrv: Turn off PCIe > services during shutdown"), it also calls pci_disable_device() during > shutdown, this seems unnecessary, so just remove it. I would like to get rid of the portdrv completely by folding its functionality into the PCI core itself, so there would be no portdrv probe or remove. Does this solve a problem? If not, I'm inclined to just leave it as-is for now. But if it fixes something, we should do the fix, of course. > Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn> > --- > drivers/pci/pcie/portdrv_core.c | 1 - > drivers/pci/pcie/portdrv_pci.c | 14 +++++++++++++- > 2 files changed, 13 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c > index 50a9522..1991aca 100644 > --- a/drivers/pci/pcie/portdrv_core.c > +++ b/drivers/pci/pcie/portdrv_core.c > @@ -491,7 +491,6 @@ void pcie_port_device_remove(struct pci_dev *dev) > { > device_for_each_child(&dev->dev, NULL, remove_iter); > pci_free_irq_vectors(dev); > - pci_disable_device(dev); > } > > /** > diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c > index 3a3ce40..cab37a8 100644 > --- a/drivers/pci/pcie/portdrv_pci.c > +++ b/drivers/pci/pcie/portdrv_pci.c > @@ -143,6 +143,18 @@ static void pcie_portdrv_remove(struct pci_dev *dev) > } > > pcie_port_device_remove(dev); > + pci_disable_device(dev); > +} > + > +static void pcie_portdrv_shutdown(struct pci_dev *dev) > +{ > + if (pci_bridge_d3_possible(dev)) { > + pm_runtime_forbid(&dev->dev); > + pm_runtime_get_noresume(&dev->dev); > + pm_runtime_dont_use_autosuspend(&dev->dev); > + } > + > + pcie_port_device_remove(dev); > } > > static pci_ers_result_t pcie_portdrv_error_detected(struct pci_dev *dev, > @@ -211,7 +223,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, > > -- > 2.1.0 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] PCI/portdrv: No need to call pci_disable_device() during shutdown 2020-09-10 19:41 ` Bjorn Helgaas @ 2020-09-10 20:21 ` Bjorn Helgaas 2020-09-11 1:54 ` Tiezhu Yang 0 siblings, 1 reply; 6+ messages in thread From: Bjorn Helgaas @ 2020-09-10 20:21 UTC (permalink / raw) To: Tiezhu Yang Cc: Bjorn Helgaas, linux-pci, linux-kernel, Rafael J. Wysocki, Xuefeng Li, Huacai Chen [+cc Huacai] On Thu, Sep 10, 2020 at 02:41:39PM -0500, Bjorn Helgaas wrote: > On Sat, Sep 05, 2020 at 04:33:26PM +0800, Tiezhu Yang wrote: > > After commit 745be2e700cd ("PCIe: portdrv: call pci_disable_device > > during remove") and commit cc27b735ad3a ("PCI/portdrv: Turn off PCIe > > services during shutdown"), it also calls pci_disable_device() during > > shutdown, this seems unnecessary, so just remove it. > > I would like to get rid of the portdrv completely by folding its > functionality into the PCI core itself, so there would be no portdrv > probe or remove. > > Does this solve a problem? If not, I'm inclined to just leave it > as-is for now. But if it fixes something, we should do the fix, of > course. This looks awfully similar to [1], so I guess we *do* need to do something here. I'll respond there since it has more details. [1] https://lore.kernel.org/r/1596268180-9114-1-git-send-email-chenhc@lemote.com > > Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn> > > --- > > drivers/pci/pcie/portdrv_core.c | 1 - > > drivers/pci/pcie/portdrv_pci.c | 14 +++++++++++++- > > 2 files changed, 13 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c > > index 50a9522..1991aca 100644 > > --- a/drivers/pci/pcie/portdrv_core.c > > +++ b/drivers/pci/pcie/portdrv_core.c > > @@ -491,7 +491,6 @@ void pcie_port_device_remove(struct pci_dev *dev) > > { > > device_for_each_child(&dev->dev, NULL, remove_iter); > > pci_free_irq_vectors(dev); > > - pci_disable_device(dev); > > } > > > > /** > > diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c > > index 3a3ce40..cab37a8 100644 > > --- a/drivers/pci/pcie/portdrv_pci.c > > +++ b/drivers/pci/pcie/portdrv_pci.c > > @@ -143,6 +143,18 @@ static void pcie_portdrv_remove(struct pci_dev *dev) > > } > > > > pcie_port_device_remove(dev); > > + pci_disable_device(dev); > > +} > > + > > +static void pcie_portdrv_shutdown(struct pci_dev *dev) > > +{ > > + if (pci_bridge_d3_possible(dev)) { > > + pm_runtime_forbid(&dev->dev); > > + pm_runtime_get_noresume(&dev->dev); > > + pm_runtime_dont_use_autosuspend(&dev->dev); > > + } > > + > > + pcie_port_device_remove(dev); > > } > > > > static pci_ers_result_t pcie_portdrv_error_detected(struct pci_dev *dev, > > @@ -211,7 +223,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, > > > > -- > > 2.1.0 > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] PCI/portdrv: No need to call pci_disable_device() during shutdown 2020-09-10 20:21 ` Bjorn Helgaas @ 2020-09-11 1:54 ` Tiezhu Yang 2020-09-11 2:35 ` Oliver O'Halloran 0 siblings, 1 reply; 6+ messages in thread From: Tiezhu Yang @ 2020-09-11 1:54 UTC (permalink / raw) To: Bjorn Helgaas Cc: Bjorn Helgaas, linux-pci, linux-kernel, Rafael J. Wysocki, Xuefeng Li, Huacai Chen, Jiaxun Yang On 09/11/2020 04:21 AM, Bjorn Helgaas wrote: > [+cc Huacai] > > On Thu, Sep 10, 2020 at 02:41:39PM -0500, Bjorn Helgaas wrote: >> On Sat, Sep 05, 2020 at 04:33:26PM +0800, Tiezhu Yang wrote: >>> After commit 745be2e700cd ("PCIe: portdrv: call pci_disable_device >>> during remove") and commit cc27b735ad3a ("PCI/portdrv: Turn off PCIe >>> services during shutdown"), it also calls pci_disable_device() during >>> shutdown, this seems unnecessary, so just remove it. >> I would like to get rid of the portdrv completely by folding its >> functionality into the PCI core itself, so there would be no portdrv >> probe or remove. >> >> Does this solve a problem? Yes, sometimes it can not shutdown or reboot normally with pci_disable_device(). >> If not, I'm inclined to just leave it >> as-is for now. But if it fixes something, we should do the fix, of >> course. > This looks awfully similar to [1], so I guess we *do* need to do > something here. I'll respond there since it has more details. > > [1] https://lore.kernel.org/r/1596268180-9114-1-git-send-email-chenhc@lemote.com > >>> Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn> >>> --- >>> drivers/pci/pcie/portdrv_core.c | 1 - >>> drivers/pci/pcie/portdrv_pci.c | 14 +++++++++++++- >>> 2 files changed, 13 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c >>> index 50a9522..1991aca 100644 >>> --- a/drivers/pci/pcie/portdrv_core.c >>> +++ b/drivers/pci/pcie/portdrv_core.c >>> @@ -491,7 +491,6 @@ void pcie_port_device_remove(struct pci_dev *dev) >>> { >>> device_for_each_child(&dev->dev, NULL, remove_iter); >>> pci_free_irq_vectors(dev); >>> - pci_disable_device(dev); >>> } >>> >>> /** >>> diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c >>> index 3a3ce40..cab37a8 100644 >>> --- a/drivers/pci/pcie/portdrv_pci.c >>> +++ b/drivers/pci/pcie/portdrv_pci.c >>> @@ -143,6 +143,18 @@ static void pcie_portdrv_remove(struct pci_dev *dev) >>> } >>> >>> pcie_port_device_remove(dev); >>> + pci_disable_device(dev); >>> +} >>> + >>> +static void pcie_portdrv_shutdown(struct pci_dev *dev) >>> +{ >>> + if (pci_bridge_d3_possible(dev)) { >>> + pm_runtime_forbid(&dev->dev); >>> + pm_runtime_get_noresume(&dev->dev); >>> + pm_runtime_dont_use_autosuspend(&dev->dev); >>> + } >>> + >>> + pcie_port_device_remove(dev); >>> } >>> >>> static pci_ers_result_t pcie_portdrv_error_detected(struct pci_dev *dev, >>> @@ -211,7 +223,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, >>> >>> -- >>> 2.1.0 >>> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] PCI/portdrv: No need to call pci_disable_device() during shutdown 2020-09-11 1:54 ` Tiezhu Yang @ 2020-09-11 2:35 ` Oliver O'Halloran 2020-09-11 3:19 ` Tiezhu Yang 0 siblings, 1 reply; 6+ messages in thread From: Oliver O'Halloran @ 2020-09-11 2:35 UTC (permalink / raw) To: Tiezhu Yang Cc: Bjorn Helgaas, Bjorn Helgaas, linux-pci, Linux Kernel Mailing List, Rafael J. Wysocki, Xuefeng Li, Huacai Chen, Jiaxun Yang On Fri, Sep 11, 2020 at 11:55 AM Tiezhu Yang <yangtiezhu@loongson.cn> wrote: > > On 09/11/2020 04:21 AM, Bjorn Helgaas wrote: > > [+cc Huacai] > > > > On Thu, Sep 10, 2020 at 02:41:39PM -0500, Bjorn Helgaas wrote: > >> On Sat, Sep 05, 2020 at 04:33:26PM +0800, Tiezhu Yang wrote: > >>> After commit 745be2e700cd ("PCIe: portdrv: call pci_disable_device > >>> during remove") and commit cc27b735ad3a ("PCI/portdrv: Turn off PCIe > >>> services during shutdown"), it also calls pci_disable_device() during > >>> shutdown, this seems unnecessary, so just remove it. > >> I would like to get rid of the portdrv completely by folding its > >> functionality into the PCI core itself, so there would be no portdrv > >> probe or remove. > >> > >> Does this solve a problem? > > Yes, sometimes it can not shutdown or reboot normally with > pci_disable_device(). Do you have any more details about what goes wrong here? Leaving devices enabled when actually shutting down probably doesn't matter. However, .shutdown() is also used when kexec()ing into a new kernel and we probably should be disabling devices before handing over to the new kernel. Is the real issue that we're closing the bridge windows before the endpoint drivers have had a chance to clean up? Oliver ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] PCI/portdrv: No need to call pci_disable_device() during shutdown 2020-09-11 2:35 ` Oliver O'Halloran @ 2020-09-11 3:19 ` Tiezhu Yang 0 siblings, 0 replies; 6+ messages in thread From: Tiezhu Yang @ 2020-09-11 3:19 UTC (permalink / raw) To: Oliver O'Halloran Cc: Bjorn Helgaas, Bjorn Helgaas, linux-pci, Linux Kernel Mailing List, Rafael J. Wysocki, Xuefeng Li, Huacai Chen, Jiaxun Yang On 09/11/2020 10:35 AM, Oliver O'Halloran wrote: > On Fri, Sep 11, 2020 at 11:55 AM Tiezhu Yang <yangtiezhu@loongson.cn> wrote: >> On 09/11/2020 04:21 AM, Bjorn Helgaas wrote: >>> [+cc Huacai] >>> >>> On Thu, Sep 10, 2020 at 02:41:39PM -0500, Bjorn Helgaas wrote: >>>> On Sat, Sep 05, 2020 at 04:33:26PM +0800, Tiezhu Yang wrote: >>>>> After commit 745be2e700cd ("PCIe: portdrv: call pci_disable_device >>>>> during remove") and commit cc27b735ad3a ("PCI/portdrv: Turn off PCIe >>>>> services during shutdown"), it also calls pci_disable_device() during >>>>> shutdown, this seems unnecessary, so just remove it. >>>> I would like to get rid of the portdrv completely by folding its >>>> functionality into the PCI core itself, so there would be no portdrv >>>> probe or remove. >>>> >>>> Does this solve a problem? >> Yes, sometimes it can not shutdown or reboot normally with >> pci_disable_device(). > Do you have any more details about what goes wrong here? This issue is related with the operation "pci_command &= ~PCI_COMMAND_MASTER;" in the following function: drivers/pci/pci.c static void do_pci_disable_device(struct pci_dev *dev) { u16 pci_command; pci_read_config_word(dev, PCI_COMMAND, &pci_command); if (pci_command & PCI_COMMAND_MASTER) { pci_command &= ~PCI_COMMAND_MASTER; pci_write_config_word(dev, PCI_COMMAND, pci_command); } pcibios_disable_device(dev); } When remove "pci_command &= ~PCI_COMMAND_MASTER;", it can work well. > Leaving > devices enabled when actually shutting down probably doesn't matter. Yes, I think so too. > However, .shutdown() is also used when kexec()ing into a new kernel > and we probably should be disabling devices before handing over to the > new kernel. > > Is the real issue that we're closing the bridge windows before the > endpoint drivers have had a chance to clean up? I notice that check kexec_in_progress first before call pci_disable_devie() in pci_device_shutdown(), can we do the similar thing in pcie? drivers/pci/pci-driver.c static void pci_device_shutdown(struct device *dev) { struct pci_dev *pci_dev = to_pci_dev(dev); struct pci_driver *drv = pci_dev->driver; pm_runtime_resume(dev); if (drv && drv->shutdown) drv->shutdown(pci_dev); /* * If this is a kexec reboot, turn off Bus Master bit on the * device to tell it to not continue to do DMA. Don't touch * devices in D3cold or unknown states. * If it is not a kexec reboot, firmware will hit the PCI * devices with big hammer and stop their DMA any way. */ if (kexec_in_progress && (pci_dev->current_state <= PCI_D3hot)) pci_clear_master(pci_dev); } Something like this: diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c index 50a9522..3de1dab 100644 --- a/drivers/pci/pcie/portdrv_core.c +++ b/drivers/pci/pcie/portdrv_core.c @@ -491,7 +491,8 @@ void pcie_port_device_remove(struct pci_dev *dev) { device_for_each_child(&dev->dev, NULL, remove_iter); pci_free_irq_vectors(dev); - pci_disable_device(dev); + if (kexec_in_progress && (pci_dev->current_state <= PCI_D3hot)) + pci_disable_device(dev); } /** > > Oliver ^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-09-11 3:20 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-09-05 8:33 [RFC PATCH] PCI/portdrv: No need to call pci_disable_device() during shutdown Tiezhu Yang 2020-09-10 19:41 ` Bjorn Helgaas 2020-09-10 20:21 ` Bjorn Helgaas 2020-09-11 1:54 ` Tiezhu Yang 2020-09-11 2:35 ` Oliver O'Halloran 2020-09-11 3:19 ` Tiezhu Yang
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.