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

[Gerd Bayer]

On Wed, 2025-12-03 at 15:36 +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@xxxxxxxxxxxxx
> 

  [ ... snip ... ]

> +static void zpci_msi_domain_free(struct irq_domain *domain, unsigned int virq,
> +				 unsigned int nr_irqs)
> +{
> +	struct zpci_dev *zdev;
> +	struct msi_desc *desc;
> +	struct irq_data *d;
> +	unsigned long bit;
> +	unsigned int cpu;
> +	u16 msi_index;
> +	int i;
> +
> +	for (i = 0; i < nr_irqs; i++) {
> +		d = irq_domain_get_irq_data(domain, virq + i);
> +		msi_index = zpci_decode_hwirq_msi_index(d->hwirq);
> +		desc = irq_data_get_msi_desc(d);
> +		zdev = to_zpci_dev(desc->dev);
> +		bit = zdev->msi_first_bit + msi_index;
>  
> +		if (irq_delivery == DIRECTED) {
>  			for_each_possible_cpu(cpu) {
> -				for (i = 0; i < irqs_per_msi; i++)
> -					airq_iv_set_data(zpci_ibv[cpu],
> -							 hwirq + i, irq + i);
> +				airq_iv_set_ptr(zpci_ibv[cpu], bit + i, 0);
> +				airq_iv_set_data(zpci_ibv[cpu], bit + i, 0);
>  			}
>  		} else {
> -			msg.address_lo = zdev->msi_addr & 0xffffffff;
> -			for (i = 0; i < irqs_per_msi; i++)
> -				airq_iv_set_data(zdev->aibv, hwirq + i, irq + i);
> +			airq_iv_set_ptr(zdev->aibv, bit + i, 0);
> +			airq_iv_set_data(zdev->aibv, bit + i, 0);
>  		}
> -		msg.address_hi = zdev->msi_addr >> 32;
> -		pci_write_msi_msg(irq, &msg);
> -		hwirq += irqs_per_msi;
> +
> +		irq_domain_reset_irq_data(d);
>  	}
> +} 

Thanks for addressing my concern about clearing the airq data!

FWIW, what you thing about abstracting out the airq clearing stuff with
something like this diff on top, so the loop body remains somewhat
short and zpci_msi_domain_free() keeps its working set of local
variables.


diff --git a/arch/s390/pci/pci_irq.c b/arch/s390/pci/pci_irq.c
index 5639789dc58f..3322d8c9aff1 100644
--- a/arch/s390/pci/pci_irq.c
+++ b/arch/s390/pci/pci_irq.c
@@ -439,34 +439,37 @@ static int zpci_msi_domain_alloc(struct
irq_domain *domain, unsigned int virq,
        return 0;
 }
 
-static void zpci_msi_domain_free(struct irq_domain *domain, unsigned
int virq,
-                                unsigned int nr_irqs)
+static void zpci_msi_clear_airq(struct irq_data *d, int i)
 {
-       struct zpci_dev *zdev;
-       struct msi_desc *desc;
-       struct irq_data *d;
+       struct msi_desc *desc = irq_data_get_msi_desc(d);
+       struct zpci_dev *zdev = to_zpci_dev(desc->dev);
        unsigned long bit;
        unsigned int cpu;
        u16 msi_index;
-       int i;
 
-       for (i = 0; i < nr_irqs; i++) {
-               d = irq_domain_get_irq_data(domain, virq + i);
-               msi_index = zpci_decode_hwirq_msi_index(d->hwirq);
-               desc = irq_data_get_msi_desc(d);
-               zdev = to_zpci_dev(desc->dev);
-               bit = zdev->msi_first_bit + msi_index;
+       msi_index = zpci_decode_hwirq_msi_index(d->hwirq);
+       bit = zdev->msi_first_bit + msi_index;
 
-               if (irq_delivery == DIRECTED) {
-                       for_each_possible_cpu(cpu) {
-                               airq_iv_set_ptr(zpci_ibv[cpu], bit + i,
0);
-                               airq_iv_set_data(zpci_ibv[cpu], bit +
i, 0);
-                       }
-               } else {
-                       airq_iv_set_ptr(zdev->aibv, bit + i, 0);
-                       airq_iv_set_data(zdev->aibv, bit + i, 0);
+       if (irq_delivery == DIRECTED) {
+               for_each_possible_cpu(cpu) {
+                       airq_iv_set_ptr(zpci_ibv[cpu], bit + i, 0);
+                       airq_iv_set_data(zpci_ibv[cpu], bit + i, 0);
                }
+       } else {
+               airq_iv_set_ptr(zdev->aibv, bit + i, 0);
+               airq_iv_set_data(zdev->aibv, bit + i, 0);
+       }
+}
 
+static void zpci_msi_domain_free(struct irq_domain *domain, unsigned
int virq,
+                                unsigned int nr_irqs)
+{
+       struct irq_data *d;
+       int i;
+
+       for (i = 0; i < nr_irqs; i++) {
+               d = irq_domain_get_irq_data(domain, virq + i);
+               zpci_msi_clear_airq(d, i);
                irq_domain_reset_irq_data(d);
        }
 }

Sorry for me keeping nagging...
Gerd