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-7e92d95d8...@arm.com
> Signed-off-by: Kishon Vijay Abraham I <kis...@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.
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
>
