* 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®