Hi Sean, On Tue, 8 Sep 2020 at 17:59, Sean Anderson <sean...@gmail.com> wrote: > > On 9/8/20 7:56 PM, Simon Glass wrote: > > Hi Sean, > > > > On Mon, 7 Sep 2020 at 09:51, Sean Anderson <sean...@gmail.com> wrote: > >> > >> On 9/7/20 9:57 AM, Simon Glass wrote: > >>> Hi Sean, > >>> > >>> On Sun, 6 Sep 2020 at 20:02, Sean Anderson <sean...@gmail.com> wrote: > >>>> > >>>> On 9/6/20 9:43 PM, Simon Glass wrote: > >>>>> Hi Sean, > >>>>> > >>>>> On Tue, 1 Sep 2020 at 13:56, Sean Anderson <sean...@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 changes get_ticks to always return 0 when there is an error. > >>>>>> 0 is the least unsigned integer, ensuring get_ticks appears monotonic. > >>>>>> This > >>>>>> has the side effect of time apparently not passing until the timer is > >>>>>> initialized. However, without this patch, time does not pass anyway, > >>>>>> because the error value is likely to be the same. > >>>>>> > >>>>>> Fixes: c8a7ba9e6a5 > >>>>>> Signed-off-by: Sean Anderson <sean...@gmail.com> > >>>>>> --- > >>>>>> > >>>>>> lib/time.c | 4 ++-- > >>>>>> 1 file changed, 2 insertions(+), 2 deletions(-) > >>>>> > >>>>> Would it be better to panic so people can fix the bug? > >>>> > >>>> I thought this was expected behavior. It's only a bug if you do > >>>> something like udelay before any timers are created. We just can't > >>>> report errors through get_ticks, because its users assume that it always > >>>> returns a time of some kind. > >>> > >>> I think it indicates a bug. If you use a device before it is ready you > >>> don't really know what it will do. I worry that this patch is just > >>> going to cause confusion, since the behaviour depends on when you call > >>> it. If we panic, people can figure out why the timer is being inited > >>> too late, or being used too early. > >> > >> Hm, maybe. I don't think it's as clear cut as "us[ing] a device before > >> it is ready," because get_ticks tries to initialize the timer if it > >> isn't already initialized. Unless someone else does it first, the first > >> call to get_ticks will always be before the timer is initialized. > >> > >> The specific problem I ran into was that after relocation, the watchdog > >> may be initialized before the timer. This occurs on RISC-V because > >> without [1] a timer only exists after arch_early_init_r. So, for the > >> first few calls to watchdog_reset there is no timer. > >> > >> The second return could probably be turned into a panic. I checked, and > >> all current timer drivers always succeed in getting the time (except for > >> the RISC-V timer, which is fixed in [1]), so the only way for > >> timer_get_count to fail is if timer_ops.get_count doesn't exist. That is > >> almost certainly an error on the driver author's part, so I think > >> panicking there is the only reasonable option. > > > > OK good, let's do that and update docs in timer.h > > That being to panic both times, or just panic the second time?
Well I like a panic if the call is invalid, ie. in both cases. Regards, Simon
