From: Bjorn Helgaas <helgaas@kernel.org>
To: "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>
Cc: linux-pci@vger.kernel.org,
"Lorenzo Pieralisi" <lorenzo.pieralisi@arm.com>,
"Rob Herring" <robh@kernel.org>,
"Krzysztof Wilczyński" <kw@linux.com>,
"Lukas Wunner" <lukas@wunner.de>,
"Rafael J . Wysocki" <rafael@kernel.org>,
"Heiner Kallweit" <hkallweit1@gmail.com>,
"Emmanuel Grumbach" <emmanuel.grumbach@intel.com>,
linux-kernel@vger.kernel.org,
"Bjorn Helgaas" <bhelgaas@google.com>,
ath10k@lists.infradead.org, ath11k@lists.infradead.org,
ath12k@lists.infradead.org, intel-wired-lan@lists.osuosl.org,
linux-arm-kernel@lists.infradead.org,
linux-bluetooth@vger.kernel.org,
linux-mediatek@lists.infradead.org, linux-rdma@vger.kernel.org,
linux-wireless@vger.kernel.org, netdev@vger.kernel.org
Subject: Re: [PATCH v2 03/13] PCI/ASPM: Disable ASPM when driver requests it
Date: Wed, 11 Oct 2023 15:04:42 -0500 [thread overview]
Message-ID: <20231011200442.GA1040348@bhelgaas> (raw)
In-Reply-To: <20230918131103.24119-4-ilpo.jarvinen@linux.intel.com>
On Mon, Sep 18, 2023 at 04:10:53PM +0300, Ilpo Järvinen wrote:
> PCI core/ASPM service driver allows controlling ASPM state through
> pci_disable_link_state() and pci_enable_link_state() API. It was
> decided earlier (see the Link below), to not allow ASPM changes when OS
> does not have control over it but only log a warning about the problem
> (commit 2add0ec14c25 ("PCI/ASPM: Warn when driver asks to disable ASPM,
> but we can't do it")). Similarly, if ASPM is not enabled through
> config, ASPM cannot be disabled.
>
> A number of drivers have added workarounds to force ASPM off with own
> writes into the Link Control Register (some even with comments
> explaining why PCI core does not disable it under some circumstances).
> According to the comments, some drivers require ASPM to be off for
> reliable operation.
>
> Having custom ASPM handling in drivers is problematic because the state
> kept in the ASPM service driver is not updated by the changes made
> outside the link state management API.
>
> As the first step to address this issue, make pci_disable_link_state()
> to unconditionally disable ASPM so the motivation for drivers to come
> up with custom ASPM handling code is eliminated.
>
> Place the minimal ASPM disable handling into own file as it is too
> complicated to fit into a header as static inline and it has almost no
> overlap with the existing, more complicated ASPM code in
> drivers/pci/pce/aspm.c.
>
> Make pci_disable_link_state() function comment to comply kerneldoc
> formatting while changing the description.
>
> Link: https://lore.kernel.org/all/CANUX_P3F5YhbZX3WGU-j1AGpbXb_T9Bis2ErhvKkFMtDvzatVQ@mail.gmail.com/
> Link: https://lore.kernel.org/all/20230511131441.45704-1-ilpo.jarvinen@linux.intel.com/
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
> drivers/pci/pcie/Makefile | 1 +
> drivers/pci/pcie/aspm.c | 33 ++++++++++-------
> drivers/pci/pcie/aspm_minimal.c | 66 +++++++++++++++++++++++++++++++++
> include/linux/pci.h | 6 +--
> 4 files changed, 88 insertions(+), 18 deletions(-)
> create mode 100644 drivers/pci/pcie/aspm_minimal.c
>
> diff --git a/drivers/pci/pcie/Makefile b/drivers/pci/pcie/Makefile
> index 8de4ed5f98f1..ec7f04037b01 100644
> --- a/drivers/pci/pcie/Makefile
> +++ b/drivers/pci/pcie/Makefile
> @@ -6,6 +6,7 @@ pcieportdrv-y := portdrv.o rcec.o
>
> obj-$(CONFIG_PCIEPORTBUS) += pcieportdrv.o
>
> +obj-y += aspm_minimal.o
Can we put this code in drivers/pci/pci.c instead of creating a new
file for it? pci.c is kind of a dumping ground and isn't ideal
either, but we do have a few other things there that we *always* want
even though they're related to a separate Kconfig feature, e.g.,
pci_bridge_reconfigure_ltr(), pcie_clear_device_status(),
pcie_clear_root_pme_status().
> obj-$(CONFIG_PCIEASPM) += aspm.o
Or maybe it would be better to just put it in aspm.c, drop this
compilation guard, and wrap the rest of the file in #ifdef
CONFIG_PCIEASPM. Then everything would be in one file, which is a
major boon for code readers.
What do you think?
> obj-$(CONFIG_PCIEAER) += aer.o err.o
> obj-$(CONFIG_PCIEAER_INJECT) += aer_inject.o
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 860bc94974ec..ec6d7a092ac1 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -1042,16 +1042,23 @@ static int __pci_disable_link_state(struct pci_dev *pdev, int state, bool sem)
> return -EINVAL;
> /*
> * A driver requested that ASPM be disabled on this device, but
> - * if we don't have permission to manage ASPM (e.g., on ACPI
> + * if we might not have permission to manage ASPM (e.g., on ACPI
> * systems we have to observe the FADT ACPI_FADT_NO_ASPM bit and
> - * the _OSC method), we can't honor that request. Windows has
> - * a similar mechanism using "PciASPMOptOut", which is also
> - * ignored in this situation.
> + * the _OSC method), previously we chose to not honor disable
> + * request in that case. Windows has a similar mechanism using
> + * "PciASPMOptOut", which is also ignored in this situation.
> + *
> + * Not honoring the requests to disable ASPM, however, led to
> + * drivers forcing ASPM off on their own. As such changes of ASPM
> + * state are not tracked by this service driver, the state kept here
> + * became out of sync.
> + *
> + * Therefore, honor ASPM disable requests even when OS does not have
> + * ASPM control. Plain disable for ASPM is assumed to be slightly
> + * safer than fully managing it.
> */
> - if (aspm_disabled) {
> - pci_warn(pdev, "can't disable ASPM; OS doesn't have ASPM control\n");
> - return -EPERM;
> - }
> + if (aspm_disabled)
> + pci_warn(pdev, "OS doesn't have ASPM control, disabling ASPM anyway\n");
I think this is better than the previous situation, but I think we
should taint the kernel here because it's possible the firmware had a
reason for retaining ASPM control, so we might be stepping on
something. Arguably the message is already enough of a signal, but
checking for a taint is potentially a little more automatable.
> +int pci_disable_link_state_locked(struct pci_dev *pdev, int state)
> +{
> + struct pci_dev *parent = pdev->bus->self;
> + struct pci_bus *linkbus = pdev->bus;
> + struct pci_dev *child;
> + u16 aspm_enabled, linkctl;
> + int ret;
> +
> + if (!parent)
> + return -ENODEV;
> +
> + ret = pcie_capability_read_word(parent, PCI_EXP_LNKCTL, &linkctl);
> + if (ret != PCIBIOS_SUCCESSFUL)
> + return pcibios_err_to_errno(ret);
> + aspm_enabled = linkctl & PCI_EXP_LNKCTL_ASPMC;
In this case, we don't care about the shift offset of the
PCI_EXP_LNKCTL_ASPMC bitfield, but if we use FIELD_GET() in most/all
other cases where we look at PCI_EXP_LNKCTL, maybe it would be worth
using it here as well?
Tangent, but I'm always dubious about the idea that e1000e is so
special that only there do we need the "_locked" variant of this
function. No suggestion though; no need to do anything about it in
this series ;)
> + ret = pcie_capability_read_word(pdev, PCI_EXP_LNKCTL, &linkctl);
> + if (ret != PCIBIOS_SUCCESSFUL)
> + return pcibios_err_to_errno(ret);
> + aspm_enabled |= linkctl & PCI_EXP_LNKCTL_ASPMC;
> +
> + /* If no states need to be disabled, don't touch LNKCTL */
> + if (state & aspm_enabled)
> + return 0;
> +
> + ret = pcie_capability_clear_word(parent, PCI_EXP_LNKCTL, PCI_EXP_LNKCTL_ASPMC);
> + if (ret != PCIBIOS_SUCCESSFUL)
> + return pcibios_err_to_errno(ret);
> + list_for_each_entry(child, &linkbus->devices, bus_list)
> + pcie_capability_clear_word(child, PCI_EXP_LNKCTL, PCI_EXP_LNKCTL_ASPMC);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(pci_disable_link_state_locked);
_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
next prev parent reply other threads:[~2023-10-11 20:05 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-09-18 13:10 [PATCH v2 00/13] PCI/ASPM: Make ASPM in core robust and remove driver workarounds Ilpo Järvinen
2023-09-18 13:10 ` [PATCH v2 01/13] PCI/ASPM: Rename pci_enable_link_state() to pci_set_default_link_state() Ilpo Järvinen
2023-09-18 13:10 ` [PATCH v2 02/13] PCI/ASPM: Improve pci_set_default_link_state() kerneldoc Ilpo Järvinen
2023-09-18 13:10 ` [PATCH v2 03/13] PCI/ASPM: Disable ASPM when driver requests it Ilpo Järvinen
2023-10-11 20:04 ` Bjorn Helgaas [this message]
2023-10-12 10:47 ` Ilpo Järvinen
2023-10-11 21:22 ` Bjorn Helgaas
2023-10-12 10:56 ` Ilpo Järvinen
2023-10-13 16:42 ` Bjorn Helgaas
2023-10-16 14:27 ` Ilpo Järvinen
2023-10-26 22:02 ` Bjorn Helgaas
2023-09-18 13:10 ` [PATCH v2 04/13] PCI/ASPM: Move L0S/L1/sub states mask calculation into a helper Ilpo Järvinen
2023-10-11 19:32 ` Bjorn Helgaas
2023-10-12 10:29 ` Ilpo Järvinen
2023-09-18 13:10 ` [PATCH v2 05/13] PCI/ASPM: Add pci_enable_link_state() Ilpo Järvinen
2023-10-11 21:53 ` Bjorn Helgaas
2023-10-12 12:53 ` Ilpo Järvinen
2023-10-13 16:48 ` Bjorn Helgaas
2023-10-16 12:57 ` Ilpo Järvinen
2023-09-18 13:10 ` [PATCH v2 06/13] Bluetooth: hci_bcm4377: Convert aspm disable to quirk Ilpo Järvinen
2023-09-19 13:36 ` Sven Peter
2023-09-18 13:10 ` [PATCH v2 07/13] mt76: Remove unreliable pci_disable_link_state() workaround Ilpo Järvinen
2023-09-18 13:10 ` [PATCH v2 08/13] e1000e: Remove unreliable pci_disable_link_state{,_locked}() workaround Ilpo Järvinen
2023-09-18 13:10 ` [PATCH v2 09/13] wifi: ath10k: Use pci_disable/enable_link_state() Ilpo Järvinen
2023-09-19 9:39 ` Kalle Valo
2023-09-18 13:11 ` [PATCH v2 10/13] wifi: ath11k: " Ilpo Järvinen
2023-09-19 9:40 ` Kalle Valo
2023-09-18 13:11 ` [PATCH v2 11/13] wifi: ath12k: " Ilpo Järvinen
2023-09-19 9:40 ` Kalle Valo
2023-09-18 13:11 ` [PATCH v2 12/13] RDMA/hfi1: " Ilpo Järvinen
2023-09-18 13:11 ` [PATCH v2 13/13] misc: rtsx: " Ilpo Järvinen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20231011200442.GA1040348@bhelgaas \
--to=helgaas@kernel.org \
--cc=ath10k@lists.infradead.org \
--cc=ath11k@lists.infradead.org \
--cc=ath12k@lists.infradead.org \
--cc=bhelgaas@google.com \
--cc=emmanuel.grumbach@intel.com \
--cc=hkallweit1@gmail.com \
--cc=ilpo.jarvinen@linux.intel.com \
--cc=intel-wired-lan@lists.osuosl.org \
--cc=kw@linux.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-bluetooth@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mediatek@lists.infradead.org \
--cc=linux-pci@vger.kernel.org \
--cc=linux-rdma@vger.kernel.org \
--cc=linux-wireless@vger.kernel.org \
--cc=lorenzo.pieralisi@arm.com \
--cc=lukas@wunner.de \
--cc=netdev@vger.kernel.org \
--cc=rafael@kernel.org \
--cc=robh@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).