Re: Doing _OSC right

[Andrew Patterson <andrew.patterson@hp.com>]

On Fri, 2008-10-24 at 23:30 -0600, Matthew Wilcox wrote:
> Andrew Patterson recently reported a problem where a machine with dozens
> of PCIe root bridges would take half an hour just calling _OSC.  The
> root cause is the AER code calling:
> 
> 	pcie_osc_support_set(OSC_EXT_PCI_CONFIG_SUPPORT);
> 
> for each bridge.  pcie_osc_support_set() also iterates over each bridge,
> so _OSC is called a quadratic number of times.  Obviously, this isn't
> hard to fix -- just moving the call to pcie_osc_support_set() to
> aer_service_init() would do.
> 
> But why should a driver be calling pcie_osc_support_set() anyway?  This
> is something the PCI core should be taking care of for it.
> 
> The patch below illustrates one possible solution.  I create a new
> function (pci_acpi_osc_support()) which is called from pci_root.c before
> we call any other methods on the device (as recommended by the ACPI spec).
> Since we know what capabilities the system supports, individual modules
> do not now need to inform the core of their support for various
> capabilities.
> 
> I do not think this patch is perfect.  One defect is that there are
> now no callers of pci_osc_support_set nor pcie_osc_support_set, so they
> could be deleted.
> 
> Another problem is that if, for example, MSI is disabled at boot time,
> we will incorrectly inform ACPI that we support MSI.  So something more
> flexible is needed.

Check pci_msi_enable? After exposing it.

> 
> I think all the existing _OSC code should be moved from pci-acpi.c to
> pci_root.c.  I also think the acpi_osc_data should be made part of the
> acpi_pci_root struct, eliminating a separate allocation.
> 
> We could also use an interface that iterates over all existing busses
> calling _OSC with new flags.  Shouldn't be hard, once it's integrated
> into pci_root.c.
> 

Need to think about root hotplug at some point.

> Further ahead, we don't actually check that the bits we asked for (in
> 'control') were actually granted to us.  The PCI firmware spec is quite
> explicit about interdependencies amongst the bits.

Do we need to do this in addition to the typical
pci_find_capability(dev, PCI_CAP_ID_MSIX) check? Perhaps we can write a
function that does both?

> 
> We also need to re-evaluate _OSC when coming out of S4.  I'm not much of
> a power-management maven so I don't know where to put this call.
> 
> Thoughts on the road ahead?
> 

This patch does not compile on pci-2.6/linux-next with ia-64.

> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index 1b8f67d..3af9fff 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -31,6 +31,7 @@
>  #include <linux/spinlock.h>
>  #include <linux/pm.h>
>  #include <linux/pci.h>
> +#include <linux/pci-acpi.h>
>  #include <linux/acpi.h>
>  #include <acpi/acpi_bus.h>
>  #include <acpi/acpi_drivers.h>
> @@ -210,6 +211,18 @@ static int __devinit acpi_pci_root_add(struct acpi_device *device)
>  
>  	device->ops.bind = acpi_pci_bind;
>  
> +	pci_acpi_osc_support(device->handle,
> +					OSC_EXT_PCI_CONFIG_SUPPORT |
> +					OSC_PCI_SEGMENT_GROUPS_SUPPORT |
> +#ifdef CONFIG_PCI_MSI
> +					OSC_MSI_SUPPORT |
> +#endif
> +#ifdef CONFIG_PCIEASPM
> +					OSC_ACTIVE_STATE_PWR_SUPPORT |
> +					OSC_CLOCK_PWR_CAPABILITY_SUPPORT |
> +#endif
> +					0);
> +
>  	/* 
>  	 * Segment
>  	 * -------
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index 74801f7..d281201 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -759,24 +759,3 @@ void pci_msi_init_pci_dev(struct pci_dev *dev)
>  {
>  	INIT_LIST_HEAD(&dev->msi_list);
>  }
> -
> -#ifdef CONFIG_ACPI
> -#include <linux/acpi.h>
> -#include <linux/pci-acpi.h>
> -static void __devinit msi_acpi_init(void)
> -{
> -	if (acpi_pci_disabled)
> -		return;
> -	pci_osc_support_set(OSC_MSI_SUPPORT);
> -	pcie_osc_support_set(OSC_MSI_SUPPORT);

Calling pcie_osc_support_set() here is redundant. pci_osc_support_set
will get all PCI/PCIe root bridges.

> -}
> -#else
> -static inline void msi_acpi_init(void) { }
> -#endif /* CONFIG_ACPI */
> -
> -void __devinit msi_init(void)
> -{
> -	if (!pci_msi_enable)
> -		return;
> -	msi_acpi_init();
> -}
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index dfe7c8e..3ec5ecc 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -139,28 +139,41 @@ static acpi_status __acpi_query_osc(u32 flags, struct acpi_osc_data *osc_data,
>  	return status;
>  }
>  
> -static acpi_status acpi_query_osc(acpi_handle handle,
> -				  u32 level, void *context, void **retval)
> +/*
> + * acpi_osc_support: Invoke _OSC indicating support for the given feature
> + * flags: Bitmask of flags to support
> + *
> + * See the ACPI spec for the definition of the flags
> + */
> +int pci_acpi_osc_support(acpi_handle handle, u32 flags)
>  {
> +	u32 dummy;
>  	acpi_status status;
> -	struct acpi_osc_data *osc_data;
> -	u32 flags = (unsigned long)context, dummy;
>  	acpi_handle tmp;
> +	struct acpi_osc_data *osc_data;
> +	int rc = 0;
>  
>  	status = acpi_get_handle(handle, "_OSC", &tmp);
>  	if (ACPI_FAILURE(status))
> -		return AE_OK;
> -
> +		return -ENODEV;
>  	mutex_lock(&pci_acpi_lock);
>  	osc_data = acpi_get_osc_data(handle);
>  	if (!osc_data) {
>  		printk(KERN_ERR "acpi osc data array is full\n");
> +		rc = -ENOMEM;
>  		goto out;
>  	}
>  
>  	__acpi_query_osc(flags, osc_data, &dummy);
> -out:
> + out:
>  	mutex_unlock(&pci_acpi_lock);
> +	return rc;
> +}
> +
> +static acpi_status acpi_query_osc(acpi_handle handle, u32 level,
> +						void *context, void **retval)
> +{
> +	pci_acpi_osc_support(handle, (unsigned long)context);
>  	return AE_OK;
>  }
>  
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 21f2ac6..44d00c9 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -2037,8 +2037,6 @@ static int __devinit pci_init(void)
>  		pci_fixup_device(pci_fixup_final, dev);
>  	}
>  
> -	msi_init();
> -
>  	return 0;
>  }
>  
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 9de87e9..b205ab8 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -98,11 +98,9 @@ extern unsigned int pci_pm_d3_delay;
>  #ifdef CONFIG_PCI_MSI
>  void pci_no_msi(void);
>  extern void pci_msi_init_pci_dev(struct pci_dev *dev);
> -extern void __devinit msi_init(void);
>  #else
>  static inline void pci_no_msi(void) { }
>  static inline void pci_msi_init_pci_dev(struct pci_dev *dev) { }
> -static inline void msi_init(void) { }
>  #endif
>  
>  #ifdef CONFIG_PCIEAER
> diff --git a/drivers/pci/pcie/aer/aerdrv_acpi.c b/drivers/pci/pcie/aer/aerdrv_acpi.c
> index 6dd7b13..ebce26c 100644
> --- a/drivers/pci/pcie/aer/aerdrv_acpi.c
> +++ b/drivers/pci/pcie/aer/aerdrv_acpi.c
> @@ -38,7 +38,6 @@ int aer_osc_setup(struct pcie_device *pciedev)
>  
>  	handle = acpi_find_root_bridge_handle(pdev);
>  	if (handle) {
> -		pcie_osc_support_set(OSC_EXT_PCI_CONFIG_SUPPORT);
>  		status = pci_osc_control_set(handle,
>  					OSC_PCI_EXPRESS_AER_CONTROL |
>  					OSC_PCI_EXPRESS_CAP_STRUCTURE_CONTROL);
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 8f63f4c..2c87883 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -833,25 +833,3 @@ void pcie_no_aspm(void)
>  	if (!aspm_force)
>  		aspm_disabled = 1;
>  }
> -
> -#ifdef CONFIG_ACPI
> -#include <acpi/acpi_bus.h>
> -#include <linux/pci-acpi.h>
> -static void pcie_aspm_platform_init(void)
> -{
> -	pcie_osc_support_set(OSC_ACTIVE_STATE_PWR_SUPPORT|
> -		OSC_CLOCK_PWR_CAPABILITY_SUPPORT);
> -}
> -#else
> -static inline void pcie_aspm_platform_init(void) { }
> -#endif
> -
> -static int __init pcie_aspm_init(void)
> -{
> -	if (aspm_disabled)
> -		return 0;
> -	pcie_aspm_platform_init();
> -	return 0;
> -}
> -

