diff mbox series

PCI: dwc: Use the correct sleep function in wait_for_link

Message ID	20240215-topic-pci_sleep-v1-1-7ac79ac9739a@linaro.org
State	New
Headers	Received-SPF: pass (google.com: domain of linux-kernel+bounces-66682-ouuuleilei=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161; From: Konrad Dybcio <konrad.dybcio@linaro.org> Date: Thu, 15 Feb 2024 11:39:31 +0100 Subject: [PATCH] PCI: dwc: Use the correct sleep function in wait_for_link Precedence: bulk MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Message-Id: <20240215-topic-pci_sleep-v1-1-7ac79ac9739a@linaro.org> To: Jingoo Han <jingoohan1@gmail.com>, Gustavo Pimentel <gustavo.pimentel@synopsys.com>, Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>, Lorenzo Pieralisi <lpieralisi@kernel.org>, =?utf-8?q?Krzysztof_Wilczy=C5=84?= =?utf-8?q?ski?= <kw@linux.com>, Rob Herring <robh@kernel.org>, Bjorn Helgaas <bhelgaas@google.com> Cc: Marijn Suijten <marijn.suijten@somainline.org>, linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org, Johan Hovold <johan+linaro@kernel.org>, Konrad Dybcio <konrad.dybcio@linaro.org>
Series	PCI: dwc: Use the correct sleep function in wait_for_link \| PCI: dwc: Use the correct sleep function in wait_for_link

Commit Message

Konrad Dybcio Feb. 15, 2024, 10:39 a.m. UTC

  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(-)


---
base-commit: 26d7d52b6253574d5b6fec16a93e1110d1489cef
change-id: 20240215-topic-pci_sleep-368108a1fb6f

Best regards,

Comments

Alexander Lobakin Feb. 15, 2024, 1:35 p.m. UTC | #1

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.

>  	}
>  
>  	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
>  
>  /* Parameters for the waiting for iATU enabled routine */
>  #define LINK_WAIT_MAX_IATU_RETRIES	5
> 
> ---
> base-commit: 26d7d52b6253574d5b6fec16a93e1110d1489cef
> change-id: 20240215-topic-pci_sleep-368108a1fb6f
> 
> Best regards,

Thanks,
Olek

Serge Semin Feb. 15, 2024, 2:17 p.m. UTC | #2

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@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);
>  	}
>  
>  	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.

-Serge(y)

>  
>  /* Parameters for the waiting for iATU enabled routine */
>  #define LINK_WAIT_MAX_IATU_RETRIES	5
> 
> ---
> base-commit: 26d7d52b6253574d5b6fec16a93e1110d1489cef
> change-id: 20240215-topic-pci_sleep-368108a1fb6f
> 
> Best regards,
> -- 
> Konrad Dybcio <konrad.dybcio@linaro.org>
> 
>

Kuppuswamy Sathyanarayanan Feb. 15, 2024, 2:51 p.m. UTC | #3

On 2/15/24 2:39 AM, 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@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);
>  	}
>  
>  	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

Since 90 ms is an acceptable value, why not use it?

>  
>  /* Parameters for the waiting for iATU enabled routine */
>  #define LINK_WAIT_MAX_IATU_RETRIES	5
>
> ---
> base-commit: 26d7d52b6253574d5b6fec16a93e1110d1489cef
> change-id: 20240215-topic-pci_sleep-368108a1fb6f
>
> Best regards,

Bjorn Helgaas Feb. 15, 2024, 5:02 p.m. UTC | #4

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.

Odd.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/timers/timers-howto.rst?id=v6.7#n114
mentions fsleep() (with no real guidance about when to use it), but
https://www.kernel.org/doc/Documentation/timers/timers-howto.txt 
seems to be a stale copy from 2017, before fsleep() was added.  I
emailed helpdesk@kernel.org to see if the stale content can be
removed.

I do think fsleep() should be more widely used.

> >  /* 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

Since you're touching this anyway, it would be helpful to include the
units on the timeout.

USLEEP/MSLEEP is definitely a hint, but I think the "SLEEP" part
suggests something about atomic/non-atomic context and isn't relevant
to the time interval itself, and something like "TIMEOUT" would be
better.

I think an explicit "_US" or "_MS" would better indicate the units.

This is turning into a long tangent, but I'm not a huge fan of the
LINK_WAIT_* pattern where I have to look up the code that uses
LINK_WAIT_MAX_RETRIES and LINK_WAIT_USLEEP_MAX and do the math to see
what the actual timeout is.  Obviously not fodder for *this* patch.

Bjorn

Konrad Dybcio Feb. 15, 2024, 5:46 p.m. UTC | #5

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

> 
> Odd.
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/timers/timers-howto.rst?id=v6.7#n114
> mentions fsleep() (with no real guidance about when to use it), but
> https://www.kernel.org/doc/Documentation/timers/timers-howto.txt 
> seems to be a stale copy from 2017, before fsleep() was added.  I
> emailed helpdesk@kernel.org to see if the stale content can be
> removed.
> 
> I do think fsleep() should be more widely used.
> 
>>>  /* 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
> 
> Since you're touching this anyway, it would be helpful to include the
> units on the timeout.
> 
> USLEEP/MSLEEP is definitely a hint, but I think the "SLEEP" part
> suggests something about atomic/non-atomic context and isn't relevant
> to the time interval itself, and something like "TIMEOUT" would be
> better.
> 
> I think an explicit "_US" or "_MS" would better indicate the units.
> 
> This is turning into a long tangent, but I'm not a huge fan of the
> LINK_WAIT_* pattern where I have to look up the code that uses
> LINK_WAIT_MAX_RETRIES and LINK_WAIT_USLEEP_MAX and do the math to see
> what the actual timeout is.  Obviously not fodder for *this* patch.

Might as well do that indeed

Konrad

Konrad Dybcio Feb. 15, 2024, 5:47 p.m. UTC | #6

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@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);
>>  	}
>>  
>>  	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

Konrad Dybcio Feb. 15, 2024, 5:48 p.m. UTC | #7

On 15.02.2024 15:51, Kuppuswamy Sathyanarayanan wrote:
> 
> On 2/15/24 2:39 AM, 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@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);
>>  	}
>>  
>>  	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
> 
> Since 90 ms is an acceptable value, why not use it?

I suppose I can do that indeed.. Usually I go for the safer option
when cleaning up old code, but you're right, 90 should be ok

(unless somebody has some documentation stating otherwise)

Konrad

Johan Hovold Feb. 20, 2024, 8:23 a.m. UTC | #8

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

Bjorn Helgaas Feb. 20, 2024, 11 p.m. UTC | #9

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@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.

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

Johan Hovold Feb. 21, 2024, 12:40 p.m. UTC | #10

On Tue, Feb 20, 2024 at 05:00:48PM -0600, Bjorn Helgaas wrote:
> 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:

> > > >> 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.

It's not about the generated code, but about hiding what's really going
from kernel developers that should be aware of such things. The fact
that you end up with an usleep range of 20 to 40 ms if you want to sleep
for 20 ms is not very nice either.

Except possibly for a few cases where you'd otherwise end up open-coding
fsleep() I don't think there's any reason to use it.

Johan

diff mbox series

Patch

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
 
 /* Parameters for the waiting for iATU enabled routine */
 #define LINK_WAIT_MAX_IATU_RETRIES	5