Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Fri, 20 May 2022 08:41:48 +0200
From:   Lukas Wunner <lukas@wunner.de>
To:     Sheng Bi <windy.bi.enflame@gmail.com>
Cc:     helgaas@kernel.org, alex.williamson@redhat.com,
        linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] PCI: Fix no-op wait after secondary bus reset
Message-ID: <20220520064148.GA20418@wunner.de>
References: <20220516165740.6256af51.alex.williamson@redhat.com>
 <20220518115432.76183-1-windy.bi.enflame@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220518115432.76183-1-windy.bi.enflame@gmail.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk

On Wed, May 18, 2022 at 07:54:32PM +0800, Sheng Bi wrote:
> +static int pci_bridge_secondary_bus_wait(struct pci_dev *bridge, int timeout)
> +{
> +	struct pci_dev *dev;
> +	int delay = 0;
> +
> +	if (!bridge->subordinate || list_empty(&bridge->subordinate->devices))
> +		return 0;
> +
> +	list_for_each_entry(dev, &bridge->subordinate->devices, bus_list) {
> +		while (!pci_device_is_present(dev)) {
> +			if (delay > timeout) {
> +				pci_warn(dev, "not ready %dms after secondary bus reset; giving up\n",
> +					delay);
> +				return -ENOTTY;
> +			}
> +
> +			msleep(20);
> +			delay += 20;
> +		}
> +
> +		if (delay > 1000)
> +			pci_info(dev, "ready %dms after secondary bus reset\n",
> +				delay);
> +	}
> +
> +	return 0;
> +}

An alternative approach you may want to consider is to call
pci_dev_wait() in the list_for_each_entry loop, but instead of
passing it a constant timeout you'd pass the remaining time.

Get the current time before and after each pci_dev_wait() call
from "jiffies", calculate the difference, convert to msecs with
jiffies_to_msecs() and subtract from the "timeout" parameter
passed in by the caller, then simply pass "timeout" to each
pci_dev_wait() call.

As a side note, traversing the bus list normally requires
holding the pci_bus_sem for reading.  But it's probably unlikely
that devices are added/removed concurrently to a bus reset
and we're doing it wrong pretty much everywhere in the
PCI reset code, so...

(I fixed up one of the reset functions with 10791141a6cf,
but plenty of others remain...)

Thanks,

Lukas