ARM: bcm283x: Repair wdog.h

Reorder the timer.h file so it can be included from board config file.

Signed-off-by Marek Vasut <[email protected]>

Cc Stephen Warren <[email protected]>

Cc Tyler Baker <[email protected]>

The get_timer_us() function is something which is no longer existing in case we use generic timer framework, so replace it with get_timer().

Signed-off-by Marek Vasut <[email protected]>

Cc Stephen Warren <[email protected]>

Cc Tyler Baker <[email protected]>

---
drivers/mmc/bcm2835_sdhci.c | 4 ++--

1 file changed, 2 insertions(+), 2 deletions(-)

drivers/mmc/bcm2835_sdhci.c +2 -2

index 4ec2968..0f81add 100644
@@ -69,11 +69,11 @@ static inline void bcm2835_sdhci_raw_writel(struct sdhci_host *host, u32 val,
 	 * (Which is just as well - otherwise we'd have to nobble the DMA engine
 	 * too)
 	 */
-	while (get_timer_us(bcm_host->last_write) < bcm_host->twoticks_delay)
+	while (get_timer(bcm_host->last_write) < bcm_host->twoticks_delay)
 		;
 
 	writel(val, host->ioaddr + reg);
-	bcm_host->last_write = get_timer_us(0);
+	bcm_host->last_write = get_timer(0);
 }
 
 static inline u32 bcm2835_sdhci_raw_readl(struct sdhci_host *host, int reg)

Signature · 1 line

2.1.4

The get_timer_us() function is something which is no longer existing in case we use generic timer framework, so replace it with get_timer().

Quoted 3 lines · Marek Vasut

The get_timer_us() function is something which is no longer existing in case we use generic timer framework, so replace it with get_timer().

Marek,

This patch causes "saveenv" to got from almost no time to nearly 50s on my RPi model A+. Can you take a look at that please?

Quoted 5 lines · Stephen Warren

Quoted 3 lines · Marek Vasut

The get_timer_us() function is something which is no longer existing in case we use generic timer framework, so replace it with get_timer().

Quoted 4 lines

Marek,

This patch causes "saveenv" to got from almost no time to nearly 50s on my RPi model A+. Can you take a look at that please?

Can you try the attached diff ? ;-/ I think I mistakenly used get_timer(), which returns msecs instead of timer_get_us() which reports usecs, sorry.

Quoted 8 lines · Marek Vasut

Quoted 7 lines · Stephen Warren

Quoted 3 lines · Marek Vasut

The get_timer_us() function is something which is no longer existing in case we use generic timer framework, so replace it with get_timer().

Marek,

Quoted 6 lines

Quoted 2 lines

This patch causes "saveenv" to got from almost no time to nearly 50s on my RPi model A+. Can you take a look at that please?

Can you try the attached diff ? ;-/ I think I mistakenly used get_timer(), which returns msecs instead of timer_get_us() which reports usecs, sorry.

I can confirm this solves the regression.

Quoted 12 lines · Jakub Kici?ski

Quoted 11 lines · Marek Vasut

Quoted 9 lines · Stephen Warren

Quoted 3 lines · Marek Vasut

The get_timer_us() function is something which is no longer existing in case we use generic timer framework, so replace it with get_timer().

Marek,

This patch causes "saveenv" to got from almost no time to nearly 50s on my RPi model A+. Can you take a look at that please?

Quoted 5 lines

Quoted 3 lines

Can you try the attached diff ? ;-/ I think I mistakenly used get_timer(), which returns msecs instead of timer_get_us() which reports usecs, sorry.

I can confirm this solves the regression.

That's not a regression, that was a bug ;-)

Quoted 17 lines · Marek Vasut

Quoted 16 lines · Jakub Kici?ski

Quoted 14 lines · Marek Vasut

Quoted 9 lines · Stephen Warren

Quoted 3 lines · Marek Vasut

The get_timer_us() function is something which is no longer existing in case we use generic timer framework, so replace it with get_timer().

Marek,

This patch causes "saveenv" to got from almost no time to nearly 50s on my RPi model A+. Can you take a look at that please?

Can you try the attached diff ? ;-/ I think I mistakenly used get_timer(), which returns msecs instead of timer_get_us() which reports usecs, sorry.

Quoted 3 lines

Quoted 1 line

I can confirm this solves the regression.

That's not a regression, that was a bug ;-)

Ach OK, I thought Stephen said that MMC used to be fast before the offending patch ;) Anyway I don't really know what's the difference and wikipedia says regression is a bug too so we may both be right :P

