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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, Feb 19, 2021 at 03:39:14PM +0100, Jan Beulich wrote:
> 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).

Sure, if you want to queue this already please fix the format
string.

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

Indeed. I don't think it's worth fixing this now.

Thanks, Roger.