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
> 

Reply via email to