Re: [PATCH v4 2/3] PCI/ASPM: Fix NULL pointer when find parent pcie_link_state

[Bjorn Helgaas <bhelgaas@google.com>]

On Thu, May 21, 2015 at 03:05:03PM +0800, Yijing Wang wrote:
> https://bugzilla.kernel.org/show_bug.cgi?id=94361 reported
> in ATCA platform, system had unusual pcie topology:
> 
> (root port)   (downstream port)   (upstream port)
> +-1c.0-[02-0a]----00.0-[03-0a]--+-00.0-[04]--
> |                               +-01.0-[05]-- (downstream port)
> |                               +-02.0-[06]--
> |                               +-03.0-[07]--
> |                               +-08.0-[08]--
> |                               +-09.0-[09]--
> |                               \-0a.0-[0a]--
> We assumed root port and downstream port always
> have external link, and downstream port always has a
> upstream port. So in this case, when we allocated
> pcie_link_state for downstream port 02:00.0, it try
> to get parent bus pcie_link_state,
> parent = pdev->bus->parent->self->link_state;
> because root bus self is NULL, system will crash here.
> 
> Use pdev->has_secondary_link(introduced in previous patch)
> to fix this issue.
> 
> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
> ---
>  drivers/pci/pcie/aspm.c |    7 +++----
>  1 files changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 7d4fcdc..8830740 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -561,8 +561,8 @@ void pcie_aspm_init_link_state(struct pci_dev *pdev)
>  
>  	if (!pci_is_pcie(pdev) || pdev->link_state)
>  		return;
> -	if (pci_pcie_type(pdev) != PCI_EXP_TYPE_ROOT_PORT &&
> -	    pci_pcie_type(pdev) != PCI_EXP_TYPE_DOWNSTREAM)
> +
> +	if (!pdev->has_secondary_link)
>  		return;
>  
>  	/* VIA has a strange chipset, root port is under a bridge */
> @@ -723,8 +723,7 @@ static void __pci_disable_link_state(struct pci_dev *pdev, int state, bool sem,
>  	if (!pci_is_pcie(pdev))
>  		return;
>  
> -	if (pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT ||
> -	    pci_pcie_type(pdev) == PCI_EXP_TYPE_DOWNSTREAM)
> +	if (pdev->has_secondary_link)
>  		parent = pdev;
>  	if (!parent || !parent->link_state)
>  		return;

I think we also have a problem here:

   517  static struct pcie_link_state *alloc_pcie_link_state(struct pci_dev *pdev)
   518  {
   519          struct pcie_link_state *link;
   520  
   521          link = kzalloc(sizeof(*link), GFP_KERNEL);
   522          if (!link)
   523                  return NULL;
   524          INIT_LIST_HEAD(&link->sibling);
   525          INIT_LIST_HEAD(&link->children);
   526          INIT_LIST_HEAD(&link->link);
   527          link->pdev = pdev;
   528          if (pci_pcie_type(pdev) == PCI_EXP_TYPE_DOWNSTREAM) {
   529                  struct pcie_link_state *parent;
   530                  parent = pdev->bus->parent->self->link_state;
   531                  if (!parent) {
   532                          kfree(link);
   533                          return NULL;
   534                  }
   535                  link->parent = parent;
   536                  list_add(&link->link, &parent->children);
   537          }

Before your patch, we could only see Root Ports or Downstream Ports at line
528.  Now we may also see Upstream Ports, so I think that test should be
changed to "if (pci_pcie_type(pdev) != PCI_EXP_TYPE_ROOT_PORT)".

Without this change, I think we would allocate pcie_link_state for the
03:00.0 Upstream Port in your example, but it would not be connected to the
tree of links starting at the 00:1c.0 Root Port.

Do you agree?

Bjorn