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

[Yinghai Lu <yi...@ke...>]

On Thu, Mar 28, 2013 at 8:22 PM, Bjorn Helgaas <bhe...@go...> wrote:

> [+cc Matthew]
> [+cc e10...@li... for suspected 82575/82598
> regression]
>
> On Thu, Mar 28, 2013 at 01:24:55PM -0700, Yinghai Lu wrote:
> > patch for Roman
> >
> > On Thu, Mar 28, 2013 at 1:24 PM, Yinghai Lu <yi...@ke...> wrote:
> > > resending with adding To Roman.
> > >
> > > On Thu, Mar 28, 2013 at 5:46 AM, Bjorn Helgaas <bhe...@go...>
> wrote:
> > >> This patch might be *safe*, but it (and the changelog) are completely
> > >> unintelligible.
> > >>
> > >> The problem with applying an unintelligible stop-gap patch is that it
> > >> becomes forever part of the changelog, and it's a huge waste of time
> > >> to everybody who tries to understand the history later.  That's why I
> > >> think it's worth spending some time to make a good patch now.
> > >
> > > Please check if attached patch is doing what you want.
>
> Patch inlined below for convenience.
>
> > Subject: [PATCH] PCI: Remove not needed check in disable aspm link
> >
> > Roman reported ath5k does not work anymore on 3.8.
> > Bisected to
> > | commit 8c33f51df406e1a1f7fa4e9b244845b7ebd61fa6
> > | Author: Taku Izumi <izu...@jp...>
> > | 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.
>
> I think this regression has nothing to do with pci_disable_link_state().
>
> When aspm_disabled is set, pci_disable_link_state() doesn't do anything.
>
> In both 3.7 and 3.8, aspm_disabled is set in acpi_pci_root_add() before
> any driver probe routines are run, so it looks like calling
> pci_disable_link_state() from a driver had no effect even in 3.7.  This
> is a problem, of course, but not the one Roman is seeing, because ath5k
> calls pci_disable_link_state() from the driver probe routine.
>
> There are also PCI_FIXUP_FINAL quirks for 82575 and 82598 NICs that call
> pci_disable_link_state().  In 3.7, these quirks are run before
> aspm_disabled is set, but 8c33f51d moved the pcie_no_aspm() call up
> before we start scanning the bus, so in 3.8, aspm_disabled is set
> *before* we run them.  I think that means 8c33f51d broke all these
> quirks.  That's also a problem, of course, but this isn't the one Roman
> is seeing either.
>
> I think the problem Roman is seeing happens when
> pcie_aspm_init_link_state() calls pcie_aspm_sanity_check() during device
> enumeration.  In 3.8, the fact that aspm_disabled is already set by the
> time we get here means we skip the check for pre-1.1 PCIe devices, and
> I think *this* is what Roman is seeing.
>
> I suspect the following hunk of your patch is enough to fix things for
> Roman:
>
> > --- linux-2.6.orig/drivers/pci/pcie/aspm.c
> > +++ linux-2.6/drivers/pci/pcie/aspm.c
> > @@ -493,15 +492,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
> >                */
>
> However, this test was added by Matthew in c9651e70, and I can't remove
> it unless we have an explanation of why removing it will not reintroduce
> the bug he was fixing.
>
> This code is such a terrible mess that it's not surprising at all that
> we have all these issues.  But there's too much to untangle in v3.9; all
> we can hope for is to fix the regressions in v3.9 and clean it up later.
>

v1 will fix quirks and pcie_aspm_sanity_check path.
v2. will go further even user pass "aspm=off", those quirks and disable
aspm in driver
will still work, and also call pcie_no_aspm for disable aspm for FADT path
early.

So now you want half of v1, and not want to fix quirk path.
Is my understanding right?

Yinghai