Quoted 5 lines · Stephen Warren

Quoted 3 lines · Marek Vasut

The get_timer_us() function is something which is no longer existing in case we use generic timer framework, so replace it with get_timer().

Quoted 4 lines

Marek,

This patch causes "saveenv" to got from almost no time to nearly 50s on my RPi model A+. Can you take a look at that please?

Can you try the attached diff ? ;-/ I think I mistakenly used get_timer(), which returns msecs instead of timer_get_us() which reports usecs, sorry.

Switch to generic timer implementation from lib/time.c . This also fixes a signed overflow which was in __udelay() implementation.

Quoted 3 lines · Marek Vasut

Switch to generic timer implementation from lib/time.c . This also fixes a signed overflow which was in __udelay() implementation.

Can you explain that a bit more?

Quoted 11 lines

arch/arm/mach-bcm283x/timer.c deleted +0 -11

-void __udelay(unsigned long usec)
-{
-	ulong endtime;
-	signed long diff;
-
-	endtime = get_timer_us(0) + usec;
-
-	do {
-		ulong now = get_timer_us(0);
-		diff = endtime - now;
-	} while (diff >= 0);

Quoted 1 line

-}

I believe since endtime and now hold micro seconds, there shouldn't be any overflow so long as the microsecond difference fits into 31 bits, i.e. so long as usec is less than ~36 minutes. I doubt anything is calling __udelay() with that large of a value. Perhaps the issue this patch fixes is in get_timer_us(0) instead, or something else changed as a side-effect?

Quoted 18 lines · Stephen Warren

Quoted 3 lines · Marek Vasut

Switch to generic timer implementation from lib/time.c . This also fixes a signed overflow which was in __udelay() implementation.

Can you explain that a bit more?

Quoted 11 lines

arch/arm/mach-bcm283x/timer.c deleted +0 -11

-void __udelay(unsigned long usec)
-{
-	ulong endtime;
-	signed long diff;
-
-	endtime = get_timer_us(0) + usec;
-
-	do {
-		ulong now = get_timer_us(0);
-		diff = endtime - now;
-	} while (diff >= 0);

Quoted 8 lines

Quoted 1 line

-}

I believe since endtime and now hold micro seconds, there shouldn't be any overflow so long as the microsecond difference fits into 31 bits, i.e. so long as usec is less than ~36 minutes. I doubt anything is calling __udelay() with that large of a value. Perhaps the issue this patch fixes is in get_timer_us(0) instead, or something else changed as a side-effect?

The generic implementation caters for full 32-bit range, that's all. Since the argument of this function is unsigned, it can overflow if you use argument which is bigger than 31 bits. OK like that ?

Quoted 28 lines · Marek Vasut

Quoted 26 lines · Stephen Warren

Quoted 3 lines · Marek Vasut

Switch to generic timer implementation from lib/time.c . This also fixes a signed overflow which was in __udelay() implementation.

Can you explain that a bit more?

arch/arm/mach-bcm283x/timer.c line 58

Quoted 12 lines

arch/arm/mach-bcm283x/timer.c deleted +0 -12

-void __udelay(unsigned long usec)
-{
-	ulong endtime;
-	signed long diff;
-
-	endtime = get_timer_us(0) + usec;
-
-	do {
-		ulong now = get_timer_us(0);
-		diff = endtime - now;
-	} while (diff >= 0);
-}

I believe since endtime and now hold micro seconds, there shouldn't be any overflow so long as the microsecond difference fits into 31 bits, i.e. so long as usec is less than ~36 minutes. I doubt anything is calling __udelay() with that large of a value. Perhaps the issue this patch fixes is in get_timer_us(0) instead, or something else changed as a side-effect?

Quoted 3 lines

The generic implementation caters for full 32-bit range, that's all. Since the argument of this function is unsigned, it can overflow if you use argument which is bigger than 31 bits. OK like that ?

Sorry, I still don't understand. Both the __udelay() here and in lib/time.c take an unsigned long argument. I don't see how switching one out for the other can affect anything if the argument type is the issue.

Besides, what's passing a value >~36 minutes to udelay()?

Quoted 33 lines · Stephen Warren

Quoted 31 lines · Marek Vasut

Quoted 18 lines · Stephen Warren

Quoted 3 lines · Marek Vasut

Switch to generic timer implementation from lib/time.c . This also fixes a signed overflow which was in __udelay() implementation.

