diff mbox series

[V4] PCI/ASPM: Update saved buffers with latest ASPM

Message ID 20240222174436.3565146-1-vidyas@nvidia.com (mailing list archive)
State Accepted
Delegated to: Bjorn Helgaas
Headers show
Series [V4] PCI/ASPM: Update saved buffers with latest ASPM | expand

Commit Message

Vidya Sagar Feb. 22, 2024, 5:44 p.m. UTC
Many PCIe device drivers save the configuration state of their respective
devices during probe and restore the same when their 'slot_reset' hook
is called through PCIe Error Recovery Handler.

If the system has a change in ASPM policy after the driver's probe is
called and before error event occurred, 'slot_reset' hook restores the
PCIe configuration state to what it was at the time of probe but not to
what it was just before the occurrence of the error event.
This effectively leads to a mismatch in the ASPM configuration between
the device and its upstream parent device.

Update the saved configuration state of the device with the latest info
whenever there is a change w.r.t ASPM policy.

Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
---
V4:
* Rebased on top of pci/aspm branch

V3:
* Addressed sathyanarayanan.kuppuswamy's review comments

V2:
* Rebased on top of the tree code
* Addressed Bjorn's review comments

 drivers/pci/pcie/aspm.c | 28 ++++++++++++++++++++++++++--
 3 files changed, 28 insertions(+), 4 deletions(-)

Comments

Kuppuswamy Sathyanarayanan Feb. 22, 2024, 6:20 p.m. UTC | #1
On 2/22/24 9:44 AM, Vidya Sagar wrote:
> Many PCIe device drivers save the configuration state of their respective
> devices during probe and restore the same when their 'slot_reset' hook
> is called through PCIe Error Recovery Handler.
>
> If the system has a change in ASPM policy after the driver's probe is
> called and before error event occurred, 'slot_reset' hook restores the
> PCIe configuration state to what it was at the time of probe but not to
> what it was just before the occurrence of the error event.
> This effectively leads to a mismatch in the ASPM configuration between
> the device and its upstream parent device.
>
> Update the saved configuration state of the device with the latest info
> whenever there is a change w.r.t ASPM policy.
>
> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
> ---
> V4:
> * Rebased on top of pci/aspm branch
>
> V3:
> * Addressed sathyanarayanan.kuppuswamy's review comments
>
> V2:
> * Rebased on top of the tree code
> * Addressed Bjorn's review comments
>
>  drivers/pci/pcie/aspm.c | 28 ++++++++++++++++++++++++++--
>  3 files changed, 28 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index cfc5b84dc9c9..3db606ba9344 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1648,7 +1648,7 @@ static int pci_save_pcie_state(struct pci_dev *dev)
>  	pcie_capability_read_word(dev, PCI_EXP_LNKCTL2, &cap[i++]);
>  	pcie_capability_read_word(dev, PCI_EXP_SLTCTL2, &cap[i++]);
>  
> -	pci_save_aspm_state(dev);
> +	pci_save_aspm_l1ss_state(dev);
>  	pci_save_ltr_state(dev);
>  
>  	return 0;
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index b217e74966eb..9fe78eb8b07d 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -95,7 +95,7 @@ void pci_msix_init(struct pci_dev *dev);
>  bool pci_bridge_d3_possible(struct pci_dev *dev);
>  void pci_bridge_d3_update(struct pci_dev *dev);
>  void pci_aspm_get_l1ss(struct pci_dev *pdev);
> -void pci_save_aspm_state(struct pci_dev *pdev);
> +void pci_save_aspm_l1ss_state(struct pci_dev *pdev);

is this rename a review request? It is not clear from the commit log
why you are doing it?

