[PATCH] powerpc: Replaced msleep with usleep_range

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 <redacted>
Signed-off-by: Bjoern Noetel <redacted>
---
 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':

quoted

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 [off-list ref]
:::::: CC: Michael Ellerman [off-list ref]

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

kbuild test robot [off-list ref] 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':

quoted

quoted

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 [off-list ref] writes:

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 <redacted>
Signed-off-by: Bjoern Noetel <redacted>
---
 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 [off-list ref] 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 <redacted>
Signed-off-by: Bjoern Noetel <redacted>

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 [off-list ref] wrote:

Daniel Klamt [off-list ref] writes:

quoted

Replaced msleep(x) with with msleep(OPAL_BUSY_DELAY_MS)
to diocument these sleep is to wait for opal.

Signed-off-by: Daniel Klamt <redacted>
Signed-off-by: Bjoern Noetel <redacted>  

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 <redacted>
Signed-off-by: Bjoern Noetel <redacted>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/9c3250a127901c93b23723dc5859c7

cheers