Can you explain that a bit more?

Quoted 11 lines

arch/arm/mach-bcm283x/timer.c deleted +0 -11

-void __udelay(unsigned long usec)
-{
-	ulong endtime;
-	signed long diff;
-
-	endtime = get_timer_us(0) + usec;
-
-	do {
-		ulong now = get_timer_us(0);
-		diff = endtime - now;
-	} while (diff >= 0);

Quoted 8 lines

Quoted 1 line

-}

I believe since endtime and now hold micro seconds, there shouldn't be any overflow so long as the microsecond difference fits into 31 bits, i.e. so long as usec is less than ~36 minutes. I doubt anything is calling __udelay() with that large of a value. Perhaps the issue this patch fixes is in get_timer_us(0) instead, or something else changed as a side-effect?

The generic implementation caters for full 32-bit range, that's all. Since the argument of this function is unsigned, it can overflow if you use argument which is bigger than 31 bits. OK like that ?

Quoted 3 lines

Sorry, I still don't understand. Both the __udelay() here and in lib/time.c take an unsigned long argument. I don't see how switching one out for the other can affect anything if the argument type is the issue.

So, if now is close to 0x7fffffff (which it can), then if endtime is big-ish, diff will become negative and this udelay() will not perform the correct delay, right ?

Quoted 1 line

Besides, what's passing a value >~36 minutes to udelay()?

Nothing, but that doesn't mean we can have a possibly broken implementation, right ?

Quoted 34 lines · Marek Vasut

Quoted 33 lines · Stephen Warren

Quoted 31 lines · Marek Vasut

Quoted 18 lines · Stephen Warren

Quoted 3 lines · Marek Vasut

Switch to generic timer implementation from lib/time.c . This also fixes a signed overflow which was in __udelay() implementation.

Can you explain that a bit more?

Quoted 11 lines

arch/arm/mach-bcm283x/timer.c deleted +0 -11

-void __udelay(unsigned long usec)
-{
-	ulong endtime;
-	signed long diff;
-
-	endtime = get_timer_us(0) + usec;
-
-	do {
-		ulong now = get_timer_us(0);
-		diff = endtime - now;
-	} while (diff >= 0);

Quoted 8 lines

Quoted 1 line

-}

I believe since endtime and now hold micro seconds, there shouldn't be any overflow so long as the microsecond difference fits into 31 bits, i.e. so long as usec is less than ~36 minutes. I doubt anything is calling __udelay() with that large of a value. Perhaps the issue this patch fixes is in get_timer_us(0) instead, or something else changed as a side-effect?

The generic implementation caters for full 32-bit range, that's all. Since the argument of this function is unsigned, it can overflow if you use argument which is bigger than 31 bits. OK like that ?

Quoted 7 lines

Quoted 3 lines

Sorry, I still don't understand. Both the __udelay() here and in lib/time.c take an unsigned long argument. I don't see how switching one out for the other can affect anything if the argument type is the issue.

So, if now is close to 0x7fffffff (which it can), then if endtime is big-ish, diff will become negative and this udelay() will not perform the correct delay, right ?

I don't believe so, no.

endtime and now are both unsigned. My (admittedly intuitive rather than well-researched) understanding of C math promotion rules means that "endtime - now" will be calculated as an unsigned value, then converted into a signed value to be stored in the signed diff. As such, I would expect the value of diff to be a small value in this case. I wrote a test program to validate this; endtime = 0x80000002, now = 0x7ffffffe, yields diff=4 as expected.

Perhaps you meant a much larger endtime value than 0x80000002; perhaps 0xffffffff? This doesn't cause issues either. All that's relevant is the difference between endtime and now, not their absolute values, and not whether endtime has wrapped but now has or hasn't. For example, endtime = 0x00000002, now = 0xfffffff0 yields diff=18 as expected.

Quoted 4 lines

Quoted 1 line

Besides, what's passing a value >~36 minutes to udelay()?

Nothing, but that doesn't mean we can have a possibly broken implementation, right ?

True. However, I'd expect that any specification for udelay would disallow such large parameter values, and hence its behaviour wouldn't be relevant if such values were passed.

Quoted 53 lines · Stephen Warren

Quoted 34 lines · Marek Vasut

Quoted 33 lines · Stephen Warren

Quoted 31 lines · Marek Vasut

Quoted 18 lines · Stephen Warren

Quoted 3 lines · Marek Vasut

