Re: [PATCH V8 3/5] PCI/ASPM: add init hook to device_add

[Bjorn Helgaas]

On Thu, Apr 13, 2017 at 8:19 PM, Sinan Kaya  wrote:
> On 4/13/2017 5:02 PM, Bjorn Helgaas wrote:
>> I do see that you change the deallocation in patch [5/5], but I think
>> the deallocation change should be in the same patch as the allocation
>> change.  Otherwise I think we have a use-after-free problem in this
>> sequence:
>
> Sure, I'll reorder. As you can see here, link will be only removed if
> root port is being removed.
>
> Without this, we'll hit the use after free issue you mentioned.
>
> if (pdev->has_secondary_link) {
> link = pdev->link_state;
> down_read(_bus_sem);
> mutex_lock(_lock);
>
> list_del(>sibling);
> list_del(>link);
>
> /* Clock PM is for endpoint device */
> free_link_state(link);
> mutex_unlock(_lock);
> up_read(_bus_sem);
> return;
> }

Right, but this "pdev->has_secondary_link" check is in your new code
and doesn't show up until patch [5/5].

As of *this* patch [3/5], the link is removed when the endpoint is
removed, so we could hit the use-after-free.

Granted, we'll only be susceptible to this while bisecting because
normally all the patches will be applied.  But I think this patch will
make more sense and be easier to review if it makes the link_state
lifetime match the Port's lifetime.

Bjorn

Re: [PATCH V8 3/5] PCI/ASPM: add init hook to device_add

[Bjorn Helgaas]

On Thu, Apr 13, 2017 at 8:19 PM, Sinan Kaya  wrote:
> On 4/13/2017 5:02 PM, Bjorn Helgaas wrote:
>> I do see that you change the deallocation in patch [5/5], but I think
>> the deallocation change should be in the same patch as the allocation
>> change.  Otherwise I think we have a use-after-free problem in this
>> sequence:
>
> Sure, I'll reorder. As you can see here, link will be only removed if
> root port is being removed.
>
> Without this, we'll hit the use after free issue you mentioned.
>
> if (pdev->has_secondary_link) {
> link = pdev->link_state;
> down_read(_bus_sem);
> mutex_lock(_lock);
>
> list_del(>sibling);
> list_del(>link);
>
> /* Clock PM is for endpoint device */
> free_link_state(link);
> mutex_unlock(_lock);
> up_read(_bus_sem);
> return;
> }

Right, but this "pdev->has_secondary_link" check is in your new code
and doesn't show up until patch [5/5].

As of *this* patch [3/5], the link is removed when the endpoint is
removed, so we could hit the use-after-free.

Granted, we'll only be susceptible to this while bisecting because
normally all the patches will be applied.  But I think this patch will
make more sense and be easier to review if it makes the link_state
lifetime match the Port's lifetime.

Bjorn

Re: [PATCH V8 3/5] PCI/ASPM: add init hook to device_add

[Sinan Kaya]

On 4/13/2017 5:02 PM, Bjorn Helgaas wrote:
> I do see that you change the deallocation in patch [5/5], but I think
> the deallocation change should be in the same patch as the allocation
> change.  Otherwise I think we have a use-after-free problem in this
> sequence:

Sure, I'll reorder. As you can see here, link will be only removed if
root port is being removed.

Without this, we'll hit the use after free issue you mentioned.

if (pdev->has_secondary_link) {
link = pdev->link_state;
down_read(_bus_sem);
mutex_lock(_lock);

list_del(>sibling);
list_del(>link);

/* Clock PM is for endpoint device */
free_link_state(link);
mutex_unlock(_lock);
up_read(_bus_sem);
return;
}  

-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux 
Foundation Collaborative Project.

Re: [PATCH V8 3/5] PCI/ASPM: add init hook to device_add

[Sinan Kaya]

On 4/13/2017 5:02 PM, Bjorn Helgaas wrote:
> I do see that you change the deallocation in patch [5/5], but I think
> the deallocation change should be in the same patch as the allocation
> change.  Otherwise I think we have a use-after-free problem in this
> sequence:

Sure, I'll reorder. As you can see here, link will be only removed if
root port is being removed.

Without this, we'll hit the use after free issue you mentioned.

