LinuxLists.cc - [PATCH V1] PCI/ASPM: Update saved buffers with latest ASPM configuration

[permalink] [raw]

Subject: Re: [PATCH V1] PCI/ASPM: Update saved buffers with latest ASPM configuration

On 1/25/2023 8:31 PM, Wysocki, Rafael J wrote:
> External email: Use caution opening links or attachments
>
>
> On 1/25/2023 2:38 PM, 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 System.
>> 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 with
>> 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.
>> This patch addresses that issue by updating 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 <[email protected]>
>
> If it is a bug fix (which I think it is), a Fixes tag should be present
> here.

It is kind of a bug fix but I couldn't pin point to any particular
commit that would have introduced it.

>
> If the reporter's names are known, Reported-by tags should be present
> here too.

I was experimenting with the error handling code and happen to find this.

>
> If anyone except for you has tested this patch, a Tested-by tag should
> be present here.

Only I tested this patch for now. It would be great if more verification
is done on this patch.

Thanks,
Vidya Sagar

>
>> ---
>> drivers/pci/pci.h       | 4 ++++
>> drivers/pci/pcie/aspm.c | 40 ++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 44 insertions(+)
>>
>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>> index 9ed3b5550043..f4a91d4fe96d 100644
>> --- a/drivers/pci/pci.h
>> +++ b/drivers/pci/pci.h
>> @@ -566,12 +566,16 @@ bool pcie_wait_for_link(struct pci_dev *pdev,
>> bool active);
>> void pcie_aspm_init_link_state(struct pci_dev *pdev);
>> void pcie_aspm_exit_link_state(struct pci_dev *pdev);
>> void pcie_aspm_powersave_config_link(struct pci_dev *pdev);
>> +void pci_save_aspm_state(struct pci_dev *dev);
>> +void pci_restore_aspm_state(struct pci_dev *dev);
>> void pci_save_aspm_l1ss_state(struct pci_dev *dev);
>> void pci_restore_aspm_l1ss_state(struct pci_dev *dev);
>> #else
>> static inline void pcie_aspm_init_link_state(struct pci_dev *pdev) { }
>> static inline void pcie_aspm_exit_link_state(struct pci_dev *pdev) { }
>> static inline void pcie_aspm_powersave_config_link(struct pci_dev
>> *pdev) { }
>> +static inline void pci_save_aspm_state(struct pci_dev *dev) { }
>> +static inline void pci_restore_aspm_state(struct pci_dev *dev) { }
>> static inline void pci_save_aspm_l1ss_state(struct pci_dev *dev) { }
>> static inline void pci_restore_aspm_l1ss_state(struct pci_dev *dev) { }
>> #endif
>> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
>> index 53a1fa306e1e..f25e0440d36b 100644
>> --- a/drivers/pci/pcie/aspm.c
>> +++ b/drivers/pci/pcie/aspm.c
>> @@ -151,6 +151,7 @@ static void pcie_set_clkpm_nocheck(struct
>> pcie_link_state *link, int enable)
>>
>> PCI_EXP_LNKCTL_CLKREQ_EN,
>>                                                  val);
>>       link->clkpm_enabled = !!enable;
>> +     pci_save_aspm_state(child);
>> }
>>
>> static void pcie_set_clkpm(struct pcie_link_state *link, int enable)
>> @@ -757,6 +758,39 @@ static void pcie_config_aspm_l1ss(struct
>> pcie_link_state *link, u32 state)
>>                               PCI_L1SS_CTL1_L1SS_MASK, val);
>> }
>>
>> +void pci_save_aspm_state(struct pci_dev *dev)
>> +{
>> +     int i = 0;
>> +     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];
>> +     i++;
>> +     pcie_capability_read_word(dev, PCI_EXP_LNKCTL, &cap[i++]);
>> +}
>> +
>> +void pci_restore_aspm_state(struct pci_dev *dev)
>> +{
>> +     int i = 0;
>> +     struct pci_cap_saved_state *save_state;
>> +     u16 *cap;
>> +
>> +     save_state = pci_find_saved_cap(dev, PCI_CAP_ID_EXP);
>> +     if (!save_state)
>> +             return;
>> +
>> +     cap = (u16 *)&save_state->cap.data[0];
>> +     i++;
>> +     pcie_capability_write_word(dev, PCI_EXP_LNKCTL, cap[i++]);
>> +}
>> +
>> void pci_save_aspm_l1ss_state(struct pci_dev *dev)
>> {
>>       struct pci_cap_saved_state *save_state;
>> @@ -849,6 +883,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)

