[PATCH v2] time: Fix get_ticks being non-monotonic
get_ticks does not always succeed. Sometimes it can be called before the timer has been initialized. If it does, it returns a negative errno. This causes the timer to appear non-monotonic, because the value will become much smaller after the timer is initialized.
No users of get_ticks which I checked handle errors of this kind. Further, functions like tick_to_time mangle the result of get_ticks, making it very unlikely that one could check for an error without suggesting a patch such as this one.
This patch panics if we ever get an error. There are two cases in which this can occur. The first is if we couldn't find/probe the timer for some reason. One reason for this is if the timer is not available so early. This likely indicates misconfiguration. Another reason is that the timer has an invalid/missing device tree binding. In this case, panicing is also correct. The second case covers errors calling get_count. This can only occur if the timer is missing a get_count function (or on RISC-V, but that should be fixed soon).
Fixes: c8a7ba9e6a5 Signed-off-by: Sean Anderson seanga2@gmail.com ---
Changes in v2: - Panic on error
lib/time.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/lib/time.c b/lib/time.c index 47f8c84327..88bc50405f 100644 --- a/lib/time.c +++ b/lib/time.c @@ -91,13 +91,13 @@ uint64_t notrace get_ticks(void)
ret = dm_timer_init(); if (ret) - return ret; + panic("Could not initialize timer (err %d)\n", ret); #endif }
ret = timer_get_count(gd->timer, &count); if (ret) - return ret; + panic("Could not read count from timer (err %d)\n", ret);
return count; }
On Wed, 9 Sep 2020 at 14:25, Sean Anderson seanga2@gmail.com wrote:
get_ticks does not always succeed. Sometimes it can be called before the timer has been initialized. If it does, it returns a negative errno. This causes the timer to appear non-monotonic, because the value will become much smaller after the timer is initialized.
No users of get_ticks which I checked handle errors of this kind. Further, functions like tick_to_time mangle the result of get_ticks, making it very unlikely that one could check for an error without suggesting a patch such as this one.
This patch panics if we ever get an error. There are two cases in which this can occur. The first is if we couldn't find/probe the timer for some reason. One reason for this is if the timer is not available so early. This likely indicates misconfiguration. Another reason is that the timer has an invalid/missing device tree binding. In this case, panicing is also correct. The second case covers errors calling get_count. This can only occur if the timer is missing a get_count function (or on RISC-V, but that should be fixed soon).
Fixes: c8a7ba9e6a5 Signed-off-by: Sean Anderson seanga2@gmail.com
Changes in v2:
- Panic on error
lib/time.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
On Wed, Sep 09, 2020 at 04:24:56PM -0400, Sean Anderson wrote:
get_ticks does not always succeed. Sometimes it can be called before the timer has been initialized. If it does, it returns a negative errno. This causes the timer to appear non-monotonic, because the value will become much smaller after the timer is initialized.
No users of get_ticks which I checked handle errors of this kind. Further, functions like tick_to_time mangle the result of get_ticks, making it very unlikely that one could check for an error without suggesting a patch such as this one.
This patch panics if we ever get an error. There are two cases in which this can occur. The first is if we couldn't find/probe the timer for some reason. One reason for this is if the timer is not available so early. This likely indicates misconfiguration. Another reason is that the timer has an invalid/missing device tree binding. In this case, panicing is also correct. The second case covers errors calling get_count. This can only occur if the timer is missing a get_count function (or on RISC-V, but that should be fixed soon).
Fixes: c8a7ba9e6a5 Signed-off-by: Sean Anderson seanga2@gmail.com Reviewed-by: Simon Glass sjg@chromium.org
Applied to u-boot/master, thanks!
Hi,
Sorry, no messaging quoting, I was not subscribed to the list at that time.
Merging this change into master actually broke the SPL on Atmel/Microchip SAMA5D3, at least booting from MMC:
RomBOOT
<debug_uart> Could not initialize timer (err -11)
Could not initialize timer (err -11)
Could not initialize timer (err -11) ...
I'll look for a fix, but suggestions are welcome!
Cheers,
Michael.
Hi Michael,
On Thu, 19 Nov 2020 at 04:23, Michael Opdenacker michael.opdenacker@bootlin.com wrote:
Hi,
Sorry, no messaging quoting, I was not subscribed to the list at that time.
Merging this change into master actually broke the SPL on Atmel/Microchip SAMA5D3, at least booting from MMC:
RomBOOT
<debug_uart> Could not initialize timer (err -11)
Could not initialize timer (err -11)
Could not initialize timer (err -11) ...
I'll look for a fix, but suggestions are welcome!
The board might need CONFIG_TIMER_EARLY, but otherwise I think it is a case of figuring out why the timer is used before it is available.
You can use dm_dump_all() to print out available devices and whether they are probed.
Regards, Simon
Hi Simon,
I'm back working on this old issue. Thanks for your help with this!
So, SPL support for Atmel/Microchip SAMA5D3 is still broken in the latest mainline, at least for the Xplained board with the MMC configuration.
My comments and further questions below...
On 11/22/20 12:07 AM, Simon Glass wrote:
Hi Michael,
On Thu, 19 Nov 2020 at 04:23, Michael Opdenacker michael.opdenacker@bootlin.com wrote:
Hi,
Sorry, no messaging quoting, I was not subscribed to the list at that time.
Merging this change into master actually broke the SPL on Atmel/Microchip SAMA5D3, at least booting from MMC:
RomBOOT
<debug_uart> Could not initialize timer (err -11)
Could not initialize timer (err -11)
Could not initialize timer (err -11) ...
I'll look for a fix, but suggestions are welcome!
Now, it's:
Could not initialize timer (err -19) (-19 is -ENODEV /* No such device */)
The board might need CONFIG_TIMER_EARLY, but otherwise I think it is a case of figuring out why the timer is used before it is available.
I tried to enable CONFIG_TIMER_EARLY but it fails at link time:
LD u-boot arm-linux-ld.bfd: lib/built-in.o: in function `get_tbclk': /home/mike/work/git/git.denx.de/u-boot/lib/time.c:70: undefined reference to `timer_early_get_rate' arm-linux-ld.bfd: lib/built-in.o: in function `get_ticks': /home/mike/work/git/git.denx.de/u-boot/lib/time.c:90: undefined reference to `timer_early_get_count' make: *** [Makefile:1765: u-boot] Error 1
This is not a surprise, as according to https://elixir.bootlin.com/u-boot/latest/C/ident/timer_early_get_rate,%C2%A0 timer_early_get_rate() is not implemented on ARM, only on sandbox (drivers/timer/sandbox_timer.c) and on x86 (drivers/timer/tsc_timer.c).
So, I'm moving to your second suggestion...
You can use dm_dump_all() to print out available devices and whether they are probed.
Done, I added dm_dump_all to lib/time.c right before the panic() message:
Class Index Probed Driver Name ----------------------------------------------------------- root 0 [ + ] root_driver root_driver simple_bus 0 [ ] simple_bus `-- ahb simple_bus 1 [ ] simple_bus `-- apb mmc 0 [ ] atmel-mci |-- mmc@f0000000 blk 0 [ ] mmc_blk | `-- mmc@f0000000.blk mmc 1 [ ] atmel-mci |-- mmc@f8000000 blk 1 [ ] mmc_blk | `-- mmc@f8000000.blk serial 0 [ ] serial_atmel |-- serial@ffffee00 pinctrl 0 [ ] atmel_sama5d3_pinctrl |-- pinctrl@fffff200 pinconfig 0 [ ] pinconfig | |-- dbgu pinconfig 1 [ ] pinconfig | | `-- dbgu-0 pinconfig 2 [ ] pinconfig | |-- mmc0 pinconfig 3 [ ] pinconfig | | |-- mmc0_clk_cmd_dat0 pinconfig 4 [ ] pinconfig | | |-- mmc0_dat1_3 pinconfig 5 [ ] pinconfig | | `-- mmc0_dat4_7 pinconfig 6 [ ] pinconfig | |-- mmc1 pinconfig 7 [ ] pinconfig | | |-- mmc1_clk_cmd_dat0 pinconfig 8 [ ] pinconfig | | `-- mmc1_dat1_3 pinconfig 9 [ ] pinconfig | |-- spi0 pinconfig 10 [ ] pinconfig | | `-- spi0-0 pinconfig 11 [ ] pinconfig | |-- spi1 pinconfig 12 [ ] pinconfig | | `-- spi1-0 pinconfig 13 [ ] pinconfig | `-- board pinconfig 14 [ ] pinconfig | |-- mmc0_cd pinconfig 15 [ ] pinconfig | `-- mmc1_cd gpio 0 [ ] atmel_at91rm9200_gpio |-- gpio@fffff200 gpio 1 [ ] atmel_at91rm9200_gpio |-- gpio@fffff400 gpio 2 [ ] atmel_at91rm9200_gpio |-- gpio@fffff600 gpio 3 [ ] atmel_at91rm9200_gpio |-- gpio@fffff800 gpio 4 [ ] atmel_at91rm9200_gpio |-- gpio@fffffa00 simple_bus 2 [ ] at91-pmc `-- pmc@fffffc00 clk 0 [ ] at91sam9x5-utmi-clk |-- utmick clk 1 [ ] at91-master-clk |-- masterck misc 0 [ ] sam9x5-periph-clk `-- periphck clk 2 [ ] periph-clk |-- dbgu_clk@2 clk 3 [ ] periph-clk |-- pioA_clk@6 clk 4 [ ] periph-clk |-- pioB_clk@7 clk 5 [ ] periph-clk |-- pioC_clk@8 clk 6 [ ] periph-clk |-- pioD_clk@9 clk 7 [ ] periph-clk |-- pioE_clk@10 clk 8 [ ] periph-clk |-- mci0_clk@21 clk 9 [ ] periph-clk |-- mci1_clk@22 clk 10 [ ] periph-clk |-- spi0_clk@24 clk 11 [ ] periph-clk `-- spi1_clk@25 Could not initialize timer (err -19)
I'm not familiar enough with U-Boot yet (and with SAMA5D3 support either) to see why the timer device is missing here, but I hope our Microchip friends can give us further clues...
Thanks again,
Cheers,
Michael.
On 3/1/21 11:40 AM, Michael Opdenacker wrote:
Hi Simon,
I'm back working on this old issue. Thanks for your help with this!
So, SPL support for Atmel/Microchip SAMA5D3 is still broken in the latest mainline, at least for the Xplained board with the MMC configuration.
My comments and further questions below...
On 11/22/20 12:07 AM, Simon Glass wrote:
Hi Michael,
On Thu, 19 Nov 2020 at 04:23, Michael Opdenacker michael.opdenacker@bootlin.com wrote:
Hi,
Sorry, no messaging quoting, I was not subscribed to the list at that time.
Merging this change into master actually broke the SPL on Atmel/Microchip SAMA5D3, at least booting from MMC:
RomBOOT
<debug_uart> Could not initialize timer (err -11)
Could not initialize timer (err -11)
Could not initialize timer (err -11) ...
I'll look for a fix, but suggestions are welcome!
Now, it's:
Could not initialize timer (err -19) (-19 is -ENODEV /* No such device */)
The board might need CONFIG_TIMER_EARLY, but otherwise I think it is a case of figuring out why the timer is used before it is available.
I tried to enable CONFIG_TIMER_EARLY but it fails at link time:
LD u-boot arm-linux-ld.bfd: lib/built-in.o: in function `get_tbclk': /home/mike/work/git/git.denx.de/u-boot/lib/time.c:70: undefined reference to `timer_early_get_rate' arm-linux-ld.bfd: lib/built-in.o: in function `get_ticks': /home/mike/work/git/git.denx.de/u-boot/lib/time.c:90: undefined reference to `timer_early_get_count' make: *** [Makefile:1765: u-boot] Error 1
This is not a surprise, as according to https://elixir.bootlin.com/u-boot/latest/C/ident/timer_early_get_rate, timer_early_get_rate() is not implemented on ARM, only on sandbox (drivers/timer/sandbox_timer.c) and on x86 (drivers/timer/tsc_timer.c).
It is also available on most RISC-V boards, not that it helps :)
So, I'm moving to your second suggestion...
You can use dm_dump_all() to print out available devices and whether they are probed.
Done, I added dm_dump_all to lib/time.c right before the panic() message:
Class Index Probed Driver Name
root 0 [ + ] root_driver root_driver simple_bus 0 [ ] simple_bus `-- ahb simple_bus 1 [ ] simple_bus `-- apb mmc 0 [ ] atmel-mci |-- mmc@f0000000 blk 0 [ ] mmc_blk | `-- mmc@f0000000.blk mmc 1 [ ] atmel-mci |-- mmc@f8000000 blk 1 [ ] mmc_blk | `-- mmc@f8000000.blk serial 0 [ ] serial_atmel |-- serial@ffffee00 pinctrl 0 [ ] atmel_sama5d3_pinctrl |-- pinctrl@fffff200 pinconfig 0 [ ] pinconfig | |-- dbgu pinconfig 1 [ ] pinconfig | | `-- dbgu-0 pinconfig 2 [ ] pinconfig | |-- mmc0 pinconfig 3 [ ] pinconfig | | |-- mmc0_clk_cmd_dat0 pinconfig 4 [ ] pinconfig | | |-- mmc0_dat1_3 pinconfig 5 [ ] pinconfig | | `-- mmc0_dat4_7 pinconfig 6 [ ] pinconfig | |-- mmc1 pinconfig 7 [ ] pinconfig | | |-- mmc1_clk_cmd_dat0 pinconfig 8 [ ] pinconfig | | `-- mmc1_dat1_3 pinconfig 9 [ ] pinconfig | |-- spi0 pinconfig 10 [ ] pinconfig | | `-- spi0-0 pinconfig 11 [ ] pinconfig | |-- spi1 pinconfig 12 [ ] pinconfig | | `-- spi1-0 pinconfig 13 [ ] pinconfig | `-- board pinconfig 14 [ ] pinconfig | |-- mmc0_cd pinconfig 15 [ ] pinconfig | `-- mmc1_cd gpio 0 [ ] atmel_at91rm9200_gpio |-- gpio@fffff200 gpio 1 [ ] atmel_at91rm9200_gpio |-- gpio@fffff400 gpio 2 [ ] atmel_at91rm9200_gpio |-- gpio@fffff600 gpio 3 [ ] atmel_at91rm9200_gpio |-- gpio@fffff800 gpio 4 [ ] atmel_at91rm9200_gpio |-- gpio@fffffa00 simple_bus 2 [ ] at91-pmc `-- pmc@fffffc00 clk 0 [ ] at91sam9x5-utmi-clk |-- utmick clk 1 [ ] at91-master-clk |-- masterck misc 0 [ ] sam9x5-periph-clk `-- periphck clk 2 [ ] periph-clk |-- dbgu_clk@2 clk 3 [ ] periph-clk |-- pioA_clk@6 clk 4 [ ] periph-clk |-- pioB_clk@7 clk 5 [ ] periph-clk |-- pioC_clk@8 clk 6 [ ] periph-clk |-- pioD_clk@9 clk 7 [ ] periph-clk |-- pioE_clk@10 clk 8 [ ] periph-clk |-- mci0_clk@21 clk 9 [ ] periph-clk |-- mci1_clk@22 clk 10 [ ] periph-clk |-- spi0_clk@24 clk 11 [ ] periph-clk `-- spi1_clk@25 Could not initialize timer (err -19)
So nothing here is probed, but additionally nothing has UCLASS_TIMER. What do you expect the timer device to be?
--Sean
I'm not familiar enough with U-Boot yet (and with SAMA5D3 support either) to see why the timer device is missing here, but I hope our Microchip friends can give us further clues...
Thanks again,
Cheers,
Michael.
participants (4)
-
Michael Opdenacker -
Sean Anderson -
Simon Glass -
Tom Rini