* Re: [PATCH] PCI: dwc: Use the correct sleep function in wait_for_link
2024-02-15 10:39 [PATCH] PCI: dwc: Use the correct sleep function in wait_for_link Konrad Dybcio
@ 2024-02-15 13:35 ` Alexander Lobakin
2024-02-15 17:02 ` Bjorn Helgaas
2024-02-15 14:17 ` Serge Semin
2024-02-15 14:51 ` Kuppuswamy Sathyanarayanan
2 siblings, 1 reply; 11+ messages in thread
From: Alexander Lobakin @ 2024-02-15 13:35 UTC (permalink / raw)
To: Konrad Dybcio
Cc: Jingoo Han, Gustavo Pimentel, Manivannan Sadhasivam,
Lorenzo Pieralisi, Krzysztof Wilczyński, Rob Herring,
Bjorn Helgaas, Marijn Suijten, linux-pci, linux-kernel,
Johan Hovold
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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] PCI: dwc: Use the correct sleep function in wait_for_link
2024-02-15 13:35 ` Alexander Lobakin
@ 2024-02-15 17:02 ` Bjorn Helgaas
2024-02-15 17:46 ` Konrad Dybcio
0 siblings, 1 reply; 11+ messages in thread
From: Bjorn Helgaas @ 2024-02-15 17:02 UTC (permalink / raw)
To: Alexander Lobakin
Cc: Konrad Dybcio, Jingoo Han, Gustavo Pimentel,
Manivannan Sadhasivam, Lorenzo Pieralisi,
Krzysztof Wilczyński, Rob Herring, Bjorn Helgaas,
Marijn Suijten, linux-pci, linux-kernel, Johan Hovold
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] PCI: dwc: Use the correct sleep function in wait_for_link
2024-02-15 17:02 ` Bjorn Helgaas
@ 2024-02-15 17:46 ` Konrad Dybcio
2024-02-20 8:23 ` Johan Hovold
0 siblings, 1 reply; 11+ messages in thread
From: Konrad Dybcio @ 2024-02-15 17:46 UTC (permalink / raw)
To: Bjorn Helgaas, Alexander Lobakin
Cc: Jingoo Han, Gustavo Pimentel, Manivannan Sadhasivam,
Lorenzo Pieralisi, Krzysztof Wilczyński, Rob Herring,
Bjorn Helgaas, Marijn Suijten, linux-pci, linux-kernel,
Johan Hovold
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] PCI: dwc: Use the correct sleep function in wait_for_link
2024-02-15 17:46 ` Konrad Dybcio
@ 2024-02-20 8:23 ` Johan Hovold
2024-02-20 23:00 ` Bjorn Helgaas
0 siblings, 1 reply; 11+ messages in thread
From: Johan Hovold @ 2024-02-20 8:23 UTC (permalink / raw)
To: Konrad Dybcio
Cc: Bjorn Helgaas, Alexander Lobakin, Jingoo Han, Gustavo Pimentel,
Manivannan Sadhasivam, Lorenzo Pieralisi,
Krzysztof Wilczyński, Rob Herring, Bjorn Helgaas,
Marijn Suijten, linux-pci, linux-kernel, Johan Hovold
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] PCI: dwc: Use the correct sleep function in wait_for_link
2024-02-20 8:23 ` Johan Hovold
@ 2024-02-20 23:00 ` Bjorn Helgaas
2024-02-21 12:40 ` Johan Hovold
0 siblings, 1 reply; 11+ messages in thread
From: Bjorn Helgaas @ 2024-02-20 23:00 UTC (permalink / raw)
To: Johan Hovold
Cc: Konrad Dybcio, Alexander Lobakin, Jingoo Han, Gustavo Pimentel,
Manivannan Sadhasivam, Lorenzo Pieralisi,
Krzysztof Wilczyński, Rob Herring, Bjorn Helgaas,
Marijn Suijten, linux-pci, linux-kernel, Johan Hovold
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] PCI: dwc: Use the correct sleep function in wait_for_link
2024-02-20 23:00 ` Bjorn Helgaas
@ 2024-02-21 12:40 ` Johan Hovold
0 siblings, 0 replies; 11+ messages in thread
From: Johan Hovold @ 2024-02-21 12:40 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Konrad Dybcio, Alexander Lobakin, Jingoo Han, Gustavo Pimentel,
Manivannan Sadhasivam, Lorenzo Pieralisi,
Krzysztof Wilczyński, Rob Herring, Bjorn Helgaas,
Marijn Suijten, linux-pci, linux-kernel, Johan Hovold
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] PCI: dwc: Use the correct sleep function in wait_for_link
2024-02-15 10:39 [PATCH] PCI: dwc: Use the correct sleep function in wait_for_link Konrad Dybcio
2024-02-15 13:35 ` Alexander Lobakin
@ 2024-02-15 14:17 ` Serge Semin
2024-02-15 17:47 ` Konrad Dybcio
2024-02-15 14:51 ` Kuppuswamy Sathyanarayanan
2 siblings, 1 reply; 11+ messages in thread
From: Serge Semin @ 2024-02-15 14:17 UTC (permalink / raw)
To: Konrad Dybcio
Cc: Jingoo Han, Gustavo Pimentel, Manivannan Sadhasivam,
Lorenzo Pieralisi, Krzysztof Wilczyński, Rob Herring,
Bjorn Helgaas, Marijn Suijten, linux-pci, linux-kernel,
Johan Hovold
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>
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] PCI: dwc: Use the correct sleep function in wait_for_link
2024-02-15 14:17 ` Serge Semin
@ 2024-02-15 17:47 ` Konrad Dybcio
0 siblings, 0 replies; 11+ messages in thread
From: Konrad Dybcio @ 2024-02-15 17:47 UTC (permalink / raw)
To: Serge Semin
Cc: Jingoo Han, Gustavo Pimentel, Manivannan Sadhasivam,
Lorenzo Pieralisi, Krzysztof Wilczyński, Rob Herring,
Bjorn Helgaas, Marijn Suijten, linux-pci, linux-kernel,
Johan Hovold
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] PCI: dwc: Use the correct sleep function in wait_for_link
2024-02-15 10:39 [PATCH] PCI: dwc: Use the correct sleep function in wait_for_link Konrad Dybcio
2024-02-15 13:35 ` Alexander Lobakin
2024-02-15 14:17 ` Serge Semin
@ 2024-02-15 14:51 ` Kuppuswamy Sathyanarayanan
2024-02-15 17:48 ` Konrad Dybcio
2 siblings, 1 reply; 11+ messages in thread
From: Kuppuswamy Sathyanarayanan @ 2024-02-15 14:51 UTC (permalink / raw)
To: Konrad Dybcio, Jingoo Han, Gustavo Pimentel,
Manivannan Sadhasivam, Lorenzo Pieralisi,
Krzysztof Wilczyński, Rob Herring, Bjorn Helgaas
Cc: Marijn Suijten, linux-pci, linux-kernel, Johan Hovold
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,
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] PCI: dwc: Use the correct sleep function in wait_for_link
2024-02-15 14:51 ` Kuppuswamy Sathyanarayanan
@ 2024-02-15 17:48 ` Konrad Dybcio
0 siblings, 0 replies; 11+ messages in thread
From: Konrad Dybcio @ 2024-02-15 17:48 UTC (permalink / raw)
To: Kuppuswamy Sathyanarayanan, Jingoo Han, Gustavo Pimentel,
Manivannan Sadhasivam, Lorenzo Pieralisi,
Krzysztof Wilczyński, Rob Herring, Bjorn Helgaas
Cc: Marijn Suijten, linux-pci, linux-kernel, Johan Hovold
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
^ permalink raw reply [flat|nested] 11+ messages in thread