Re: [PATCH v5 03/14] vpci: move lock outside of struct vpci

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

Hello, Roger, Jan!

On 12.01.22 16:42, Jan Beulich wrote:
> On 11.01.2022 16:17, Roger Pau Monné wrote:
>> On Thu, Nov 25, 2021 at 01:02:40PM +0200, Oleksandr Andrushchenko wrote:
>>> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
>>> index 657697fe3406..ceaac4516ff8 100644
>>> --- a/xen/drivers/vpci/vpci.c
>>> +++ b/xen/drivers/vpci/vpci.c
>>> @@ -35,12 +35,10 @@ extern vpci_register_init_t *const __start_vpci_array[];
>>>   extern vpci_register_init_t *const __end_vpci_array[];
>>>   #define NUM_VPCI_INIT (__end_vpci_array - __start_vpci_array)
>>>   
>>> -void vpci_remove_device(struct pci_dev *pdev)
>>> +static void vpci_remove_device_handlers_locked(struct pci_dev *pdev)
>>>   {
>>> -    if ( !has_vpci(pdev->domain) )
>>> -        return;
>>> +    ASSERT(spin_is_locked(&pdev->vpci_lock));
>>>   
>>> -    spin_lock(&pdev->vpci->lock);
>>>       while ( !list_empty(&pdev->vpci->handlers) )
>>>       {
>>>           struct vpci_register *r = list_first_entry(&pdev->vpci->handlers,
>>> @@ -50,15 +48,33 @@ void vpci_remove_device(struct pci_dev *pdev)
>>>           list_del(&r->node);
>>>           xfree(r);
>>>       }
>>> -    spin_unlock(&pdev->vpci->lock);
>>> +}
>>> +
>>> +void vpci_remove_device_locked(struct pci_dev *pdev)
>> I think this could be static instead, as it's only used by
>> vpci_remove_device and vpci_add_handlers which are local to the
>> file.
This is going to be used outside later on while processing pending mappings,
so I think it is not worth it defining it static here and then removing the 
static
key word later on: please see [PATCH v5 04/14] vpci: cancel pending map/unmap 
on vpci removal [1]
> Does the splitting out of vpci_remove_device_handlers_locked() belong in
> this patch in the first place? There's no second caller being added, so
> this looks to be an orthogonal adjustment.
I think of it as a preparation for the upcoming code: although the reason for 
the
change might not be immediately seen in this patch it is still in line with what
happens next.
So, I would prefer to keep the change as is: anyways the whole series should 
probably
be committed as a single piece of work, so it won't look inconsistent then

Thank you,
Oleksandr
>
> Jan
>

[1] 
https://patchwork.kernel.org/project/xen-devel/patch/20211125110251.2877218-5-andr2000@xxxxxxxxx/