Re: [PATCH] PCI: tegra: Convert to MSI domains

From: Marc Zyngier
Date: Fri Sep 04 2020 - 11:48:58 EST


Hi Thierry,

On Fri, 04 Sep 2020 11:56:13 +0100,
Thierry Reding <thierry.reding@xxxxxxxxx> wrote:
>
> From: Marc Zyngier <maz@xxxxxxxxxx>
>
> In anticipation of the removal of the msi_controller structure, convert
> the Tegra 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 Tegra-specific and handling
> the actual HW interrupt allocation.
>
> While at it, convert the normal interrupt handler to a chained handler,
> handle the controller's MSI IRQ edge triggered, support multiple MSIs
> per device and use the AFI_MSI_EN_VEC* registers to provide MSI masking.
>
> Signed-off-by: Marc Zyngier <maz@xxxxxxxxxx>
> [treding@xxxxxxxxxx: fix, clean up and address TODOs from Marc's draft]
> Signed-off-by: Thierry Reding <treding@xxxxxxxxxx>
> ---
> This is basically Marc's patch from here:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/commit/?h=irq/tegra-msi&id=8ba6858f07a7554d4e56a6d5103cd914dcf23af0
>
> That didn't work as-is, but it was pretty easy to get it to build and
> run properly. Once I had that I went through Marc's list of TODOs and
> addressed them, which was also not very difficult.
>
> I haven't done any extensive testing, but I can boot fine over NFS with
> a PCI Ethernet card. Given how fundamental this is, I would expect that
> to be a good enough test.
>
> Marc, I've kept your authorship on this. There are substantial changes
> from your version, but you had laid out the plan for all these changes,
> so it seemed fair. However, let me know if you're not comfortable with
> taking responsibility for any potential bugs I've introduced.

Thanks a lot for picking this up at short notice. Definitely happy
with you preserving the authorship, though I wouldn't object if you
did it the other way around.

> I had originally considered sending this out as a series with more
> intermediate steps, but it turns out most of the TODO's that Marc had
> listed are related, so this could've been at maximum two or three
> patches. That didn't seem worth splitting up for. I think most of this
> work is still all related to the MSI domain conversion, so it makes
> sense to keep it in one patch.
>
> drivers/pci/controller/pci-tegra.c | 316 +++++++++++++++--------------
> 1 file changed, 165 insertions(+), 151 deletions(-)
>
> diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c
> index c1d34353c29b..f7f92718ce40 100644
> --- a/drivers/pci/controller/pci-tegra.c
> +++ b/drivers/pci/controller/pci-tegra.c

[...]

> +static void tegra_msi_irq_mask(struct irq_data *d)
> +{
> + struct tegra_msi *msi = irq_data_get_irq_chip_data(d);
> + struct tegra_pcie *pcie = msi_to_pcie(msi);
> + unsigned int index = d->hwirq / 32;
> + u32 value;
> +
> + value = afi_readl(pcie, AFI_MSI_EN_VEC(index));
> + value &= ~BIT(d->hwirq % 32);
> + afi_writel(pcie, value, AFI_MSI_EN_VEC(index));

You need some extra locking here, as this register looks shared with
another 31 MSIs. Same for the unmask.

[...]

> +static int tegra_allocate_domains(struct tegra_msi *msi)
> {
> - irq_set_chip_and_handler(irq, &tegra_msi_irq_chip, handle_simple_irq);
> - irq_set_chip_data(irq, domain->host_data);
> + struct tegra_pcie *pcie = container_of(msi, struct tegra_pcie, msi);
> + struct fwnode_handle *fwnode = of_node_to_fwnode(pcie->dev->of_node);
> + struct irq_domain *parent;
>
> - tegra_cpuidle_pcie_irqs_in_use();
> + msi->top.name = "Tegra PCIe MSI";
> + msi->top.irq_ack = tegra_msi_top_irq_ack;
> + msi->top.irq_mask = tegra_msi_top_irq_mask;
> + msi->top.irq_unmask = tegra_msi_top_irq_unmask;
> +
> + msi->info.flags = MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS | MSI_FLAG_PCI_MSIX;
> + msi->info.chip = &msi->top;
> +
> + msi->bottom.name = "Tegra MSI";
> + msi->bottom.irq_ack = tegra_msi_irq_ack;
> + msi->bottom.irq_mask = tegra_msi_irq_mask;
> + msi->bottom.irq_unmask = tegra_msi_irq_unmask;
> + msi->bottom.irq_set_affinity = tegra_msi_set_affinity;
> + msi->bottom.irq_compose_msi_msg = tegra_compose_msi_msg;

It's not obvious to me why these irq_chip and msi_domain_info are
dynamically allocated, given that they only contain static data, which
should be common across PCIe controllers.

Am I missing something here?

The reason I'm being picky about it is that I'm trying to get to a
point where we can make most struct irq_chip to be const and thus
marked read-only (PM is getting in the way at the moment).

The rest looks good to me, and only the above locking issue is a real
showstopper.

Thanks,

M.

--
Without deviation from the norm, progress is not possible.