Re: [PATCH v22 net-next 04/12] net/nebula-matrix: add channel layer
flat view
From: Jakub Kicinski <kuba@kernel.org>
Date: 2026-07-31 01:27:20
Also in:
linux-doc, lkml
On Thu, 23 Jul 2026 12:00:56 +0800 illusion.wang wrote:
quoted hunk ↗ jump to hunk
+#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?
quoted hunk ↗ jump to hunk
+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...
quoted hunk ↗ jump to hunk
+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.
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
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
quoted hunk ↗ jump to hunk
+#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.
quoted hunk ↗ jump to hunk
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 {