[PATCH v3 0/3] clocksource/vt8500: Fix hangs in small delays

[PATCH v3 0/3] clocksource/vt8500: Fix hangs in small delays

[Roman Volkov]

From: Roman Volkov <rvolkov@v1ros.org>

vt8500 hangs in nanosleep() function, starting from commit
c6eb3f70d4482806dc2d3e1e3c7736f497b1d418, making the system unusable.
Per investigation, looks like set_next_event() now receives too small
delta and fails with -ETIME.

Google group discussion:
https://groups.google.com/forum/#!topic/vt8500-wm8505-linux-kernel/vDMF_mDOb1k

v2:
Address comments by Alexey Charkov. Merge patches to get less amount of
changes (three patches instead of four).

v3:
Address comments by Thomas Gleixner. Edit the changelog.

Tested on my WM8650, no issues in three days uptime.

Roman Volkov (3):
  clocksource/vt8500: Use MIN_OSCR_DELTA from PXA
  clocksource/vt8500: Remove the 'loops' variable
  clocksource/vt8500: Add register R/W functions

 drivers/clocksource/vt8500_timer.c | 98 +++++++++++++++++++++++++++-----------
 1 file changed, 69 insertions(+), 29 deletions(-)

-- 
2.6.2

[PATCH v3 1/3] clocksource/vt8500: Increase the minimum delta

[Roman Volkov]

From: Roman Volkov <rvolkov@v1ros.org>

The vt8500 clocksource driver declares itself as capable to handle the
minimum delay of 4 cycles by passing the value into
clockevents_config_and_register(). The vt8500_timer_set_next_event()
requires the passed cycles value to be at least 16. The impact is that
userspace hangs in nanosleep() calls with small delay intervals.

This problem is reproducible in Linux 4.2 starting from:
c6eb3f70d448 ('hrtimer: Get rid of hrtimer softirq')

Signed-off-by: Roman Volkov <rvolkov@v1ros.org>
Acked-by: Alexey Charkov <alchark@gmail.com>
---
 drivers/clocksource/vt8500_timer.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/clocksource/vt8500_timer.c b/drivers/clocksource/vt8500_timer.c
index a92e94b..dfc3bb4 100644
--- a/drivers/clocksource/vt8500_timer.c
+++ b/drivers/clocksource/vt8500_timer.c
@@ -50,6 +50,8 @@
 
 #define msecs_to_loops(t) (loops_per_jiffy / 1000 * HZ * t)
 
+#define MIN_OSCR_DELTA		16
+
 static void __iomem *regbase;
 
 static cycle_t vt8500_timer_read(struct clocksource *cs)
@@ -80,7 +82,7 @@ static int vt8500_timer_set_next_event(unsigned long cycles,
 		cpu_relax();
 	writel((unsigned long)alarm, regbase + TIMER_MATCH_VAL);
 
-	if ((signed)(alarm - clocksource.read(&clocksource)) <= 16)
+	if ((signed)(alarm - clocksource.read(&clocksource)) <= MIN_OSCR_DELTA)
 		return -ETIME;
 
 	writel(1, regbase + TIMER_IER_VAL);
@@ -151,7 +153,7 @@ static void __init vt8500_timer_init(struct device_node *np)
 		pr_err("%s: setup_irq failed for %s\n", __func__,
 							clockevent.name);
 	clockevents_config_and_register(&clockevent, VT8500_TIMER_HZ,
-					4, 0xf0000000);
+					MIN_OSCR_DELTA * 2, 0xf0000000);
 }
 
 CLOCKSOURCE_OF_DECLARE(vt8500, "via,vt8500-timer", vt8500_timer_init);
-- 
2.6.2

[PATCH v3 2/3] clocksource/vt8500: Remove the 'loops' variable

[Roman Volkov]

From: Roman Volkov <rvolkov@v1ros.org>

The purpose of the 'loops' variable is unclear. vt8500 hardware does not
require any protections, in case if these variables intended for preventing
infinite loops (identical PXA timer works perfectly without these ones).

Signed-off-by: Roman Volkov <rvolkov@v1ros.org>
Acked-by: Alexey Charkov <alchark@gmail.com>
---
 drivers/clocksource/vt8500_timer.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/clocksource/vt8500_timer.c b/drivers/clocksource/vt8500_timer.c
