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:
- read LNKCTL
- save LNKCTL value in ->link_ctl
- write LNKCTL with "->link_ctl & ~PCI_EXP_LNKCTL_ASPMC" to disable ASPM
- 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