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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
>
[+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/[email protected]
> > Signed-off-by: Tiezhu Yang <[email protected]>
> > ---
> > 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
> >
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/[email protected]
>
>>> Signed-off-by: Tiezhu Yang <[email protected]>
>>> ---
>>> 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
>>>
On Fri, Sep 11, 2020 at 11:55 AM Tiezhu Yang <[email protected]> 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
On 09/11/2020 10:35 AM, Oliver O'Halloran wrote:
> On Fri, Sep 11, 2020 at 11:55 AM Tiezhu Yang <[email protected]> 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