Re: [PATCH for-next resend 01/24] RDMA/hfi2: Start hfi2 driver by basing off of hfi1

[Leon Romanovsky <leon@kernel.org>]

On Wed, Mar 11, 2026 at 01:53:50PM -0400, Dennis Dalessandro wrote:
> The hfi1 driver has seen a near complete rewrite. Instead of trying to
> wedge all of the changes back into hfi1 we are going to deprecate the
> driver in favor of a new driver. This driver will not have the
> objectionalbe private cdev and instead will rely on the infrastructure
> provided by the ib core for doing user interaction.
> 
> This new driver will support both the old/existing chip (WFR) as well as
> the new (JKR) chip and follow on chips for 800Gbps and 1.6Tbps.
> 
> Co-developed-by: Dean Luick <dean.luick@cornelisnetworks.com>
> Signed-off-by: Dean Luick <dean.luick@cornelisnetworks.com>
> Co-developed-by: Bendan Cunningham <brendan.cunningham@cornelisnetworks.com>
> Signed-off-by: Breandan Cunningham <brendan.cunningham@cornelisnetworks.com>
> Co-developed-by: Douglas Miller <doug.miller@cornelisnetworks.com>
> Signed-off-by: Douglas Miller <doug.miller@cornelisnetworks.com>
> Signed-off-by: Dennis Dalessandro <dennis.dalessandro@cornelisnetworks.com>
> ---
>  drivers/infiniband/hw/hfi2/hfi2.h | 3471 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 3471 insertions(+)
>  create mode 100644 drivers/infiniband/hw/hfi2/hfi2.h
> 
> diff --git a/drivers/infiniband/hw/hfi2/hfi2.h b/drivers/infiniband/hw/hfi2/hfi2.h
> new file mode 100644
> index 000000000000..a522f4606d3d
> --- /dev/null
> +++ b/drivers/infiniband/hw/hfi2/hfi2.h
> @@ -0,0 +1,3471 @@
> +/* SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause */
> +/*
> + * Copyright(c) 2025-2026 Cornelis Networks, Inc.
> + * Copyright(c) 2015-2020 Intel Corporation.
> + */
> +
> +#ifndef _HFI2_KERNEL_H
> +#define _HFI2_KERNEL_H
> +
> +#include <linux/refcount.h>
> +#include <linux/interrupt.h>
> +#include <linux/pci.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/mutex.h>
> +#include <linux/list.h>
> +#include <linux/scatterlist.h>
> +#include <linux/slab.h>
> +#include <linux/io.h>
> +#include <linux/fs.h>
> +#include <linux/completion.h>
> +#include <linux/kref.h>
> +#include <linux/sched.h>
> +#include <linux/cdev.h>
> +#include <linux/delay.h>
> +#include <linux/kthread.h>
> +#include <linux/i2c.h>
> +#include <linux/i2c-algo-bit.h>
> +#include <linux/xarray.h>
> +#include <rdma/ib_hdrs.h>
> +#include <rdma/opa_addr.h>
> +#include <linux/rhashtable.h>
> +#include <rdma/rdma_vt.h>
> +
> +struct hfi2_devdata;
> +struct hfi2_devrsrcs;

Please put these declarations in relevant header files.

> +#include "chip_registers.h"
> +#include "chip_registers_jkr.h"
> +#include "common.h"
> +#include "opfn.h"
> +#include "verbs.h"
> +#include "pio.h"
> +#include "chip.h"
> +#include "mad.h"
> +#include "qsfp.h"
> +#include "platform.h"
> +#include "affinity.h"
> +#include "msix.h"
> +#include "cport.h"
> +#ifdef CONFIG_HFI_L8SIM

This config needs to be in hfi_sim.h file.

> +#include "hfi_sim.h"
> +#endif

<...>