2023-01-26 11:44:10

by Wysocki, Rafael J

[permalink] [raw]

Subject: Re: [PATCH V1] PCI/ASPM: Update saved buffers with latest ASPM configuration

On 1/25/2023 6:22 PM, Vidya Sagar wrote:
>
>
> On 1/25/2023 8:31 PM, Wysocki, Rafael J wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> On 1/25/2023 2:38 PM, 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 System.
>>> 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
>>> with
>>> 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.
>>> This patch addresses that issue by updating 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 <[email protected]>
>>
>> If it is a bug fix (which I think it is), a Fixes tag should be present
>> here.
>
> It is kind of a bug fix but I couldn't pin point to any particular
> commit that would have introduced it.
>
>>
>> If the reporter's names are known, Reported-by tags should be present
>> here too.
>
> I was experimenting with the error handling code and happen to find this.
>
>>
>> If anyone except for you has tested this patch, a Tested-by tag should
>> be present here.
>
> Only I tested this patch for now. It would be great if more
> verification is done on this patch.
>
Fair enough.

Please feel free to add

Acked-by: Rafael J. Wysocki <[email protected]>

to this patch.

>
>>
>>> ---
>>> drivers/pci/pci.h       | 4 ++++
>>> drivers/pci/pcie/aspm.c | 40 ++++++++++++++++++++++++++++++++++++++++
>>> 2 files changed, 44 insertions(+)
>>>
>>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>>> index 9ed3b5550043..f4a91d4fe96d 100644
>>> --- a/drivers/pci/pci.h
>>> +++ b/drivers/pci/pci.h
>>> @@ -566,12 +566,16 @@ bool pcie_wait_for_link(struct pci_dev *pdev,
>>> bool active);
>>> void pcie_aspm_init_link_state(struct pci_dev *pdev);
>>> void pcie_aspm_exit_link_state(struct pci_dev *pdev);
>>> void pcie_aspm_powersave_config_link(struct pci_dev *pdev);
>>> +void pci_save_aspm_state(struct pci_dev *dev);
>>> +void pci_restore_aspm_state(struct pci_dev *dev);
>>> void pci_save_aspm_l1ss_state(struct pci_dev *dev);
>>> void pci_restore_aspm_l1ss_state(struct pci_dev *dev);
>>> #else
>>> static inline void pcie_aspm_init_link_state(struct pci_dev *pdev)
>>> { }
>>> static inline void pcie_aspm_exit_link_state(struct pci_dev *pdev)
>>> { }
>>> static inline void pcie_aspm_powersave_config_link(struct pci_dev
>>> *pdev) { }
>>> +static inline void pci_save_aspm_state(struct pci_dev *dev) { }
>>> +static inline void pci_restore_aspm_state(struct pci_dev *dev) { }
>>> static inline void pci_save_aspm_l1ss_state(struct pci_dev *dev) { }
>>> static inline void pci_restore_aspm_l1ss_state(struct pci_dev
>>> *dev) { }
>>> #endif
>>> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
>>> index 53a1fa306e1e..f25e0440d36b 100644
>>> --- a/drivers/pci/pcie/aspm.c
>>> +++ b/drivers/pci/pcie/aspm.c
>>> @@ -151,6 +151,7 @@ static void pcie_set_clkpm_nocheck(struct
>>> pcie_link_state *link, int enable)
>>> PCI_EXP_LNKCTL_CLKREQ_EN,
>>>                                                  val);
>>>       link->clkpm_enabled = !!enable;
>>> +     pci_save_aspm_state(child);
>>> }
>>>
>>> static void pcie_set_clkpm(struct pcie_link_state *link, int enable)
>>> @@ -757,6 +758,39 @@ static void pcie_config_aspm_l1ss(struct
>>> pcie_link_state *link, u32 state)
>>>                               PCI_L1SS_CTL1_L1SS_MASK, val);
>>> }
>>>
>>> +void pci_save_aspm_state(struct pci_dev *dev)
>>> +{
>>> +     int i = 0;
>>> +     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];
>>> +     i++;
>>> +     pcie_capability_read_word(dev, PCI_EXP_LNKCTL, &cap[i++]);
>>> +}
>>> +
>>> +void pci_restore_aspm_state(struct pci_dev *dev)
>>> +{
>>> +     int i = 0;
>>> +     struct pci_cap_saved_state *save_state;
>>> +     u16 *cap;
>>> +
>>> +     save_state = pci_find_saved_cap(dev, PCI_CAP_ID_EXP);
>>> +     if (!save_state)
>>> +             return;
>>> +
>>> +     cap = (u16 *)&save_state->cap.data[0];
>>> +     i++;
>>> +     pcie_capability_write_word(dev, PCI_EXP_LNKCTL, cap[i++]);
>>> +}
>>> +
>>> void pci_save_aspm_l1ss_state(struct pci_dev *dev)
>>> {
>>>       struct pci_cap_saved_state *save_state;
>>> @@ -849,6 +883,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)

