Received-SPF: pass (google.com: domain of linux-kernel+bounces-72540-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Tue, 20 Feb 2024 09:23:24 +0100
From: Johan Hovold <johan@kernel.org>
To: Konrad Dybcio <konrad.dybcio@linaro.org>
Cc: Bjorn Helgaas <helgaas@kernel.org>,
	Alexander Lobakin <aleksander.lobakin@intel.com>,
	Jingoo Han <jingoohan1@gmail.com>,
	Gustavo Pimentel <gustavo.pimentel@synopsys.com>,
	Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>,
	Lorenzo Pieralisi <lpieralisi@kernel.org>,
	Krzysztof =?utf-8?Q?Wilczy=C5=84ski?= <kw@linux.com>,
	Rob Herring <robh@kernel.org>, Bjorn Helgaas <bhelgaas@google.com>,
	Marijn Suijten <marijn.suijten@somainline.org>,
	linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org,
	Johan Hovold <johan+linaro@kernel.org>
Subject: Re: [PATCH] PCI: dwc: Use the correct sleep function in wait_for_link
Message-ID: <ZdRhfCXejhrKQLPC@hovoldconsulting.com>
References: <20240215170258.GA1292702@bhelgaas>
 <a12217ec-ce63-4312-a412-9a5c8d1d92fe@linaro.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <a12217ec-ce63-4312-a412-9a5c8d1d92fe@linaro.org>

On Thu, Feb 15, 2024 at 06:46:55PM +0100, Konrad Dybcio wrote:
> On 15.02.2024 18:02, Bjorn Helgaas wrote:
> > On Thu, Feb 15, 2024 at 02:35:13PM +0100, Alexander Lobakin wrote:
> >> From: Konrad Dybcio <konrad.dybcio@linaro.org>
> >> Date: Thu, 15 Feb 2024 11:39:31 +0100
> >>
> >>> According to [1], msleep should be used for large sleeps, such as the
> >>> 100-ish ms one in this function. Comply with the guide and use it.
> >>>
> >>> [1] https://www.kernel.org/doc/Documentation/timers/timers-howto.txt
> >>>
> >>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> >>> ---
> >>> Tested on Qualcomm SC8280XP CRD
> >>> ---
> >>>  drivers/pci/controller/dwc/pcie-designware.c | 2 +-
> >>>  drivers/pci/controller/dwc/pcie-designware.h | 3 +--
> >>>  2 files changed, 2 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> >>> index 250cf7f40b85..abce6afceb91 100644
> >>> --- a/drivers/pci/controller/dwc/pcie-designware.c
> >>> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> >>> @@ -655,7 +655,7 @@ int dw_pcie_wait_for_link(struct dw_pcie *pci)
> >>>  		if (dw_pcie_link_up(pci))
> >>>  			break;
> >>>  
> >>> -		usleep_range(LINK_WAIT_USLEEP_MIN, LINK_WAIT_USLEEP_MAX);
> >>> +		msleep(LINK_WAIT_MSLEEP_MAX);
> >>
> >> Just use fsleep(LINK_WAIT_USLEEP_MAX) and let the kernel decide which
> >> function to pick.
> 
> IMO, fsleep only makes sense when the argument is variable.. This way, we
> can save on bothering the compiler or adding an unnecessary branch

I fully agree. Using fsleep() with a constant just looks sloppy (e.g.
with that hardcoded usleep range) and hides what is really going on for
no good reason.

Johan