linux-kernel - Re: 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: <6607c5b8.21d6.19ba27df74f.Coremail.zhangsenchuan@eswincomputing.com>
Date: Fri, 9 Jan 2026 19:22:02 +0800 (GMT+08:00)
From: zhangsenchuan <zhangsenchuan@...incomputing.com>
To: "Bjorn Helgaas" <helgaas@...nel.org>
Cc: "Manivannan Sadhasivam" <mani@...nel.org>, bhelgaas@...gle.com,
	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

> > On Tue, Jan 06, 2026 at 08:43:11PM +0800, zhangsenchuan wrote:
> > > > 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?
> 
> Check pci_ids.h and follow the style used there.  Device ID macros
> typically include both the vendor and the device.

Okey, thanks.

> 
> > > > > +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?
> > 
> > Yes, builtin_platform_driver() wouldn't allow the users to remove
> > the module. So remove() callback will become useless. The reason why
> > this driver is tristate is that it could be loaded from rootfs and
> > not always statically built to the kernel image.
> 
> This .remove() vs IRQ thing is a perennial issue and it's hard to know
> what style new drivers should copy.
> 
> There are lots of DWC-based drivers that are tristate, implement
> .remove(), and use module_platform_driver() (e.g., bt1, kirin,
> tegra194, rcar-gen4, exynos, k1, stm32).  Is there something different
> about the way they implement irqchip that makes .remove() safe?

Hi Bjorn, Mani,

The comments are as follows:
"You can make it tristate as you've used builtin_platform_driver() which
guarantees that this driver won't be removed once loaded."
https://lore.kernel.org/linux-pci/uijg47suvluvamftyxwc65kl34eo2eu2af2o5aia4nu45hanqc@grcr2bjgph2i/

Do not add the remove callback. It needs to be set to a bool：
In v6 patch, it was mentioned to set tristate. Now, after careful 
consideration, setting tristate can allow loading as a module, but the 
driver implementation does not have a remove function. If it exists in 
the form of a module, after testing, When insmod driver is followed by 
rmmod driver, the resources cannot be released, and problems will occur
when insmod driver is used again. So I think that if the remove callback
function is not provided in the form of builtin, it can only be set to 
bool.

Add the remove callback. It can make it tristate:
Questions about removing it during runtime. I don't have a very good idea.
I still don't quite understand why it's not safe. Could you explain it to 
me?

At present, refer to other manufacturers, i think there are two ways to 
achieve it.
1.Set a bool. Do not add the remove function, module loading is not 
allowed, and the driver currently does not support HotPlug.
2.Set a tristate, add .remove callback.

I think the first one might be better for me, because there is no need 
to add the remove function, my understanding might also be incorrect. 
Please review it for me. Thanks！

Kind regards,
Senchuan