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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.




[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Photo]     [Yosemite News]     [Yosemite Photos]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux