Lukas Wunner wrote: > On Fri, Apr 26, 2024 at 12:46:45PM -0700, Dan Williams wrote: > > This also highlights that the pci_dev_lock() performed by > > pci_reset_function() has long been insufficient for the > > pci_reset_bus_function() method case that could race userspace when > > pci_reset_secondary_bus() is manipulating the bridge control register. > > > > So, if the goal of the lock is to prevent userspace from clobbering the > > kernel's read-modify-write cycles of @dev's parent bridge, then the lock > > needs to be held over both the cxl_reset_bus_function() and the > > pci_reset_bus_function() cases, and it needs to be taken in > > upstream-bridge => endpoint order. > > No, the device lock is taken to prevent the driver from unbinding. > It has nothing to do with protecting RMW of parent bridge registers. > > Here's Christoph Hellwig's explanation why he introduced acquisition > of the device lock in the PCI reset code paths: > > https://lore.kernel.org/all/20200325104018.GA30853@xxxxxx/ > > TL;DR: The PCI core calls the driver's ->reset_prepare and ->reset_done > callbacks and the driver needs to be held in place for that. Yes, that's what device_lock() is for, *but* that's not what pci_cfg_access_lock() is for. So when I say that pci_dev_lock() is to protect read-modify-write configuration cycle access of an endpoint to be reset, I am talking about pci_cfg_access_lock() not device_lock(). For better or worse pci_dev_lock() does both, and maybe device_lock() should be open-coded separately. So there is a discrepancy when it comes to protecting manipulation of secondary bus reset control registers of a bridge. The pci_cfg_access_lock() protection is applied to the bridge in the pci_reset_bus() case, but not the pci_reset_bus_function() case.