index dfc3bb4..eb08d96 100644
--- a/drivers/clocksource/vt8500_timer.c
+++ b/drivers/clocksource/vt8500_timer.c
@@ -48,18 +48,14 @@
 #define TIMER_COUNT_W_ACTIVE	(1 << 4)	/* not ready for write */
 #define TIMER_MATCH_W_ACTIVE	(1 << 0)	/* not ready for write */
 
-#define msecs_to_loops(t) (loops_per_jiffy / 1000 * HZ * t)
-
 #define MIN_OSCR_DELTA		16
 
 static void __iomem *regbase;
 
 static cycle_t vt8500_timer_read(struct clocksource *cs)
 {
-	int loops = msecs_to_loops(10);
 	writel(3, regbase + TIMER_CTRL_VAL);
-	while ((readl((regbase + TIMER_AS_VAL)) & TIMER_COUNT_R_ACTIVE)
-						&& --loops)
+	while (readl(regbase + TIMER_AS_VAL) & TIMER_COUNT_R_ACTIVE)
 		cpu_relax();
 	return readl(regbase + TIMER_COUNT_VAL);
 }
@@ -75,10 +71,8 @@ static struct clocksource clocksource = {
 static int vt8500_timer_set_next_event(unsigned long cycles,
 				    struct clock_event_device *evt)
 {
-	int loops = msecs_to_loops(10);
 	cycle_t alarm = clocksource.read(&clocksource) + cycles;
-	while ((readl(regbase + TIMER_AS_VAL) & TIMER_MATCH_W_ACTIVE)
-						&& --loops)
+	while (readl(regbase + TIMER_AS_VAL) & TIMER_MATCH_W_ACTIVE)
 		cpu_relax();
 	writel((unsigned long)alarm, regbase + TIMER_MATCH_VAL);
 
-- 
2.6.2

[PATCH v3 3/3] clocksource/vt8500: Add register R/W functions

[Roman Volkov]

From: Roman Volkov <rvolkov@v1ros.org>

vt8500 timer requires special synchronization for accessing some of its
registers. Define special read and write functions to handle this process
transparently.

Use relaxed read/write, according to the following:
http://permalink.gmane.org/gmane.linux.ports.arm.kernel/117658:
"For accesses to the same device we don't
actually need any barriers on ARM as this is guaranteed by the
architecture."

To perform a read from the Timer Count register, user must write a one
to the Timer Control register and wait for completion flag by polling the
Timer Read Count Active bit.

To perform a write to the Count or Match registers, user must poll the
write completion flag for the corresponding register to ensure that the
previous write completed and then write the actual value.

Signed-off-by: Roman Volkov <rvolkov@v1ros.org>
Acked-by: Alexey Charkov <alchark@gmail.com>
---
 drivers/clocksource/vt8500_timer.c | 88 ++++++++++++++++++++++++++++----------
 1 file changed, 66 insertions(+), 22 deletions(-)

diff --git a/drivers/clocksource/vt8500_timer.c b/drivers/clocksource/vt8500_timer.c
index eb08d96..fd88a5b 100644
--- a/drivers/clocksource/vt8500_timer.c
+++ b/drivers/clocksource/vt8500_timer.c
@@ -38,32 +38,75 @@
 
 #define VT8500_TIMER_OFFSET	0x0100
 #define VT8500_TIMER_HZ		3000000
-#define TIMER_MATCH_VAL		0x0000
+#define TIMER_MATCH0_VAL	0x0000
+#define TIMER_MATCH1_VAL	0x0004
+#define TIMER_MATCH2_VAL	0x0008
+#define TIMER_MATCH3_VAL	0x000c
 #define TIMER_COUNT_VAL		0x0010
 #define TIMER_STATUS_VAL	0x0014
 #define TIMER_IER_VAL		0x001c		/* interrupt enable */
 #define TIMER_CTRL_VAL		0x0020
 #define TIMER_AS_VAL		0x0024		/* access status */
-#define TIMER_COUNT_R_ACTIVE	(1 << 5)	/* not ready for read */
-#define TIMER_COUNT_W_ACTIVE	(1 << 4)	/* not ready for write */
-#define TIMER_MATCH_W_ACTIVE	(1 << 0)	/* not ready for write */
+/* R/W status flags */
+#define TIMER_COUNT_R_ACTIVE	(1 << 5)
+#define TIMER_COUNT_W_ACTIVE	(1 << 4)
+#define TIMER_MATCH3_W_ACTIVE	(1 << 3)
+#define TIMER_MATCH2_W_ACTIVE	(1 << 2)
+#define TIMER_MATCH1_W_ACTIVE	(1 << 1)
+#define TIMER_MATCH0_W_ACTIVE	(1 << 0)
 
 #define MIN_OSCR_DELTA		16
 
+#define vt8500_timer_sync(bit)	{ while (readl_relaxed \
+				    (regbase + TIMER_AS_VAL) & bit) \
+					cpu_relax(); }
+
 static void __iomem *regbase;
 
