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....@citrix.com>
>
> Reviewed-by: Jan Beulich <jbeul...@suse.com>
>
> 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.
