Re: [PATCH] PCI: Remove not needed check in disable aspm link

[Bjorn Helgaas]

On Mon, Mar 18, 2013 at 11:37 AM, Yinghai Lu <yinghai@xxxxxxxxxx> wrote:
> Roman reported ath5k does not work anymore on 3.8.
> Bisected to
> | commit 8c33f51df406e1a1f7fa4e9b244845b7ebd61fa6
> | Author: Taku Izumi <izumi.taku@xxxxxxxxxxxxxx>
> | Date:   Tue Oct 30 15:27:13 2012 +0900
> |
> |    PCI/ACPI: Request _OSC control before scanning PCI root bus
> |
> |    This patch moves up the code block to request _OSC control in order to
> |    separate ACPI work and PCI work in acpi_pci_root_add().
>
> It make pci_disable_link_state does not work anymore as acpi_disabled
> is set before pci root bus scanning.
> It will skip that in quirks and pcie_aspm_sanity_check.
>
> We could revert to old logic, but that will make booting path and hotplug
> path with different aspm_disabled again.
>
> Acctually we don't need to check aspm_disabled in disable link, as
> we already have protection about link state following.
>
> https://bugzilla.kernel.org/show_bug.cgi?id=55211
> http://article.gmane.org/gmane.linux.kernel.pci/20640
>
> Need it for 3.8 stable.
>
> Reported-by: Roman Yepishev <roman.yepishev@xxxxxxxxx>
> Bisected-by: Roman Yepishev <roman.yepishev@xxxxxxxxx>
> Tested-by: Roman Yepishev <roman.yepishev@xxxxxxxxx>
> Signed-off-by: Yinghai Lu <yinghai@xxxxxxxxxx>
> Cc: Taku Izumi <izumi.taku@xxxxxxxxxxxxxx>
> Cc: Kenji Kaneshige <kaneshige.kenji@xxxxxxxxxxxxxx>
> Cc: stable@xxxxxxxxxx
>
> ---
>  drivers/pci/pcie/aspm.c |   21 ++++-----------------
>  1 file changed, 4 insertions(+), 17 deletions(-)
>
> Index: linux-2.6/drivers/pci/pcie/aspm.c
> ===================================================================
> --- linux-2.6.orig/drivers/pci/pcie/aspm.c
> +++ linux-2.6/drivers/pci/pcie/aspm.c
> @@ -493,15 +493,6 @@ static int pcie_aspm_sanity_check(struct
>                         return -EINVAL;
>
>                 /*
> -                * If ASPM is disabled then we're not going to change
> -                * the BIOS state. It's safe to continue even if it's a
> -                * pre-1.1 device
> -                */
> -
> -               if (aspm_disabled)
> -                       continue;
> -
> -               /*
>                  * Disable ASPM for pre-1.1 PCIe device, we follow MS to use
>                  * RBER bit to determine if a function is 1.1 version device
>                  */
> @@ -718,15 +709,11 @@ void pcie_aspm_powersave_config_link(str
>   * pci_disable_link_state - disable pci device's link state, so the link will
>   * never enter specific states
>   */
> -static void __pci_disable_link_state(struct pci_dev *pdev, int state, bool sem,
> -                                    bool force)
> +static void __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 (aspm_disabled && !force)
> -               return;
> -
>         if (!pci_is_pcie(pdev))
>                 return;
>
> @@ -757,13 +744,13 @@ static void __pci_disable_link_state(str
>
>  void pci_disable_link_state_locked(struct pci_dev *pdev, int state)
>  {
> -       __pci_disable_link_state(pdev, state, false, false);
> +       __pci_disable_link_state(pdev, state, false);
>  }
>  EXPORT_SYMBOL(pci_disable_link_state_locked);
>
>  void pci_disable_link_state(struct pci_dev *pdev, int state)
>  {
> -       __pci_disable_link_state(pdev, state, true, false);
> +       __pci_disable_link_state(pdev, state, true);
>  }
>  EXPORT_SYMBOL(pci_disable_link_state);
>
> @@ -781,7 +768,7 @@ void pcie_clear_aspm(struct pci_bus *bus
>                 __pci_disable_link_state(child, PCIE_LINK_STATE_L0S |
>                                          PCIE_LINK_STATE_L1 |
>                                          PCIE_LINK_STATE_CLKPM,
> -                                        false, true);
> +                                        false);
>         }
>  }
>

This might fix the problem, but the code is still a mess.  In
acpi_pci_root_add(), why do we have the following?

    acpi_pci_root_add

      acpi_pci_osc_support
      if (flags != base_flags)
        pcie_no_aspm
      if (...)
        acpi_pci_osc_control_set
        if (ACPI_SUCCESS)
          is_osc_granted = true

      pci_acpi_scan_root

      if (is_osc_granted)
        if (acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_ASPM)
          pcie_clear_aspm
      else
        pcie_no_aspm

Why can't we set all the ASPM flags *first*, before calling
pci_acpi_scan_root()?  That way we could just do the correct ASPM
setup as we discover devices during enumeration, rather than trying to
fix things up afterwards.  I suspect pcie_clear_aspm() is broken
anyway, because it looks like it only touches one level of the
hierarchy, without recursively descending it.

But Taku went to some trouble in 8c33f51d to introduce is_osc_granted
to remember this until after pci_acpi_scan_root(), so presumably
there's some reason for this.  Do you remember why, Taku?

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/