>  void pci_restore_aspm_state(struct pci_dev *pdev);
>  void pci_save_ltr_state(struct pci_dev *dev);
>  void pci_restore_ltr_state(struct pci_dev *dev);
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 7f1d674ff171..a62648dd52bc 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -24,13 +24,29 @@
>  
>  #include "../pci.h"
>  
> +static void pci_save_aspm_state(struct pci_dev *dev)
> +{
> +	struct pci_cap_saved_state *save_state;
> +	u16 *cap;
> +
> +	if (!pci_is_pcie(dev))
> +		return;
> +
> +	save_state = pci_find_saved_cap(dev, PCI_CAP_ID_EXP);
> +	if (!save_state)
> +		return;
> +
> +	cap = (u16 *)&save_state->cap.data[0];
> +	pcie_capability_read_word(dev, PCI_EXP_LNKCTL, &cap[1]);
> +}
> +
>  void pci_aspm_get_l1ss(struct pci_dev *pdev)
>  {
>  	/* Read L1 PM substate capabilities */
>  	pdev->l1ss = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_L1SS);
>  }
>  
> -void pci_save_aspm_state(struct pci_dev *pdev)
> +void pci_save_aspm_l1ss_state(struct pci_dev *pdev)
>  {
>  	struct pci_cap_saved_state *save_state;
>  	u16 l1ss = pdev->l1ss;
> @@ -309,10 +325,12 @@ static void pcie_set_clkpm_nocheck(struct pcie_link_state *link, int enable)
>  	struct pci_bus *linkbus = link->pdev->subordinate;
>  	u32 val = enable ? PCI_EXP_LNKCTL_CLKREQ_EN : 0;
>  
> -	list_for_each_entry(child, &linkbus->devices, bus_list)
> +	list_for_each_entry(child, &linkbus->devices, bus_list) {
>  		pcie_capability_clear_and_set_word(child, PCI_EXP_LNKCTL,
>  						   PCI_EXP_LNKCTL_CLKREQ_EN,
>  						   val);
> +		pci_save_aspm_state(child);
> +	}
>  	link->clkpm_enabled = !!enable;
>  }
>  
> @@ -931,6 +949,12 @@ static void pcie_config_aspm_link(struct pcie_link_state *link, u32 state)
>  		pcie_config_aspm_dev(parent, upstream);
>  
>  	link->aspm_enabled = state;
> +
> +	/* Update latest ASPM configuration in saved context */
> +	pci_save_aspm_state(link->downstream);
> +	pci_save_aspm_l1ss_state(link->downstream);
> +	pci_save_aspm_state(parent);
> +	pci_save_aspm_l1ss_state(parent);
>  }
>  
>  static void pcie_config_aspm_path(struct pcie_link_state *link)
Bjorn Helgaas Feb. 22, 2024, 6:54 p.m. UTC | #2
On Thu, Feb 22, 2024 at 10:20:06AM -0800, Kuppuswamy Sathyanarayanan wrote:
> On 2/22/24 9:44 AM, Vidya Sagar wrote:
> > Many PCIe device drivers save the configuration state of their respective
> > devices during probe and restore the same when their 'slot_reset' hook
> > is called through PCIe Error Recovery Handler.
> >
> > If the system has a change in ASPM policy after the driver's probe is
> > called and before error event occurred, 'slot_reset' hook restores the
> > PCIe configuration state to what it was at the time of probe but not to
> > what it was just before the occurrence of the error event.
> > This effectively leads to a mismatch in the ASPM configuration between
> > the device and its upstream parent device.
> >
> > Update the saved configuration state of the device with the latest info
> > whenever there is a change w.r.t ASPM policy.
> >
> > Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
> > ---
> > V4:
> > * Rebased on top of pci/aspm branch
> >
> > V3:
> > * Addressed sathyanarayanan.kuppuswamy's review comments
> >
> > V2:
> > * Rebased on top of the tree code
> > * Addressed Bjorn's review comments
> >
> >  drivers/pci/pcie/aspm.c | 28 ++++++++++++++++++++++++++--
> >  3 files changed, 28 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index cfc5b84dc9c9..3db606ba9344 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -1648,7 +1648,7 @@ static int pci_save_pcie_state(struct pci_dev *dev)
> >  	pcie_capability_read_word(dev, PCI_EXP_LNKCTL2, &cap[i++]);
> >  	pcie_capability_read_word(dev, PCI_EXP_SLTCTL2, &cap[i++]);
> >  
> > -	pci_save_aspm_state(dev);
> > +	pci_save_aspm_l1ss_state(dev);
> >  	pci_save_ltr_state(dev);
> >  
> >  	return 0;
> > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> > index b217e74966eb..9fe78eb8b07d 100644
> > --- a/drivers/pci/pci.h
> > +++ b/drivers/pci/pci.h
> > @@ -95,7 +95,7 @@ void pci_msix_init(struct pci_dev *dev);
> >  bool pci_bridge_d3_possible(struct pci_dev *dev);
> >  void pci_bridge_d3_update(struct pci_dev *dev);
> >  void pci_aspm_get_l1ss(struct pci_dev *pdev);
> > -void pci_save_aspm_state(struct pci_dev *pdev);
> > +void pci_save_aspm_l1ss_state(struct pci_dev *pdev);
> 
> is this rename a review request? It is not clear from the commit log
> why you are doing it?

David's changes already on pci/aspm added pci_save_aspm_state(), but
it actually only saves L1SS data, and Vidya needs to save the non-L1SS
data also.

I think I'm going to rework David's changes a little bit so this is
named pci_save_aspm_l1ss_state() from the beginning so we won't need
the rename here.

Bjorn
Kuppuswamy Sathyanarayanan Feb. 22, 2024, 7:54 p.m. UTC | #3
On 2/22/24 10:54 AM, Bjorn Helgaas wrote:
> On Thu, Feb 22, 2024 at 10:20:06AM -0800, Kuppuswamy Sathyanarayanan wrote:
>> On 2/22/24 9:44 AM, Vidya Sagar wrote:
>>> Many PCIe device drivers save the configuration state of their respective
>>> devices during probe and restore the same when their 'slot_reset' hook
>>> is called through PCIe Error Recovery Handler.
>>>
>>> If the system has a change in ASPM policy after the driver's probe is
>>> called and before error event occurred, 'slot_reset' hook restores the
>>> PCIe configuration state to what it was at the time of probe but not to
>>> what it was just before the occurrence of the error event.
>>> This effectively leads to a mismatch in the ASPM configuration between
>>> the device and its upstream parent device.
>>>
>>> Update the saved configuration state of the device with the latest info
>>> whenever there is a change w.r.t ASPM policy.
>>>
>>> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
>>> ---
>>> V4:
>>> * Rebased on top of pci/aspm branch
>>>
>>> V3:
>>> * Addressed sathyanarayanan.kuppuswamy's review comments
>>>
>>> V2:
>>> * Rebased on top of the tree code
>>> * Addressed Bjorn's review comments
>>>
>>>  drivers/pci/pcie/aspm.c | 28 ++++++++++++++++++++++++++--
>>>  3 files changed, 28 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>>> index cfc5b84dc9c9..3db606ba9344 100644
>>> --- a/drivers/pci/pci.c
>>> +++ b/drivers/pci/pci.c
>>> @@ -1648,7 +1648,7 @@ static int pci_save_pcie_state(struct pci_dev *dev)
>>>  	pcie_capability_read_word(dev, PCI_EXP_LNKCTL2, &cap[i++]);
>>>  	pcie_capability_read_word(dev, PCI_EXP_SLTCTL2, &cap[i++]);
>>>  
>>> -	pci_save_aspm_state(dev);
>>> +	pci_save_aspm_l1ss_state(dev);
>>>  	pci_save_ltr_state(dev);
>>>  
>>>  	return 0;
>>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>>> index b217e74966eb..9fe78eb8b07d 100644
>>> --- a/drivers/pci/pci.h
>>> +++ b/drivers/pci/pci.h
>>> @@ -95,7 +95,7 @@ void pci_msix_init(struct pci_dev *dev);
>>>  bool pci_bridge_d3_possible(struct pci_dev *dev);
>>>  void pci_bridge_d3_update(struct pci_dev *dev);
>>>  void pci_aspm_get_l1ss(struct pci_dev *pdev);
>>> -void pci_save_aspm_state(struct pci_dev *pdev);
>>> +void pci_save_aspm_l1ss_state(struct pci_dev *pdev);
>> is this rename a review request? It is not clear from the commit log
>> why you are doing it?
> David's changes already on pci/aspm added pci_save_aspm_state(), but
> it actually only saves L1SS data, and Vidya needs to save the non-L1SS
> data also.
>
> I think I'm going to rework David's changes a little bit so this is
> named pci_save_aspm_l1ss_state() from the beginning so we won't need
> the rename here.

