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

[Tom Rini]

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?

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20250617/0991609a/attachment.sig>