linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).