netdev - Re: [PATCH net-next 6/6] mlxsw: pci: Add support for new reset flow

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230413102636.GA18500@wunner.de>
Date:   Thu, 13 Apr 2023 12:26:36 +0200
From:   Lukas Wunner <lukas@...ner.de>
To:     Bjorn Helgaas <helgaas@...nel.org>
Cc:     Ido Schimmel <idosch@...dia.com>, Petr Machata <petrm@...dia.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org,
        Amit Cohen <amcohen@...dia.com>, mlxsw@...dia.com,
        linux-pci@...r.kernel.org,
        Alex Williamson <alex.williamson@...hat.com>
Subject: Re: [PATCH net-next 6/6] mlxsw: pci: Add support for new reset flow

On Wed, Mar 29, 2023 at 11:01:44AM -0500, Bjorn Helgaas wrote:
> On Sun, Mar 26, 2023 at 04:53:58PM +0300, Ido Schimmel wrote:
> > On Thu, Mar 23, 2023 at 11:51:15AM -0500, Bjorn Helgaas wrote:
> > > So the first question is why you don't simply use
> > > pci_reset_function(), since it is supposed to cause a reset and do all
> > > the necessary waiting for the device to be ready.

I agree, this driver should use one of the reset helpers provided
by the PCI core.

Not least because if the Downstream Port is hotplug-capable,
fiddling with the Link Disable bit behind the hotplug driver's back
will cause unbinding and rebinding of the device.

> > __pci_reset_function_locked() is another option, but it assumes the
> > device lock was already taken, which is correct during probe, but not
> > when reset is performed as part of devlink reload.

Just call pci_dev_lock() in the devlink reload code path.

> Hmmm, pci_pm_reset() puts the device in D3hot, then back to D0.  Spec
> says that results in "undefined internal Function state," which
> doesn't even sound like a guaranteed reset, but it's what we have, and
> in any case, it does not disable the link.

Per PCIe r6.0.1 sec 5.8, the device undergoes a Soft Reset when moving
from D3hot to D0 (unless the No_Soft_Reset bit is set in the Power
Management Control/Status Register, sec 7.5.2.2).

The driver can set the PCI_DEV_FLAGS_NO_PM_RESET flag if this reset method
doesn't have any effect (via quirk_no_pm_reset()).

We could also discuss moving pci_pm_reset after pci_reset_bus_function
in pci_reset_fn_methods[] if this reset method has little value on the
majority of devices.

Alternatively, if Secondary Bus Reset has the intended effect, the driver
could invoke that directly via pci_reset_bus().

> Spec (PCIe r6.0, sec 6.6.1) says "Disabling a Link causes Downstream
> components to undergo a hot reset."  That seems like it *could* be a
> general-purpose method of doing a reset, and I don't know why the PCI
> core doesn't support it.  Added Alex and Lukas in case they know.

Sounds reasonable to me.

Thanks,

Lukas