Switch to generic timer implementation from lib/time.c . This also fixes a signed overflow which was in __udelay() implementation.

Can you explain that a bit more?

Quoted 11 lines

(quoted diff) +0 -11

-void __udelay(unsigned long usec)
-{
-	ulong endtime;
-	signed long diff;
-
-	endtime = get_timer_us(0) + usec;
-
-	do {
-		ulong now = get_timer_us(0);
-		diff = endtime - now;
-	} while (diff >= 0);

Quoted 8 lines

Quoted 1 line

-}

I believe since endtime and now hold micro seconds, there shouldn't be any overflow so long as the microsecond difference fits into 31 bits, i.e. so long as usec is less than ~36 minutes. I doubt anything is calling __udelay() with that large of a value. Perhaps the issue this patch fixes is in get_timer_us(0) instead, or something else changed as a side-effect?

The generic implementation caters for full 32-bit range, that's all. Since the argument of this function is unsigned, it can overflow if you use argument which is bigger than 31 bits. OK like that ?

Quoted 7 lines

Quoted 3 lines

Sorry, I still don't understand. Both the __udelay() here and in lib/time.c take an unsigned long argument. I don't see how switching one out for the other can affect anything if the argument type is the issue.

So, if now is close to 0x7fffffff (which it can), then if endtime is big-ish, diff will become negative and this udelay() will not perform the correct delay, right ?

I don't believe so, no.

endtime and now are both unsigned. My (admittedly intuitive rather than well-researched) understanding of C math promotion rules means that "endtime - now" will be calculated as an unsigned value, then converted into a signed value to be stored in the signed diff. As such, I would expect the value of diff to be a small value in this case. I wrote a test program to validate this; endtime = 0x80000002, now = 0x7ffffffe, yields diff=4 as expected.

Quoted 5 lines

Perhaps you meant a much larger endtime value than 0x80000002; perhaps 0xffffffff? This doesn't cause issues either. All that's relevant is the difference between endtime and now, not their absolute values, and not whether endtime has wrapped but now has or hasn't. For example, endtime = 0x00000002, now = 0xfffffff0 yields diff=18 as expected.

So what if the difference is bigger than 1 << 31 ?

Quoted 8 lines

Quoted 4 lines

Quoted 1 line

Besides, what's passing a value >~36 minutes to udelay()?

Nothing, but that doesn't mean we can have a possibly broken implementation, right ?

True. However, I'd expect that any specification for udelay would disallow such large parameter values, and hence its behaviour wouldn't be relevant if such values were passed.

Do you think you can pick this patch and drop the "fixes overflow" part or do you need resubmission ?

Quoted 20 lines

Quoted 19 lines

Quoted 3 lines

So, if now is close to 0x7fffffff (which it can), then if endtime is big-ish, diff will become negative and this udelay() will not perform the correct delay, right ?

I don't believe so, no.

endtime and now are both unsigned. My (admittedly intuitive rather than well-researched) understanding of C math promotion rules means that "endtime - now" will be calculated as an unsigned value, then converted into a signed value to be stored in the signed diff. As such, I would expect the value of diff to be a small value in this case. I wrote a test program to validate this; endtime = 0x80000002, now = 0x7ffffffe, yields diff=4 as expected.

Perhaps you meant a much larger endtime value than 0x80000002; perhaps 0xffffffff? This doesn't cause issues either. All that's relevant is the difference between endtime and now, not their absolute values, and not whether endtime has wrapped but now has or hasn't. For example, endtime = 0x00000002, now = 0xfffffff0 yields diff=18 as expected.

Quoted 1 line

So what if the difference is bigger than 1 << 31 ?

As I said, I don't believe that case is relevant; it can only happen if passing ridiculously large delay values into __udelay() (i.e. greater than the 1<<31value you mention), and I don't believe there's any need to support that.

The implementation in lib/time.c probably has exactly the same problem, except that since it uses 64-bit math rather than 32-bit math, so the issue happens at 1<<63 rather than 1<<31. It's probably equally problematic for delay values as large as 1<<63:-) In practice, given 1<<31 us is so large, I don't think there's any practical difference.

Quoted 11 lines

Quoted 8 lines

Quoted 4 lines

Quoted 1 line

Besides, what's passing a value >~36 minutes to udelay()?

Nothing, but that doesn't mean we can have a possibly broken implementation, right ?

True. However, I'd expect that any specification for udelay would disallow such large parameter values, and hence its behaviour wouldn't be relevant if such values were passed.

