linux-kernel - Re: [PATCH] net: usb: ax88179

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] [day] [month] [year] [list]

Message-ID: <20211112194755.6862ac1b@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Fri, 12 Nov 2021 19:47:55 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Jacky Chou <jackychou@...x.com.tw>
Cc:     davem@...emloft.net, linux-usb@...r.kernel.org,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        louis@...x.com.tw
Subject: Re: [PATCH] net: usb: ax88179_178a: add TSO feature

On Fri, 12 Nov 2021 11:33:22 +0800 Jacky Chou wrote:
> On low-effciency embedded platforms, transmission performance is poor
> due to on Bulk-out with single packet.
> Adding TSO feature improves the transmission performance and reduces
> the number of interrupt caused by Bulk-out complete.
> 
> Reference to module, net: usb: aqc111.
> 
> Signed-off-by: Jacky Chou <jackychou@...x.com.tw>
> ---
>  drivers/net/usb/ax88179_178a.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c
> index c13167183..866954155 100644
> --- a/drivers/net/usb/ax88179_178a.c
> +++ b/drivers/net/usb/ax88179_178a.c
> @@ -1368,6 +1368,9 @@ static int ax88179_bind(struct usbnet *dev, struct usb_interface *intf)
>  	dev->net->needed_headroom = 8;
>  	dev->net->max_mtu = 4088;
>  
> +	if (usb_device_no_sg_constraint(dev->udev))
> +		dev->can_dma_sg = 1;

Why don't you use this information...

>  	/* Initialize MII structure */
>  	dev->mii.dev = dev->net;
>  	dev->mii.mdio_read = ax88179_mdio_read;
> @@ -1377,11 +1380,14 @@ static int ax88179_bind(struct usbnet *dev, struct usb_interface *intf)
>  	dev->mii.phy_id = 0x03;
>  	dev->mii.supports_gmii = 1;
>  
> -	dev->net->features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
> -			      NETIF_F_RXCSUM;
> +	dev->net->features |= NETIF_F_SG | NETIF_F_IP_CSUM |
> +			      NETIF_F_IPV6_CSUM | NETIF_F_RXCSUM | NETIF_F_TSO;

... to conditionally set the NETIF_F_SG flag?

> -	dev->net->hw_features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
> -				 NETIF_F_RXCSUM;
> +	dev->net->hw_features |= NETIF_F_SG | NETIF_F_IP_CSUM |
> +				 NETIF_F_IPV6_CSUM | NETIF_F_RXCSUM |
> +				 NETIF_F_TSO;

If you want to enable all the features just write:

	dev->net->hw_features |= dev->net->features;

No need to repeat all the flags.

> +	netif_set_gso_max_size(dev->net, 16384);
>  
>  	/* Enable checksum offload */
>  	*tmp = AX_RXCOE_IP | AX_RXCOE_TCP | AX_RXCOE_UDP |
> @@ -1537,6 +1543,10 @@ ax88179_tx_fixup(struct usbnet *dev, struct sk_buff *skb, gfp_t flags)
>  
>  	headroom = skb_headroom(skb) - 8;
>  
> +	if (!dev->can_dma_sg && (dev->net->features & NETIF_F_SG) &&
> +	    skb_linearize(skb))
> +		return NULL;
> +
>  	if ((skb_header_cloned(skb) || headroom < 0) &&
>  	    pskb_expand_head(skb, headroom < 0 ? 8 : 0, 0, GFP_ATOMIC)) {
>  		dev_kfree_skb_any(skb);

Okay... where is the TSO implementation? The TCP stack passes
parameters like MSS which you need to inform the device about.
You should also count TSO packets as multiple packets in stats.

Are you sure this device supports TSO and not just SG?