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

[Marc Zyngier <maz@kernel.org>]

Hi Thierry,

On Fri, 04 Sep 2020 11:56:13 +0100,
Thierry Reding <thierry.reding@gmail.com> wrote:
> 
> From: Marc Zyngier <maz@kernel.org>
> 
> 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@kernel.org>
> [treding@nvidia.com: fix, clean up and address TODOs from Marc's draft]
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
> 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.