On Thu, Mar 28, 2013 at 8:22 PM, Bjorn Helgaas <[email protected]> wrote:
> [+cc Matthew]
> [+cc [email protected] 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 <[email protected]> wrote:
> > > resending with adding To Roman.
> > >
> > > On Thu, Mar 28, 2013 at 5:46 AM, Bjorn Helgaas <[email protected]>
> 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 <[email protected]>
> > | 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
------------------------------------------------------------------------------
Own the Future-Intel(R) Level Up Game Demo Contest 2013
Rise to greatness in Intel's independent game demo contest. Compete
for recognition, cash, and the chance to get your game on Steam.
$5K grand prize plus 10 genre and skill prizes. Submit your demo
by 6/6/13. http://altfarm.mediaplex.com/ad/ck/12124-176961-30367-2
_______________________________________________
E1000-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit
http://communities.intel.com/community/wired
