| 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
| ||
|
Message-ID: <3c8d6749.1f49.19b93552d97.Coremail.zhangsenchuan@eswincomputing.com>
Date: Tue, 6 Jan 2026 20:43:11 +0800 (GMT+08:00)
From: zhangsenchuan <zhangsenchuan@...incomputing.com>
To: "Bjorn Helgaas" <helgaas@...nel.org>
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: Re: [PATCH v9 2/2] PCI: eic7700: Add Eswin PCIe host controller
driver
> 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?
Okey, thanks.
Perhaps it's a problem. Maybe PCI_DEVICE_ID_EIC7700 is better?
>
> > +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.
After reading patch's comments, lists and arrays seem to be good choices,
I don't have any particularly good ideas for the time being. Anyway, this
is a very good patch that supports multiple Root Ports resolutions.
>
> > + 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?
I only considered that there is currently only one Root Port. Maybe
there will be multiple Root Ports in the future.
Perhaps this is the best:
list_for_each_entry_safe(port, tmp, &pcie->ports, list){
if (!IS_ERR_OR_NULL(port->perst))
reset_control_put(port->perst);
list_del(&port->list);
}
>
> > +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?
In v2 patch, I referred to Mani's comments and removed the .remove()
callback, as follows:
"Since this controller implements irqchip using the DWC core driver,
it is not safe to remove it during runtime."
https://lore.kernel.org/linux-pci/jghozurjqyhmtunivotitgs67h6xo4sb46qcycnbbwyvjcm4ek@vgq75olazmoi/
In addition, remove .remove() callback, because this driver has been
modified to builtin_platform_driver and does not support HotPlug,
therefore, the .remove() callback is not needed. Do you have any
better suggestions?
Kind regards,
Senchuan Zhang
>
> > + .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);
Powered by blists - more mailing lists