> +/*
> + * per driver stats, either not device nor port-specific, or
> + * summed over all of the devices and ports.
> + * They are described by name via ipathfs filesystem, so layout
> + * and number of elements can change without breaking compatibility.
> + * If members are added or deleted hfi2_statnames[] in debugfs.c must
> + * change to match.
> + */

What is ipathfs filesystem?

> +struct hfi2_ib_stats {
> +	__u64 sps_ints; /* number of interrupts handled */
> +	__u64 sps_errints; /* number of error interrupts */
> +	__u64 sps_txerrs; /* tx-related packet errors */
> +	__u64 sps_rcverrs; /* non-crc rcv packet errors */
> +	__u64 sps_hwerrs; /* hardware errors reported (parity, etc.) */
> +	__u64 sps_nopiobufs; /* no pio bufs avail from kernel */
> +	__u64 sps_ctxts; /* number of contexts currently open */
> +	__u64 sps_lenerrs; /* number of kernel packets where RHF != LRH len */
> +	__u64 sps_buffull;
> +	__u64 sps_hdrfull;
> +};
> +
> +extern struct hfi2_ib_stats hfi2_stats;
> +extern const struct pci_error_handlers hfi2_pci_err_handler;
> +
> +extern int num_driver_cntrs;

git grep shows that hfi1 has too many extern variables. Can we reduce
that number?

<...>

