Message-ID: <ddhcpvasw3ozlb6u6tzarhsliszm44rsfoplvlplsyjnxvxenz@3gg73hro74gz>
Date: Wed, 21 Jan 2026 18:38:32 +0530
From: Manivannan Sadhasivam <mani@...nel.org>
To: zhangsenchuan <zhangsenchuan@...incomputing.com>
Cc: Bjorn Helgaas <helgaas@...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: [PATCH v9 2/2] PCI: eic7700: Add Eswin PCIe host controller
 driver

On Fri, Jan 09, 2026 at 07:22:02PM +0800, zhangsenchuan wrote:
> > > 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?
> 

It is mostly due to IRQ disposal concern. Kernel's IRQ core gives no guarantee
that all the client drivers requested IRQs would've freed them before the
irqchip drivers like the controller drivers dispose the IRQs in their remove
callback.

More info here: https://lkml.iu.edu/hypermail/linux/kernel/2207.2/08367.html

> 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！
> 

I'd prefer to make it tristate, builtin_platform_driver() and not have
.remove() callback.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Powered by blists - more mailing lists