Re: [PATCH net-next 1/2] PCI: let pci_disable_link_state propagate errors

[Bjorn Helgaas <helgaas@kernel.org>]

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