> +struct hfi2_ctxtdata {

<...>

> +	/* Reference count the base context usage */
> +	struct kref kref;
> +	/* numa node of this context */
> +	int numa_id;

I have very strong feeling that these two shouldn't be stored in the driver.

> +	/* same size as task_struct .comm[], command that opened context */
> +	char comm[TASK_COMM_LEN];

This is too.

> +	/* The version of the library which opened this ctxt */
> +	u32 userversion;

And this.

<...>

> +struct rvt_sge_state;

No random forward declarations, please

> +
> +/*
> + * Get/Set IB link-level config parameters for f_get/set_ib_cfg()
> + * Mostly for MADs that set or query link parameters, also ipath
> + * config interfaces
> + */

Please put the below into enum and the one after that.

> +#define HFI2_IB_CFG_LIDLMC 0 /* LID (LS16b) and Mask (MS16b) */
> +#define HFI2_IB_CFG_LWID_DG_ENB 1 /* allowed Link-width downgrade */
> +#define HFI2_IB_CFG_LWID_ENB 2 /* allowed Link-width */
> +#define HFI2_IB_CFG_LWID 3 /* currently active Link-width */
> +#define HFI2_IB_CFG_SPD_ENB 4 /* allowed Link speeds */
> +#define HFI2_IB_CFG_SPD 5 /* current Link spd */
> +#define HFI2_IB_CFG_RXPOL_ENB 6 /* Auto-RX-polarity enable */
> +#define HFI2_IB_CFG_LREV_ENB 7 /* Auto-Lane-reversal enable */
> +#define HFI2_IB_CFG_LINKLATENCY 8 /* Link Latency (IB1.2 only) */
> +#define HFI2_IB_CFG_HRTBT 9 /* IB heartbeat off/enable/auto; DDR/QDR only */
> +#define HFI2_IB_CFG_OP_VLS 10 /* operational VLs */
> +#define HFI2_IB_CFG_VL_HIGH_CAP 11 /* num of VL high priority weights */
> +#define HFI2_IB_CFG_VL_LOW_CAP 12 /* num of VL low priority weights */
> +#define HFI2_IB_CFG_OVERRUN_THRESH 13 /* IB overrun threshold */
> +#define HFI2_IB_CFG_PHYERR_THRESH 14 /* IB PHY error threshold */
> +#define HFI2_IB_CFG_LINKDEFAULT 15 /* IB link default (sleep/poll) */
> +#define HFI2_IB_CFG_PKEYS 16 /* update partition keys */
> +#define HFI2_IB_CFG_MTU 17 /* update MTU in IBC */
> +#define HFI2_IB_CFG_VL_HIGH_LIMIT 19
> +#define HFI2_IB_CFG_PMA_TICKS 20 /* PMA sample tick resolution */
> +#define HFI2_IB_CFG_PORT 21 /* switch port we are connected to */
> +
> +/*
> + * HFI or Host Link States
> + *
> + * These describe the states the driver thinks the logical and physical
> + * states are in.  Used as an argument to set_link_state().  Implemented
> + * as bits for easy multi-state checking.  The actual state can only be
> + * one.
> + */
> +#define __HLS_UP_INIT_BP	0
> +#define __HLS_UP_ARMED_BP	1
> +#define __HLS_UP_ACTIVE_BP	2
> +#define __HLS_DN_DOWNDEF_BP	3	/* link down default */
> +#define __HLS_DN_POLL_BP	4
> +#define __HLS_DN_DISABLE_BP	5
> +#define __HLS_DN_OFFLINE_BP	6
> +#define __HLS_VERIFY_CAP_BP	7
> +#define __HLS_GOING_UP_BP	8
> +#define __HLS_GOING_OFFLINE_BP  9
> +#define __HLS_LINK_COOLDOWN_BP 10
> +
> +#define HLS_UP_INIT	  BIT(__HLS_UP_INIT_BP)
> +#define HLS_UP_ARMED	  BIT(__HLS_UP_ARMED_BP)
> +#define HLS_UP_ACTIVE	  BIT(__HLS_UP_ACTIVE_BP)
> +#define HLS_DN_DOWNDEF	  BIT(__HLS_DN_DOWNDEF_BP) /* link down default */
> +#define HLS_DN_POLL	  BIT(__HLS_DN_POLL_BP)
> +#define HLS_DN_DISABLE	  BIT(__HLS_DN_DISABLE_BP)
> +#define HLS_DN_OFFLINE	  BIT(__HLS_DN_OFFLINE_BP)
> +#define HLS_VERIFY_CAP	  BIT(__HLS_VERIFY_CAP_BP)
> +#define HLS_GOING_UP	  BIT(__HLS_GOING_UP_BP)
> +#define HLS_GOING_OFFLINE BIT(__HLS_GOING_OFFLINE_BP)
> +#define HLS_LINK_COOLDOWN BIT(__HLS_LINK_COOLDOWN_BP)
> +
> +#define HLS_UP (HLS_UP_INIT | HLS_UP_ARMED | HLS_UP_ACTIVE)
> +#define HLS_DOWN ~(HLS_UP)
> +
> +#define HLS_DEFAULT HLS_DN_POLL

<...>

> +/*
> + * Possible fabric manager config parameters for fm_{get,set}_table()
> + */
> +#define FM_TBL_VL_HIGH_ARB		1 /* Get/set VL high prio weights */
> +#define FM_TBL_VL_LOW_ARB		2 /* Get/set VL low prio weights */
> +#define FM_TBL_BUFFER_CONTROL		3 /* Get/set Buffer Control */
> +#define FM_TBL_SC2VLNT			4 /* Get/set SC->VLnt */
> +#define FM_TBL_VL_PREEMPT_ELEMS		5 /* Get (no set) VL preempt elems */
> +#define FM_TBL_VL_PREEMPT_MATRIX	6 /* Get (no set) VL preempt matrix */

enum again


<...>

> +static inline void incr_cntr64(u64 *cntr)
> +{
> +	if (*cntr < (u64)-1LL)
> +		(*cntr)++;
> +}

This is too close to size_add/check_add_overflow. Let's not open-coded
variants of existing functions.

> +

<...>

> +	void (*setextled)(struct hfi2_pportdata *ppd, u32 on);
> +	void (*start_led_override)(struct hfi2_pportdata *ppd,
> +				   unsigned int timeon,
> +				   unsigned int timeoff);
> +	void (*shutdown_led_override)(struct hfi2_pportdata *ppd);
> +	void (*read_guid)(struct hfi2_devdata *dd);
> +	int (*early_per_chip_init)(struct hfi2_devdata *dd);
> +	int (*mid_per_chip_init)(struct hfi2_devdata *dd);
> +	void (*init_other)(struct hfi2_devdata *dd);
> +	int (*late_per_chip_init)(struct hfi2_devdata *dd);
> +	void (*start_port)(struct hfi2_pportdata *ppd);
> +	void (*stop_port)(struct hfi2_pportdata *ppd);
> +	void (*put_tid)(struct hfi2_ctxtdata *rcd, u32 index,
> +			u32 type, unsigned long pa, u16 order, bool flush);
> +	void (*rcv_array_wc_fill)(struct hfi2_ctxtdata *rcd, u32 index,
> +				  u32 type);
> +	void (*set_port_tid_config)(struct hfi2_devdata *dd, int pidx, u16 ctxt,
> +				    u32 eager_base, u16 alloced,
> +				    u32 expected_base, u32 expected_count);
> +	void (*set_port_max_mtu)(struct hfi2_pportdata *ppd, u32 maxvlmtu);
> +	void (*update_rcv_hdr_size)(struct hfi2_pportdata *ppd, u16 ctxt,
> +				    u32 size);
> +	bool (*check_synth_status)(struct hfi2_devdata *dd);
> +	void (*update_synth_status)(struct hfi2_devdata *dd);
> +	u64 (*create_pbc)(struct hfi2_pportdata *ppd, bool loopback, u64 flags,
> +			  int srate_mbs, u32 vl, u32 dw_len, u32 l2, u32 dlid, u32 sctxt);
> +	void (*set_pio_integrity)(struct hfi2_devdata *dd, u32 pidx, u32 ctxt, int type,
> +				  enum spi_cmds cmd);
> +	int (*find_used_resources)(struct hfi2_devdata *dd);
> +	void (*read_link_quality)(struct hfi2_pportdata *ppd, u8 *link_quality);
> +	void (*set_rheq_addr)(struct hfi2_devdata *dd, u16 ctxt, u64 dma_addr);
> +	void (*handle_link_bounce)(struct work_struct *work);
> +	void (*enable_rcv_context)(struct hfi2_pportdata *ppd, u16 ctxt,
> +				   u64 *kctxt_ctrl, bool enable);

Do you really need function pointers inside driver? It makes possible refactoring harder.

> +};

