Subject: Re: [PATCH v2 2/2] net: asix: handle packets crossing URB boundaries

From: Christian Riesch <christian.riesch-3mrvs1K0uXizZXS1Dc/[email-masked]>
Date: Wed, 19 Dec 2012 08:50:42 +0100

Hi Lucas,

On 2012-12-18 16:21, Lucas Stach wrote:
> ASIX AX88772B started to pack data even more tightly. Packets and the ASIX packet
> header may now cross URB boundaries. To handle this we have to introduce
> some state between individual calls to asix_rx_fixup().

cc'ed Eric Dumazet, he did some asix_rx_fixup fixes in spring.

>
> Signed-off-by: Lucas Stach <[email-masked]>
> ---
> I've running this patch for some weeks already now and it gets rid of all
> the commonly seen rx failures with AX88772B.
>
> v2: don't forget to free driver_private
> ---
>   drivers/net/usb/asix.h         | 11 ++++++
>   drivers/net/usb/asix_common.c  | 81 +++++++++++++++++++++++++++++-------------
>   drivers/net/usb/asix_devices.c | 17 +++++++++
>   3 Dateien geändert, 85 Zeilen hinzugefügt(+), 24 Zeilen entfernt(-)

Your patch breaks the driver of the AX88172A in 
drivers/net/usb/ax88172a.c. It uses the asix_rx_fixup() function as 
well, but you did not add the struct asix_rx_fixup_info to its 
driver_priv struct.

Regards, Christian

