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

["Rafael J. Wysocki" <rjw@sisk.pl>]

On Monday, April 01, 2013 05:52:56 PM Bjorn Helgaas wrote:
> On Fri, Mar 29, 2013 at 11:04:48AM -0700, Yinghai Lu wrote:
> > attatched -v3 again
> > 
> > On Fri, Mar 29, 2013 at 11:02 AM, Yinghai Lu <yinghai@kernel.org> wrote:
> > > On Fri, Mar 29, 2013 at 5:24 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> > >>
> > >> 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.
> > >
> > >
> > > First commit from Matthew
> > >  0ae5eaf10     PCI: ignore pre-1.1 ASPM quirking when ASPM is disabled
> > >     Right now we won't touch ASPM state if ASPM is disabled, except in the case
> > >     where we find a device that appears to be too old to reliably support ASPM.
> > >     Right now we'll clear it in that case, which is almost certainly the wrong
> > >     thing to do
> > >
> > > Try to not touch pre-1.1 ASPM for all, and it causes lots of regression.
> > >
> > > So second commit
> > >
> > > cdb0f9a1ad2e ASPM: Fix pcie devices with non-pcie children
> > >     Since 3.2.12 and 3.3, some systems are failing to boot with a BUG_ON.
> > >     Some other systems using the pata_jmicron driver fail to boot because no
> > >     disks are detected.  Passing pcie_aspm=force on the kernel command line
> > >     works around it.
> > >
> > > move the check aspm_disabled down.
> > >
> > > but ath5 and etc (pre-1.1) really need to aspm_disable to change their
> > > hw setting.
> > >
> > > So the right solution would be dropping pcie_aspm_sanity_check()
> > > change -in v2 should make all both happy, as quirk and disable that in
> > > driver for ath5 are calling
> > > pcie_disable_aspm_state explicitly.
> > >
> > > In v2, we already removed pcie_clear_aspm() that is calling
> > > pcie_disable_aspm_state.
> > >
> > >
> > > Please check attached -v3.
> 
> It's getting late in the v3.9 cycle already, and while your v3 patch
> probably fixes Roman's problem, I can't convince myself that it is
> safe in general.
> 
> I think the safest thing to do at this point is to revert 8c33f51df
> ("PCI/ACPI: Request _OSC control before scanning PCI root bus") with a
> patch like the one below.
> 
> That does mean the booting path and hotplug paths will be different (we set
> aspm_disabled after boot but before hotplug), but it was that way for a
> long time before 8c33f51df.  I think it's more important to fix this recent
> ath5k regression than to fix a long-standing hotplug bug that nobody ever
> complained about.
> 
> Obviously, I think we should fix the hotplug bug and clean up the ASPM
> mess, too.  But we need to do that when we have more time to do it right
> and test it.
> 
> Bjorn
> 
> 
> commit 96e5d01cd536458435ef0678d9fa3dc542afb41f
> Author: Bjorn Helgaas <bhelgaas@google.com>
> Date:   Mon Apr 1 15:47:39 2013 -0600
> 
>     Revert "PCI/ACPI: Request _OSC control before scanning PCI root bus"
>     
>     This reverts commit 8c33f51df406e1a1f7fa4e9b244845b7ebd61fa6.
>     
>     Conflicts:
>     	drivers/acpi/pci_root.c
>     
>     Reference: https://bugzilla.kernel.org/show_bug.cgi?id=55211
>     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index 0ac546d..c740364 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -415,7 +415,6 @@ static int acpi_pci_root_add(struct acpi_device *device,
>  	struct acpi_pci_root *root;
>  	struct acpi_pci_driver *driver;
>  	u32 flags, base_flags;
> -	bool is_osc_granted = false;
>  
>  	root = kzalloc(sizeof(struct acpi_pci_root), GFP_KERNEL);
>  	if (!root)
> @@ -476,6 +475,30 @@ static int acpi_pci_root_add(struct acpi_device *device,
>  	flags = base_flags = OSC_PCI_SEGMENT_GROUPS_SUPPORT;
>  	acpi_pci_osc_support(root, flags);
>  
> +	/*
> +	 * TBD: Need PCI interface for enumeration/configuration of roots.
> +	 */
> +
> +	mutex_lock(&acpi_pci_root_lock);
> +	list_add_tail(&root->node, &acpi_pci_roots);
> +	mutex_unlock(&acpi_pci_root_lock);
> +
> +	/*
> +	 * Scan the Root Bridge
> +	 * --------------------
> +	 * Must do this prior to any attempt to bind the root device, as the
> +	 * PCI namespace does not get created until this call is made (and
> +	 * thus the root bridge's pci_dev does not exist).
> +	 */
> +	root->bus = pci_acpi_scan_root(root);
> +	if (!root->bus) {
> +		printk(KERN_ERR PREFIX
> +			    "Bus %04x:%02x not present in PCI namespace\n",
> +			    root->segment, (unsigned int)root->secondary.start);
> +		result = -ENODEV;
> +		goto out_del_root;
> +	}
> +
>  	/* Indicate support for various _OSC capabilities. */
>  	if (pci_ext_cfg_avail())
>  		flags |= OSC_EXT_PCI_CONFIG_SUPPORT;
> @@ -494,6 +517,7 @@ static int acpi_pci_root_add(struct acpi_device *device,
>  			flags = base_flags;
>  		}
>  	}
> +
>  	if (!pcie_ports_disabled
>  	    && (flags & ACPI_PCIE_REQ_SUPPORT) == ACPI_PCIE_REQ_SUPPORT) {
>  		flags = OSC_PCI_EXPRESS_CAP_STRUCTURE_CONTROL
> @@ -514,54 +538,28 @@ static int acpi_pci_root_add(struct acpi_device *device,
>  		status = acpi_pci_osc_control_set(device->handle, &flags,
>  				       OSC_PCI_EXPRESS_CAP_STRUCTURE_CONTROL);
>  		if (ACPI_SUCCESS(status)) {
> -			is_osc_granted = true;
>  			dev_info(&device->dev,
>  				"ACPI _OSC control (0x%02x) granted\n", flags);
> +			if (acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_ASPM) {
> +				/*
> +				 * We have ASPM control, but the FADT indicates
> +				 * that it's unsupported. Clear it.
> +				 */
> +				pcie_clear_aspm(root->bus);
> +			}
>  		} else {
> -			is_osc_granted = false;
>  			dev_info(&device->dev,
>  				"ACPI _OSC request failed (%s), "
>  				"returned control mask: 0x%02x\n",
>  				acpi_format_exception(status), flags);
> +			pr_info("ACPI _OSC control for PCIe not granted, "
> +				"disabling ASPM\n");
> +			pcie_no_aspm();
>  		}
>  	} else {
>  		dev_info(&device->dev,
> -			"Unable to request _OSC control "
> -			"(_OSC support mask: 0x%02x)\n", flags);
> -	}
> -
> -	/*
> -	 * TBD: Need PCI interface for enumeration/configuration of roots.
> -	 */
> -
> -	mutex_lock(&acpi_pci_root_lock);
> -	list_add_tail(&root->node, &acpi_pci_roots);
> -	mutex_unlock(&acpi_pci_root_lock);
> -
> -	/*
> -	 * Scan the Root Bridge
> -	 * --------------------
> -	 * Must do this prior to any attempt to bind the root device, as the
> -	 * PCI namespace does not get created until this call is made (and 
> -	 * thus the root bridge's pci_dev does not exist).
> -	 */
> -	root->bus = pci_acpi_scan_root(root);
> -	if (!root->bus) {
> -		printk(KERN_ERR PREFIX
> -			    "Bus %04x:%02x not present in PCI namespace\n",
> -			    root->segment, (unsigned int)root->secondary.start);
> -		result = -ENODEV;
> -		goto out_del_root;
> -	}
> -
> -	/* ASPM setting */
> -	if (is_osc_granted) {
> -		if (acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_ASPM)
> -			pcie_clear_aspm(root->bus);
> -	} else {
> -		pr_info("ACPI _OSC control for PCIe not granted, "
> -			"disabling ASPM\n");
> -		pcie_no_aspm();
> +			 "Unable to request _OSC control "
> +			 "(_OSC support mask: 0x%02x)\n", flags);
>  	}
>  
>  	pci_acpi_add_bus_pm_notifier(device, root->bus);
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.