From a86411b81e4a1bdf03df4610d4207158286591e2 Mon Sep 17 00:00:00 2001 From: Benjamin Valentin Date: Wed, 20 Nov 2024 16:40:36 +0100 Subject: [PATCH 1/2] drivers/netdev: allow .send() to return > 0 to signal synchronos send --- drivers/include/net/netdev.h | 8 +++++++- pkg/lwip/contrib/netdev/lwip_netdev.c | 22 +++++++++++++--------- sys/net/gnrc/netif/gnrc_netif.c | 2 +- 3 files changed, 21 insertions(+), 11 deletions(-) diff --git a/drivers/include/net/netdev.h b/drivers/include/net/netdev.h index 66ebc37c65..30fdaf9aa1 100644 --- a/drivers/include/net/netdev.h +++ b/drivers/include/net/netdev.h @@ -440,7 +440,7 @@ typedef struct netdev_driver { * @retval -ENETDOWN Device is powered down * @retval <0 Other error * @retval 0 Transmission successfully started - * @retval >0 Number of bytes transmitted (deprecated!) + * @retval >0 Number of bytes transmitted (transmission already complete) * * This function will cause the driver to start the transmission in an * async fashion. The driver will "own" the `iolist` until a subsequent @@ -448,6 +448,12 @@ typedef struct netdev_driver { * than `-EAGAIN`. The driver must signal completion using the * NETDEV_EVENT_TX_COMPLETE event, regardless of success or failure. * + * If the driver implements blocking send (e.g. because it writes out the + * frame byte-by-byte over a serial line) it can also return the number of bytes + * transmitted here directly. In this case it MUST NOT emit a NETDEV_EVENT_TX_COMPLETE + * event, netdev_driver_t::confirm_send will never be called but should still be + * implemented to signal conformance to the new API. + * * Old drivers might not be ported to the new API and have * netdev_driver_t::confirm_send set to `NULL`. In that case the driver * will return the number of bytes transmitted on success (instead of `0`) diff --git a/pkg/lwip/contrib/netdev/lwip_netdev.c b/pkg/lwip/contrib/netdev/lwip_netdev.c index dde6e631a9..592e6e59b8 100644 --- a/pkg/lwip/contrib/netdev/lwip_netdev.c +++ b/pkg/lwip/contrib/netdev/lwip_netdev.c @@ -275,8 +275,9 @@ static err_t _common_link_output(struct netif *netif, netdev_t *netdev, iolist_t { lwip_netif_dev_acquire(netif); + err_t res; if (is_netdev_legacy_api(netdev)) { - err_t res = (netdev->driver->send(netdev, iolist) > 0) ? ERR_OK : ERR_BUF; + res = (netdev->driver->send(netdev, iolist) > 0) ? ERR_OK : ERR_BUF; lwip_netif_dev_release(netif); return res; } @@ -288,7 +289,8 @@ static err_t _common_link_output(struct netif *netif, netdev_t *netdev, iolist_t compat_netif->thread_doing_tx = thread_get_active(); irq_restore(irq_state); - if (netdev->driver->send(netdev, iolist) < 0) { + res = netdev->driver->send(netdev, iolist); + if (res < 0) { lwip_netif_dev_release(netif); irq_state = irq_disable(); compat_netif->thread_doing_tx = NULL; @@ -303,17 +305,19 @@ static err_t _common_link_output(struct netif *netif, netdev_t *netdev, iolist_t compat_netif->thread_doing_tx = NULL; irq_restore(irq_state); - int retval; - while (-EAGAIN == (retval = netdev->driver->confirm_send(netdev, NULL))) { - /* this should not happen, as the driver really only should emit the - * TX done event when it is actually done. But better be safe than - * sorry */ - DEBUG_PUTS("[lwip_netdev] confirm_send() returned -EAGAIN\n"); + if (res == 0) { + /* async send */ + while (-EAGAIN == (res = netdev->driver->confirm_send(netdev, NULL))) { + /* this should not happen, as the driver really only should emit the + * TX done event when it is actually done. But better be safe than + * sorry */ + DEBUG_PUTS("[lwip_netdev] confirm_send() returned -EAGAIN\n"); + } } lwip_netif_dev_release(netif); - if (retval < 0) { + if (res < 0) { return ERR_IF; } diff --git a/sys/net/gnrc/netif/gnrc_netif.c b/sys/net/gnrc/netif/gnrc_netif.c index 541de5cf29..aa6cd4020f 100644 --- a/sys/net/gnrc/netif/gnrc_netif.c +++ b/sys/net/gnrc/netif/gnrc_netif.c @@ -1915,7 +1915,7 @@ static void _send(gnrc_netif_t *netif, gnrc_pktsnip_t *pkt, bool push_back) * completed. For new netdevs (with confirm_send), TX is async. It is only * done if TX failed right away (res < 0). */ - if (gnrc_netif_netdev_legacy_api(netif) || (res < 0)) { + if (gnrc_netif_netdev_legacy_api(netif) || (res != 0)) { _tx_done(netif, pkt, tx_sync, res, push_back); } #if IS_USED(MODULE_NETDEV_NEW_API) From 4eb1c35fe3df1d4e57687eafe91cf18978df1453 Mon Sep 17 00:00:00 2001 From: Benjamin Valentin Date: Wed, 20 Nov 2024 18:17:22 +0100 Subject: [PATCH 2/2] cpu/sam0_eth: fix return values of sam0_eth_send() --- cpu/sam0_common/periph/eth.c | 43 +++++++++++++-------------- cpu/sam0_common/sam0_eth/eth-netdev.c | 9 +----- 2 files changed, 21 insertions(+), 31 deletions(-) diff --git a/cpu/sam0_common/periph/eth.c b/cpu/sam0_common/periph/eth.c index 3993b988b1..f2f54982f5 100644 --- a/cpu/sam0_common/periph/eth.c +++ b/cpu/sam0_common/periph/eth.c @@ -217,45 +217,42 @@ void sam0_eth_get_mac(eui48_t *out) int sam0_eth_send(const struct iolist *iolist) { - unsigned len = iolist_size(iolist); unsigned tx_len = 0; tx_curr = &tx_desc[tx_idx]; if (_is_sleeping) { - return -ENOTSUP; + return -ENETDOWN; } /* load packet data into TX buffer */ for (const iolist_t *iol = iolist; iol; iol = iol->iol_next) { if (tx_len + iol->iol_len > ETHERNET_MAX_LEN) { - return -EBUSY; + return -EOVERFLOW; } if (iol->iol_len) { memcpy ((uint32_t*)(tx_curr->address + tx_len), iol->iol_base, iol->iol_len); tx_len += iol->iol_len; } } - if (len == tx_len) { - /* Clear and set the frame size */ - tx_curr->status = (len & DESC_TX_STATUS_LEN_MASK) - /* Indicate this is the last buffer and the frame is ready */ - | DESC_TX_STATUS_LAST_BUF; - /* Prepare next buffer index */ - if (++tx_idx == ETH_TX_BUFFER_COUNT) { - /* Set WRAP flag to indicate last buffer */ - tx_curr->status |= DESC_TX_STATUS_WRAP; - tx_idx = 0; - } - __DMB(); - /* Start transmission */ - GMAC->NCR.reg |= GMAC_NCR_TSTART; - /* Set the next buffer */ - tx_curr = &tx_desc[tx_idx]; + + /* Clear and set the frame size */ + tx_curr->status = (tx_len & DESC_TX_STATUS_LEN_MASK) + /* Indicate this is the last buffer and the frame is ready */ + | DESC_TX_STATUS_LAST_BUF; + /* Prepare next buffer index */ + if (++tx_idx == ETH_TX_BUFFER_COUNT) { + /* Set WRAP flag to indicate last buffer */ + tx_curr->status |= DESC_TX_STATUS_WRAP; + tx_idx = 0; } - else { - DEBUG("Mismatch TX len, abort send\n"); - } - return len; + __DMB(); + + /* Start transmission */ + GMAC->NCR.reg |= GMAC_NCR_TSTART; + /* Set the next buffer */ + tx_curr = &tx_desc[tx_idx]; + + return 0; } unsigned _sam0_eth_get_last_len(void) diff --git a/cpu/sam0_common/sam0_eth/eth-netdev.c b/cpu/sam0_common/sam0_eth/eth-netdev.c index d61bb19e94..77af15ee29 100644 --- a/cpu/sam0_common/sam0_eth/eth-netdev.c +++ b/cpu/sam0_common/sam0_eth/eth-netdev.c @@ -197,15 +197,8 @@ static int _sam0_eth_recv(netdev_t *netdev, void *buf, size_t len, void *info) static int _sam0_eth_send(netdev_t *netdev, const iolist_t *iolist) { - int ret; - netdev->event_callback(netdev, NETDEV_EVENT_TX_STARTED); - ret = sam0_eth_send(iolist); - if (ret == -EOVERFLOW) { - /* TODO: use a specific netdev callback here ? */ - return -EOVERFLOW; - } - return ret; + return sam0_eth_send(iolist); } static int _sam0_eth_confirm_send(netdev_t *netdev, void *info)