xiaoxiang781216 commented on code in PR #7616:
URL: https://github.com/apache/nuttx/pull/7616#discussion_r1037364684
##########
net/utils/Make.defs:
##########
@@ -21,7 +21,11 @@
# Common utilities
NET_CSRCS += net_dsec2tick.c net_dsec2timeval.c net_timeval2dsec.c
-NET_CSRCS += net_chksum.c net_ipchksum.c net_incr32.c net_lock.c net_snoop.c
+NET_CSRCS += net_incr32.c net_lock.c net_snoop.c
+
+ifeq ($(CONFIG_MM_IOB),y)
+ NET_CSRCS += net_chksum.c net_ipchksum.c
Review Comment:
should we always build check sum?
##########
net/utils/net_chksum.c:
##########
@@ -90,6 +90,51 @@ uint16_t chksum(uint16_t sum, FAR const uint8_t *data,
uint16_t len)
}
#endif /* CONFIG_NET_ARCH_CHKSUM */
+/****************************************************************************
+ * Name: chksum_iob
+ *
+ * Description:
+ * Calculate the Internet checksum over an iob chain buffer.
+ *
+ * Input Parameters:
+ * sum - Partial calculations carried over from a previous call to
+ * chksum(). This should be zero on the first time that check
+ * sum is called.
+ * iob - An iob chain buffer over which the checksum is to be computed.
+ * offset - Specifies the byte offset of the start of valid data.
+ *
+ * Returned Value:
+ * The updated checksum value.
+ *
+ ****************************************************************************/
+
+#if !defined(CONFIG_NET_ARCH_CHKSUM) && defined(CONFIG_MM_IOB)
+uint16_t chksum_iob(uint16_t sum, FAR struct iob_s *iob, uint16_t offset)
Review Comment:
add to Kconfig
##########
net/utils/net_icmpchksum.c:
##########
@@ -61,6 +61,23 @@ uint16_t icmp_chksum(FAR struct net_driver_s *dev, int len)
}
#endif /* CONFIG_NET_ICMP */
+/****************************************************************************
+ * Name: icmp_chksum_iob
+ *
+ * Description:
+ * Calculate the checksum of the ICMP message, the input can be an I/O
+ * buffer chain
+ *
+ ****************************************************************************/
+
+#if (defined(CONFIG_NET_ICMP) || \
+ defined(CONFIG_NET_ICMPv6)) && defined(CONFIG_MM_IOB)
Review Comment:
```suggestion
#if defined(CONFIG_NET_ICMP) && defined(CONFIG_MM_IOB)
```
##########
net/udp/udp_sendto_buffered.c:
##########
@@ -737,14 +749,21 @@ ssize_t psock_udp_sendto(FAR struct socket *psock, FAR
const void *buf,
memcpy(&wrb->wb_dest, to, tolen);
}
+ /* Skip l2/l3/l4 offset before copy */
+
+ uint16_t udpiplen = udpip_hdrsize(conn);
Review Comment:
move to begin
##########
net/tcp/tcp_recvfrom.c:
##########
@@ -244,6 +255,7 @@ static inline void tcp_readahead(struct tcp_recvfrom_s
*pstate)
*/
recvlen = iob_copyout(pstate->ir_buffer, iob, pstate->ir_buflen, 0);
+
Review Comment:
revert
##########
arch/sim/src/sim/sim_netdriver.c:
##########
@@ -334,22 +358,21 @@ int sim_netdriver_init(void)
netdriver_txdone_interrupt,
netdriver_rxready_interrupt);
- /* Update the buffer size */
-
- pktsize = dev->d_pktsize ? dev->d_pktsize :
- (MAX_NETDEV_PKTSIZE + CONFIG_NET_GUARDSIZE);
-
/* Allocate packet buffer */
- pktbuf = kmm_malloc(pktsize);
- if (pktbuf == NULL)
+#ifdef SIM_NETDEV_IOB_OFFLOAD
+ dev->d_buf = NULL;
+#else
Review Comment:
```suggestion
#ifndef SIM_NETDEV_IOB_OFFLOAD
```
##########
net/devif/devif_poll.c:
##########
@@ -769,6 +875,69 @@ int devif_poll(FAR struct net_driver_s *dev,
devif_poll_callback_t callback)
/* Nothing more to do */
}
+ /* Device polling completed, release iob */
+
+ netdev_iob_release(dev);
+
+ return bstop;
+}
+
+/****************************************************************************
+ * Name: devif_poll
+ *
+ * Description:
+ * This function will traverse each active network connection structure and
+ * will perform network polling operations. devif_poll() may be called
+ * asynchronously with the network driver can accept another outgoing
+ * packet.
+ *
+ * This function will call the provided callback function for every active
+ * connection. Polling will continue until all connections have been polled
+ * or until the user-supplied function returns a non-zero value (which it
+ * should do only if it cannot accept further write data).
+ *
+ * When the callback function is called, there may be an outbound packet
+ * waiting for service in the device packet buffer, and if so the d_len
+ * field is set to a value larger than zero. The device driver should then
+ * send out the packet.
+ *
+ * Compatible with all old flat buffer NICs
+ *
+ * tcp_poll()/udp_poll()/pkt_poll()/...(l3|l4)
+ * / \
+ * / \
+ * devif_poll_[l3|l4]_connections() devif_iob_send() (nocopy:udp/icmp/..)
+ * / \ (copy:tcp)
+ * / \
+ * devif_iob_poll(devif_poll_callback()) devif_poll_callback()
+ * / \
+ * / \
+ * devif_poll("NIC"_txpoll) "NIC"_send()(dev->d_buf)
+ *
+ *
+ * Assumptions:
+ * This function is called from the MAC device driver with the network
+ * locked.
+ *
+ ****************************************************************************/
+
+int devif_poll(FAR struct net_driver_s *dev, devif_poll_callback_t callback)
+{
+ int bstop;
+
+ /* Hook the original callback/buffer before device poll,
+ * devif_poll_callback() will convert the stack iob to flat buffer.
+ */
+
+ dev->d_pollcallback = callback;
+ dev->d_pollbuf = dev->d_buf;
+
+ bstop = devif_iob_poll(dev, devif_poll_callback);
Review Comment:
can we detect iob automatically
##########
net/devif/devif_poll.c:
##########
@@ -617,12 +617,89 @@ static inline int devif_poll_tcp_connections(FAR struct
net_driver_s *dev,
# define devif_poll_tcp_connections(dev, callback) (0)
#endif
+/****************************************************************************
+ * Name: devif_poll_callback
+ *
+ * Description:
+ * This function will help us to gather multiple iob memory slices into a
+ * linear device buffer. if devices with small memory, this function will
+ * trigger a memory copy if net device start transmit the iob slices to
+ * flat buffer
+ *
+ ****************************************************************************/
+
+static int devif_poll_callback(FAR struct net_driver_s *dev)
+{
+ FAR struct iob_s *iob;
+ uint16_t llhdrlen;
+ int bstop;
+
+ if (dev->d_len == 0 || dev->d_iob == NULL)
+ {
+ return 0;
+ }
+
+ /* Loopback messge ? */
+
+ bstop = devif_loopback(dev);
+ if (bstop)
+ {
+ return bstop;
+ }
+
+ llhdrlen = NET_LL_HDRLEN(dev);
+
+ /* Copy iob to flat buffer */
+
+ iob_copyout(dev->d_pollbuf + llhdrlen, dev->d_iob, dev->d_len, 0);
Review Comment:
let's move the following code to devif_poll to avoid add the field
d_pollcallback and d_pollbuf
##########
include/nuttx/net/netdev.h:
##########
@@ -154,17 +157,25 @@
# define NETDEV_ERRORS(dev)
#endif
-/* There are some helper pointers for accessing the contents of the Ethernet
- * headers
+/* There are some helper pointers for accessing the contents of the Link
+ * layer headers
*/
-#define ETHBUF ((FAR struct eth_hdr_s *)&dev->d_buf[0])
+#define LLBUF ((FAR void *)(dev->d_iob ? \
+ &dev->d_iob->io_data[CONFIG_NET_LL_GUARDSIZE - \
Review Comment:
both point to the link layer header why need check d_iob?
##########
include/nuttx/net/netdev.h:
##########
@@ -802,4 +936,88 @@ void net_incr32(FAR uint8_t *op32, uint16_t op16);
int netdev_lladdrsize(FAR struct net_driver_s *dev);
+/****************************************************************************
+ * Name: netdev_iob_prepare
+ *
+ * Description:
+ * Prepare data buffer for a given NIC
+ * The iob offset will be updated to l2 gruard size by default:
+ * ----------------------------------------------------------------
+ * | iob entry |
+ * ---------------------------------------------------------------|
+ * |<--- CONFIG_NET_LL_GUARDSIZE -->|<--- io_len/io_pktlen(0) --->|
+ * ---------------------------------------------------------------|
+ *
+ * Assumptions:
+ * The caller has locked the network.
+ *
+ * Returned Value:
+ * A non-zero copy is returned on success.
+ *
+ ****************************************************************************/
+
+int netdev_iob_prepare(FAR struct net_driver_s *dev, bool throttled,
Review Comment:
```suggestion
int netdev_iob_alloc(FAR struct net_driver_s *dev, bool throttled,
```
##########
net/utils/net_chksum.c:
##########
@@ -123,6 +168,39 @@ uint16_t net_chksum(FAR uint16_t *data, uint16_t len)
}
#endif /* CONFIG_NET_ARCH_CHKSUM */
+/****************************************************************************
+ * Name: net_chksum_iob
+ *
+ * Description:
+ * Calculate the Internet checksum over an iob chain buffer.
+ *
+ * The Internet checksum is the one's complement of the one's complement
+ * sum of all 16-bit words in the buffer.
+ *
+ * See RFC1071.
+ *
+ * If CONFIG_NET_ARCH_CHKSUM is defined, then this function must be
+ * provided by architecture-specific logic.
+ *
+ * Input Parameters:
+ * sum - Partial calculations carried over from a previous call to
+ * chksum(). This should be zero on the first time that check
+ * sum is called.
+ * iob - An iob chain buffer over which the checksum is to be computed.
+ * offset - Specifies the byte offset of the start of valid data.
+ *
+ * Returned Value:
+ * The Internet checksum of the given iob chain buffer.
+ *
+ ****************************************************************************/
+
+#if !defined(CONFIG_NET_ARCH_CHKSUM) && defined(CONFIG_MM_IOB)
+uint16_t net_chksum_iob(uint16_t sum, FAR struct iob_s *iob, uint16_t offset)
Review Comment:
add to Kconfig
##########
net/netdev/netdev_input.c:
##########
@@ -0,0 +1,117 @@
+/****************************************************************************
+ * net/netdev/netdev_input.c
+ *
+ * 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/netdev.h>
+
+#include "utils/utils.h"
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: netdev_input
+ *
+ * Description:
+ * This function will copy the flat buffer that does not support
+ * Scatter/gather to the iob vector buffer.
+ *
+ * Compatible with all old flat buffer NICs:
+ *
+ * [tcp|udp|icmp|...]ipv[4|6]_data_handler()
+ * | (iob_concat/append to readahead)
+ * |
+ * pkt/ipv[4/6]_in()/...
+ * |
+ * |
+ * netdev_input() // new interface, Scatter/gather flat/iobs
+ * |
+ * |
+ * pkt/ipv[4|6]_input()/...
+ * |
+ * |
+ * NICs io vector receive(Orignal flat buffer)
+ *
+ * Input Parameters:
+ * NULL
+ *
+ * Returned Value:
+ * Pointer to default network driver on success; null on failure
+ *
+ ****************************************************************************/
+
+int netdev_input(FAR struct net_driver_s *dev,
+ devif_poll_callback_t callback, bool reply)
Review Comment:
why need reply
##########
net/tcp/tcp_callback.c:
##########
@@ -139,6 +118,16 @@ uint16_t tcp_callback(FAR struct net_driver_s *dev,
uint16_t orig = flags;
#endif
+ /* Prepare device buffer */
+
+ if (dev->d_iob == NULL && netdev_iob_prepare(dev, false, 0) != OK)
Review Comment:
let's change bool argument of netdev_iob_prepare to try throttle if fail to
avoid the call it twice.
##########
net/Kconfig:
##########
@@ -117,6 +117,15 @@ config NET_GUARDSIZE
packet size will be chopped down to the size indicated in the
TCP
header.
+config NET_LL_GRUARDSIZE
+ int "Data Link Layer(L2) Guard size of Network buffer(IOB)"
+ default 14 if NET_ETHERNET
+ default 16
Review Comment:
should we use zero as default value since the ipforward isn't the normal case
--
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: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
