Merge pull request #21012 from benpicco/netdev_new_api-relax

netdev_new_api: allow `.send()` to return > 0 to signal synchronos send

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)

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)

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`)

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;
     }
 

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)