[PATCH v2] PCI: Fix NULL pointer when find parent pcie_link_state

[PATCH v2] PCI: 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.

This patch fix this issue based on the following
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/pcie/aspm.c |   23 +++++++++++++++++++++--
 1 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 7d4fcdc..61b9d3f 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -546,6 +546,25 @@ static struct pcie_link_state *alloc_pcie_link_state(struct pci_dev *pdev)
 	return link;
 }
 
+/*
+ * 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.
+ */
+static bool pcie_has_external_link(struct pci_dev *pdev)
+{
+	if (!pci_is_pcie(pdev))
+		return false;
+
+	if (pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT)
+		return true;
+
+	if (pci_pcie_type(pdev) == PCI_EXP_TYPE_DOWNSTREAM)
+		if (!pdev->bus->self->link_state)
+			return true;
+
+	return false;
+}
 /*
  * pcie_aspm_init_link_state: Initiate PCI express link state.
  * It is called after the pcie and its children devices are scanned.
@@ -561,8 +580,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 (!pcie_has_external_link(pdev))
 		return;
 
 	/* VIA has a strange chipset, root port is under a bridge */
-- 
1.7.1

Re: [PATCH v2] PCI: Fix NULL pointer when find parent pcie_link_state

[Yijing Wang]

Ping... ?

On 2015/4/24 14:59, 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.
> 
> This patch fix this issue based on the following
> 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/pcie/aspm.c |   23 +++++++++++++++++++++--
>  1 files changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 7d4fcdc..61b9d3f 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -546,6 +546,25 @@ static struct pcie_link_state *alloc_pcie_link_state(struct pci_dev *pdev)
>  	return link;
>  }
>  
> +/*
> + * 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.
> + */
> +static bool pcie_has_external_link(struct pci_dev *pdev)
> +{
> +	if (!pci_is_pcie(pdev))
> +		return false;
> +
> +	if (pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT)
> +		return true;
> +
> +	if (pci_pcie_type(pdev) == PCI_EXP_TYPE_DOWNSTREAM)
> +		if (!pdev->bus->self->link_state)
> +			return true;
> +
> +	return false;
> +}
>  /*
>   * pcie_aspm_init_link_state: Initiate PCI express link state.
>   * It is called after the pcie and its children devices are scanned.
> @@ -561,8 +580,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 (!pcie_has_external_link(pdev))
>  		return;
>  
>  	/* VIA has a strange chipset, root port is under a bridge */
> 


-- 
Thanks!
Yijing

Re: [PATCH v2] PCI: Fix NULL pointer when find parent pcie_link_state

[Bjorn Helgaas]

On Fri, Apr 24, 2015 at 02:59:51PM +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.
> 
> This patch fix this issue based on the following
> 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/pcie/aspm.c |   23 +++++++++++++++++++++--
>  1 files changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 7d4fcdc..61b9d3f 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -546,6 +546,25 @@ static struct pcie_link_state *alloc_pcie_link_state(struct pci_dev *pdev)
>  	return link;
>  }
>  
> +/*
> + * 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.
> + */
> +static bool pcie_has_external_link(struct pci_dev *pdev)
> +{
> +	if (!pci_is_pcie(pdev))
> +		return false;
> +
> +	if (pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT)
> +		return true;
> +
> +	if (pci_pcie_type(pdev) == PCI_EXP_TYPE_DOWNSTREAM)
> +		if (!pdev->bus->self->link_state)
> +			return true;
> +
> +	return false;

I like the idea of this, but I'd rather not bury it inside ASPM and I'd
rather not have it depend on ASPM data (the link_state pointer).  I think
it would be better if we had a real PCI interface, maybe using a bit in
struct pci_dev that we could set in set_pcie_port_type().

We should also look for other cases that might not work on this topology.
The following functions test for PCI_EXP_TYPE_DOWNSTREAM and I think most
of them assume that the Downstream Port has a link on its secondary side,
which is not the case in this topology:

  reset_link()
  alloc_pcie_link_state()
  pcie_aspm_pm_state_change()
  pcie_aspm_powersave_config_link()
  __pci_disable_link_state()
  pcie_aspm_create_sysfs_dev_files()
  pcie_aspm_remove_sysfs_dev_files()
  only_one_child()
  quirk_apple_wait_for_thunderbolt()
  pci_vc_enable()

"has_external_link" doesn't seem very descriptive to me -- I'm not sure how
to interpret "external."  "has_downstream_link" suggests a link on the
downstream (secondary) side, which is more what we're getting at, although
it might be confusing that an Upstream Port can have a downstream link.  Or
maybe "has_secondary_link" or "secondary_is_link".

Bjorn

> +}
>  /*
>   * pcie_aspm_init_link_state: Initiate PCI express link state.
>   * It is called after the pcie and its children devices are scanned.
> @@ -561,8 +580,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 (!pcie_has_external_link(pdev))
>  		return;
>  
>  	/* VIA has a strange chipset, root port is under a bridge */
> -- 
> 1.7.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] PCI: Fix NULL pointer when find parent pcie_link_state

