netdev - Re: [PATCH net-next v4] cadence: Add LSO support.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Tue, 8 Nov 2016 11:09:09 -0800
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Rafal Ozieblo <rafalo@...ence.com>, nicolas.ferre@...el.com,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v4] cadence: Add LSO support.

On 11/08/2016 05:41 AM, Rafal Ozieblo wrote:
> New Cadence GEM hardware support Large Segment Offload (LSO):
> TCP segmentation offload (TSO) as well as UDP fragmentation
> offload (UFO). Support for those features was added to the driver.
> 
> Signed-off-by: Rafal Ozieblo <rafalo@...ence.com>
> ---

> -#define MACB_MAX_TX_LEN		((unsigned int)((1 << MACB_TX_FRMLEN_SIZE) - 1))
> -#define GEM_MAX_TX_LEN		((unsigned int)((1 << GEM_TX_FRMLEN_SIZE) - 1))
> +/* Max length of transmit frame must be a multiple of 8 bytes */
> +#define MACB_TX_LEN_ALIGN	8
> +#define MACB_MAX_TX_LEN		((unsigned int)((1 << MACB_TX_FRMLEN_SIZE) - 1) & ~((unsigned int)(MACB_TX_LEN_ALIGN - 1)))
> +#define GEM_MAX_TX_LEN		((unsigned int)((1 << GEM_TX_FRMLEN_SIZE) - 1) & ~((unsigned int)(MACB_TX_LEN_ALIGN - 1)))
>  
>  #define GEM_MTU_MIN_SIZE	ETH_MIN_MTU
> +#define MACB_NETIF_LSO		(NETIF_F_TSO | NETIF_F_UFO)

Not a huge fan of this definition, since it is always used in conjuction
with netdev_features_t, having it expanded all the time is kind of nicer
for the reader, but this is just personal preference here.

>  
>  #define MACB_WOL_HAS_MAGIC_PACKET	(0x1 << 0)
>  #define MACB_WOL_ENABLED		(0x1 << 1)
> @@ -1223,7 +1228,8 @@ static void macb_poll_controller(struct net_device *dev)
>  
>  static unsigned int macb_tx_map(struct macb *bp,
>  				struct macb_queue *queue,
> -				struct sk_buff *skb)
> +				struct sk_buff *skb,
> +				unsigned int hdrlen)
>  {
>  	dma_addr_t mapping;
>  	unsigned int len, entry, i, tx_head = queue->tx_head;
> @@ -1231,14 +1237,27 @@ static unsigned int macb_tx_map(struct macb *bp,
>  	struct macb_dma_desc *desc;
>  	unsigned int offset, size, count = 0;
>  	unsigned int f, nr_frags = skb_shinfo(skb)->nr_frags;
> -	unsigned int eof = 1;
> -	u32 ctrl;
> +	unsigned int eof = 1, mss_mfs = 0;
> +	u32 ctrl, lso_ctrl = 0, seq_ctrl = 0;
> +
> +	/* LSO */
> +	if (skb_shinfo(skb)->gso_size != 0) {
> +		if (IPPROTO_UDP == (ip_hdr(skb)->protocol))

Most checks are usually done the other way with the left and right
member swapped.

> +			/* UDP - UFO */
> +			lso_ctrl = MACB_LSO_UFO_ENABLE;
> +		else
> +			/* TCP - TSO */
> +			lso_ctrl = MACB_LSO_TSO_ENABLE;
> +	}

>  
>  	/* Then, map paged data from fragments */
> @@ -1311,6 +1332,20 @@ static unsigned int macb_tx_map(struct macb *bp,
>  	desc = &queue->tx_ring[entry];
>  	desc->ctrl = ctrl;
>  
> +	if (lso_ctrl) {
> +		if (lso_ctrl == MACB_LSO_UFO_ENABLE)
> +			/* include header and FCS in value given to h/w */
> +			mss_mfs = skb_shinfo(skb)->gso_size +
> +					skb_transport_offset(skb) + 4;

ETH_FCS_LEN instead of 4?


> +static netdev_features_t macb_features_check(struct sk_buff *skb,
> +					     struct net_device *dev,
> +					     netdev_features_t features)
> +{
> +	unsigned int nr_frags, f;
> +	unsigned int hdrlen;
> +
> +	/* Validate LSO compatibility */
> +
> +	/* there is only one buffer */
> +	if (!skb_is_nonlinear(skb))
> +		return features;
> +
> +	/* length of header */
> +	hdrlen = skb_transport_offset(skb);
> +	if (IPPROTO_TCP == (ip_hdr(skb)->protocol))
> +		hdrlen += tcp_hdrlen(skb);

Same here, please reverse the left and right members, no need for
parenthesis aground ip_hdr(skb)->protocol.

> +
> +	/* For LSO:
> +	 * When software supplies two or more payload buffers all payload buffers
> +	 * apart from the last must be a multiple of 8 bytes in size.
> +	 */
> +	if (!IS_ALIGNED(skb_headlen(skb) - hdrlen, MACB_TX_LEN_ALIGN))
> +		return features & ~MACB_NETIF_LSO;
> +
> +	nr_frags = skb_shinfo(skb)->nr_frags;
> +	/* No need to check last fragment */
> +	nr_frags--;
> +	for (f = 0; f < nr_frags; f++) {
> +		const skb_frag_t *frag = &skb_shinfo(skb)->frags[f];
> +
> +		if (!IS_ALIGNED(skb_frag_size(frag), MACB_TX_LEN_ALIGN))
> +			return features & ~MACB_NETIF_LSO;
> +	}
> +	return features;
> +}
> +
>  static inline int macb_clear_csum(struct sk_buff *skb)
>  {
>  	/* no change for packets without checksum offloading */
> @@ -1374,7 +1456,27 @@ static int macb_start_xmit(struct sk_buff *skb, struct net_device *dev)
>  	struct macb *bp = netdev_priv(dev);
>  	struct macb_queue *queue = &bp->queues[queue_index];
>  	unsigned long flags;
> -	unsigned int count, nr_frags, frag_size, f;
> +	unsigned int desc_cnt, nr_frags, frag_size, f;
> +	unsigned int is_lso = 0, is_udp, hdrlen;
> +
> +	is_lso = (skb_shinfo(skb)->gso_size != 0);
> +
> +	if (is_lso) {
> +		is_udp = (IPPROTO_UDP == (ip_hdr(skb)->protocol));

Same here, and you may want to declare is_udp as boolean and do this:

		is_udp = !!(ip_hdr(skb)->protocl == IPPROTO_UDP);

> +
> +		/* length of headers */
> +		if (is_udp)
> +			/* only queue eth + ip headers separately for UDP */
> +			hdrlen = skb_transport_offset(skb);
> +		else
> +			hdrlen = skb_transport_offset(skb) + tcp_hdrlen(skb);
> +		if (skb_headlen(skb) < hdrlen) {
> +			netdev_err(bp->dev, "Error - LSO headers fragmented!!!\n");
> +			/* if this is required, would need to copy to single buffer */
> +			return NETDEV_TX_BUSY;
> +		}

>  
> +	if (is_lso) {
> +		if (is_udp)
> +			/* zero UDP checksum, not calculated by h/w for UFO */
> +			udp_hdr(skb)->check = 0;

is_udp is only set when (is_lso) is checked, so the two conditions are
redundant, just checking is_udp should be enough?
-- 
Florian