Message ID | 20240130110944.26771-3-r-gunasekaran@ti.com (mailing list archive) |
---|---|
State | RFC |
Delegated to: | Netdev Maintainers |
Headers | show |
Series | RPMsg based shared memory ethernet driver | expand |
On Tue, Jan 30, 2024 at 04:39:44PM +0530, Ravi Gunasekaran wrote: > Register the RPMsg driver as network device and add support for > basic ethernet functionality by using the shared memory for data > plane. > > The shared memory layout is as below, with the region between > PKT_1_LEN to PKT_N modelled as circular buffer. > > ------------------------- > | HEAD | > ------------------------- > | TAIL | > ------------------------- > | PKT_1_LEN | > | PKT_1 | > ------------------------- > | PKT_2_LEN | > | PKT_2 | > ------------------------- > | . | > | . | > ------------------------- > | PKT_N_LEN | > | PKT_N | > ------------------------- > > The offset between the HEAD and TAIL is polled to process the Rx packets. > > Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> > Signed-off-by: Ravi Gunasekaran <r-gunasekaran@ti.com> Hi Ravi, some feedback, mainly regarding issues flagged by linters and static analysis. > --- > drivers/net/ethernet/ti/inter-core-virt-eth.c | 316 ++++++++++++++++++ > drivers/net/ethernet/ti/inter-core-virt-eth.h | 16 + > 2 files changed, 332 insertions(+) > > diff --git a/drivers/net/ethernet/ti/inter-core-virt-eth.c b/drivers/net/ethernet/ti/inter-core-virt-eth.c > index d3b689eab1c0..735482001f4d 100644 > --- a/drivers/net/ethernet/ti/inter-core-virt-eth.c > +++ b/drivers/net/ethernet/ti/inter-core-virt-eth.c > @@ -6,6 +6,50 @@ > > #include "inter-core-virt-eth.h" > > +#define ICVE_MIN_PACKET_SIZE ETH_ZLEN > +#define ICVE_MAX_PACKET_SIZE (ETH_FRAME_LEN + ETH_FCS_LEN) > +#define ICVE_MAX_TX_QUEUES 1 > +#define ICVE_MAX_RX_QUEUES 1 > + > +#define TEST_DEBUG 1 Please remove TEST_DEBUG and associated code. This does not seem appropriate for upstream. > + > +#ifdef TEST_DEBUG > +#define ICVE_MAX_BUFFERS 100 //TODO : Set to power of 2 to leverage shift operations > +#endif > + > +#define PKT_LEN_SIZE_TYPE sizeof(u32) > + > +/* 4 bytes to hold packet length and ICVE_MAX_PACKET_SIZE to hold packet */ > +#define ICVE_BUFFER_SIZE (ICVE_MAX_PACKET_SIZE + PKT_LEN_SIZE_TYPE) > + > +#define RX_POLL_TIMEOUT 250 > + > +#define icve_ndev_to_priv(ndev) \ > + ((struct icve_ndev_priv *)netdev_priv(ndev)) > +#define icve_ndev_to_port(ndev) (icve_ndev_to_priv(ndev)->port) > +#define icve_ndev_to_common(ndev) (icve_ndev_to_port(ndev)->common) > + > +static void icve_rx_timer(struct timer_list *timer) > +{ > + struct icve_port *port = from_timer(port, timer, rx_timer); > + struct napi_struct *napi; > + int num_pkts = 0; > + u32 head, tail; > + > + head = port->rx_buffer->head; > + tail = port->rx_buffer->tail; > + > + num_pkts = tail - head; > + num_pkts = num_pkts >= 0 ? num_pkts : (num_pkts + port->icve_max_buffers); Networking (still) prefers code to be < 80 columns wide. In this case I'd probably go for (completely untested!): num_pkts = tail - head; if (num_pkts <= 0) num_pkts = num_pkts + port->icve_max_buffers; Please consider running checkpatch as it is run by the CI. [1] https://github.com/linux-netdev/nipa/blob/main/tests/patch/checkpatch/checkpatch.sh` > + > + napi = &port->rx_napi; > + if (num_pkts && likely(napi_schedule_prep(napi))) { > + __napi_schedule(napi); > + } else { > + mod_timer(&port->rx_timer, RX_POLL_TIMEOUT); Smatch warns that mod_timer takes an absolute time rather than an offset. So, perhaps (completely untested!): mod_timer(&port->rx_timer, jiffies + RX_POLL_TIMEOUT); > + } If any arm of an if statement has {} then all should, but {} is not needed unless there is more than one statement convered by conditions. So (completely untested!): if (num_pkts && likely(napi_schedule_prep(napi))) __napi_schedule(napi); else mod_timer(&port->rx_timer, jiffies + RX_POLL_TIMEOUT); ... > @@ -85,11 +145,262 @@ static int create_request(struct icve_common *common, enum icve_rpmsg_type rpmsg > return ret; > } > > +static int icve_rx_packets(struct napi_struct *napi, int budget) > +{ > + struct icve_port *port = container_of(napi, struct icve_port, rx_napi); > + u32 count, process_pkts; > + struct sk_buff *skb; > + u32 head, tail; > + u32 pkt_len; > + int num_pkts; nit: Please use reverse xmas tree - longest line to shortest - for local variables in Networking code. This tool can help achieve that. * https://github.com/ecree-solarflare/xmastree > + > + head = port->rx_buffer->head; > + tail = port->rx_buffer->tail; > + > + num_pkts = tail - head; > + num_pkts = num_pkts >= 0 ? num_pkts : (num_pkts + port->icve_max_buffers); > + process_pkts = min(num_pkts, budget); > + count = 0; > + while (count < process_pkts) { > + memcpy((void *)&pkt_len, > + (void *)port->rx_buffer->base_addr + ((head + count) * ICVE_BUFFER_SIZE), > + PKT_LEN_SIZE_TYPE); I don't think there is any need to cast to (void *) like this. ... > +static netdev_tx_t icve_start_xmit(struct sk_buff *skb, struct net_device *ndev) > +{ > + struct icve_port *port = icve_ndev_to_port(ndev); > + struct ethhdr *ether; > + u32 head, tail; > + u32 num_pkts; > + u32 len; > + > + ether = eth_hdr(skb); ether is assigned but otherwise unused in this function. Flaged by W=1 builds using both gcc-13 and clang-17. > + len = skb_headlen(skb); > + > + head = port->tx_buffer->head; > + tail = port->tx_buffer->tail; > + > + /* If the buffer queue is full, then drop packet */ > + num_pkts = tail - head; > + num_pkts = num_pkts >= 0 ? num_pkts : (num_pkts + port->icve_max_buffers); num_pkts is unsigned, so the condition above is always true. Flagged by Coccinelle and Smatch. > + if ((num_pkts + 1) == port->icve_max_buffers) { > + netdev_warn(ndev, "Tx buffer full\n"); > + goto ring_full; > + } > + > + /* Copy length */ > + memcpy((void *)port->tx_buffer->base_addr + (port->tx_buffer->tail * ICVE_BUFFER_SIZE), > + (void *)&len, PKT_LEN_SIZE_TYPE); > + > + /* Copy data to shared mem */ > + memcpy((void *)(port->tx_buffer->base_addr + PKT_LEN_SIZE_TYPE) + > + (port->tx_buffer->tail * ICVE_BUFFER_SIZE), > + (void *)skb->data, len); > + > +#ifdef TEST_DEBUG > + /* For quick Rx path testing, inject Tx pkt back into network */ > + test_tx_rx_path(skb, ndev); > +#endif > + port->tx_buffer->tail = (port->tx_buffer->tail + 1) % port->icve_max_buffers; > + > + dev_consume_skb_any(skb); > + > + return NETDEV_TX_OK; > + > +ring_full: > + return NETDEV_TX_BUSY; > +} > + > +static int icve_set_mac_address(struct net_device *ndev, void *addr) > +{ > + eth_mac_addr(ndev, addr); > + > + /* TODO : Inform remote core about MAC address change */ > + return 0; > +} > + > +static const struct net_device_ops icve_netdev_ops = { > + .ndo_open = icve_ndo_open, > + .ndo_stop = icve_ndo_stop, > + .ndo_start_xmit = icve_start_xmit, > + .ndo_set_mac_address = icve_set_mac_address, > +}; > + > +static int icve_init_ndev(struct icve_common *common) > +{ > + struct device *dev = &common->rpdev->dev; > + struct icve_ndev_priv *ndev_priv; > + struct icve_port *port; > + static u32 port_id; > + int err; > + > + port = common->port; > + port->common = common; > + port->port_id = port_id++; > + > + port->ndev = devm_alloc_etherdev_mqs(common->dev, sizeof(*ndev_priv), > + ICVE_MAX_TX_QUEUES, > + ICVE_MAX_RX_QUEUES); > + > + if (!port->ndev) { > + dev_err(dev, "error allocating net_device\n"); > + return -ENOMEM; > + } > + > + ndev_priv = netdev_priv(port->ndev); > + ndev_priv->port = port; > + SET_NETDEV_DEV(port->ndev, dev); > + > + port->ndev->min_mtu = ICVE_MIN_PACKET_SIZE; > + port->ndev->max_mtu = ICVE_MAX_PACKET_SIZE; > + port->ndev->netdev_ops = &icve_netdev_ops; > + > +#ifdef TEST_DEBUG > + /* Allocate memory to test without actual RPMsg handshaking */ > + port->tx_buffer = devm_kzalloc(dev, sizeof(port->tx_buffer), > + GFP_KERNEL); (I think this code should be removed, but in any case I'll flag the problems that I am aware of.) This allocates the size of the port->tx_buffer pointer, rather than the size of port->tx_buffer itself (4 or 8 bytes instead of 12 or 16 bytes). So perhaps it should be (completely untested!): port->tx_buffer = devm_kzalloc(dev, sizeof(*port->tx_buffer), GFP_KERNEL); Flagged by Sparse. > + if (!port->tx_buffer) { > + dev_err(dev, "Memory not available\n"); Out of memory messages will be logged by the code. So there is no need for the dev_err() call above. > + return -ENOMEM; > + } > + > + port->tx_buffer->base_addr = devm_kzalloc(dev, ICVE_BUFFER_SIZE * ICVE_MAX_BUFFERS, > + GFP_KERNEL); > + if (!port->tx_buffer->base_addr) { > + dev_err(dev, "Memory not available\n"); > + return -ENOMEM; > + } > + > + port->rx_buffer = devm_kzalloc(dev, sizeof(port->rx_buffer), > + GFP_KERNEL); > + if (!port->rx_buffer) { > + dev_err(dev, "Memory not available\n"); > + return -ENOMEM; > + }; > + > + port->rx_buffer->base_addr = devm_kzalloc(dev, ICVE_BUFFER_SIZE * ICVE_MAX_BUFFERS, > + GFP_KERNEL); > + if (!port->rx_buffer->base_addr) { > + dev_err(dev, "Memory not available\n"); > + return -ENOMEM; > + } > + > + port->icve_max_buffers = ICVE_MAX_BUFFERS; > +#else > + /* Shared memory details will be sent by the remote core. > + * So turn off the carrier, until both the virtual port and > + * remote core is ready > + */ > + netif_carrier_off(port->ndev); > + > +#endif > + err = register_netdev(port->ndev); > + > + if (err) > + dev_err(dev, "error registering icve net device %d\n", err); > + > + return 0; > +} > + ... > diff --git a/drivers/net/ethernet/ti/inter-core-virt-eth.h b/drivers/net/ethernet/ti/inter-core-virt-eth.h ... > @@ -70,7 +78,11 @@ struct shared_mem { > struct icve_port { > struct shared_mem *tx_buffer; /* Write buffer for data to be consumed remote side */ > struct shared_mem *rx_buffer; /* Read buffer for data to be consumed by this driver */ > + struct timer_list rx_timer; > struct icve_common *common; > + struct napi_struct rx_napi; > + u8 local_mac_addr[ETH_ALEN]; local_mac_addr does not appear to be used in this patch. If so, please drop it and add it when it is needed. > + struct net_device *ndev; > u32 icve_max_buffers; > u32 port_id; /* Unique ID for the port : TODO: Define range for use by Linux and non linux */ > ...
On 2/1/2024 6:49 PM, Simon Horman wrote: > On Tue, Jan 30, 2024 at 04:39:44PM +0530, Ravi Gunasekaran wrote: >> Register the RPMsg driver as network device and add support for >> basic ethernet functionality by using the shared memory for data >> plane. >> >> The shared memory layout is as below, with the region between >> PKT_1_LEN to PKT_N modelled as circular buffer. >> >> ------------------------- >> | HEAD | >> ------------------------- >> | TAIL | >> ------------------------- >> | PKT_1_LEN | >> | PKT_1 | >> ------------------------- >> | PKT_2_LEN | >> | PKT_2 | >> ------------------------- >> | . | >> | . | >> ------------------------- >> | PKT_N_LEN | >> | PKT_N | >> ------------------------- >> >> The offset between the HEAD and TAIL is polled to process the Rx packets. >> >> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> >> Signed-off-by: Ravi Gunasekaran <r-gunasekaran@ti.com> > Hi Ravi, > > some feedback, mainly regarding issues flagged by linters and static analysis. > >> --- >> drivers/net/ethernet/ti/inter-core-virt-eth.c | 316 ++++++++++++++++++ >> drivers/net/ethernet/ti/inter-core-virt-eth.h | 16 + >> 2 files changed, 332 insertions(+) >> >> diff --git a/drivers/net/ethernet/ti/inter-core-virt-eth.c b/drivers/net/ethernet/ti/inter-core-virt-eth.c >> index d3b689eab1c0..735482001f4d 100644 >> --- a/drivers/net/ethernet/ti/inter-core-virt-eth.c >> +++ b/drivers/net/ethernet/ti/inter-core-virt-eth.c >> @@ -6,6 +6,50 @@ >> >> #include "inter-core-virt-eth.h" >> >> +#define ICVE_MIN_PACKET_SIZE ETH_ZLEN >> +#define ICVE_MAX_PACKET_SIZE (ETH_FRAME_LEN + ETH_FCS_LEN) >> +#define ICVE_MAX_TX_QUEUES 1 >> +#define ICVE_MAX_RX_QUEUES 1 >> + >> +#define TEST_DEBUG 1 > Please remove TEST_DEBUG and associated code. > This does not seem appropriate for upstream. Sure, I will do so. > >> + >> +#ifdef TEST_DEBUG >> +#define ICVE_MAX_BUFFERS 100 //TODO : Set to power of 2 to leverage shift operations >> +#endif >> + >> +#define PKT_LEN_SIZE_TYPE sizeof(u32) >> + >> +/* 4 bytes to hold packet length and ICVE_MAX_PACKET_SIZE to hold packet */ >> +#define ICVE_BUFFER_SIZE (ICVE_MAX_PACKET_SIZE + PKT_LEN_SIZE_TYPE) >> + >> +#define RX_POLL_TIMEOUT 250 >> + >> +#define icve_ndev_to_priv(ndev) \ >> + ((struct icve_ndev_priv *)netdev_priv(ndev)) >> +#define icve_ndev_to_port(ndev) (icve_ndev_to_priv(ndev)->port) >> +#define icve_ndev_to_common(ndev) (icve_ndev_to_port(ndev)->common) >> + >> +static void icve_rx_timer(struct timer_list *timer) >> +{ >> + struct icve_port *port = from_timer(port, timer, rx_timer); >> + struct napi_struct *napi; >> + int num_pkts = 0; >> + u32 head, tail; >> + >> + head = port->rx_buffer->head; >> + tail = port->rx_buffer->tail; >> + >> + num_pkts = tail - head; >> + num_pkts = num_pkts >= 0 ? num_pkts : (num_pkts + port->icve_max_buffers); > Networking (still) prefers code to be < 80 columns wide. > In this case I'd probably go for (completely untested!): I will take care of this from now on. > num_pkts = tail - head; > if (num_pkts <= 0) > num_pkts = num_pkts + port->icve_max_buffers; > > Please consider running checkpatch as it is run by the CI. > > [1] https://github.com/linux-netdev/nipa/blob/main/tests/patch/checkpatch/checkpatch.sh` > >> + >> + napi = &port->rx_napi; >> + if (num_pkts && likely(napi_schedule_prep(napi))) { >> + __napi_schedule(napi); >> + } else { >> + mod_timer(&port->rx_timer, RX_POLL_TIMEOUT); > Smatch warns that mod_timer takes an absolute time rather than an offset. > So, perhaps (completely untested!): > > mod_timer(&port->rx_timer, jiffies + RX_POLL_TIMEOUT); > >> + } > If any arm of an if statement has {} then all should, > but {} is not needed unless there is more than one statement > convered by conditions. > > So (completely untested!): > > if (num_pkts && likely(napi_schedule_prep(napi))) > __napi_schedule(napi); > else > mod_timer(&port->rx_timer, jiffies + RX_POLL_TIMEOUT); > > ... Noted. I had added debug prints between the {}, forgot take care the single statement rule while cleaning up. Not an excuse though. >> @@ -85,11 +145,262 @@ static int create_request(struct icve_common *common, enum icve_rpmsg_type rpmsg >> return ret; >> } >> >> +static int icve_rx_packets(struct napi_struct *napi, int budget) >> +{ >> + struct icve_port *port = container_of(napi, struct icve_port, rx_napi); >> + u32 count, process_pkts; >> + struct sk_buff *skb; >> + u32 head, tail; >> + u32 pkt_len; >> + int num_pkts; > nit: Please use reverse xmas tree - longest line to shortest - for local > variables in Networking code. > > This tool can help achieve that. > * https://github.com/ecree-solarflare/xmastree Thanks for sharing the link. I will use it from now on. >> + >> + head = port->rx_buffer->head; >> + tail = port->rx_buffer->tail; >> + >> + num_pkts = tail - head; >> + num_pkts = num_pkts >= 0 ? num_pkts : (num_pkts + port->icve_max_buffers); >> + process_pkts = min(num_pkts, budget); >> + count = 0; >> + while (count < process_pkts) { >> + memcpy((void *)&pkt_len, >> + (void *)port->rx_buffer->base_addr + ((head + count) * ICVE_BUFFER_SIZE), >> + PKT_LEN_SIZE_TYPE); > I don't think there is any need to cast to (void *) like this. > > ... Ok. > >> +static netdev_tx_t icve_start_xmit(struct sk_buff *skb, struct net_device *ndev) >> +{ >> + struct icve_port *port = icve_ndev_to_port(ndev); >> + struct ethhdr *ether; >> + u32 head, tail; >> + u32 num_pkts; >> + u32 len; >> + >> + ether = eth_hdr(skb); > ether is assigned but otherwise unused in this function. > > Flaged by W=1 builds using both gcc-13 and clang-17. > >> + len = skb_headlen(skb); >> + >> + head = port->tx_buffer->head; >> + tail = port->tx_buffer->tail; >> + >> + /* If the buffer queue is full, then drop packet */ >> + num_pkts = tail - head; >> + num_pkts = num_pkts >= 0 ? num_pkts : (num_pkts + port->icve_max_buffers); > num_pkts is unsigned, so the condition above is always true. > > Flagged by Coccinelle and Smatch. > >> + if ((num_pkts + 1) == port->icve_max_buffers) { >> + netdev_warn(ndev, "Tx buffer full\n"); >> + goto ring_full; >> + } >> + >> + /* Copy length */ >> + memcpy((void *)port->tx_buffer->base_addr + (port->tx_buffer->tail * ICVE_BUFFER_SIZE), >> + (void *)&len, PKT_LEN_SIZE_TYPE); >> + >> + /* Copy data to shared mem */ >> + memcpy((void *)(port->tx_buffer->base_addr + PKT_LEN_SIZE_TYPE) + >> + (port->tx_buffer->tail * ICVE_BUFFER_SIZE), >> + (void *)skb->data, len); >> + >> +#ifdef TEST_DEBUG >> + /* For quick Rx path testing, inject Tx pkt back into network */ >> + test_tx_rx_path(skb, ndev); >> +#endif >> + port->tx_buffer->tail = (port->tx_buffer->tail + 1) % port->icve_max_buffers; >> + >> + dev_consume_skb_any(skb); >> + >> + return NETDEV_TX_OK; >> + >> +ring_full: >> + return NETDEV_TX_BUSY; >> +} >> + >> +static int icve_set_mac_address(struct net_device *ndev, void *addr) >> +{ >> + eth_mac_addr(ndev, addr); >> + >> + /* TODO : Inform remote core about MAC address change */ >> + return 0; >> +} >> + >> +static const struct net_device_ops icve_netdev_ops = { >> + .ndo_open = icve_ndo_open, >> + .ndo_stop = icve_ndo_stop, >> + .ndo_start_xmit = icve_start_xmit, >> + .ndo_set_mac_address = icve_set_mac_address, >> +}; >> + >> +static int icve_init_ndev(struct icve_common *common) >> +{ >> + struct device *dev = &common->rpdev->dev; >> + struct icve_ndev_priv *ndev_priv; >> + struct icve_port *port; >> + static u32 port_id; >> + int err; >> + >> + port = common->port; >> + port->common = common; >> + port->port_id = port_id++; >> + >> + port->ndev = devm_alloc_etherdev_mqs(common->dev, sizeof(*ndev_priv), >> + ICVE_MAX_TX_QUEUES, >> + ICVE_MAX_RX_QUEUES); >> + >> + if (!port->ndev) { >> + dev_err(dev, "error allocating net_device\n"); >> + return -ENOMEM; >> + } >> + >> + ndev_priv = netdev_priv(port->ndev); >> + ndev_priv->port = port; >> + SET_NETDEV_DEV(port->ndev, dev); >> + >> + port->ndev->min_mtu = ICVE_MIN_PACKET_SIZE; >> + port->ndev->max_mtu = ICVE_MAX_PACKET_SIZE; >> + port->ndev->netdev_ops = &icve_netdev_ops; >> + >> +#ifdef TEST_DEBUG >> + /* Allocate memory to test without actual RPMsg handshaking */ >> + port->tx_buffer = devm_kzalloc(dev, sizeof(port->tx_buffer), >> + GFP_KERNEL); > (I think this code should be removed, but in any case I'll flag > the problems that I am aware of.) > > This allocates the size of the port->tx_buffer pointer, rather than the > size of port->tx_buffer itself (4 or 8 bytes instead of 12 or 16 bytes). > So perhaps it should be (completely untested!): > > port->tx_buffer = devm_kzalloc(dev, sizeof(*port->tx_buffer), > GFP_KERNEL); > > Flagged by Sparse. My bad, a mistake on my part. > >> + if (!port->tx_buffer) { >> + dev_err(dev, "Memory not available\n"); > Out of memory messages will be logged by the code. > So there is no need for the dev_err() call above. > >> + return -ENOMEM; >> + } >> + >> + port->tx_buffer->base_addr = devm_kzalloc(dev, ICVE_BUFFER_SIZE * ICVE_MAX_BUFFERS, >> + GFP_KERNEL); >> + if (!port->tx_buffer->base_addr) { >> + dev_err(dev, "Memory not available\n"); >> + return -ENOMEM; >> + } >> + >> + port->rx_buffer = devm_kzalloc(dev, sizeof(port->rx_buffer), >> + GFP_KERNEL); >> + if (!port->rx_buffer) { >> + dev_err(dev, "Memory not available\n"); >> + return -ENOMEM; >> + }; >> + >> + port->rx_buffer->base_addr = devm_kzalloc(dev, ICVE_BUFFER_SIZE * ICVE_MAX_BUFFERS, >> + GFP_KERNEL); >> + if (!port->rx_buffer->base_addr) { >> + dev_err(dev, "Memory not available\n"); >> + return -ENOMEM; >> + } >> + >> + port->icve_max_buffers = ICVE_MAX_BUFFERS; >> +#else >> + /* Shared memory details will be sent by the remote core. >> + * So turn off the carrier, until both the virtual port and >> + * remote core is ready >> + */ >> + netif_carrier_off(port->ndev); >> + >> +#endif >> + err = register_netdev(port->ndev); >> + >> + if (err) >> + dev_err(dev, "error registering icve net device %d\n", err); >> + >> + return 0; >> +} >> + > ... > >> diff --git a/drivers/net/ethernet/ti/inter-core-virt-eth.h b/drivers/net/ethernet/ti/inter-core-virt-eth.h > ... > >> @@ -70,7 +78,11 @@ struct shared_mem { >> struct icve_port { >> struct shared_mem *tx_buffer; /* Write buffer for data to be consumed remote side */ >> struct shared_mem *rx_buffer; /* Read buffer for data to be consumed by this driver */ >> + struct timer_list rx_timer; >> struct icve_common *common; >> + struct napi_struct rx_napi; >> + u8 local_mac_addr[ETH_ALEN]; > local_mac_addr does not appear to be used in this patch. > If so, please drop it and add it when it is needed. Noted. > >> + struct net_device *ndev; >> u32 icve_max_buffers; >> u32 port_id; /* Unique ID for the port : TODO: Define range for use by Linux and non linux */ >> > ... Thanks for taking time to review the patches. The primary intention of this series was to know if the RPMsg based approach would be upstream friendly or not. But I would not like to use that as an excuse for not fixing checks/warnings/errors reported by checkpatch completely. Even though if its RFC, I will treat it as an actual upstream patch and address the checkpatch/smatch/sparse findings or atleast mention in the cover letter that the findings have not been fully addressed. I apologize for the inconvenience caused and be careful in future.
On Thu, Feb 01, 2024 at 07:54:24PM +0530, Ravi Gunasekaran wrote: ... > Thanks for taking time to review the patches. > > The primary intention of this series was to know if the RPMsg based approach > would be upstream friendly or not. But I would not like to use that as an excuse > for not fixing checks/warnings/errors reported by checkpatch completely. > Even though if its RFC, I will treat it as an actual upstream patch and address the > checkpatch/smatch/sparse findings or atleast mention in the cover letter that the > findings have not been fully addressed. Understood. TBH, I am unsure of the value of this kind of review for an RFC - I understand it is important to get the bigger picture questions out of the way at this point. ...
diff --git a/drivers/net/ethernet/ti/inter-core-virt-eth.c b/drivers/net/ethernet/ti/inter-core-virt-eth.c index d3b689eab1c0..735482001f4d 100644 --- a/drivers/net/ethernet/ti/inter-core-virt-eth.c +++ b/drivers/net/ethernet/ti/inter-core-virt-eth.c @@ -6,6 +6,50 @@ #include "inter-core-virt-eth.h" +#define ICVE_MIN_PACKET_SIZE ETH_ZLEN +#define ICVE_MAX_PACKET_SIZE (ETH_FRAME_LEN + ETH_FCS_LEN) +#define ICVE_MAX_TX_QUEUES 1 +#define ICVE_MAX_RX_QUEUES 1 + +#define TEST_DEBUG 1 + +#ifdef TEST_DEBUG +#define ICVE_MAX_BUFFERS 100 //TODO : Set to power of 2 to leverage shift operations +#endif + +#define PKT_LEN_SIZE_TYPE sizeof(u32) + +/* 4 bytes to hold packet length and ICVE_MAX_PACKET_SIZE to hold packet */ +#define ICVE_BUFFER_SIZE (ICVE_MAX_PACKET_SIZE + PKT_LEN_SIZE_TYPE) + +#define RX_POLL_TIMEOUT 250 + +#define icve_ndev_to_priv(ndev) \ + ((struct icve_ndev_priv *)netdev_priv(ndev)) +#define icve_ndev_to_port(ndev) (icve_ndev_to_priv(ndev)->port) +#define icve_ndev_to_common(ndev) (icve_ndev_to_port(ndev)->common) + +static void icve_rx_timer(struct timer_list *timer) +{ + struct icve_port *port = from_timer(port, timer, rx_timer); + struct napi_struct *napi; + int num_pkts = 0; + u32 head, tail; + + head = port->rx_buffer->head; + tail = port->rx_buffer->tail; + + num_pkts = tail - head; + num_pkts = num_pkts >= 0 ? num_pkts : (num_pkts + port->icve_max_buffers); + + napi = &port->rx_napi; + if (num_pkts && likely(napi_schedule_prep(napi))) { + __napi_schedule(napi); + } else { + mod_timer(&port->rx_timer, RX_POLL_TIMEOUT); + } +} + static int icve_rpmsg_cb(struct rpmsg_device *rpdev, void *data, int len, void *priv, u32 src) { struct icve_common *common = dev_get_drvdata(&rpdev->dev); @@ -57,6 +101,18 @@ static int icve_rpmsg_cb(struct rpmsg_device *rpdev, void *data, int len, void * break; } break; + case ICVE_NOTIFY_MSG: + rpmsg_type = msg->notify_msg.type; + switch (rpmsg_type) { + case ICVE_NOTIFY_REMOTE_READY: + /* Turn on carrier once remote core signals ready */ + netif_carrier_on(port->ndev); + break; + case ICVE_NOTIFY_PORT_UP: + case ICVE_NOTIFY_PORT_DOWN: + break; + } + break; default: dev_err(common->dev, "Invalid msg type\n"); break; @@ -77,6 +133,10 @@ static int create_request(struct icve_common *common, enum icve_rpmsg_type rpmsg case ICVE_REQ_SHM_INFO: msg->msg_hdr.msg_type = ICVE_REQUEST_MSG; break; + case ICVE_NOTIFY_PORT_UP: + case ICVE_NOTIFY_PORT_DOWN: + msg->msg_hdr.msg_type = ICVE_NOTIFY_MSG; + break; default: ret = -EINVAL; dev_err(common->dev, "Invalid RPMSG request\n"); @@ -85,11 +145,262 @@ static int create_request(struct icve_common *common, enum icve_rpmsg_type rpmsg return ret; } +static int icve_rx_packets(struct napi_struct *napi, int budget) +{ + struct icve_port *port = container_of(napi, struct icve_port, rx_napi); + u32 count, process_pkts; + struct sk_buff *skb; + u32 head, tail; + u32 pkt_len; + int num_pkts; + + head = port->rx_buffer->head; + tail = port->rx_buffer->tail; + + num_pkts = tail - head; + num_pkts = num_pkts >= 0 ? num_pkts : (num_pkts + port->icve_max_buffers); + process_pkts = min(num_pkts, budget); + count = 0; + while (count < process_pkts) { + memcpy((void *)&pkt_len, + (void *)port->rx_buffer->base_addr + ((head + count) * ICVE_BUFFER_SIZE), + PKT_LEN_SIZE_TYPE); + + /* Start building the skb */ + skb = napi_alloc_skb(napi, pkt_len); + skb->dev = port->ndev; + skb_put(skb, pkt_len); + + memcpy((void *)skb->data, + (void *)(port->rx_buffer->base_addr + PKT_LEN_SIZE_TYPE) + ((head + count) * ICVE_BUFFER_SIZE), + pkt_len); + + skb->protocol = eth_type_trans(skb, port->ndev); + + /* Push skb into network stack */ + napi_gro_receive(napi, skb); + + count++; + } + + if (num_pkts) { + port->rx_buffer->head = (port->rx_buffer->head + count) % port->icve_max_buffers; + + if (num_pkts < budget && napi_complete_done(napi, count)) + mod_timer(&port->rx_timer, RX_POLL_TIMEOUT); + } + return count; +} + +#ifdef TEST_DEBUG +static int test_tx_rx_path(struct sk_buff *skb, struct net_device *ndev) +{ + struct icve_port *port = icve_ndev_to_port(ndev); + u32 *data; + u32 len; + + len = skb_headlen(skb); + + /* Copy length */ + memcpy((void *)port->rx_buffer->base_addr + (port->rx_buffer->tail * ICVE_BUFFER_SIZE), + (void *)&len, PKT_LEN_SIZE_TYPE); + + /* Copy data to shared mem */ + memcpy((void *)(port->rx_buffer->base_addr + PKT_LEN_SIZE_TYPE) + (port->rx_buffer->tail * ICVE_BUFFER_SIZE), + (void *)skb->data, len); + + data = (u32 *)(port->rx_buffer->base_addr + (port->rx_buffer->tail * ICVE_BUFFER_SIZE)); + + port->rx_buffer->tail = (port->rx_buffer->tail + 1) % ICVE_MAX_BUFFERS; + + return 0; +} +#endif + +static int icve_ndo_open(struct net_device *ndev) +{ + struct icve_common *common = icve_ndev_to_common(ndev); + struct icve_port *port = icve_ndev_to_port(ndev); + unsigned long flags; + + /* Send a msg to remote core signalling that we are ready */ + spin_lock_irqsave(&common->send_msg_lock, flags); +#ifndef TEST_DEBUG + create_request(common, ICVE_NOTIFY_PORT_UP); + rpmsg_send(common->rpdev->ept, (void *)(&common->send_msg), sizeof(common->send_msg)); +#endif + spin_unlock_irqrestore(&common->send_msg_lock, flags); + + if (!(port->tx_buffer && port->rx_buffer)) { + netdev_err(ndev, "Shared memory not setup\n"); + return -EPERM; + } + + netif_napi_add(ndev, &port->rx_napi, icve_rx_packets); + napi_enable(&port->rx_napi); + + timer_setup(&port->rx_timer, icve_rx_timer, 0); + mod_timer(&port->rx_timer, RX_POLL_TIMEOUT); + + return 0; +} + +static int icve_ndo_stop(struct net_device *ndev) +{ + struct icve_common *common = icve_ndev_to_common(ndev); + unsigned long flags; + + spin_lock_irqsave(&common->send_msg_lock, flags); +#ifndef TEST_DEBUG + create_request(common, ICVE_NOTIFY_PORT_DOWN); + rpmsg_send(common->rpdev->ept, (void *)(&common->send_msg), sizeof(common->send_msg)); +#endif + spin_unlock_irqrestore(&common->send_msg_lock, flags); + return 0; +} + +static netdev_tx_t icve_start_xmit(struct sk_buff *skb, struct net_device *ndev) +{ + struct icve_port *port = icve_ndev_to_port(ndev); + struct ethhdr *ether; + u32 head, tail; + u32 num_pkts; + u32 len; + + ether = eth_hdr(skb); + len = skb_headlen(skb); + + head = port->tx_buffer->head; + tail = port->tx_buffer->tail; + + /* If the buffer queue is full, then drop packet */ + num_pkts = tail - head; + num_pkts = num_pkts >= 0 ? num_pkts : (num_pkts + port->icve_max_buffers); + if ((num_pkts + 1) == port->icve_max_buffers) { + netdev_warn(ndev, "Tx buffer full\n"); + goto ring_full; + } + + /* Copy length */ + memcpy((void *)port->tx_buffer->base_addr + (port->tx_buffer->tail * ICVE_BUFFER_SIZE), + (void *)&len, PKT_LEN_SIZE_TYPE); + + /* Copy data to shared mem */ + memcpy((void *)(port->tx_buffer->base_addr + PKT_LEN_SIZE_TYPE) + + (port->tx_buffer->tail * ICVE_BUFFER_SIZE), + (void *)skb->data, len); + +#ifdef TEST_DEBUG + /* For quick Rx path testing, inject Tx pkt back into network */ + test_tx_rx_path(skb, ndev); +#endif + port->tx_buffer->tail = (port->tx_buffer->tail + 1) % port->icve_max_buffers; + + dev_consume_skb_any(skb); + + return NETDEV_TX_OK; + +ring_full: + return NETDEV_TX_BUSY; +} + +static int icve_set_mac_address(struct net_device *ndev, void *addr) +{ + eth_mac_addr(ndev, addr); + + /* TODO : Inform remote core about MAC address change */ + return 0; +} + +static const struct net_device_ops icve_netdev_ops = { + .ndo_open = icve_ndo_open, + .ndo_stop = icve_ndo_stop, + .ndo_start_xmit = icve_start_xmit, + .ndo_set_mac_address = icve_set_mac_address, +}; + +static int icve_init_ndev(struct icve_common *common) +{ + struct device *dev = &common->rpdev->dev; + struct icve_ndev_priv *ndev_priv; + struct icve_port *port; + static u32 port_id; + int err; + + port = common->port; + port->common = common; + port->port_id = port_id++; + + port->ndev = devm_alloc_etherdev_mqs(common->dev, sizeof(*ndev_priv), + ICVE_MAX_TX_QUEUES, + ICVE_MAX_RX_QUEUES); + + if (!port->ndev) { + dev_err(dev, "error allocating net_device\n"); + return -ENOMEM; + } + + ndev_priv = netdev_priv(port->ndev); + ndev_priv->port = port; + SET_NETDEV_DEV(port->ndev, dev); + + port->ndev->min_mtu = ICVE_MIN_PACKET_SIZE; + port->ndev->max_mtu = ICVE_MAX_PACKET_SIZE; + port->ndev->netdev_ops = &icve_netdev_ops; + +#ifdef TEST_DEBUG + /* Allocate memory to test without actual RPMsg handshaking */ + port->tx_buffer = devm_kzalloc(dev, sizeof(port->tx_buffer), + GFP_KERNEL); + if (!port->tx_buffer) { + dev_err(dev, "Memory not available\n"); + return -ENOMEM; + } + + port->tx_buffer->base_addr = devm_kzalloc(dev, ICVE_BUFFER_SIZE * ICVE_MAX_BUFFERS, + GFP_KERNEL); + if (!port->tx_buffer->base_addr) { + dev_err(dev, "Memory not available\n"); + return -ENOMEM; + } + + port->rx_buffer = devm_kzalloc(dev, sizeof(port->rx_buffer), + GFP_KERNEL); + if (!port->rx_buffer) { + dev_err(dev, "Memory not available\n"); + return -ENOMEM; + }; + + port->rx_buffer->base_addr = devm_kzalloc(dev, ICVE_BUFFER_SIZE * ICVE_MAX_BUFFERS, + GFP_KERNEL); + if (!port->rx_buffer->base_addr) { + dev_err(dev, "Memory not available\n"); + return -ENOMEM; + } + + port->icve_max_buffers = ICVE_MAX_BUFFERS; +#else + /* Shared memory details will be sent by the remote core. + * So turn off the carrier, until both the virtual port and + * remote core is ready + */ + netif_carrier_off(port->ndev); + +#endif + err = register_netdev(port->ndev); + + if (err) + dev_err(dev, "error registering icve net device %d\n", err); + + return 0; +} + static int icve_rpmsg_probe(struct rpmsg_device *rpdev) { struct device *dev = &rpdev->dev; struct icve_common *common; unsigned long flags; + int ret; common = devm_kzalloc(&rpdev->dev, sizeof(*common), GFP_KERNEL); if (!common) @@ -104,6 +415,11 @@ static int icve_rpmsg_probe(struct rpmsg_device *rpdev) spin_lock_init(&common->send_msg_lock); spin_lock_init(&common->recv_msg_lock); + /* Register the network device */ + ret = icve_init_ndev(common); + if (ret) + return ret; + /* Send request to fetch shared memory details from remote core */ spin_lock_irqsave(&common->send_msg_lock, flags); create_request(common, ICVE_REQ_SHM_INFO); diff --git a/drivers/net/ethernet/ti/inter-core-virt-eth.h b/drivers/net/ethernet/ti/inter-core-virt-eth.h index 063cc371eeb3..c3386a5ff714 100644 --- a/drivers/net/ethernet/ti/inter-core-virt-eth.h +++ b/drivers/net/ethernet/ti/inter-core-virt-eth.h @@ -7,13 +7,16 @@ #ifndef __INTER_CORE_VIRT_ETH_H__ #define __INTER_CORE_VIRT_ETH_H__ +#include <linux/etherdevice.h> #include <linux/kernel.h> #include <linux/module.h> +#include <linux/netdevice.h> #include <linux/rpmsg.h> enum icve_msg_type { ICVE_REQUEST_MSG = 0, ICVE_RESPONSE_MSG, + ICVE_NOTIFY_MSG, }; enum icve_rpmsg_type { @@ -22,6 +25,11 @@ enum icve_rpmsg_type { /* Response types */ ICVE_RESP_SHM_INFO, + + /* Notification types */ + ICVE_NOTIFY_PORT_UP, + ICVE_NOTIFY_PORT_DOWN, + ICVE_NOTIFY_REMOTE_READY, }; struct icve_shm_info { @@ -70,7 +78,11 @@ struct shared_mem { struct icve_port { struct shared_mem *tx_buffer; /* Write buffer for data to be consumed remote side */ struct shared_mem *rx_buffer; /* Read buffer for data to be consumed by this driver */ + struct timer_list rx_timer; struct icve_common *common; + struct napi_struct rx_napi; + u8 local_mac_addr[ETH_ALEN]; + struct net_device *ndev; u32 icve_max_buffers; u32 port_id; /* Unique ID for the port : TODO: Define range for use by Linux and non linux */ @@ -86,4 +98,8 @@ struct icve_common { struct device *dev; } __packed; +struct icve_ndev_priv { + struct icve_port *port; +}; + #endif /* __INTER_CORE_VIRT_ETH_H__ */