Re: [PATCH] vpci: introduce per-domain lock to protect vpci structure

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.02.2022 15:26, Oleksandr Andrushchenko wrote:
> 
> 
> On 14.02.22 16:19, Jan Beulich wrote:
>> On 09.02.2022 14:36, Oleksandr Andrushchenko wrote:
>>> @@ -410,14 +428,37 @@ static void vpci_write_helper(const struct pci_dev 
>>> *pdev,
>>>                r->private);
>>>   }
>>>   
>>> +static bool vpci_header_write_lock(const struct pci_dev *pdev,
>>> +                                   unsigned int start, unsigned int size)
>>> +{
>>> +    /*
>>> +     * Writing the command register and ROM BAR register may trigger
>>> +     * modify_bars to run which in turn may access multiple pdevs while
>>> +     * checking for the existing BAR's overlap. The overlapping check, if 
>>> done
>>> +     * under the read lock, requires vpci->lock to be acquired on both 
>>> devices
>>> +     * being compared, which may produce a deadlock. It is not possible to
>>> +     * upgrade read lock to write lock in such a case. So, in order to 
>>> prevent
>>> +     * the deadlock, check which registers are going to be written and 
>>> acquire
>>> +     * the lock in the appropriate mode from the beginning.
>>> +     */
>>> +    if ( !vpci_offset_cmp(start, size, PCI_COMMAND, 2) )
>>> +        return true;
>>> +
>>> +    if ( !vpci_offset_cmp(start, size, pdev->vpci->header.rom_reg, 4) )
>>> +        return true;
>>> +
>>> +    return false;
>>> +}
>> A function of this name gives (especially at the call site(s)) the
>> impression of acquiring a lock. Considering that of the prefixes
>> neither "vpci" nor "header" are really relevant here, may I suggest
>> to use need_write_lock()?
>>
>> May I further suggest that you either split the comment or combine
>> the two if()-s (perhaps even straight into single return statement)?
>> Personally I'd prefer the single return statement approach here ...
> That was already questioned by Roger and now it looks like:
> 
> static bool overlap(unsigned int r1_offset, unsigned int r1_size,
>                      unsigned int r2_offset, unsigned int r2_size)
> {
>      /* Return true if there is an overlap. */
>      return r1_offset < r2_offset + r2_size && r2_offset < r1_offset + 
> r1_size;
> }
> 
> bool vpci_header_write_lock(const struct pci_dev *pdev,
>                              unsigned int start, unsigned int size)
> {
>      /*
>       * Writing the command register and ROM BAR register may trigger
>       * modify_bars to run which in turn may access multiple pdevs while
>       * checking for the existing BAR's overlap. The overlapping check, if 
> done
>       * under the read lock, requires vpci->lock to be acquired on both 
> devices
>       * being compared, which may produce a deadlock. It is not possible to
>       * upgrade read lock to write lock in such a case. So, in order to 
> prevent
>       * the deadlock, check which registers are going to be written and 
> acquire
>       * the lock in the appropriate mode from the beginning.
>       */
>      if ( overlap(start, size, PCI_COMMAND, 2) ||
>           (pdev->vpci->header.rom_reg &&
>            overlap(start, size, pdev->vpci->header.rom_reg, 4)) )
>          return true;
> 
>      return false;
> }
> 
> vpci_header_write_lock moved to header.c and is not static anymore.
> So, sitting in header.c, the name seems to be appropriate now

The prefix of the name - yes. But as said, a function of this name looks
as if it would acquire a lock. Imo you want to insert "need" or some
such.

Jan