<...>

> +/*
> + * This determines the source for data to fill-in dd->rsrcs.
> + */
> +#define HFI_SRIOV_MOD_PARAMS

What value should it be defined to?

> +

<...>

> +/* hfi2_put_tid types */
> +#define PT_EXPECTED       0
> +#define PT_EAGER          1

It is amazing how many times you already defined 0 and 1 :).

> +
> +struct tid_rb_node;

<...>

> +/**
> + * hfi2_rcd_head - add accessor for rcd head
> + * @rcd: the context
> + */
> +static inline u32 hfi2_rcd_head(struct hfi2_ctxtdata *rcd)
> +{
> +	return rcd->head;
> +}
> +
> +/**
> + * hfi2_set_rcd_head - add accessor for rcd head
> + * @rcd: the context
> + * @head: the new head
> + */
> +static inline void hfi2_set_rcd_head(struct hfi2_ctxtdata *rcd, u32 head)
> +{
> +	rcd->head = head;
> +}

Do we really need to define such basic operation?

> +
> +/* calculate the current RHF address */
> +static inline __le32 *get_rhf_addr(struct hfi2_ctxtdata *rcd)
> +{
> +	return (__le32 *)rcd->rcvhdrq + rcd->head + rcd->rhf_offset;
> +}
> +
> +/* return DMA_RTAIL configuration */
> +static inline bool get_dma_rtail_setting(struct hfi2_ctxtdata *rcd)
> +{
> +	return !!HFI2_CAP_KGET_MASK(rcd->flags, DMA_RTAIL);
> +}