I suspect the following was not supposed to be included.

> -fs_initcall(pcie_aspm_init);
> diff --git a/drivers/scsi/sym53c8xx_2/sym_glue.c b/drivers/scsi/sym53c8xx_2/sym_glue.c
> index d1fcd7e..ec048d3 100644
> --- a/drivers/scsi/sym53c8xx_2/sym_glue.c
> +++ b/drivers/scsi/sym53c8xx_2/sym_glue.c
> @@ -605,17 +605,17 @@ static int sym_eh_handler(int op, char *opname, struct scsi_cmnd *cmd)
>   * interrupt.  This can happen for a number of reasons, including buggy
>   * hardware and interrupt handler failures.
>   */
> -static enum scsi_eh_timer_return sym53c8xx_eh_timed_out(struct scsi_cmnd *cmd)
> +static enum blk_eh_timer_return sym53c8xx_eh_timed_out(struct scsi_cmnd *cmd)
>  {
>  	static int printed_warning;
>  	int result = sym53c8xx_intr(0, cmd->device->host);
>  
>  	if (result == IRQ_NONE)
> -		return EH_NOT_HANDLED;
> +		return BLK_EH_NOT_HANDLED;
>  
>  	/* When commands have been handled, host_data is set to NULL */
>  	if (cmd->host_data)
> -		return EH_NOT_HANDLED;
> +		return BLK_EH_NOT_HANDLED;
>  
>  	if (!printed_warning) {
>  		scmd_printk(KERN_ERR, cmd, "Command timed out and was "
> @@ -624,7 +624,7 @@ static enum scsi_eh_timer_return sym53c8xx_eh_timed_out(struct scsi_cmnd *cmd)
>  		printed_warning = 1;
>  	}
>  
> -	return EH_HANDLED;
> +	return BLK_EH_HANDLED;
>  }
>  

Separate patch?

>  static int sym53c8xx_eh_abort_handler(struct scsi_cmnd *cmd)
> diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
> index 8837928..424f06f 100644
> --- a/include/linux/pci-acpi.h
> +++ b/include/linux/pci-acpi.h
> @@ -8,6 +8,8 @@
>  #ifndef _PCI_ACPI_H_
>  #define _PCI_ACPI_H_
>  
> +#include <linux/acpi.h>
> +
>  #define OSC_QUERY_TYPE			0
>  #define OSC_SUPPORT_TYPE 		1
>  #define OSC_CONTROL_TYPE		2
> @@ -49,6 +51,7 @@
>  #ifdef CONFIG_ACPI
>  extern acpi_status pci_osc_control_set(acpi_handle handle, u32 flags);
>  extern acpi_status __pci_osc_support_set(u32 flags, const char *hid);
> +int pci_acpi_osc_support(acpi_handle handle, u32 flags);
>  static inline acpi_status pci_osc_support_set(u32 flags)
>  {
>  	return __pci_osc_support_set(flags, PCI_ROOT_HID_STRING);

Andrew