On 6/13/24 16:01, Przemek Kitszel wrote: > [Some people who received this message don't often get email from przemyslaw.kitszel@xxxxxxxxx. 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@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> >> --- >> .../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@xxxxxxxxxxxxxxxx. > > 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 > > [...]
