Re: [PATCH] x86/irq: simplify loop in unmap_domain_pirq

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.02.2021 10:22, Roger Pau Monne wrote:
> The for loop in unmap_domain_pirq is unnecessary complicated, with
> several places where the index is incremented, and also different
> exit conditions spread between the loop body.
> 
> Simplify it by looping over each possible PIRQ using the for loop
> syntax, and remove all possible in-loop exit points.
> 
> No functional change intended.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Quite a bit better indeed. Just one nit below (can be taken care of
while committing, once the tree will re-open).

> @@ -2356,11 +2355,23 @@ int unmap_domain_pirq(struct domain *d, int pirq)
>      if ( msi_desc != NULL )
>          pci_disable_msi(msi_desc);
>  
> -    spin_lock_irqsave(&desc->lock, flags);
> -
> -    for ( i = 0; ; )
> +    for ( i = 0; i < nr; i++, info = pirq_info(d, pirq + i) )
>      {
> +        unsigned long flags;
> +
> +        if ( !info || info->arch.irq <= 0 )
> +        {
> +            printk(XENLOG_G_ERR "dom%d: MSI pirq %d not mapped\n",
> +                   d->domain_id, pirq + i);

%pd please as you touch/move this anyway.

> @@ -2378,45 +2389,6 @@ int unmap_domain_pirq(struct domain *d, int pirq)
>              desc->msi_desc = NULL;
>          }
>  
> -        if ( ++i == nr )
> -            break;
> -
> -        spin_unlock_irqrestore(&desc->lock, flags);
> -
> -        if ( !forced_unbind )
> -           cleanup_domain_irq_pirq(d, irq, info);
> -
> -        rc = irq_deny_access(d, irq);
> -        if ( rc )
> -        {
> -            printk(XENLOG_G_ERR
> -                   "dom%d: could not deny access to IRQ%d (pirq %d)\n",
> -                   d->domain_id, irq, pirq + i);

Looks like the pirq number logged here also was off by one, which
the re-arrangement takes care of.

Jan