Re: [PATCH 2/8] ACPI, PCI: call _OSC support during root bridge discovery

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

On Wed, 2008-10-29 at 08:30 -0600, Bjorn Helgaas wrote:
> On Tuesday 28 October 2008 11:48:26 pm Andrew Patterson wrote:
> > ACPI, PCI: call _OSC support during root bridge discovery
> > 
> > Added pci_acpi_isc_support() which is called when a PCI bridge is
> 
> s/_isc_/_osc_/

Fixed.

> 
> > added, so individual PCI root bridge drivers do not have to call _OSC
> > support for every root bridge in their probe functions.
> > ---
> > 
> >  drivers/acpi/pci_root.c  |    6 ++++++
> >  drivers/pci/pci-acpi.c   |   24 +++++++++++++++++++-----
> >  include/linux/pci-acpi.h |    1 +
> >  3 files changed, 26 insertions(+), 5 deletions(-)
> > 
> > 
> > diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> > index 1b8f67d..47df4a8 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,11 @@ 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 |
> > +			     0);
> > +
> >  	/* 
> >  	 * Segment
> >  	 * -------
> > diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> > index dfe7c8e..f457387 100644
> > --- a/drivers/pci/pci-acpi.c
> > +++ b/drivers/pci/pci-acpi.c
> > @@ -139,28 +139,42 @@ 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)
> > +/*
> > + * pci_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 -ENOTTY;
> >  
> >  	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");
> 
> I know you didn't change this printk, but since you're here,
> can you clean this up?  I propose removing this printk altogether,
> then checking the return from pci_acpi_osc_support() and using
> dev_warn (with the flags and return value) if it fails.
> 

One problem we would have with this change is that there are a lot of
platforms that don't implement _OSC, so we would get a lot of spew when
calling pci_acpi_osc_support on those systems (returning -ENOTTY).
Matthew is talking about moving this allocation into the acpi_pci_root
struct, so this issue should become moot at that point.

> > +		rc = -ENOMEM;
> >  		goto out;
> >  	}
> >  

Andrew