Hi Simon,
> -----Original Message-----
> From: Simon Horman <simon.horman@xxxxxxxxxxxx>
> Sent: Thursday, 13 July 2023 11:29
> To: Sriram Yagnaraman <sriram.yagnaraman@xxxxxxxx>
> Cc: intel-wired-lan@xxxxxxxxxxxxxxxx; bpf@xxxxxxxxxxxxxxx;
> netdev@xxxxxxxxxxxxxxx; Jesse Brandeburg <jesse.brandeburg@xxxxxxxxx>;
> Tony Nguyen <anthony.l.nguyen@xxxxxxxxx>; David S . Miller
> <davem@xxxxxxxxxxxxx>; Eric Dumazet <edumazet@xxxxxxxxxx>; Jakub
> Kicinski <kuba@xxxxxxxxxx>; Paolo Abeni <pabeni@xxxxxxxxxx>; Alexei
> Starovoitov <ast@xxxxxxxxxx>; Daniel Borkmann <daniel@xxxxxxxxxxxxx>;
> Jesper Dangaard Brouer <hawk@xxxxxxxxxx>; John Fastabend
> <john.fastabend@xxxxxxxxx>; Björn Töpel <bjorn@xxxxxxxxxx>; Magnus
> Karlsson <magnus.karlsson@xxxxxxxxx>; Maciej Fijalkowski
> <maciej.fijalkowski@xxxxxxxxx>; Jonathan Lemon
> <jonathan.lemon@xxxxxxxxx>
> Subject: Re: [PATCH iwl-next v2 3/4] igb: add AF_XDP zero-copy Rx support
>
> On Tue, Jul 11, 2023 at 01:47:04PM +0200, Sriram Yagnaraman wrote:
> > Add support for AF_XDP zero-copy receive path.
> >
> > When AF_XDP zero-copy is enabled, the rx buffers are allocated from
> > the xsk buff pool using igb_alloc_rx_buffers_zc.
> >
> > Use xsk_pool_get_rx_frame_size to set SRRCTL rx buf size when
> > zero-copy is enabled.
> >
> > Signed-off-by: Sriram Yagnaraman <sriram.yagnaraman@xxxxxxxx>
>
> Hi Sriram,
>
> > +bool igb_alloc_rx_buffers_zc(struct igb_ring *rx_ring, u16 count) {
> > + union e1000_adv_rx_desc *rx_desc;
> > + u32 nb_buffs_extra = 0, nb_buffs;
> > + u16 ntu = rx_ring->next_to_use;
> > + u16 total_count = count;
> > + struct xdp_buff **xdp;
> > +
> > + rx_desc = IGB_RX_DESC(rx_ring, ntu);
> > + xdp = &rx_ring->rx_buffer_info_zc[ntu];
> > +
> > + if (ntu + count >= rx_ring->count) {
> > + nb_buffs_extra = igb_fill_rx_descs(rx_ring->xsk_pool, xdp,
> > + rx_desc,
> > + rx_ring->count - ntu);
> > + if (nb_buffs_extra != rx_ring->count - ntu) {
> > + ntu += nb_buffs_extra;
> > + goto exit;
>
> nb_buffs is uninitialised here...
Thank you, will fix it in v3
>
> > + }
> > + rx_desc = IGB_RX_DESC(rx_ring, 0);
> > + xdp = rx_ring->rx_buffer_info_zc;
> > + ntu = 0;
> > + count -= nb_buffs_extra;
> > + }
> > +
> > + nb_buffs = igb_fill_rx_descs(rx_ring->xsk_pool, xdp, rx_desc, count);
> > + ntu += nb_buffs;
> > + if (ntu == rx_ring->count)
> > + ntu = 0;
> > +
> > + /* clear the length for the next_to_use descriptor */
> > + rx_desc = IGB_RX_DESC(rx_ring, ntu);
> > + rx_desc->wb.upper.length = 0;
> > +
> > +exit:
> > + if (rx_ring->next_to_use != ntu) {
> > + rx_ring->next_to_use = ntu;
> > +
> > + /* Force memory writes to complete before letting h/w
> > + * know there are new descriptors to fetch. (Only
> > + * applicable for weak-ordered memory model archs,
> > + * such as IA-64).
> > + */
> > + wmb();
> > + writel(ntu, rx_ring->tail);
> > + }
> > +
> > + return total_count == (nb_buffs + nb_buffs_extra);
>
> But it is used here.
>
> ...
>
> The following will tell you about this problem:
> - Smatch
> - clang-16 W=1 build [-Wsometimes-uninitialized]
> - gcc-12 build with -Wmaybe-uninitialized
