linux-kernel - Re: [PATCH 3/5] PCI: cadence: Add host driver for Cadence PCIe controller

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: <20171129141430.GA30617@red-moon>
Date:   Wed, 29 Nov 2017 14:14:30 +0000
From:   Lorenzo Pieralisi <lorenzo.pieralisi@....com>
To:     Bjorn Helgaas <helgaas@...nel.org>
Cc:     Cyrille Pitchen <cyrille.pitchen@...e-electrons.com>,
        bhelgaas@...gle.com, kishon@...com, linux-pci@...r.kernel.org,
        adouglas@...ence.com, stelford@...ence.com, dgary@...ence.com,
        kgopi@...ence.com, eandrews@...ence.com,
        thomas.petazzoni@...e-electrons.com, sureshp@...ence.com,
        nsekhar@...com, linux-kernel@...r.kernel.org, robh@...nel.org,
        devicetree@...r.kernel.org
Subject: Re: [PATCH 3/5] PCI: cadence: Add host driver for Cadence PCIe
 controller

On Tue, Nov 28, 2017 at 02:41:14PM -0600, Bjorn Helgaas wrote:

[...]

> > +static int cdns_pcie_parse_request_of_pci_ranges(struct device *dev,
> > +						 struct list_head *resources,
> > +						 struct resource **bus_range)
> > +{
> > +	int err, res_valid = 0;
> > +	struct device_node *np = dev->of_node;
> > +	resource_size_t iobase;
> > +	struct resource_entry *win, *tmp;
> > +
> > +	err = of_pci_get_host_bridge_resources(np, 0, 0xff, resources, &iobase);
> > +	if (err)
> > +		return err;
> > +
> > +	err = devm_request_pci_bus_resources(dev, resources);
> > +	if (err)
> > +		return err;
> > +
> > +	resource_list_for_each_entry_safe(win, tmp, resources) {
> > +		struct resource *res = win->res;
> > +
> > +		switch (resource_type(res)) {
> > +		case IORESOURCE_IO:
> > +			err = pci_remap_iospace(res, iobase);
> > +			if (err) {
> > +				dev_warn(dev, "error %d: failed to map resource %pR\n",
> > +					 err, res);
> > +				resource_list_destroy_entry(win);
> > +			}
> > +			break;
> > +		case IORESOURCE_MEM:
> > +			res_valid |= !(res->flags & IORESOURCE_PREFETCH);
> > +			break;
> > +		case IORESOURCE_BUS:
> > +			*bus_range = res;
> > +			break;
> > +		}
> > +	}
> > +
> > +	if (res_valid)
> > +		return 0;
> > +
> > +	dev_err(dev, "non-prefetchable memory resource required\n");
> > +	return -EINVAL;
> > +}
> 
> The code above is starting to look awfully familiar.  I wonder if it's
> time to think about some PCI-internal interface that can encapsulate
> this.  In this case, there's really nothing Cadence-specific here.
> There are other callers where there *is* vendor-specific code, but
> possibly that could be handled by returning pointers to bus number,
> I/O port, and MMIO resources so the caller could do the
> vendor-specific stuff?

Yes and that's not the only one, pattern below is duplicated
(with some minor differences across host bridges that I think
can be managed through function parameters), it is probably worth
moving them both into a core code helper.

list_splice_init(&resources, &bridge->windows);
bridge->dev.parent = dev;
bridge->busnr = bus;
bridge->ops = &pci_ops;
bridge->map_irq = of_irq_parse_and_map_pci;
bridge->swizzle_irq = pci_common_swizzle;

ret = pci_scan_root_bus_bridge(bridge);
if (ret < 0) {
	dev_err(dev, "Scanning root bridge failed");
	goto err_init;
}

bus = bridge->bus;
pci_bus_size_bridges(bus);
pci_bus_assign_resources(bus);

list_for_each_entry(child, &bus->children, node)
	pcie_bus_configure_settings(child);

pci_bus_add_devices(bus);