On Mon, Jul 21, 2025 at 02:28:24PM GMT, Ilpo Järvinen wrote:
> On Mon, 21 Jul 2025, Manivannan Sadhasivam wrote:
>
> > On Mon, Jul 21, 2025 at 01:09:05PM GMT, Ilpo Järvinen wrote:
> > > On Mon, 21 Jul 2025, Manivannan Sadhasivam wrote:
> > > > On Mon, Jul 21, 2025 at 11:04:10AM GMT, Ilpo Järvinen wrote:
> > > > > On Wed, 16 Jul 2025, Manivannan Sadhasivam via B4 Relay wrote:
> > > > > > From: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxxxxxxxx>
> > > > > >
> > > > > > It is not recommended to enable/disable the ASPM states on the back of the
> > > > > > PCI core directly using the LNKCTL register. It will break the PCI core's
> > > > > > knowledge about the device ASPM states. So use the APIs exposed by the PCI
> > > > > > core to enable/disable ASPM states.
> > > > > >
> > > > > > Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3
> > > > > >
> > > > > > Reported-by: Qiang Yu <qiang.yu@xxxxxxxxxxxxxxxx>
> > > > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxxxxxxxx>
> > > > > > ---
> > > > > > drivers/net/wireless/ath/ath.h | 14 ++++++++++++++
> > > > > > drivers/net/wireless/ath/ath12k/pci.c | 10 ++++------
> > > > > > 2 files changed, 18 insertions(+), 6 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/net/wireless/ath/ath.h b/drivers/net/wireless/ath/ath.h
> > > > > > index 34654f710d8a1e63f65a47d4602e2035262a4d9e..ef685123b66bf4f41428fec67c1967f242a9ef27 100644
> > > > > > --- a/drivers/net/wireless/ath/ath.h
> > > > > > +++ b/drivers/net/wireless/ath/ath.h
> > > > > > @@ -21,6 +21,8 @@
> > > > > > #include <linux/skbuff.h>
> > > > > > #include <linux/if_ether.h>
> > > > > > #include <linux/spinlock.h>
> > > > > > +#include <linux/pci.h>
> > > > > > +#include <linux/pci_regs.h>
> > > > > > #include <net/mac80211.h>
> > > > > >
> > > > > > /*
> > > > > > @@ -336,4 +338,16 @@ static inline const char *ath_bus_type_to_string(enum ath_bus_type bustype)
> > > > > > return ath_bus_type_strings[bustype];
> > > > > > }
> > > > > >
> > > > > > +static inline int ath_pci_aspm_state(u16 lnkctl)
> > > > > > +{
> > > > > > + int state = 0;
> > > > > > +
> > > > > > + if (lnkctl & PCI_EXP_LNKCTL_ASPM_L0S)
> > > > > > + state |= PCIE_LINK_STATE_L0S;
> > > > > > + if (lnkctl & PCI_EXP_LNKCTL_ASPM_L1)
> > > > > > + state |= PCIE_LINK_STATE_L1;
> > > > > > +
> > > > > > + return state;
> > > > > > +}
> > > > > > +
> > > > > > #endif /* ATH_H */
> > > > > > diff --git a/drivers/net/wireless/ath/ath12k/pci.c b/drivers/net/wireless/ath/ath12k/pci.c
> > > > > > index 489d546390fcdab8f615cc9184006a958d9f140a..a5e11509e3ab8faad6638ff78ce6a8a5e9c3cbbd 100644
> > > > > > --- a/drivers/net/wireless/ath/ath12k/pci.c
> > > > > > +++ b/drivers/net/wireless/ath/ath12k/pci.c
> > > > > > @@ -16,6 +16,8 @@
> > > > > > #include "mhi.h"
> > > > > > #include "debug.h"
> > > > > >
> > > > > > +#include "../ath.h"
> > > > > > +
> > > > > > #define ATH12K_PCI_BAR_NUM 0
> > > > > > #define ATH12K_PCI_DMA_MASK 36
> > > > > >
> > > > > > @@ -928,8 +930,7 @@ 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_clear_word(ab_pci->pdev, PCI_EXP_LNKCTL,
> > > > > > - PCI_EXP_LNKCTL_ASPMC);
> > > > > > + pci_disable_link_state(ab_pci->pdev, PCIE_LINK_STATE_L0S | PCIE_LINK_STATE_L1);
> > > > >
> > > > > I'd remove to comment too as the code is self-explanatory after this
> > > > > change.
> > > > >
> > > >
> > > > Ack
> > > >
> > > > > >
> > > > > > set_bit(ATH12K_PCI_ASPM_RESTORE, &ab_pci->flags);
> > > > > > }
> > > > > > @@ -958,10 +959,7 @@ static void ath12k_pci_aspm_restore(struct ath12k_pci *ab_pci)
> > > > > > {
> > > > > > if (ab_pci->ab->hw_params->supports_aspm &&
> > > > > > test_and_clear_bit(ATH12K_PCI_ASPM_RESTORE, &ab_pci->flags))
> > > > > > - pcie_capability_clear_and_set_word(ab_pci->pdev, PCI_EXP_LNKCTL,
> > > > > > - PCI_EXP_LNKCTL_ASPMC,
> > > > > > - ab_pci->link_ctl &
> > > > > > - PCI_EXP_LNKCTL_ASPMC);
> > > > > > + pci_enable_link_state(ab_pci->pdev, ath_pci_aspm_state(ab_pci->link_ctl));
> > > > > > }
> > > > > >
> > > > > > static void ath12k_pci_cancel_workqueue(struct ath12k_base *ab)
> > > > >
> > > > > As you now depend on ASPM driver being there, these should also add to
> > > > > Kconfig:
> > > > >
> > > > > depends on PCIEASPM
> > > > >
> > > >
> > > > I thought about it, but since this driver doesn't necessarily enable ASPM for
> > > > all the devices it supports, I didn't add the dependency. But looking at it
> > > > again, I think makes sense to add the dependency since the driver cannot work
> > > > reliably without disabling ASPM (for the supported devices).
> > >
> > > PCIEASPM is already default y and if EXPERT so it is not something
> > > that is expected to be disabled.
> > >
> > > You also no longer need to move the ASPM link state defines LKP found out
> > > about after adding the depends on.
> > >
> >
> > Yes, it will fix the reported issue, but guarding the definitions feels wrong to
> > me still. Maybe that's something we can worry later.
> >
> > > I'm a bit worried this series will regress in the cases where OS doesn't
> > > control ASPM so it might be necessary to include something along the
> > > lines of the patch below too (the earlier discussion on this is in Link
> > > tags):
> > >
> >
> > atheros drivers didn't have such comment (why they were manually changing the
> > LNKCTL register), but I agree that there is a chance that they could cause issue
> > on platforms where BIOS didn't give ASPM control to the OS.
> >
> > But as a non-ACPI developer, I don't know what does 'ACPI doesn't give
> > permission to manage ASPM' mean exactly. Does ACPI allow to disable ASPM but not
> > enable it?
>
> While others are likely better qualified to answer this, my impression is
> that even disabling ASPM is not allowed when OS does has not been granted
> control over ASPM (OS should not change the value of ASPM Control in
> LNKCTL at all).
>
> The obvious trouble then is, if a driver/hw needs ASPM to be disabled over
> certain period of its operation or entirely to ensure stable operation,
> and ASPM is enabled, we're between a rock and a hard place when changes to
> ASPM Control field are disallowed.
>
> Because the ASPM driver took a hard stance of conformance here and did
> not let touching the ASPM Control field, we ended up having drivers that
> then write ASPM Control on their own to work around HW problems (see e.g.
> the comments in drivers/net/ethernet/intel/e1000e/netdev.c that make it
> very clear it was intentional from the driver) so it was considered that
> allowing disabling ASPM might be acceptable compromise over drivers doing
> it on their own (and leaving the ASPM driver out of the loop because it
> cannot be relied to disable ASPM consistently in all cases).
>
Since there was no comments from ACPI folks, I will go ahead with your patch. I
will also CC them in the next version so that they can yell at me if they want.
- Mani
--
மணிவண்ணன் சதாசிவம்