[Yijing Wang]

>> +/*
>> + * 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.
>> + */
>> +static bool pcie_has_external_link(struct pci_dev *pdev)
>> +{
>> +	if (!pci_is_pcie(pdev))
>> +		return false;
>> +
>> +	if (pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT)
>> +		return true;
>> +
>> +	if (pci_pcie_type(pdev) == PCI_EXP_TYPE_DOWNSTREAM)
>> +		if (!pdev->bus->self->link_state)
>> +			return true;
>> +
>> +	return false;
> 
> I like the idea of this, but I'd rather not bury it inside ASPM and I'd
> rather not have it depend on ASPM data (the link_state pointer).  I think
> it would be better if we had a real PCI interface, maybe using a bit in
> struct pci_dev that we could set in set_pcie_port_type().

OK， I will add a flag in pci_dev.

> 
> We should also look for other cases that might not work on this topology.
> The following functions test for PCI_EXP_TYPE_DOWNSTREAM and I think most
> of them assume that the Downstream Port has a link on its secondary side,
> which is not the case in this topology:
> 
>   reset_link()

Right, we should identify pcie link by the new flag.

>   alloc_pcie_link_state()
>   __pci_disable_link_state()

Ditto.

>   pcie_aspm_pm_state_change()
>   pcie_aspm_powersave_config_link()
>   pcie_aspm_create_sysfs_dev_files()
>   pcie_aspm_remove_sysfs_dev_files()

I think it's no need to use new flag to identify whether pcie device has pcie link in above
functions.
For ASPM, we alloc pcie_link_state when call pcie_aspm_init_link_state() -> alloc_pcie_link_state()
In pcie_aspm_init_link_state(), only root port and downstream port devices would have chance to
have a valid pcie_link_state, so in ASPM functions(above), only to check pdev->link_state is enough.
So I would post a cleanup patch to clean the redundant checking code.

>   only_one_child()

Would use the new flag instead.

>   quirk_apple_wait_for_thunderbolt()

This is a quirk, if I use the new flag instead, it may change the logic(now only downstream port deivce could come in), for safe, keep it.

>   pci_vc_enable()

Would use the new flag instead.

> 
> "has_external_link" doesn't seem very descriptive to me -- I'm not sure how
> to interpret "external."  "has_downstream_link" suggests a link on the
> downstream (secondary) side, which is more what we're getting at, although
> it might be confusing that an Upstream Port can have a downstream link.  Or
> maybe "has_secondary_link" or "secondary_is_link"./parent

I like has_secondary_link better.


Thanks!
Yijing.

> 
> Bjorn
> 
>> +}
>>  /*
>>   * pcie_aspm_init_link_state: Initiate PCI express link state.
>>   * It is called after the pcie and its children devices are scanned.
>> @@ -561,8 +580,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 (!pcie_has_external_link(pdev))
>>  		return;
>>  
>>  	/* VIA has a strange chipset, root port is under a bridge */
>> -- 
>> 1.7.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> .
> 


-- 
Thanks!
Yijing