On 14.02.22 16:31, Jan Beulich wrote:
> 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.
Agree. Then vpci_header_need_write_lock.
I will also update the comment because it makes an impression that
the function acquires the lock
>
> Jan
>
Thank you,
Oleksandr
