| Message ID | 20240215-topic-pci_sleep-v1-1-7ac79ac9739a@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: dwc: Use the correct sleep function in wait_for_link | expand |
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
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> > >
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,
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
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
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
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
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
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
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 --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
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,