Do you think you can pick this patch and drop the "fixes overflow" part or do you need resubmission ?

Tom Rini (or in the past Albert Aribaud) actually apply the patches.

Re: the patch description: I'd certainly be happy if it was re-written to say something more like "replace bcm2835-specific timer logic with common code to reduce the number of different implementations for the same thing".

I think you'd mentioned on IRC that this change fixed something USB-related for you, and I still don't understand how that could be possible. Perhaps there's some intermittent problem, and it just happened not to show up when you tested after this patch?

On Wednesday, May 06, 2015 at 05:52:37 PM, Stephen Warren wrote:
[...]

Quoted 20 lines

Quoted 20 lines

Quoted 19 lines

Quoted 3 lines

So, if now is close to 0x7fffffff (which it can), then if endtime is big-ish, diff will become negative and this udelay() will not perform the correct delay, right ?

I don't believe so, no.

endtime and now are both unsigned. My (admittedly intuitive rather than well-researched) understanding of C math promotion rules means that "endtime - now" will be calculated as an unsigned value, then converted into a signed value to be stored in the signed diff. As such, I would expect the value of diff to be a small value in this case. I wrote a test program to validate this; endtime = 0x80000002, now = 0x7ffffffe, yields diff=4 as expected.

Perhaps you meant a much larger endtime value than 0x80000002; perhaps 0xffffffff? This doesn't cause issues either. All that's relevant is the difference between endtime and now, not their absolute values, and not whether endtime has wrapped but now has or hasn't. For example, endtime = 0x00000002, now = 0xfffffff0 yields diff=18 as expected.

Quoted 6 lines

Quoted 1 line

So what if the difference is bigger than 1 << 31 ?

As I said, I don't believe that case is relevant; it can only happen if passing ridiculously large delay values into __udelay() (i.e. greater than the 1<<31value you mention), and I don't believe there's any need to support that.

So what you say is that it's OK to have a function which is buggy in corner cases ?

Quoted 5 lines

The implementation in lib/time.c probably has exactly the same problem, except that since it uses 64-bit math rather than 32-bit math, so the issue happens at 1<<63 rather than 1<<31. It's probably equally problematic for delay values as large as 1<<63:-) In practice, given 1<<31 us is so large, I don't think there's any practical difference.

The implementation in lib/time.c uses 32bit usec argument though, so it's not prone to this overflow. Please correct me if I'm wrong.

Quoted 12 lines

Quoted 11 lines

Quoted 8 lines

Quoted 4 lines

Quoted 1 line

Besides, what's passing a value >~36 minutes to udelay()?

Nothing, but that doesn't mean we can have a possibly broken implementation, right ?

True. However, I'd expect that any specification for udelay would disallow such large parameter values, and hence its behaviour wouldn't be relevant if such values were passed.

Do you think you can pick this patch and drop the "fixes overflow" part or do you need resubmission ?

Quoted 6 lines

Tom Rini (or in the past Albert Aribaud) actually apply the patches.

Re: the patch description: I'd certainly be happy if it was re-written to say something more like "replace bcm2835-specific timer logic with common code to reduce the number of different implementations for the same thing".

Tom, do you want a repost ?

Quoted 4 lines

I think you'd mentioned on IRC that this change fixed something USB-related for you, and I still don't understand how that could be possible. Perhaps there's some intermittent problem, and it just happened not to show up when you tested after this patch?

I think Tyler can elaborate on that, but in his test case, he still triggers the USB issue.

Quoted 62 lines · Marek Vasut

On Wednesday, May 06, 2015 at 05:52:37 PM, Stephen Warren wrote:
[...]

Quoted 20 lines

Quoted 20 lines

Quoted 19 lines

Quoted 3 lines

So, if now is close to 0x7fffffff (which it can), then if endtime is big-ish, diff will become negative and this udelay() will not perform the correct delay, right ?

I don't believe so, no.

endtime and now are both unsigned. My (admittedly intuitive rather than well-researched) understanding of C math promotion rules means that "endtime - now" will be calculated as an unsigned value, then converted into a signed value to be stored in the signed diff. As such, I would expect the value of diff to be a small value in this case. I wrote a test program to validate this; endtime = 0x80000002, now = 0x7ffffffe, yields diff=4 as expected.

Perhaps you meant a much larger endtime value than 0x80000002; perhaps 0xffffffff? This doesn't cause issues either. All that's relevant is the difference between endtime and now, not their absolute values, and not whether endtime has wrapped but now has or hasn't. For example, endtime = 0x00000002, now = 0xfffffff0 yields diff=18 as expected.

