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

[Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>]

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;
		...

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
> 
>