Re: [PATCH net] net: liquidio: lock upstream bridge for function reset

[Runyu Xiao]

Hi,

Thanks for the detailed review.

> This isn't a bug introduced by this patch, but the kernel-doc directly
> above octeon_pci_flr() still says "PCI FLR" while the body calls the
> generic reset helper. Would it make sense to update the doc string?

The documentation issue is valid and predates this patch. However, it is
secondary to the locking problem. Since the current locking approach is
unsafe, I will not update the comment in this patch.

> Can this acquisition order deadlock against the PCI core?

Yes, this concern is valid. octeon_pci_flr() is called from the remove
and probe-failure paths while the endpoint device lock is already held.
Taking the upstream bridge configuration lock from there can conflict
with the bridge-first locking order used by the PCI core.

> Since block_cfg_access is not lockdep-tracked, would this show up only
> as a hung task, with the bridge's configuration access flag left
> blocked?

That is possible. The wait in pci_cfg_access_lock() is not represented
as a normal lockdep dependency, so this ordering problem may appear as a
hung task rather than a lockdep report.

> Before this change the driver took only the endpoint's own
> configuration lock, which the core also acquires after
> device_lock(endpoint), so there was no inversion.

I agree that the new bridge-lock acquisition introduces this additional
inversion. The original missing bridge serialization is a separate
problem, but adding the bridge lock at this point is not a safe fix for
it.

> Would deferring the bridge lock to a path that does not already hold
> the endpoint device lock, or using the core's pci_reset_function()
> style bridge-first sequence, avoid the cycle?

Yes, the reset must be redesigned around the PCI core locking order.
Taking the bridge lock from the current callback is unsafe because the
endpoint device lock is already held. Simply switching to
pci_reset_function() would not solve the problem because that helper
also tries to acquire the endpoint device lock.

There is also a more direct issue with the current approach. If
__pci_reset_function_locked() selects the bus-reset method,
pci_bus_lock() locks the upstream bridge and then tries to lock the
endpoint again. The endpoint lock is already held by the caller in this
path.

The existing user-space model did not model the endpoint device lock
being held before the reset. It therefore does not validate the
submitted patch and should be discarded.

I will withdraw this driver-only approach and will not send a revised
version using the same locking sequence.

Regards,
Runyu Xiao