Re: [PATCH 10/11] ARM: tegra: pcie: Add MSI support

[Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 2264 bytes --]

* Stephen Warren wrote:
> On 03/08/2012 07:51 AM, Thierry Reding wrote:
> > diff --git a/arch/arm/mach-tegra/pcie.c b/arch/arm/mach-tegra/pcie.c
> 
> > +static int tegra_pcie_enable_msi(struct platform_device *pdev)
> > +{
> > +	struct tegra_pcie_info *pcie = platform_get_drvdata(pdev);
> > +	volatile void *pages;
> > +	unsigned long base;
> > +	unsigned int msi;
> > +	int msi_base;
> > +	int err;
> > +	u32 reg;
> > +
> > +	mutex_init(&pcie->msi_lock);
> > +
> > +	msi_base = irq_alloc_descs(-1, 0, INT_PCI_MSI_NR, 0);
> > +	if (msi_base < 0) {
> > +		dev_err(&pdev->dev, "failed to allocate IRQs\n");
> > +		return msi_base;
> > +	}
> > +
> > +	pcie->msi_domain = irq_domain_add_legacy(pcie->dev->of_node,
> > +						 INT_PCI_MSI_NR, msi_base,
> > +						 0, &irq_domain_simple_ops,
> > +						 NULL);
> > +	if (!pcie->msi_domain) {
> > +		dev_err(&pdev->dev, "failed to create IRQ domain\n");
> 
> Free the IRQ descriptors in the error paths?
> 
> > +		return -ENOMEM;
> > +	}
> > +
> > +	pcie->msi_chip.name = "PCIe-MSI";
> > +	pcie->msi_chip.irq_enable = unmask_msi_irq;
> > +	pcie->msi_chip.irq_disable = mask_msi_irq;
> > +	pcie->msi_chip.irq_mask = mask_msi_irq;
> > +	pcie->msi_chip.irq_unmask = unmask_msi_irq;
> > +
> > +	for (msi = 0; msi < INT_PCI_MSI_NR; msi++) {
> > +		unsigned int irq = irq_find_mapping(pcie->msi_domain, msi);
> > +
> > +		irq_set_chip_data(irq, pcie);
> > +		irq_set_chip_and_handler(irq, &pcie->msi_chip,
> > +					 handle_simple_irq);
> > +		set_irq_flags(irq, IRQF_VALID);
> > +	}
> > +
> > +	err = platform_get_irq(pdev, 1);
> > +	if (err < 0) {
> > +		dev_err(&pdev->dev, "failed to get IRQ: %d\n", err);
> 
> Same here, and undo setting IRQF_VALID?

Does it make sense to explicitly unset the IRQF_VALID flag when the IRQ
descriptors are free'd afterwards anyway? I also think it would be necessary
to free the struct irq_domain, but I wasn't able to find any function that
does this other than just calling kfree(), which obviously wouldn't be
enough. Maybe Grant can shed some light onto this. I'm also Cc'ing Thomas
Gleixner as maintainer of the IRQ subsystem, he probably knows best how
dynamically allocated interrupts should be cleaned up.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]