Same question.

> +

<...>

> +/**
> + * hfi2_seq_cnt - return seq_cnt member
> + * @rcd: the receive context
> + *
> + * Return seq_cnt member
> + */
> +static inline u8 hfi2_seq_cnt(struct hfi2_ctxtdata *rcd)
> +{
> +	return rcd->seq_cnt;
> +}
> +
> +/**
> + * hfi2_set_seq_cnt - return seq_cnt member
> + * @rcd: the receive context
> + *
> + * Return seq_cnt member
> + */
> +static inline void hfi2_set_seq_cnt(struct hfi2_ctxtdata *rcd, u8 cnt)
> +{
> +	rcd->seq_cnt = cnt;
> +}
> +
> +/**
> + * last_rcv_seq - is last
> + * @rcd: the receive context
> + * @seq: sequence
> + *
> + * return true if last packet
> + */
> +static inline bool last_rcv_seq(struct hfi2_ctxtdata *rcd, u32 seq)
> +{
> +	return seq != rcd->seq_cnt;
> +}

Again

> +
> +/**
> + * rcd_seq_incr - increment context sequence number
> + * @rcd: the receive context
> + * @seq: the current sequence number
> + *
> + * Returns: true if the this was the last packet
> + */
> +static inline bool hfi2_seq_incr(struct hfi2_ctxtdata *rcd, u32 seq)
> +{
> +	rcd->seq_cnt = hfi2_seq_incr_wrap(rcd->seq_cnt);
> +	return last_rcv_seq(rcd, seq);
> +}
> +
> +/**
> + * get_hdrqentsize - return hdrq entry size
> + * @rcd: the receive context
> + */
> +static inline u8 get_hdrqentsize(struct hfi2_ctxtdata *rcd)
> +{
> +	return rcd->rcvhdrqentsize;
> +}

Again, and again and again ....

> +
> +#define DEFAULT_HDRQ_ENTSIZE 32

<...>

> +	if (link_speed == OPA_LINK_SPEED_100G)
> +		egress_rate = 100000;
> +	else if (link_speed == OPA_LINK_SPEED_50G)
> +		egress_rate = 50000;
> +	else if (link_speed == OPA_LINK_SPEED_25G)
> +		egress_rate = 25000;
> +	else /* assume OPA_LINK_SPEED_12_5G */
> +		egress_rate = 12500;

It is switch-case.

> +

<...>

> +/* MTU enumeration, 256-4k match IB */
> +#define OPA_MTU_0     0
> +#define OPA_MTU_256   1
> +#define OPA_MTU_512   2
> +#define OPA_MTU_1024  3
> +#define OPA_MTU_2048  4
> +#define OPA_MTU_4096  5

enum

> +
> +u32 lrh_max_header_bytes(struct hfi2_pportdata *ppd);
> +int mtu_to_enum(u32 mtu, int default_if_bad);
> +u16 enum_to_mtu(int mtu);
> +static inline int valid_ib_mtu(unsigned int mtu)
> +{
> +	return mtu == 256 || mtu == 512 ||
> +		mtu == 1024 || mtu == 2048 ||
> +		mtu == 4096;
> +}

switch-case

> +
> +static inline int valid_opa_max_mtu(unsigned int mtu)
> +{
> +	return mtu >= 2048 &&
> +		(valid_ib_mtu(mtu) || mtu == 8192 || mtu == 10240);
> +}

No to module parameter.


<...>

> +/*
> + * Called by readers of cc_state only, must call under rcu_read_lock().
> + */
> +static inline struct cc_state *get_cc_state(struct hfi2_pportdata *ppd)
> +{
> +	return rcu_dereference(ppd->cc_state);
> +}
> +
> +/*
> + * Called by writers of cc_state only,  must call under cc_state_lock.
> + */
> +static inline
> +struct cc_state *get_cc_state_protected(struct hfi2_pportdata *ppd)
> +{
> +	return rcu_dereference_protected(ppd->cc_state,
> +					 lockdep_is_held(&ppd->cc_state_lock));
> +}

