diff mbox

is L1 really disabled in iwlwifi

Message ID 20130510225257.GA10847@google.com (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Bjorn Helgaas May 10, 2013, 10:52 p.m. UTC 
[+cc Rafael, other pci_disable_link_state() users]

On Wed, May 01, 2013 at 11:13:15AM -0600, Bjorn Helgaas wrote:
> On Wed, May 1, 2013 at 2:31 AM, Emmanuel Grumbach <egrumbach@gmail.com> wrote:
> > [from Bjorn's mail]
> >> In Emmanuel's case, we don't get _OSC control, so
> >> pci_disable_link_state() does nothing.
> >
> > Right, but this is true with the specific log I sent to you. Is it
> > possible that another platform  / BIOS, we *will* get _OSC control and
> > that pci_disable_link_state() will actually do something?  In that case
> > I would prefer not to remove the call to pcie_disable_link_state().
> 
> Yes, absolutely, on many platforms we will get _OSC control, and
> pci_disable_link_state() will work as expected.  The problem is that
> the driver doesn't have a good way to know whether pci_disable_link()
> did anything or not.
> 
> Today I think we have:
> 
> 1) If the BIOS grants the OS permission to control PCIe services via
> _OSC, pci_disable_link_state() works and L1 will be disabled.
> 
> 2) If the BIOS does not grant permission, pci_disable_link_state()
> does nothing and L1 may be enabled or not depending on what
> configuration the BIOS did.
> 
> If the device really doesn't work reliably when L1 is enabled, we're
> currently at the mercy of the BIOS -- if the BIOS enables L1 but
> doesn't grant us permission via _OSC, L1 will remain enabled (as it is
> on your system).

I propose the following patch.  Any comments?


commit cd11e3f87c4d2777cf8921c0454500c9baa54b46
Author: Bjorn Helgaas <bhelgaas@google.com>
Date:   Fri May 10 15:54:35 2013 -0600

    PCI/ASPM: Allow drivers to disable ASPM unconditionally
    
    Some devices have hardware problems related to using ASPM.  Drivers for
    these devices use pci_disable_link_state() to prevent their device from
    entering L0s or L1.  But on platforms where the OS doesn't have permission
    to manage ASPM, pci_disable_link_state() does nothing, and the driver has
    no way to know this.
    
    Therefore, if the BIOS enables ASPM but declines (either via the FADT
    ACPI_FADT_NO_ASPM bit or the _OSC method) to allow the OS to manage it,
    the device can still use ASPM and trip over the hardware issue.
    
    This patch makes pci_disable_link_state() disable ASPM unconditionally,
    regardless of whether the OS has permission to manage ASPM in general.
    
    Reported-by: Emmanuel Grumbach <egrumbach@gmail.com>
    Reference: https://lkml.kernel.org/r/CANUX_P3F5YhbZX3WGU-j1AGpbXb_T9Bis2ErhvKkFMtDvzatVQ@mail.gmail.com
    Reference: https://bugzilla.kernel.org/show_bug.cgi?id=57331
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Matthew Garrett May 11, 2013, 8:22 p.m. UTC | #1 
On Sat, 2013-05-11 at 22:26 +0200, Rafael J. Wysocki wrote:
> On Friday, May 10, 2013 04:52:57 PM Bjorn Helgaas wrote:

> > I propose the following patch.  Any comments?

> 

> In my opinion this is dangerous, because it opens us to bugs that right now

> are prevented from happening due to the way the code works.


Right, I'm also not entirely comfortable with this. The current
behaviour may be confusing, but we could reduce that by renaming the
functions. I'm still not clear on whether anyone's actually seeing
problems caused by the existing behaviour.

-- 
Matthew Garrett | mjg59@srcf.ucam.org
Rafael Wysocki May 11, 2013, 8:26 p.m. UTC | #2 
On Friday, May 10, 2013 04:52:57 PM Bjorn Helgaas wrote:
> [+cc Rafael, other pci_disable_link_state() users]
> 
> On Wed, May 01, 2013 at 11:13:15AM -0600, Bjorn Helgaas wrote:
> > On Wed, May 1, 2013 at 2:31 AM, Emmanuel Grumbach <egrumbach@gmail.com> wrote:
> > > [from Bjorn's mail]
> > >> In Emmanuel's case, we don't get _OSC control, so
> > >> pci_disable_link_state() does nothing.
> > >
> > > Right, but this is true with the specific log I sent to you. Is it
> > > possible that another platform  / BIOS, we *will* get _OSC control and
> > > that pci_disable_link_state() will actually do something?  In that case
> > > I would prefer not to remove the call to pcie_disable_link_state().
> > 
> > Yes, absolutely, on many platforms we will get _OSC control, and
> > pci_disable_link_state() will work as expected.  The problem is that
> > the driver doesn't have a good way to know whether pci_disable_link()
> > did anything or not.
> > 
> > Today I think we have:
> > 
> > 1) If the BIOS grants the OS permission to control PCIe services via
> > _OSC, pci_disable_link_state() works and L1 will be disabled.
> > 
> > 2) If the BIOS does not grant permission, pci_disable_link_state()
> > does nothing and L1 may be enabled or not depending on what
> > configuration the BIOS did.
> > 
> > If the device really doesn't work reliably when L1 is enabled, we're
> > currently at the mercy of the BIOS -- if the BIOS enables L1 but
> > doesn't grant us permission via _OSC, L1 will remain enabled (as it is
> > on your system).
> 
> I propose the following patch.  Any comments?

In my opinion this is dangerous, because it opens us to bugs that right now
are prevented from happening due to the way the code works.

Thanks,
Rafael


> commit cd11e3f87c4d2777cf8921c0454500c9baa54b46
> Author: Bjorn Helgaas <bhelgaas@google.com>
> Date:   Fri May 10 15:54:35 2013 -0600
> 
>     PCI/ASPM: Allow drivers to disable ASPM unconditionally
>     
>     Some devices have hardware problems related to using ASPM.  Drivers for
>     these devices use pci_disable_link_state() to prevent their device from
>     entering L0s or L1.  But on platforms where the OS doesn't have permission
>     to manage ASPM, pci_disable_link_state() does nothing, and the driver has
>     no way to know this.
>     
>     Therefore, if the BIOS enables ASPM but declines (either via the FADT
>     ACPI_FADT_NO_ASPM bit or the _OSC method) to allow the OS to manage it,
>     the device can still use ASPM and trip over the hardware issue.
>     
>     This patch makes pci_disable_link_state() disable ASPM unconditionally,
>     regardless of whether the OS has permission to manage ASPM in general.
>     
>     Reported-by: Emmanuel Grumbach <egrumbach@gmail.com>
>     Reference: https://lkml.kernel.org/r/CANUX_P3F5YhbZX3WGU-j1AGpbXb_T9Bis2ErhvKkFMtDvzatVQ@mail.gmail.com
>     Reference: https://bugzilla.kernel.org/show_bug.cgi?id=57331
>     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> 
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index d320df6..9ef4ab8 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -718,15 +718,11 @@ void pcie_aspm_powersave_config_link(struct pci_dev *pdev)
>   * 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 +753,13 @@ static void __pci_disable_link_state(struct pci_dev *pdev, int state, bool sem,
>  
>  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 +777,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);
>  	}
>  }
>
diff mbox

Patch

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index d320df6..9ef4ab8 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -718,15 +718,11 @@  void pcie_aspm_powersave_config_link(struct pci_dev *pdev)
  * 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 +753,13 @@  static void __pci_disable_link_state(struct pci_dev *pdev, int state, bool sem,
 
 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 +777,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);
 	}
 }