2023-01-26 21:22:45

[permalink] [raw]

Subject: Re: [PATCH V1] PCI/ASPM: Update saved buffers with latest ASPM configuration

Hi,

On 1/25/23 5:38 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 System.
> 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 with
> 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.
> This patch addresses that issue by updating the saved configuration state
> of the device with the latest info whenever there is a change w.r.t ASPM
> policy.

Do we need two save/restore calls for ASPM function? Is it not possible
to extend pci_save_aspm_l1ss_state() to meet your need?

>
> Signed-off-by: Vidya Sagar <[email protected]>
> ---
> drivers/pci/pci.h | 4 ++++
> drivers/pci/pcie/aspm.c | 40 ++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 44 insertions(+)
>
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 9ed3b5550043..f4a91d4fe96d 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -566,12 +566,16 @@ bool pcie_wait_for_link(struct pci_dev *pdev, bool active);
> void pcie_aspm_init_link_state(struct pci_dev *pdev);
> void pcie_aspm_exit_link_state(struct pci_dev *pdev);
> void pcie_aspm_powersave_config_link(struct pci_dev *pdev);
> +void pci_save_aspm_state(struct pci_dev *dev);
> +void pci_restore_aspm_state(struct pci_dev *dev);
> void pci_save_aspm_l1ss_state(struct pci_dev *dev);
> void pci_restore_aspm_l1ss_state(struct pci_dev *dev);
> #else
> static inline void pcie_aspm_init_link_state(struct pci_dev *pdev) { }
> static inline void pcie_aspm_exit_link_state(struct pci_dev *pdev) { }
> static inline void pcie_aspm_powersave_config_link(struct pci_dev *pdev) { }
> +static inline void pci_save_aspm_state(struct pci_dev *dev) { }
> +static inline void pci_restore_aspm_state(struct pci_dev *dev) { }
> static inline void pci_save_aspm_l1ss_state(struct pci_dev *dev) { }
> static inline void pci_restore_aspm_l1ss_state(struct pci_dev *dev) { }
> #endif
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 53a1fa306e1e..f25e0440d36b 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -151,6 +151,7 @@ static void pcie_set_clkpm_nocheck(struct pcie_link_state *link, int enable)
> PCI_EXP_LNKCTL_CLKREQ_EN,
> val);
> link->clkpm_enabled = !!enable;
> + pci_save_aspm_state(child);

Add some details about this change to the commit log. Currently, you have talked only
about the ASPM policy change issue.

> }
>
> static void pcie_set_clkpm(struct pcie_link_state *link, int enable)
> @@ -757,6 +758,39 @@ static void pcie_config_aspm_l1ss(struct pcie_link_state *link, u32 state)
> PCI_L1SS_CTL1_L1SS_MASK, val);
> }
>
> +void pci_save_aspm_state(struct pci_dev *dev)
> +{
> + int i = 0;
> + 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];
> + i++;
> + pcie_capability_read_word(dev, PCI_EXP_LNKCTL, &cap[i++]);
> +}
> +
> +void pci_restore_aspm_state(struct pci_dev *dev)
> +{
> + int i = 0;
> + struct pci_cap_saved_state *save_state;
> + u16 *cap;
> +
> + save_state = pci_find_saved_cap(dev, PCI_CAP_ID_EXP);
> + if (!save_state)
> + return;
> +
> + cap = (u16 *)&save_state->cap.data[0];
> + i++;
> + pcie_capability_write_word(dev, PCI_EXP_LNKCTL, cap[i++]);
> +}
> +

