Re: [PATCH v3] PCI: Fix no-op wait after secondary bus reset

From: Lukas Wunner <lukas@wunner.de>
To: Sheng Bi <windy.bi.enflame@gmail.com>
Cc: helgaas@kernel.org, bhelgaas@google.com,
	alex.williamson@redhat.com, linux-pci@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3] PCI: Fix no-op wait after secondary bus reset
Date: Wed, 8 Jun 2022 17:23:44 +0200	[thread overview]
Message-ID: <20220608152344.GA17064@wunner.de> (raw)
In-Reply-To: <20220523171517.32407-1-windy.bi.enflame@gmail.com>

On Tue, May 24, 2022 at 01:15:17AM +0800, Sheng Bi wrote:
> pci_bridge_secondary_bus_reset() triggers SBR followed by 1 second sleep,
> and then uses pci_dev_wait() for waiting device ready. The dev parameter
> passes to the wait function is currently the bridge itself, but not the
> device been reset.
> 
> If we call pci_bridge_secondary_bus_reset() to trigger SBR to a device,
> there is 1 second sleep but not waiting device ready, since the bridge
> is always ready while resetting downstream devices. pci_dev_wait() here
> is a no-op actually. This would be risky in the case which the device
> becomes ready after more than 1 second, especially while hotplug enabled.
> The late coming hotplug event after 1 second will trigger hotplug module
> to remove/re-insert the device.
> 
> Instead of waiting ready of bridge itself, changing to wait all the
> downstream devices become ready with timeout PCIE_RESET_READY_POLL_MS
> after SBR, considering all downstream devices are affected during SBR.
> Once one of the devices doesn't reappear within the timeout, return
> -ENOTTY to indicate SBR doesn't complete successfully.
> 
> Fixes: 6b2f1351af56 ("PCI: Wait for device to become ready after secondary bus reset")
> Signed-off-by: Sheng Bi <windy.bi.enflame@gmail.com>

Reviewed-by: Lukas Wunner <lukas@wunner.de>
Cc: stable@vger.kernel.org # v4.17+

Code-wise, this LGTM.  There are a few things that could be
improved in the commit message, e.g. in the last paragraph,
"changing" (gerund form) is not proper English and the
imperative form "change" would be correct here.  However,
these details are difficult to get right for anyone who is
not an English native speaker and often Bjorn will wordsmith
the commit message to perfect it.

See here for some of the things Bjorn looks for:
https://lore.kernel.org/linux-pci/20171026223701.GA25649@bhelgaas-glaptop.roam.corp.google.com/

Bjorn may not find the time to look over your patch immediately,
so please be patient.

Thanks!

Lukas