Re: [PATCH net-next v13 2/5] net: rnpgbe: Add basic TX packet transmission support

Thank you for your contribution! Sashiko AI review found 5 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 · Low: 3

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] rnpgbe_probe() replaces the previously error-checked
  `dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(56))` (which aborted…
- [Medium] In this patch ndo_start_xmit becomes reachable before the TX
  ring software resources exist: rnpgbe_add_adapter() calls…
- [Low] In drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.h,
  RESV_DESC_NEEDED is documented as "2 desc gap to keep tail from…
- [Low] Two headers touched by the patch are not self-contained.
- [Low] The patch builds an RCU protocol for the
  mucse->tx_ring[]/rx_ring[] slots (adds `struct rcu_head rcu` to struct…

--

net: rnpgbe: Add basic TX packet transmission support

This adds the TX path for rnpgbe: the TX descriptor layout and buffer
management, rnpgbe_xmit_frame_ring(), TX ring setup/teardown, TX completion
handling in rnpgbe_clean_tx_irq(), and per-ring TX stats reported through
ndo_get_stats64(). It also switches the DMA mask setup to
dma_set_mask_and_coherent() and enables NETIF_F_SG/NETIF_F_HIGHDMA.

A few questions below.

[ ... ]

[Severity: Low]
The patch introduces an RCU protocol for these slots: struct mucse_q_vector
gains an rcu_head, rnpgbe_free_q_vector() switches from kfree() to
kfree_rcu(), and rnpgbe_get_stats64() reads the slots under
rcu_read_lock(). Should the arrays carry __rcu and should the publish and
consume sides use rcu_assign_pointer()/rcu_dereference() rather than
WRITE_ONCE()/READ_ONCE()?

In rnpgbe_alloc_q_vector() the store is:

	ring->pfvfnum = hw->pfvfnum;
	u64_stats_init(&ring->syncp);
	WRITE_ONCE(mucse->tx_ring[ring->queue_index], ring);

WRITE_ONCE() carries no release semantics, so the pointer store is not
ordered against the preceding ring->dev/ring->count/ring->netdev stores or
u64_stats_init(&ring->syncp), and rnpgbe_get_stats64() reads the slot with a
plain READ_ONCE().

Today this looks unreachable: rnpgbe_alloc_q_vector() runs from
rnpgbe_init_interrupt_scheme() before register_netdev(), and
rnpgbe_free_q_vector() runs either on the probe error path or after
unregister_netdev(), so no ndo_get_stats64 caller can race. Would it be
worth completing the idiom now, before any runtime queue reconfiguration or
reset path is added, and so sparse/RCU checkers can validate it?

[ ... ]

[Severity: Low]
This isn't a bug, but rnpgbe_hw.h has no #include lines at all while now
using GENMASK_U32() (and BIT() already). It builds only because the .c files
happen to pull in the right headers first. Would adding
#include <linux/bits.h> here make sense? The driver's other GENMASK_U32
users, rnpgbe_mbx.c and rnpgbe_mbx_fw.c, do include <linux/bitfield.h>.

[ ... ]

+static int rnpgbe_tx_map(struct mucse_ring *tx_ring,
+			 struct mucse_tx_buffer *first, u32 mac_ip_len,
+			 u32 tx_flags)
+{

[ ... ]

+	if (size) {
+		dma = dma_map_single(tx_ring->dev, skb->data, size,
+				     DMA_TO_DEVICE);
+		first->mapped_as_page = false;

[ ... ]

+	/* write last descriptor with RS and EOP bits */
+	tx_desc->vlan_cmd_bsz = build_ctob(tx_flags | M_TXD_CMD_EOP |
+					   M_TXD_CMD_RS,
+					   mac_ip_len, size);

[Severity: Low]
This isn't a bug, but rnpgbe_tx_map() only ever writes data descriptors
(pkt_addr plus build_ctob()), with tx_flags hardcoded to 0 and mac_ip_len
fixed at M_DEFAULT_MAC_IP_LEN, so the "1 desc for context descriptor"
comment on RESV_DESC_NEEDED in rnpgbe_lib.h describes a descriptor class the
code never emits. Should the comment be trimmed to the two-descriptor gap
until the offload code that needs the context descriptor lands?

[ ... ]

+netdev_tx_t rnpgbe_xmit_frame_ring(struct sk_buff *skb,
+				   struct mucse_ring *tx_ring)
+{

[ ... ]

+	/* record the location of the first descriptor for this packet */
+	first = &tx_ring->tx_buffer_info[tx_ring->next_to_use];
+	first->skb = skb;

[Severity: Medium]
Can this dereference a NULL tx_buffer_info during the first ifup?

At this patch rnpgbe_add_adapter() calls register_netdev() without
netif_carrier_off() or netif_tx_stop_all_queues(), and __dev_open() sets
__LINK_STATE_START before calling ndo_open, so all three conditions checked
by __dev_direct_xmit() are satisfied while rnpgbe_open() is still running:

	netif_running() && netif_carrier_ok() &&
	!netif_xmit_frozen_or_drv_stopped()

An AF_PACKET socket with PACKET_QDISC_BYPASS can then enter
rnpgbe_xmit_frame() -> rnpgbe_xmit_frame_ring() in the window inside
rnpgbe_open():

	err = netif_set_real_num_queues(netdev, mucse->num_tx_queues,
					mucse->num_rx_queues);
	if (err)
		goto err_free_irqs;

	err = rnpgbe_setup_all_tx_resources(mucse);

where tx_buffer_info is still NULL, and a slightly later landing reaches
writel(i, tx_ring->tail) in rnpgbe_tx_map() with tail still NULL.

The window is closed later in the series by "net: rnpgbe: Add link status
handling support", which adds netif_carrier_off() before register_netdev()
and at the top of rnpgbe_open(). Would it make sense to fold that
netif_carrier_off() into this patch so it stands on its own?

[ ... ]

[ ... ]

+#define M_MAX_TXD_PWR             12
+#define M_MAX_DATA_PER_TXD        (0x1 << M_MAX_TXD_PWR)
+#define TXD_USE_COUNT(S)          DIV_ROUND_UP((S), M_MAX_DATA_PER_TXD)
+#define DESC_NEEDED               (MAX_SKB_FRAGS + 4)
+/* 2 desc gap to keep tail from touching head */
+/* 1 desc for context descriptor */
+#define RESV_DESC_NEEDED          3

[Severity: Low]
Same self-containment question as for rnpgbe_hw.h: rnpgbe_lib.h has no
#include lines, yet now uses MAX_SKB_FRAGS and DIV_ROUND_UP here, and
netdev_tx_t, struct sk_buff, struct net_device and struct rtnl_link_stats64
in the prototypes below. Should it include <linux/skbuff.h> and
<linux/netdevice.h> so it does not depend on include order in the .c files?

[ ... ]

[Severity: Medium]
Should the return value of dma_set_mask_and_coherent() still be checked
here? The previous code aborted probe and printed "No usable DMA
configuration, aborting", and dma_set_mask_and_coherent() is a plain static
inline int with no __must_check, so a failure is now completely silent.

If dma_supported() rejects a 56-bit mask, neither the streaming nor the
coherent mask is changed, the device keeps the PCI core's 32-bit default,
and probe continues through pci_request_mem_regions() to register_netdev().

The dma_get_mask() test above keeps highmem frags away by not setting
NETIF_F_HIGHDMA, but it does not cover the linear-data mapping in
rnpgbe_tx_map():

	dma = dma_map_single(tx_ring->dev, skb->data, size,
			     DMA_TO_DEVICE);
	...
	if (dma_mapping_error(tx_ring->dev, dma))
		goto err_unmap;

nor the descriptor allocation in rnpgbe_setup_tx_resources():

	tx_ring->desc = dma_alloc_coherent(dev, tx_ring->size, &tx_ring->dma,
					   GFP_KERNEL);

So every frame whose linear data sits above 4GB would be dropped with only
the private per-ring dropped counter moving and no kernel message.

The commit message says only:

  "Configure streaming and coherent DMA masks for the hardware's 56-bit
  DMA addressing capability."

Could it also state that a DMA configuration failure is now non-fatal and
unreported, or could the check and the error path be restored?

The call is still unchecked at the end of the series, so no later patch
covers this.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/B8AF0FDE24FAFB51%2B20260920092424.345309-1-dong100%40mucse.com