* Re: [PATCH 1/2] PCI: ASPM exit link state code could skip devices [not found] ` <alpine.DEB.2.02.1301181322500.31163@jlaw-desktop.mno.stratus.com> @ 2013-01-31 23:29 ` Myron Stowe 2013-02-01 19:55 ` Joe Lawrence 0 siblings, 1 reply; 3+ messages in thread From: Myron Stowe @ 2013-01-31 23:29 UTC (permalink / raw) To: Joe Lawrence Cc: linux-pci, Matthew Garrett, David Bulkow, shli, shli, kaneshige.kenji, linux-kernel On Fri, 2013-01-18 at 13:23 -0500, Joe Lawrence wrote: > From 3a51bbad5555ba6c6e144aa5176c8112eb449325 Mon Sep 17 00:00:00 2001 > From: Joe Lawrence <joe.lawrence@stratus.com> > Date: Tue, 15 Jan 2013 14:51:57 -0500 > Subject: [PATCH 1/2] PCI: ASPM exit link state code could skip devices > > On PCI bus hotplug removal, pcie_aspm_exit_link_state could potentially skip > parent devices that have link_state allocated. Instead of exiting early if > a given device is not PCIe, check that the device's parent is PCIe. This > enables pcie_aspm_exit_link_state to properly clean up parent link_state when > the last function in a slot might not be PCIe. > > Reviewed-by: David Bulkow <david.bulkow@stratus.com> > Signed-off-by: Joe Lawrence <joe.lawrence@stratus.com> > --- > drivers/pci/pcie/aspm.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c > index b52630b..6122447 100644 > --- a/drivers/pci/pcie/aspm.c > +++ b/drivers/pci/pcie/aspm.c > @@ -634,7 +634,7 @@ void pcie_aspm_exit_link_state(struct pci_dev *pdev) > struct pci_dev *parent = pdev->bus->self; > struct pcie_link_state *link, *root, *parent_link; > > - if (!pci_is_pcie(pdev) || !parent || !parent->link_state) > + if (!parent || !pci_is_pcie(parent) || !parent->link_state) > return; > if ((pci_pcie_type(parent) != PCI_EXP_TYPE_ROOT_PORT) && > (pci_pcie_type(parent) != PCI_EXP_TYPE_DOWNSTREAM)) Joe: Bjorn and I looked at this again today and think the checks that occur early in 'pcie_aspm_exit_link_state()' could be simplified (as shown below) to avoid issues such as you are encountering. I think part of the confusion concerning the asymmetry between 'pcie_aspm_init_link_state()' and 'pcie_aspm_exit_link_state()' is that the former is passed in a pointer to a bridge device whereas the latter is passed in a pointer to a end-point device. Frankly, I find the entire driver to be confusing and very hard to understand. Could you please test with the following patch and let us know the results? PCI: ASPM exit link state code is skipping devices From: Myron Stowe <myron.stowe@redhat.com> On PCI bus hotplug removal, 'pcie_aspm_exit_link_state' can potentially skip parent devices that have link_state allocated. Instead of exiting early if a given device in not PCIe, check whether or not the device's parent has link_state allocated. This enables 'pcie_aspm_exit_link_state' to properly clean up parent link_state when the last function in a slot might not be PCIe. Reported-by: Joe Lawrence <joe.lawrence@stratus.com> Signed-off-by: Myron Stowe <myron.stowe@redhat.com> --- drivers/pci/pcie/aspm.c | 5 +---- 1 files changed, 1 insertions(+), 4 deletions(-) diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c index 8474b6a..aa52727 100644 --- a/drivers/pci/pcie/aspm.c +++ b/drivers/pci/pcie/aspm.c @@ -634,10 +634,7 @@ void pcie_aspm_exit_link_state(struct pci_dev *pdev) struct pci_dev *parent = pdev->bus->self; struct pcie_link_state *link, *root, *parent_link; - if (!pci_is_pcie(pdev) || !parent || !parent->link_state) - return; - if ((pci_pcie_type(parent) != PCI_EXP_TYPE_ROOT_PORT) && - (pci_pcie_type(parent) != PCI_EXP_TYPE_DOWNSTREAM)) + if (!parent || !parent->link_state) return; down_read(&pci_bus_sem); ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH 1/2] PCI: ASPM exit link state code could skip devices 2013-01-31 23:29 ` [PATCH 1/2] PCI: ASPM exit link state code could skip devices Myron Stowe @ 2013-02-01 19:55 ` Joe Lawrence 2013-02-01 22:31 ` Bjorn Helgaas 0 siblings, 1 reply; 3+ messages in thread From: Joe Lawrence @ 2013-02-01 19:55 UTC (permalink / raw) To: Myron Stowe Cc: Joe Lawrence, linux-pci, Matthew Garrett, David Bulkow, shli, shli, kaneshige.kenji, linux-kernel On Thu, 31 Jan 2013, Myron Stowe wrote: > On Fri, 2013-01-18 at 13:23 -0500, Joe Lawrence wrote: > > From 3a51bbad5555ba6c6e144aa5176c8112eb449325 Mon Sep 17 00:00:00 2001 > > From: Joe Lawrence <joe.lawrence@stratus.com> > > Date: Tue, 15 Jan 2013 14:51:57 -0500 > > Subject: [PATCH 1/2] PCI: ASPM exit link state code could skip devices > > > > On PCI bus hotplug removal, pcie_aspm_exit_link_state could potentially skip > > parent devices that have link_state allocated. Instead of exiting early if > > a given device is not PCIe, check that the device's parent is PCIe. This > > enables pcie_aspm_exit_link_state to properly clean up parent link_state when > > the last function in a slot might not be PCIe. > > > > Reviewed-by: David Bulkow <david.bulkow@stratus.com> > > Signed-off-by: Joe Lawrence <joe.lawrence@stratus.com> > > --- > > drivers/pci/pcie/aspm.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c > > index b52630b..6122447 100644 > > --- a/drivers/pci/pcie/aspm.c > > +++ b/drivers/pci/pcie/aspm.c > > @@ -634,7 +634,7 @@ void pcie_aspm_exit_link_state(struct pci_dev *pdev) > > struct pci_dev *parent = pdev->bus->self; > > struct pcie_link_state *link, *root, *parent_link; > > > > - if (!pci_is_pcie(pdev) || !parent || !parent->link_state) > > + if (!parent || !pci_is_pcie(parent) || !parent->link_state) > > return; > > if ((pci_pcie_type(parent) != PCI_EXP_TYPE_ROOT_PORT) && > > (pci_pcie_type(parent) != PCI_EXP_TYPE_DOWNSTREAM)) > > Joe: > > Bjorn and I looked at this again today and think the checks that occur > early in 'pcie_aspm_exit_link_state()' could be simplified (as shown > below) to avoid issues such as you are encountering. > > I think part of the confusion concerning the asymmetry between > 'pcie_aspm_init_link_state()' and 'pcie_aspm_exit_link_state()' is that > the former is passed in a pointer to a bridge device whereas the latter > is passed in a pointer to a end-point device. Frankly, I find the > entire driver to be confusing and very hard to understand. > > Could you please test with the following patch and let us know the > results? > > > PCI: ASPM exit link state code is skipping devices > > From: Myron Stowe <myron.stowe@redhat.com> > > On PCI bus hotplug removal, 'pcie_aspm_exit_link_state' can potentially > skip parent devices that have link_state allocated. Instead of exiting > early if a given device in not PCIe, check whether or not the device's > parent has link_state allocated. This enables 'pcie_aspm_exit_link_state' > to properly clean up parent link_state when the last function in a slot > might not be PCIe. > > Reported-by: Joe Lawrence <joe.lawrence@stratus.com> > Signed-off-by: Myron Stowe <myron.stowe@redhat.com> > --- > > drivers/pci/pcie/aspm.c | 5 +---- > 1 files changed, 1 insertions(+), 4 deletions(-) > > > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c > index 8474b6a..aa52727 100644 > --- a/drivers/pci/pcie/aspm.c > +++ b/drivers/pci/pcie/aspm.c > @@ -634,10 +634,7 @@ void pcie_aspm_exit_link_state(struct pci_dev *pdev) > struct pci_dev *parent = pdev->bus->self; > struct pcie_link_state *link, *root, *parent_link; > > - if (!pci_is_pcie(pdev) || !parent || !parent->link_state) > - return; > - if ((pci_pcie_type(parent) != PCI_EXP_TYPE_ROOT_PORT) && > - (pci_pcie_type(parent) != PCI_EXP_TYPE_DOWNSTREAM)) > + if (!parent || !parent->link_state) > return; > > down_read(&pci_bus_sem); > > > Hi Myron, The patch tests eliminates the reported crash when hotplugging our problem PCI bus. The additional checks removed from pcie_aspm_exit_link_state (ie, the simplification) do appear to be unnecessary as pcie_aspm_init_link_state is already preventing such devices from allocating link state. Thanks, -- Joe ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH 1/2] PCI: ASPM exit link state code could skip devices 2013-02-01 19:55 ` Joe Lawrence @ 2013-02-01 22:31 ` Bjorn Helgaas 0 siblings, 0 replies; 3+ messages in thread From: Bjorn Helgaas @ 2013-02-01 22:31 UTC (permalink / raw) To: Joe Lawrence Cc: Myron Stowe, linux-pci, Matthew Garrett, David Bulkow, shli, shli, kaneshige.kenji, linux-kernel On Fri, Feb 1, 2013 at 12:55 PM, Joe Lawrence <Joe.Lawrence@stratus.com> wrote: > On Thu, 31 Jan 2013, Myron Stowe wrote: >> PCI: ASPM exit link state code is skipping devices >> >> From: Myron Stowe <myron.stowe@redhat.com> >> >> On PCI bus hotplug removal, 'pcie_aspm_exit_link_state' can potentially >> skip parent devices that have link_state allocated. Instead of exiting >> early if a given device in not PCIe, check whether or not the device's >> parent has link_state allocated. This enables 'pcie_aspm_exit_link_state' >> to properly clean up parent link_state when the last function in a slot >> might not be PCIe. >> >> Reported-by: Joe Lawrence <joe.lawrence@stratus.com> >> Signed-off-by: Myron Stowe <myron.stowe@redhat.com> >> --- >> >> drivers/pci/pcie/aspm.c | 5 +---- >> 1 files changed, 1 insertions(+), 4 deletions(-) >> >> >> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c >> index 8474b6a..aa52727 100644 >> --- a/drivers/pci/pcie/aspm.c >> +++ b/drivers/pci/pcie/aspm.c >> @@ -634,10 +634,7 @@ void pcie_aspm_exit_link_state(struct pci_dev *pdev) >> struct pci_dev *parent = pdev->bus->self; >> struct pcie_link_state *link, *root, *parent_link; >> >> - if (!pci_is_pcie(pdev) || !parent || !parent->link_state) >> - return; >> - if ((pci_pcie_type(parent) != PCI_EXP_TYPE_ROOT_PORT) && >> - (pci_pcie_type(parent) != PCI_EXP_TYPE_DOWNSTREAM)) >> + if (!parent || !parent->link_state) >> return; >> >> down_read(&pci_bus_sem); >> >> >> > > Hi Myron, > > The patch tests eliminates the reported crash when hotplugging our > problem PCI bus. > > The additional checks removed from pcie_aspm_exit_link_state (ie, the > simplification) do appear to be unnecessary as pcie_aspm_init_link_state > is already preventing such devices from allocating link state. I added a Tested-by for you, Joe, and applied this to my pci/joe-aspm branch for v3.9. Thanks! ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2013-02-01 22:32 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <alpine.DEB.2.02.1301181302260.31163@jlaw-desktop.mno.stratus.com>
[not found] ` <alpine.DEB.2.02.1301181322500.31163@jlaw-desktop.mno.stratus.com>
2013-01-31 23:29 ` [PATCH 1/2] PCI: ASPM exit link state code could skip devices Myron Stowe
2013-02-01 19:55 ` Joe Lawrence
2013-02-01 22:31 ` Bjorn Helgaas
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®