Got it.

Change wise, this patch looks fine to me.

Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

>
> Bjorn
Vidya Sagar Feb. 22, 2024, 8:59 p.m. UTC | #4
On 22-02-2024 23:50, Kuppuswamy Sathyanarayanan wrote:
> External email: Use caution opening links or attachments
>
>
> On 2/22/24 9:44 AM, Vidya Sagar wrote:
>> Many PCIe device drivers save the configuration state of their respective
>> devices during probe and restore the same when their 'slot_reset' hook
>> is called through PCIe Error Recovery Handler.
>>
>> If the system has a change in ASPM policy after the driver's probe is
>> called and before error event occurred, 'slot_reset' hook restores the
>> PCIe configuration state to what it was at the time of probe but not to
>> what it was just before the occurrence of the error event.
>> This effectively leads to a mismatch in the ASPM configuration between
>> the device and its upstream parent device.
>>
>> Update the saved configuration state of the device with the latest info
>> whenever there is a change w.r.t ASPM policy.
>>
>> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
>> ---
>> V4:
>> * Rebased on top of pci/aspm branch
>>
>> V3:
>> * Addressed sathyanarayanan.kuppuswamy's review comments
>>
>> V2:
>> * Rebased on top of the tree code
>> * Addressed Bjorn's review comments
>>
>>   drivers/pci/pcie/aspm.c | 28 ++++++++++++++++++++++++++--
>>   3 files changed, 28 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index cfc5b84dc9c9..3db606ba9344 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -1648,7 +1648,7 @@ static int pci_save_pcie_state(struct pci_dev *dev)
>>        pcie_capability_read_word(dev, PCI_EXP_LNKCTL2, &cap[i++]);
>>        pcie_capability_read_word(dev, PCI_EXP_SLTCTL2, &cap[i++]);
>>
>> -     pci_save_aspm_state(dev);
>> +     pci_save_aspm_l1ss_state(dev);
>>        pci_save_ltr_state(dev);
>>
>>        return 0;
>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>> index b217e74966eb..9fe78eb8b07d 100644
>> --- a/drivers/pci/pci.h
>> +++ b/drivers/pci/pci.h
>> @@ -95,7 +95,7 @@ void pci_msix_init(struct pci_dev *dev);
>>   bool pci_bridge_d3_possible(struct pci_dev *dev);
>>   void pci_bridge_d3_update(struct pci_dev *dev);
>>   void pci_aspm_get_l1ss(struct pci_dev *pdev);
>> -void pci_save_aspm_state(struct pci_dev *pdev);
>> +void pci_save_aspm_l1ss_state(struct pci_dev *pdev);
> is this rename a review request? It is not clear from the commit log
> why you are doing it?
I rebased my changes on top of pci/aspm branch which got the support for
save/restore of ASPM L1-SubStates registers merged recently. As I see,
the name of the function that saves the L1SS registers is given as
pci_save_aspm_state() instead of pci_save_aspm_l1ss_state(). My original
change already adds a function to save the ASPM state (only L0s and L1)
info from the link control register and I called it pci_save_aspm_state().
I thought it makes sense to use pci_save_aspm_state() for saving the info
of ASPM-L0s/L1 related registers whereas use pci_save_aspm_l1ss_state() to
save the info of ASPM-L1SS.

