Re: [PATCH v5 net-next 05/11] net/nebula-matrix: add channel layer
From: Andrew Lunn <andrew@lunn.ch>
Date: 2026-02-27 23:32:04
Also in:
linux-doc, lkml
+ /* wmb */ + wmb();
What does the comment tell me which the function name does not? What would be more interesting in the comment is what is actually critical to be flushed.
quoted hunk ↗ jump to hunk
+static int nbl_chan_kick_tx_ring(struct nbl_channel_mgt *chan_mgt, + struct nbl_chan_info *chan_info) +{ + struct nbl_hw_ops *hw_ops = chan_mgt->hw_ops_tbl->ops; + struct nbl_chan_ring *txq = &chan_info->txq; + struct device *dev = chan_mgt->common->dev; + struct nbl_chan_tx_desc *tx_desc; + int i = 0; + + /* mb for tx notify */ + mb();
This comment is also not so useful. Why is a memory barrier needed here?
quoted hunk ↗ jump to hunk
+ NBL_UPDATE_QUEUE_TAIL_PTR(chan_info, hw_ops, chan_mgt, txq->tail_ptr, + NBL_MB_TX_QID); + + tx_desc = NBL_CHAN_TX_RING_TO_DESC(txq, txq->next_to_clean); + + while (!(tx_desc->flags & NBL_CHAN_TX_DESC_USED)) { + udelay(NBL_CHAN_TX_WAIT_US); + i++; + + if (!(i % NBL_CHAN_TX_REKICK_WAIT_TIMES)) + NBL_UPDATE_QUEUE_TAIL_PTR(chan_info, hw_ops, chan_mgt, + txq->tail_ptr, NBL_MB_TX_QID); + + if (i == NBL_CHAN_TX_WAIT_TIMES) { + dev_err(dev, "chan send message type: %d timeout\n", + tx_desc->msg_type); + return -EAGAIN;
Would -ETIMEDOUT be better here? What does it actually mean? That the firmware is dead?
quoted hunk ↗ jump to hunk
+static int nbl_chan_get_msg_id(struct nbl_chan_info *chan_info, + union nbl_chan_msg_id *msgid) +{ + struct nbl_chan_waitqueue_head *wait = NULL; + int valid_loc = chan_info->wait_head_index, i; + + for (i = 0; i < NBL_CHAN_QUEUE_LEN; i++) { + wait = &chan_info->wait[valid_loc]; + + if (wait->status != NBL_MBX_STATUS_WAITING) { + wait->msg_index = NBL_NEXT_ID(wait->msg_index, + NBL_CHAN_MSG_INDEX_MAX); + msgid->info.index = wait->msg_index; + msgid->info.loc = valid_loc; + + valid_loc = NBL_NEXT_ID(valid_loc, + chan_info->num_txq_entries - 1); + chan_info->wait_head_index = valid_loc; + return 0; + } + + valid_loc = + NBL_NEXT_ID(valid_loc, chan_info->num_txq_entries - 1); + } + + return -ENOSPC;
man 3 errno:
ENOSPC No space left on device (POSIX.1-2001).
Seems like something a file system would return. Why would this error
happen?
quoted hunk ↗ jump to hunk
+} + +static int nbl_chan_send_msg(struct nbl_channel_mgt *chan_mgt, + struct nbl_chan_send_info *chan_send) +{ + struct nbl_chan_info *chan_info = NBL_CHAN_MGT_TO_MBX(chan_mgt); + struct nbl_common_info *common = chan_mgt->common; + struct nbl_chan_waitqueue_head *wait_head; + union nbl_chan_msg_id msgid = { { 0 } }; + struct nbl_chan_tx_param tx_param = { 0 }; + int i = NBL_CHAN_TX_WAIT_ACK_TIMES, ret; + struct device *dev = common->dev; + + if (test_bit(NBL_CHAN_ABNORMAL, chan_info->state)) + return -EFAULT;
EFAULT Bad address (POSIX.1-2001).
This is about memory address, unexpected page faults, access outside
of the processes memory space.
quoted hunk ↗ jump to hunk
+#define NBL_CHAN_TX_RING_TO_DESC(tx_ring, i) \ + (&(((struct nbl_chan_tx_desc *)((tx_ring)->desc))[i])) +#define NBL_CHAN_RX_RING_TO_DESC(rx_ring, i) \ + (&(((struct nbl_chan_rx_desc *)((rx_ring)->desc))[i]))
Are the casts needed here?
quoted hunk ↗ jump to hunk
+struct nbl_chan_tx_desc { + u16 flags; + u16 srcid; + u16 dstid; + u16 data_len;
+ u16 buf_len;
quoted hunk ↗ jump to hunk
+ u64 buf_addr; + u16 msg_type; + u8 data[16]; + u16 msgid; + u8 rsv[26]; +} __packed;
This is a hardware descriptor? It is actually in memory like this, with the u64 buf_addr badly aligned? What was the hardware engineer thinking? Just swapping buf_addr and buf_len would of made it a lot better.
quoted hunk ↗ jump to hunk
+struct nbl_chan_rx_desc { + u16 flags; + u32 buf_len; + u16 buf_id; + u64 buf_addr; +} __packed;
Similarly badly designed.
quoted hunk ↗ jump to hunk
+++ b/drivers/net/ethernet/nebula-matrix/nbl/nbl_hw/nbl_hw_leonis/nbl_hw_leonis.c@@ -5,8 +5,168 @@ */ #include "nbl_hw_leonis.h" +static inline void nbl_hw_read_mbx_regs(struct nbl_hw_mgt *hw_mgt, u64 reg, + u8 *data, u32 len) +{
No inline functions in .c files. Let the compiler decide.
quoted hunk ↗ jump to hunk
+static void nbl_hw_rd_regs(struct nbl_hw_mgt *hw_mgt, u64 reg, u8 *data, + u32 len) +{ + u32 size = len / 4; + u32 i = 0; + + if (len % 4) + return; + + spin_lock(&hw_mgt->reg_lock); + + for (i = 0; i < size; i++) + *(u32 *)(data + i * sizeof(u32)) = + rd32(hw_mgt->hw_addr, reg + i * sizeof(u32)); + spin_unlock(&hw_mgt->reg_lock); +}
quoted hunk ↗ jump to hunk
+static u32 nbl_hw_get_host_pf_mask(struct nbl_hw_mgt *hw_mgt) +{ + u32 data; + + nbl_hw_rd_regs(hw_mgt, NBL_PCIE_HOST_K_PF_MASK_REG, (u8 *)&data, + sizeof(data)); + return data;
More code with dubious casts. data is a u32, which you cast to u8 for the call, and nbl_hw_rd_regs() casts it back to u32.