-static cycle_t vt8500_timer_read(struct clocksource *cs)
+static void vt8500_timer_write(u32 value, unsigned long reg)
 {
-	writel(3, regbase + TIMER_CTRL_VAL);
-	while (readl(regbase + TIMER_AS_VAL) & TIMER_COUNT_R_ACTIVE)
-		cpu_relax();
-	return readl(regbase + TIMER_COUNT_VAL);
+	switch (reg) {
+	case TIMER_COUNT_VAL:
+		vt8500_timer_sync(TIMER_COUNT_W_ACTIVE);
+		break;
+	case TIMER_MATCH0_VAL:
+		vt8500_timer_sync(TIMER_MATCH0_W_ACTIVE);
+		break;
+	case TIMER_MATCH1_VAL:
+		vt8500_timer_sync(TIMER_MATCH1_W_ACTIVE);
+		break;
+	case TIMER_MATCH2_VAL:
+		vt8500_timer_sync(TIMER_MATCH2_W_ACTIVE);
+		break;
+	case TIMER_MATCH3_VAL:
+		vt8500_timer_sync(TIMER_MATCH3_W_ACTIVE);
+		break;
+	}
+
+	writel_relaxed(value, regbase + reg);
+}
+
+static u32 vt8500_timer_read(unsigned long reg)
+{
+	if (reg == TIMER_COUNT_VAL) {
+		vt8500_timer_write(3, TIMER_CTRL_VAL);
+		vt8500_timer_sync(TIMER_COUNT_R_ACTIVE);
+
+		return readl_relaxed(regbase + TIMER_COUNT_VAL);
+	}
+
+	return readl_relaxed(regbase + reg);
+}
+
+static cycle_t vt8500_oscr0_read(struct clocksource *cs)
+{
+	return vt8500_timer_read(TIMER_COUNT_VAL);
 }
 
 static struct clocksource clocksource = {
 	.name           = "vt8500_timer",
 	.rating         = 200,
-	.read           = vt8500_timer_read,
+	.read           = vt8500_oscr0_read,
 	.mask           = CLOCKSOURCE_MASK(32),
 	.flags          = CLOCK_SOURCE_IS_CONTINUOUS,
 };
