pcie-designware msi_controller->dev handling

pcie-designware msi_controller->dev handling

[Thomas Petazzoni]

Jingoo, Pratyush,

I was going to some of the MSI handling code in the pcie-designware
driver, and stumbled across the following part of the code:

static struct msi_controller dw_pcie_msi_chip = {
	.setup_irq = dw_msi_setup_irq,
	.setup_irqs = dw_msi_setup_irqs,
	.teardown_irq = dw_msi_teardown_irq,
};

[...]

int dw_pcie_host_init(struct pcie_port *pp)
{
	[...]

	if (IS_ENABLED(CONFIG_PCI_MSI)) {
		bus = pci_scan_root_bus_msi(pp->dev, pp->root_bus_nr,
					    &dw_pcie_ops, pp, &res,
					    &dw_pcie_msi_chip);
		dw_pcie_msi_chip.dev = pp->dev;
	[...]
}

I am a bit surprised by the dw_pcie_msi_chip.dev assignment here.
Indeed, there is only one global dw_pcie_msi_chip structure, but the
function dw_pcie_host_init() can be called several times, once for each
PCIe controller available on a platform.

This means that the dw_pcie_msi_chip.dev will point to the last
Designware PCIe controller that was registered.

If the MSI controller is indeed part of the PCIe controller IP, I think
the msi_controller structure needs to be dynamically allocated, so that
you have one instance for each PCIe controller.

Am I missing something?

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: pcie-designware msi_controller->dev handling

[Pratyush Anand]

Hi Thomas,

Sorry for delayed response.

On Fri, Aug 5, 2016 at 9:20 PM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> Jingoo, Pratyush,
>
> I was going to some of the MSI handling code in the pcie-designware
> driver, and stumbled across the following part of the code:
>
> static struct msi_controller dw_pcie_msi_chip = {
>         .setup_irq = dw_msi_setup_irq,
>         .setup_irqs = dw_msi_setup_irqs,
>         .teardown_irq = dw_msi_teardown_irq,
> };
>
> [...]
>
> int dw_pcie_host_init(struct pcie_port *pp)
> {
>         [...]
>
>         if (IS_ENABLED(CONFIG_PCI_MSI)) {
>                 bus = pci_scan_root_bus_msi(pp->dev, pp->root_bus_nr,
>                                             &dw_pcie_ops, pp, &res,
>                                             &dw_pcie_msi_chip);
>                 dw_pcie_msi_chip.dev = pp->dev;
>         [...]
> }
>
> I am a bit surprised by the dw_pcie_msi_chip.dev assignment here.
> Indeed, there is only one global dw_pcie_msi_chip structure, but the
> function dw_pcie_host_init() can be called several times, once for each
> PCIe controller available on a platform.
>
> This means that the dw_pcie_msi_chip.dev will point to the last
> Designware PCIe controller that was registered.
>
> If the MSI controller is indeed part of the PCIe controller IP, I think
> the msi_controller structure needs to be dynamically allocated, so that
> you have one instance for each PCIe controller.

I think, you are right. Probably, we can make "struct msi_controller"
as part of "struct pcie_port", and can initialize it in
dw_pcie_host_init().

Thanks a lot for finding it.

~Pratyush

>
> Am I missing something?
>
> Best regards,
>
> Thomas
> --
> Thomas Petazzoni, CTO, Free Electrons
> Embedded Linux, Kernel and Android engineering
> http://free-electrons.com