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

[Jerome Forissier]

On 6/18/25 21:38, Tim Harvey wrote:
> On Wed, Jun 18, 2025 at 8:32 AM Tim Harvey <tharvey at gateworks.com> wrote:
>>
>> On Tue, Jun 17, 2025 at 3:45 PM Tom Rini <trini at konsulko.com> wrote:
>>>
>>> On Sun, Jun 01, 2025 at 11:07:16PM +0200, Heinrich Schuchardt 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.
>>>
>>> So for testing purposes, if CYCLIC is disabled but LWIP is enabled,
>>> things are fine here? And so we need to at least partially revert commit
>>> 5c8dddadf1c9 ("video: Use cyclic to handle video sync") if not fully
>>> revert it?
>>
>> Hi Tom,
>>
>> I can't disable CYCLIC without also disabling WDT which then works
>> fine as the watchdog isn't being used.
>>
>> You are likely pointing to the correct patch that is causing
>> Heinrich's issue but as far as I know he has the
>> hardware/configuration needed to test this and I don't know how to
>> re-create his issue.
>>
>> So in my opinion there is nothing wrong with my patch here and it's
>> more of an issue with the video using cyclic for video sync.

I agree. This patch is:

Reviewed-by: Jerome Forissier <jerome.forissier at linaro.org>


...and I will add the whole series in my next PR to Tom.

Thanks,

-- 
Jerome

>>
> 
> Incidentally, I've noticed that I only have this
> watchdog-not-getting-reset issue when using the IMX FEC driver. The
> IMX QOS (imx8mp) has calls to udelay in the packet send and udelay
> calls schedule. I'm not sure how many network drivers out there end up
> calling udelay/schedule on their own.
> 
> Best Regards,
> 
> Tim