Re: [PATCH v3 1/2] ACPI: pci_root: save downstream bus range

[Bjorn Helgaas <bjorn.helgaas@hp.com>]

On Wednesday 10 March 2010 02:50:01 am Kenji Kaneshige wrote:
> Looks good to me. But one nitpicking.
> 
> > +	root->secondary.flags = IORESOURCE_BUS;
> > +	root->secondary.start = 0;
> > +	root->secondary.end = 0xFF;
> > +	status = try_get_root_bridge_busnr(device->handle, &root->secondary);
> >  	if (ACPI_FAILURE(status)) {
> 
> I feel a little strange about initializing 'start' and 'end' fields
> here because those are changed in try_get_root_bridge_busnr().
> What about the below?
> 
> 	root->secondary.flags = IORESOURCE_BUS;
> 	status = try_get_root_bridge_busnr(device->handle, &root->secondary);
> 	if (ACPI_FAILURE(status)) {
> 		root->secondary = 0xFF;
> 		...

Oooh, that's much better.  Wish I'd thought of that to begin with!
I'll fix that and repost these.  Thanks!

Bjorn

> Reviewed-by: Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>
> 
> Thanks,
> Kenji Kaneshige
> 
> 
> Bjorn Helgaas wrote:
> > Previously, we only saved the root bus number, i.e., the beginning of the
> > downstream bus range.  We now support IORESOURCE_BUS resources, so this
> > patch uses that to keep track of both the beginning and the end of the
> > downstream bus range.
> > 
> > It's important to know both the beginning and the end for supporting _CBA
> > (see PCI Firmware spec, rev 3.0, sec 4.1.3) and so we know the limits for
> > any possible PCI bus renumbering (we can't renumber downstream buses to be
> > outside the bus number range claimed by the host bridge).
> > 
> > It's clear from the spec that the bus range is supposed to be in _CRS, but
> > if we don't find it there, we'll assume [_BBN - 0xFF].
> > 
> > Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
> > ---
> > 
> >  drivers/acpi/pci_root.c |   67 ++++++++++++++++++++++++++++-------------------
> >  include/acpi/acpi_bus.h |    2 +
> >  2 files changed, 41 insertions(+), 28 deletions(-)
> > 
> > 
> > diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> > index d724736..e309f56 100644
> > --- a/drivers/acpi/pci_root.c
> > +++ b/drivers/acpi/pci_root.c
> > @@ -119,7 +119,8 @@ acpi_handle acpi_get_pci_rootbridge_handle(unsigned int seg, unsigned int bus)
> >  	struct acpi_pci_root *root;
> >  	
> >  	list_for_each_entry(root, &acpi_pci_roots, node)
> > -		if ((root->segment == (u16) seg) && (root->bus_nr == (u16) bus))
> > +		if ((root->segment == (u16) seg) &&
> > +		    (root->secondary.start == (u16) bus))
> >  			return root->device->handle;
> >  	return NULL;		
> >  }
> > @@ -153,7 +154,7 @@ EXPORT_SYMBOL_GPL(acpi_is_root_bridge);
> >  static acpi_status
> >  get_root_bridge_busnr_callback(struct acpi_resource *resource, void *data)
> >  {
> > -	int *busnr = data;
> > +	struct resource *res = data;
> >  	struct acpi_resource_address64 address;
> >  
> >  	if (resource->type != ACPI_RESOURCE_TYPE_ADDRESS16 &&
> > @@ -163,28 +164,27 @@ get_root_bridge_busnr_callback(struct acpi_resource *resource, void *data)
> >  
> >  	acpi_resource_to_address64(resource, &address);
> >  	if ((address.address_length > 0) &&
> > -	    (address.resource_type == ACPI_BUS_NUMBER_RANGE))
> > -		*busnr = address.minimum;
> > +	    (address.resource_type == ACPI_BUS_NUMBER_RANGE)) {
> > +		res->start = address.minimum;
> > +		res->end = address.minimum + address.address_length - 1;
> > +	}
> >  
> >  	return AE_OK;
> >  }
> >  
> >  static acpi_status try_get_root_bridge_busnr(acpi_handle handle,
> > -					     unsigned long long *bus)
> > +					     struct resource *res)
> >  {
> >  	acpi_status status;
> > -	int busnum;
> >  
> > -	busnum = -1;
> > +	res->start = -1;
> >  	status =
> >  	    acpi_walk_resources(handle, METHOD_NAME__CRS,
> > -				get_root_bridge_busnr_callback, &busnum);
> > +				get_root_bridge_busnr_callback, res);
> >  	if (ACPI_FAILURE(status))
> >  		return status;
> > -	/* Check if we really get a bus number from _CRS */
> > -	if (busnum == -1)
> > +	if (res->start == -1)
> >  		return AE_ERROR;
> > -	*bus = busnum;
> >  	return AE_OK;
> >  }
> >  
> > @@ -428,34 +428,47 @@ static int __devinit acpi_pci_root_add(struct acpi_device *device)
> >  	struct acpi_device *child;
> >  	u32 flags, base_flags;
> >  
> > +	root = kzalloc(sizeof(struct acpi_pci_root), GFP_KERNEL);
> > +	if (!root)
> > +		return -ENOMEM;
> > +
> >  	segment = 0;
> >  	status = acpi_evaluate_integer(device->handle, METHOD_NAME__SEG, NULL,
> >  				       &segment);
> >  	if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) {
> >  		printk(KERN_ERR PREFIX "can't evaluate _SEG\n");
> > -		return -ENODEV;
> > +		result = -ENODEV;
> > +		goto end;
> >  	}
> >  
> >  	/* Check _CRS first, then _BBN.  If no _BBN, default to zero. */
> > -	bus = 0;
> > -	status = try_get_root_bridge_busnr(device->handle, &bus);
> > +	root->secondary.flags = IORESOURCE_BUS;
> > +	root->secondary.start = 0;
> > +	root->secondary.end = 0xFF;
> > +	status = try_get_root_bridge_busnr(device->handle, &root->secondary);
> >  	if (ACPI_FAILURE(status)) {
> > +		/*
> > +		 * We need both the start and end of the downstream bus range
> > +		 * to interpret _CBA (MMCONFIG base address), so it really is
> > +		 * supposed to be in _CRS.
> > +		 */
> > +		printk(KERN_WARNING FW_BUG PREFIX
> > +		       "no secondary bus range in _CRS\n");
> >  		status = acpi_evaluate_integer(device->handle, METHOD_NAME__BBN,					       NULL, &bus);
> > -		if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) {
> > -			printk(KERN_ERR PREFIX
> > -			     "no bus number in _CRS and can't evaluate _BBN\n");
> > -			return -ENODEV;
> > +		if (ACPI_SUCCESS(status))
> > +			root->secondary.start = bus;
> > +		else if (status == AE_NOT_FOUND)
> > +			root->secondary.start = 0;
> > +		else {
> > +			printk(KERN_ERR PREFIX "can't evaluate _BBN\n");
> > +			result = -ENODEV;
> > +			goto end;
> >  		}
> >  	}
> >  
> > -	root = kzalloc(sizeof(struct acpi_pci_root), GFP_KERNEL);
> > -	if (!root)
> > -		return -ENOMEM;
> > -
> >  	INIT_LIST_HEAD(&root->node);
> >  	root->device = device;
> >  	root->segment = segment & 0xFFFF;
> > -	root->bus_nr = bus & 0xFF;
> >  	strcpy(acpi_device_name(device), ACPI_PCI_ROOT_DEVICE_NAME);
> >  	strcpy(acpi_device_class(device), ACPI_PCI_ROOT_CLASS);
> >  	device->driver_data = root;
> > @@ -474,9 +487,9 @@ static int __devinit acpi_pci_root_add(struct acpi_device *device)
> >  	/* TBD: Locking */
> >  	list_add_tail(&root->node, &acpi_pci_roots);
> >  
> > -	printk(KERN_INFO PREFIX "%s [%s] (%04x:%02x)\n",
> > +	printk(KERN_INFO PREFIX "%s [%s] (domain %04x %pR)\n",
> >  	       acpi_device_name(device), acpi_device_bid(device),
> > -	       root->segment, root->bus_nr);
> > +	       root->segment, &root->secondary);
> >  
> >  	/*
> >  	 * Scan the Root Bridge
> > @@ -485,11 +498,11 @@ static int __devinit acpi_pci_root_add(struct acpi_device *device)
> >  	 * 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(device, segment, bus);
> > +	root->bus = pci_acpi_scan_root(device, segment, root->secondary.start);
> >  	if (!root->bus) {
> >  		printk(KERN_ERR PREFIX
> >  			    "Bus %04x:%02x not present in PCI namespace\n",
> > -			    root->segment, root->bus_nr);
> > +			    root->segment, (unsigned int)root->secondary.start);
> >  		result = -ENODEV;
> >  		goto end;
> >  	}
> > diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> > index 7bf83dd..baacd98 100644
> > --- a/include/acpi/acpi_bus.h
> > +++ b/include/acpi/acpi_bus.h
> > @@ -373,7 +373,7 @@ struct acpi_pci_root {
> >  	struct acpi_pci_id id;
> >  	struct pci_bus *bus;
> >  	u16 segment;
> > -	u8 bus_nr;
> > +	struct resource secondary;	/* downstream bus range */
> >  
> >  	u32 osc_support_set;	/* _OSC state of support bits */
> >  	u32 osc_control_set;	/* _OSC state of control bits */
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> > 
> 
> 
>