casaroli commented on code in PR #21502:
URL: https://github.com/apache/nuttx/pull/21502#discussion_r4496390735
##########
drivers/net/netdev_upperhalf.c:
##########
@@ -1318,7 +1321,15 @@ static int netdev_upper_ifup(FAR struct net_driver_s
*dev)
if (upper->lower->ops->ifup)
{
- return upper->lower->ops->ifup(upper->lower);
+ ret = upper->lower->ops->ifup(upper->lower);
+ if (ret < 0)
+ {
+ return ret;
+ }
+
+ /* Register netdev drivers */
+
+ return netdev_drivers_init(upper->lower);
Review Comment:
No lower-half driver in the tree implements the new `txpoll` op, so
`netpoll_udp_init()` returns `-EINVAL` and this line makes `ifup` of
`CONFIG_DRIVERS_NOTENET_INTF` (default `eth0`) fail. At that point the lower
half is already up (and the RX threads of a `NETDEV_RX_THREAD` driver run), but
`IFF_UP` is not set. On qemu-armv8a:netnsh_hv (virtio-net) with
`CONFIG_DRIVERS_NOTENET=y`, `ifup eth0` prints `Failed` and `ping 10.0.2.2`
fails with `ENETUNREACH`; with only `CONFIG_NET_NETPOLL=y`, both work.
Fix: do not fail `ifup` when the note binding fails (log it and return
`OK`), and add `txpoll` to the driver you test with.
##########
drivers/note/notenet_driver.c:
##########
@@ -0,0 +1,301 @@
+/****************************************************************************
+ * drivers/note/notenet_driver.c
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <nuttx/net/netpoll.h>
+#include <nuttx/note/note_driver.h>
+#include <nuttx/note/notenet_driver.h>
+#include <nuttx/panic_notifier.h>
+#include <nuttx/spinlock.h>
+#include <nuttx/wqueue.h>
+
+/****************************************************************************
+ * Pre-processor definitions
+ ****************************************************************************/
+
+#define NOTENET_WORK_DELAY MSEC2TICK(CONFIG_DRIVERS_NOTENET_WORK_DELAY)
+
+/****************************************************************************
+ * Private Types
+ ****************************************************************************/
+
+struct notenet_driver_s
+{
+ struct note_driver_s driver; /* Note driver part, must be first element */
+ spinlock_t lock; /* Lock */
+ FAR netpkt_t *pkt; /* Allocated net packet */
+ size_t next; /* The next index (where data is added) */
+ struct work_s work; /* Used for deferred callback work */
+ struct netpoll_s np; /* Netpoll */
+ struct notifier_block nb; /* Panic notifier */
+};
+
+/****************************************************************************
+ * Private Function Prototypes
+ ****************************************************************************/
+
+static bool notenetdev_transfer(FAR struct notenet_driver_s *drv);
+static void notenetdev_work(FAR void *priv);
+static void note_netdev_add(FAR struct note_driver_s *driver,
+ FAR const void *note, size_t notelen);
+static int netdev_note_panic_callback(FAR struct notifier_block *nb,
+ unsigned long action,
+ FAR void *data);
+
+/****************************************************************************
+ * Private Data
+ ****************************************************************************/
+
+static const struct note_driver_ops_s g_notenetdev_ops =
+{
+ note_netdev_add
+};
+
+/****************************************************************************
+ * Public Data
+ ****************************************************************************/
+
+struct notenet_driver_s g_notenet =
+{
+ {
+#ifdef CONFIG_SCHED_INSTRUMENTATION_FILTER
+ "net",
+ {
+ {
+ CONFIG_SCHED_INSTRUMENTATION_FILTER_DEFAULT_MODE,
+# ifdef CONFIG_SMP
+ CONFIG_SCHED_INSTRUMENTATION_CPUSET
+# endif
+ },
+ },
+#endif
+ &g_notenetdev_ops
+ },
+ .lock = SP_UNLOCKED
+};
+
+/****************************************************************************
+ * Private Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: notenetdev_transfer
+ ****************************************************************************/
+
+static bool notenetdev_transfer(FAR struct notenet_driver_s *drv)
+{
+ irqstate_t flags;
+ int ret;
+
+ flags = spin_lock_irqsave_notrace(&drv->lock);
+
+ if (drv->next == 0)
+ {
+ spin_unlock_irqrestore_notrace(&drv->lock, flags);
+ return true;
+ }
+
+ /* Send IOB */
+
+ ret = netpoll_udp_pkt_send(&drv->np, drv->pkt);
+ if (ret < 0)
+ {
+ spin_unlock_irqrestore_notrace(&drv->lock, flags);
+ return false;
+ }
+
+ /* Get IOB - blocking */
+
+ drv->pkt = netpoll_udp_pkt_alloc(&drv->np);
+ drv->next = 0;
+
+ spin_unlock_irqrestore_notrace(&drv->lock, flags);
+
+ return true;
+}
+
+/****************************************************************************
+ * Name: notenetdev_work
+ ****************************************************************************/
+
+static void notenetdev_work(FAR void *arg)
+{
+ FAR struct notenet_driver_s *drv = arg;
+
+ if (!notenetdev_transfer(drv))
+ {
+ work_queue(HPWORK, &drv->work, notenetdev_work, arg,
+ NOTENET_WORK_DELAY);
+ }
+}
+
+/****************************************************************************
+ * Name: note_netdev_add
+ *
+ * Description:
+ * Add the variable length note to the transport layer
+ *
+ * Input Parameters:
+ * note - The note buffer
+ * notelen - The buffer length
+ *
+ * Returned Value:
+ * None
+ *
+ * Assumptions:
+ * We are within a critical section.
+ *
+ ****************************************************************************/
+
+static void note_netdev_add(FAR struct note_driver_s *driver,
+ FAR const void *note, size_t notelen)
+{
+ FAR struct notenet_driver_s *drv =
+ (FAR struct notenet_driver_s *)driver;
+ clock_t delay = NOTENET_WORK_DELAY;
+ size_t maxlen = netpoll_get_maxlen(&drv->np);
+ size_t space;
+ irqstate_t flags;
+
+ flags = spin_lock_irqsave_notrace(&drv->lock);
+
+ /* Ignore data if device is not ready */
+
+ if (!drv->pkt)
+ {
+ spin_unlock_irqrestore_notrace(&drv->lock, flags);
+ return;
+ }
+
+try:
+ if (drv->next + notelen > maxlen)
+ {
+ /* Unlock before entering notenetdev_transfer,
+ * it is already locked internally
+ */
+
+ spin_unlock_irqrestore_notrace(&drv->lock, flags);
+ notenetdev_transfer(drv);
+ flags = spin_lock_irqsave_notrace(&drv->lock);
+ goto try;
Review Comment:
When `notenetdev_transfer()` fails, `drv->next` stays the same and this loop
never ends. `netpoll_udp_pkt_send()` returns `-ENODEV` when `IFF_RUNNING` is
clear, for example after `ifdown` (`netdev_ifdown()` clears it) or a carrier
loss. The next note that does not fit then hangs the CPU in the note hook,
often with interrupts disabled. On qemu-armv8a:netnsh_hv with a `txpoll` added
to virtio-net (it calls `virtio_net_txfree()`), the console stops answering
about 3 s after `ifdown eth0`; without `CONFIG_DRIVERS_NOTENET` it keeps
working.
Fix: when the transfer fails, drop the note (or the buffered data) and
return, as `noterpmsg_add()` does.
##########
net/netpoll/netpoll_udp.c:
##########
@@ -0,0 +1,414 @@
+/****************************************************************************
+ * net/netpoll/netpoll_udp.c
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <netinet/ether.h>
+#include <nuttx/net/netpoll.h>
+#include <nuttx/net/udp.h>
+
+#include "inet/inet.h"
+
+/****************************************************************************
+ * Private Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: netpoll_udp_build_header
+ *
+ * Description:
+ * Fill packet with UDP header
+ *
+ * Input Parameters:
+ * np - pointer to netpoll data
+ * pkt - packet to send
+ *
+ * Returned Value:
+ * OK on success; Negated errno on failure.
+ *
+ ****************************************************************************/
+
+static void netpoll_udp_build_header(FAR struct netpoll_s *np,
+ FAR netpkt_t *pkt)
+{
+ FAR struct netdev_lowerhalf_s *lower = np->lower;
+ FAR struct eth_hdr_s *eth;
+ FAR struct ipv4_hdr_s *ipv4;
+ FAR struct udp_hdr_s *udp;
+ FAR uint8_t *data;
+ size_t offset = 0;
+
+ /* Get IO data */
+
+ data = netpkt_getdata(lower, pkt);
+
+ /* Fill MAC header */
+
+ eth = (FAR struct eth_hdr_s *)&data[offset];
+ eth->type = htons(ETHTYPE_IP);
+ memcpy(eth->dest, np->destmac, sizeof(eth->dest));
+ memcpy(eth->src, &lower->netdev.d_mac, sizeof(eth->src));
+
+ /* Fill IPv4 header */
+
+ offset += ETH_HDRLEN;
+ ipv4 = (FAR struct ipv4_hdr_s *)&data[offset];
+ ipv4_build_header(ipv4, pkt->io_len, IP_PROTO_UDP, &lower->netdev.d_ipaddr,
Review Comment:
`pkt->io_len` is the data in the first IOB only. With the default
`CONFIG_IOB_BUFSIZE=196` (also in `sim:tcpblaster`) a note packet of up to 1472
bytes is an IOB chain, so the IPv4 total length here and the UDP length on line
89 are too short and the receiver cuts or drops the packet. On
qemu-armv8a:netnsh_hv with `CONFIG_IOB_BUFSIZE=196`, a note frame with 501
bytes of UDP payload has IPv4 length 164 and UDP length 144; with 1534 the
lengths are right.
Fix: use `pkt->io_pktlen` for both lengths.
##########
drivers/net/Make.defs:
##########
@@ -27,7 +27,7 @@ ifeq ($(CONFIG_NET),y)
# Include network interface drivers
ifeq ($(CONFIG_MM_IOB),y)
- CSRCS += netdev_upperhalf.c
+ CSRCS += netdev_upperhalf.c netdev_drivers.c
Review Comment:
`netdev_drivers.c` is now built into every network build, and
`netdev_upper_ifup()` calls it on every `ifup`, also with
`CONFIG_DRIVERS_NOTENET=n`, where it does nothing.
Fix: build it and call it only when `CONFIG_DRIVERS_NOTENET` is set (same in
`drivers/net/CMakeLists.txt`).
##########
net/netpoll/netpoll_udp.c:
##########
@@ -0,0 +1,414 @@
+/****************************************************************************
+ * net/netpoll/netpoll_udp.c
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <netinet/ether.h>
+#include <nuttx/net/netpoll.h>
+#include <nuttx/net/udp.h>
+
+#include "inet/inet.h"
+
+/****************************************************************************
+ * Private Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: netpoll_udp_build_header
+ *
+ * Description:
+ * Fill packet with UDP header
+ *
+ * Input Parameters:
+ * np - pointer to netpoll data
+ * pkt - packet to send
+ *
+ * Returned Value:
+ * OK on success; Negated errno on failure.
+ *
+ ****************************************************************************/
+
+static void netpoll_udp_build_header(FAR struct netpoll_s *np,
+ FAR netpkt_t *pkt)
+{
+ FAR struct netdev_lowerhalf_s *lower = np->lower;
+ FAR struct eth_hdr_s *eth;
+ FAR struct ipv4_hdr_s *ipv4;
+ FAR struct udp_hdr_s *udp;
+ FAR uint8_t *data;
+ size_t offset = 0;
+
+ /* Get IO data */
+
+ data = netpkt_getdata(lower, pkt);
+
+ /* Fill MAC header */
+
+ eth = (FAR struct eth_hdr_s *)&data[offset];
+ eth->type = htons(ETHTYPE_IP);
+ memcpy(eth->dest, np->destmac, sizeof(eth->dest));
+ memcpy(eth->src, &lower->netdev.d_mac, sizeof(eth->src));
+
+ /* Fill IPv4 header */
+
+ offset += ETH_HDRLEN;
+ ipv4 = (FAR struct ipv4_hdr_s *)&data[offset];
+ ipv4_build_header(ipv4, pkt->io_len, IP_PROTO_UDP, &lower->netdev.d_ipaddr,
+ &np->destipaddr, IP_TTL_DEFAULT, 0, NULL);
+
+ /* Fill UDP header */
+
+ offset += IPv4_HDRLEN;
+ udp = (FAR struct udp_hdr_s *)&data[offset];
+
+ udp->srcport = np->srcport;
+ udp->destport = np->destport;
+ udp->udplen = htons(pkt->io_len - IPv4_HDRLEN);
+ udp->udpchksum = 0;
+}
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: netpoll_udp_init
+ *
+ * Description:
+ * Initialize netpoll with UDP.
+ *
+ * Input Parameters:
+ * np - pointer to netpoll data
+ * lower - pointer to netdev lower
+ * srcport - source port number
+ * destipaddr - destination IP address
+ * destport - destination port number
+ * destmac - MAC address octets in string format: "ff:ff:ff:ff:ff:ff".
+ * Default: "ff:ff:ff:ff:ff:ff"
+ *
+ * Returned Value:
+ * Returns OK on success
+ *
+ ****************************************************************************/
+
+int netpoll_udp_init(FAR struct netpoll_s *np,
+ FAR struct netdev_lowerhalf_s *lower,
+ uint16_t srcport, in_addr_t destipaddr,
+ uint16_t destport, FAR const char *destmac)
+{
+ struct ether_addr addr;
+
+ if (lower->ops->txpoll == NULL)
+ {
+ return -EINVAL;
+ }
+
+ memset(np, 0, sizeof(struct netpoll_s));
+
+ if (destmac == NULL)
+ {
+ destmac = "ff:ff:ff:ff:ff:ff";
+ }
+
+ /* Decode MAC */
+
+ if (!ether_aton_r(destmac, &addr))
+ {
+ return -EINVAL;
+ }
+
+ memcpy(np->destmac, addr.ether_addr_octet, ETHER_ADDR_LEN);
+
+ /* Parameters */
+
+ np->srcport = htons(srcport);
+ np->destipaddr = htonl(destipaddr);
+ np->destport = htons(destport);
+
+ /* Store lower half reference */
+
+ np->lower = lower;
+
+ /* Store header length for later */
+
+ np->hdrlen = NET_LL_HDRLEN(&lower->netdev) + IPv4UDP_HDRLEN;
+
+ /* Get maximum UDP packet length for this device */
+
+ np->maxlen = NETDEV_PKTSIZE(&lower->netdev) - np->hdrlen;
+
+ return OK;
+}
+
+/****************************************************************************
+ * Name: netpoll_udp_pkt_alloc
+ *
+ * Description:
+ * Allocate UDP packet.
+ *
+ * Input Parameters:
+ * np - pointer to netpoll data
+ *
+ * Returned Value:
+ * Pointer to packet on success; NULL on failure.
+ *
+ ****************************************************************************/
+
+FAR netpkt_t *netpoll_udp_pkt_alloc(FAR struct netpoll_s *np)
+{
+ FAR struct netdev_lowerhalf_s *lower = np->lower;
+ FAR netpkt_t *pkt;
+
+ /* Get packet */
+
+ pkt = netpoll_pkt_alloc(lower);
+ if (!pkt)
+ {
+ return NULL;
+ }
+
+ /* Initialize packet with header length */
+
+ netpkt_setdatalen(lower, pkt, np->hdrlen);
+
+ return pkt;
+}
+
+/****************************************************************************
+ * Name: netpoll_udp_pkt_send
+ *
+ * Description:
+ * Send packet as UDP packet with netpoll.
+ *
+ * Input Parameters:
+ * np - pointer to netpoll data
+ * pkt - packet to send
+ *
+ * Returned Value:
+ * Length of header if success, negative value on failure
+ *
+ * Assumption:
+ * Packet is ready to send, all packet headers are complete, only UDP
+ * payload len can change.
+ *
+ ****************************************************************************/
+
+int netpoll_udp_pkt_send(FAR struct netpoll_s *np, FAR netpkt_t *pkt)
+{
+ FAR struct netdev_lowerhalf_s *lower = np->lower;
+ int ret;
+
+ /* Don't send data if device is not ready */
+
+ if (!IFF_IS_RUNNING(lower->netdev.d_flags))
+ {
+ return -ENODEV;
+ }
+
+ /* Fill header */
+
+ netpoll_udp_build_header(np, pkt);
+
+ /* Transmit packet */
+
+ ret = lower->ops->transmit(lower, pkt);
Review Comment:
This calls the driver's `transmit` from the note work and, when the note
buffer is full, from any note hook (also an interrupt handler), with no lock
that the upper half also takes. The upper half calls `transmit` with
`netdev_lock()` held (`netdev_upper_txavail_work()`). On SMP, or when a note
hook interrupts the upper half inside `transmit`, two calls run in the driver
at the same time and can corrupt its TX ring.
Fix: serialize with the upper half: send only from the work with
`netdev_lock()` held, and send inline from interrupt context only on the panic
path.
##########
net/Kconfig:
##########
@@ -67,6 +67,15 @@ config NET_DEFAULT_MAX_PORT
---help---
Default Network max port
+config NET_NETPOLL
Review Comment:
`NET_NETPOLL` has no dependencies, but `net/netpoll/netpoll_udp.c` builds
IPv4 and Ethernet headers (`d_ipaddr`, `ipv4_build_header()`, `struct
eth_hdr_s`). `d_ipaddr` exists only with `CONFIG_NET_IPv4`
(`include/nuttx/net/netdev.h:391`), and so does `ipv4_build_header()`
(`net/inet/inet.h:289`), so with `CONFIG_NET_NETPOLL=y` and IPv4 off,
`netpoll_udp.c:79` fails: `'struct net_driver_s' has no member named
'd_ipaddr'`.
Fix: add `depends on NET_IPv4 && NET_ETHERNET`.
##########
drivers/note/notenet_driver.c:
##########
@@ -0,0 +1,301 @@
+/****************************************************************************
+ * drivers/note/notenet_driver.c
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <nuttx/net/netpoll.h>
+#include <nuttx/note/note_driver.h>
+#include <nuttx/note/notenet_driver.h>
+#include <nuttx/panic_notifier.h>
+#include <nuttx/spinlock.h>
+#include <nuttx/wqueue.h>
+
+/****************************************************************************
+ * Pre-processor definitions
+ ****************************************************************************/
+
+#define NOTENET_WORK_DELAY MSEC2TICK(CONFIG_DRIVERS_NOTENET_WORK_DELAY)
+
+/****************************************************************************
+ * Private Types
+ ****************************************************************************/
+
+struct notenet_driver_s
+{
+ struct note_driver_s driver; /* Note driver part, must be first element */
+ spinlock_t lock; /* Lock */
+ FAR netpkt_t *pkt; /* Allocated net packet */
+ size_t next; /* The next index (where data is added) */
+ struct work_s work; /* Used for deferred callback work */
+ struct netpoll_s np; /* Netpoll */
+ struct notifier_block nb; /* Panic notifier */
+};
+
+/****************************************************************************
+ * Private Function Prototypes
+ ****************************************************************************/
+
+static bool notenetdev_transfer(FAR struct notenet_driver_s *drv);
+static void notenetdev_work(FAR void *priv);
+static void note_netdev_add(FAR struct note_driver_s *driver,
+ FAR const void *note, size_t notelen);
+static int netdev_note_panic_callback(FAR struct notifier_block *nb,
+ unsigned long action,
+ FAR void *data);
+
+/****************************************************************************
+ * Private Data
+ ****************************************************************************/
+
+static const struct note_driver_ops_s g_notenetdev_ops =
+{
+ note_netdev_add
+};
+
+/****************************************************************************
+ * Public Data
+ ****************************************************************************/
+
+struct notenet_driver_s g_notenet =
+{
+ {
+#ifdef CONFIG_SCHED_INSTRUMENTATION_FILTER
+ "net",
+ {
+ {
+ CONFIG_SCHED_INSTRUMENTATION_FILTER_DEFAULT_MODE,
+# ifdef CONFIG_SMP
+ CONFIG_SCHED_INSTRUMENTATION_CPUSET
+# endif
+ },
+ },
+#endif
+ &g_notenetdev_ops
+ },
+ .lock = SP_UNLOCKED
+};
+
+/****************************************************************************
+ * Private Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: notenetdev_transfer
+ ****************************************************************************/
+
+static bool notenetdev_transfer(FAR struct notenet_driver_s *drv)
+{
+ irqstate_t flags;
+ int ret;
+
+ flags = spin_lock_irqsave_notrace(&drv->lock);
+
+ if (drv->next == 0)
+ {
+ spin_unlock_irqrestore_notrace(&drv->lock, flags);
+ return true;
+ }
+
+ /* Send IOB */
+
+ ret = netpoll_udp_pkt_send(&drv->np, drv->pkt);
+ if (ret < 0)
+ {
+ spin_unlock_irqrestore_notrace(&drv->lock, flags);
+ return false;
+ }
+
+ /* Get IOB - blocking */
+
+ drv->pkt = netpoll_udp_pkt_alloc(&drv->np);
+ drv->next = 0;
+
+ spin_unlock_irqrestore_notrace(&drv->lock, flags);
+
+ return true;
+}
+
+/****************************************************************************
+ * Name: notenetdev_work
+ ****************************************************************************/
+
+static void notenetdev_work(FAR void *arg)
+{
+ FAR struct notenet_driver_s *drv = arg;
+
+ if (!notenetdev_transfer(drv))
+ {
+ work_queue(HPWORK, &drv->work, notenetdev_work, arg,
+ NOTENET_WORK_DELAY);
+ }
+}
+
+/****************************************************************************
+ * Name: note_netdev_add
+ *
+ * Description:
+ * Add the variable length note to the transport layer
+ *
+ * Input Parameters:
+ * note - The note buffer
+ * notelen - The buffer length
+ *
+ * Returned Value:
+ * None
+ *
+ * Assumptions:
+ * We are within a critical section.
+ *
+ ****************************************************************************/
+
+static void note_netdev_add(FAR struct note_driver_s *driver,
+ FAR const void *note, size_t notelen)
+{
+ FAR struct notenet_driver_s *drv =
+ (FAR struct notenet_driver_s *)driver;
+ clock_t delay = NOTENET_WORK_DELAY;
+ size_t maxlen = netpoll_get_maxlen(&drv->np);
+ size_t space;
+ irqstate_t flags;
+
+ flags = spin_lock_irqsave_notrace(&drv->lock);
+
+ /* Ignore data if device is not ready */
+
+ if (!drv->pkt)
+ {
+ spin_unlock_irqrestore_notrace(&drv->lock, flags);
+ return;
+ }
+
+try:
+ if (drv->next + notelen > maxlen)
+ {
+ /* Unlock before entering notenetdev_transfer,
+ * it is already locked internally
+ */
+
+ spin_unlock_irqrestore_notrace(&drv->lock, flags);
+ notenetdev_transfer(drv);
+ flags = spin_lock_irqsave_notrace(&drv->lock);
+ goto try;
+ }
+
+ /* Copy data to packet */
+
+ netpoll_pkt_copy(&drv->np, drv->pkt, note, notelen, drv->next);
+ drv->next += notelen;
+
+ /* Start work immediately when data more then 75% */
+
+ space = maxlen - drv->next;
+ if (space < maxlen / 4)
+ {
+ delay = 0;
+ }
+
+ spin_unlock_irqrestore_notrace(&drv->lock, flags);
+
+ if (work_available(&drv->work))
+ {
+ work_queue(HPWORK, &drv->work, notenetdev_work, drv, delay);
+ }
+}
+
+/****************************************************************************
+ * Name: netdev_note_panic_callback
+ *
+ * Description:
+ * This is panic callback for note driver.
+ *
+ ****************************************************************************/
+
+static int netdev_note_panic_callback(FAR struct notifier_block *nb,
+ unsigned long action,
+ FAR void *data)
+{
+ FAR struct notenet_driver_s *note =
+ container_of(nb, struct notenet_driver_s, nb);
+
+ if (action == PANIC_KERNEL_FINAL)
+ {
+ notenetdev_transfer(note);
+ }
+
+ return 0;
+}
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: notenet_init
+ *
+ * Description:
+ * Register a netdev channel to note.
+ *
+ * Input Parameters:
+ * dev - The lower half netdev driver structure to be bound.
+ *
+ * Returned Value:
+ * Zero on success. A negated errno value is returned on a failure.
+ *
+ ****************************************************************************/
+
+int notenet_init(FAR struct netdev_lowerhalf_s *dev)
+{
+ FAR struct notenet_driver_s *note = &g_notenet;
+ irqstate_t flags;
+ int ret;
+
+ flags = spin_lock_irqsave_notrace(¬e->lock);
+
+ /* Initialize netpoll */
+
+ ret = netpoll_udp_init(¬e->np, dev,
+ CONFIG_DRIVERS_NOTENET_PORT,
+ CONFIG_DRIVERS_NOTENET_IPADDR,
+ CONFIG_DRIVERS_NOTENET_PORT,
+ CONFIG_DRIVERS_NOTENET_MAC);
+ if (ret < 0)
+ {
+ goto errout;
+ }
+
+ /* Register panic notifier */
+
+ note->nb.notifier_call = netdev_note_panic_callback;
+ panic_notifier_chain_register(¬e->nb);
+
+ /* Allocate iob */
+
+ note->pkt = netpoll_udp_pkt_alloc(¬e->np);
Review Comment:
`notenet_init()` runs at every `ifup` and overwrites `note->pkt` without
freeing it, so each `ifdown`/`ifup` leaks one TX packet of the driver quota.
When the quota is gone, `netpoll_pkt_alloc()` loops forever (it never returns
`NULL`, `net/netpoll/netpoll_pkt.c:54`), here and in `notenetdev_transfer()`,
with the spinlock held and interrupts disabled.
Fix: bind only once (or free the old packet first), and let
`netpoll_pkt_alloc()` give up and return `NULL` as its comment says.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
