[+cc Dan]
On Wed, Jul 03, 2024 at 05:42:05PM +0300, Ido Schimmel wrote:
> On Tue, Jul 02, 2024 at 09:35:50AM +0200, Przemek Kitszel wrote:
> > On 7/1/24 18:41, Petr Machata wrote:
> > > From: Ido Schimmel <idosch@xxxxxxxxxx>
> > >
> > > The driver triggers a "Secondary Bus Reset" (SBR) by calling
> > > __pci_reset_function_locked() which asserts the SBR bit in the "Bridge
> > > Control Register" in the configuration space of the upstream bridge for
> > > 2ms. This is done without locking the configuration space of the
> > > upstream bridge port, allowing user space to access it concurrently.
> >
> > This means your patch is a bugfix.
> >
> > > Linux 6.11 will start warning about such unlocked resets [1][2]:
> > >
> > > pcieport 0000:00:01.0: unlocked secondary bus reset via: pci_reset_bus_function+0x51c/0x6a0
> > >
> > > Avoid the warning by locking the configuration space of the upstream
> > > bridge prior to the reset and unlocking it afterwards.
> >
> > You are not avoiding the warning but protecting concurrent access,
> > please add a Fixes tag.
>
> The patch that added the missing lock in PCI core was posted without a
> Fixes tag and merged as part of the 6.10 PR. See commit 7e89efc6e9e4
> ("PCI: Lock upstream bridge for pci_reset_function()").
>
> I don't see a good reason for root to poke in the configuration space of
> the upstream bridge during SBR, but AFAICT the worst that can happen is
> that reset will fail and while it is a bug, it is not a regression.
>
> Bjorn, do you see a reason to post this as a fix?
Sorry, I was on vacation and missed this when I returned.
mlxsw is one of the few users of __pci_reset_function_locked().
Others are liquidio (octeon), VFIO, and Xen.
You need __pci_reset_function_locked() if you're already holding the
device mutex, i.e., device_lock(&pdev->dev). I looked at the
mlxsw_pci_reset_at_pci_disable() path, and didn't see where it holds
that device lock, but I probably missed it.
The usual pci_reset_function() path, which would be preferable if you
can use it, does basically this:
pci_dev_lock(bridge)
device_lock(&bridge->dev)
pci_cfg_access_lock(bridge)
pci_dev_lock(pdev)
device_lock(&pdev->dev)
pci_cfg_access_lock(pdev)
pci_dev_save_and_disable(dev)
__pci_reset_function_locked(pdev)
This patch adds pci_cfg_access_lock(bridge), but doesn't acquire the
device_lock for the bridge.
It looks like you always reset the device at mlxsw_pci_probe()-time,
which is quite unusual in the first place, but I suppose there's some
good reason for it.
If you can use pci_reset_function() directly (or avoid the reset
altogether), it would be far preferable and would avoid potential
issues like the warning here.
Bjorn
> > > [1] https://lore.kernel.org/all/171711746953.1628941.4692125082286867825.stgit@xxxxxxxxxxxxxxxxxxxxxxxxx/
> > > [2] https://lore.kernel.org/all/20240531213150.GA610983@bhelgaas/
> > >
> > > Cc: linux-pci@xxxxxxxxxxxxxxx
> > > Signed-off-by: Ido Schimmel <idosch@xxxxxxxxxx>
> > > Signed-off-by: Petr Machata <petrm@xxxxxxxxxx>
