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

[Tobias Schumacher]

On Wed Dec 3, 2025 at 5:14 PM CET, Gerd Bayer wrote:
> 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.

Sounds good, will do.

> 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...

No worries :-)

Thanks
Tobias