Message-ID: <83a6029e-1e45-4ce7-99bb-a3643ddbf8ab@habana.ai>
Date: Mon, 17 Jun 2024 08:08:26 +0000
From: Omer Shpigelman <oshpigelman@...ana.ai>
To: Przemek Kitszel <przemyslaw.kitszel@...el.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>
CC: "ogabbay@...nel.org" <ogabbay@...nel.org>,
        Zvika Yehudai
	<zyehudai@...ana.ai>
Subject: Re: [PATCH 01/15] net: hbl_cn: add habanalabs Core Network driver

On 6/13/24 16:01, Przemek Kitszel wrote:
> [Some people who received this message don't often get email from przemyslaw.kitszel@...el.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> On 6/13/24 10:21, Omer Shpigelman wrote:
>> Add the hbl_cn driver which will serve both Ethernet and InfiniBand
>> drivers.
>> hbl_cn is the layer which is used by the satellite drivers for many shared
>> operations that are needed by both EN and IB subsystems like QPs, CQs etc.
>> The CN driver is initialized via auxiliary bus by the habanalabs driver.
>>
>> 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>
>> ---
>>   .../device_drivers/ethernet/index.rst         |    1 +
>>   .../device_drivers/ethernet/intel/hbl.rst     |   82 +
>>   MAINTAINERS                                   |   11 +
>>   drivers/net/ethernet/intel/Kconfig            |   20 +
>>   drivers/net/ethernet/intel/Makefile           |    1 +
>>   drivers/net/ethernet/intel/hbl_cn/Makefile    |    9 +
>>   .../net/ethernet/intel/hbl_cn/common/Makefile |    3 +
>>   .../net/ethernet/intel/hbl_cn/common/hbl_cn.c | 5954 +++++++++++++++++
>>   .../net/ethernet/intel/hbl_cn/common/hbl_cn.h | 1627 +++++
>>   .../ethernet/intel/hbl_cn/common/hbl_cn_drv.c |  220 +
>>   .../intel/hbl_cn/common/hbl_cn_memory.c       |   40 +
>>   .../ethernet/intel/hbl_cn/common/hbl_cn_phy.c |   33 +
>>   .../ethernet/intel/hbl_cn/common/hbl_cn_qp.c  |   13 +
>>   include/linux/habanalabs/cpucp_if.h           |  125 +-
>>   include/linux/habanalabs/hl_boot_if.h         |    9 +-
>>   include/linux/net/intel/cn.h                  |  474 ++
>>   include/linux/net/intel/cn_aux.h              |  298 +
>>   include/linux/net/intel/cni.h                 |  636 ++
>>   18 files changed, 9545 insertions(+), 11 deletions(-)
>
> this is a very big patch, it asks for a split; what's worse, it's
> proportional to the size of this series:
>  146 files changed, 148514 insertions(+), 70 deletions(-)
> which is just too big
>
> [...]
>

Yeah, well I'm limited to 15 patches per patch set according to the kernel
doc so I had to have this big patch.
Our changes are contained in 4 different drivers and all of the changes
should be merged together so the HW will be operational.
Hence I had to squeeze some code to a big patch.

>> +Support
>> +=======
>> +For general information, go to the Intel support website at:
>> +https://www.intel.com/support/
>> +
>> +If an issue is identified with the released source code on a supported kernel
>> +with a supported adapter, email the specific information related to the issue
>> +to intel-wired-lan@...ts.osuosl.org.
>
> I'm welcoming you to post next version of the driver to the IWL mailing
> list, and before that, to go through our Intel path for ethernet
> subsystem (rdma and a few smaller ones also go through that)
> (that starts internally, I will PM you the details)
>
> [...]
>

Ok, I'll go the Intel path first.

>> +++ b/drivers/net/ethernet/intel/hbl_cn/common/hbl_cn.c
>> @@ -0,0 +1,5954 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* Copyright 2020-2024 HabanaLabs, Ltd.
>> + * Copyright (C) 2023-2024, Intel Corporation.
>> + * All Rights Reserved.
>> + */
>> +
>> +#include "hbl_cn.h"
>> +
>> +#include <linux/file.h>
>> +#include <linux/module.h>
>> +#include <linux/overflow.h>
>> +#include <linux/pci.h>
>> +#include <linux/slab.h>
>> +
>> +#define NIC_MIN_WQS_PER_PORT         2
>> +
>> +#define NIC_SEQ_RESETS_TIMEOUT_MS    15000 /* 15 seconds */
>> +#define NIC_MAX_SEQ_RESETS           3
>> +
>> +#define HBL_CN_IPV4_PROTOCOL_UDP     17
>> +
>> +/* SOB mask is not expected to change across ASIC. Hence common defines. */
>> +#define NIC_SOB_INC_MASK             0x80000000
>> +#define NIC_SOB_VAL_MASK             0x7fff
>> +
>> +#define NIC_DUMP_QP_SZ                       SZ_4K
>> +
>> +#define HBL_AUX2NIC(aux_dev) \
>> +     ({ \
>> +             struct hbl_aux_dev *__aux_dev = (aux_dev); \
>> +             ((__aux_dev)->type == HBL_AUX_DEV_ETH) ? \
>> +             container_of(__aux_dev, struct hbl_cn_device, en_aux_dev) : \
>> +             container_of(__aux_dev, struct hbl_cn_device, ib_aux_dev); \
>> +     })
>
> this should be a function
>

I'll switch it to a function.

>> +
>> +#define RAND_STAT_CNT(cnt) \
>> +     do { \
>> +             u32 __cnt = get_random_u32(); \
>> +             (cnt) = __cnt; \
>> +             dev_info(hdev->dev, "port %d, %s: %u\n", port, #cnt, __cnt); \
>
> no way for such message, ditto for the function
>

The thing is that I'd like to print the counter name and it's value.
For that I need to stringify the counter name.
IMO it is nicer to have the current code rather than something like:

RAND_STAT_CNT(status->high_ber_reinit,
              __stringify(status->high_ber_reinit));
RAND_STAT_CNT(status->correctable_err_cnt,
              __stringify(status->correctable_err_cnt));

or:

RAND_STAT_CNT(status->high_ber_reinit, "high_ber_reinit");
RAND_STAT_CNT(status->correctable_err_cnt, "correctable_err_cnt");

But I'll change it if it's not common to print from macros.

>> +     } while (0)
>> +
>> +struct hbl_cn_stat hbl_cn_mac_fec_stats[] = {
>> +     {"correctable_errors", 0x2, 0x3},
>> +     {"uncorrectable_errors", 0x4, 0x5}
>> +};
>> +
>> +struct hbl_cn_stat hbl_cn_mac_stats_rx[] = {
>> +     {"Octets", 0x0},
>> +     {"OctetsReceivedOK", 0x4},
>> +     {"aAlignmentErrors", 0x8},
>> +     {"aPAUSEMACCtrlFramesReceived", 0xC},
>> +     {"aFrameTooLongErrors", 0x10},
>> +     {"aInRangeLengthErrors", 0x14},
>> +     {"aFramesReceivedOK", 0x18},
>> +     {"aFrameCheckSequenceErrors", 0x1C},
>> +     {"VLANReceivedOK", 0x20},
>> +     {"ifInErrors", 0x24},
>> +     {"ifInUcastPkts", 0x28},
>> +     {"ifInMulticastPkts", 0x2C},
>> +     {"ifInBroadcastPkts", 0x30},
>> +     {"DropEvents", 0x34},
>> +     {"Pkts", 0x38},
>> +     {"UndersizePkts", 0x3C},
>> +     {"Pkts64Octets", 0x40},
>> +     {"Pkts65to127Octets", 0x44},
>> +     {"Pkts128to255Octets", 0x48},
>> +     {"Pkts256to511Octets", 0x4C},
>> +     {"Pkts512to1023Octets", 0x50},
>> +     {"Pkts1024to1518Octets", 0x54},
>> +     {"Pkts1519toMaxOctets", 0x58},
>> +     {"OversizePkts", 0x5C},
>> +     {"Jabbers", 0x60},
>> +     {"Fragments", 0x64},
>> +     {"aCBFCPAUSERx0", 0x68},
>> +     {"aCBFCPAUSERx1", 0x6C},
>> +     {"aCBFCPAUSERx2", 0x70},
>> +     {"aCBFCPAUSERx3", 0x74},
>> +     {"aCBFCPAUSERx4", 0x78},
>> +     {"aCBFCPAUSERx5", 0x7C},
>> +     {"aCBFCPAUSERx6", 0x80},
>> +     {"aCBFCPAUSERx7", 0x84},
>> +     {"aMACControlFramesReceived", 0x88}
>> +};
>> +
>> +struct hbl_cn_stat hbl_cn_mac_stats_tx[] = {
>> +     {"Octets", 0x0},
>> +     {"OctetsTransmittedOK", 0x4},
>> +     {"aPAUSEMACCtrlFramesTransmitted", 0x8},
>> +     {"aFramesTransmittedOK", 0xC},
>> +     {"VLANTransmittedOK", 0x10},
>> +     {"ifOutErrors", 0x14},
>> +     {"ifOutUcastPkts", 0x18},
>> +     {"ifOutMulticastPkts", 0x1C},
>> +     {"ifOutBroadcastPkts", 0x20},
>> +     {"Pkts64Octets", 0x24},
>> +     {"Pkts65to127Octets", 0x28},
>> +     {"Pkts128to255Octets", 0x2C},
>> +     {"Pkts256to511Octets", 0x30},
>> +     {"Pkts512to1023Octets", 0x34},
>> +     {"Pkts1024to1518Octets", 0x38},
>> +     {"Pkts1519toMaxOctets", 0x3C},
>> +     {"aCBFCPAUSETx0", 0x40},
>> +     {"aCBFCPAUSETx1", 0x44},
>> +     {"aCBFCPAUSETx2", 0x48},
>> +     {"aCBFCPAUSETx3", 0x4C},
>> +     {"aCBFCPAUSETx4", 0x50},
>> +     {"aCBFCPAUSETx5", 0x54},
>> +     {"aCBFCPAUSETx6", 0x58},
>> +     {"aCBFCPAUSETx7", 0x5C},
>> +     {"aMACControlFramesTx", 0x60},
>> +     {"Pkts", 0x64}
>> +};
>> +
>> +static const char pcs_counters_str[][ETH_GSTRING_LEN] = {
>> +     {"pcs_local_faults"},
>> +     {"pcs_remote_faults"},
>> +     {"pcs_remote_fault_reconfig"},
>> +     {"pcs_link_restores"},
>> +     {"pcs_link_toggles"},
>> +};
>> +
>> +static size_t pcs_counters_str_len = ARRAY_SIZE(pcs_counters_str);
>> +size_t hbl_cn_mac_fec_stats_len = ARRAY_SIZE(hbl_cn_mac_fec_stats);
>> +size_t hbl_cn_mac_stats_rx_len = ARRAY_SIZE(hbl_cn_mac_stats_rx);
>> +size_t hbl_cn_mac_stats_tx_len = ARRAY_SIZE(hbl_cn_mac_stats_tx);
>
> why those are not const?
>

I'll add const.

>> +
>> +static void qps_stop(struct hbl_cn_device *hdev);
>> +static void qp_destroy_work(struct work_struct *work);
>> +static int __user_wq_arr_unset(struct hbl_cn_ctx *ctx, struct hbl_cn_port *cn_port, u32 type);
>> +static void user_cq_destroy(struct kref *kref);
>> +static void set_app_params_clear(struct hbl_cn_device *hdev);
>> +static int hbl_cn_ib_cmd_ctrl(struct hbl_aux_dev *aux_dev, void *cn_ib_ctx, u32 op, void *input,
>> +                           void *output);
>> +static int hbl_cn_ib_query_mem_handle(struct hbl_aux_dev *ib_aux_dev, u64 mem_handle,
>> +                                   struct hbl_ib_mem_info *info);
>> +
>> +static void hbl_cn_reset_stats_counters_port(struct hbl_cn_device *hdev, u32 port);
>> +static void hbl_cn_late_init(struct hbl_cn_device *hdev);
>> +static void hbl_cn_late_fini(struct hbl_cn_device *hdev);
>> +static int hbl_cn_sw_init(struct hbl_cn_device *hdev);
>> +static void hbl_cn_sw_fini(struct hbl_cn_device *hdev);
>> +static void hbl_cn_spmu_init(struct hbl_cn_port *cn_port, bool full);
>> +static int hbl_cn_cmd_port_check(struct hbl_cn_device *hdev, u32 port, u32 flags);
>> +static void hbl_cn_qps_stop(struct hbl_cn_port *cn_port);
>> +
>> +static int hbl_cn_request_irqs(struct hbl_cn_device *hdev)
>> +{
>> +     struct hbl_cn_asic_funcs *asic_funcs = hdev->asic_funcs;
>> +
>> +     return asic_funcs->request_irqs(hdev);
>> +}
>> +
>> +static void hbl_cn_free_irqs(struct hbl_cn_device *hdev)
>> +{
>> +     struct hbl_cn_asic_funcs *asic_funcs =  hdev->asic_funcs;
>> +
>> +     asic_funcs->free_irqs(hdev);
>> +}
>> +
>> +static void hbl_cn_synchronize_irqs(struct hbl_aux_dev *cn_aux_dev)
>> +{
>> +     struct hbl_cn_device *hdev = cn_aux_dev->priv;
>> +     struct hbl_cn_asic_funcs *asic_funcs;
>> +
>> +     asic_funcs = hdev->asic_funcs;
>> +
>> +     asic_funcs->synchronize_irqs(hdev);
>> +}
>> +
>> +void hbl_cn_get_frac_info(u64 numerator, u64 denominator, u64 *integer, u64 *exp)
>> +{
>> +     u64 high_digit_n, high_digit_d, integer_tmp, exp_tmp;
>> +     u8 num_digits_n, num_digits_d;
>> +     int i;
>> +
>> +     num_digits_d = hbl_cn_get_num_of_digits(denominator);
>> +     high_digit_d = denominator;
>> +     for (i = 0; i < num_digits_d - 1; i++)
>> +             high_digit_d /= 10;
>> +
>> +     integer_tmp = 0;
>> +     exp_tmp = 0;
>> +
>> +     if (numerator) {
>> +             num_digits_n = hbl_cn_get_num_of_digits(numerator);
>> +             high_digit_n = numerator;
>> +             for (i = 0; i < num_digits_n - 1; i++)
>> +                     high_digit_n /= 10;
>> +
>> +             exp_tmp = num_digits_d - num_digits_n;
>> +
>> +             if (high_digit_n < high_digit_d) {
>> +                     high_digit_n *= 10;
>> +                     exp_tmp++;
>> +             }
>> +
>> +             integer_tmp = div_u64(high_digit_n, high_digit_d);
>> +     }
>> +
>> +     *integer = integer_tmp;
>> +     *exp = exp_tmp;
>> +}
>
> this function sounds suspicious for a network driver, what do you need
> it for?
>

Some of our counters are exposed by the HW with numerator/denominator
representation and we'd like to expose them to the user with exponent
representation.
This function converts a counter value from one representation to the
other.

>> +
>> +int hbl_cn_read_spmu_counters(struct hbl_cn_port *cn_port, u64 out_data[], u32 *num_out_data)
>> +{
>> +     struct hbl_cn_device *hdev = cn_port->hdev;
>> +     struct hbl_cn_asic_port_funcs *port_funcs;
>> +     struct hbl_cn_stat *ignore;
>> +     int rc;
>> +
>> +     port_funcs = hdev->asic_funcs->port_funcs;
>> +
>> +     port_funcs->spmu_get_stats_info(cn_port, &ignore, num_out_data);
>
> hard to ignore that you deref uninitialized pointer...
>
> please consider going one step back and start with our internal mailing
> lists, thank you
> Przemek
>
> [...]

Powered by blists - more mailing lists