Thanks,
Vidya Sagar
>
>>   void pci_restore_aspm_state(struct pci_dev *pdev);
>>   void pci_save_ltr_state(struct pci_dev *dev);
>>   void pci_restore_ltr_state(struct pci_dev *dev);
>> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
>> index 7f1d674ff171..a62648dd52bc 100644
>> --- a/drivers/pci/pcie/aspm.c
>> +++ b/drivers/pci/pcie/aspm.c
>> @@ -24,13 +24,29 @@
>>
>>   #include "../pci.h"
>>
>> +static void pci_save_aspm_state(struct pci_dev *dev)
>> +{
>> +     struct pci_cap_saved_state *save_state;
>> +     u16 *cap;
>> +
>> +     if (!pci_is_pcie(dev))
>> +             return;
>> +
>> +     save_state = pci_find_saved_cap(dev, PCI_CAP_ID_EXP);
>> +     if (!save_state)
>> +             return;
>> +
>> +     cap = (u16 *)&save_state->cap.data[0];
>> +     pcie_capability_read_word(dev, PCI_EXP_LNKCTL, &cap[1]);
>> +}
>> +
>>   void pci_aspm_get_l1ss(struct pci_dev *pdev)
>>   {
>>        /* Read L1 PM substate capabilities */
>>        pdev->l1ss = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_L1SS);
>>   }
>>
>> -void pci_save_aspm_state(struct pci_dev *pdev)
>> +void pci_save_aspm_l1ss_state(struct pci_dev *pdev)
>>   {
>>        struct pci_cap_saved_state *save_state;
>>        u16 l1ss = pdev->l1ss;
>> @@ -309,10 +325,12 @@ static void pcie_set_clkpm_nocheck(struct pcie_link_state *link, int enable)
>>        struct pci_bus *linkbus = link->pdev->subordinate;
>>        u32 val = enable ? PCI_EXP_LNKCTL_CLKREQ_EN : 0;
>>
>> -     list_for_each_entry(child, &linkbus->devices, bus_list)
>> +     list_for_each_entry(child, &linkbus->devices, bus_list) {
>>                pcie_capability_clear_and_set_word(child, PCI_EXP_LNKCTL,
>>                                                   PCI_EXP_LNKCTL_CLKREQ_EN,
>>                                                   val);
>> +             pci_save_aspm_state(child);
>> +     }
>>        link->clkpm_enabled = !!enable;
>>   }
>>
>> @@ -931,6 +949,12 @@ static void pcie_config_aspm_link(struct pcie_link_state *link, u32 state)
>>                pcie_config_aspm_dev(parent, upstream);
>>
>>        link->aspm_enabled = state;
>> +
>> +     /* Update latest ASPM configuration in saved context */
>> +     pci_save_aspm_state(link->downstream);
>> +     pci_save_aspm_l1ss_state(link->downstream);
>> +     pci_save_aspm_state(parent);
>> +     pci_save_aspm_l1ss_state(parent);
>>   }
>>
>>   static void pcie_config_aspm_path(struct pcie_link_state *link)
> --
> Sathyanarayanan Kuppuswamy
> Linux Kernel Developer
>
David E. Box Feb. 22, 2024, 10:14 p.m. UTC | #5
On Thu, 2024-02-22 at 11:54 -0800, Kuppuswamy Sathyanarayanan wrote:
> 
> On 2/22/24 10:54 AM, Bjorn Helgaas wrote:
> > On Thu, Feb 22, 2024 at 10:20:06AM -0800, Kuppuswamy Sathyanarayanan wrote:
> > > On 2/22/24 9:44 AM, Vidya Sagar wrote:
> > > > Many PCIe device drivers save the configuration state of their
> > > > respective
> > > > devices during probe and restore the same when their 'slot_reset' hook
> > > > is called through PCIe Error Recovery Handler.
> > > > 
> > > > If the system has a change in ASPM policy after the driver's probe is
> > > > called and before error event occurred, 'slot_reset' hook restores the
> > > > PCIe configuration state to what it was at the time of probe but not to
> > > > what it was just before the occurrence of the error event.
> > > > This effectively leads to a mismatch in the ASPM configuration between
> > > > the device and its upstream parent device.
> > > > 
> > > > Update the saved configuration state of the device with the latest info
> > > > whenever there is a change w.r.t ASPM policy.
> > > > 
> > > > Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
> > > > ---
> > > > V4:
> > > > * Rebased on top of pci/aspm branch
> > > > 
> > > > V3:
> > > > * Addressed sathyanarayanan.kuppuswamy's review comments
> > > > 
> > > > V2:
> > > > * Rebased on top of the tree code
> > > > * Addressed Bjorn's review comments
> > > > 
> > > >  drivers/pci/pcie/aspm.c | 28 ++++++++++++++++++++++++++--
> > > >  3 files changed, 28 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > > index cfc5b84dc9c9..3db606ba9344 100644
> > > > --- a/drivers/pci/pci.c
> > > > +++ b/drivers/pci/pci.c
> > > > @@ -1648,7 +1648,7 @@ static int pci_save_pcie_state(struct pci_dev
> > > > *dev)
> > > >  	pcie_capability_read_word(dev, PCI_EXP_LNKCTL2, &cap[i++]);
> > > >  	pcie_capability_read_word(dev, PCI_EXP_SLTCTL2, &cap[i++]);
> > > >  
> > > > -	pci_save_aspm_state(dev);
> > > > +	pci_save_aspm_l1ss_state(dev);
> > > >  	pci_save_ltr_state(dev);
> > > >  
> > > >  	return 0;
> > > > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> > > > index b217e74966eb..9fe78eb8b07d 100644
> > > > --- a/drivers/pci/pci.h
> > > > +++ b/drivers/pci/pci.h
> > > > @@ -95,7 +95,7 @@ void pci_msix_init(struct pci_dev *dev);
> > > >  bool pci_bridge_d3_possible(struct pci_dev *dev);
> > > >  void pci_bridge_d3_update(struct pci_dev *dev);
> > > >  void pci_aspm_get_l1ss(struct pci_dev *pdev);
> > > > -void pci_save_aspm_state(struct pci_dev *pdev);
> > > > +void pci_save_aspm_l1ss_state(struct pci_dev *pdev);
> > > is this rename a review request? It is not clear from the commit log
> > > why you are doing it?
> > David's changes already on pci/aspm added pci_save_aspm_state(), but
> > it actually only saves L1SS data, and Vidya needs to save the non-L1SS
> > data also.
> > 
> > I think I'm going to rework David's changes a little bit so this is
> > named pci_save_aspm_l1ss_state() from the beginning so we won't need
> > the rename here.

Ack

> 
> Got it.
> 
> Change wise, this patch looks fine to me.
> 
> Reviewed-by: Kuppuswamy Sathyanarayanan
> <sathyanarayanan.kuppuswamy@linux.intel.com>

LGTM too.

Reviewed-by: David E. Box <david.e.box@linux.intel.com>
Bjorn Helgaas March 5, 2024, 10:03 p.m. UTC | #6
[+to Sathy, David in case you want to update your Reviewed-by]

On Thu, Feb 22, 2024 at 11:14:36PM +0530, Vidya Sagar wrote:
> Many PCIe device drivers save the configuration state of their respective
> devices during probe and restore the same when their 'slot_reset' hook
> is called through PCIe Error Recovery Handler.
> 
> If the system has a change in ASPM policy after the driver's probe is
> called and before error event occurred, 'slot_reset' hook restores the
> PCIe configuration state to what it was at the time of probe but not to
> what it was just before the occurrence of the error event.
> This effectively leads to a mismatch in the ASPM configuration between
> the device and its upstream parent device.
> 
> Update the saved configuration state of the device with the latest info
> whenever there is a change w.r.t ASPM policy.
> 
> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>

> -void pci_save_aspm_state(struct pci_dev *pdev);
> +void pci_save_aspm_l1ss_state(struct pci_dev *pdev);

I rebased this again on top of my pci/aspm updates to remove the need
for the rename above.