Quoted 6 lines

Quoted 1 line

So what if the difference is bigger than 1 << 31 ?

As I said, I don't believe that case is relevant; it can only happen if passing ridiculously large delay values into __udelay() (i.e. greater than the 1<<31value you mention), and I don't believe there's any need to support that.

So what you say is that it's OK to have a function which is buggy in corner cases ?

Quoted 5 lines

The implementation in lib/time.c probably has exactly the same problem, except that since it uses 64-bit math rather than 32-bit math, so the issue happens at 1<<63 rather than 1<<31. It's probably equally problematic for delay values as large as 1<<63:-) In practice, given 1<<31 us is so large, I don't think there's any practical difference.

The implementation in lib/time.c uses 32bit usec argument though, so it's not prone to this overflow. Please correct me if I'm wrong.

Quoted 12 lines

Quoted 11 lines

Quoted 8 lines

Quoted 4 lines

Quoted 1 line

Besides, what's passing a value >~36 minutes to udelay()?

Nothing, but that doesn't mean we can have a possibly broken implementation, right ?

True. However, I'd expect that any specification for udelay would disallow such large parameter values, and hence its behaviour wouldn't be relevant if such values were passed.

Do you think you can pick this patch and drop the "fixes overflow" part or do you need resubmission ?

Quoted 6 lines

Tom Rini (or in the past Albert Aribaud) actually apply the patches.

Re: the patch description: I'd certainly be happy if it was re-written to say something more like "replace bcm2835-specific timer logic with common code to reduce the number of different implementations for the same thing".

Tom, do you want a repost ?

Quoted 7 lines

Quoted 4 lines

I think you'd mentioned on IRC that this change fixed something USB-related for you, and I still don't understand how that could be possible. Perhaps there's some intermittent problem, and it just happened not to show up when you tested after this patch?

I think Tyler can elaborate on that, but in his test case, he still triggers the USB issue.

I'll provide some context on the issue I'm fighting...

I recently bought a RPi B+ Model, flashed the latest raspbian image[0] to an sd card, built the master branch (v2015.05+) u-boot and overwrote kernel.img with u-boot.bin. U-Boot came right up but I was unable to obtain a DHCP lease after using 'usb start; dhcp'. I ran tcpdump and saw the DHCP requests being made, but shortly after the board was seemly ignoring responses from the DHCP server. Now sometimes if the response from the DHCP server was quick enough, it could get a lease, but the tftp transfer would stall. I decided it would be best to set 'ipaddr, gateway, netmask' to see if this was a DHCP issue. It turned out that I had no network connectivity even when I configured the ip address statically.

Could not obtain a lease:

starting USB...
USB0: Core Release: 2.80a
scanning bus 0 for devices... 3 USB Device(s) found scanning usb for storage devices... 0 Storage Device(s) found scanning usb for ethernet devices... 1 Ethernet Device(s) found Waiting for Ethernet connection... done.
BOOTP broadcast 1
BOOTP broadcast 2
BOOTP broadcast 3
BOOTP broadcast 4
BOOTP broadcast 5
BOOTP broadcast 6
BOOTP broadcast 7
BOOTP broadcast 8
Retry time exceeded; starting again
Bad Linux ARM zImage magic!

Obtains a lease, but stalls on transfer:

starting USB...
USB0: Core Release: 2.80a
scanning bus 0 for devices... 3 USB Device(s) found scanning usb for storage devices... 0 Storage Device(s) found scanning usb for ethernet devices... 1 Ethernet Device(s) found Waiting for Ethernet connection... done.
BOOTP broadcast 1
DHCP client bound to address 192.168.2.55 (1328 ms) Waiting for Ethernet connection... done.
Using sms0 device
TFTP from server 192.168.2.2; our IP address is 192.168.2.55 Filename 'tmp2rkX_N/.zImage'.
Load address: 0x1000000
Loading: * T T T T T T T T T T
Retry count exceeded; starting again
Bad Linux ARM zImage magic!

At this point I reached out for help on IRC and that is when Marek and I starting chatting about this. Hacking around, I found that using v2015.01 I was able to obtain a lease and transfer files 100% of the time (although it seems very slow). Thinking that we have a good and bad commit, this would be easy to bisect. Wrong, we both tried and got different results. It seems that as you move from v2015.01 -> HEAD master this issue becomes very intermittent and thus hard to pin down.

