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@xxxxxxx > Signed-off-by: Kishon Vijay Abraham I <kishon@xxxxxx> > --- > 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. See below. > struct irq_chip *chip = irq_desc_get_chip(desc); > > dev_dbg(dev, ": Handling legacy irq %d\n", irq); > @@ -657,6 +657,7 @@ static int ks_pcie_config_legacy_irq(struct keystone_pcie *ks_pcie) > struct device_node *np = ks_pcie->np; > struct device_node *intc_np; > int irq_count, irq, ret, i; > + struct irq_data *irq_data; > > intc_np = of_get_child_by_name(np, "legacy-interrupt-controller"); > if (!intc_np) { > @@ -677,7 +678,15 @@ static int ks_pcie_config_legacy_irq(struct keystone_pcie *ks_pcie) > ret = -EINVAL; > goto err; > } > - ks_pcie->legacy_host_irqs[i] = irq; > + > + if (!ks_pcie->legacy_host_irq) { > + irq_data = irq_get_irq_data(irq); > + if (!irq_data) { > + ret = -EINVAL; > + goto err; > + } > + ks_pcie->legacy_host_irq = irq_data->hwirq; This depends on DT property ordering and may well be wrong. On top of that as I said above I do not think it is even needed. Thanks, Lorenzo > + } > > irq_set_chained_handler_and_data(irq, > ks_pcie_legacy_irq_handler, > -- > 2.17.1 >
