Re: [PATCH v2 9/9] wifi: ath10k: Use RMW accessors for changing LNKCTL

From: "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>
To: Bjorn Helgaas <helgaas@kernel.org>
Cc: linux-pci@vger.kernel.org, "Bjorn Helgaas" <bhelgaas@google.com>,
	"Lorenzo Pieralisi" <lorenzo.pieralisi@arm.com>,
	"Rob Herring" <robh@kernel.org>,
	"Krzysztof Wilczyński" <kw@linux.com>,
	"Emmanuel Grumbach" <emmanuel.grumbach@intel.com>,
	"Rafael J . Wysocki" <rafael@kernel.org>,
	"Heiner Kallweit" <hkallweit1@gmail.com>,
	"Lukas Wunner" <lukas@wunner.de>, "Kalle Valo" <kvalo@kernel.org>,
	"David S. Miller" <davem@davemloft.net>,
	"Eric Dumazet" <edumazet@google.com>,
	"Jakub Kicinski" <kuba@kernel.org>,
	"Paolo Abeni" <pabeni@redhat.com>,
	"Michal Kazior" <michal.kazior@tieto.com>,
	"Janusz Dziedzic" <janusz.dziedzic@tieto.com>,
	ath10k@lists.infradead.org, linux-wireless@vger.kernel.org,
	Netdev <netdev@vger.kernel.org>,
	LKML <linux-kernel@vger.kernel.org>,
	"Dean Luick" <dean.luick@cornelisnetworks.com>,
	"Andy Shevchenko" <andriy.shevchenko@linux.intel.com>,
	stable@vger.kernel.org
Subject: Re: [PATCH v2 9/9] wifi: ath10k: Use RMW accessors for changing LNKCTL
Date: Thu, 25 May 2023 13:11:51 +0300 (EEST)	[thread overview]
Message-ID: <ecdc8e85-786-db97-a7d4-bfd82c08714@linux.intel.com> (raw)
In-Reply-To: <ZG4o/pYseBklnrTc@bhelgaas>

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