So my test case for this issue has become...

* Obtain lease
* Transfer kernel, dtb, ramdisk without stalling/timing out
* Do this 10 times in a row with a power cycle in between

Hope this help clarify the situation in some way,

Quoted 5 lines

* Obtain lease
* Transfer kernel, dtb, ramdisk without stalling/timing out
* Do this 10 times in a row with a power cycle in between

Hope this help clarify the situation in some way,

OK, but if you apply Marek's change to replace the timer implementation with the one in lib/time.c, does that reliably fix the issue when tested over a large number of runs with/without that change? With the current explanation, I can't see how it possibly could. Equally, I can't see why the move from v2015.01 to HEAD would affect the issue if it was caused by the timer implementation.

My suspicion is that there's something else entirely going on.

On Wednesday, May 06, 2015 at 05:52:37 PM, Stephen Warren wrote:
[...]

Quoted 22 lines

Quoted 21 lines

Quoted 14 lines

Quoted 3 lines

So, if now is close to 0x7fffffff (which it can), then if endtime is big-ish, diff will become negative and this udelay() will not perform the correct delay, right ?

I don't believe so, no.

endtime and now are both unsigned. My (admittedly intuitive rather than well-researched) understanding of C math promotion rules means that "endtime - now" will be calculated as an unsigned value, then converted into a signed value to be stored in the signed diff. As such, I would expect the value of diff to be a small value in this case. I wrote a test program to validate this; endtime = 0x80000002, now = 0x7ffffffe, yields diff=4 as expected.

Quoted 5 lines

Perhaps you meant a much larger endtime value than 0x80000002; perhaps 0xffffffff? This doesn't cause issues either. All that's relevant is the difference between endtime and now, not their absolute values, and not whether endtime has wrapped but now has or hasn't. For example, endtime = 0x00000002, now = 0xfffffff0 yields diff=18 as expected.

So what if the difference is bigger than 1 << 31 ?

Quoted 4 lines

As I said, I don't believe that case is relevant; it can only happen if passing ridiculously large delay values into __udelay() (i.e. greater than the 1<<31value you mention), and I don't believe there's any need to support that.

So what you say is that it's OK to have a function which is buggy in corner cases ?

Quoted 26 lines · Marek Vasut

On Wednesday, May 06, 2015 at 05:52:37 PM, Stephen Warren wrote:
[...]

Quoted 23 lines

Quoted 22 lines

Quoted 14 lines

Quoted 3 lines

So, if now is close to 0x7fffffff (which it can), then if endtime is big-ish, diff will become negative and this udelay() will not perform the correct delay, right ?

I don't believe so, no.

endtime and now are both unsigned. My (admittedly intuitive rather than well-researched) understanding of C math promotion rules means that "endtime - now" will be calculated as an unsigned value, then converted into a signed value to be stored in the signed diff. As such, I would expect the value of diff to be a small value in this case. I wrote a test program to validate this; endtime = 0x80000002, now = 0x7ffffffe, yields diff=4 as expected.

Quoted 6 lines

Perhaps you meant a much larger endtime value than 0x80000002; perhaps 0xffffffff? This doesn't cause issues either. All that's relevant is the difference between endtime and now, not their absolute values, and not whether endtime has wrapped but now has or hasn't. For example, endtime = 0x00000002, now = 0xfffffff0 yields diff=18 as expected.

So what if the difference is bigger than 1 << 31 ?

Quoted 7 lines

Quoted 4 lines

As I said, I don't believe that case is relevant; it can only happen if passing ridiculously large delay values into __udelay() (i.e. greater than the 1<<31value you mention), and I don't believe there's any need to support that.

So what you say is that it's OK to have a function which is buggy in corner cases ?

