Replaced msleep for less than 10ms with usleep_range because will
often sleep longer than intended.
For original explanation see:
Documentation/timers/timers-howto.txt
Signed-off-by: Daniel Klamt <[email protected]>
Signed-off-by: Bjoern Noetel <[email protected]>
---
arch/powerpc/sysdev/xive/native.c | 24 ++++++++++++------------
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/arch/powerpc/sysdev/xive/native.c b/arch/powerpc/sysdev/xive/native.c
index 311185b9960a..b164b1cdf4d6 100644
--- a/arch/powerpc/sysdev/xive/native.c
+++ b/arch/powerpc/sysdev/xive/native.c
@@ -109,7 +109,7 @@ int xive_native_configure_irq(u32 hw_irq, u32 target, u8 prio, u32 sw_irq)
rc = opal_xive_set_irq_config(hw_irq, target, prio, sw_irq);
if (rc != OPAL_BUSY)
break;
- msleep(1);
+ usleep_range(1000, 1100)
}
return rc == 0 ? 0 : -ENXIO;
}
@@ -163,7 +163,7 @@ int xive_native_configure_queue(u32 vp_id, struct xive_q *q, u8 prio,
rc = opal_xive_set_queue_info(vp_id, prio, qpage_phys, order, flags);
if (rc != OPAL_BUSY)
break;
- msleep(1);
+ usleep_range(1000, 1100);
}
if (rc) {
pr_err("Error %lld setting queue for prio %d\n", rc, prio);
@@ -190,7 +190,7 @@ static void __xive_native_disable_queue(u32 vp_id, struct xive_q *q, u8 prio)
rc = opal_xive_set_queue_info(vp_id, prio, 0, 0, 0);
if (rc != OPAL_BUSY)
break;
- msleep(1);
+ usleep_range(1000, 1100);
}
if (rc)
pr_err("Error %lld disabling queue for prio %d\n", rc, prio);
@@ -253,7 +253,7 @@ static int xive_native_get_ipi(unsigned int cpu, struct xive_cpu *xc)
for (;;) {
irq = opal_xive_allocate_irq(chip_id);
if (irq == OPAL_BUSY) {
- msleep(1);
+ usleep_range(1000, 1100);
continue;
}
if (irq < 0) {
@@ -275,7 +275,7 @@ u32 xive_native_alloc_irq(void)
rc = opal_xive_allocate_irq(OPAL_XIVE_ANY_CHIP);
if (rc != OPAL_BUSY)
break;
- msleep(1);
+ usleep_range(1000, 1100);
}
if (rc < 0)
return 0;
@@ -289,7 +289,7 @@ void xive_native_free_irq(u32 irq)
s64 rc = opal_xive_free_irq(irq);
if (rc != OPAL_BUSY)
break;
- msleep(1);
+ usleep_range(1000, 1100);
}
}
EXPORT_SYMBOL_GPL(xive_native_free_irq);
@@ -305,7 +305,7 @@ static void xive_native_put_ipi(unsigned int cpu, struct xive_cpu *xc)
for (;;) {
rc = opal_xive_free_irq(xc->hw_ipi);
if (rc == OPAL_BUSY) {
- msleep(1);
+ usleep_range(1000, 1100);
continue;
}
xc->hw_ipi = 0;
@@ -400,7 +400,7 @@ static void xive_native_setup_cpu(unsigned int cpu, struct xive_cpu *xc)
rc = opal_xive_set_vp_info(vp, OPAL_XIVE_VP_ENABLED, 0);
if (rc != OPAL_BUSY)
break;
- msleep(1);
+ usleep_range(1000, 1100);
}
if (rc) {
pr_err("Failed to enable pool VP on CPU %d\n", cpu);
@@ -444,7 +444,7 @@ static void xive_native_teardown_cpu(unsigned int cpu, struct xive_cpu *xc)
rc = opal_xive_set_vp_info(vp, 0, 0);
if (rc != OPAL_BUSY)
break;
- msleep(1);
+ usleep_range(1000, 1100);
}
}
@@ -645,7 +645,7 @@ u32 xive_native_alloc_vp_block(u32 max_vcpus)
rc = opal_xive_alloc_vp_block(order);
switch (rc) {
case OPAL_BUSY:
- msleep(1);
+ usleep_range(1000, 1100);
break;
case OPAL_XIVE_PROVISIONING:
if (!xive_native_provision_pages())
@@ -687,7 +687,7 @@ int xive_native_enable_vp(u32 vp_id, bool single_escalation)
rc = opal_xive_set_vp_info(vp_id, flags, 0);
if (rc != OPAL_BUSY)
break;
- msleep(1);
+ usleep_range(1000, 1100);
}
return rc ? -EIO : 0;
}
@@ -701,7 +701,7 @@ int xive_native_disable_vp(u32 vp_id)
rc = opal_xive_set_vp_info(vp_id, 0, 0);
if (rc != OPAL_BUSY)
break;
- msleep(1);
+ usleep_range(1000, 1100);
}
return rc ? -EIO : 0;
}
--
2.11.0
Hi Daniel,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on powerpc/next]
[also build test ERROR on v4.18-rc4 next-20180709]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Daniel-Klamt/powerpc-Replaced-msleep-with-usleep_range/20180709-231913
base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: powerpc-defconfig (attached as .config)
compiler: powerpc64-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.2.0 make.cross ARCH=powerpc
All errors (new ones prefixed by >>):
arch/powerpc/sysdev/xive/native.c: In function 'xive_native_configure_irq':
>> arch/powerpc/sysdev/xive/native.c:113:2: error: expected ';' before '}' token
}
^
vim +113 arch/powerpc/sysdev/xive/native.c
243e2511 Benjamin Herrenschmidt 2017-04-05 103
243e2511 Benjamin Herrenschmidt 2017-04-05 104 int xive_native_configure_irq(u32 hw_irq, u32 target, u8 prio, u32 sw_irq)
243e2511 Benjamin Herrenschmidt 2017-04-05 105 {
243e2511 Benjamin Herrenschmidt 2017-04-05 106 s64 rc;
243e2511 Benjamin Herrenschmidt 2017-04-05 107
243e2511 Benjamin Herrenschmidt 2017-04-05 108 for (;;) {
243e2511 Benjamin Herrenschmidt 2017-04-05 109 rc = opal_xive_set_irq_config(hw_irq, target, prio, sw_irq);
243e2511 Benjamin Herrenschmidt 2017-04-05 110 if (rc != OPAL_BUSY)
243e2511 Benjamin Herrenschmidt 2017-04-05 111 break;
c332c793 Daniel Klamt 2018-07-09 112 usleep_range(1000, 1100)
243e2511 Benjamin Herrenschmidt 2017-04-05 @113 }
243e2511 Benjamin Herrenschmidt 2017-04-05 114 return rc == 0 ? 0 : -ENXIO;
243e2511 Benjamin Herrenschmidt 2017-04-05 115 }
5af50993 Benjamin Herrenschmidt 2017-04-05 116 EXPORT_SYMBOL_GPL(xive_native_configure_irq);
5af50993 Benjamin Herrenschmidt 2017-04-05 117
243e2511 Benjamin Herrenschmidt 2017-04-05 118
:::::: The code at line 113 was first introduced by commit
:::::: 243e25112d06b348f087a6f7aba4bbc288285bdd powerpc/xive: Native exploitation of the XIVE interrupt controller
:::::: TO: Benjamin Herrenschmidt <[email protected]>
:::::: CC: Michael Ellerman <[email protected]>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Mon, 2018-07-09 at 15:57 +0200, Daniel Klamt wrote:
> Replaced msleep for less than 10ms with usleep_range because will
> often sleep longer than intended.
> For original explanation see:
> Documentation/timers/timers-howto.txt
Why ? This is pointless. The original code is smaller and more
readable. We don't care how long it actually sleeps, this is the FW
telling us it's busy (or the HW is), come back a bit later.
Ben.
> Signed-off-by: Daniel Klamt <[email protected]>
> Signed-off-by: Bjoern Noetel <[email protected]>
> ---
> arch/powerpc/sysdev/xive/native.c | 24 ++++++++++++------------
> 1 file changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/arch/powerpc/sysdev/xive/native.c b/arch/powerpc/sysdev/xive/native.c
> index 311185b9960a..b164b1cdf4d6 100644
> --- a/arch/powerpc/sysdev/xive/native.c
> +++ b/arch/powerpc/sysdev/xive/native.c
> @@ -109,7 +109,7 @@ int xive_native_configure_irq(u32 hw_irq, u32 target, u8 prio, u32 sw_irq)
> rc = opal_xive_set_irq_config(hw_irq, target, prio, sw_irq);
> if (rc != OPAL_BUSY)
> break;
> - msleep(1);
> + usleep_range(1000, 1100)
> }
> return rc == 0 ? 0 : -ENXIO;
> }
> @@ -163,7 +163,7 @@ int xive_native_configure_queue(u32 vp_id, struct xive_q *q, u8 prio,
> rc = opal_xive_set_queue_info(vp_id, prio, qpage_phys, order, flags);
> if (rc != OPAL_BUSY)
> break;
> - msleep(1);
> + usleep_range(1000, 1100);
> }
> if (rc) {
> pr_err("Error %lld setting queue for prio %d\n", rc, prio);
> @@ -190,7 +190,7 @@ static void __xive_native_disable_queue(u32 vp_id, struct xive_q *q, u8 prio)
> rc = opal_xive_set_queue_info(vp_id, prio, 0, 0, 0);
> if (rc != OPAL_BUSY)
> break;
> - msleep(1);
> + usleep_range(1000, 1100);
> }
> if (rc)
> pr_err("Error %lld disabling queue for prio %d\n", rc, prio);
> @@ -253,7 +253,7 @@ static int xive_native_get_ipi(unsigned int cpu, struct xive_cpu *xc)
> for (;;) {
> irq = opal_xive_allocate_irq(chip_id);
> if (irq == OPAL_BUSY) {
> - msleep(1);
> + usleep_range(1000, 1100);
> continue;
> }
> if (irq < 0) {
> @@ -275,7 +275,7 @@ u32 xive_native_alloc_irq(void)
> rc = opal_xive_allocate_irq(OPAL_XIVE_ANY_CHIP);
> if (rc != OPAL_BUSY)
> break;
> - msleep(1);
> + usleep_range(1000, 1100);
> }
> if (rc < 0)
> return 0;
> @@ -289,7 +289,7 @@ void xive_native_free_irq(u32 irq)
> s64 rc = opal_xive_free_irq(irq);
> if (rc != OPAL_BUSY)
> break;
> - msleep(1);
> + usleep_range(1000, 1100);
> }
> }
> EXPORT_SYMBOL_GPL(xive_native_free_irq);
> @@ -305,7 +305,7 @@ static void xive_native_put_ipi(unsigned int cpu, struct xive_cpu *xc)
> for (;;) {
> rc = opal_xive_free_irq(xc->hw_ipi);
> if (rc == OPAL_BUSY) {
> - msleep(1);
> + usleep_range(1000, 1100);
> continue;
> }
> xc->hw_ipi = 0;
> @@ -400,7 +400,7 @@ static void xive_native_setup_cpu(unsigned int cpu, struct xive_cpu *xc)
> rc = opal_xive_set_vp_info(vp, OPAL_XIVE_VP_ENABLED, 0);
> if (rc != OPAL_BUSY)
> break;
> - msleep(1);
> + usleep_range(1000, 1100);
> }
> if (rc) {
> pr_err("Failed to enable pool VP on CPU %d\n", cpu);
> @@ -444,7 +444,7 @@ static void xive_native_teardown_cpu(unsigned int cpu, struct xive_cpu *xc)
> rc = opal_xive_set_vp_info(vp, 0, 0);
> if (rc != OPAL_BUSY)
> break;
> - msleep(1);
> + usleep_range(1000, 1100);
> }
> }
>
> @@ -645,7 +645,7 @@ u32 xive_native_alloc_vp_block(u32 max_vcpus)
> rc = opal_xive_alloc_vp_block(order);
> switch (rc) {
> case OPAL_BUSY:
> - msleep(1);
> + usleep_range(1000, 1100);
> break;
> case OPAL_XIVE_PROVISIONING:
> if (!xive_native_provision_pages())
> @@ -687,7 +687,7 @@ int xive_native_enable_vp(u32 vp_id, bool single_escalation)
> rc = opal_xive_set_vp_info(vp_id, flags, 0);
> if (rc != OPAL_BUSY)
> break;
> - msleep(1);
> + usleep_range(1000, 1100);
> }
> return rc ? -EIO : 0;
> }
> @@ -701,7 +701,7 @@ int xive_native_disable_vp(u32 vp_id)
> rc = opal_xive_set_vp_info(vp_id, 0, 0);
> if (rc != OPAL_BUSY)
> break;
> - msleep(1);
> + usleep_range(1000, 1100);
> }
> return rc ? -EIO : 0;
> }
kbuild test robot <[email protected]> writes:
> Hi Daniel,
>
> Thank you for the patch! Yet something to improve:
>
> [auto build test ERROR on powerpc/next]
> [also build test ERROR on v4.18-rc4 next-20180709]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
>
> url: https://github.com/0day-ci/linux/commits/Daniel-Klamt/powerpc-Replaced-msleep-with-usleep_range/20180709-231913
> base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
> config: powerpc-defconfig (attached as .config)
> compiler: powerpc64-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
> reproduce:
> wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # save the attached .config to linux build tree
> GCC_VERSION=7.2.0 make.cross ARCH=powerpc
>
> All errors (new ones prefixed by >>):
>
> arch/powerpc/sysdev/xive/native.c: In function 'xive_native_configure_irq':
>>> arch/powerpc/sysdev/xive/native.c:113:2: error: expected ';' before '}' token
> }
> ^
There's also instructions here for building the powerpc kernel:
https://github.com/linuxppc/linux/wiki/Building-powerpc-kernels
cheers
Hi Daniel,
Daniel Klamt <[email protected]> writes:
> Replaced msleep for less than 10ms with usleep_range because will
> often sleep longer than intended.
> For original explanation see:
> Documentation/timers/timers-howto.txt
>
> Signed-off-by: Daniel Klamt <[email protected]>
> Signed-off-by: Bjoern Noetel <[email protected]>
> ---
> arch/powerpc/sysdev/xive/native.c | 24 ++++++++++++------------
> 1 file changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/arch/powerpc/sysdev/xive/native.c b/arch/powerpc/sysdev/xive/native.c
> index 311185b9960a..b164b1cdf4d6 100644
> --- a/arch/powerpc/sysdev/xive/native.c
> +++ b/arch/powerpc/sysdev/xive/native.c
> @@ -109,7 +109,7 @@ int xive_native_configure_irq(u32 hw_irq, u32 target, u8 prio, u32 sw_irq)
> rc = opal_xive_set_irq_config(hw_irq, target, prio, sw_irq);
> if (rc != OPAL_BUSY)
> break;
> - msleep(1);
> + usleep_range(1000, 1100)
> }
We have actually recently added OPAL_BUSY_DELAY_MS (see opal.h) and
should convert these to use that. eg:
msleep(OPAL_BUSY_DELAY_MS);
That has the effect of documenting that this is a short sleep to wait
for opal (our firmware), and we aren't concerned about rounding errors
in msleep().
If you send me a patch doing that I'd be happy to merge it.
cheers
Replaced msleep(x) with with msleep(OPAL_BUSY_DELAY_MS)
to diocument these sleep is to wait for opal.
Signed-off-by: Daniel Klamt <[email protected]>
Signed-off-by: Bjoern Noetel <[email protected]>
---
arch/powerpc/sysdev/xive/native.c | 24 ++++++++++++------------
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/arch/powerpc/sysdev/xive/native.c b/arch/powerpc/sysdev/xive/native.c
index 311185b9960a..39ab5ad58297 100644
--- a/arch/powerpc/sysdev/xive/native.c
+++ b/arch/powerpc/sysdev/xive/native.c
@@ -109,7 +109,7 @@ int xive_native_configure_irq(u32 hw_irq, u32 target, u8 prio, u32 sw_irq)
rc = opal_xive_set_irq_config(hw_irq, target, prio, sw_irq);
if (rc != OPAL_BUSY)
break;
- msleep(1);
+ msleep(OPAL_BUSY_DELAY_MS);
}
return rc == 0 ? 0 : -ENXIO;
}
@@ -163,7 +163,7 @@ int xive_native_configure_queue(u32 vp_id, struct xive_q *q, u8 prio,
rc = opal_xive_set_queue_info(vp_id, prio, qpage_phys, order, flags);
if (rc != OPAL_BUSY)
break;
- msleep(1);
+ msleep(OPAL_BUSY_DELAY_MS);
}
if (rc) {
pr_err("Error %lld setting queue for prio %d\n", rc, prio);
@@ -190,7 +190,7 @@ static void __xive_native_disable_queue(u32 vp_id, struct xive_q *q, u8 prio)
rc = opal_xive_set_queue_info(vp_id, prio, 0, 0, 0);
if (rc != OPAL_BUSY)
break;
- msleep(1);
+ msleep(OPAL_BUSY_DELAY_MS);
}
if (rc)
pr_err("Error %lld disabling queue for prio %d\n", rc, prio);
@@ -253,7 +253,7 @@ static int xive_native_get_ipi(unsigned int cpu, struct xive_cpu *xc)
for (;;) {
irq = opal_xive_allocate_irq(chip_id);
if (irq == OPAL_BUSY) {
- msleep(1);
+ msleep(OPAL_BUSY_DELAY_MS);
continue;
}
if (irq < 0) {
@@ -275,7 +275,7 @@ u32 xive_native_alloc_irq(void)
rc = opal_xive_allocate_irq(OPAL_XIVE_ANY_CHIP);
if (rc != OPAL_BUSY)
break;
- msleep(1);
+ msleep(OPAL_BUSY_DELAY_MS);
}
if (rc < 0)
return 0;
@@ -289,7 +289,7 @@ void xive_native_free_irq(u32 irq)
s64 rc = opal_xive_free_irq(irq);
if (rc != OPAL_BUSY)
break;
- msleep(1);
+ msleep(OPAL_BUSY_DELAY_MS);
}
}
EXPORT_SYMBOL_GPL(xive_native_free_irq);
@@ -305,7 +305,7 @@ static void xive_native_put_ipi(unsigned int cpu, struct xive_cpu *xc)
for (;;) {
rc = opal_xive_free_irq(xc->hw_ipi);
if (rc == OPAL_BUSY) {
- msleep(1);
+ msleep(OPAL_BUSY_DELAY_MS);
continue;
}
xc->hw_ipi = 0;
@@ -400,7 +400,7 @@ static void xive_native_setup_cpu(unsigned int cpu, struct xive_cpu *xc)
rc = opal_xive_set_vp_info(vp, OPAL_XIVE_VP_ENABLED, 0);
if (rc != OPAL_BUSY)
break;
- msleep(1);
+ msleep(OPAL_BUSY_DELAY_MS);
}
if (rc) {
pr_err("Failed to enable pool VP on CPU %d\n", cpu);
@@ -444,7 +444,7 @@ static void xive_native_teardown_cpu(unsigned int cpu, struct xive_cpu *xc)
rc = opal_xive_set_vp_info(vp, 0, 0);
if (rc != OPAL_BUSY)
break;
- msleep(1);
+ msleep(OPAL_BUSY_DELAY_MS);
}
}
@@ -645,7 +645,7 @@ u32 xive_native_alloc_vp_block(u32 max_vcpus)
rc = opal_xive_alloc_vp_block(order);
switch (rc) {
case OPAL_BUSY:
- msleep(1);
+ msleep(OPAL_BUSY_DELAY_MS);
break;
case OPAL_XIVE_PROVISIONING:
if (!xive_native_provision_pages())
@@ -687,7 +687,7 @@ int xive_native_enable_vp(u32 vp_id, bool single_escalation)
rc = opal_xive_set_vp_info(vp_id, flags, 0);
if (rc != OPAL_BUSY)
break;
- msleep(1);
+ msleep(OPAL_BUSY_DELAY_MS);
}
return rc ? -EIO : 0;
}
@@ -701,7 +701,7 @@ int xive_native_disable_vp(u32 vp_id)
rc = opal_xive_set_vp_info(vp_id, 0, 0);
if (rc != OPAL_BUSY)
break;
- msleep(1);
+ msleep(OPAL_BUSY_DELAY_MS);
}
return rc ? -EIO : 0;
}
--
2.11.0
Daniel Klamt <[email protected]> writes:
> Replaced msleep(x) with with msleep(OPAL_BUSY_DELAY_MS)
> to diocument these sleep is to wait for opal.
>
> Signed-off-by: Daniel Klamt <[email protected]>
> Signed-off-by: Bjoern Noetel <[email protected]>
Thanks.
Your change log should be in the imperative mood, see:
https://git.kernel.org/pub/scm/git/git.git/tree/Documentation/SubmittingPatches?id=HEAD#n133
In this case that just means saying "Replace" rather than "Replaced".
Also the prefix should be "powerpc/xive". You can guess that by doing:
$ git log --oneline arch/powerpc/sysdev/xive/native.c
And notice that the majority of commits use that prefix.
I've fixed both of those things up for you.
cheers
On Thu, 12 Jul 2018 15:46:06 +1000
Michael Ellerman <[email protected]> wrote:
> Daniel Klamt <[email protected]> writes:
>
> > Replaced msleep(x) with with msleep(OPAL_BUSY_DELAY_MS)
> > to diocument these sleep is to wait for opal.
> >
> > Signed-off-by: Daniel Klamt <[email protected]>
> > Signed-off-by: Bjoern Noetel <[email protected]>
>
> Thanks.
>
> Your change log should be in the imperative mood, see:
>
> https://git.kernel.org/pub/scm/git/git.git/tree/Documentation/SubmittingPatches?id=HEAD#n133
>
>
> In this case that just means saying "Replace" rather than "Replaced".
>
> Also the prefix should be "powerpc/xive". You can guess that by doing:
>
> $ git log --oneline arch/powerpc/sysdev/xive/native.c
>
> And notice that the majority of commits use that prefix.
>
>
> I've fixed both of those things up for you.
Sorry, just noticed this. I've got a patch which changes the xive stuff
to the "standard" format this will clash with.
if (rc == OPAL_BUSY_EVENT) {
msleep(OPAL_BUSY_DELAY_MS);
opal_poll_events(NULL);
} else if (rc == OPAL_BUSY) {
msleep(OPAL_BUSY_DELAY_MS);
}
If it's already merged that's fine, I can rebase.
Thanks,
Nick
On Wed, 2018-07-11 at 22:03:04 UTC, Daniel Klamt wrote:
> Replaced msleep(x) with with msleep(OPAL_BUSY_DELAY_MS)
> to diocument these sleep is to wait for opal.
>
> Signed-off-by: Daniel Klamt <[email protected]>
> Signed-off-by: Bjoern Noetel <[email protected]>
Applied to powerpc next, thanks.
https://git.kernel.org/powerpc/c/9c3250a127901c93b23723dc5859c7
cheers