> +static void pci_save_aspm_state(struct pci_dev *dev)
> +{
> +	struct pci_cap_saved_state *save_state;
> +	u16 *cap;
> +
> +	if (!pci_is_pcie(dev))
> +		return;
> +
> +	save_state = pci_find_saved_cap(dev, PCI_CAP_ID_EXP);
> +	if (!save_state)
> +		return;
> +
> +	cap = (u16 *)&save_state->cap.data[0];
> +	pcie_capability_read_word(dev, PCI_EXP_LNKCTL, &cap[1]);

And I changed this part so it only updates the PCI_EXP_LNKCTL_ASPMC
bits, not the entire LNKCTL.

Updating the entire saved register probably wouldn't *break* anything,
but it could randomly hide other LNKCTL changes depending on whether
or not ASPM configuration was changed in the interim.  For example:

  - driver .probe() saves LNKCTL
  - LNKCTL changes some non-ASPMC thing via setpci or other mechanism
  - save_state updated via pcie_config_aspm_link()

A restore in .slot_reset() would restore different LNKCTL values for
the non-ASPMC change depending on whether pcie_config_aspm_link() was
used.

I applied it on pci/aspm for v6.9.  Please take a look and make sure
it still does what you need:
https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/commit/?h=aspm&id=a6315434436d587f70e489e6365c5b7e20176a71

Sathy and David, I didn't add your Reviewed-by because I didn't want
to presume that you were OK with my changes.  But I'd be more than
happy to add them if you take a look.

Bjorn

> +}
> +
>  void pci_aspm_get_l1ss(struct pci_dev *pdev)
>  {
>  	/* Read L1 PM substate capabilities */
>  	pdev->l1ss = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_L1SS);
>  }
>  
> -void pci_save_aspm_state(struct pci_dev *pdev)
> +void pci_save_aspm_l1ss_state(struct pci_dev *pdev)
>  {
>  	struct pci_cap_saved_state *save_state;
>  	u16 l1ss = pdev->l1ss;
> @@ -309,10 +325,12 @@ static void pcie_set_clkpm_nocheck(struct pcie_link_state *link, int enable)
>  	struct pci_bus *linkbus = link->pdev->subordinate;
>  	u32 val = enable ? PCI_EXP_LNKCTL_CLKREQ_EN : 0;
>  
> -	list_for_each_entry(child, &linkbus->devices, bus_list)
> +	list_for_each_entry(child, &linkbus->devices, bus_list) {
>  		pcie_capability_clear_and_set_word(child, PCI_EXP_LNKCTL,
>  						   PCI_EXP_LNKCTL_CLKREQ_EN,
>  						   val);
> +		pci_save_aspm_state(child);
> +	}
>  	link->clkpm_enabled = !!enable;
>  }
>  
> @@ -931,6 +949,12 @@ static void pcie_config_aspm_link(struct pcie_link_state *link, u32 state)
>  		pcie_config_aspm_dev(parent, upstream);
>  
>  	link->aspm_enabled = state;
> +
> +	/* Update latest ASPM configuration in saved context */
> +	pci_save_aspm_state(link->downstream);
> +	pci_save_aspm_l1ss_state(link->downstream);
> +	pci_save_aspm_state(parent);
> +	pci_save_aspm_l1ss_state(parent);
>  }
>  
>  static void pcie_config_aspm_path(struct pcie_link_state *link)
> -- 
> 2.25.1
>
Kuppuswamy Sathyanarayanan March 5, 2024, 10:35 p.m. UTC | #7
On 3/5/24 2:03 PM, Bjorn Helgaas wrote:
> [+to Sathy, David in case you want to update your Reviewed-by]
>
> On Thu, Feb 22, 2024 at 11:14:36PM +0530, Vidya Sagar wrote:
>> Many PCIe device drivers save the configuration state of their respective
>> devices during probe and restore the same when their 'slot_reset' hook
>> is called through PCIe Error Recovery Handler.
>>
>> If the system has a change in ASPM policy after the driver's probe is
>> called and before error event occurred, 'slot_reset' hook restores the
>> PCIe configuration state to what it was at the time of probe but not to
>> what it was just before the occurrence of the error event.
>> This effectively leads to a mismatch in the ASPM configuration between
>> the device and its upstream parent device.
>>
>> Update the saved configuration state of the device with the latest info
>> whenever there is a change w.r.t ASPM policy.
>>
>> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
>> -void pci_save_aspm_state(struct pci_dev *pdev);
>> +void pci_save_aspm_l1ss_state(struct pci_dev *pdev);
> I rebased this again on top of my pci/aspm updates to remove the need
> for the rename above.
>
>> +static void pci_save_aspm_state(struct pci_dev *dev)
>> +{
>> +	struct pci_cap_saved_state *save_state;
>> +	u16 *cap;
>> +
>> +	if (!pci_is_pcie(dev))
>> +		return;
>> +
>> +	save_state = pci_find_saved_cap(dev, PCI_CAP_ID_EXP);
>> +	if (!save_state)
>> +		return;
>> +
>> +	cap = (u16 *)&save_state->cap.data[0];
>> +	pcie_capability_read_word(dev, PCI_EXP_LNKCTL, &cap[1]);
> And I changed this part so it only updates the PCI_EXP_LNKCTL_ASPMC
> bits, not the entire LNKCTL.
>
> Updating the entire saved register probably wouldn't *break* anything,
> but it could randomly hide other LNKCTL changes depending on whether
> or not ASPM configuration was changed in the interim.  For example:
>
>   - driver .probe() saves LNKCTL
>   - LNKCTL changes some non-ASPMC thing via setpci or other mechanism
>   - save_state updated via pcie_config_aspm_link()
>
> A restore in .slot_reset() would restore different LNKCTL values for
> the non-ASPMC change depending on whether pcie_config_aspm_link() was
> used.
>
> I applied it on pci/aspm for v6.9.  Please take a look and make sure
> it still does what you need:
> https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/commit/?h=aspm&id=a6315434436d587f70e489e6365c5b7e20176a71
>
> Sathy and David, I didn't add your Reviewed-by because I didn't want
> to presume that you were OK with my changes.  But I'd be more than
> happy to add them if you take a look.

Your update looks fine to me.

Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

>
> Bjorn
>
>> +}
>> +
>>  void pci_aspm_get_l1ss(struct pci_dev *pdev)
>>  {
>>  	/* Read L1 PM substate capabilities */
>>  	pdev->l1ss = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_L1SS);
>>  }
>>  
>> -void pci_save_aspm_state(struct pci_dev *pdev)
>> +void pci_save_aspm_l1ss_state(struct pci_dev *pdev)
>>  {
>>  	struct pci_cap_saved_state *save_state;
>>  	u16 l1ss = pdev->l1ss;
>> @@ -309,10 +325,12 @@ static void pcie_set_clkpm_nocheck(struct pcie_link_state *link, int enable)
>>  	struct pci_bus *linkbus = link->pdev->subordinate;
>>  	u32 val = enable ? PCI_EXP_LNKCTL_CLKREQ_EN : 0;
>>  
>> -	list_for_each_entry(child, &linkbus->devices, bus_list)
>> +	list_for_each_entry(child, &linkbus->devices, bus_list) {
>>  		pcie_capability_clear_and_set_word(child, PCI_EXP_LNKCTL,
>>  						   PCI_EXP_LNKCTL_CLKREQ_EN,
>>  						   val);
>> +		pci_save_aspm_state(child);
>> +	}
>>  	link->clkpm_enabled = !!enable;
>>  }
>>  
>> @@ -931,6 +949,12 @@ static void pcie_config_aspm_link(struct pcie_link_state *link, u32 state)
>>  		pcie_config_aspm_dev(parent, upstream);
>>  
>>  	link->aspm_enabled = state;
>> +
>> +	/* Update latest ASPM configuration in saved context */
>> +	pci_save_aspm_state(link->downstream);
>> +	pci_save_aspm_l1ss_state(link->downstream);
>> +	pci_save_aspm_state(parent);
>> +	pci_save_aspm_l1ss_state(parent);
>>  }
>>  
>>  static void pcie_config_aspm_path(struct pcie_link_state *link)
>> -- 
>> 2.25.1
>>
David E. Box March 6, 2024, 1:37 a.m. UTC | #8
On Tue, 2024-03-05 at 16:03 -0600, Bjorn Helgaas wrote:
> [+to Sathy, David in case you want to update your Reviewed-by]
> 
> On Thu, Feb 22, 2024 at 11:14:36PM +0530, Vidya Sagar wrote:
> > Many PCIe device drivers save the configuration state of their respective
> > devices during probe and restore the same when their 'slot_reset' hook
> > is called through PCIe Error Recovery Handler.
> > 
> > If the system has a change in ASPM policy after the driver's probe is
> > called and before error event occurred, 'slot_reset' hook restores the
> > PCIe configuration state to what it was at the time of probe but not to
> > what it was just before the occurrence of the error event.
> > This effectively leads to a mismatch in the ASPM configuration between
> > the device and its upstream parent device.
> > 
> > Update the saved configuration state of the device with the latest info
> > whenever there is a change w.r.t ASPM policy.
> > 
> > Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
> 
> > -void pci_save_aspm_state(struct pci_dev *pdev);
> > +void pci_save_aspm_l1ss_state(struct pci_dev *pdev);
> 
> I rebased this again on top of my pci/aspm updates to remove the need
> for the rename above.
> 
> > +static void pci_save_aspm_state(struct pci_dev *dev)
> > +{
> > +       struct pci_cap_saved_state *save_state;
> > +       u16 *cap;
> > +
> > +       if (!pci_is_pcie(dev))
> > +               return;
> > +
> > +       save_state = pci_find_saved_cap(dev, PCI_CAP_ID_EXP);
> > +       if (!save_state)
> > +               return;
> > +
> > +       cap = (u16 *)&save_state->cap.data[0];
> > +       pcie_capability_read_word(dev, PCI_EXP_LNKCTL, &cap[1]);
> 
> And I changed this part so it only updates the PCI_EXP_LNKCTL_ASPMC
> bits, not the entire LNKCTL.
> 
> Updating the entire saved register probably wouldn't *break* anything,
> but it could randomly hide other LNKCTL changes depending on whether
> or not ASPM configuration was changed in the interim.  For example:
> 
>   - driver .probe() saves LNKCTL
>   - LNKCTL changes some non-ASPMC thing via setpci or other mechanism
>   - save_state updated via pcie_config_aspm_link()
> 
> A restore in .slot_reset() would restore different LNKCTL values for
> the non-ASPMC change depending on whether pcie_config_aspm_link() was
> used.
> 
> I applied it on pci/aspm for v6.9.  Please take a look and make sure
> it still does what you need:
> https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/commit/?h=aspm&id=a6315434436d587f70e489e6365c5b7e20176a71
> 
> Sathy and David, I didn't add your Reviewed-by because I didn't want
> to presume that you were OK with my changes.  But I'd be more than
> happy to add them if you take a look.
> 
> Bjorn
> 
> > +}
> > +
> >  void pci_aspm_get_l1ss(struct pci_dev *pdev)
> >  {
> >         /* Read L1 PM substate capabilities */
> >         pdev->l1ss = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_L1SS);
> >  }
> >  
> > -void pci_save_aspm_state(struct pci_dev *pdev)
> > +void pci_save_aspm_l1ss_state(struct pci_dev *pdev)
> >  {
> >         struct pci_cap_saved_state *save_state;
> >         u16 l1ss = pdev->l1ss;
> > @@ -309,10 +325,12 @@ static void pcie_set_clkpm_nocheck(struct
> > pcie_link_state *link, int enable)
> >         struct pci_bus *linkbus = link->pdev->subordinate;
> >         u32 val = enable ? PCI_EXP_LNKCTL_CLKREQ_EN : 0;
> >  
> > -       list_for_each_entry(child, &linkbus->devices, bus_list)
> > +       list_for_each_entry(child, &linkbus->devices, bus_list) {
> >                 pcie_capability_clear_and_set_word(child, PCI_EXP_LNKCTL,
> >                                                    PCI_EXP_LNKCTL_CLKREQ_EN,
> >                                                    val);
> > +               pci_save_aspm_state(child);
> > +       }
> >         link->clkpm_enabled = !!enable;
> >  }
> >  
> > @@ -931,6 +949,12 @@ static void pcie_config_aspm_link(struct
> > pcie_link_state *link, u32 state)
> >                 pcie_config_aspm_dev(parent, upstream);
> >  
> >         link->aspm_enabled = state;
> > +
> > +       /* Update latest ASPM configuration in saved context */
> > +       pci_save_aspm_state(link->downstream);
> > +       pci_save_aspm_l1ss_state(link->downstream);
> > +       pci_save_aspm_state(parent);
> > +       pci_save_aspm_l1ss_state(parent);
> >  }
> >  
> >  static void pcie_config_aspm_path(struct pcie_link_state *link)
> > -- 
> > 2.25.1
> > 