No to redeine of basic kernel coding routines.


<...>

> +/*
> + * Flush write combining store buffers (if present) and perform a write
> + * barrier.
> + */
> +static inline void flush_wc(void)
> +{
> +	asm volatile("sfence" : : : "memory");
> +}

It is very unexpected to see asm code in the driver. I'm aware that it
is from hfi1, but it doesn't mean that you should copy it to new code.

> +
> +void handle_eflags(struct hfi2_packet *packet);
> +void seqfile_dump_rcd(struct seq_file *s, struct hfi2_ctxtdata *rcd);
> +
> +/* global module parameter variables */

Let's remove module parameters for now.

> +extern unsigned int hfi2_max_mtu;
> +extern unsigned int hfi2_cu;
> +extern unsigned int user_credit_return_threshold;
> +extern int num_user_contexts;
> +extern unsigned long n_krcvqs;
> +extern uint krcvqs[];
> +extern int krcvqsset;
> +extern uint loopback;
> +extern uint quick_linkup;
> +extern uint rcv_intr_timeout;
> +extern uint rcv_intr_count;
> +extern uint rcv_intr_dynamic;
> +extern ushort link_crc_mask;
> +
> +extern struct mutex hfi2_mutex;
> +
> +/* Number of seconds before our card status check...  */
> +#define STATUS_TIMEOUT 60
> +
> +#define DRIVER_NAME		"hfi2"
> +#define HFI2_USER_MINOR_BASE     0
> +#define HFI2_TRACE_MINOR         127
> +#define HFI2_NMINORS             255
> +
> +#define PCI_VENDOR_ID_INTEL 0x8086

This is already defined in include/linux/pci_ids.h

> +#define PCI_DEVICE_ID_INTEL0 0x24f0
> +#define PCI_DEVICE_ID_INTEL1 0x24f1
> +#define PCI_VENDOR_ID_CORNELIS 0x434e
> +#define PCI_DEVICE_ID_CORNELIS_CN5000 0x0001
> +#define PCI_SUBDEVICE_CN5000_DUAL_PORT 0x0002
> +#define PCI_SUBDEVICE_CN5000_SINGLE_PORT_PS 0x0003 /* PS = Port Swap */
> +#define PCI_SUBDEVICE_CN5000_DUAL_PORT_PS 0x0004
> +
> +/* create a ULL mask out of the given number of bits */
> +#define MASK_ULL(bits) ((1ull << (bits)) - 1)

It is basic


<...>

