Re: [PATCH 2/2] s390/pci: Migrate s390 IRQ logic to IRQ domain API

[Niklas Schnelle <schnelle@linux.ibm.com>]

On Wed, 2025-11-12 at 16:40 +0100, Tobias Schumacher wrote:
> s390 is one of the last architectures using the legacy API for setup and
> teardown of PCI MSI IRQs. Migrate the s390 IRQ allocation and teardown
> to the MSI parent domain API. For details, see:
> 
> https://lore.kernel.org/lkml/20221111120501.026511281@linutronix.de
> 
> In detail, create an MSI parent domain for zpci which is used by
> all PCI devices. When a PCI device sets up MSI or MSI-X IRQs, the
> library creates a per-device IRQ domain for this device, which is
> be used by the device for allocating and freeing IRQs.

Nit: superfluous word "be"

> 
> The per-device domain delegates this allocation and freeing to the
> parent-domain. In the end, the corresponding callbacks of the parent
> domain are responsible for allocating and freeing the IRQs.
--- snip ---
> diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
> index df22b10d91415e1ed183cc8add9ad0ac4293c50e..739a9a9a86a277be1ba750cb2e98af0547df89fd 100644
> --- a/arch/s390/Kconfig
> +++ b/arch/s390/Kconfig
> @@ -251,6 +251,7 @@ config S390
>  	select HOTPLUG_SMT
>  	select IOMMU_HELPER		if PCI
>  	select IOMMU_SUPPORT		if PCI
> +  select IRQ_MSI_LIB if PCI

The indentation is wrong here, instead of two spaces you want one tab
to match the context.

>  	select KASAN_VMALLOC if KASAN
>  	select LOCK_MM_AND_FIND_VMA
>  	select MMU_GATHER_MERGE_VMAS
--- snip ---
> +
> +static int zpci_msi_prepare(struct irq_domain *domain,
> +			    struct device *dev, int nvec,
> +			    msi_alloc_info_t *info)
> +{
--- snip ---
> -
> -		msg.data = hwirq - bit;
> +	zdev->msi_first_bit = bit;
> +	rc = zpci_set_irq(zdev);
> +	if (rc) {
> +		pr_err("Registering floating adapter interruptions for %s failed\n",
> +		       pci_name(pdev));

The error message is misleading as this could also happen for directed.
I'd just drop the "floating" in there. Also the wording is inconsistent
with the error message for allocation. I don't really have a preference
between "AIRQ" and "adapter interruptions" but from a Linux point of
view I think IRQ would be what I'd be grepping for which finds the
former.

>  		if (irq_delivery == DIRECTED) {
> -			if (msi->affinity)
--- snip ---
> 
> +static int __init zpci_create_parent_msi_domain(void)
> +{
> +	struct irq_domain_info info = {
> +		.fwnode		= irq_domain_alloc_named_fwnode("zpci_msi"),
> +		.ops		= &zpci_msi_domain_ops
> +	};

Nit: Missing empty line as the above is a variable definition

> +	if (!info.fwnode) {
> +		pr_err("Failed to allocate fwnode for MSI IRQ domain\n");
> +		return -ENOMEM;
> +	}
> +
> +	zpci_msi_parent_domain = msi_create_parent_irq_domain(&info, &zpci_msi_parent_ops);
> +	if (!zpci_msi_parent_domain) {
> +		irq_domain_free_fwnode(info.fwnode);
> +		pr_err("Failed to create MSI IRQ domain\n");
> +		return -ENOMEM;
> +	}
> +
> +	return 0;
> +}
> +
> 
> 
--- snip ---
> @@ -549,6 +625,7 @@ void __init zpci_irq_exit(void)
>  		}
>  	}
>  	kfree(zpci_ibv);
> +	irq_domain_remove(zpci_msi_parent_domain);
>  	if (zpci_sbv)
>  		airq_iv_release(zpci_sbv);
>  	unregister_adapter_interrupt(&zpci_airq);

Shouldn't zpci_irq_exit() also call irq_domain_free_fwnode() with
zpci_msi_parent_domain->fwnode? Otherwise I think a potential
subsequent zpci_irq_init() would fail because the fwnode already
exists. Not that we currently ever do that but still it should be all
undone.

Overall this looks great to me. I did run into an issue with my
experimental directed IRQ setup, we can work off list to solve this
before v2.

Thanks,
Niklas