Re: [PATCH v22 net-next 04/12] net/nebula-matrix: add channel layer

[Jakub Kicinski <kuba@kernel.org>]

On Thu, 23 Jul 2026 12:00:56 +0800 illusion.wang wrote:
> +#define FNV_PRIME_32 0x01000193
> +#define FNV_OFFSET_32 0x811C9DC5
> +static u32 nbl_common_calc_hash_key(void *key, u32 key_size, u32 bucket_size)
> +{
> +	u32 hash = FNV_OFFSET_32;
> +	u8 *p = (u8 *)key;
> +	u32 i;
> +
> +	if (bucket_size == 0 || bucket_size == NBL_HASH_TBL_LIST_BUCKET_SIZE)
> +		return 0;
> +
> +	for (i = 0; i < key_size; i++) {
> +		hash ^= p[i];
> +		hash *= FNV_PRIME_32;
> +	}
> +	/* Use bitmask if bucket_size is a power of 2 */
> +	if ((bucket_size & (bucket_size - 1)) == 0)
> +		return hash & (bucket_size - 1);
> +	else
> +		return hash % bucket_size;
> +}

Why are you implementing your own hashing function and your own hash
table? Can't one of existing implementations in the kernel be used?

> +int nbl_common_alloc_hash_node(struct nbl_hash_tbl_mgt *tbl_mgt, void *key,
> +			       void *data, void **out_data)
> +{
> +	struct nbl_hash_entry_node *hash_node;
> +	u16 data_size;
> +	u32 hash_val;
> +	u16 key_size;
> +
> +	hash_node = devm_kzalloc(tbl_mgt->tbl_key.dev, sizeof(*hash_node),
> +				 GFP_KERNEL);

Don't use devm_ for inherently dynamically allocated memory...

> +static void nbl_common_detach_hash_node(struct nbl_hash_tbl_mgt *tbl_mgt,
> +					struct nbl_hash_entry_node *hash_node)
> +{
> +	hlist_del(&hash_node->node);
> +	devm_kfree(tbl_mgt->tbl_key.dev, hash_node->key);
> +	devm_kfree(tbl_mgt->tbl_key.dev, hash_node->data);
> +	devm_kfree(tbl_mgt->tbl_key.dev, hash_node);

... if you ever have to explicitly free something, chances are you
shouldn't be using devm_ in the first place.

> index 7ae331959ca1..d6b7bfff3cc6 100644
> --- a/drivers/net/ethernet/nebula-matrix/nbl/nbl_core.h
> +++ b/drivers/net/ethernet/nebula-matrix/nbl/nbl_core.h
> @@ -11,23 +11,34 @@
>  #include "nbl_include/nbl_def_common.h"
>  
>  struct nbl_hw_mgt;
> +struct nbl_hw_ops_tbl;
> +struct nbl_channel_ops_tbl;
> +struct nbl_channel_mgt;
>  

Lots of unnecessary forward declarations.
Using type as a member implicitly forward declares.

>  struct nbl_adapter *nbl_core_init(struct pci_dev *pdev,
>  				  struct nbl_init_param *param);
>  void nbl_core_remove(struct nbl_adapter *adapter);
> +
>  #endif

This chunk should be squashed into an earlier patch

> +#define NBL_CHAN_SEND(chan_send, dst_id, mesg_type, argument, arg_length,\
> +		      response, resp_length, need_ack)			\
> +do {									\
> +	typeof(chan_send)	*__chan_send = &(chan_send);		\
> +	__chan_send->dstid	= (dst_id);				\
> +	__chan_send->msg_type	= (mesg_type);				\
> +	__chan_send->arg	= (argument);				\
> +	__chan_send->arg_len	= (arg_length);				\
> +	__chan_send->resp	= (response);				\
> +	__chan_send->resp_len	= (resp_length);			\
> +	__chan_send->ack	= (need_ack);				\
> +} while (0)
> +
> +#define NBL_CHAN_ACK(chan_ack, dst_id, mesg_type, msg_id, err_code, ack_data, \
> +		     data_length)					\
> +do {									\
> +	typeof(chan_ack)	*__chan_ack = &(chan_ack);		\
> +	__chan_ack->dstid	= (dst_id);				\
> +	__chan_ack->msg_type	= (mesg_type);				\
> +	__chan_ack->msgid	= (msg_id);				\
> +	__chan_ack->err		= (err_code);				\
> +	__chan_ack->data	= (ack_data);				\
> +	__chan_ack->data_len	= (data_length);			\
> +} while (0)
> +

Why are these macros and not C code?
You only seem to pass struct nbl_chan_ack_info into NBL_CHAN_ACK()

> +struct nbl_hash_tbl_mgt;

This forward declaration here makes no sense.
Please look at your patches before you post them.

>  struct nbl_common_info {
>  	struct pci_dev *pdev;
>  	struct device *dev;
> @@ -29,4 +31,19 @@ struct nbl_common_info {
>  	u8 has_net;
>  };
>  
> +struct nbl_hash_tbl_key {