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 ...
Jan
