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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Tue, 18 Jun 2024 19:37:36 +0000
From: Omer Shpigelman <oshpigelman@...ana.ai>
To: Joe Damato <jdamato@...tly.com>,
        "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>,
        "linux-rdma@...r.kernel.org"
	<linux-rdma@...r.kernel.org>,
        "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>,
        "dri-devel@...ts.freedesktop.org"
	<dri-devel@...ts.freedesktop.org>,
        "ogabbay@...nel.org" <ogabbay@...nel.org>,
        Zvika Yehudai <zyehudai@...ana.ai>
Subject: Re: [PATCH 09/15] net: hbl_en: add habanalabs Ethernet driver

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