Re: [PATCH net-next v3 1/2] e1000e: add basic XDP support

[Mohsin Bashir <mohsin.bashr@gmail.com>]

> + * 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

> +		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?


> -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;
     }
}