| Message ID | 1386926708-2343-1-git-send-email-freddy@asix.com.tw |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
> From: freddy@asix.com.tw ... > - skb = __netdev_alloc_skb_ip_align(dev->net, size, flags); > + if (dev->driver_info->flags & FLAG_HW_IPALIGN) > + skb = __netdev_alloc_skb(dev->net, size, flags); > + else > + skb = __netdev_alloc_skb_ip_align(dev->net, size, flags); Given the definition: static inline struct sk_buff *__netdev_alloc_skb_ip_align(struct net_device *dev, unsigned int length, gfp_t gfp) { struct sk_buff *skb = __netdev_alloc_skb(dev, length + NET_IP_ALIGN, gfp); if (NET_IP_ALIGN && skb) skb_reserve(skb, NET_IP_ALIGN); return skb; } It really ought to be possible to code that without an extra conditional. David -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2013年12月13日 18:36, David Laight wrote: >> From: freddy@asix.com.tw > ... >> - skb = __netdev_alloc_skb_ip_align(dev->net, size, flags); >> + if (dev->driver_info->flags & FLAG_HW_IPALIGN) >> + skb = __netdev_alloc_skb(dev->net, size, flags); >> + else >> + skb = __netdev_alloc_skb_ip_align(dev->net, size, flags); > Given the definition: > static inline struct sk_buff *__netdev_alloc_skb_ip_align(struct net_device *dev, > unsigned int length, gfp_t gfp) > { > struct sk_buff *skb = __netdev_alloc_skb(dev, length + NET_IP_ALIGN, gfp); > > if (NET_IP_ALIGN && skb) > skb_reserve(skb, NET_IP_ALIGN); > return skb; > } > > It really ought to be possible to code that without an extra conditional. > > David > > > > The AX88179_178A driver do need to know the value of NET_IP_ALIGN to determine whether enabling the feature that makes IP header align on a dword-aligned address, but according to the comments from David Miller, I need to consider all situations, not just for the case that NET_IP_ALIGN is zero, so the condition added in rx_submit is just used to determine whether reserving NET_IP_ALIGN bytes for each SKB. Freddy -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> From: Freddy Xin > On 2013年12月13日 18:36, David Laight wrote: > >> From: freddy@asix.com.tw > > ... > >> - skb = __netdev_alloc_skb_ip_align(dev->net, size, flags); > >> + if (dev->driver_info->flags & FLAG_HW_IPALIGN) > >> + skb = __netdev_alloc_skb(dev->net, size, flags); > >> + else > >> + skb = __netdev_alloc_skb_ip_align(dev->net, size, flags); > > Given the definition: > > static inline struct sk_buff *__netdev_alloc_skb_ip_align(struct net_device *dev, > > unsigned int length, gfp_t gfp) > > { > > struct sk_buff *skb = __netdev_alloc_skb(dev, length + NET_IP_ALIGN, gfp); > > > > if (NET_IP_ALIGN && skb) > > skb_reserve(skb, NET_IP_ALIGN); > > return skb; > > } > > > > It really ought to be possible to code that without an extra conditional. > > > > David > > The AX88179_178A driver do need to know the value of NET_IP_ALIGN > to determine whether enabling the feature that makes IP header align > on a dword-aligned address, but according to the comments from David > Miller, I need to consider all situations, not just for the case that > NET_IP_ALIGN is zero, so the condition added in rx_submit is just used to > determine whether reserving NET_IP_ALIGN bytes for each SKB. I was thinking of something like: skb = netdev_alloc_skb(dev, length + dev->skb_align, gfp); if (NET_IP_ALIGN && skb && !(ev->driver_flags & FLAG_HW_IPALIGN)) skb_reserve(skb, NET_IP_ALIGN); It might even be reasonable to remove the length adjustment - provided that all the later code uses the skb length. David
On 2013年12月16日 18:09, David Laight wrote: >> I was thinking of something like: skb = netdev_alloc_skb(dev, length >> + dev->skb_align, gfp); if (NET_IP_ALIGN && skb && !(ev->driver_flags >> & FLAG_HW_IPALIGN)) skb_reserve(skb, NET_IP_ALIGN); It might even be >> reasonable to remove the length adjustment - provided that all the >> later code uses the skb length. David Thanks for your advice. In the way you advised, does the dev->skb_align equal to NET_IP_ALIGN in the case that HW doesn't supoort IP alignment? In other words, dev->skb_align should be initialized to NET_IP_ALIGN in USBNET, and I can change its value to 0 in AX88179_178A driver, right? Freddy -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> From: Freddy Xin > On 2013年12月16日 18:09, David Laight wrote: > >> I was thinking of something like: skb = netdev_alloc_skb(dev, length > >> + dev->skb_align, gfp); if (NET_IP_ALIGN && skb && !(ev->driver_flags > >> & FLAG_HW_IPALIGN)) skb_reserve(skb, NET_IP_ALIGN); It might even be > >> reasonable to remove the length adjustment - provided that all the > >> later code uses the skb length. David > > Thanks for your advice. > In the way you advised, does the dev->skb_align equal > to NET_IP_ALIGN in the case that HW doesn't supoort > IP alignment? Yes > In other words, dev->skb_align should be initialized to > NET_IP_ALIGN in USBNET, and I can change its value to > 0 in AX88179_178A driver, right? I'd probably set a flag to request that usbnet set the offset to the correct value. That makes the initialisation a bit clearer. David
diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c index 8e8d0fc..04a2cc9 100644 --- a/drivers/net/usb/ax88179_178a.c +++ b/drivers/net/usb/ax88179_178a.c @@ -1374,7 +1374,7 @@ static const struct driver_info ax88179_info = { .link_reset = ax88179_link_reset, .reset = ax88179_reset, .stop = ax88179_stop, - .flags = FLAG_ETHER | FLAG_FRAMING_AX, + .flags = FLAG_ETHER | FLAG_FRAMING_AX | FLAG_HW_IPALIGN, .rx_fixup = ax88179_rx_fixup, .tx_fixup = ax88179_tx_fixup, }; @@ -1387,7 +1387,7 @@ static const struct driver_info ax88178a_info = { .link_reset = ax88179_link_reset, .reset = ax88179_reset, .stop = ax88179_stop, - .flags = FLAG_ETHER | FLAG_FRAMING_AX, + .flags = FLAG_ETHER | FLAG_FRAMING_AX | FLAG_HW_IPALIGN, .rx_fixup = ax88179_rx_fixup, .tx_fixup = ax88179_tx_fixup, }; @@ -1400,7 +1400,7 @@ static const struct driver_info sitecom_info = { .link_reset = ax88179_link_reset, .reset = ax88179_reset, .stop = ax88179_stop, - .flags = FLAG_ETHER | FLAG_FRAMING_AX, + .flags = FLAG_ETHER | FLAG_FRAMING_AX | FLAG_HW_IPALIGN, .rx_fixup = ax88179_rx_fixup, .tx_fixup = ax88179_tx_fixup, }; @@ -1413,7 +1413,7 @@ static const struct driver_info samsung_info = { .link_reset = ax88179_link_reset, .reset = ax88179_reset, .stop = ax88179_stop, - .flags = FLAG_ETHER | FLAG_FRAMING_AX, + .flags = FLAG_ETHER | FLAG_FRAMING_AX | FLAG_HW_IPALIGN, .rx_fixup = ax88179_rx_fixup, .tx_fixup = ax88179_tx_fixup, }; diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c index 8494bb5..8a618b8 100644 --- a/drivers/net/usb/usbnet.c +++ b/drivers/net/usb/usbnet.c @@ -473,7 +473,10 @@ static int rx_submit (struct usbnet *dev, struct urb *urb, gfp_t flags) return -ENOLINK; } - skb = __netdev_alloc_skb_ip_align(dev->net, size, flags); + if (dev->driver_info->flags & FLAG_HW_IPALIGN) + skb = __netdev_alloc_skb(dev->net, size, flags); + else + skb = __netdev_alloc_skb_ip_align(dev->net, size, flags); if (!skb) { netif_dbg(dev, rx_err, dev->net, "no rx skb\n"); usbnet_defer_kevent (dev, EVENT_RX_MEMORY); diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h index e303eef..e4855cf 100644 --- a/include/linux/usb/usbnet.h +++ b/include/linux/usb/usbnet.h @@ -116,6 +116,9 @@ struct driver_info { #define FLAG_MULTI_PACKET 0x2000 #define FLAG_RX_ASSEMBLE 0x4000 /* rx packets may span >1 frames */ #define FLAG_NOARP 0x8000 /* device can't do ARP */ +#define FLAG_HW_IPALIGN 0x10000 /* hardware adds headers to let the + * IP header align on a dword-aligned + * address */ /* init device ... can sleep, or cause probe() failure */ int (*bind)(struct usbnet *, struct usb_interface *);