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

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

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
>
> Jan
>
Thank you,
Oleksandr