linux-kernel - Re: [PATCH v3 2/5] PCI: sg2042: Add Sophgo SG2042 PCIe 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] [day] [month] [year] [list]

Message-ID: <20250207174939.w2e2bdvvcmqojlse@thinkpad>
Date: Fri, 7 Feb 2025 23:19:39 +0530
From: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
To: Marc Zyngier <maz@...nel.org>
Cc: Chen Wang <unicorn_wang@...look.com>, Chen Wang <unicornxw@...il.com>,
	kw@...ux.com, u.kleine-koenig@...libre.com, aou@...s.berkeley.edu,
	arnd@...db.de, bhelgaas@...gle.com, conor+dt@...nel.org,
	guoren@...nel.org, inochiama@...look.com, krzk+dt@...nel.org,
	lee@...nel.org, lpieralisi@...nel.org, palmer@...belt.com,
	paul.walmsley@...ive.com, pbrobinson@...il.com, robh@...nel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-pci@...r.kernel.org, linux-riscv@...ts.infradead.org,
	chao.wei@...hgo.com, xiaoguang.xing@...hgo.com,
	fengchun.li@...hgo.com, helgaas@...nel.org
Subject: Re: [PATCH v3 2/5] PCI: sg2042: Add Sophgo SG2042 PCIe driver

On Thu, Jan 23, 2025 at 12:12:03PM +0000, Marc Zyngier wrote:
> On Wed, 22 Jan 2025 17:34:51 +0000,
> Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org> wrote:
> > 
> > + Marc (for the IRQCHIP implementation review)
> > 
> > On Wed, Jan 22, 2025 at 09:28:12PM +0800, Chen Wang wrote:
> > > 
> > > > > +static int sg2042_pcie_setup_msi(struct sg2042_pcie *pcie,
> > > > > +				 struct device_node *msi_node)
> > > > > +{
> > > > > +	struct device *dev = pcie->cdns_pcie->dev;
> > > > > +	struct fwnode_handle *fwnode = of_node_to_fwnode(dev->of_node);
> > > > > +	struct irq_domain *parent_domain;
> > > > > +	int ret = 0;
> > > > > +
> > > > > +	if (!of_property_read_bool(msi_node, "msi-controller"))
> > > > > +		return -ENODEV;
> > > > > +
> > > > > +	ret = of_irq_get_byname(msi_node, "msi");
> > > > > +	if (ret <= 0) {
> > > > > +		dev_err(dev, "%pOF: failed to get MSI irq\n", msi_node);
> > > > > +		return ret;
> > > > > +	}
> > > > > +	pcie->msi_irq = ret;
> > > > > +
> > > > > +	irq_set_chained_handler_and_data(pcie->msi_irq,
> > > > > +					 sg2042_pcie_msi_chained_isr, pcie);
> > > > > +
> > > > > +	parent_domain = irq_domain_create_linear(fwnode, MSI_DEF_NUM_VECTORS,
> > > > > +						 &sg2042_pcie_msi_domain_ops, pcie);
> > > > > +	if (!parent_domain) {
> > > > > +		dev_err(dev, "%pfw: Failed to create IRQ domain\n", fwnode);
> > > > > +		return -ENOMEM;
> > > > > +	}
> > > > > +	irq_domain_update_bus_token(parent_domain, DOMAIN_BUS_NEXUS);
> > > > > +
> > > > The MSI controller is wired to PLIC isn't it? If so, why can't you use
> > > > hierarchial MSI domain implementation as like other controller drivers?
> > > 
> > > The method used here is somewhat similar to dw_pcie_allocate_domains() in
> > > drivers/pci/controller/dwc/pcie-designware-host.c. This MSI controller is
> > > about Method A, the PCIe controller implements an MSI interrupt controller
> > > inside, and connect to PLIC upward through only ONE interrupt line. Because
> > > MSI to PLIC is multiple to one, I use linear mode here and use chained ISR
> > > to handle the interrupts.
> > > 
> > 
> > Hmm, ok. I'm not an IRQCHIP expert, but I'll defer to Marc to review the IRQCHIP
> > implementation part.
> 
> I don't offer this service anymore, I'm afraid.
> 
> As for the "I create my own non-hierarchical IRQ domain", this is
> something that happens for all completely mis-designed interrupt
> controllers, MSI or not, that multiplex interrupts.
> 
> These implementations are stuck in the previous century, and seeing
> this on modern designs, for a "server SoC", is really pathetic.
> 
> maybe you now understand why I don't offer this sort of reviewing
> service anymore.
> 

Ok, I can understand your pain as a maintainer. I'll try my best to review these
implementations as I have no other choice :)

- Mani

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