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

[Leon Romanovsky <leon@kernel.org>]

On Mon, Mar 16, 2026 at 06:00:22PM -0400, Dennis Dalessandro wrote:
> On 3/16/26 11:51 AM, Leon Romanovsky wrote:

<...>

> > 
> > > +	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.
> 
> We do use function pointers in a number of places because of performance
> reasons. However these are more for abstracting away the differences between
> the different chips. I'll consult with the team and see if we can clean this
> up any.

BTW, function pointers generally perform worse than direct calls, since  
they always require an indirect jump, which is harder for the branch  
predictor to handle.

Their main purpose is to allow the invoked function to be changed  
dynamically.


<...>

> > > +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.
> 
> Why? This has been a long standing knob available to users. Same with many
> of the others below. We can look into whittling them down if they are not
> being used regularly. Some of these are important for tuning performance and
> are meant to be tuned driver wide at init time, not per hfi instance which
> generally is my litmus test for if something can be a module paramter.
> 
> Perhaps it would be better dealt with as a follow on patch to this series to
> refine the whole set?

Yes, please remove the module parameters from the initial submission.
We can address them afterward. There is also a possibility that these
parameters could be useful for other drivers.

> 
> Thanks for the detailed review.

I'm continuing the review today as well.

Thanks

> 
> -Denny
>