Il giorno ven 20 mar 2026 alle ore 22:23 Mohsin Bashir
<[email protected]> ha scritto:
>
>
> > + * e1000_xdp_xmit_ring - transmit an XDP frame on the TX ring
> > + * @adapter: board private structure
> > + * @tx_ring: Tx descriptor ring
> > + * @xdpf: XDP frame to transmit
> > + *
> > + * Returns E1000_XDP_TX on success, E1000_XDP_CONSUMED on failure
> > + **/
> > +static int e1000_xdp_xmit_ring(struct e1000_adapter *adapter,
> > + struct e1000_ring *tx_ring,
> minor nit: alignment issue
>
Uh funny that checkpatch.pl doesn't notice it.
> > + if (unlikely((staterr & E1000_RXDEXT_ERR_FRAME_ERR_MASK) &&
> > + !(netdev->features & NETIF_F_RXALL))) {
> > + page_pool_put_full_page(adapter->page_pool,
> > + buffer_info->page, true);
> > + buffer_info->page = NULL;
> > + goto next_desc;
> > + }
> > +
> > + /* adjust length to remove Ethernet CRC */
> > + if (!(adapter->flags2 & FLAG2_CRC_STRIPPING)) {
> > + if (netdev->features & NETIF_F_RXFCS)
> > + total_rx_bytes -= 4;
>
> Looks like, total_rx_bytes can go negative here since it is not updated
> after being initialized to 0. Is this expected?
>
No, that's a bug. I'll count the crc size separately and take care of it.
>
> > -static inline void e1000_rx_hash(struct net_device *netdev, __le32 rss,
> > - struct sk_buff *skb)
> > -{
> > - if (netdev->features & NETIF_F_RXHASH)
> > - skb_set_hash(skb, le32_to_cpu(rss), PKT_HASH_TYPE_L3);
> > -}
> > -
>
> The function was just moved. Looks like an unrelated change?
>
> > +/**
> > + * e1000_xdp_setup - add/remove an XDP program
> > + * @netdev: network interface device structure
> > + * @bpf: XDP program setup structure
> > + **/
> > +static int e1000_xdp_setup(struct net_device *netdev, struct netdev_bpf
> > *bpf)
> > +{
> > + struct e1000_adapter *adapter = netdev_priv(netdev);
> > + struct bpf_prog *prog = bpf->prog, *old_prog;
> > + bool running = netif_running(netdev);
> > + bool need_reset;
> > +
> > + /* XDP is incompatible with jumbo frames */
> > + if (prog && netdev->mtu > ETH_DATA_LEN) {
> > + NL_SET_ERR_MSG_MOD(bpf->extack,
> > + "XDP is not supported with jumbo frames");
> > + return -EINVAL;
> > + }
> > +
> > + /* Validate frame fits in a single page with XDP headroom */
> > + if (prog && netdev->mtu + VLAN_ETH_HLEN + ETH_FCS_LEN +
> > + XDP_PACKET_HEADROOM > PAGE_SIZE) {
> > + NL_SET_ERR_MSG_MOD(bpf->extack,
> > + "Frame size too large for XDP");
> > + return -EINVAL;
> > + }
> > +
> > + old_prog = xchg(&adapter->xdp_prog, prog);
> > + need_reset = (!!prog != !!old_prog);
> > +
> > + /* Transition between XDP and non-XDP requires ring reconfiguration */
> > + if (need_reset && running)
> > + e1000e_close(netdev);
> > +
> > + if (old_prog)
> > + bpf_prog_put(old_prog);
> > +
> > + if (!need_reset)
> > + return 0;
> > +
> > + if (running)
> > + e1000e_open(netdev);
> > +
> > + return 0;
> > +}
> > +
>
> If I am reading it correctly, this can be problematic (maybe with very
> small likelihood). If e1000e_open() fails here, we will still return
> success. How about the following?
>
> if (running) {
> int err = e1000e_open(netdev);
>
> if (err) {
> /* Remove the XDP program since interface is down */
> xchg(&adapter->xdp_prog, NULL);
>
> return err;
> }
> }
>
Makes sense.
I addressed all the comments and I'll send a v4 soon, thanks!
--
Matteo Croce
perl -e 'for($t=0;;$t++){print chr($t*($t>>8|$t>>13)&255)}' |aplay
