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

[Lukas Wunner <lukas@wunner.de>]

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