Reviewed-by: David E. Box <david.e.box@linux.intel.com>
Bjorn Helgaas March 7, 2024, 10:01 p.m. UTC | #9
On Tue, Mar 05, 2024 at 04:03:42PM -0600, Bjorn Helgaas wrote:
> [+to Sathy, David in case you want to update your Reviewed-by]
> 
> On Thu, Feb 22, 2024 at 11:14:36PM +0530, Vidya Sagar wrote:
> > Many PCIe device drivers save the configuration state of their respective
> > devices during probe and restore the same when their 'slot_reset' hook
> > is called through PCIe Error Recovery Handler.
> > 
> > If the system has a change in ASPM policy after the driver's probe is
> > called and before error event occurred, 'slot_reset' hook restores the
> > PCIe configuration state to what it was at the time of probe but not to
> > what it was just before the occurrence of the error event.
> > This effectively leads to a mismatch in the ASPM configuration between
> > the device and its upstream parent device.
> > 
> > Update the saved configuration state of the device with the latest info
> > whenever there is a change w.r.t ASPM policy.
> > 
> > Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
> 
> > -void pci_save_aspm_state(struct pci_dev *pdev);
> > +void pci_save_aspm_l1ss_state(struct pci_dev *pdev);
> 
> I rebased this again on top of my pci/aspm updates to remove the need
> for the rename above.
> 
> > +static void pci_save_aspm_state(struct pci_dev *dev)
> > +{
> > +	struct pci_cap_saved_state *save_state;
> > +	u16 *cap;
> > +
> > +	if (!pci_is_pcie(dev))
> > +		return;
> > +
> > +	save_state = pci_find_saved_cap(dev, PCI_CAP_ID_EXP);
> > +	if (!save_state)
> > +		return;
> > +
> > +	cap = (u16 *)&save_state->cap.data[0];
> > +	pcie_capability_read_word(dev, PCI_EXP_LNKCTL, &cap[1]);
> 
> And I changed this part so it only updates the PCI_EXP_LNKCTL_ASPMC
> bits, not the entire LNKCTL.
> 
> Updating the entire saved register probably wouldn't *break* anything,
> but it could randomly hide other LNKCTL changes depending on whether
> or not ASPM configuration was changed in the interim.  For example:
> 
>   - driver .probe() saves LNKCTL
>   - LNKCTL changes some non-ASPMC thing via setpci or other mechanism
>   - save_state updated via pcie_config_aspm_link()
> 
> A restore in .slot_reset() would restore different LNKCTL values for
> the non-ASPMC change depending on whether pcie_config_aspm_link() was
> used.

Oops, I blew it here.  I think it's good to limit the LNKCTL changes
we put in save_state, but PCI_EXP_LNKCTL_ASPMC is not enough.  We
should include PCI_EXP_LNKCTL_CLKREQ_EN as well since that may be
updated in many of the same paths that update PCI_EXP_LNKCTL_ASPMC.

I updated it to the patch at the bottom; here's the interdiff first:

-------------------------------------------------------------------------

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 46352132bb14..10160d82c10a 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -304,18 +304,25 @@ static int policy_to_clkpm_state(struct pcie_link_state *link)
 static void pci_update_aspm_saved_state(struct pci_dev *dev)
 {
 	struct pci_cap_saved_state *save_state;
-	u16 *cap, lnkctl, aspmc;
+	u16 *cap, lnkctl, aspm_ctl;
 
 	save_state = pci_find_saved_cap(dev, PCI_CAP_ID_EXP);
 	if (!save_state)
 		return;
 
+	pcie_capability_read_word(dev, PCI_EXP_LNKCTL, &lnkctl);
+
+	/*
+	 * Update ASPM and CLKREQ bits of LNKCTL in save_state. We only
+	 * write PCI_EXP_LNKCTL_CCC during enumeration, so it shouldn't
+	 * change after being captured in save_state.
+	 */
+	aspm_ctl = lnkctl & (PCI_EXP_LNKCTL_ASPMC | PCI_EXP_LNKCTL_CLKREQ_EN);
+	lnkctl &= ~(PCI_EXP_LNKCTL_ASPMC | PCI_EXP_LNKCTL_CLKREQ_EN);
+
 	/* Depends on pci_save_pcie_state(): cap[1] is LNKCTL */
 	cap = (u16 *)&save_state->cap.data[0];
-	pcie_capability_read_word(dev, PCI_EXP_LNKCTL, &lnkctl);
-	aspmc = FIELD_GET(PCI_EXP_LNKCTL_ASPMC, lnkctl);
-	cap[1] = (lnkctl & ~PCI_EXP_LNKCTL_ASPMC) |
-		FIELD_PREP(PCI_EXP_LNKCTL_ASPMC, aspmc);
+	cap[1] = lnkctl | aspm_ctl;
 }
 
 static void pcie_set_clkpm_nocheck(struct pcie_link_state *link, int enable)

-------------------------------------------------------------------------

And here's the entire patch:

commit ff92b9348534 ("PCI/ASPM: Update save_state when configuration changes")
Author: Vidya Sagar <vidyas@nvidia.com>
Date:   Fri Feb 23 13:36:24 2024 -0600

    PCI/ASPM: Update save_state when configuration changes
    
    Many PCIe device drivers save the configuration state of their device
    during probe and restore it when their .slot_reset() hook is called during
    PCIe error recovery.
    
    If the ASPM configuration is changed after the driver's probe is called and
    before an error event occurs, .slot_reset() restores the ASPM configuration
    to what it was at the time of probe, not to what it was just before the
    occurrence of the error event.  This leads to a mismatch in ASPM
    configuration between the device and its upstream device.
    
    Update the saved configuration of the device when the ASPM configuration
    changes.
    
    Link: https://lore.kernel.org/r/20240222174436.3565146-1-vidyas@nvidia.com
    Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
    [bhelgaas: commit log, rebase to pci/aspm, rename to
    pci_update_aspm_saved_state() since it updates only LNKCTL, update only
    ASPMC and CLKREQ_EN in LNKCTL]
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
    Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
    Reviewed-by: David E. Box <david.e.box@linux.intel.com>


diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 1379b8decdf1..10160d82c10a 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -301,16 +301,42 @@ static int policy_to_clkpm_state(struct pcie_link_state *link)
 	return 0;
 }
 
