linux-kernel - Re: [Resend PATCH 5/5] IA64/PCI/ACPI: Rework PCI root bridge ACPI resource conversion

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20131016235541.GD17866@google.com>
Date:	Wed, 16 Oct 2013 17:55:41 -0600
From:	Bjorn Helgaas <bhelgaas@...gle.com>
To:	tianyu.lan@...el.com
Cc:	tony.luck@...el.com, lenb@...nel.org, rjw@...k.pl,
	yinghai@...nel.org, linux-ia64@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-acpi@...r.kernel.org
Subject: Re: [Resend PATCH 5/5] IA64/PCI/ACPI: Rework PCI root bridge ACPI
 resource conversion

On Fri, Oct 11, 2013 at 08:19:01PM +0800, tianyu.lan@...el.com wrote:
> From: Lan Tianyu <tianyu.lan@...el.com>
> 
> Using ACPI resource functions to convert ACPI resource to generic resource
> 
> Signed-off-by: Lan Tianyu <tianyu.lan@...el.com>
> ---
> This patch just passes through compilation test due to no ia64 machine on hand.
> 
>  arch/ia64/pci/pci.c | 38 +++++++++++++++++++++-----------------
>  1 file changed, 21 insertions(+), 17 deletions(-)
> 
> diff --git a/arch/ia64/pci/pci.c b/arch/ia64/pci/pci.c
> index 2326790..14fa175 100644
> --- a/arch/ia64/pci/pci.c
> +++ b/arch/ia64/pci/pci.c
> @@ -232,8 +232,9 @@ out:
>  	return ~0;
>  }
>  
> -static acpi_status resource_to_window(struct acpi_resource *resource,
> -				      struct acpi_resource_address64 *addr)
> +static acpi_status resource_to_window(struct acpi_resource *ares,
> +				      struct acpi_resource_address64 *addr,
> +				      struct resource *res)
>  {
>  	acpi_status status;
>  
> @@ -244,7 +245,7 @@ static acpi_status resource_to_window(struct acpi_resource *resource,
>  	 *	- producers, i.e., the address space is routed downstream,
>  	 *	  not consumed by the bridge itself
>  	 */
> -	status = acpi_resource_to_address64(resource, addr);
> +	status = acpi_dev_resource_address_space_full(ares, addr, res);
>  	if (ACPI_SUCCESS(status) &&
>  	    (addr->resource_type == ACPI_MEMORY_RANGE ||
>  	     addr->resource_type == ACPI_IO_RANGE) &&
> @@ -255,51 +256,54 @@ static acpi_status resource_to_window(struct acpi_resource *resource,
>  	return AE_ERROR;
>  }
>  
> -static acpi_status count_window(struct acpi_resource *resource, void *data)
> +static acpi_status count_window(struct acpi_resource *ares, void *data)
>  {
>  	unsigned int *windows = (unsigned int *) data;
>  	struct acpi_resource_address64 addr;
> +	struct resource res;
>  	acpi_status status;
>  
> -	status = resource_to_window(resource, &addr);
> +	status = resource_to_window(ares, &addr, &res);
>  	if (ACPI_SUCCESS(status))
>  		(*windows)++;
>  
>  	return AE_OK;
>  }
>  
> -static acpi_status add_window(struct acpi_resource *res, void *data)
> +static acpi_status add_window(struct acpi_resource *ares, void *data)
>  {
>  	struct pci_root_info *info = data;
> -	struct resource *resource;
> +	struct resource *resource = &info->res[info->res_num];
>  	struct acpi_resource_address64 addr;
>  	acpi_status status;
> -	unsigned long flags, offset = 0;
> +	unsigned long offset = 0;
>  	struct resource *root;
>  
>  	/* Return AE_OK for non-window resources to keep scanning for more */
> -	status = resource_to_window(res, &addr);
> +	status = resource_to_window(ares, &addr, resource);
>  	if (!ACPI_SUCCESS(status))
>  		return AE_OK;
>  
> -	if (addr.resource_type == ACPI_MEMORY_RANGE) {
> -		flags = IORESOURCE_MEM;
> +	if (resource->flags & IORESOURCE_MEM) {
>  		root = &iomem_resource;
>  		offset = addr.translation_offset;
> -	} else if (addr.resource_type == ACPI_IO_RANGE) {
> -		flags = IORESOURCE_IO;
> +	} else if (resource->flags & IORESOURCE_IO) {
>  		root = &ioport_resource;
>  		offset = add_io_space(info, &addr);
>  		if (offset == ~0)
>  			return AE_OK;
> +
> +		/*
> +		 * io space address translation offset depends
> +		 * on the return value of add_io_space(). So
> +		 * Repopulate resource->start and end here.

"Repopulate" makes it sound like "resource->start" got clobbered
somewhere.  But it didn't.  I think it's just that each bridge can
support its own I/O port range, and the PCI port numbers reported
in the acpi_resource may not be distinct, and add_io_space() adds
an offset so all the I/O port ranges fit into one global I/O port
space.

For example, I think these two bridges have the same port numbers
(0x0-0xfff) in their acpi_resource, but the second has an offset
of 0x1000000 in the system I/O port space, and I think this offset
is what add_io_space() returns:

  HWP0002:00: host bridge window [io  0x0000-0x0fff]	   (PCI [0x0-0xfff])
  HWP0002:09: host bridge window [io  0x1000000-0x1000fff] (PCI [0x0-0xfff])

> +		 */
> +		resource->start = addr.minimum + offset;
> +		resource->end = resource->start + addr.address_length - 1;

Can't we use this:

		resource->start += offset;
		resource->end += offset;

to avoid breaking the encapsulation of struct acpi_resource_address64?

>  	} else
>  		return AE_OK;
>  
> -	resource = &info->res[info->res_num];
>  	resource->name = info->name;
> -	resource->flags = flags;
> -	resource->start = addr.minimum + offset;
> -	resource->end = resource->start + addr.address_length - 1;
>  	info->res_offset[info->res_num] = offset;
>  
>  	if (insert_resource(root, resource)) {
> -- 
> 1.8.2.1
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/