On 9/20/23 04:09, Roger Pau Monné wrote: > On Tue, Sep 19, 2023 at 12:20:39PM -0400, Stewart Hildebrand wrote: >> On 9/19/23 11:39, Roger Pau Monné wrote: >>> On Tue, Aug 29, 2023 at 11:19:42PM +0000, Volodymyr Babchuk wrote: >>>> diff --git a/xen/drivers/vpci/msi.c b/xen/drivers/vpci/msi.c >>>> index 8f2b59e61a..a0733bb2cb 100644 >>>> --- a/xen/drivers/vpci/msi.c >>>> +++ b/xen/drivers/vpci/msi.c >>>> @@ -318,15 +321,28 @@ void vpci_dump_msi(void) >>>> * holding the lock. >>>> */ >>>> printk("unable to print all MSI-X entries: %d\n", rc); >>>> - process_pending_softirqs(); >>>> - continue; >>>> + goto pdev_done; >>>> } >>>> } >>>> >>>> spin_unlock(&pdev->vpci->lock); >>>> + pdev_done: >>>> + /* >>>> + * Unlock lock to process pending softirqs. This is >>>> + * potentially unsafe, as d->pdev_list can be changed in >>>> + * meantime. >>>> + */ >>>> + read_unlock(&d->pci_lock); >>>> process_pending_softirqs(); >>>> + if ( !read_trylock(&d->pci_lock) ) >>>> + { >>>> + printk("unable to access other devices for the domain\n"); >>>> + goto domain_done; >>> >>> Shouldn't the domain_done label be after the read_unlock(), so that we >>> can proceed to try to dump the devices for the next domain? With the >>> proposed code a failure to acquire one of the domains pci_lock >>> terminates the dump. >>> >>>> + } >>>> } >>>> + read_unlock(&d->pci_lock); >>>> } >>>> + domain_done: >>>> rcu_read_unlock(&domlist_read_lock); >>>> } >>>> >> >> With the label moved, a no-op expression after the label is needed to make >> the compiler happy: >> >> } >> } >> read_unlock(&d->pci_lock); >> domain_done: >> (void)0; >> } >> rcu_read_unlock(&domlist_read_lock); >> } >> >> >> If the no-op is omitted, the compiler may complain (gcc 9.4.0): >> >> drivers/vpci/msi.c: In function ‘vpci_dump_msi’: >> drivers/vpci/msi.c:351:2: error: label at end of compound statement >> 351 | domain_done: >> | ^~~~~~~~~~~ > > > Might be better to place the label at the start of the loop, and > likely rename to next_domain.
That would bypass the loop condition and increment statements.