From: Bjorn Helgaas <helgaas@kernel.org>
To: Heiner Kallweit <hkallweit1@gmail.com>
Cc: Realtek linux nic maintainers <nic_swsd@realtek.com>,
David Miller <davem@davemloft.net>,
"linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
"netdev@vger.kernel.org" <netdev@vger.kernel.org>
Subject: Re: [PATCH net-next 1/2] PCI: let pci_disable_link_state propagate errors
Date: Wed, 19 Jun 2019 16:32:39 -0500 [thread overview]
Message-ID: <20190619213238.GD143205@google.com> (raw)
In-Reply-To: <604f2954-c60c-d2aa-3849-9a2f8872001c@gmail.com>
On Tue, Jun 18, 2019 at 11:13:48PM +0200, Heiner Kallweit wrote:
> Drivers may rely on pci_disable_link_state() having disabled certain
> ASPM link states. If OS can't control ASPM then pci_disable_link_state()
> turns into a no-op w/o informing the caller. The driver therefore may
> falsely assume the respective ASPM link states are disabled.
> Let pci_disable_link_state() propagate errors to the caller, enabling
> the caller to react accordingly.
>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
Acked-by: Bjorn Helgaas <bhelgaas@google.com>
Thanks, I think this makes good sense.
> ---
> drivers/pci/pcie/aspm.c | 20 +++++++++++---------
> include/linux/pci-aspm.h | 7 ++++---
> 2 files changed, 15 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index fd4cb7508..e44af7f4d 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -1062,18 +1062,18 @@ void pcie_aspm_powersave_config_link(struct pci_dev *pdev)
> up_read(&pci_bus_sem);
> }
>
> -static void __pci_disable_link_state(struct pci_dev *pdev, int state, bool sem)
> +static int __pci_disable_link_state(struct pci_dev *pdev, int state, bool sem)
> {
> struct pci_dev *parent = pdev->bus->self;
> struct pcie_link_state *link;
>
> if (!pci_is_pcie(pdev))
> - return;
> + return 0;
>
> if (pdev->has_secondary_link)
> parent = pdev;
> if (!parent || !parent->link_state)
> - return;
> + return -EINVAL;
>
> /*
> * A driver requested that ASPM be disabled on this device, but
> @@ -1085,7 +1085,7 @@ static void __pci_disable_link_state(struct pci_dev *pdev, int state, bool sem)
> */
> if (aspm_disabled) {
> pci_warn(pdev, "can't disable ASPM; OS doesn't have ASPM control\n");
> - return;
> + return -EPERM;
> }
>
> if (sem)
> @@ -1105,11 +1105,13 @@ static void __pci_disable_link_state(struct pci_dev *pdev, int state, bool sem)
> mutex_unlock(&aspm_lock);
> if (sem)
> up_read(&pci_bus_sem);
> +
> + return 0;
> }
>
> -void pci_disable_link_state_locked(struct pci_dev *pdev, int state)
> +int pci_disable_link_state_locked(struct pci_dev *pdev, int state)
> {
> - __pci_disable_link_state(pdev, state, false);
> + return __pci_disable_link_state(pdev, state, false);
> }
> EXPORT_SYMBOL(pci_disable_link_state_locked);
>
> @@ -1117,14 +1119,14 @@ EXPORT_SYMBOL(pci_disable_link_state_locked);
> * pci_disable_link_state - Disable device's link state, so the link will
> * never enter specific states. Note that if the BIOS didn't grant ASPM
> * control to the OS, this does nothing because we can't touch the LNKCTL
> - * register.
> + * register. Returns 0 or a negative errno.
> *
> * @pdev: PCI device
> * @state: ASPM link state to disable
> */
> -void pci_disable_link_state(struct pci_dev *pdev, int state)
> +int pci_disable_link_state(struct pci_dev *pdev, int state)
> {
> - __pci_disable_link_state(pdev, state, true);
> + return __pci_disable_link_state(pdev, state, true);
> }
> EXPORT_SYMBOL(pci_disable_link_state);
>
> diff --git a/include/linux/pci-aspm.h b/include/linux/pci-aspm.h
> index df28af5ce..67064145d 100644
> --- a/include/linux/pci-aspm.h
> +++ b/include/linux/pci-aspm.h
> @@ -24,11 +24,12 @@
> #define PCIE_LINK_STATE_CLKPM 4
>
> #ifdef CONFIG_PCIEASPM
> -void pci_disable_link_state(struct pci_dev *pdev, int state);
> -void pci_disable_link_state_locked(struct pci_dev *pdev, int state);
> +int pci_disable_link_state(struct pci_dev *pdev, int state);
> +int pci_disable_link_state_locked(struct pci_dev *pdev, int state);
> void pcie_no_aspm(void);
> #else
> -static inline void pci_disable_link_state(struct pci_dev *pdev, int state) { }
> +static inline int pci_disable_link_state(struct pci_dev *pdev, int state)
> +{ return 0; }
> static inline void pcie_no_aspm(void) { }
> #endif
>
> --
> 2.22.0
>
>
next prev parent reply other threads:[~2019-06-19 21:32 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-06-18 21:12 [PATCH net-next 0/2] PCI: let pci_disable_link_state propagate errors Heiner Kallweit
2019-06-18 21:13 ` [PATCH net-next 1/2] " Heiner Kallweit
2019-06-19 21:32 ` Bjorn Helgaas [this message]
2019-06-18 21:14 ` [PATCH net-next 2/2] r8169: don't activate ASPM in chip if OS can't control ASPM Heiner Kallweit
2019-06-19 21:26 ` [PATCH net-next 0/2] PCI: let pci_disable_link_state propagate errors David Miller
2019-06-22 2:06 ` David Miller
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=20190619213238.GD143205@google.com \
--to=helgaas@kernel.org \
--cc=davem@davemloft.net \
--cc=hkallweit1@gmail.com \
--cc=linux-pci@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=nic_swsd@realtek.com \
/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).