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
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