@@ -71,23 +114,24 @@ static struct clocksource clocksource = {
 static int vt8500_timer_set_next_event(unsigned long cycles,
 				    struct clock_event_device *evt)
 {
-	cycle_t alarm = clocksource.read(&clocksource) + cycles;
-	while (readl(regbase + TIMER_AS_VAL) & TIMER_MATCH_W_ACTIVE)
-		cpu_relax();
-	writel((unsigned long)alarm, regbase + TIMER_MATCH_VAL);
+	unsigned long alarm = vt8500_timer_read(TIMER_COUNT_VAL) + cycles;
 
-	if ((signed)(alarm - clocksource.read(&clocksource)) <= MIN_OSCR_DELTA)
+	vt8500_timer_write(alarm, TIMER_MATCH0_VAL);
+	if ((signed)(alarm - vt8500_timer_read(
+				TIMER_COUNT_VAL)) <= MIN_OSCR_DELTA) {
 		return -ETIME;
+	}
 
-	writel(1, regbase + TIMER_IER_VAL);
+	vt8500_timer_write(1, TIMER_IER_VAL);
 
 	return 0;
 }
 
 static int vt8500_shutdown(struct clock_event_device *evt)
 {
-	writel(readl(regbase + TIMER_CTRL_VAL) | 1, regbase + TIMER_CTRL_VAL);
-	writel(0, regbase + TIMER_IER_VAL);
+	vt8500_timer_write(vt8500_timer_read(TIMER_CTRL_VAL) | 1,
+					TIMER_CTRL_VAL);
+	vt8500_timer_write(0, TIMER_IER_VAL);
 	return 0;
 }
 
@@ -103,7 +147,7 @@ static struct clock_event_device clockevent = {
 static irqreturn_t vt8500_timer_interrupt(int irq, void *dev_id)
 {
 	struct clock_event_device *evt = dev_id;
-	writel(0xf, regbase + TIMER_STATUS_VAL);
+	vt8500_timer_write(0xf, TIMER_STATUS_VAL);
 	evt->event_handler(evt);
 
 	return IRQ_HANDLED;
@@ -133,9 +177,9 @@ static void __init vt8500_timer_init(struct device_node *np)
 		return;
 	}
 
-	writel(1, regbase + TIMER_CTRL_VAL);
-	writel(0xf, regbase + TIMER_STATUS_VAL);
-	writel(~0, regbase + TIMER_MATCH_VAL);
+	vt8500_timer_write(1, TIMER_CTRL_VAL);
+	vt8500_timer_write(0xf, TIMER_STATUS_VAL);
+	vt8500_timer_write(~0, TIMER_MATCH0_VAL);
 
 	if (clocksource_register_hz(&clocksource, VT8500_TIMER_HZ))
 		pr_err("%s: vt8500_timer_init: clocksource_register failed for %s\n",
-- 
2.6.2

Re: [PATCH v3 1/3] clocksource/vt8500: Increase the minimum delta

[Daniel Lezcano]

On 01/01/2016 02:24 PM, Roman Volkov wrote:
> From: Roman Volkov <rvolkov@v1ros.org>
>
> The vt8500 clocksource driver declares itself as capable to handle the
> minimum delay of 4 cycles by passing the value into
> clockevents_config_and_register(). The vt8500_timer_set_next_event()
> requires the passed cycles value to be at least 16. The impact is that
> userspace hangs in nanosleep() calls with small delay intervals.
>
> This problem is reproducible in Linux 4.2 starting from:
> c6eb3f70d448 ('hrtimer: Get rid of hrtimer softirq')
>
> Signed-off-by: Roman Volkov <rvolkov@v1ros.org>
> Acked-by: Alexey Charkov <alchark@gmail.com>

Hi Roman,

I looked at the email thread, and IIUC if set_next_event fails, the 
system freeze. Your patch fixes the issue for your driver but not the 
real issue because if set_next_event fails, at least a warning should 
appear in the log or better nanosleep should fail gracefully.

BTW why min delta is MIN_OSCR_DELTA * 2 in clockevents_config_and_register ?

> ---
>   drivers/clocksource/vt8500_timer.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/clocksource/vt8500_timer.c b/drivers/clocksource/vt8500_timer.c
> index a92e94b..dfc3bb4 100644
> --- a/drivers/clocksource/vt8500_timer.c
> +++ b/drivers/clocksource/vt8500_timer.c
> @@ -50,6 +50,8 @@
>
>   #define msecs_to_loops(t) (loops_per_jiffy / 1000 * HZ * t)
>
> +#define MIN_OSCR_DELTA		16
> +
>   static void __iomem *regbase;
>
>   static cycle_t vt8500_timer_read(struct clocksource *cs)
> @@ -80,7 +82,7 @@ static int vt8500_timer_set_next_event(unsigned long cycles,
>   		cpu_relax();
>   	writel((unsigned long)alarm, regbase + TIMER_MATCH_VAL);
>
> -	if ((signed)(alarm - clocksource.read(&clocksource)) <= 16)
> +	if ((signed)(alarm - clocksource.read(&clocksource)) <= MIN_OSCR_DELTA)
>   		return -ETIME;
>
>   	writel(1, regbase + TIMER_IER_VAL);
> @@ -151,7 +153,7 @@ static void __init vt8500_timer_init(struct device_node *np)
>   		pr_err("%s: setup_irq failed for %s\n", __func__,
>   							clockevent.name);
>   	clockevents_config_and_register(&clockevent, VT8500_TIMER_HZ,
> -					4, 0xf0000000);
> +					MIN_OSCR_DELTA * 2, 0xf0000000);
>   }
>
>   CLOCKSOURCE_OF_DECLARE(vt8500, "via,vt8500-timer", vt8500_timer_init);
>


-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH v3 1/3] clocksource/vt8500: Increase the minimum delta

[Roman Volkov]

В Tue, 5 Jan 2016 10:01:07 +0100
Daniel Lezcano <daniel.lezcano@linaro.org> пишет:

> On 01/01/2016 02:24 PM, Roman Volkov wrote:
> > From: Roman Volkov <rvolkov@v1ros.org>
> >
> > The vt8500 clocksource driver declares itself as capable to handle
> > the minimum delay of 4 cycles by passing the value into
> > clockevents_config_and_register(). The vt8500_timer_set_next_event()
> > requires the passed cycles value to be at least 16. The impact is
> > that userspace hangs in nanosleep() calls with small delay
> > intervals.
> >
> > This problem is reproducible in Linux 4.2 starting from:
> > c6eb3f70d448 ('hrtimer: Get rid of hrtimer softirq')
> >
> > Signed-off-by: Roman Volkov <rvolkov@v1ros.org>
> > Acked-by: Alexey Charkov <alchark@gmail.com>  
> 
> Hi Roman,
> 
> I looked at the email thread, and IIUC if set_next_event fails, the 
> system freeze. Your patch fixes the issue for your driver but not the 
> real issue because if set_next_event fails, at least a warning should 
> appear in the log or better nanosleep should fail gracefully.

Hi Daniel,

I agree, but if nanosleep will return immediately, this can lead to
undefined behavior in the software. Maybe the system can go busyloop
to somehow recover from this state and print a message to the log? At
the driver level it seems to be enough to fail the function without
printing logs.
 
> BTW why min delta is MIN_OSCR_DELTA * 2 in
> clockevents_config_and_register ?

All this just to be consistent with PXA. Maybe PXA works with lesser
values, e.g., 8. For vt8500, accessing the registers is more complex,
and this should consume more time. IIUC, if the driver does not support
too small delays, the system will handle it with busyloop?

Why multiply by two? Good question. Maybe there is a reserve for
stability. The value passed by the system to the set_next_event() should
be not lesser than this value, and theoretically, we should not
multiply MIN_OSCR_DELTA by two. As I can see, in many drivers there is
no such minimal values at all.

Added Robert

Regards,
Roman

Re: [PATCH v3 1/3] clocksource/vt8500: Increase the minimum delta

[Russell King - ARM Linux]

On Tue, Jan 05, 2016 at 12:42:42PM +0300, Roman Volkov wrote:
> Why multiply by two? Good question. Maybe there is a reserve for
> stability. The value passed by the system to the set_next_event() should
> be not lesser than this value, and theoretically, we should not
> multiply MIN_OSCR_DELTA by two. As I can see, in many drivers there is
> no such minimal values at all.

It's a speciality of the StrongARM/PXA hardware.  It takes a certain
number of OSCR cycles for the value written to hit the compare registers.
So, if a very small delta is written (eg, the compare register is written
with a value of OSCR + 1), the OSCR will have incremented past this value
before it hits the underlying hardware.  The result is, that you end up
waiting a very long time for the OSCR to wrap before the event fires.

So, we introduce a check in set_next_event() to detect this and return
-ETIME if the calculated delta is too small, which causes the generic
clockevents code to retry after adding the min_delta specified in
clockevents_config_and_register() to the current time value.

min_delta must be sufficient that we don't re-trip the -ETIME check - if
we do, we will return -ETIME, forward the next event time, try to set it,
return -ETIME again, and basically lock the system up.  So, min_delta
must be larger than the check inside set_next_event().  A factor of two
was chosen to ensure that this situation would never occur.

The PXA code worked on PXA systems for years, and I'd suggest no one
changes this mechanism without access to a wide range of PXA systems,
otherwise they're risking breakage.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH v3 1/3] clocksource/vt8500: Increase the minimum delta

[Daniel Lezcano]

On 01/05/2016 10:42 AM, Roman Volkov wrote:
> В Tue, 5 Jan 2016 10:01:07 +0100
> Daniel Lezcano <daniel.lezcano@linaro.org> пишет:
>
>> On 01/01/2016 02:24 PM, Roman Volkov wrote:
>>> From: Roman Volkov <rvolkov@v1ros.org>
>>>
>>> The vt8500 clocksource driver declares itself as capable to handle
>>> the minimum delay of 4 cycles by passing the value into
>>> clockevents_config_and_register(). The vt8500_timer_set_next_event()
>>> requires the passed cycles value to be at least 16. The impact is
>>> that userspace hangs in nanosleep() calls with small delay
>>> intervals.
>>>
>>> This problem is reproducible in Linux 4.2 starting from:
>>> c6eb3f70d448 ('hrtimer: Get rid of hrtimer softirq')
>>>
>>> Signed-off-by: Roman Volkov <rvolkov@v1ros.org>
>>> Acked-by: Alexey Charkov <alchark@gmail.com>
>>
>> Hi Roman,
>>
>> I looked at the email thread, and IIUC if set_next_event fails, the
>> system freeze. Your patch fixes the issue for your driver but not the
>> real issue because if set_next_event fails, at least a warning should
>> appear in the log or better nanosleep should fail gracefully.
>
> Hi Daniel,
>
> I agree, but if nanosleep will return immediately, this can lead to
> undefined behavior in the software.

The nanosleep syscall is supposed to return an error code. If the 
software does not pay attention to the syscall's return code, then the 
bug is in the software, it is not up to the kernel to work around it.

> Maybe the system can go busyloop
> to somehow recover from this state and print a message to the log? At
> the driver level it seems to be enough to fail the function without
> printing logs.
>
>> BTW why min delta is MIN_OSCR_DELTA * 2 in
>> clockevents_config_and_register ?
>
> All this just to be consistent with PXA. Maybe PXA works with lesser
> values, e.g., 8. For vt8500, accessing the registers is more complex,
> and this should consume more time. IIUC, if the driver does not support
> too small delays, the system will handle it with busyloop?

[ Added John Stultz and Thomas Gleixner ] to answer those questions above.

> Why multiply by two? Good question. Maybe there is a reserve for
> stability. The value passed by the system to the set_next_event() should
> be not lesser than this value, and theoretically, we should not
> multiply MIN_OSCR_DELTA by two. As I can see, in many drivers there is
> no such minimal values at all.
>
> Added Robert
>
> Regards,
> Roman
>


-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH v3 1/3] clocksource/vt8500: Increase the minimum delta

