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

[Bjorn Helgaas]

On Thu, Mar 28, 2013 at 11:59 PM, Yinghai Lu <yinghai@xxxxxxxxxx> wrote:
>
> On Thu, Mar 28, 2013 at 8:22 PM, Bjorn Helgaas <bhelgaas@xxxxxxxxxx> wrote:
>>
>> [+cc Matthew]
>> [+cc e1000-devel@xxxxxxxxxxxxxxxxxxxxx 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 <yinghai@xxxxxxxxxx> wrote:
>> > > resending with adding To Roman.
>> > >
>> > > On Thu, Mar 28, 2013 at 5:46 AM, Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
>> > > 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 <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.
>>
>> 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?

What I want is a patch that fixes the regression and doesn't break
anything else, along with a changelog that makes it obvious that we're
doing the right thing.  I don't know what that looks like yet.  None
of your patches so far is even close.

Half of your v1 patch (removing the pcie_aspm_sanity_check() test)
*might* be the right thing, but only if you can clearly explain why
that will not reintroduce the bug Matthew fixed with c9651e70.

I think we also need to fix the PCI_FIXUP_FINAL quirk regression, but
that's a separate issue and should be a separate patch.
--
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/