回复：[v4,net-next,11/11] net/nebula-matrix: add common dev start/stop operation

[Illusion Wang]

Thank you for your feedback
But
enum nbl_msix_serv_type {
	/* virtio_dev has a config vector_id, and the vector_id need is 0 */
	NBL_MSIX_VIRTIO_TYPE = 0,
	NBL_MSIX_NET_TYPE,
	NBL_MSIX_MAILBOX_TYPE,
	NBL_MSIX_TYPE_MAX
};
NBL_MSIX_NET_TYPE equals 1，so this function has no problem？

--illusion.wang

> +static int nbl_dev_configure_msix_map(struct nbl_dev_mgt *dev_mgt)
> +{
> + struct nbl_dispatch_ops *disp_ops = dev_mgt->disp_ops_tbl->ops;
> + struct nbl_dev_common *dev_common = dev_mgt->common_dev;
> + struct nbl_msix_info *msix_info = &dev_common->msix_info;
> + bool mask_en = msix_info->serv_info[NBL_MSIX_NET_TYPE].hw_self_mask_en;
> + u16 msix_net_num = msix_info->serv_info[NBL_MSIX_NET_TYPE].num;
> + u16 msix_not_net_num = 0;
> + int err, i;
> +
> + for (i = NBL_MSIX_NET_TYPE; i < NBL_MSIX_TYPE_MAX; i++)
> +  msix_info->serv_info[i].base_vector_id =
> +   msix_info->serv_info[i - 1].base_vector_id +
> +   msix_info->serv_info[i - 1].num;
             ^^^^^^

Does this loop cause an array underflow? NBL_MSIX_NET_TYPE equals 0 (from
the enum in nbl_dev.h), so on the first iteration i=0 and the code accesses
serv_info[-1].

The serv_info array is declared with NBL_MSIX_TYPE_MAX (2) elements in
struct nbl_msix_info, giving valid indices [0,1]. Accessing index -1 reads
uninitialized memory before the array start.

Call chain: nbl_core_start()->nbl_dev_start()->nbl_dev_start_common_dev()
->nbl_dev_configure_msix_map() guarantees this occurs during device start.

Should the loop start at i=1 (NBL_MSIX_MAILBOX_TYPE) instead, or should
serv_info[0].base_vector_id be initialized before the loop?

> +
> + for (i = NBL_MSIX_MAILBOX_TYPE; i < NBL_MSIX_TYPE_MAX; i++) {

[ ... ]


Thank you for pointing this out! Upon reviewing the code again, I believe
the variable declarations in nbl_dev_request_mailbox_irq() already follow
the reverse Christmas tree convention:

dev_common (longest identifier) is declared first, as it is required for
subsequent declarations.

The remaining variables (msix_info and common) are ordered by decreasing 
dentifier length, aligning with the style guide.
However, I appreciate the clarification opportunity—if there’s a nuance
I’m missing (e.g., stricter adherence to identifier length over dependency
order), I’d be happy to adjust the code accordingly.creasing identifier
length, aligning with the style guide.

---illusion.wang


Andrew Lunn also noted in v3 that variable declarations should follow
reverse Christmas tree ordering (longest line first). Looking at functions
in this commit like nbl_dev_request_mailbox_irq():

 struct nbl_dev_common *dev_common = dev_mgt->common_dev;
 struct nbl_msix_info *msix_info = &dev_common->msix_info;
 struct nbl_common_info *common = dev_mgt->common;

the declarations are not in reverse Christmas tree order.

Link: https://lore.kernel.org/netdev/c6d37ecd-7dc6-4f83-b76c-2735a5f116fd@xxxxxxx/