+static void pci_update_aspm_saved_state(struct pci_dev *dev)
+{
+	struct pci_cap_saved_state *save_state;
+	u16 *cap, lnkctl, aspm_ctl;
+
+	save_state = pci_find_saved_cap(dev, PCI_CAP_ID_EXP);
+	if (!save_state)
+		return;
+
+	pcie_capability_read_word(dev, PCI_EXP_LNKCTL, &lnkctl);
+
+	/*
+	 * Update ASPM and CLKREQ bits of LNKCTL in save_state. We only
+	 * write PCI_EXP_LNKCTL_CCC during enumeration, so it shouldn't
+	 * change after being captured in save_state.
+	 */
+	aspm_ctl = lnkctl & (PCI_EXP_LNKCTL_ASPMC | PCI_EXP_LNKCTL_CLKREQ_EN);
+	lnkctl &= ~(PCI_EXP_LNKCTL_ASPMC | PCI_EXP_LNKCTL_CLKREQ_EN);
+
+	/* Depends on pci_save_pcie_state(): cap[1] is LNKCTL */
+	cap = (u16 *)&save_state->cap.data[0];
+	cap[1] = lnkctl | aspm_ctl;
+}
+
 static void pcie_set_clkpm_nocheck(struct pcie_link_state *link, int enable)
 {
 	struct pci_dev *child;
 	struct pci_bus *linkbus = link->pdev->subordinate;
 	u32 val = enable ? PCI_EXP_LNKCTL_CLKREQ_EN : 0;
 
-	list_for_each_entry(child, &linkbus->devices, bus_list)
+	list_for_each_entry(child, &linkbus->devices, bus_list) {
 		pcie_capability_clear_and_set_word(child, PCI_EXP_LNKCTL,
 						   PCI_EXP_LNKCTL_CLKREQ_EN,
 						   val);
+		pci_update_aspm_saved_state(child);
+	}
 	link->clkpm_enabled = !!enable;
 }
 