Don't you need to add this restore call in pci_restore_state()?

> void pci_save_aspm_l1ss_state(struct pci_dev *dev)
> {
> struct pci_cap_saved_state *save_state;
> @@ -849,6 +883,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

2023-01-27 20:11:02

[permalink] [raw]

Subject: Re: [PATCH V1] PCI/ASPM: Update saved buffers with latest ASPM configuration

2023-01-28 18:26:40

[permalink] [raw]

Subject: Re: [PATCH V1] PCI/ASPM: Update saved buffers with latest ASPM configuration

On Wed, Jan 25, 2023 at 07:08:30PM +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 System.

This strategy of simply restoring config space after a reset is
common, but I think it's only a 90% solution.

After reset, the device is basically in a "fresh poweron" state [1].
At boot-time or for a hot-added device, we do a lot of setup when we
enumerate the device, and assuming that:

- device reset, plus
- current state in the struct pci_dev, plus
- restoring config space

gets all the device and kernel state to the same place is a pretty big
assumption.

That said, we're pretty invested in this strategy for now, and I think
what you propose here is definitely an improvement. Minor comments on
the implementation below.

Bjorn

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/PCI/pci-error-recovery.rst?id=v6.1#n277

> 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 with
> 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.
> This patch addresses that issue by updating 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 <[email protected]>
> ---
> drivers/pci/pci.h | 4 ++++
> drivers/pci/pcie/aspm.c | 40 ++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 44 insertions(+)

> +++ b/drivers/pci/pci.h
> +void pci_save_aspm_state(struct pci_dev *dev);
> +void pci_restore_aspm_state(struct pci_dev *dev);

This patch only adds calls to these functions in aspm.c, so it doesn't
look like we need declarations here or stubs below.

> +static inline void pci_save_aspm_state(struct pci_dev *dev) { }
> +static inline void pci_restore_aspm_state(struct pci_dev *dev) { }

> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 53a1fa306e1e..f25e0440d36b 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -151,6 +151,7 @@ static void pcie_set_clkpm_nocheck(struct pcie_link_state *link, int enable)
> PCI_EXP_LNKCTL_CLKREQ_EN,
> val);
> link->clkpm_enabled = !!enable;
> + pci_save_aspm_state(child);
> }
>
> static void pcie_set_clkpm(struct pcie_link_state *link, int enable)
> @@ -757,6 +758,39 @@ static void pcie_config_aspm_l1ss(struct pcie_link_state *link, u32 state)
> PCI_L1SS_CTL1_L1SS_MASK, val);
> }
>
> +void pci_save_aspm_state(struct pci_dev *dev)

I might be missing something because these look like they should be
static. But the declarations and these being non-static suggest that
you might have something more in mind that isn't part of this patch?

Move these save-state functions higher up if necessary to resolve the
forward reference from pcie_set_clkpm_nocheck().

> +{
> + int i = 0;
> + 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];
> + i++;

"i" looks unnecessary, but I guess I see what you're doing --
mirroring the structure of pci_save_pcie_state() to make sure we put
LNKCTL in the correct element of cap[].

> + pcie_capability_read_word(dev, PCI_EXP_LNKCTL, &cap[i++]);
> +}
> +
> +void pci_restore_aspm_state(struct pci_dev *dev)

No callers for this? And I don't see why you would *need* callers;
this should be restored by pci_restore_pcie_state() already. So this
looks like it could be removed completely.