>
> diff --git a/drivers/net/usb/asix.h b/drivers/net/usb/asix.h
> index 7afe8ac..4dfdbf6 100644
> --- a/drivers/net/usb/asix.h
> +++ b/drivers/net/usb/asix.h
> @@ -167,6 +167,17 @@ struct asix_data {
>   	u8 res;
>   };
>
> +struct asix_rx_fixup_info {
> +	struct sk_buff *ax_skb;
> +	u32 header;
> +	u16 size;
> +	bool split_head;
> +};
> +
> +struct asix_private {
> +	struct asix_rx_fixup_info rx_fixup_info;
> +};
> +
>   /* ASIX specific flags */
>   #define FLAG_EEPROM_MAC		(1UL << 0)  /* init device MAC from eeprom */
>
> diff --git a/drivers/net/usb/asix_common.c b/drivers/net/usb/asix_common.c
> index 50d1673..17f9801 100644
> --- a/drivers/net/usb/asix_common.c
> +++ b/drivers/net/usb/asix_common.c
> @@ -53,44 +53,77 @@ void asix_write_cmd_async(struct usbnet *dev, u8 cmd, u16 value, u16 index,
>
>   int asix_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
>   {
> +	struct asix_private *dp = dev->driver_priv;
> +	struct asix_rx_fixup_info *rx = &dp->rx_fixup_info;
>   	int offset = 0;
>
> -	while (offset + sizeof(u32) < skb->len) {
> -		struct sk_buff *ax_skb;
> -		u16 size;
> -		u32 header = get_unaligned_le32(skb->data + offset);
> -
> -		offset += sizeof(u32);
> -
> -		/* get the packet length */
> -		size = (u16) (header & 0x7ff);
> -		if (size != ((~header >> 16) & 0x07ff)) {
> -			netdev_err(dev->net, "asix_rx_fixup() Bad Header Length\n");
> -			return 0;
> +	while (offset + sizeof(u16) <= skb->len) {
> +		u16 remaining = 0;
> +		unsigned char *data;
> +
> +		if (!rx->size) {
> +			if ((skb->len - offset == sizeof(u16)) ||
> +			    rx->split_head) {
> +				if(!rx->split_head) {
> +					rx->header = get_unaligned_le16(
> +							skb->data + offset);
> +					rx->split_head = true;
> +					offset += sizeof(u16);
> +					break;
> +				} else {
> +					rx->header |= (get_unaligned_le16(
> +							skb->data + offset)
> +							<< 16);
> +					rx->split_head = false;
> +					offset += sizeof(u16);
> +				}
> +			} else {
> +				rx->header = get_unaligned_le32(skb->data +
> +								offset);
> +				offset += sizeof(u32);
> +			}
> +
> +			/* get the packet length */
> +			rx->size = (u16) (rx->header & 0x7ff);
> +			if (rx->size != ((~rx->header >> 16) & 0x7ff)) {
> +				netdev_err(dev->net, "asix_rx_fixup() Bad Header Length 0x%x, offset %d\n",
> +					   rx->header, offset);
> +				rx->size = 0;
> +				return 0;
> +			}
> +			rx->ax_skb = netdev_alloc_skb_ip_align(dev->net,
> +							       rx->size);
> +			if (!rx->ax_skb)
> +				return 0;
>   		}
>
> -		if ((size > dev->net->mtu + ETH_HLEN + VLAN_HLEN) ||
> -		    (size + offset > skb->len)) {
> +		if (rx->size > dev->net->mtu + ETH_HLEN + VLAN_HLEN) {
>   			netdev_err(dev->net, "asix_rx_fixup() Bad RX Length %d\n",
> -				   size);
> +				   rx->size);
> +			kfree_skb(rx->ax_skb);
>   			return 0;
>   		}
> -		ax_skb = netdev_alloc_skb_ip_align(dev->net, size);
> -		if (!ax_skb)
> -			return 0;
>
> -		skb_put(ax_skb, size);
> -		memcpy(ax_skb->data, skb->data + offset, size);
> -		usbnet_skb_return(dev, ax_skb);
> +		if (rx->size > skb->len - offset) {
> +			remaining = rx->size - (skb->len - offset);
> +			rx->size = skb->len - offset;
> +		}
> +
> +		data = skb_put(rx->ax_skb, rx->size);
> +		memcpy(data, skb->data + offset, rx->size);
> +		if (!remaining)
> +			usbnet_skb_return(dev, rx->ax_skb);
>
> -		offset += (size + 1) & 0xfffe;
> +		offset += (rx->size + 1) & 0xfffe;
> +		rx->size = remaining;
>   	}
>
>   	if (skb->len != offset) {
> -		netdev_err(dev->net, "asix_rx_fixup() Bad SKB Length %d\n",
> -			   skb->len);
> +		netdev_err(dev->net, "asix_rx_fixup() Bad SKB Length %d, %d\n",
> +			   skb->len, offset);
>   		return 0;
>   	}
> +
>   	return 1;
>   }
>
> diff --git a/drivers/net/usb/asix_devices.c b/drivers/net/usb/asix_devices.c
> index 0ecc3bc..c651243 100644
> --- a/drivers/net/usb/asix_devices.c
> +++ b/drivers/net/usb/asix_devices.c
> @@ -495,9 +495,19 @@ static int ax88772_bind(struct usbnet *dev, struct usb_interface *intf)
>   		dev->rx_urb_size = 2048;
>   	}
>
> +	dev->driver_priv = kzalloc(sizeof(struct asix_private), GFP_KERNEL);
> +	if (!dev->driver_priv)
> +		return -ENOMEM;
> +
>   	return 0;
>   }
>
> +void ax88772_unbind(struct usbnet *dev, struct usb_interface *intf)
> +{
> +	if (dev->driver_priv)
> +		kfree(dev->driver_priv);
> +}
> +
>   static const struct ethtool_ops ax88178_ethtool_ops = {
>   	.get_drvinfo		= asix_get_drvinfo,
>   	.get_link		= asix_get_link,
> @@ -829,6 +839,10 @@ static int ax88178_bind(struct usbnet *dev, struct usb_interface *intf)
>   		dev->rx_urb_size = 2048;
>   	}
>
> +	dev->driver_priv = kzalloc(sizeof(struct asix_private), GFP_KERNEL);
> +	if (!dev->driver_priv)
> +			return -ENOMEM;
> +
>   	return 0;
>   }
>
> @@ -875,6 +889,7 @@ static const struct driver_info hawking_uf200_info = {
>   static const struct driver_info ax88772_info = {
>   	.description = "ASIX AX88772 USB 2.0 Ethernet",
>   	.bind = ax88772_bind,
> +	.unbind = ax88772_unbind,
>   	.status = asix_status,
>   	.link_reset = ax88772_link_reset,
>   	.reset = ax88772_reset,
> @@ -886,6 +901,7 @@ static const struct driver_info ax88772_info = {
>   static const struct driver_info ax88772b_info = {
>   	.description = "ASIX AX88772B USB 2.0 Ethernet",
>   	.bind = ax88772_bind,
> +	.unbind = ax88772_unbind,
>   	.status = asix_status,
>   	.link_reset = ax88772_link_reset,
>   	.reset = ax88772_reset,
> @@ -899,6 +915,7 @@ static const struct driver_info ax88772b_info = {
>   static const struct driver_info ax88178_info = {
>   	.description = "ASIX AX88178 USB 2.0 Ethernet",
>   	.bind = ax88178_bind,
> +	.unbind = ax88772_unbind,
>   	.status = asix_status,
>   	.link_reset = ax88178_link_reset,
>   	.reset = ax88178_reset,
>

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to [email-masked]
More majordomo info at  http://vger.kernel.org/majordomo-info.html