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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <cbc0606c-604b-4236-a063-77e081f01250@linaro.org>
Date: Thu, 15 Feb 2024 18:47:20 +0100
From: Konrad Dybcio <konrad.dybcio@...aro.org>
To: Serge Semin <fancer.lancer@...il.com>
Cc: 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 15.02.2024 15:17, Serge Semin wrote:
> On Thu, Feb 15, 2024 at 11:39:31AM +0100, Konrad Dybcio wrote:
>> 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);
>>  	}
>>  
>>  	if (retries >= LINK_WAIT_MAX_RETRIES) {
>> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
>> index 26dae4837462..3f145d6a8a31 100644
>> --- a/drivers/pci/controller/dwc/pcie-designware.h
>> +++ b/drivers/pci/controller/dwc/pcie-designware.h
>> @@ -63,8 +63,7 @@
>>  
>>  /* Parameters for the waiting for link up routine */
>>  #define LINK_WAIT_MAX_RETRIES		10
>> -#define LINK_WAIT_USLEEP_MIN		90000
>> -#define LINK_WAIT_USLEEP_MAX		100000
> 
>> +#define LINK_WAIT_MSLEEP_MAX		100
> 
> Why do you use the _MAX suffix here? AFAICS any the timers normally
> ensures the lower boundary value of the wait-duration, not the upper
> one. So the more correct suffix would be _MIN. On the other hand, as
> Alexander correctly noted, using fsleep() would be more suitable at
> least from the maintainability point of view. Thus having a macro name
> like LINK_WAIT_USLEEP_MIN or just LINK_WAIT_SLEEP_US would be more
> appropriate. The later version is more preferable IMO.

Agree with SLEEP_US

Konrad