* [PATCH v2 7/9] wifi: ath11k: Use RMW accessors for changing LNKCTL
[not found] <20230517105235.29176-1-ilpo.jarvinen@linux.intel.com>
@ 2023-05-17 10:52 ` Ilpo Järvinen
2023-05-17 11:04 ` Kalle Valo
2023-05-17 10:52 ` [PATCH v2 8/9] wifi: ath12k: " Ilpo Järvinen
2023-05-17 10:52 ` [PATCH v2 9/9] wifi: ath10k: " Ilpo Järvinen
2 siblings, 1 reply; 11+ messages in thread
From: Ilpo Järvinen @ 2023-05-17 10:52 UTC (permalink / raw)
To: linux-pci, Bjorn Helgaas, Lorenzo Pieralisi, Rob Herring,
Krzysztof Wilczyński, Emmanuel Grumbach, Rafael J . Wysocki,
Heiner Kallweit, Lukas Wunner, Kalle Valo, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Carl Huang, ath11k,
linux-wireless, netdev, linux-kernel
Cc: Dean Luick, Andy Shevchenko, Ilpo Järvinen, stable
Don't assume that only the driver would be accessing LNKCTL. ASPM
policy changes can trigger write to LNKCTL outside of driver's control.
Use RMW capability accessors which do proper locking to avoid losing
concurrent updates to the register value. On restore, clear the ASPMC
field properly.
Fixes: e9603f4bdcc0 ("ath11k: pci: disable ASPM L0sLs before downloading firmware")
Suggested-by: Lukas Wunner <lukas@wunner.de>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Cc: stable@vger.kernel.org
---
drivers/net/wireless/ath/ath11k/pci.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/net/wireless/ath/ath11k/pci.c b/drivers/net/wireless/ath/ath11k/pci.c
index 7b33731a50ee..6ba4cef6b1c7 100644
--- a/drivers/net/wireless/ath/ath11k/pci.c
+++ b/drivers/net/wireless/ath/ath11k/pci.c
@@ -581,8 +581,8 @@ static void ath11k_pci_aspm_disable(struct ath11k_pci *ab_pci)
u16_get_bits(ab_pci->link_ctl, PCI_EXP_LNKCTL_ASPM_L1));
/* disable L0s and L1 */
- pcie_capability_write_word(ab_pci->pdev, PCI_EXP_LNKCTL,
- ab_pci->link_ctl & ~PCI_EXP_LNKCTL_ASPMC);
+ pcie_capability_clear_word(ab_pci->pdev, PCI_EXP_LNKCTL,
+ PCI_EXP_LNKCTL_ASPMC);
set_bit(ATH11K_PCI_ASPM_RESTORE, &ab_pci->flags);
}
@@ -590,8 +590,10 @@ static void ath11k_pci_aspm_disable(struct ath11k_pci *ab_pci)
static void ath11k_pci_aspm_restore(struct ath11k_pci *ab_pci)
{
if (test_and_clear_bit(ATH11K_PCI_ASPM_RESTORE, &ab_pci->flags))
- pcie_capability_write_word(ab_pci->pdev, PCI_EXP_LNKCTL,
- ab_pci->link_ctl);
+ pcie_capability_clear_and_set_word(ab_pci->pdev, PCI_EXP_LNKCTL,
+ PCI_EXP_LNKCTL_ASPMC,
+ ab_pci->link_ctl &
+ PCI_EXP_LNKCTL_ASPMC);
}
static int ath11k_pci_power_up(struct ath11k_base *ab)
--
2.30.2
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 7/9] wifi: ath11k: Use RMW accessors for changing LNKCTL
2023-05-17 10:52 ` [PATCH v2 7/9] wifi: ath11k: Use RMW accessors for changing LNKCTL Ilpo Järvinen
@ 2023-05-17 11:04 ` Kalle Valo
0 siblings, 0 replies; 11+ messages in thread
From: Kalle Valo @ 2023-05-17 11:04 UTC (permalink / raw)
To: Ilpo Järvinen
Cc: linux-pci, Bjorn Helgaas, Lorenzo Pieralisi, Rob Herring,
Krzysztof Wilczyński, Emmanuel Grumbach, Rafael J . Wysocki,
Heiner Kallweit, Lukas Wunner, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Carl Huang, ath11k, linux-wireless,
netdev, linux-kernel, Dean Luick, Andy Shevchenko, stable
Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> writes:
> Don't assume that only the driver would be accessing LNKCTL. ASPM
> policy changes can trigger write to LNKCTL outside of driver's control.
>
> Use RMW capability accessors which do proper locking to avoid losing
> concurrent updates to the register value. On restore, clear the ASPMC
> field properly.
>
> Fixes: e9603f4bdcc0 ("ath11k: pci: disable ASPM L0sLs before downloading firmware")
> Suggested-by: Lukas Wunner <lukas@wunner.de>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Cc: stable@vger.kernel.org
Acked-by: Kalle Valo <kvalo@kernel.org>
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 8/9] wifi: ath12k: Use RMW accessors for changing LNKCTL
[not found] <20230517105235.29176-1-ilpo.jarvinen@linux.intel.com>
2023-05-17 10:52 ` [PATCH v2 7/9] wifi: ath11k: Use RMW accessors for changing LNKCTL Ilpo Järvinen
@ 2023-05-17 10:52 ` Ilpo Järvinen
2023-05-17 11:03 ` Kalle Valo
2023-05-17 10:52 ` [PATCH v2 9/9] wifi: ath10k: " Ilpo Järvinen
2 siblings, 1 reply; 11+ messages in thread
From: Ilpo Järvinen @ 2023-05-17 10:52 UTC (permalink / raw)
To: linux-pci, Bjorn Helgaas, Lorenzo Pieralisi, Rob Herring,
Krzysztof Wilczyński, Emmanuel Grumbach, Rafael J . Wysocki,
Heiner Kallweit, Lukas Wunner, Kalle Valo, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Sriram R, P Praneesh,
Ramya Gnanasekar, Karthikeyan Periyasamy,
Vasanthakumar Thiagarajan, ath12k, linux-wireless, netdev,
linux-kernel
Cc: Dean Luick, Andy Shevchenko, Ilpo Järvinen, stable
Don't assume that only the driver would be accessing LNKCTL. ASPM
policy changes can trigger write to LNKCTL outside of driver's control.
Use RMW capability accessors which do proper locking to avoid losing
concurrent updates to the register value. On restore, clear the ASPMC
field properly.
Fixes: d889913205cf ("wifi: ath12k: driver for Qualcomm Wi-Fi 7 devices")
Suggested-by: Lukas Wunner <lukas@wunner.de>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Cc: stable@vger.kernel.org
---
drivers/net/wireless/ath/ath12k/pci.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/net/wireless/ath/ath12k/pci.c b/drivers/net/wireless/ath/ath12k/pci.c
index 9f174daf324c..e1e45eb50f3e 100644
--- a/drivers/net/wireless/ath/ath12k/pci.c
+++ b/drivers/net/wireless/ath/ath12k/pci.c
@@ -794,8 +794,8 @@ static void ath12k_pci_aspm_disable(struct ath12k_pci *ab_pci)
u16_get_bits(ab_pci->link_ctl, PCI_EXP_LNKCTL_ASPM_L1));
/* disable L0s and L1 */
- pcie_capability_write_word(ab_pci->pdev, PCI_EXP_LNKCTL,
- ab_pci->link_ctl & ~PCI_EXP_LNKCTL_ASPMC);
+ pcie_capability_clear_word(ab_pci->pdev, PCI_EXP_LNKCTL,
+ PCI_EXP_LNKCTL_ASPMC);
set_bit(ATH12K_PCI_ASPM_RESTORE, &ab_pci->flags);
}
@@ -803,8 +803,10 @@ static void ath12k_pci_aspm_disable(struct ath12k_pci *ab_pci)
static void ath12k_pci_aspm_restore(struct ath12k_pci *ab_pci)
{
if (test_and_clear_bit(ATH12K_PCI_ASPM_RESTORE, &ab_pci->flags))
- pcie_capability_write_word(ab_pci->pdev, PCI_EXP_LNKCTL,
- ab_pci->link_ctl);
+ pcie_capability_clear_and_set_word(ab_pci->pdev, PCI_EXP_LNKCTL,
+ PCI_EXP_LNKCTL_ASPMC,
+ ab_pci->link_ctl &
+ PCI_EXP_LNKCTL_ASPMC);
}
static void ath12k_pci_kill_tasklets(struct ath12k_base *ab)
--
2.30.2
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 8/9] wifi: ath12k: Use RMW accessors for changing LNKCTL
2023-05-17 10:52 ` [PATCH v2 8/9] wifi: ath12k: " Ilpo Järvinen
@ 2023-05-17 11:03 ` Kalle Valo
0 siblings, 0 replies; 11+ messages in thread
From: Kalle Valo @ 2023-05-17 11:03 UTC (permalink / raw)
To: Ilpo Järvinen
Cc: linux-pci, Bjorn Helgaas, Lorenzo Pieralisi, Rob Herring,
Krzysztof Wilczyński, Emmanuel Grumbach, Rafael J . Wysocki,
Heiner Kallweit, Lukas Wunner, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Sriram R, P Praneesh,
Ramya Gnanasekar, Karthikeyan Periyasamy,
Vasanthakumar Thiagarajan, ath12k, linux-wireless, netdev,
linux-kernel, Dean Luick, Andy Shevchenko, stable
Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> writes:
> Don't assume that only the driver would be accessing LNKCTL. ASPM
> policy changes can trigger write to LNKCTL outside of driver's control.
>
> Use RMW capability accessors which do proper locking to avoid losing
> concurrent updates to the register value. On restore, clear the ASPMC
> field properly.
>
> Fixes: d889913205cf ("wifi: ath12k: driver for Qualcomm Wi-Fi 7 devices")
> Suggested-by: Lukas Wunner <lukas@wunner.de>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Cc: stable@vger.kernel.org
Acked-by: Kalle Valo <kvalo@kernel.org>
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 9/9] wifi: ath10k: Use RMW accessors for changing LNKCTL
[not found] <20230517105235.29176-1-ilpo.jarvinen@linux.intel.com>
2023-05-17 10:52 ` [PATCH v2 7/9] wifi: ath11k: Use RMW accessors for changing LNKCTL Ilpo Järvinen
2023-05-17 10:52 ` [PATCH v2 8/9] wifi: ath12k: " Ilpo Järvinen
@ 2023-05-17 10:52 ` Ilpo Järvinen
2023-05-17 11:05 ` Kalle Valo
2023-05-24 15:10 ` Bjorn Helgaas
2 siblings, 2 replies; 11+ messages in thread
From: Ilpo Järvinen @ 2023-05-17 10:52 UTC (permalink / raw)
To: linux-pci, Bjorn Helgaas, Lorenzo Pieralisi, Rob Herring,
Krzysztof Wilczyński, Emmanuel Grumbach, Rafael J . Wysocki,
Heiner Kallweit, Lukas Wunner, Kalle Valo, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Michal Kazior,
Janusz Dziedzic, ath10k, linux-wireless, netdev, linux-kernel
Cc: Dean Luick, Andy Shevchenko, Ilpo Järvinen, stable
Don't assume that only the driver would be accessing LNKCTL. ASPM
policy changes can trigger write to LNKCTL outside of driver's control.
Use RMW capability accessors which does proper locking to avoid losing
concurrent updates to the register value. On restore, clear the ASPMC
field properly.
Fixes: 76d870ed09ab ("ath10k: enable ASPM")
Suggested-by: Lukas Wunner <lukas@wunner.de>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Cc: stable@vger.kernel.org
---
drivers/net/wireless/ath/ath10k/pci.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index a7f44f6335fb..9275a672f90c 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -1963,8 +1963,9 @@ static int ath10k_pci_hif_start(struct ath10k *ar)
ath10k_pci_irq_enable(ar);
ath10k_pci_rx_post(ar);
- pcie_capability_write_word(ar_pci->pdev, PCI_EXP_LNKCTL,
- ar_pci->link_ctl);
+ pcie_capability_clear_and_set_word(ar_pci->pdev, PCI_EXP_LNKCTL,
+ PCI_EXP_LNKCTL_ASPMC,
+ ar_pci->link_ctl & PCI_EXP_LNKCTL_ASPMC);
return 0;
}
@@ -2821,8 +2822,8 @@ static int ath10k_pci_hif_power_up(struct ath10k *ar,
pcie_capability_read_word(ar_pci->pdev, PCI_EXP_LNKCTL,
&ar_pci->link_ctl);
- pcie_capability_write_word(ar_pci->pdev, PCI_EXP_LNKCTL,
- ar_pci->link_ctl & ~PCI_EXP_LNKCTL_ASPMC);
+ pcie_capability_clear_word(ar_pci->pdev, PCI_EXP_LNKCTL,
+ PCI_EXP_LNKCTL_ASPMC);
/*
* Bring the target up cleanly.
--
2.30.2
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 9/9] wifi: ath10k: Use RMW accessors for changing LNKCTL
2023-05-17 10:52 ` [PATCH v2 9/9] wifi: ath10k: " Ilpo Järvinen
@ 2023-05-17 11:05 ` Kalle Valo
2023-05-24 15:10 ` Bjorn Helgaas
1 sibling, 0 replies; 11+ messages in thread
From: Kalle Valo @ 2023-05-17 11:05 UTC (permalink / raw)
To: Ilpo Järvinen
Cc: linux-pci, Bjorn Helgaas, Lorenzo Pieralisi, Rob Herring,
Krzysztof Wilczyński, Emmanuel Grumbach, Rafael J . Wysocki,
Heiner Kallweit, Lukas Wunner, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Michal Kazior, Janusz Dziedzic,
ath10k, linux-wireless, netdev, linux-kernel, Dean Luick,
Andy Shevchenko, stable
Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> writes:
> Don't assume that only the driver would be accessing LNKCTL. ASPM
> policy changes can trigger write to LNKCTL outside of driver's control.
>
> Use RMW capability accessors which does proper locking to avoid losing
> concurrent updates to the register value. On restore, clear the ASPMC
> field properly.
>
> Fixes: 76d870ed09ab ("ath10k: enable ASPM")
> Suggested-by: Lukas Wunner <lukas@wunner.de>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Cc: stable@vger.kernel.org
Acked-by: Kalle Valo <kvalo@kernel.org>
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 9/9] wifi: ath10k: Use RMW accessors for changing LNKCTL
2023-05-17 10:52 ` [PATCH v2 9/9] wifi: ath10k: " Ilpo Järvinen
2023-05-17 11:05 ` Kalle Valo
@ 2023-05-24 15:10 ` Bjorn Helgaas
2023-05-25 10:11 ` Ilpo Järvinen
1 sibling, 1 reply; 11+ messages in thread
From: Bjorn Helgaas @ 2023-05-24 15:10 UTC (permalink / raw)
To: Ilpo Järvinen
Cc: linux-pci, Bjorn Helgaas, Lorenzo Pieralisi, Rob Herring,
Krzysztof Wilczyński, Emmanuel Grumbach, Rafael J . Wysocki,
Heiner Kallweit, Lukas Wunner, Kalle Valo, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Michal Kazior,
Janusz Dziedzic, ath10k, linux-wireless, netdev, linux-kernel,
Dean Luick, Andy Shevchenko, stable
On Wed, May 17, 2023 at 01:52:35PM +0300, Ilpo Järvinen wrote:
> Don't assume that only the driver would be accessing LNKCTL. ASPM
> policy changes can trigger write to LNKCTL outside of driver's control.
>
> Use RMW capability accessors which does proper locking to avoid losing
> concurrent updates to the register value. On restore, clear the ASPMC
> field properly.
>
> Fixes: 76d870ed09ab ("ath10k: enable ASPM")
> Suggested-by: Lukas Wunner <lukas@wunner.de>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Cc: stable@vger.kernel.org
> ---
> drivers/net/wireless/ath/ath10k/pci.c | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
> index a7f44f6335fb..9275a672f90c 100644
> --- a/drivers/net/wireless/ath/ath10k/pci.c
> +++ b/drivers/net/wireless/ath/ath10k/pci.c
> @@ -1963,8 +1963,9 @@ static int ath10k_pci_hif_start(struct ath10k *ar)
> ath10k_pci_irq_enable(ar);
> ath10k_pci_rx_post(ar);
>
> - pcie_capability_write_word(ar_pci->pdev, PCI_EXP_LNKCTL,
> - ar_pci->link_ctl);
> + pcie_capability_clear_and_set_word(ar_pci->pdev, PCI_EXP_LNKCTL,
> + PCI_EXP_LNKCTL_ASPMC,
> + ar_pci->link_ctl & PCI_EXP_LNKCTL_ASPMC);
>
> return 0;
> }
> @@ -2821,8 +2822,8 @@ static int ath10k_pci_hif_power_up(struct ath10k *ar,
>
> pcie_capability_read_word(ar_pci->pdev, PCI_EXP_LNKCTL,
> &ar_pci->link_ctl);
> - pcie_capability_write_word(ar_pci->pdev, PCI_EXP_LNKCTL,
> - ar_pci->link_ctl & ~PCI_EXP_LNKCTL_ASPMC);
> + pcie_capability_clear_word(ar_pci->pdev, PCI_EXP_LNKCTL,
> + PCI_EXP_LNKCTL_ASPMC);
These ath drivers all have the form:
1) read LNKCTL
2) save LNKCTL value in ->link_ctl
3) write LNKCTL with "->link_ctl & ~PCI_EXP_LNKCTL_ASPMC"
to disable ASPM
4) write LNKCTL with ->link_ctl, presumably to re-enable ASPM
These patches close the hole between 1) and 3) where other LNKCTL
updates could interfere, which is definitely a good thing.
But the hole between 1) and 4) is much bigger and still there. Any
update by the PCI core in that interval would be lost.
Straw-man proposal:
- Change pci_disable_link_state() so it ignores aspm_disabled and
always disables ASPM even if platform firmware hasn't granted
ownership. Maybe this should warn and taint the kernel.
- Change drivers to use pci_disable_link_state() instead of writing
LNKCTL directly.
Bjorn
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 9/9] wifi: ath10k: Use RMW accessors for changing LNKCTL
2023-05-24 15:10 ` Bjorn Helgaas
@ 2023-05-25 10:11 ` Ilpo Järvinen
2023-05-26 11:48 ` Ilpo Järvinen
2023-05-26 22:20 ` Bjorn Helgaas
0 siblings, 2 replies; 11+ messages in thread
From: Ilpo Järvinen @ 2023-05-25 10:11 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: linux-pci, Bjorn Helgaas, Lorenzo Pieralisi, Rob Herring,
Krzysztof Wilczyński, Emmanuel Grumbach, Rafael J . Wysocki,
Heiner Kallweit, Lukas Wunner, Kalle Valo, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Michal Kazior,
Janusz Dziedzic, ath10k, linux-wireless, Netdev, LKML,
Dean Luick, Andy Shevchenko, stable
[-- Attachment #1: Type: text/plain, Size: 3897 bytes --]
On Wed, 24 May 2023, Bjorn Helgaas wrote:
> On Wed, May 17, 2023 at 01:52:35PM +0300, Ilpo Järvinen wrote:
> > Don't assume that only the driver would be accessing LNKCTL. ASPM
> > policy changes can trigger write to LNKCTL outside of driver's control.
> >
> > Use RMW capability accessors which does proper locking to avoid losing
> > concurrent updates to the register value. On restore, clear the ASPMC
> > field properly.
> >
> > Fixes: 76d870ed09ab ("ath10k: enable ASPM")
> > Suggested-by: Lukas Wunner <lukas@wunner.de>
> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > Cc: stable@vger.kernel.org
> > ---
> > drivers/net/wireless/ath/ath10k/pci.c | 9 +++++----
> > 1 file changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
> > index a7f44f6335fb..9275a672f90c 100644
> > --- a/drivers/net/wireless/ath/ath10k/pci.c
> > +++ b/drivers/net/wireless/ath/ath10k/pci.c
> > @@ -1963,8 +1963,9 @@ static int ath10k_pci_hif_start(struct ath10k *ar)
> > ath10k_pci_irq_enable(ar);
> > ath10k_pci_rx_post(ar);
> >
> > - pcie_capability_write_word(ar_pci->pdev, PCI_EXP_LNKCTL,
> > - ar_pci->link_ctl);
> > + pcie_capability_clear_and_set_word(ar_pci->pdev, PCI_EXP_LNKCTL,
> > + PCI_EXP_LNKCTL_ASPMC,
> > + ar_pci->link_ctl & PCI_EXP_LNKCTL_ASPMC);
> >
> > return 0;
> > }
> > @@ -2821,8 +2822,8 @@ static int ath10k_pci_hif_power_up(struct ath10k *ar,
> >
> > pcie_capability_read_word(ar_pci->pdev, PCI_EXP_LNKCTL,
> > &ar_pci->link_ctl);
> > - pcie_capability_write_word(ar_pci->pdev, PCI_EXP_LNKCTL,
> > - ar_pci->link_ctl & ~PCI_EXP_LNKCTL_ASPMC);
> > + pcie_capability_clear_word(ar_pci->pdev, PCI_EXP_LNKCTL,
> > + PCI_EXP_LNKCTL_ASPMC);
>
> These ath drivers all have the form:
>
> 1) read LNKCTL
> 2) save LNKCTL value in ->link_ctl
> 3) write LNKCTL with "->link_ctl & ~PCI_EXP_LNKCTL_ASPMC"
> to disable ASPM
> 4) write LNKCTL with ->link_ctl, presumably to re-enable ASPM
>
> These patches close the hole between 1) and 3) where other LNKCTL
> updates could interfere, which is definitely a good thing.
>
> But the hole between 1) and 4) is much bigger and still there. Any
> update by the PCI core in that interval would be lost.
Any update to PCI_EXP_LNKCTL_ASPMC field in that interval is lost yes, the
updates to _the other fields_ in LNKCTL are not lost.
I know this might result in drivers/pci/pcie/aspm.c disagreeing what
the state of the ASPM is (as shown under sysfs) compared with LNKCTL
value but the cause can no longer be due racing RMW. Essentially, 4) is
seen as an override to what core did if it changed ASPMC in between.
Technically, something is still "lost" like you say but for a different
reason than this series is trying to fix.
> Straw-man proposal:
>
> - Change pci_disable_link_state() so it ignores aspm_disabled and
> always disables ASPM even if platform firmware hasn't granted
> ownership. Maybe this should warn and taint the kernel.
>
> - Change drivers to use pci_disable_link_state() instead of writing
> LNKCTL directly.
I fully agree that's the direction we should be moving, yes. However, I'm
a bit hesitant to take that leap in one step. These drivers currently not
only disable ASPM but also re-enable it (assuming we guessed the intent
right).
If I directly implement that proposal, ASPM is not going to be re-enabled
when PCI core does not allowing it. Could it cause some power related
regression?
My plan is to make another patch series after these to realize exactly
what you're proposing. It would allow better to isolate the problems that
related to the lack of ASPM.
I hope this two step approach is an acceptable way forward? I can of
course add those patches on top of these if that would be preferrable.
--
i.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 9/9] wifi: ath10k: Use RMW accessors for changing LNKCTL
2023-05-25 10:11 ` Ilpo Järvinen
@ 2023-05-26 11:48 ` Ilpo Järvinen
2023-05-26 22:26 ` Bjorn Helgaas
2023-05-26 22:20 ` Bjorn Helgaas
1 sibling, 1 reply; 11+ messages in thread
From: Ilpo Järvinen @ 2023-05-26 11:48 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: linux-pci, Bjorn Helgaas, Lorenzo Pieralisi, Rob Herring,
Krzysztof Wilczyński, Emmanuel Grumbach, Rafael J . Wysocki,
Heiner Kallweit, Lukas Wunner, Kalle Valo, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Michal Kazior,
Janusz Dziedzic, ath10k, linux-wireless, Netdev, LKML,
Dean Luick, Andy Shevchenko, stable
[-- Attachment #1: Type: text/plain, Size: 4943 bytes --]
On Thu, 25 May 2023, Ilpo Järvinen wrote:
> On Wed, 24 May 2023, Bjorn Helgaas wrote:
>
> > On Wed, May 17, 2023 at 01:52:35PM +0300, Ilpo Järvinen wrote:
> > > Don't assume that only the driver would be accessing LNKCTL. ASPM
> > > policy changes can trigger write to LNKCTL outside of driver's control.
> > >
> > > Use RMW capability accessors which does proper locking to avoid losing
> > > concurrent updates to the register value. On restore, clear the ASPMC
> > > field properly.
> > >
> > > Fixes: 76d870ed09ab ("ath10k: enable ASPM")
> > > Suggested-by: Lukas Wunner <lukas@wunner.de>
> > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > > Cc: stable@vger.kernel.org
> > > ---
> > > drivers/net/wireless/ath/ath10k/pci.c | 9 +++++----
> > > 1 file changed, 5 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
> > > index a7f44f6335fb..9275a672f90c 100644
> > > --- a/drivers/net/wireless/ath/ath10k/pci.c
> > > +++ b/drivers/net/wireless/ath/ath10k/pci.c
> > > @@ -1963,8 +1963,9 @@ static int ath10k_pci_hif_start(struct ath10k *ar)
> > > ath10k_pci_irq_enable(ar);
> > > ath10k_pci_rx_post(ar);
> > >
> > > - pcie_capability_write_word(ar_pci->pdev, PCI_EXP_LNKCTL,
> > > - ar_pci->link_ctl);
> > > + pcie_capability_clear_and_set_word(ar_pci->pdev, PCI_EXP_LNKCTL,
> > > + PCI_EXP_LNKCTL_ASPMC,
> > > + ar_pci->link_ctl & PCI_EXP_LNKCTL_ASPMC);
> > >
> > > return 0;
> > > }
> > > @@ -2821,8 +2822,8 @@ static int ath10k_pci_hif_power_up(struct ath10k *ar,
> > >
> > > pcie_capability_read_word(ar_pci->pdev, PCI_EXP_LNKCTL,
> > > &ar_pci->link_ctl);
> > > - pcie_capability_write_word(ar_pci->pdev, PCI_EXP_LNKCTL,
> > > - ar_pci->link_ctl & ~PCI_EXP_LNKCTL_ASPMC);
> > > + pcie_capability_clear_word(ar_pci->pdev, PCI_EXP_LNKCTL,
> > > + PCI_EXP_LNKCTL_ASPMC);
> >
> > These ath drivers all have the form:
> >
> > 1) read LNKCTL
> > 2) save LNKCTL value in ->link_ctl
> > 3) write LNKCTL with "->link_ctl & ~PCI_EXP_LNKCTL_ASPMC"
> > to disable ASPM
> > 4) write LNKCTL with ->link_ctl, presumably to re-enable ASPM
> >
> > These patches close the hole between 1) and 3) where other LNKCTL
> > updates could interfere, which is definitely a good thing.
> >
> > But the hole between 1) and 4) is much bigger and still there. Any
> > update by the PCI core in that interval would be lost.
>
> Any update to PCI_EXP_LNKCTL_ASPMC field in that interval is lost yes, the
> updates to _the other fields_ in LNKCTL are not lost.
>
> I know this might result in drivers/pci/pcie/aspm.c disagreeing what
> the state of the ASPM is (as shown under sysfs) compared with LNKCTL
> value but the cause can no longer be due racing RMW. Essentially, 4) is
> seen as an override to what core did if it changed ASPMC in between.
> Technically, something is still "lost" like you say but for a different
> reason than this series is trying to fix.
>
> > Straw-man proposal:
> >
> > - Change pci_disable_link_state() so it ignores aspm_disabled and
> > always disables ASPM even if platform firmware hasn't granted
> > ownership. Maybe this should warn and taint the kernel.
> >
> > - Change drivers to use pci_disable_link_state() instead of writing
> > LNKCTL directly.
Now that I took a deeper look into what pci_disable_link_state() and
pci_enable_link_state() do, I realized they're not really disable/enable
pair like I had assumed from their names. Disable adds to ->aspm_disable
and flags are never removed from that because enable does not touch
aspm_disable at all but has it's own flag variable. This asymmetry looks
intentional.
So if ath drivers would do pci_disable_link_state() to realize 1)-3),
there is no way to undo it in 4). It looks as if ath drivers would
actually want to use pci_enable_link_state() with different state
parameters to realize what they want to do in 1)-4).
Any suggestion which way I should go with these ath drivers here, use
pci_enable_link_state()?
(There are other drivers where pci_disable_link_state() is very much valid
thing to do.)
--
i.
> I fully agree that's the direction we should be moving, yes. However, I'm
> a bit hesitant to take that leap in one step. These drivers currently not
> only disable ASPM but also re-enable it (assuming we guessed the intent
> right).
>
> If I directly implement that proposal, ASPM is not going to be re-enabled
> when PCI core does not allowing it. Could it cause some power related
> regression?
>
> My plan is to make another patch series after these to realize exactly
> what you're proposing. It would allow better to isolate the problems that
> related to the lack of ASPM.
>
> I hope this two step approach is an acceptable way forward? I can of
> course add those patches on top of these if that would be preferrable.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 9/9] wifi: ath10k: Use RMW accessors for changing LNKCTL
2023-05-26 11:48 ` Ilpo Järvinen
@ 2023-05-26 22:26 ` Bjorn Helgaas
0 siblings, 0 replies; 11+ messages in thread
From: Bjorn Helgaas @ 2023-05-26 22:26 UTC (permalink / raw)
To: Ilpo Järvinen
Cc: linux-pci, Bjorn Helgaas, Lorenzo Pieralisi, Rob Herring,
Krzysztof Wilczyński, Emmanuel Grumbach, Rafael J . Wysocki,
Heiner Kallweit, Lukas Wunner, Kalle Valo, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Michal Kazior,
Janusz Dziedzic, ath10k, linux-wireless, Netdev, LKML,
Dean Luick, Andy Shevchenko, stable
On Fri, May 26, 2023 at 02:48:44PM +0300, Ilpo Järvinen wrote:
> On Thu, 25 May 2023, Ilpo Järvinen wrote:
> > On Wed, 24 May 2023, Bjorn Helgaas wrote:
> > > On Wed, May 17, 2023 at 01:52:35PM +0300, Ilpo Järvinen wrote:
> > > > Don't assume that only the driver would be accessing LNKCTL. ASPM
> > > > policy changes can trigger write to LNKCTL outside of driver's control.
> > > >
> > > > Use RMW capability accessors which does proper locking to avoid losing
> > > > concurrent updates to the register value. On restore, clear the ASPMC
> > > > field properly.
> > > >
> > > > Fixes: 76d870ed09ab ("ath10k: enable ASPM")
> > > > Suggested-by: Lukas Wunner <lukas@wunner.de>
> > > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > > > Cc: stable@vger.kernel.org
> > > > ---
> > > > drivers/net/wireless/ath/ath10k/pci.c | 9 +++++----
> > > > 1 file changed, 5 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
> > > > index a7f44f6335fb..9275a672f90c 100644
> > > > --- a/drivers/net/wireless/ath/ath10k/pci.c
> > > > +++ b/drivers/net/wireless/ath/ath10k/pci.c
> > > > @@ -1963,8 +1963,9 @@ static int ath10k_pci_hif_start(struct ath10k *ar)
> > > > ath10k_pci_irq_enable(ar);
> > > > ath10k_pci_rx_post(ar);
> > > >
> > > > - pcie_capability_write_word(ar_pci->pdev, PCI_EXP_LNKCTL,
> > > > - ar_pci->link_ctl);
> > > > + pcie_capability_clear_and_set_word(ar_pci->pdev, PCI_EXP_LNKCTL,
> > > > + PCI_EXP_LNKCTL_ASPMC,
> > > > + ar_pci->link_ctl & PCI_EXP_LNKCTL_ASPMC);
> > > >
> > > > return 0;
> > > > }
> > > > @@ -2821,8 +2822,8 @@ static int ath10k_pci_hif_power_up(struct ath10k *ar,
> > > >
> > > > pcie_capability_read_word(ar_pci->pdev, PCI_EXP_LNKCTL,
> > > > &ar_pci->link_ctl);
> > > > - pcie_capability_write_word(ar_pci->pdev, PCI_EXP_LNKCTL,
> > > > - ar_pci->link_ctl & ~PCI_EXP_LNKCTL_ASPMC);
> > > > + pcie_capability_clear_word(ar_pci->pdev, PCI_EXP_LNKCTL,
> > > > + PCI_EXP_LNKCTL_ASPMC);
> > >
> > > These ath drivers all have the form:
> > >
> > > 1) read LNKCTL
> > > 2) save LNKCTL value in ->link_ctl
> > > 3) write LNKCTL with "->link_ctl & ~PCI_EXP_LNKCTL_ASPMC"
> > > to disable ASPM
> > > 4) write LNKCTL with ->link_ctl, presumably to re-enable ASPM
> > >
> > > These patches close the hole between 1) and 3) where other LNKCTL
> > > updates could interfere, which is definitely a good thing.
> > >
> > > But the hole between 1) and 4) is much bigger and still there. Any
> > > update by the PCI core in that interval would be lost.
> >
> > Any update to PCI_EXP_LNKCTL_ASPMC field in that interval is lost yes, the
> > updates to _the other fields_ in LNKCTL are not lost.
> >
> > I know this might result in drivers/pci/pcie/aspm.c disagreeing what
> > the state of the ASPM is (as shown under sysfs) compared with LNKCTL
> > value but the cause can no longer be due racing RMW. Essentially, 4) is
> > seen as an override to what core did if it changed ASPMC in between.
> > Technically, something is still "lost" like you say but for a different
> > reason than this series is trying to fix.
> >
> > > Straw-man proposal:
> > >
> > > - Change pci_disable_link_state() so it ignores aspm_disabled and
> > > always disables ASPM even if platform firmware hasn't granted
> > > ownership. Maybe this should warn and taint the kernel.
> > >
> > > - Change drivers to use pci_disable_link_state() instead of writing
> > > LNKCTL directly.
>
> Now that I took a deeper look into what pci_disable_link_state() and
> pci_enable_link_state() do, I realized they're not really disable/enable
> pair like I had assumed from their names. Disable adds to ->aspm_disable
> and flags are never removed from that because enable does not touch
> aspm_disable at all but has it's own flag variable. This asymmetry looks
> intentional.
Yes, that's an annoying feature. There's only one caller of
pci_enable_link_state(), so it may be possible to make this more
symmetric.
> So if ath drivers would do pci_disable_link_state() to realize 1)-3),
> there is no way to undo it in 4). It looks as if ath drivers would
> actually want to use pci_enable_link_state() with different state
> parameters to realize what they want to do in 1)-4).
Yeah, that does sound like a problem. I don't have any great ideas.
Bjorn
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 9/9] wifi: ath10k: Use RMW accessors for changing LNKCTL
2023-05-25 10:11 ` Ilpo Järvinen
2023-05-26 11:48 ` Ilpo Järvinen
@ 2023-05-26 22:20 ` Bjorn Helgaas
1 sibling, 0 replies; 11+ messages in thread
From: Bjorn Helgaas @ 2023-05-26 22:20 UTC (permalink / raw)
To: Ilpo Järvinen
Cc: linux-pci, Bjorn Helgaas, Lorenzo Pieralisi, Rob Herring,
Krzysztof Wilczyński, Emmanuel Grumbach, Rafael J . Wysocki,
Heiner Kallweit, Lukas Wunner, Kalle Valo, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Michal Kazior,
Janusz Dziedzic, ath10k, linux-wireless, Netdev, LKML,
Dean Luick, Andy Shevchenko, stable
On Thu, May 25, 2023 at 01:11:51PM +0300, Ilpo Järvinen wrote:
> On Wed, 24 May 2023, Bjorn Helgaas wrote:
> > On Wed, May 17, 2023 at 01:52:35PM +0300, Ilpo Järvinen wrote:
> > > Don't assume that only the driver would be accessing LNKCTL. ASPM
> > > policy changes can trigger write to LNKCTL outside of driver's control.
> > >
> > > Use RMW capability accessors which does proper locking to avoid losing
> > > concurrent updates to the register value. On restore, clear the ASPMC
> > > field properly.
> > >
> > > Fixes: 76d870ed09ab ("ath10k: enable ASPM")
> > > Suggested-by: Lukas Wunner <lukas@wunner.de>
> > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > > Cc: stable@vger.kernel.org
> > > ---
> > > drivers/net/wireless/ath/ath10k/pci.c | 9 +++++----
> > > 1 file changed, 5 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
> > > index a7f44f6335fb..9275a672f90c 100644
> > > --- a/drivers/net/wireless/ath/ath10k/pci.c
> > > +++ b/drivers/net/wireless/ath/ath10k/pci.c
> > > @@ -1963,8 +1963,9 @@ static int ath10k_pci_hif_start(struct ath10k *ar)
> > > ath10k_pci_irq_enable(ar);
> > > ath10k_pci_rx_post(ar);
> > >
> > > - pcie_capability_write_word(ar_pci->pdev, PCI_EXP_LNKCTL,
> > > - ar_pci->link_ctl);
> > > + pcie_capability_clear_and_set_word(ar_pci->pdev, PCI_EXP_LNKCTL,
> > > + PCI_EXP_LNKCTL_ASPMC,
> > > + ar_pci->link_ctl & PCI_EXP_LNKCTL_ASPMC);
> > >
> > > return 0;
> > > }
> > > @@ -2821,8 +2822,8 @@ static int ath10k_pci_hif_power_up(struct ath10k *ar,
> > >
> > > pcie_capability_read_word(ar_pci->pdev, PCI_EXP_LNKCTL,
> > > &ar_pci->link_ctl);
> > > - pcie_capability_write_word(ar_pci->pdev, PCI_EXP_LNKCTL,
> > > - ar_pci->link_ctl & ~PCI_EXP_LNKCTL_ASPMC);
> > > + pcie_capability_clear_word(ar_pci->pdev, PCI_EXP_LNKCTL,
> > > + PCI_EXP_LNKCTL_ASPMC);
> >
> > These ath drivers all have the form:
> >
> > 1) read LNKCTL
> > 2) save LNKCTL value in ->link_ctl
> > 3) write LNKCTL with "->link_ctl & ~PCI_EXP_LNKCTL_ASPMC"
> > to disable ASPM
> > 4) write LNKCTL with ->link_ctl, presumably to re-enable ASPM
> >
> > These patches close the hole between 1) and 3) where other LNKCTL
> > updates could interfere, which is definitely a good thing.
> >
> > But the hole between 1) and 4) is much bigger and still there. Any
> > update by the PCI core in that interval would be lost.
>
> Any update to PCI_EXP_LNKCTL_ASPMC field in that interval is lost yes, the
> updates to _the other fields_ in LNKCTL are not lost.
Ah, yes, you're right, I missed the masking to PCI_EXP_LNKCTL_ASPMC in
the pcie_capability_clear_word().
> > Straw-man proposal:
> >
> > - Change pci_disable_link_state() so it ignores aspm_disabled and
> > always disables ASPM even if platform firmware hasn't granted
> > ownership. Maybe this should warn and taint the kernel.
> >
> > - Change drivers to use pci_disable_link_state() instead of writing
> > LNKCTL directly.
>
> I fully agree that's the direction we should be moving, yes. However, I'm
> a bit hesitant to take that leap in one step. These drivers currently not
> only disable ASPM but also re-enable it (assuming we guessed the intent
> right).
>
> If I directly implement that proposal, ASPM is not going to be re-enabled
> when PCI core does not allowing it. Could it cause some power related
> regression?
IIUC the potential problem only happens with:
- A platform that enables ASPM but doesn't grant PCIe Capability
ownership to the OS, and
- A device where we force-disable ASPM, presumably to avoid some
hardware defect.
I'm not sure this case is worth worrying about. A platform that
enables ASPM without allowing the OS to disable it is taking a risk
because it can't know about these device defects or even about user
preferences. A device that has an ASPM-related defect may use more
power than necessary. I think that's to be expected.
> My plan is to make another patch series after these to realize exactly
> what you're proposing. It would allow better to isolate the problems that
> related to the lack of ASPM.
>
> I hope this two step approach is an acceptable way forward? I can of
> course add those patches on top of these if that would be preferrable.
I think two steps is OK. It's a little more work for the driver
maintainers to review them, but this step is pretty trivial already
reviewed (except for the GPUs, which are probably the most important :)).
Bjorn
^ permalink raw reply [flat|nested] 11+ messages in thread