linux-kernel - Re: [PATCH] drivers: pci: convert generic host controller to DT host bridge creation API

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]

Date:	Tue, 19 Aug 2014 13:05:54 +0100
From:	Will Deacon <will.deacon@....com>
To:	Liviu Dudau <Liviu.Dudau@....com>
Cc:	Bjorn Helgaas <bhelgaas@...gle.com>,
	Catalin Marinas <Catalin.Marinas@....com>,
	Arnd Bergmann <arnd@...db.de>,
	Jingoo Han <jg1.han@...sung.com>,
	Kukjin Kim <kgene.kim@...sung.com>,
	Suravee Suthikulanit <suravee.suthikulpanit@....com>,
	linux-pci <linux-pci@...r.kernel.org>,
	LKML <linux-kernel@...r.kernel.org>,
	LAKML <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH] drivers: pci: convert generic host controller to DT host
 bridge creation API

Hi guys,

On Tue, Aug 12, 2014 at 05:41:35PM +0100, Liviu Dudau wrote:
> From: Lorenzo Pieralisi <lorenzo.pieralisi@....com>
> 
> In order to consolidate DT configuration for PCI host controllers in the
> kernel, a new API was introduced that allows creating a host bridge
> and its PCI bus from DT, removing duplicated code present in the
> majority of pci host driver implementations.
> 
> This patch converts the existing PCI generic host controller driver to
> the new API. Most of the code parsing ranges and creating resources is
> now delegated to of_create_pci_host_bridge() API which also triggers
> a scan of the root bus.
> 
> The setup hook passed by the host controller code to the generic DT
> layer completes the initialization by performing resource filtering
> (ie it checks that at least one non-prefetchable memory resource is
> present), remapping IO and configuration regions and initializing
> the required PCI flags.

[...]

> -static void gen_pci_release_of_pci_ranges(struct gen_pci *pci)
> -{
> -	struct pci_host_bridge_window *win;
> -
> -	list_for_each_entry(win, &pci->resources, list)
> -		release_resource(win->res);
> -
> -	pci_free_resource_list(&pci->resources);
> -}

I take it Liviu's core patches take care of this clean-up now if we fail mid
way through requesting the resources?

> -static int gen_pci_setup(int nr, struct pci_sys_data *sys)
> +static int gen_pci_setup(struct pci_host_bridge *host,
> +			 resource_size_t io_cpuaddr)
>  {
> -	struct gen_pci *pci = sys->private_data;
> -	list_splice_init(&pci->resources, &sys->resources);
> -	return 1;
> +	int err;
> +	struct pci_host_bridge_window *window;
> +	u32 restype, prefetchable;
> +	struct device *dev = host->dev.parent;
> +	struct resource *iores = NULL;
> +	bool res_valid = false;
> +
> +	list_for_each_entry(window, &host->windows, list) {
> +		restype = window->res->flags & IORESOURCE_TYPE_BITS;
> +		prefetchable = window->res->flags & IORESOURCE_PREFETCH;
> +
> +		/*
> +		 * Require at least one non-prefetchable MEM resource
> +		 */
> +		if ((restype == IORESOURCE_MEM) && !prefetchable)
> +			res_valid = true;
> +
> +		if (restype == IORESOURCE_IO)
> +			iores = window->res;
> +	}
> +
> +	if (!res_valid) {
> +		dev_err(dev, "non-prefetchable memory resource required\n");
> +		return -EINVAL;
> +	}
> +
> +	if (iores) {
> +		if (!PAGE_ALIGNED(io_cpuaddr))
> +			return -EINVAL;

Why is this alignment check not in the core code? Probably a question for
somebody like Arnd, but do we need to deal with multiple IO resources?
Currently we'll just silently take the last one that we found, which doesn't
sound ideal.

> +		if (err)
> +			return err;
> +	}
> +
> +	/* Parse and map our Configuration Space windows */
> +	err = gen_pci_parse_map_cfg_windows(host);
> +	if (err)
> +		return err;
> +
> +	pci_add_flags(PCI_ENABLE_PROC_DOMAINS);
> +	pci_add_flags(PCI_REASSIGN_ALL_BUS | PCI_REASSIGN_ALL_RSRC);

Why does this belong in the host controller driver and how does it interact
with the probe-only property?

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