if (pdev->has_secondary_link) {
link = pdev->link_state;
down_read(_bus_sem);
mutex_lock(_lock);

list_del(>sibling);
list_del(>link);

/* Clock PM is for endpoint device */
free_link_state(link);
mutex_unlock(_lock);
up_read(_bus_sem);
return;
}  

-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux 
Foundation Collaborative Project.

Re: [PATCH V8 3/5] PCI/ASPM: add init hook to device_add

[Bjorn Helgaas]

On Thu, Apr 13, 2017 at 03:48:00PM -0500, Bjorn Helgaas wrote:
> Hi Sinan,
> 
> On Sat, Apr 08, 2017 at 12:55:49AM -0400, Sinan Kaya wrote:
> > For bridges, have pcie_aspm_init_link_state() allocate a link_state,
> > regardless of whether it currently has any children.
> > 
> > pcie_aspm_init_link_state(): Called for bridges (upstream end of
> > link) after all children have been enumerated.  No longer needs to
> > check aspm_support_enabled or pdev->has_secondary_link or the VIA
> > quirk: pci_aspm_init() already checked that stuff, so we only need
> > to check pdev->link_state here.
> > 
> > Now that we are calling alloc_pcie_link_state() from pci_aspm_init()
> > , get rid of pci_function_0 function and detect downstream link in
> > pci_aspm_init_upstream() instead when the function number is 0.
> > 
> > Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=194895
> > Signed-off-by: Sinan Kaya 
> > ---
> >  drivers/pci/pcie/aspm.c | 72 
> > -
> >  1 file changed, 36 insertions(+), 36 deletions(-)
> > 
> > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> > index a80d64b..e33f84b 100644
> > --- a/drivers/pci/pcie/aspm.c
> > +++ b/drivers/pci/pcie/aspm.c
> > @@ -422,20 +422,6 @@ static void pcie_aspm_check_latency(struct pci_dev 
> > *endpoint)
> > }
> >  }
> >  
> > -/*
> > - * The L1 PM substate capability is only implemented in function 0 in a
> > - * multi function device.
> > - */
> > -static struct pci_dev *pci_function_0(struct pci_bus *linkbus)
> > -{
> > -   struct pci_dev *child;
> > -
> > -   list_for_each_entry(child, >devices, bus_list)
> > -   if (PCI_FUNC(child->devfn) == 0)
> > -   return child;
> > -   return NULL;
> > -}
> > -
> >  /* Calculate L1.2 PM substate timing parameters */
> >  static void aspm_calc_l1ss_info(struct pcie_link_state *link,
> > struct aspm_register_info *upreg,
> > @@ -798,7 +784,6 @@ static struct pcie_link_state 
> > *alloc_pcie_link_state(struct pci_dev *pdev)
> > INIT_LIST_HEAD(>children);
> > INIT_LIST_HEAD(>link);
> > link->pdev = pdev;
> > -   link->downstream = pci_function_0(pdev->subordinate);
> >  
> > /*
> >  * Root Ports and PCI/PCI-X to PCIe Bridges are roots of PCIe
> > @@ -828,11 +813,33 @@ static struct pcie_link_state 
> > *alloc_pcie_link_state(struct pci_dev *pdev)
> >  
> >  static int pci_aspm_init_downstream(struct pci_dev *pdev)
> >  {
> > +   struct pcie_link_state *link;
> > +   struct pci_dev *parent;
> > +
> > +   /*
> > +* The L1 PM substate capability is only implemented in function 0 in a
> > +* multi function device.
> > +*/
> > +   if (PCI_FUNC(pdev->devfn) != 0)
> > +   return -EINVAL;
> > +
> > +   parent = pdev->bus->self;
> > +   if (!parent)
> > +   return -EINVAL;
> > +
> > +   link = parent->link_state;
> > +   link->downstream = pdev;
> > return 0;
> >  }
> >  
> >  static int pci_aspm_init_upstream(struct pci_dev *pdev)
> >  {
> > +   struct pcie_link_state *link;
> > +
> > +   link = alloc_pcie_link_state(pdev);
> > +   if (!link)
> > +   return -ENOMEM;
> 
> This allocates the link_state when we enumerate a Downstream Port
> instead of when we enumerate a child device.  We want the link_state
> lifetime to match that of the Downstream Port, so this seems good.
> 
> But we shouldn't at the same time change the link_state deallocation
> so it happens when the Downstream Port is removed, not when the child
> device is removed?

I do see that you change the deallocation in patch [5/5], but I think
the deallocation change should be in the same patch as the allocation
change.  Otherwise I think we have a use-after-free problem in this
sequence:

  # initial enumeration
  pci_device_add(downstream_port)
pci_aspm_init(downstream_port)
  alloc_pcie_link_state
  pci_device_add(endpoint)
pci_aspm_init(endpoint)

  # hot-remove endpoint
  pci_stop_dev(endpoint)
pcie_aspm_exit_link_state(endpoint)
  link = parent->link_state
  free_link_state(link)

  # hot-add endpoint
  pci_aspm_init(endpoint)
link = parent->link_state <--- use after free

Re: [PATCH V8 3/5] PCI/ASPM: add init hook to device_add

[Bjorn Helgaas]

On Thu, Apr 13, 2017 at 03:48:00PM -0500, Bjorn Helgaas wrote:
> Hi Sinan,
> 
> On Sat, Apr 08, 2017 at 12:55:49AM -0400, Sinan Kaya wrote:
> > For bridges, have pcie_aspm_init_link_state() allocate a link_state,
> > regardless of whether it currently has any children.
> > 
> > pcie_aspm_init_link_state(): Called for bridges (upstream end of
> > link) after all children have been enumerated.  No longer needs to
> > check aspm_support_enabled or pdev->has_secondary_link or the VIA
> > quirk: pci_aspm_init() already checked that stuff, so we only need
> > to check pdev->link_state here.
> > 
> > Now that we are calling alloc_pcie_link_state() from pci_aspm_init()
> > , get rid of pci_function_0 function and detect downstream link in
> > pci_aspm_init_upstream() instead when the function number is 0.
> > 
> > Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=194895
> > Signed-off-by: Sinan Kaya 
> > ---
> >  drivers/pci/pcie/aspm.c | 72 
> > -
> >  1 file changed, 36 insertions(+), 36 deletions(-)
> > 
> > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> > index a80d64b..e33f84b 100644
> > --- a/drivers/pci/pcie/aspm.c
> > +++ b/drivers/pci/pcie/aspm.c
> > @@ -422,20 +422,6 @@ static void pcie_aspm_check_latency(struct pci_dev 
> > *endpoint)
> > }
> >  }
> >  
> > -/*
> > - * The L1 PM substate capability is only implemented in function 0 in a
> > - * multi function device.
> > - */
> > -static struct pci_dev *pci_function_0(struct pci_bus *linkbus)
> > -{
> > -   struct pci_dev *child;
> > -
> > -   list_for_each_entry(child, >devices, bus_list)
> > -   if (PCI_FUNC(child->devfn) == 0)
> > -   return child;
> > -   return NULL;
> > -}
> > -
> >  /* Calculate L1.2 PM substate timing parameters */
> >  static void aspm_calc_l1ss_info(struct pcie_link_state *link,
> > struct aspm_register_info *upreg,
> > @@ -798,7 +784,6 @@ static struct pcie_link_state 
> > *alloc_pcie_link_state(struct pci_dev *pdev)
> > INIT_LIST_HEAD(>children);
> > INIT_LIST_HEAD(>link);
> > link->pdev = pdev;
> > -   link->downstream = pci_function_0(pdev->subordinate);
> >  
> > /*
> >  * Root Ports and PCI/PCI-X to PCIe Bridges are roots of PCIe
> > @@ -828,11 +813,33 @@ static struct pcie_link_state 
> > *alloc_pcie_link_state(struct pci_dev *pdev)
> >  
> >  static int pci_aspm_init_downstream(struct pci_dev *pdev)
> >  {
> > +   struct pcie_link_state *link;
> > +   struct pci_dev *parent;
> > +
> > +   /*
> > +* The L1 PM substate capability is only implemented in function 0 in a
> > +* multi function device.
> > +*/
> > +   if (PCI_FUNC(pdev->devfn) != 0)
> > +   return -EINVAL;
> > +
> > +   parent = pdev->bus->self;
> > +   if (!parent)
> > +   return -EINVAL;
> > +
> > +   link = parent->link_state;
> > +   link->downstream = pdev;
> > return 0;
> >  }
> >  
> >  static int pci_aspm_init_upstream(struct pci_dev *pdev)
> >  {
> > +   struct pcie_link_state *link;
> > +
> > +   link = alloc_pcie_link_state(pdev);
> > +   if (!link)
> > +   return -ENOMEM;
> 
> This allocates the link_state when we enumerate a Downstream Port
> instead of when we enumerate a child device.  We want the link_state
> lifetime to match that of the Downstream Port, so this seems good.
> 
> But we shouldn't at the same time change the link_state deallocation
> so it happens when the Downstream Port is removed, not when the child
> device is removed?

I do see that you change the deallocation in patch [5/5], but I think
the deallocation change should be in the same patch as the allocation
change.  Otherwise I think we have a use-after-free problem in this
sequence:

  # initial enumeration
  pci_device_add(downstream_port)
pci_aspm_init(downstream_port)
  alloc_pcie_link_state
  pci_device_add(endpoint)
pci_aspm_init(endpoint)

  # hot-remove endpoint
  pci_stop_dev(endpoint)
pcie_aspm_exit_link_state(endpoint)
  link = parent->link_state
  free_link_state(link)

  # hot-add endpoint
  pci_aspm_init(endpoint)
link = parent->link_state <--- use after free

Re: [PATCH V8 3/5] PCI/ASPM: add init hook to device_add

[Bjorn Helgaas]

Hi Sinan,

On Sat, Apr 08, 2017 at 12:55:49AM -0400, Sinan Kaya wrote:
> For bridges, have pcie_aspm_init_link_state() allocate a link_state,
> regardless of whether it currently has any children.
> 
> pcie_aspm_init_link_state(): Called for bridges (upstream end of
> link) after all children have been enumerated.  No longer needs to
> check aspm_support_enabled or pdev->has_secondary_link or the VIA
> quirk: pci_aspm_init() already checked that stuff, so we only need
> to check pdev->link_state here.
> 
> Now that we are calling alloc_pcie_link_state() from pci_aspm_init()
> , get rid of pci_function_0 function and detect downstream link in
> pci_aspm_init_upstream() instead when the function number is 0.
> 
> Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=194895
> Signed-off-by: Sinan Kaya 
> ---
>  drivers/pci/pcie/aspm.c | 72 
> -
>  1 file changed, 36 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index a80d64b..e33f84b 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -422,20 +422,6 @@ static void pcie_aspm_check_latency(struct pci_dev 
> *endpoint)
>   }
>  }
>  
> -/*
> - * The L1 PM substate capability is only implemented in function 0 in a
> - * multi function device.
> - */
> -static struct pci_dev *pci_function_0(struct pci_bus *linkbus)
> -{
> - struct pci_dev *child;
> -
> - list_for_each_entry(child, >devices, bus_list)
> - if (PCI_FUNC(child->devfn) == 0)
> - return child;
> - return NULL;
> -}
> -
>  /* Calculate L1.2 PM substate timing parameters */
>  static void aspm_calc_l1ss_info(struct pcie_link_state *link,
>   struct aspm_register_info *upreg,
> @@ -798,7 +784,6 @@ static struct pcie_link_state 
> *alloc_pcie_link_state(struct pci_dev *pdev)
>   INIT_LIST_HEAD(>children);
>   INIT_LIST_HEAD(>link);
>   link->pdev = pdev;
> - link->downstream = pci_function_0(pdev->subordinate);
>  
>   /*
>* Root Ports and PCI/PCI-X to PCIe Bridges are roots of PCIe
> @@ -828,11 +813,33 @@ static struct pcie_link_state 
> *alloc_pcie_link_state(struct pci_dev *pdev)
>  
>  static int pci_aspm_init_downstream(struct pci_dev *pdev)
>  {
> + struct pcie_link_state *link;
> + struct pci_dev *parent;
> +
> + /*
> +  * The L1 PM substate capability is only implemented in function 0 in a
> +  * multi function device.
> +  */
> + if (PCI_FUNC(pdev->devfn) != 0)
> + return -EINVAL;
> +
> + parent = pdev->bus->self;
> + if (!parent)
> + return -EINVAL;
> +
> + link = parent->link_state;
> + link->downstream = pdev;
>   return 0;
>  }
>  
>  static int pci_aspm_init_upstream(struct pci_dev *pdev)
>  {
> + struct pcie_link_state *link;
> +
> + link = alloc_pcie_link_state(pdev);
> + if (!link)
> + return -ENOMEM;

This allocates the link_state when we enumerate a Downstream Port
instead of when we enumerate a child device.  We want the link_state
lifetime to match that of the Downstream Port, so this seems good.

But we shouldn't at the same time change the link_state deallocation
so it happens when the Downstream Port is removed, not when the child
device is removed?

>   return 0;
>  }
>  
> @@ -843,6 +850,17 @@ static int pci_aspm_init_upstream(struct pci_dev *pdev)
>   */
>  int pci_aspm_init(struct pci_dev *pdev)
>  {
> + if (!aspm_support_enabled)
> + return 0;
> +
> + if (!pci_is_pcie(pdev))
> + return -EINVAL;
> +
> + /* VIA has a strange chipset, root port is under a bridge */
> + if (pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT &&
> + pdev->bus->self)
> + return 0;
> +
>   if (!pdev->has_secondary_link)
>   return pci_aspm_init_downstream(pdev);
>  
> @@ -859,33 +877,16 @@ void pcie_aspm_init_link_state(struct pci_dev *pdev)
>   struct pcie_link_state *link;
>   int blacklist = !!pcie_aspm_sanity_check(pdev);
>  
> - if (!aspm_support_enabled)
> - return;
> -
> - if (pdev->link_state)
> - return;
> -
> - /*
> -  * We allocate pcie_link_state for the component on the upstream
> -  * end of a Link, so there's nothing to do unless this device has a
> -  * Link on its secondary side.
> -  */
> - if (!pdev->has_secondary_link)
> - return;
> -
> - /* VIA has a strange chipset, root port is under a bridge */
> - if (pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT &&
> - pdev->bus->self)
> + if (!pdev->link_state)
>   return;
>  
> + link = pdev->link_state;
>   down_read(_bus_sem);
>   if (list_empty(>subordinate->devices))
>   goto out;
>  
>   mutex_lock(_lock);
> - link = alloc_pcie_link_state(pdev);
> - if (!link)
> - goto 

Re: [PATCH V8 3/5] PCI/ASPM: add init hook to device_add

[Bjorn Helgaas]

Hi Sinan,

On Sat, Apr 08, 2017 at 12:55:49AM -0400, Sinan Kaya wrote:
> For bridges, have pcie_aspm_init_link_state() allocate a link_state,
> regardless of whether it currently has any children.
> 
> pcie_aspm_init_link_state(): Called for bridges (upstream end of
> link) after all children have been enumerated.  No longer needs to
> check aspm_support_enabled or pdev->has_secondary_link or the VIA
> quirk: pci_aspm_init() already checked that stuff, so we only need
> to check pdev->link_state here.
> 
> Now that we are calling alloc_pcie_link_state() from pci_aspm_init()
> , get rid of pci_function_0 function and detect downstream link in
> pci_aspm_init_upstream() instead when the function number is 0.
> 
> Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=194895
> Signed-off-by: Sinan Kaya 
> ---
>  drivers/pci/pcie/aspm.c | 72 
> -
>  1 file changed, 36 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index a80d64b..e33f84b 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -422,20 +422,6 @@ static void pcie_aspm_check_latency(struct pci_dev 
> *endpoint)
>   }
>  }
>  
> -/*
> - * The L1 PM substate capability is only implemented in function 0 in a
> - * multi function device.
> - */
> -static struct pci_dev *pci_function_0(struct pci_bus *linkbus)
> -{
> - struct pci_dev *child;
> -
> - list_for_each_entry(child, >devices, bus_list)
> - if (PCI_FUNC(child->devfn) == 0)
> - return child;
> - return NULL;
> -}
> -
>  /* Calculate L1.2 PM substate timing parameters */
>  static void aspm_calc_l1ss_info(struct pcie_link_state *link,
>   struct aspm_register_info *upreg,
> @@ -798,7 +784,6 @@ static struct pcie_link_state 
> *alloc_pcie_link_state(struct pci_dev *pdev)
>   INIT_LIST_HEAD(>children);
>   INIT_LIST_HEAD(>link);
>   link->pdev = pdev;
> - link->downstream = pci_function_0(pdev->subordinate);
>  
>   /*
>* Root Ports and PCI/PCI-X to PCIe Bridges are roots of PCIe
> @@ -828,11 +813,33 @@ static struct pcie_link_state 
> *alloc_pcie_link_state(struct pci_dev *pdev)
>  
>  static int pci_aspm_init_downstream(struct pci_dev *pdev)
>  {
> + struct pcie_link_state *link;
> + struct pci_dev *parent;
> +
> + /*
> +  * The L1 PM substate capability is only implemented in function 0 in a
> +  * multi function device.
> +  */
> + if (PCI_FUNC(pdev->devfn) != 0)
> + return -EINVAL;
> +
> + parent = pdev->bus->self;
> + if (!parent)
> + return -EINVAL;
> +
> + link = parent->link_state;
> + link->downstream = pdev;
>   return 0;
>  }
>  
>  static int pci_aspm_init_upstream(struct pci_dev *pdev)
>  {
> + struct pcie_link_state *link;
> +
> + link = alloc_pcie_link_state(pdev);
> + if (!link)
> + return -ENOMEM;

This allocates the link_state when we enumerate a Downstream Port
instead of when we enumerate a child device.  We want the link_state
lifetime to match that of the Downstream Port, so this seems good.

But we shouldn't at the same time change the link_state deallocation
so it happens when the Downstream Port is removed, not when the child
device is removed?

>   return 0;
>  }
>  
> @@ -843,6 +850,17 @@ static int pci_aspm_init_upstream(struct pci_dev *pdev)
>   */
>  int pci_aspm_init(struct pci_dev *pdev)
>  {
> + if (!aspm_support_enabled)
> + return 0;
> +
> + if (!pci_is_pcie(pdev))
> + return -EINVAL;
> +
> + /* VIA has a strange chipset, root port is under a bridge */
> + if (pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT &&
> + pdev->bus->self)
> + return 0;
> +
>   if (!pdev->has_secondary_link)
>   return pci_aspm_init_downstream(pdev);
>  
> @@ -859,33 +877,16 @@ void pcie_aspm_init_link_state(struct pci_dev *pdev)
>   struct pcie_link_state *link;
>   int blacklist = !!pcie_aspm_sanity_check(pdev);
>  
> - if (!aspm_support_enabled)
> - return;
> -
> - if (pdev->link_state)
> - return;
> -
> - /*
> -  * We allocate pcie_link_state for the component on the upstream
> -  * end of a Link, so there's nothing to do unless this device has a
> -  * Link on its secondary side.
> -  */
> - if (!pdev->has_secondary_link)
> - return;
> -
> - /* VIA has a strange chipset, root port is under a bridge */
> - if (pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT &&
> - pdev->bus->self)
> + if (!pdev->link_state)
>   return;
>  
> + link = pdev->link_state;
>   down_read(_bus_sem);
>   if (list_empty(>subordinate->devices))
>   goto out;
>  
>   mutex_lock(_lock);
> - link = alloc_pcie_link_state(pdev);
> - if (!link)
> - goto unlock;
> +
>   /*

[PATCH V8 3/5] PCI/ASPM: add init hook to device_add

[Sinan Kaya]

For bridges, have pcie_aspm_init_link_state() allocate a link_state,
regardless of whether it currently has any children.

pcie_aspm_init_link_state(): Called for bridges (upstream end of
link) after all children have been enumerated.  No longer needs to
check aspm_support_enabled or pdev->has_secondary_link or the VIA
quirk: pci_aspm_init() already checked that stuff, so we only need
to check pdev->link_state here.

Now that we are calling alloc_pcie_link_state() from pci_aspm_init()
, get rid of pci_function_0 function and detect downstream link in
pci_aspm_init_upstream() instead when the function number is 0.

Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=194895
Signed-off-by: Sinan Kaya 
---
 drivers/pci/pcie/aspm.c | 72 -
 1 file changed, 36 insertions(+), 36 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index a80d64b..e33f84b 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -422,20 +422,6 @@ static void pcie_aspm_check_latency(struct pci_dev 
*endpoint)
}
 }
 
-/*
- * The L1 PM substate capability is only implemented in function 0 in a
- * multi function device.
- */
-static struct pci_dev *pci_function_0(struct pci_bus *linkbus)
-{
-   struct pci_dev *child;
-
-   list_for_each_entry(child, >devices, bus_list)
-   if (PCI_FUNC(child->devfn) == 0)
-   return child;
-   return NULL;
-}
-
 /* Calculate L1.2 PM substate timing parameters */
 static void aspm_calc_l1ss_info(struct pcie_link_state *link,
struct aspm_register_info *upreg,
@@ -798,7 +784,6 @@ static struct pcie_link_state *alloc_pcie_link_state(struct 
pci_dev *pdev)
INIT_LIST_HEAD(>children);
INIT_LIST_HEAD(>link);
link->pdev = pdev;
-   link->downstream = pci_function_0(pdev->subordinate);
 
/*
 * Root Ports and PCI/PCI-X to PCIe Bridges are roots of PCIe
@@ -828,11 +813,33 @@ static struct pcie_link_state 
*alloc_pcie_link_state(struct pci_dev *pdev)
 
 static int pci_aspm_init_downstream(struct pci_dev *pdev)
 {
+   struct pcie_link_state *link;
+   struct pci_dev *parent;
+
+   /*
+* The L1 PM substate capability is only implemented in function 0 in a
+* multi function device.
+*/
+   if (PCI_FUNC(pdev->devfn) != 0)
+   return -EINVAL;
+
+   parent = pdev->bus->self;
+   if (!parent)
+   return -EINVAL;
+
+   link = parent->link_state;
+   link->downstream = pdev;
return 0;
 }
 
 static int pci_aspm_init_upstream(struct pci_dev *pdev)
 {
+   struct pcie_link_state *link;
+
+   link = alloc_pcie_link_state(pdev);
+   if (!link)
+   return -ENOMEM;
+
return 0;
 }
 
@@ -843,6 +850,17 @@ static int pci_aspm_init_upstream(struct pci_dev *pdev)
  */
 int pci_aspm_init(struct pci_dev *pdev)
 {
+   if (!aspm_support_enabled)
+   return 0;
+
+   if (!pci_is_pcie(pdev))
+   return -EINVAL;
+
+   /* VIA has a strange chipset, root port is under a bridge */
+   if (pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT &&
+   pdev->bus->self)
+   return 0;
+
if (!pdev->has_secondary_link)
return pci_aspm_init_downstream(pdev);
 
@@ -859,33 +877,16 @@ void pcie_aspm_init_link_state(struct pci_dev *pdev)
struct pcie_link_state *link;
int blacklist = !!pcie_aspm_sanity_check(pdev);
 
-   if (!aspm_support_enabled)
-   return;
-
-   if (pdev->link_state)
-   return;
-
-   /*
-* We allocate pcie_link_state for the component on the upstream
-* end of a Link, so there's nothing to do unless this device has a
-* Link on its secondary side.
-*/
-   if (!pdev->has_secondary_link)
-   return;
-
-   /* VIA has a strange chipset, root port is under a bridge */
-   if (pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT &&
-   pdev->bus->self)
+   if (!pdev->link_state)
return;
 
+   link = pdev->link_state;
down_read(_bus_sem);
if (list_empty(>subordinate->devices))
goto out;
 
mutex_lock(_lock);
-   link = alloc_pcie_link_state(pdev);
-   if (!link)
-   goto unlock;
+
/*
 * Setup initial ASPM state. Note that we need to configure
 * upstream links also because capable state of them can be
@@ -910,7 +911,6 @@ void pcie_aspm_init_link_state(struct pci_dev *pdev)
pcie_set_clkpm(link, policy_to_clkpm_state(link));
}
 
-unlock:
mutex_unlock(_lock);
 out:
up_read(_bus_sem);
-- 
1.9.1

[PATCH V8 3/5] PCI/ASPM: add init hook to device_add

[Sinan Kaya]

For bridges, have pcie_aspm_init_link_state() allocate a link_state,
regardless of whether it currently has any children.

pcie_aspm_init_link_state(): Called for bridges (upstream end of
link) after all children have been enumerated.  No longer needs to
check aspm_support_enabled or pdev->has_secondary_link or the VIA
quirk: pci_aspm_init() already checked that stuff, so we only need
to check pdev->link_state here.

Now that we are calling alloc_pcie_link_state() from pci_aspm_init()
, get rid of pci_function_0 function and detect downstream link in
pci_aspm_init_upstream() instead when the function number is 0.

Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=194895
Signed-off-by: Sinan Kaya 
---
 drivers/pci/pcie/aspm.c | 72 -
 1 file changed, 36 insertions(+), 36 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index a80d64b..e33f84b 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -422,20 +422,6 @@ static void pcie_aspm_check_latency(struct pci_dev 
*endpoint)
}
 }
 
-/*
- * The L1 PM substate capability is only implemented in function 0 in a
- * multi function device.
- */
-static struct pci_dev *pci_function_0(struct pci_bus *linkbus)
-{
-   struct pci_dev *child;
-
-   list_for_each_entry(child, >devices, bus_list)
-   if (PCI_FUNC(child->devfn) == 0)
-   return child;
-   return NULL;
-}
-
 /* Calculate L1.2 PM substate timing parameters */
 static void aspm_calc_l1ss_info(struct pcie_link_state *link,
struct aspm_register_info *upreg,
@@ -798,7 +784,6 @@ static struct pcie_link_state *alloc_pcie_link_state(struct 
pci_dev *pdev)
INIT_LIST_HEAD(>children);
INIT_LIST_HEAD(>link);
link->pdev = pdev;
-   link->downstream = pci_function_0(pdev->subordinate);
 
/*
 * Root Ports and PCI/PCI-X to PCIe Bridges are roots of PCIe
@@ -828,11 +813,33 @@ static struct pcie_link_state 
*alloc_pcie_link_state(struct pci_dev *pdev)
 
 static int pci_aspm_init_downstream(struct pci_dev *pdev)
 {
+   struct pcie_link_state *link;
+   struct pci_dev *parent;
+
+   /*
+* The L1 PM substate capability is only implemented in function 0 in a
+* multi function device.
+*/
+   if (PCI_FUNC(pdev->devfn) != 0)
+   return -EINVAL;
+
+   parent = pdev->bus->self;
+   if (!parent)
+   return -EINVAL;
+
+   link = parent->link_state;
+   link->downstream = pdev;
return 0;
 }
 
 static int pci_aspm_init_upstream(struct pci_dev *pdev)
 {
+   struct pcie_link_state *link;
+
+   link = alloc_pcie_link_state(pdev);
+   if (!link)
+   return -ENOMEM;
+
return 0;
 }
 
@@ -843,6 +850,17 @@ static int pci_aspm_init_upstream(struct pci_dev *pdev)
  */
 int pci_aspm_init(struct pci_dev *pdev)
 {
+   if (!aspm_support_enabled)
+   return 0;
+
+   if (!pci_is_pcie(pdev))
+   return -EINVAL;
+
+   /* VIA has a strange chipset, root port is under a bridge */
+   if (pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT &&
+   pdev->bus->self)
+   return 0;
+
if (!pdev->has_secondary_link)
return pci_aspm_init_downstream(pdev);
 
@@ -859,33 +877,16 @@ void pcie_aspm_init_link_state(struct pci_dev *pdev)
struct pcie_link_state *link;
int blacklist = !!pcie_aspm_sanity_check(pdev);
 
-   if (!aspm_support_enabled)
-   return;
-
-   if (pdev->link_state)
-   return;
-
-   /*
-* We allocate pcie_link_state for the component on the upstream
-* end of a Link, so there's nothing to do unless this device has a
-* Link on its secondary side.
-*/
-   if (!pdev->has_secondary_link)
-   return;
-
-   /* VIA has a strange chipset, root port is under a bridge */
-   if (pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT &&
-   pdev->bus->self)
+   if (!pdev->link_state)
return;
 
+   link = pdev->link_state;
down_read(_bus_sem);
if (list_empty(>subordinate->devices))
goto out;
 
mutex_lock(_lock);
-   link = alloc_pcie_link_state(pdev);
-   if (!link)
-   goto unlock;
+
/*
 * Setup initial ASPM state. Note that we need to configure
 * upstream links also because capable state of them can be
@@ -910,7 +911,6 @@ void pcie_aspm_init_link_state(struct pci_dev *pdev)
pcie_set_clkpm(link, policy_to_clkpm_state(link));
}
 
-unlock:
mutex_unlock(_lock);
 out:
up_read(_bus_sem);
-- 
1.9.1