linux-kernel - Re: [PATCH v9 2/2] PCI: eic7700: Add Eswin PCIe host controller driver

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: <20260105223037.GA332950@bhelgaas>
Date: Mon, 5 Jan 2026 16:30:37 -0600
From: Bjorn Helgaas <helgaas@...nel.org>
To: zhangsenchuan@...incomputing.com
Cc: bhelgaas@...gle.com, mani@...nel.org, krzk+dt@...nel.org,
	conor+dt@...nel.org, lpieralisi@...nel.org, kwilczynski@...nel.org,
	robh@...nel.org, p.zabel@...gutronix.de, jingoohan1@...il.com,
	gustavo.pimentel@...opsys.com, linux-pci@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	christian.bruel@...s.st.com, mayank.rana@....qualcomm.com,
	shradha.t@...sung.com, krishna.chundru@....qualcomm.com,
	thippeswamy.havalige@....com, inochiama@...il.com, Frank.li@....com,
	ningyu@...incomputing.com, linmin@...incomputing.com,
	pinkesh.vaghela@...fochips.com, ouyanghui@...incomputing.com,
	Niklas Cassel <cassel@...nel.org>
Subject: Re: [PATCH v9 2/2] PCI: eic7700: Add Eswin PCIe host controller
 driver

[+cc Niklas, list vs array of ports]

On Mon, Dec 29, 2025 at 07:32:07PM +0800, zhangsenchuan@...incomputing.com wrote:
> From: Senchuan Zhang <zhangsenchuan@...incomputing.com>
> 
> Add driver for the Eswin EIC7700 PCIe host controller, which is based on
> the DesignWare PCIe core, IP revision 5.96a. The PCIe Gen.3 controller
> supports a data rate of 8 GT/s and 4 channels, support INTx and MSI
> interrupts.

> +config PCIE_EIC7700
> +	tristate "Eswin EIC7700 PCIe controller"

> +/* Vendor and device ID value */
> +#define PCI_VENDOR_ID_ESWIN		0x1fe1
> +#define PCI_DEVICE_ID_ESWIN		0x2030

Usually the device name is a little more than just the vendor.  What
if Eswin ever makes a second device?

> +static int eic7700_pcie_parse_port(struct eic7700_pcie *pcie,
> +				   struct device_node *node)
> +{
> +	struct device *dev = pcie->pci.dev;
> +	struct eic7700_pcie_port *port;
> +
> +	port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
> +	if (!port)
> +		return -ENOMEM;
> +
> +	port->perst = of_reset_control_get_exclusive(node, "perst");
> +	if (IS_ERR(port->perst)) {
> +		dev_err(dev, "Failed to get PERST# reset\n");
> +		return PTR_ERR(port->perst);
> +	}
> +
> +	/*
> +	 * TODO: Since the Root Port node is separated out by pcie devicetree,
> +	 * the DWC core initialization code can't parse the num-lanes attribute
> +	 * in the Root Port. Before entering the DWC core initialization code,
> +	 * the platform driver code parses the Root Port node. The EIC7700 only
> +	 * supports one Root Port node, and the num-lanes attribute is suitable
> +	 * for the case of one Root Port.
> +	 */
> +	if (!of_property_read_u32(node, "num-lanes", &port->num_lanes))
> +		pcie->pci.num_lanes = port->num_lanes;
> +
> +	INIT_LIST_HEAD(&port->list);
> +	list_add_tail(&port->list, &pcie->ports);

Niklas raised an interesting question about whether a list or an array
is the best data structure for the set of Root Ports:

  https://lore.kernel.org/r/aVvkmkd5mWPmxeiS@ryzen

Might have to iterate over the child nodes twice (once to count, again
for eic7700_pcie_parse_port()), but otherwise the array is probably
simpler code.

> +	return 0;
> +}
> +
> +static int eic7700_pcie_parse_ports(struct eic7700_pcie *pcie)
> +{
> +	struct eic7700_pcie_port *port, *tmp;
> +	struct device *dev = pcie->pci.dev;
> +	int ret;
> +
> +	for_each_available_child_of_node_scoped(dev->of_node, of_port) {
> +		ret = eic7700_pcie_parse_port(pcie, of_port);
> +		if (ret)
> +			goto err_port;
> +	}
> +
> +	return 0;
> +
> +err_port:
> +	list_for_each_entry_safe(port, tmp, &pcie->ports, list)
> +		list_del(&port->list);

Is some kind of reset_control_put() needed to match the
of_reset_control_get_exclusive() above?

> +static struct platform_driver eic7700_pcie_driver = {
> +	.probe = eic7700_pcie_probe,

This driver is tristate but has no .remove() callback.  Seems like it
should have one?

> +	.driver = {
> +		.name = "eic7700-pcie",
> +		.of_match_table = eic7700_pcie_of_match,
> +		.suppress_bind_attrs = true,
> +		.pm = &eic7700_pcie_pm,
> +		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
> +	},
> +};
> +builtin_platform_driver(eic7700_pcie_driver);