RE: [PATCH v2 05/15] PCI: xilinx: Convert to MSI domains

[Bharat Kumar Gogada]

Hi Marc,

Thanks for the patch. 

> Subject: [PATCH v2 05/15] PCI: xilinx: Convert to MSI domains
> 
> In anticipation of the removal of the msi_controller structure, convert the
> ancient xilinx host controller driver to MSI domains.
> 
> We end-up with the usual two domain structure, the top one being a generic
> PCI/MSI domain, the bottom one being xilinx-specific and handling the
> actual HW interrupt allocation.
> 
> This allows us to fix some of the most appalling MSI programming, where the
> message programmed in the device is the virtual IRQ number instead of the
> allocated vector number. The allocator is also made safe with a mutex. This
> should allow support for MultiMSI, but I decided not to even try, since I
> cannot test it.
> 
> Acked-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
> Signed-off-by: Marc Zyngier <maz@xxxxxxxxxx>
> ---
>  drivers/pci/controller/Kconfig       |   2 +-
>  drivers/pci/controller/pcie-xilinx.c | 234 +++++++++++----------------
>  2 files changed, 97 insertions(+), 139 deletions(-)
> 
> diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
> index 5cc07d28a3a0..60045f7aafc5 100644
...


> +static struct irq_chip xilinx_msi_bottom_chip = {
> +	.name			= "Xilinx MSI",
> +	.irq_set_affinity 	= xilinx_msi_set_affinity,
> +	.irq_compose_msi_msg	= xilinx_compose_msi_msg,
> +};
> 
I see a crash while testing MSI in handle_edge_irq
[<c015bdd4>] (handle_edge_irq) from [<c0157164>] (generic_handle_irq+0x28/0x38)
[<c0157164>] (generic_handle_irq) from [<c03a9714>] (xilinx_pcie_intr_handler+0x17c/0x2b0)
[<c03a9714>] (xilinx_pcie_intr_handler) from [<c0157d94>] (__handle_irq_event_percpu+0x3c/0xc0)
[<c0157d94>] (__handle_irq_event_percpu) from [<c0157e44>] (handle_irq_event_percpu+0x2c/0x7c)
[<c0157e44>] (handle_irq_event_percpu) from [<c0157ecc>] (handle_irq_event+0x38/0x5c)
[<c0157ecc>] (handle_irq_event) from [<c015bc8c>] (handle_fasteoi_irq+0x9c/0x114)

void handle_edge_irq(struct irq_desc *desc) {
...
        kstat_incr_irqs_this_cpu(desc);

        /* Start handling the irq */
        desc->irq_data.chip->irq_ack(&desc->irq_data);	//There is no check here for ack function is registered for chip
..
}

> -/**
> - * xilinx_pcie_msi_setup_irq - Setup MSI request
> - * @chip: MSI chip pointer
> - * @pdev: PCIe device pointer
> - * @desc: MSI descriptor pointer
> - *
> - * Return: '0' on success and error value on failure
> - */
> -static int xilinx_pcie_msi_setup_irq(struct msi_controller *chip,
> -				     struct pci_dev *pdev,
> -				     struct msi_desc *desc)
> +static int xilinx_msi_domain_alloc(struct irq_domain *domain, unsigned int
> virq,
> +				  unsigned int nr_irqs, void *args)
>  {
> -	struct xilinx_pcie_port *port = pdev->bus->sysdata;
> -	unsigned int irq;
> -	int hwirq;
> -	struct msi_msg msg;
> -	phys_addr_t msg_addr;
> +	struct xilinx_pcie_port *port = domain->host_data;
> +	int hwirq, i;
> +
> +	mutex_lock(&port->map_lock);
> +
> +	hwirq = bitmap_find_free_region(port->msi_map,
> XILINX_NUM_MSI_IRQS,
> +order_base_2(nr_irqs));
> +
> +	mutex_unlock(&port->map_lock);
> 
> -	hwirq = xilinx_pcie_assign_msi();
>  	if (hwirq < 0)
> -		return hwirq;
> +		return -ENOSPC;
> 
> -	irq = irq_create_mapping(port->msi_domain, hwirq);
> -	if (!irq)
> -		return -EINVAL;
> +	for (i = 0; i < nr_irqs; i++)
> +		irq_domain_set_info(domain, virq + i, hwirq + i,
> +				    &xilinx_msi_bottom_chip, domain-
> >host_data,
> +				    handle_edge_irq, NULL, NULL);
> 
> -	irq_set_msi_desc(irq, desc);
> +	return 0;
> +}
>

Regards,
Bharat