Re: [PATCH 1/2] PCI: ASPM exit link state code could skip devices

[Joe Lawrence]

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@xxxxxxxxxxx>
> > 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@xxxxxxxxxxx>
> > Signed-off-by: Joe Lawrence <joe.lawrence@xxxxxxxxxxx>
> > ---
> >  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@xxxxxxxxxx>
> 
> 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@xxxxxxxxxxx>
> Signed-off-by: Myron Stowe <myron.stowe@xxxxxxxxxx>
> ---
> 
>  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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/