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
