> Thank you for your contribution! Sashiko AI review found more severe > issue(s) in other patches of this series. This mail is only an FYI, in case > you want to address the nit picks below when you respin. > > Low priority / nit picks for your consideration: > > - [Low] The authorship and Signed-off-by chain disagree. > > Pre-existing issues: > - [High] At this commit, stmmac_rx() still uses `skb == NULL` to decide > that a descriptor starts a new frame. > - [High] This problem wasn't introduced by this patch, but the commit > message relies on it being absent. > > -- > > net: stmmac: take ownership of saved RX state at poll entry > > This commit makes stmmac_rx() and stmmac_rx_zc() take the saved > partial-frame state at poll entry. A new in_progress flag tracks whether > a frame is still incomplete, and the state is saved again only in that > case. It also frees the saved skb in stmmac_reset_rx_queue() and in > __free_dma_rx_desc_resources(). > > > Signed-off-by: James Hilliard > > Reviewed-by: Nicolai Buchwitz > > Signed-off-by: Lorenzo Bianconi > > [Severity: Low] > This isn't a bug, but the author field and the Signed-off-by chain don't > match. The commit author is Lorenzo Bianconi. The first Signed-off-by is > James Hilliard, and there is no Co-developed-by tag. I will fix in the next revision. > > If James wrote the patch, should it carry a "From: James Hilliard > " line so git credits him as the author? If > Lorenzo is the author, does James's sign-off need a matching > Co-developed-by tag? > > The other patches in the series carry only Lorenzo's sign-off. That > suggests this patch was written by James and picked into the series. > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > > index 53782ab55e125..8fd2acb25fb60 100644 > > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > > [ ... ] > > > @@ -5958,9 +5970,7 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue) > > STMMAC_XDP_REDIRECT)) { > > xdp_status |= xdp_res; > > buf->page = NULL; > > - skb = NULL; > > - count++; > > - continue; > > + goto next; > > } > > } > > } > > [Severity: High] > This is a pre-existing issue, and this patch doesn't change how it > behaves. What happens here when the XDP verdict is taken on a descriptor > that still has rx_not_ls set? Since this issue has not been introduced by this patch I guess we can fix it with a dedicated patch. > > stmmac_rx() uses skb == NULL to mean a new frame is starting. The goto > next now also clears in_progress, and error stays 0. The remaining > descriptors of the same hardware frame would then each be handled as the > first descriptor of a new frame. > > The XDP_CONSUMED branch just above has the same shape: > > skb = NULL; > > if (unlikely((status & rx_not_ls))) > goto read_again; > > So does the napi_build_skb() failure path: > > if (!skb) { > page_pool_recycle_direct(rx_q->page_pool, > buf->page); > buf->page = NULL; > rx_dropped++; > count++; > goto drain_data; > } > > In each of these three cases, could the middle and tail descriptors be > passed to XDP and built into an skb with a stale or zero len? Could > buf1_len then underflow and produce an skb length near 4G? > > An on-link peer can reach this by sending a frame that spans several > descriptors while an XDP program returns a non-PASS verdict. A > napi_build_skb() failure on such a frame reaches it too. > > Later patches in this series seem to fix it. "net: stmmac: drop > multi-buffer frames in XDP mode" sets error on non-last descriptors when > XDP is attached. "net: stmmac: rework stmmac_rx to support XDP rx > multi-buff" runs XDP and stmmac_build_skb() only on the last descriptor > and sends build failures to error_free_frag. > > [ ... ] > > > @@ -8399,6 +8408,9 @@ static void stmmac_reset_rx_queue(struct stmmac_priv *priv, u32 queue) > > { > > struct stmmac_rx_queue *rx_q = &priv->dma_conf.rx_queue[queue]; > > > > + dev_kfree_skb_any(rx_q->state.skb); > > + rx_q->state.skb = NULL; > > + rx_q->state_saved = false; > > rx_q->cur_rx = 0; > > rx_q->dirty_rx = 0; > > } > > [Severity: High] > This isn't a bug introduced by this patch, but the commit message says > that, without this change, the first poll "resumes that stale skb with > descriptors from the freshly initialized ring". Is the ring actually > re-initialized on the stmmac_resume() path? Since this issue has not been introduced by this patch I guess we can fix it with a dedicated patch. Regards, Lorenzo > > stmmac_resume() does: > > stmmac_reset_queues_param(priv); > > stmmac_free_tx_skbufs(priv); > stmmac_clear_descriptors(priv, &priv->dma_conf); > > ret = stmmac_hw_setup(ndev); > > None of these calls refills buf_pool or rewrites the descriptor buffer > addresses. stmmac_clear_rx_descriptors() calls stmmac_init_rx_desc() on > every entry. On dwmac4 that only sets the owner bits: > > static void dwmac4_rd_init_rx_desc(struct dma_desc *p, int disable_rx_ic, > u8 descriptor_mode, int end, int bfsize) > { > dwmac4_set_rx_owner(p, disable_rx_ic); > } > > Suppose stmmac_rx_refill() hit a page_pool allocation failure before > suspend: > > if (!buf->page) { > buf->page = page_pool_alloc_pages(rx_q->page_pool, gfp); > if (!buf->page) > break; > } > > Then the entries between dirty_rx and cur_rx still have buf->page == > NULL. On resume, cur_rx and dirty_rx are reset to 0 and every descriptor > goes back to the DMA. When one of those entries completes, can > stmmac_rx() call page_address(buf->page) and napi_build_skb() on a NULL > page? > > There is a similar problem on dwmac4/xgmac. Descriptors that the hardware > writes back between napi_disable() in stmmac_suspend() and > stmmac_stop_all_dma() have RDES0/RDES1 overwritten with status words. > If those descriptors are given back to the DMA, would it write to a > bogus buffer address? > > The resume sequence is the same at the end of the series. > > -- > Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261018-stmmac-rx-mb-v8-0-5f58c0b25849%40oss.qualcomm.com