Re: [PATCH 4/4] vpci: move lock outside of struct vpci

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

On 02.02.22 13:14, Jan Beulich wrote:
> On 02.02.2022 12:03, Oleksandr Andrushchenko wrote:
>> On 02.02.22 11:22, Jan Beulich wrote:
>>> On 01.02.2022 17:25, Oleksandr Andrushchenko wrote:
>>>> From: Roger Pau Monne <roger.pau@xxxxxxxxxx>
>>>>
>>>> This way the lock can be used to check whether vpci is present, and
>>>> removal can be performed while holding the lock, in order to make
>>>> sure there are no accesses to the contents of the vpci struct.
>>>> Previously removal could race with vpci_read for example, since the
>>>> lock was dropped prior to freeing pdev->vpci.
>>>>
>>>> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>>>> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
>>>> ---
>>>> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>>>> Cc: Jan Beulich <jbeulich@xxxxxxxx>
>>>> Cc: Julien Grall <julien@xxxxxxx>
>>>> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>>>> ---
>>>> New in v5 of this series: this is an updated version of the patch 
>>>> published at
>>>> https://urldefense.com/v3/__https://lore.kernel.org/xen-devel/20180717094830.54806-2-roger.pau@xxxxxxxxxx/__;!!GF_29dbcQIUBPA!jmmcewY6y9Ur4rgvOgqscz8gBWaod2JnQOkHvWtYKgnqeU6BoWJTqCN3UDpCw3io8Ynk-wBXhA$
>>>>  [lore[.]kernel[.]org]
>>>>
>>>> Changes since v5:
>>> This is a little odd in a series implicitly tagged as v1.
>>>
>>>> --- a/xen/drivers/vpci/header.c
>>>> +++ b/xen/drivers/vpci/header.c
>>>> @@ -142,12 +142,13 @@ bool vpci_process_pending(struct vcpu *v)
>>>>            if ( rc == -ERESTART )
>>>>                return true;
>>>>    
>>>> -        spin_lock(&v->vpci.pdev->vpci->lock);
>>>> -        /* Disable memory decoding unconditionally on failure. */
>>>> -        modify_decoding(v->vpci.pdev,
>>>> -                        rc ? v->vpci.cmd & ~PCI_COMMAND_MEMORY : 
>>>> v->vpci.cmd,
>>>> -                        !rc && v->vpci.rom_only);
>>>> -        spin_unlock(&v->vpci.pdev->vpci->lock);
>>>> +        spin_lock(&v->vpci.pdev->vpci_lock);
>>>> +        if ( v->vpci.pdev->vpci )
>>>> +            /* Disable memory decoding unconditionally on failure. */
>>>> +            modify_decoding(v->vpci.pdev,
>>>> +                            rc ? v->vpci.cmd & ~PCI_COMMAND_MEMORY : 
>>>> v->vpci.cmd,
>>>> +                            !rc && v->vpci.rom_only);
>>>> +        spin_unlock(&v->vpci.pdev->vpci_lock);
>>> While I certainly see the point, the addition of this if() (and a
>>> few more elsewhere) isn't covered by title or description.
>> The commit message says:
>> "This way the lock can be used to check whether vpci is present, and
>> removal can be performed while holding the lock, in order to make
>> sure there are no accesses to the contents of the vpci struct."
>> So, I think this is enough to describe the fact that after you have locked
>> the protected structure may have gone already and we need to
>> re-check it is still present.
> I'm afraid that to me "can be used" describes future behavior, as
> opposed to e.g. "is used". If you want to point out both aspects,
> maybe "... can be used (and in a few cases is used right away) ..."?
This sounds good to me, thank you for suggesting that
>
> Jan
>

Thank you,
Oleksandr