Re: [PATCH 09/15] net: hbl_en: add habanalabs Ethernet driver

Omer Shpigelman
2024-06-18T19:37:56Z

On 6/15/24 01:48, Joe Damato wrote:
> [You don't often get email from jdamato@fastly.com. 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@habana.ai>
>> Co-developed-by: Abhilash K V <kvabhilash@habana.ai>
>> Signed-off-by: Abhilash K V <kvabhilash@habana.ai>
>> Co-developed-by: Andrey Agranovich <aagranovich@habana.ai>
>> Signed-off-by: Andrey Agranovich <aagranovich@habana.ai>
>> Co-developed-by: Bharat Jauhari <bjauhari@habana.ai>
>> Signed-off-by: Bharat Jauhari <bjauhari@habana.ai>
>> Co-developed-by: David Meriin <dmeriin@habana.ai>
>> Signed-off-by: David Meriin <dmeriin@habana.ai>
>> Co-developed-by: Sagiv Ozeri <sozeri@habana.ai>
>> Signed-off-by: Sagiv Ozeri <sozeri@habana.ai>
>> Co-developed-by: Zvika Yehudai <zyehudai@habana.ai>
>> Signed-off-by: Zvika Yehudai <zyehudai@habana.ai>

<...>

>> + 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.