A corner case (something that's within spec but perhaps hard/unusual) should not be buggy.

The behaviour of something outside spec isn't relevant; it's actively not specified.

I suppose there is no specification of what range of values this function is supposed to accept. I'd argue we should create one, and that spec should likely limit the range to much less than the 32-bit parameter can actually hold, since some HW timer implementations may have well less than 32-bits of range.

Quoted 41 lines · Stephen Warren

Quoted 26 lines · Marek Vasut

On Wednesday, May 06, 2015 at 05:52:37 PM, Stephen Warren wrote:
[...]

Quoted 23 lines

Quoted 22 lines

Quoted 14 lines

Quoted 3 lines

So, if now is close to 0x7fffffff (which it can), then if endtime is big-ish, diff will become negative and this udelay() will not perform the correct delay, right ?

I don't believe so, no.

endtime and now are both unsigned. My (admittedly intuitive rather than well-researched) understanding of C math promotion rules means that "endtime - now" will be calculated as an unsigned value, then converted into a signed value to be stored in the signed diff. As such, I would expect the value of diff to be a small value in this case. I wrote a test program to validate this; endtime = 0x80000002, now = 0x7ffffffe, yields diff=4 as expected.

Quoted 6 lines

Perhaps you meant a much larger endtime value than 0x80000002; perhaps 0xffffffff? This doesn't cause issues either. All that's relevant is the difference between endtime and now, not their absolute values, and not whether endtime has wrapped but now has or hasn't. For example, endtime = 0x00000002, now = 0xfffffff0 yields diff=18 as expected.

So what if the difference is bigger than 1 << 31 ?

Quoted 7 lines

Quoted 4 lines

As I said, I don't believe that case is relevant; it can only happen if passing ridiculously large delay values into __udelay() (i.e. greater than the 1<<31value you mention), and I don't believe there's any need to support that.

So what you say is that it's OK to have a function which is buggy in corner cases ?

A corner case (something that's within spec but perhaps hard/unusual) should not be buggy.

The behaviour of something outside spec isn't relevant; it's actively not specified.

Quoted 5 lines

I suppose there is no specification of what range of values this function is supposed to accept. I'd argue we should create one, and that spec should likely limit the range to much less than the 32-bit parameter can actually hold, since some HW timer implementations may have well less than 32-bits of range.

Maybe we should just accept this patch and be done with it? It's clearly and improvement which migrates away from old timer code to generic timer.

Quoted 48 lines · Marek Vasut

Quoted 46 lines · Stephen Warren

Quoted 26 lines · Marek Vasut

On Wednesday, May 06, 2015 at 05:52:37 PM, Stephen Warren wrote:
[...]

Quoted 23 lines

Quoted 22 lines

Quoted 14 lines

Quoted 3 lines

So, if now is close to 0x7fffffff (which it can), then if endtime is big-ish, diff will become negative and this udelay() will not perform the correct delay, right ?

I don't believe so, no.

endtime and now are both unsigned. My (admittedly intuitive rather than well-researched) understanding of C math promotion rules means that "endtime - now" will be calculated as an unsigned value, then converted into a signed value to be stored in the signed diff. As such, I would expect the value of diff to be a small value in this case. I wrote a test program to validate this; endtime = 0x80000002, now = 0x7ffffffe, yields diff=4 as expected.

Quoted 6 lines

Perhaps you meant a much larger endtime value than 0x80000002; perhaps 0xffffffff? This doesn't cause issues either. All that's relevant is the difference between endtime and now, not their absolute values, and not whether endtime has wrapped but now has or hasn't. For example, endtime = 0x00000002, now = 0xfffffff0 yields diff=18 as expected.

So what if the difference is bigger than 1 << 31 ?

Quoted 7 lines

Quoted 4 lines

As I said, I don't believe that case is relevant; it can only happen if passing ridiculously large delay values into __udelay() (i.e. greater than the 1<<31value you mention), and I don't believe there's any need to support that.

So what you say is that it's OK to have a function which is buggy in corner cases ?

A corner case (something that's within spec but perhaps hard/unusual) should not be buggy.

The behaviour of something outside spec isn't relevant; it's actively not specified.

I suppose there is no specification of what range of values this function is supposed to accept. I'd argue we should create one, and that spec should likely limit the range to much less than the 32-bit parameter can actually hold, since some HW timer implementations may have well less than 32-bits of range.

Quoted 2 lines

Maybe we should just accept this patch and be done with it? It's clearly and improvement which migrates away from old timer code to generic timer.

The code change is fine. I have no issues with that.

I just don't think the patch description is appropriate, since the version in lib/time.c has exactly the same overflow issue (albeit with a 64-bit type rather than a 32-bit type).

Switch to generic timer implementation from lib/time.c . This also fixes a signed overflow which was in __udelay() implementation.

Signed-off-by Marek Vasut <[email protected]>

Cc Stephen Warren <[email protected]>

Cc Tyler Baker <[email protected]>

Acked-by Stephen Warren <[email protected]>

Trivially fix the include check in wdog.h.

Signed-off-by Marek Vasut <[email protected]>

Cc Stephen Warren <[email protected]>

Cc Tyler Baker <[email protected]>