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

[Lorenzo Pieralisi]

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
>