On 6/15/24 01:48, Joe Damato wrote: > [You don't often get email from jdamato@xxxxxxxxxx. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ] > > On Thu, Jun 13, 2024 at 11:22:02AM +0300, Omer Shpigelman wrote: >> This ethernet driver is initialized via auxiliary bus by the hbl_cn >> driver. >> It serves mainly for control operations that are needed for AI scaling. >> >> Signed-off-by: Omer Shpigelman <oshpigelman@xxxxxxxxx> >> Co-developed-by: Abhilash K V <kvabhilash@xxxxxxxxx> >> Signed-off-by: Abhilash K V <kvabhilash@xxxxxxxxx> >> Co-developed-by: Andrey Agranovich <aagranovich@xxxxxxxxx> >> Signed-off-by: Andrey Agranovich <aagranovich@xxxxxxxxx> >> Co-developed-by: Bharat Jauhari <bjauhari@xxxxxxxxx> >> Signed-off-by: Bharat Jauhari <bjauhari@xxxxxxxxx> >> Co-developed-by: David Meriin <dmeriin@xxxxxxxxx> >> Signed-off-by: David Meriin <dmeriin@xxxxxxxxx> >> Co-developed-by: Sagiv Ozeri <sozeri@xxxxxxxxx> >> Signed-off-by: Sagiv Ozeri <sozeri@xxxxxxxxx> >> Co-developed-by: Zvika Yehudai <zyehudai@xxxxxxxxx> >> Signed-off-by: Zvika Yehudai <zyehudai@xxxxxxxxx> <...> >> + if (hdev->poll_enable) >> + skb = __netdev_alloc_skb_ip_align(ndev, pkt_size, GFP_KERNEL); >> + else >> + skb = napi_alloc_skb(&port->napi, pkt_size); >> + >> + if (!skb) { >> + atomic64_inc(&port->net_stats.rx_dropped); > > It seems like buffer exhaustion (!skb) would be rx_missed_errors? > > The documentation in include/uapi/linux/if_link.h: > > * @rx_dropped: Number of packets received but not processed, > * e.g. due to lack of resources or unsupported protocol. > * For hardware interfaces this counter may include packets discarded > * due to L2 address filtering but should not include packets dropped > * by the device due to buffer exhaustion which are counted separately in > * @rx_missed_errors (since procfs folds those two counters together). > > But, I don't know much about your hardware so I could be wrong. > Per my understanding rx_dropped should be used here. According the doc you posted, rx_dropped should be used in case of dropped packets due to lack of resources, while rx_missed_errors should be used for packets that were dropped by the device due to buffer exhaustion, not by the driver. Please correct me if I misunderstood something. >> + break; >> + } >> + >> + skb_copy_to_linear_data(skb, pkt_addr, pkt_size); >> + skb_put(skb, pkt_size); >> + >> + if (is_pkt_swap_enabled(hdev)) >> + dump_swap_pkt(port, skb); >> + >> + skb->protocol = eth_type_trans(skb, ndev); >> + >> + /* Zero the packet buffer memory to avoid leak in case of wrong >> + * size is used when next packet populates the same memory >> + */ >> + memset(pkt_addr, 0, pkt_size); >> + >> + /* polling is done in thread context and hence BH should be disabled */ >> + if (hdev->poll_enable) >> + local_bh_disable(); >> + >> + rc = netif_receive_skb(skb); > > Is there any reason in particular to call netif_receive_skb instead of > napi_gro_receive ? > As you can see, we also support polling mode which is a non-NAPI flow. We could use napi_gro_receive() for NAPI flow and netif_receive_skb() for polling mode but we don't support RX checksum offload anyway. >> + >> + if (hdev->poll_enable) >> + local_bh_enable(); <...> >> + pkt_count = hbl_en_handle_rx(port, budget); >> + >> + /* If budget not fully consumed, exit the polling mode */ >> + if (pkt_count < budget) { >> + napi_complete_done(napi, pkt_count); > > I believe this code might be incorrect and that it should be: > > if (napi_complete_done(napi, pkt_done)) > hdev->asic_funcs.reenable_rx_irq(port); > Thanks, I'll add the condition.