[Daniel Lezcano]

On 01/05/2016 11:00 AM, Russell King - ARM Linux wrote:
> On Tue, Jan 05, 2016 at 12:42:42PM +0300, Roman Volkov wrote:
>> Why multiply by two? Good question. Maybe there is a reserve for
>> stability. The value passed by the system to the set_next_event() should
>> be not lesser than this value, and theoretically, we should not
>> multiply MIN_OSCR_DELTA by two. As I can see, in many drivers there is
>> no such minimal values at all.
>
> It's a speciality of the StrongARM/PXA hardware.  It takes a certain
> number of OSCR cycles for the value written to hit the compare registers.
> So, if a very small delta is written (eg, the compare register is written
> with a value of OSCR + 1), the OSCR will have incremented past this value
> before it hits the underlying hardware.  The result is, that you end up
> waiting a very long time for the OSCR to wrap before the event fires.
>
> So, we introduce a check in set_next_event() to detect this and return
> -ETIME if the calculated delta is too small, which causes the generic
> clockevents code to retry after adding the min_delta specified in
> clockevents_config_and_register() to the current time value.
>
> min_delta must be sufficient that we don't re-trip the -ETIME check - if
> we do, we will return -ETIME, forward the next event time, try to set it,
> return -ETIME again, and basically lock the system up.  So, min_delta
> must be larger than the check inside set_next_event().  A factor of two
> was chosen to ensure that this situation would never occur.

