On 19.02.2024 13:47, Stewart Hildebrand wrote:
> On 2/19/24 07:10, Jan Beulich wrote:
>> On 19.02.2024 12:47, Stewart Hildebrand wrote:
>>> @@ -895,6 +891,15 @@ int vpci_msix_arch_print(const struct vpci_msix *msix)
>>> {
>>> unsigned int i;
>>>
>>> + /*
>>> + * Assert that d->pdev_list doesn't change.
>>> ASSERT_PDEV_LIST_IS_READ_LOCKED
>>> + * is not suitable here because it may allow either pcidevs_lock() or
>>> + * d->pci_lock to be held, but here we rely on d->pci_lock being held,
>>> not
>>> + * pcidevs_lock().
>>> + */
>>> + ASSERT(rw_is_locked(&msix->pdev->domain->pci_lock));
>>> + ASSERT(spin_is_locked(&msix->pdev->vpci->lock));
>>
>> There's no "d" in sight here, so it's a little odd that "d" is being talked
>> about. But I guess people can infer what's meant without too much trouble.
>
> I can s/d->pci_lock/msix->pdev->domain->pci_lock/ for the next rev.
Or simply drop the d-s? That would be better for readability's sake,
I think.
>>> @@ -313,17 +316,36 @@ void vpci_dump_msi(void)
>>> {
>>> /*
>>> * On error vpci_msix_arch_print will always return
>>> without
>>> - * holding the lock.
>>> + * holding the locks.
>>> */
>>> printk("unable to print all MSI-X entries: %d\n", rc);
>>> - process_pending_softirqs();
>>> - continue;
>>> + goto pdev_done;
>>> }
>>> }
>>>
>>> + /*
>>> + * Unlock locks to process pending softirqs. This is
>>> + * potentially unsafe, as d->pdev_list can be changed in
>>> + * meantime.
>>> + */
>>> spin_unlock(&pdev->vpci->lock);
>>> + read_unlock(&d->pci_lock);
>>> + pdev_done:
>>> process_pending_softirqs();
>>> + if ( !read_trylock(&d->pci_lock) )
>>> + {
>>> + printk("unable to access other devices for the domain\n");
>>> + goto domain_done;
>>> + }
>>> }
>>> + read_unlock(&d->pci_lock);
>>> + domain_done:
>>> + /*
>>> + * We need this label at the end of the loop, but some
>>> + * compilers might not be happy about label at the end of the
>>> + * compound statement so we adding an empty statement here.
>>> + */
>>> + ;
>>
>> As to "some compilers": Are there any which accept a label not followed
>> by a statement? Depending on the answer, this comment may be viewed as
>> superfluous. Or else I'd ask about wording: Besides a grammar issue I
>> also don't view it as appropriate that a comment talks about "adding"
>> something when its adjacent code that is meant. That something is there
>> when the comment is there, hence respective wording should imo be used.
>
> It seems like hit or miss whether gcc would accept it or not (prior
> discussion at [1]). I agree the comment is rather lengthy for what it's
> trying to convey. I'd be happy to either remove the comment or reduce
> it to:
>
> domain_done:
> ; /* Empty statement to make some compilers happy */
>
> [1]
> https://lore.kernel.org/xen-devel/[email protected]/
This earlier discussion only proves that there is at least one compiler
objecting. There's no proof there that any compiler exists which, as a
language extension, actually permits such syntax. Yet if the comment
was purely about normal language syntax, then imo it should be zapped
altogether, not just be shrunk.
Jan
