Re: [PATCH v3 3/9] PCI: keystone: Use hwirq to get the legacy IRQ number offset

From: Marc Zyngier <marc.zyngier@arm.com>
To: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Kishon Vijay Abraham I <kishon@ti.com>,
	Murali Karicheri <m-karicheri2@ti.com>,
	Bjorn Helgaas <bhelgaas@google.com>,
	Jingoo Han <jingoohan1@gmail.com>,
	Gustavo Pimentel <gustavo.pimentel@synopsys.com>,
	<linux-pci@vger.kernel.org>,
	<linux-arm-kernel@lists.infradead.org>,
	<linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v3 3/9] PCI: keystone: Use hwirq to get the legacy IRQ number offset
Date: Thu, 14 Feb 2019 12:46:49 +0000	[thread overview]
Message-ID: <86ftsqsfk6.wl-marc.zyngier@arm.com> (raw)
In-Reply-To: <20190214103138.GA24593@e107981-ln.cambridge.arm.com>

Hi Lirenzo,

On Thu, 14 Feb 2019 10:31:38 +0000,
Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> 
> [CC'ed MarcZ]
> 
> On Thu, Feb 14, 2019 at 10:27:19AM +0530, Kishon Vijay Abraham I wrote:
> > Hi Lorenzo,
> > 
> > On 13/02/19 10:27 PM, Lorenzo Pieralisi wrote:
> > > On Wed, Feb 13, 2019 at 06:56:23PM +0530, Kishon Vijay Abraham I wrote:
> > >> ks_pcie_legacy_irq_handler() uses 'virq' to get the IRQ number offset.
> > >> This offset is used to get the correct IRQ_STATUS register
> > >> corresponding to the IRQ line that raised the interrupt.
> > >> There is no guarantee that 'virq' assigned for consecutive hardware
> > >> IRQ will be contiguous. And this might get us an incorrect IRQ number
> > >> offset.
> > >>
> > >> Fix it here by using 'hwirq' to get the IRQ number offset.
> > >>
> > >> Link: https://lkml.kernel.org/r/bb081d21-7c03-0357-4294-7e92d95d838c@arm.com
> > >> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> > >> ---
> > >>  drivers/pci/controller/dwc/pci-keystone.c | 17 +++++++++++++----
> > >>  1 file changed, 13 insertions(+), 4 deletions(-)
> > >>
> > >> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
> > >> index e8b1d8eca78e..d35ac712a9f8 100644
> > >> --- a/drivers/pci/controller/dwc/pci-keystone.c
> > >> +++ b/drivers/pci/controller/dwc/pci-keystone.c
> > >> @@ -87,7 +87,7 @@ struct keystone_pcie {
> > >>  	struct dw_pcie		*pci;
> > >>  	/* PCI Device ID */
> > >>  	u32			device_id;
> > >> -	int			legacy_host_irqs[PCI_NUM_INTX];
> > >> +	int			legacy_host_irq;
> > >>  	struct			device_node *legacy_intc_np;
> > >>  
> > >>  	int			msi_host_irqs[MAX_MSI_HOST_IRQS];
> > >> @@ -582,11 +582,11 @@ static void ks_pcie_msi_irq_handler(struct irq_desc *desc)
> > >>   */
> > >>  static void ks_pcie_legacy_irq_handler(struct irq_desc *desc)
> > >>  {
> > >> -	unsigned int irq = irq_desc_get_irq(desc);
> > >> +	unsigned int irq = desc->irq_data.hwirq;
> > >>  	struct keystone_pcie *ks_pcie = irq_desc_get_handler_data(desc);
> > >>  	struct dw_pcie *pci = ks_pcie->pci;
> > >>  	struct device *dev = pci->dev;
> > >> -	u32 irq_offset = irq - ks_pcie->legacy_host_irqs[0];
> > >> +	u32 irq_offset = irq - ks_pcie->legacy_host_irq;
> > > 
> > > I think you should use the plain hwirq number (that if I understand
> > > correctly range in [0,3]) and drop legacy_host_irq.
> > 
> > The hwirq is [80, 83] for Keystone. We store legacy_host_irq (for Keystone it
> > is 80) to get the correct offset in the range [0, 3].
> 
> IIUC the _parent_ hw_irq number should be what you need here, Marc (if
> he has time) can correct me if I am wrong (or what I am suggesting is
> a violation of IRQ domain bstraction usage).

It isn't pretty, but it is something we already do in some cases (see
irq-gic-v3-mbi.c::mbi_compose_msi_msg as an infamous example).

Thanks,

	M.

-- 
Jazz is not dead, it just smell funny.