> +#define dd_dev_emerg(dd, fmt, ...) \
> +	dev_emerg(&(dd)->pcidev->dev, "%s: " fmt, \
> +		  rvt_get_ibdev_name(&(dd)->verbs_dev.rdi), ##__VA_ARGS__)
> +
> +#define dd_dev_err(dd, fmt, ...) \
> +	dev_err(&(dd)->pcidev->dev, "%s: " fmt, \
> +		rvt_get_ibdev_name(&(dd)->verbs_dev.rdi), ##__VA_ARGS__)
> +
> +#define dd_dev_err_ratelimited(dd, fmt, ...) \
> +	dev_err_ratelimited(&(dd)->pcidev->dev, "%s: " fmt, \
> +			    rvt_get_ibdev_name(&(dd)->verbs_dev.rdi), \
> +			    ##__VA_ARGS__)
> +
> +#define dd_dev_warn(dd, fmt, ...) \
> +	dev_warn(&(dd)->pcidev->dev, "%s: " fmt, \
> +		 rvt_get_ibdev_name(&(dd)->verbs_dev.rdi), ##__VA_ARGS__)
> +
> +#define dd_dev_warn_ratelimited(dd, fmt, ...) \
> +	dev_warn_ratelimited(&(dd)->pcidev->dev, "%s: " fmt, \
> +			     rvt_get_ibdev_name(&(dd)->verbs_dev.rdi), \
> +			     ##__VA_ARGS__)
> +
> +#define dd_dev_info(dd, fmt, ...) \
> +	dev_info(&(dd)->pcidev->dev, "%s: " fmt, \
> +		 rvt_get_ibdev_name(&(dd)->verbs_dev.rdi), ##__VA_ARGS__)
> +
> +#define dd_dev_info_ratelimited(dd, fmt, ...) \
> +	dev_info_ratelimited(&(dd)->pcidev->dev, "%s: " fmt, \
> +			     rvt_get_ibdev_name(&(dd)->verbs_dev.rdi), \
> +			     ##__VA_ARGS__)
> +
> +#define dd_dev_dbg(dd, fmt, ...) \
> +	dev_dbg(&(dd)->pcidev->dev, "%s: " fmt, \
> +		rvt_get_ibdev_name(&(dd)->verbs_dev.rdi), ##__VA_ARGS__)
> +
> +#define hfi2_dev_porterr(dd, port, fmt, ...) \
> +	dev_err(&(dd)->pcidev->dev, "%s: port %u: " fmt, \
> +		rvt_get_ibdev_name(&(dd)->verbs_dev.rdi), (port), ##__VA_ARGS__)
> +
> +#define ppd_dev_err(ppd, fmt, ...) \
> +	dev_err(&(ppd)->dd->pcidev->dev, "%s.%u: " fmt, \
> +		rvt_get_ibdev_name(&(ppd)->dd->verbs_dev.rdi), (ppd)->hw_pidx, \
> +		##__VA_ARGS__)
> +
> +#define ppd_dev_warn(ppd, fmt, ...) \
> +	dev_warn(&(ppd)->dd->pcidev->dev, "%s.%u: " fmt, \
> +		 rvt_get_ibdev_name(&(ppd)->dd->verbs_dev.rdi), (ppd)->hw_pidx, \
> +		 ##__VA_ARGS__)
> +
> +#define ppd_dev_warn_ratelimited(ppd, fmt, ...) \
> +	dev_warn_ratelimited(&(ppd)->dd->pcidev->dev, "%s.%u: " fmt, \
> +			     rvt_get_ibdev_name(&(ppd)->dd->verbs_dev.rdi), (ppd)->hw_pidx, \
> +			     ##__VA_ARGS__)
> +
> +#define ppd_dev_info(ppd, fmt, ...) \
> +	dev_info(&(ppd)->dd->pcidev->dev, "%s.%u: " fmt, \
> +		 rvt_get_ibdev_name(&(ppd)->dd->verbs_dev.rdi), (ppd)->hw_pidx, \
> +		 ##__VA_ARGS__)

ibdev_* prints for any new code.


<...>

> +static inline bool rhe_crk_err(struct hfi2_packet *packet)
> +{
> +	/* same bit location on WFR, JKR; different u64 */
> +	return !!(packet->err_flags & RHF_DC_ERR);
> +}
> +
> +static inline bool rhe_tid_err(struct hfi2_packet *packet)
> +{
> +	/* same bit location on WFR, JKR; different u64 */
> +	return !!(packet->err_flags & RHF_TID_ERR);
> +}
> +
> +static inline bool rhe_len_err(struct hfi2_packet *packet)
> +{
> +	/* same bit location on WFR, JKR; different u64 */
> +	return !!(packet->err_flags & RHF_LEN_ERR);
> +}
> +
> +static inline bool rhe_icrc_err(struct hfi2_packet *packet)
> +{
> +	/* same bit location on WFR, JKR; different u64 */
> +	return !!(packet->err_flags & RHF_ICRC_ERR);
> +}

bool type doesn't need !! operator.

Thanks