[PATCH v4 1/3] PCI: Add pdev->has_secondary_link to mark pcie link

[PATCH v4 1/3] PCI: Add pdev->has_secondary_link to mark pcie link

[Yijing Wang]

We assumed root port and downstream port always have
pcie link, downstream port always has a upstream port.
It's not always correct, In ATCA platform, system has
unusual pcie topology like:

(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]--

In this pcie tree, downstream port 02:00.0 has no
pcie link, and upstream port 03:00.0 has a pcie link.

This patch introduced a new assumption suggested by Bjorn.
1. Root port is always on the upstream end of a link.
2. The pcie hierarchy should alternate between links
and internal switch logic, there should be no adjacent
links or internal buses in pcie tree.

Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
Signed-off-by: Yijing Wang <wangyijing@huawei.com>
---
 drivers/pci/probe.c |   16 ++++++++++++++++
 include/linux/pci.h |    1 +
 2 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index a9c5e63..192c6b9 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -973,6 +973,7 @@ void set_pcie_port_type(struct pci_dev *pdev)
 {
 	int pos;
 	u16 reg16;
+	struct pci_dev *parent;
 
 	pos = pci_find_capability(pdev, PCI_CAP_ID_EXP);
 	if (!pos)
@@ -982,6 +983,21 @@ void set_pcie_port_type(struct pci_dev *pdev)
 	pdev->pcie_flags_reg = reg16;
 	pci_read_config_word(pdev, pos + PCI_EXP_DEVCAP, &reg16);
 	pdev->pcie_mpss = reg16 & PCI_EXP_DEVCAP_PAYLOAD;
+
+	/*
+	 * We assume root port is always on the upstream end of
+	 * a link, and the pcie hierarchy should alternate
+	 * between links and internal switch logic.
+	 */
+	if (pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT)
+		pdev->has_secondary_link = 1;
+
+	if (pci_pcie_type(pdev) == PCI_EXP_TYPE_UPSTREAM
+			|| pci_pcie_type(pdev) == PCI_EXP_TYPE_DOWNSTREAM) {
+		parent = pci_upstream_bridge(pdev);
+		if (!parent->has_secondary_link)
+			pdev->has_secondary_link = 1;
+	}
 }
 
 void set_pcie_hotplug_bridge(struct pci_dev *pdev)
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 50b7c7d..141fcc1 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -355,6 +355,7 @@ struct pci_dev {
 	unsigned int	broken_intx_masking:1;
 	unsigned int	io_window_1k:1;	/* Intel P2P bridge 1K I/O windows */
 	unsigned int	irq_managed:1;
+	unsigned int	has_secondary_link:1;
 	pci_dev_flags_t dev_flags;
 	atomic_t	enable_cnt;	/* pci_enable_device has been called */
 
-- 
1.7.1

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

[Yijing Wang]

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;
-- 
1.7.1

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

[Bjorn Helgaas]

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

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

[Yijing Wang]

> 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)".

Good catch, thanks!

> 
> 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?

Yes.

I will update it.

Thanks!
Yijing.

> 
> Bjorn
> 
> .
> 


-- 
Thanks!
Yijing

[PATCH v4 3/3] PCI: Use pdev->has_secondary_link to identify pcie link

[Yijing Wang]

We assumed pcie root port and downstream port always have
pcie link, but in some unusual pcie topology platform like
ATCA, it may has the following pcie tree:

root port ---- downstream port -----upstream port
                                   |
                                   |downstream port
Now we introduce a flag pdev->has_secondary_link to tag
a device whether has pcie link, use it instead.

Signed-off-by: Yijing Wang <wangyijing@huawei.com>
---
 drivers/pci/pcie/aer/aerdrv_core.c |    3 +--
 drivers/pci/probe.c                |    2 +-
 drivers/pci/vc.c                   |    3 +--
 3 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/pcie/aer/aerdrv_core.c b/drivers/pci/pcie/aer/aerdrv_core.c
