On 20.09.2023 15:56, Stewart Hildebrand wrote:
> 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.
Right, such a label would be bogus even without that; instead of "goto"
the use site then simply should use "continue".
Jan