Russell,

thank you for taking the time to write this detailed explanation. I 
believe that clarifies everything (the issue with the lockup and the 
value of the min delta).

Roman,

If we are in the situation Russell is describing above, failing 
gracefully as mentioned before does not make sense.

Do you have a idea why this is happening with 4.2 and not before ?

> The PXA code worked on PXA systems for years, and I'd suggest no one
> changes this mechanism without access to a wide range of PXA systems,
> otherwise they're risking breakage.

Copy that :)


-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH v3 1/3] clocksource/vt8500: Increase the minimum delta

[Roman Volkov]

В Tue, 5 Jan 2016 11:31:37 +0100
Daniel Lezcano <daniel.lezcano@linaro.org> пишет:

> On 01/05/2016 11:00 AM, Russell King - ARM Linux wrote:
> > On Tue, Jan 05, 2016 at 12:42:42PM +0300, Roman Volkov wrote:  
> >> Why multiply by two? Good question. Maybe there is a reserve for
> >> stability. The value passed by the system to the set_next_event()
> >> should be not lesser than this value, and theoretically, we should
> >> not multiply MIN_OSCR_DELTA by two. As I can see, in many drivers
> >> there is no such minimal values at all.  
> >
> > It's a speciality of the StrongARM/PXA hardware.  It takes a certain
> > number of OSCR cycles for the value written to hit the compare
> > registers. So, if a very small delta is written (eg, the compare
> > register is written with a value of OSCR + 1), the OSCR will have
> > incremented past this value before it hits the underlying
> > hardware.  The result is, that you end up waiting a very long time
> > for the OSCR to wrap before the event fires.
> >
> > So, we introduce a check in set_next_event() to detect this and
> > return -ETIME if the calculated delta is too small, which causes
> > the generic clockevents code to retry after adding the min_delta
> > specified in clockevents_config_and_register() to the current time
> > value.
> >
> > min_delta must be sufficient that we don't re-trip the -ETIME check
> > - if we do, we will return -ETIME, forward the next event time, try
> > to set it, return -ETIME again, and basically lock the system up.
> > So, min_delta must be larger than the check inside
> > set_next_event().  A factor of two was chosen to ensure that this
> > situation would never occur.  
> 
> Russell,
> 
> thank you for taking the time to write this detailed explanation. I 
> believe that clarifies everything (the issue with the lockup and the 
> value of the min delta).

