On 28/10/2022 16.10, Stefan Roese wrote: > On 28.10.22 13:50, Rasmus Villemoes wrote:
>> As for cyclic_uninit(), it was never really the opposite of >> cyclic_init() since it didn't free the struct cyclic_drv nor set >> gd->cyclic to NULL. Rename it to cyclic_unregister_all() and use that >> in test/, and also insert a call at the end of the board_init_f >> sequence so that gd->cyclic_list is a fresh empty list before we enter >> board_init_r(). > > While reviewing the code, this was the only thing I wanted to > ask about. Now with this explanation it makes perfect sense. > Perhaps a small comment with this reasoning in the code here in > board_init_r would be helpful. Yeah, so I went back and forth on whether to put it early in board_init_r or late in board_init_f, but went with the latter so that whatever free() gets called goes with the same malloc() - i.e. to avoid introducing any new ordering dependency against when we can initialize the full malloc. Perhaps something like this above the cyclic_unregister_all entry in board_init_f sequence: /* * Deregister all cyclic functions before relocation, so that gd->cyclic_list does not contain any references to pre-relocation devices. Drivers will register their cyclic functions anew when the devices are probed again. This should happen as late as possible so that the window where a watchdog device is not serviced is as small as possible. */ But I don't know if that's too verbose; many other important initialization functions with implicit ordering dependencies do not have anything similar. That's not necessarily an argument against starting to add such comments. > Reviewed-by: Stefan Roese <s...@denx.de> > Tested-by: Stefan Roese <s...@denx.de> Thanks, Rasmus