> +{
> + int i = 0;
> + struct pci_cap_saved_state *save_state;
> + u16 *cap;
> +
> + save_state = pci_find_saved_cap(dev, PCI_CAP_ID_EXP);
> + if (!save_state)
> + return;
> +
> + cap = (u16 *)&save_state->cap.data[0];
> + i++;
> + pcie_capability_write_word(dev, PCI_EXP_LNKCTL, cap[i++]);
> +}
> +
> void pci_save_aspm_l1ss_state(struct pci_dev *dev)
> {
> struct pci_cap_saved_state *save_state;
> @@ -849,6 +883,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.17.1
>

2024-01-03 10:40:56

[permalink] [raw]

Subject: [PATCH V2] PCI/ASPM: Update saved buffers with latest ASPM

2024-01-03 15:38:27

[permalink] [raw]

Subject: Re: [PATCH V2] PCI/ASPM: Update saved buffers with latest ASPM

On 1/3/2024 2:35 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 System.

Recovery System -> 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 with
> 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 <[email protected]>
> ---
> V2:
> * Rebased on top of the tree code
> * Addressed Bjorn's review comments
>
> drivers/pci/pcie/aspm.c | 26 +++++++++++++++++++++++++-
> 1 file changed, 25 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 67b13f26ba7c..d247cabb5e4c 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -138,16 +138,36 @@ static int policy_to_clkpm_state(struct pcie_link_state *link)
> return 0;
> }
>
> +static void pci_save_aspm_state(struct pci_dev *dev)
> +{
> + int i = 0;

I don't see a need for this variable. You can use index directly.

> + 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];
> + i++;
> + pcie_capability_read_word(dev, PCI_EXP_LNKCTL, &cap[i++]);
> +}
> +
> 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_save_aspm_state(child);
> + }
> link->clkpm_enabled = !!enable;
> }
>
> @@ -767,6 +787,10 @@ 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_state(parent);
> }
>
> static void pcie_config_aspm_path(struct pcie_link_state *link)

--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

2024-01-08 12:49:40

[permalink] [raw]

Subject: [PATCH V3] PCI/ASPM: Update saved buffers with latest ASPM

2024-02-20 04:07:55

[permalink] [raw]

Subject: Re: [PATCH V3] PCI/ASPM: Update saved buffers with latest ASPM

Hi Bjorn,

Any comments for this patch?

Thanks,

Vidya Sagar

On 08-01-2024 18:12, 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 with
> 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 <[email protected]>
> ---
> 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 | 24 +++++++++++++++++++++++-
> 1 file changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 67b13f26ba7c..1b4f03044ce2 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -138,16 +138,34 @@ static int policy_to_clkpm_state(struct pcie_link_state *link)
> return 0;
> }
>
> +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]);
> +}
> +
> 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_save_aspm_state(child);
> + }
> link->clkpm_enabled = !!enable;
> }
>
> @@ -767,6 +785,10 @@ 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_state(parent);
> }
>
> static void pcie_config_aspm_path(struct pcie_link_state *link)

2024-02-20 17:26:25

[permalink] [raw]

Subject: Re: [PATCH V3] PCI/ASPM: Update saved buffers with latest ASPM

[+cc David]

On Mon, Jan 08, 2024 at 06:12:48PM +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 with
> 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 <[email protected]>

This overlaps with David's patches that are currently queued for v6.9:
https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/log/?h=be00f078ad2a

Can you rebase this to apply on top of those (this is the 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 | 24 +++++++++++++++++++++++-
> 1 file changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 67b13f26ba7c..1b4f03044ce2 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -138,16 +138,34 @@ static int policy_to_clkpm_state(struct pcie_link_state *link)
> return 0;
> }
>
> +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]);
> +}
> +
> 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_save_aspm_state(child);
> + }
> link->clkpm_enabled = !!enable;
> }
>
> @@ -767,6 +785,10 @@ 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_state(parent);
> }
>
> static void pcie_config_aspm_path(struct pcie_link_state *link)
> --
> 2.25.1
>

2024-02-22 17:59:13

[permalink] [raw]

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

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 <[email protected]>
---
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);
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)
--
2.25.1

2024-02-22 18:35:32

[permalink] [raw]

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

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 <[email protected]>
> ---
> 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)

--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

2024-02-22 18:57:58

[permalink] [raw]

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

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 <[email protected]>
> > ---
> > 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

2024-02-22 19:54:35

[permalink] [raw]

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

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 <[email protected]>
>>> ---
>>> 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 <[email protected]>

>
> Bjorn

--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

2024-02-22 20:59:51

[permalink] [raw]

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

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 <[email protected]>
>> ---
>> 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
>

2024-02-22 22:15:12

by David E. Box

[permalink] [raw]

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

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 <[email protected]>
> > > > ---
> > > > 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
> <[email protected]>

LGTM too.

Reviewed-by: David E. Box <[email protected]>

2024-03-05 22:03:52

[permalink] [raw]

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

[+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 <[email protected]>

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

2024-03-05 22:36:14

[permalink] [raw]

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

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 <[email protected]>
>> -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 <[email protected]>

>
> 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
>>
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

2024-03-06 01:37:49

by David E. Box

[permalink] [raw]

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

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 <[email protected]>
>
> > -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 <[email protected]>

2024-03-07 22:01:30