index 5653ea9..9803e3d 100644
--- a/drivers/pci/pcie/aer/aerdrv_core.c
+++ b/drivers/pci/pcie/aer/aerdrv_core.c
@@ -425,8 +425,7 @@ static pci_ers_result_t reset_link(struct pci_dev *dev)
 
 	if (driver && driver->reset_link) {
 		status = driver->reset_link(udev);
-	} else if (pci_pcie_type(udev) == PCI_EXP_TYPE_DOWNSTREAM ||
-		pci_pcie_type(udev) == PCI_EXP_TYPE_ROOT_PORT) {
+	} else if (udev->has_secondary_link) {
 		status = default_reset_link(udev);
 	} else {
 		dev_printk(KERN_DEBUG, &dev->dev,
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 192c6b9..eba4928 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1645,7 +1645,7 @@ static int only_one_child(struct pci_bus *bus)
 		return 0;
 	if (pci_pcie_type(parent) == PCI_EXP_TYPE_ROOT_PORT)
 		return 1;
-	if (pci_pcie_type(parent) == PCI_EXP_TYPE_DOWNSTREAM &&
+	if (parent->has_secondary_link &&
 	    !pci_has_flag(PCI_SCAN_ALL_PCIE_DEVS))
 		return 1;
 	return 0;
diff --git a/drivers/pci/vc.c b/drivers/pci/vc.c
index 7e1304d..dfbab61 100644
--- a/drivers/pci/vc.c
+++ b/drivers/pci/vc.c
@@ -108,8 +108,7 @@ static void pci_vc_enable(struct pci_dev *dev, int pos, int res)
 	struct pci_dev *link = NULL;
 
 	/* Enable VCs from the downstream device */
-	if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
-	    pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM)
+	if (!dev->has_secondary_link)
 		return;
 
 	ctrl_pos = pos + PCI_VC_RES_CTRL + (res * PCI_CAP_VC_PER_VC_SIZEOF);
-- 
1.7.1

Re: [PATCH v4 3/3] PCI: Use pdev->has_secondary_link to identify pcie link

[Bjorn Helgaas]

On Thu, May 21, 2015 at 03:05:04PM +0800, Yijing Wang wrote:
> We assumed pcie root port and downstream port always have
> pcie link, but in some unusual pcie topology platform like
> ATCA, it may has the following pcie tree:
> 
> root port ---- downstream port -----upstream port
>                                    |
>                                    |downstream port
> Now we introduce a flag pdev->has_secondary_link to tag
> a device whether has pcie link, use it instead.
> 
> Signed-off-by: Yijing Wang <wangyijing@huawei.com>

Applied to pci/aspm for v4.2, thanks!

> ---
>  drivers/pci/pcie/aer/aerdrv_core.c |    3 +--
>  drivers/pci/probe.c                |    2 +-
>  drivers/pci/vc.c                   |    3 +--
>  3 files changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/pcie/aer/aerdrv_core.c b/drivers/pci/pcie/aer/aerdrv_core.c
> index 5653ea9..9803e3d 100644
> --- a/drivers/pci/pcie/aer/aerdrv_core.c
> +++ b/drivers/pci/pcie/aer/aerdrv_core.c
> @@ -425,8 +425,7 @@ static pci_ers_result_t reset_link(struct pci_dev *dev)
>  
>  	if (driver && driver->reset_link) {
>  		status = driver->reset_link(udev);
> -	} else if (pci_pcie_type(udev) == PCI_EXP_TYPE_DOWNSTREAM ||
> -		pci_pcie_type(udev) == PCI_EXP_TYPE_ROOT_PORT) {
> +	} else if (udev->has_secondary_link) {
>  		status = default_reset_link(udev);
>  	} else {
>  		dev_printk(KERN_DEBUG, &dev->dev,
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 192c6b9..eba4928 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1645,7 +1645,7 @@ static int only_one_child(struct pci_bus *bus)
>  		return 0;
>  	if (pci_pcie_type(parent) == PCI_EXP_TYPE_ROOT_PORT)
>  		return 1;
> -	if (pci_pcie_type(parent) == PCI_EXP_TYPE_DOWNSTREAM &&
> +	if (parent->has_secondary_link &&
>  	    !pci_has_flag(PCI_SCAN_ALL_PCIE_DEVS))
>  		return 1;
>  	return 0;
> diff --git a/drivers/pci/vc.c b/drivers/pci/vc.c
> index 7e1304d..dfbab61 100644
> --- a/drivers/pci/vc.c
> +++ b/drivers/pci/vc.c
> @@ -108,8 +108,7 @@ static void pci_vc_enable(struct pci_dev *dev, int pos, int res)
>  	struct pci_dev *link = NULL;
>  
>  	/* Enable VCs from the downstream device */
> -	if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
> -	    pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM)
> +	if (!dev->has_secondary_link)
>  		return;
>  
>  	ctrl_pos = pos + PCI_VC_RES_CTRL + (res * PCI_CAP_VC_PER_VC_SIZEOF);
> -- 
> 1.7.1
> 

Re: [PATCH v4 1/3] PCI: Add pdev->has_secondary_link to mark pcie link

[Bjorn Helgaas]

On Thu, May 21, 2015 at 03:05:02PM +0800, Yijing Wang wrote:
> We assumed root port and downstream port always have
> pcie link, downstream port always has a upstream port.
> It's not always correct, In ATCA platform, system has
> unusual pcie topology like:
> 
> (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]--
> 
> In this pcie tree, downstream port 02:00.0 has no
> pcie link, and upstream port 03:00.0 has a pcie link.
> 
> This patch introduced a new assumption suggested by Bjorn.
> 1. Root port is always on the upstream end of a link.
> 2. The pcie hierarchy should alternate between links
> and internal switch logic, there should be no adjacent
> links or internal buses in pcie tree.
> 
> Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
> Signed-off-by: Yijing Wang <wangyijing@huawei.com>

Applied to pci/aspm for v4.2, thanks!

> ---
>  drivers/pci/probe.c |   16 ++++++++++++++++
>  include/linux/pci.h |    1 +
>  2 files changed, 17 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index a9c5e63..192c6b9 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -973,6 +973,7 @@ void set_pcie_port_type(struct pci_dev *pdev)
>  {
>  	int pos;
>  	u16 reg16;
> +	struct pci_dev *parent;
>  
>  	pos = pci_find_capability(pdev, PCI_CAP_ID_EXP);
>  	if (!pos)
> @@ -982,6 +983,21 @@ void set_pcie_port_type(struct pci_dev *pdev)
>  	pdev->pcie_flags_reg = reg16;
>  	pci_read_config_word(pdev, pos + PCI_EXP_DEVCAP, &reg16);
>  	pdev->pcie_mpss = reg16 & PCI_EXP_DEVCAP_PAYLOAD;
> +
> +	/*
> +	 * We assume root port is always on the upstream end of
> +	 * a link, and the pcie hierarchy should alternate
> +	 * between links and internal switch logic.
> +	 */
> +	if (pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT)
> +		pdev->has_secondary_link = 1;
> +
> +	if (pci_pcie_type(pdev) == PCI_EXP_TYPE_UPSTREAM
> +			|| pci_pcie_type(pdev) == PCI_EXP_TYPE_DOWNSTREAM) {
> +		parent = pci_upstream_bridge(pdev);
> +		if (!parent->has_secondary_link)
> +			pdev->has_secondary_link = 1;
> +	}
>  }
>  
>  void set_pcie_hotplug_bridge(struct pci_dev *pdev)
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 50b7c7d..141fcc1 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -355,6 +355,7 @@ struct pci_dev {
>  	unsigned int	broken_intx_masking:1;
>  	unsigned int	io_window_1k:1;	/* Intel P2P bridge 1K I/O windows */
>  	unsigned int	irq_managed:1;
> +	unsigned int	has_secondary_link:1;
>  	pci_dev_flags_t dev_flags;
>  	atomic_t	enable_cnt;	/* pci_enable_device has been called */
>  
> -- 
> 1.7.1
>