Re: [PATCH v2 0/7] Add support for cyclic function execution infrastruture

[Heinrich Schuchardt <xypron.glpk@gmx.de>]

On 7/28/22 09:09, Stefan Roese wrote:
> This patchset adds the basic infrastructure to periodically execute
> code, e.g. all 100ms. Examples for such functions might be LED blinking
> etc. The functions that are hooked into this cyclic list should be
> small timewise as otherwise the execution of the other code that relies
> on a high frequent polling (e.g. UART rx char ready check) might be
> delayed too much. This patch also adds the Kconfig option
> CONFIG_CYCLIC_MAX_CPU_TIME_US, which configures the max allowed time
> for such a cyclic function. If it's execution time exceeds this time,
> this cyclic function will get removed from the cyclic list.
>
> How is this cyclic functionality executed?
> This patchset integrates the main function responsible for calling all
> registered cyclic functions cyclic_run() into the common WATCHDOG_RESET
> macro. This guarantees that cyclic_run() is executed very often, which
> is necessary for the cyclic functions to get scheduled and executed at
> their configured periods.
>
> This cyclic infrastructure will be used by a board specific function on
> the NIC23 MIPS Octeon board, which needs to check periodically, if a
> PCIe FLR has occurred.
>
> Ideas how to continue:
> One idea is to rename WATCHDOG_RESET to something like SCHEDULE and
> move the watchdog_reset call into this cyclic infrastructure as well.
> Or to perhaps move the shell UART RX ready polling to a cyclic
> function.

Hello Stefan

I am missing rework defining where WATCHDOG_RESET has to be called:

WATCHDOG_RESET is called in net_loop() but not in eth_rx() and eth_tx().
This is clearly the wrong place as not all network traffic uses net_loop().

Same is true for your reference to UART rx. WATCHDOG_RESET is called in
individual UART drivers. But this does not help if tstc() is calling
usb_kbd_testc().

So before adding this patchset please provide the concept defining where
WATCHDOG_RESET should be invoked.

A framework like the one proposed requires documentation. This needs to
be an integral part of the next version of the series.

With the infrastructure you are building efi_timer_check() is a
candidate for refactoring. Currently efi_timer_check() is called
whenever the EFI sub-system is waiting for anything (keyboard input,
polling events, network I/O).

Best regards

Heinrich

>
> It's also possible to extend the "cyclic" command, to support the
> creation of periodically executed shell commands (for testing etc).
>
> Here the Azure build, without any issues:
> https://dev.azure.com/sr0718/u-boot/_build/results?buildId=219&view=results
>
> Thanks,
> Stefan
>
> Aaron Williams (1):
>    mips: octeon_nic23: Add PCIe FLR fixup via cyclic infrastructure
>
> Stefan Roese (6):
>    time: Import time_after64() and friends from Linux
>    cyclic: Add basic support for cyclic function execution infrastruture
>    cyclic: Integrate cyclic infrastructure into WATCHDOG_RESET
>    cyclic: Integrate cyclic functionality at bootup in board_r/f
>    cyclic: Add 'cyclic list' command
>    sandbox: Add cyclic demo function
>
>   MAINTAINERS                        |   7 +
>   board/Marvell/octeon_nic23/board.c | 197 +++++++++++++++++++++++++++++
>   board/sandbox/sandbox.c            |  15 +++
>   cmd/Kconfig                        |   7 +
>   cmd/Makefile                       |   1 +
>   cmd/cyclic.c                       |  40 ++++++
>   common/Kconfig                     |  20 +++
>   common/Makefile                    |   1 +
>   common/board_f.c                   |   2 +
>   common/board_r.c                   |   2 +
>   common/cyclic.c                    | 112 ++++++++++++++++
>   configs/octeon_nic23_defconfig     |   3 +
>   configs/sandbox_defconfig          |   3 +
>   fs/cramfs/uncompress.c             |   2 +-
>   include/cyclic.h                   |  97 ++++++++++++++
>   include/time.h                     |  19 +++
>   include/watchdog.h                 |  23 +++-
>   17 files changed, 547 insertions(+), 4 deletions(-)
>   create mode 100644 cmd/cyclic.c
>   create mode 100644 common/cyclic.c
>   create mode 100644 include/cyclic.h
>