@@ -929,6 +955,12 @@ static void pcie_config_aspm_link(struct pcie_link_state *link, u32 state)
 		pcie_config_aspm_dev(parent, upstream);
 
 	link->aspm_enabled = state;
+
+	/* Update latest ASPM configuration in saved context */
+	pci_save_aspm_l1ss_state(link->downstream);
+	pci_update_aspm_saved_state(link->downstream);
+	pci_save_aspm_l1ss_state(parent);
+	pci_update_aspm_saved_state(parent);
 }
 
 static void pcie_config_aspm_path(struct pcie_link_state *link)
diff mbox series

Patch

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index cfc5b84dc9c9..3db606ba9344 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1648,7 +1648,7 @@  static int pci_save_pcie_state(struct pci_dev *dev)
 	pcie_capability_read_word(dev, PCI_EXP_LNKCTL2, &cap[i++]);
 	pcie_capability_read_word(dev, PCI_EXP_SLTCTL2, &cap[i++]);
 
-	pci_save_aspm_state(dev);
+	pci_save_aspm_l1ss_state(dev);
 	pci_save_ltr_state(dev);
 
 	return 0;
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index b217e74966eb..9fe78eb8b07d 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -95,7 +95,7 @@  void pci_msix_init(struct pci_dev *dev);
 bool pci_bridge_d3_possible(struct pci_dev *dev);
 void pci_bridge_d3_update(struct pci_dev *dev);
 void pci_aspm_get_l1ss(struct pci_dev *pdev);
-void pci_save_aspm_state(struct pci_dev *pdev);
+void pci_save_aspm_l1ss_state(struct pci_dev *pdev);
 void pci_restore_aspm_state(struct pci_dev *pdev);
 void pci_save_ltr_state(struct pci_dev *dev);
 void pci_restore_ltr_state(struct pci_dev *dev);
diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 7f1d674ff171..a62648dd52bc 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -24,13 +24,29 @@ 
 
 #include "../pci.h"
 
+static void pci_save_aspm_state(struct pci_dev *dev)
+{
+	struct pci_cap_saved_state *save_state;
+	u16 *cap;
+
+	if (!pci_is_pcie(dev))
+		return;
+
+	save_state = pci_find_saved_cap(dev, PCI_CAP_ID_EXP);
+	if (!save_state)
+		return;
+
+	cap = (u16 *)&save_state->cap.data[0];
+	pcie_capability_read_word(dev, PCI_EXP_LNKCTL, &cap[1]);
+}
+
 void pci_aspm_get_l1ss(struct pci_dev *pdev)
 {
 	/* Read L1 PM substate capabilities */
 	pdev->l1ss = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_L1SS);
 }
 
-void pci_save_aspm_state(struct pci_dev *pdev)
+void pci_save_aspm_l1ss_state(struct pci_dev *pdev)
 {
 	struct pci_cap_saved_state *save_state;
 	u16 l1ss = pdev->l1ss;
@@ -309,10 +325,12 @@  static void pcie_set_clkpm_nocheck(struct pcie_link_state *link, int enable)
 	struct pci_bus *linkbus = link->pdev->subordinate;
 	u32 val = enable ? PCI_EXP_LNKCTL_CLKREQ_EN : 0;
 
-	list_for_each_entry(child, &linkbus->devices, bus_list)
+	list_for_each_entry(child, &linkbus->devices, bus_list) {
 		pcie_capability_clear_and_set_word(child, PCI_EXP_LNKCTL,
 						   PCI_EXP_LNKCTL_CLKREQ_EN,
 						   val);
+		pci_save_aspm_state(child);
+	}
 	link->clkpm_enabled = !!enable;
 }
 
@@ -931,6 +949,12 @@  static void pcie_config_aspm_link(struct pcie_link_state *link, u32 state)
 		pcie_config_aspm_dev(parent, upstream);
 
 	link->aspm_enabled = state;
+
+	/* Update latest ASPM configuration in saved context */
+	pci_save_aspm_state(link->downstream);
+	pci_save_aspm_l1ss_state(link->downstream);
+	pci_save_aspm_state(parent);
+	pci_save_aspm_l1ss_state(parent);
 }
 
 static void pcie_config_aspm_path(struct pcie_link_state *link)