Yes, thanks for the explanation how this exactly works! Some points
were not obvious.

> Roman,
> 
> If we are in the situation Russell is describing above, failing 
> gracefully as mentioned before does not make sense.
> 
> Do you have a idea why this is happening with 4.2 and not before ?

No, which change from c6eb3f70 caused this problem is unclear for me.
Maybe the new IRQ handling revealed this defect. What is obvious now,
the value passed to clockevents_config_and_register() was incorrect.

Regards,
Roman

Re: [PATCH v3 0/3] clocksource/vt8500: Fix hangs in small delays

[Daniel Lezcano]

On 01/01/2016 02:24 PM, Roman Volkov wrote:
> From: Roman Volkov <rvolkov@v1ros.org>
>
> vt8500 hangs in nanosleep() function, starting from commit
> c6eb3f70d4482806dc2d3e1e3c7736f497b1d418, making the system unusable.
> Per investigation, looks like set_next_event() now receives too small
> delta and fails with -ETIME.


Hi Roman,

I think the patch 1/3 should go as a fix for 4.4-rc7 and stable and the 
two other should go for the next version as they are improvements 
regarding the current code.

-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH v3 0/3] clocksource/vt8500: Fix hangs in small delays

[Roman Volkov]

В Wed, 6 Jan 2016 15:24:07 +0100
Daniel Lezcano <daniel.lezcano@linaro.org> пишет:

> On 01/01/2016 02:24 PM, Roman Volkov wrote:
> > From: Roman Volkov <rvolkov@v1ros.org>
> >
> > vt8500 hangs in nanosleep() function, starting from commit
> > c6eb3f70d4482806dc2d3e1e3c7736f497b1d418, making the system
> > unusable. Per investigation, looks like set_next_event() now
> > receives too small delta and fails with -ETIME.  
> 
> 
> Hi Roman,
> 
> I think the patch 1/3 should go as a fix for 4.4-rc7 and stable and
> the two other should go for the next version as they are improvements 
> regarding the current code.
> 

Hi Daniel,

I agree, -stable can live without the patches 2 an 3.

Regards,
Roman

Re: [PATCH v3 0/3] clocksource/vt8500: Fix hangs in small delays

[Daniel Lezcano]

On 01/06/2016 04:30 PM, Roman Volkov wrote:
> В Wed, 6 Jan 2016 15:24:07 +0100
> Daniel Lezcano <daniel.lezcano@linaro.org> пишет:
>
>> On 01/01/2016 02:24 PM, Roman Volkov wrote:
>>> From: Roman Volkov <rvolkov@v1ros.org>
>>>
>>> vt8500 hangs in nanosleep() function, starting from commit
>>> c6eb3f70d4482806dc2d3e1e3c7736f497b1d418, making the system
>>> unusable. Per investigation, looks like set_next_event() now
>>> receives too small delta and fails with -ETIME.
>>
>>
>> Hi Roman,
>>
>> I think the patch 1/3 should go as a fix for 4.4-rc7 and stable and
>> the two other should go for the next version as they are improvements
>> regarding the current code.
>>
>
> Hi Daniel,
>
> I agree, -stable can live without the patches 2 an 3.

Ok, I applied patch 1 for fix/urgent + stable and 2/3 for the next release.

-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog