linux-kernel - Re: [PATCH] PCI: dwc: Use the correct sleep function in wait_for

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20240220230048.GA1511259@bhelgaas>
Date: Tue, 20 Feb 2024 17:00:48 -0600
From: Bjorn Helgaas <helgaas@...nel.org>
To: Johan Hovold <johan@...nel.org>
Cc: Konrad Dybcio <konrad.dybcio@...aro.org>,
	Alexander Lobakin <aleksander.lobakin@...el.com>,
	Jingoo Han <jingoohan1@...il.com>,
	Gustavo Pimentel <gustavo.pimentel@...opsys.com>,
	Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
	Lorenzo Pieralisi <lpieralisi@...nel.org>,
	Krzysztof Wilczyński <kw@...ux.com>,
	Rob Herring <robh@...nel.org>, Bjorn Helgaas <bhelgaas@...gle.com>,
	Marijn Suijten <marijn.suijten@...ainline.org>,
	linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org,
	Johan Hovold <johan+linaro@...nel.org>
Subject: Re: [PATCH] PCI: dwc: Use the correct sleep function in wait_for_link

On Tue, Feb 20, 2024 at 09:23:24AM +0100, Johan Hovold wrote:
> 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@...aro.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@...aro.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.

Why does it look sloppy?  I'd be surprised if using a constant led to
more executable code, given that fsleep() is inline.  I'm all for
having the compiler choose the right thing instead of having to look
up the guidelines myself.

Bjorn