linux-kernel - RE: [PATCH v5] PCI: Xilinx-NWL-PCIe: Added support for Xilinx NWL PCIe Host Controller

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]

Date:	Wed, 28 Oct 2015 10:17:22 +0000
From:	Bharat Kumar Gogada <bharat.kumar.gogada@...inx.com>
To:	Bjorn Helgaas <helgaas@...nel.org>
CC:	"robh+dt@...nel.org" <robh+dt@...nel.org>,
	"pawel.moll@....com" <pawel.moll@....com>,
	"mark.rutland@....com" <mark.rutland@....com>,
	"ijc+devicetree@...lion.org.uk" <ijc+devicetree@...lion.org.uk>,
	"galak@...eaurora.org" <galak@...eaurora.org>,
	Michal Simek <michals@...inx.com>,
	Soren Brinkmann <sorenb@...inx.com>,
	"bhelgaas@...gle.com" <bhelgaas@...gle.com>,
	"arnd@...db.de" <arnd@...db.de>,
	"tinamdar@....com" <tinamdar@....com>,
	"treding@...dia.com" <treding@...dia.com>,
	"rjui@...adcom.com" <rjui@...adcom.com>,
	"Minghuan.Lian@...escale.com" <Minghuan.Lian@...escale.com>,
	"m-karicheri2@...com" <m-karicheri2@...com>,
	"hauke@...ke-m.de" <hauke@...ke-m.de>,
	"marc.zyngier@....com" <marc.zyngier@....com>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
	Ravikiran Gummaluri <rgummal@...inx.com>
Subject: RE: [PATCH v5] PCI: Xilinx-NWL-PCIe: Added support for Xilinx NWL
 PCIe Host Controller

> On Mon, Oct 26, 2015 at 08:26:26PM +0530, Bharat Kumar Gogada wrote:
> > Adding PCIe Root Port driver for Xilinx PCIe NWL bridge IP.

> > +
> > +	while ((status = nwl_bridge_readl(pcie, MSGF_MSI_STATUS_LO)) !=
> 0) {
> > +		for_each_set_bit(bit, &status, 32) {
> > +			nwl_bridge_writel(pcie, 1 << bit,
> MSGF_MSI_STATUS_LO);
> > +			virq = irq_find_mapping(msi->dev_domain, bit);
> > +			if (virq)
> > +				generic_handle_irq(virq);
> > +		}
> > +	}
> > +
> > +	chained_irq_exit(chip, desc);
> > +}
> 
> These are basically identical.  Can you factor them out somehow to avoid
> repeating the code?

Is it okay if irq_set_chained_handler_and_data being invoked with two different interrupt numbers, but pointing to 
same interrupt handler?

> > +
> > +	pcie->legacy_irq_domain =
> irq_domain_add_linear(legacy_intc_node, 4,
> > +
> 	&legacy_domain_ops,
> > +							pcie);
> > +
> > +	if (!pcie->legacy_irq_domain) {
> > +		dev_err(pcie->dev, "failed to create IRQ domain\n");
> > +		return -ENOMEM;
> > +	}
> > +
> > +#ifdef CONFIG_PCI_MSI
> > +	msi->dev_domain = irq_domain_add_linear(NULL,
> INT_PCI_MSI_NR,
> > +					&dev_msi_domain_ops, pcie);
> > +	if (!msi->dev_domain) {
> > +		dev_err(pcie->dev, "failed to create dev IRQ domain\n");
> > +		return -ENOMEM;
> > +	}
> > +	msi->msi_chip.domain = pci_msi_create_irq_domain(node,
> > +
> 	&nwl_msi_domain_info,
> > +							msi->dev_domain);
> > +	if (!msi->msi_chip.domain) {
> > +		dev_err(pcie->dev, "failed to create msi IRQ domain\n");
> > +		irq_domain_remove(msi->dev_domain);
> > +		return -ENOMEM;
> > +	}
> > +#endif
> > +	return 0;
> > +}
> > +
> > +static int nwl_pcie_enable_msi(struct nwl_pcie *pcie, struct pci_bus
> > +*bus) {
> 
> It looks strange to have all the "#ifdef CONFIG_PCI_MSI" above, and here
> we have this long MSI-related function without any ifdefs around it.  Seems
> like this should be ifdef'ed also?  What about nwl_pcie_msi_handler_high(),
> nwl_pcie_msi_handler_low(), nwl_compose_msi_msg(),
> nwl_msi_set_affinity(), etc.?
> 
In probe I'm invoking "nwl_pcie_enable_msi" using "if (IS_ENABLED(CONFIG_PCI_MSI)) " check, since this is at run time 
I haven't kept above mentioned functions under #ifdef CONFIG_PCI_MSI.
The above MSI domain allocation was under ifdef, since if driver was compiled for legacy some of the MSI hierarchy API's and structures aren't available.

> > +	struct platform_device *pdev = to_platform_device(pcie->dev);
> > +	struct nwl_msi *msi = &pcie->msi;

Regards,
Bharat
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/