[PATCH v2 1/5] net: lwip: call sys_check_timeouts and schedule on rx

[Jerome Forissier]

On 6/16/25 19:37, Tim Harvey wrote:
> On Mon, Jun 16, 2025 at 12:18 AM Jerome Forissier
> <jerome.forissier at linaro.org> wrote:
>>
>>
>>
>> On 6/9/25 19:35, Tim Harvey wrote:
>>> On Tue, Jun 3, 2025 at 5:04 PM Tim Harvey <tharvey at gateworks.com> wrote:
>>>>
>>>> On Sun, Jun 1, 2025 at 2:07 PM Heinrich Schuchardt
>>>> <heinrich.schuchardt at canonical.com> wrote:
>>>>>
>>>>> On 5/30/25 17:38, Tim Harvey wrote:
>>>>>> Call schedule() in net_lwip_rx() to service U-Boot tasks and
>>>>>> actions during packet rx.
>>>>>>
>>>>>> As a cleanup also move sys_check_timeouts() here and remove it from the
>>>>>> functions that call net_lwip_rx().
>>>>>>
>>>>>> This resolves the issue of an active watchdog resetting the board on
>>>>>> long network activities.
>>>>>>
>>>>>> Suggested-by: Jerome Forissier <jerome.forissier at linaro.org>
>>>>>> Signed-off-by: Tim Harvey <tharvey at gateworks.com>
>>>>>> ---
>>>>>> v2:
>>>>>>   - remove duplication of sys_check_timeouts() from functions that call
>>>>>>     net_lwip_rx()
>>>>>> ---
>>>>>>   net/lwip/dhcp.c     | 1 -
>>>>>>   net/lwip/dns.c      | 1 -
>>>>>>   net/lwip/net-lwip.c | 7 +++++++
>>>>>>   net/lwip/ping.c     | 1 -
>>>>>>   net/lwip/tftp.c     | 1 -
>>>>>>   net/lwip/wget.c     | 1 -
>>>>>>   6 files changed, 7 insertions(+), 5 deletions(-)
>>>>>>
>>>>>> diff --git a/net/lwip/dhcp.c b/net/lwip/dhcp.c
>>>>>> index 92bd7067a7fb..b79b746db390 100644
>>>>>> --- a/net/lwip/dhcp.c
>>>>>> +++ b/net/lwip/dhcp.c
>>>>>> @@ -57,7 +57,6 @@ static int dhcp_loop(struct udevice *udev)
>>>>>>       /* Wait for DHCP to complete */
>>>>>>       do {
>>>>>>               net_lwip_rx(udev, netif);
>>>>>> -             sys_check_timeouts();
>>>>>>               bound = dhcp_supplied_address(netif);
>>>>>>               if (bound)
>>>>>>                       break;
>>>>>> diff --git a/net/lwip/dns.c b/net/lwip/dns.c
>>>>>> index 19172ac959ac..738f8bf3196b 100644
>>>>>> --- a/net/lwip/dns.c
>>>>>> +++ b/net/lwip/dns.c
>>>>>> @@ -91,7 +91,6 @@ static int dns_loop(struct udevice *udev, const char *name, const char *var)
>>>>>>                       net_lwip_rx(udev, netif);
>>>>>>                       if (dns_cb_arg.done)
>>>>>>                               break;
>>>>>> -                     sys_check_timeouts();
>>>>>>                       if (ctrlc()) {
>>>>>>                               printf("\nAbort\n");
>>>>>>                               break;
>>>>>> diff --git a/net/lwip/net-lwip.c b/net/lwip/net-lwip.c
>>>>>> index f05c4cd3f64f..e8d4cc542ed8 100644
>>>>>> --- a/net/lwip/net-lwip.c
>>>>>> +++ b/net/lwip/net-lwip.c
>>>>>> @@ -13,8 +13,10 @@
>>>>>>   #include <lwip/etharp.h>
>>>>>>   #include <lwip/init.h>
>>>>>>   #include <lwip/prot/etharp.h>
>>>>>> +#include <lwip/timeouts.h>
>>>>>>   #include <net.h>
>>>>>>   #include <timer.h>
>>>>>> +#include <u-boot/schedule.h>
>>>>>>
>>>>>>   /* xx:xx:xx:xx:xx:xx\0 */
>>>>>>   #define MAC_ADDR_STRLEN 18
>>>>>> @@ -284,6 +286,11 @@ int net_lwip_rx(struct udevice *udev, struct netif *netif)
>>>>>>       int len;
>>>>>>       int i;
>>>>>>
>>>>>> +     /* lwIP timers */
>>>>>> +     sys_check_timeouts();
>>>>>> +     /* Other tasks and actions */
>>>>>> +     schedule(
>>>>>
>>>>> Some scheduled tasks have a long duration, specifically video_sync_all()
>>>>> called via video_idle(). This change will lead to lost packages.
>>>>>
>>>>>
>>>>> @Tom, Simon
>>>>>
>>>>> The EFI code was impacted by 29caf9305b6f ("cyclic: Use schedule()
>>>>> instead of WATCHDOG_RESET()"). This added schedule() calls to EFI
>>>>> networking. Package losses can expected if schedule() takes more than 1
>>>>> µs. This is much shorter than the time needed for synchronizing a 1 MiB
>>>>> frame buffer. The cyclic framework was explicitly meant for short
>>>>> duration function calls, not for long duration tasks like syncing the
>>>>> video frame buffer.
>>>>>
>>>>>
>>>>> I think we should remove video_idle() from the cyclic framework.
>>>>> video_sync_all() should only be triggered if there is video output.
>>>>>
>>>>
>>>> Hi Heinrich,
>>>>
>>>> It makes sense what you are saying but I don't have the hardware to
>>>> test your scenario. Can you submit a patch for discussion with a fixes
>>>> tag?
>>>
>>> Gentle ping here.
>>>
>>> Can we get some discussion on the point Heinrich brings up about
>>> video_idle in cyclic? Without my patch here which this discussion is
>>> blocking, large lwIP network transfers are quite broken on boards with
>>> watchdogs.
>>
>> Another option might be to increase the watchdog timeout?
>>
> Hi Jerome,
> 
> I don't think that makes much sense. Long transfers for compressed
> disk images can take minutes sometimes. It doesn't make sense to not
> call any cyclic's in that timeframe.

Sorry I wasn't clear and I was mistaken.  I was not suggesting not to
call schedule() in the network receive loop, I was responding to the
fact that schedule()->video_idle()->video_sync_all() can take a long
time. I thought this would in turn cause issues with watchdogs (hence
my question if the watchdog can be adjusted), while in fact the issue
is packet loss. So please ignore my comment ;)
Not using the cyclic framework for this video thing is probably the
right thing to do I guess.

> Additionally its possible some
> older less capable watchdogs don't have that much flexibility in
> timeouts.
> 
> What Heinrich points out here seems valid but I'm not familiar with
> the video framework to work up a patch.

Yeah, same.

Thanks,
-- 
Jerome
> Best Regards,
> 
> Tim