| Message ID | 1238050359-28415-1-git-send-email-peppe.cavallaro@st.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Giuseppe CAVALLARO <peppe.cavallaro@st.com> Date: Thu, 26 Mar 2009 07:52:39 +0100 > Move in memory all the frames with an incorrect alignment. > This is to prevent unaligned memory accesses into the upper layers. > Note 1: this is a penalty for some architecture like SH. > Note 2: indeed, this patch restores an old one posted three years ago > (http://marc.info/?l=linux-usb-devel&m=116578791817830&w=2). > > Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com> There are some gratuitous (and wrong) changes in here: > - u32 header; ... > + unsigned int header; It's a u32 whether you like it or not. Please don't change away from fixed sized types. Also isn't there a way we can prefixed what this packet offset is going to be? If so, we can adjust the skb_reserve() call the generic USB net code uses. Thanks. -- 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
David Miller wrote: > From: Giuseppe CAVALLARO <peppe.cavallaro@st.com> > Date: Thu, 26 Mar 2009 07:52:39 +0100 > > >> Move in memory all the frames with an incorrect alignment. >> This is to prevent unaligned memory accesses into the upper layers. >> Note 1: this is a penalty for some architecture like SH. >> Note 2: indeed, this patch restores an old one posted three years ago >> (http://marc.info/?l=linux-usb-devel&m=116578791817830&w=2). >> >> Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com> >> > > There are some gratuitous (and wrong) changes in here: > > >> - u32 header; >> > ... > >> + unsigned int header; >> > > It's a u32 whether you like it or not. Please don't > change away from fixed sized types. > I agree... it's a typo error. > Also isn't there a way we can prefixed what this packet > offset is going to be? If so, we can adjust the skb_reserve() > call the generic USB net code uses. > This was my first test. I had tried to change/adjust headroom but no success. Unfortunately, unaligned memory accesses seems to depend on the Asix HW that packs several incoming frames. So when these frames are 'unpacked' within the fix-up function, and pushed to the upper layer, they can have a wrong alignment, indeed. When no frame is packed all works fine and the IP never works with unaligned addresses. I think, the skb_reserve could actually help us, if this last scenario generated misaligned accesses. Please let me know if I'm missing something. Thanks for your feedback! Regards, Peppe > Thanks. > -- > 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 > > -- 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: Giuseppe CAVALLARO <peppe.cavallaro@st.com> Date: Thu, 26 Mar 2009 09:40:13 +0100 > Unfortunately, unaligned memory accesses seems to depend on the Asix HW > that packs several incoming frames. > So when these frames are 'unpacked' within the fix-up function, and > pushed to the upper layer, they can have a wrong alignment, indeed. > When no frame is packed all works fine and the IP never works with > unaligned addresses. > I think, the skb_reserve could actually help us, if this last scenario > generated misaligned accesses. > Please let me know if I'm missing something. The unpacker is taking a set of packet(s) in a USB buffer and copying them into SKB's right? That code should be where the offset is checked in the child driver, and adjustments made as-needed. This code seems to call the downstream driver callback after the damage is done. I think it needs to ask the driver to look for and indicate the offset before the building of the SKB is performed. -- 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
David Miller wrote: > From: Giuseppe CAVALLARO <peppe.cavallaro@st.com> > Date: Thu, 26 Mar 2009 09:40:13 +0100 > > >> Unfortunately, unaligned memory accesses seems to depend on the Asix HW >> that packs several incoming frames. >> So when these frames are 'unpacked' within the fix-up function, and >> pushed to the upper layer, they can have a wrong alignment, indeed. >> When no frame is packed all works fine and the IP never works with >> unaligned addresses. >> I think, the skb_reserve could actually help us, if this last scenario >> generated misaligned accesses. >> Please let me know if I'm missing something. >> > > The unpacker is taking a set of packet(s) in a USB buffer > and copying them into SKB's right? That code should be where > the offset is checked in the child driver, and adjustments > made as-needed. > > This code seems to call the downstream driver callback after > the damage is done. I think it needs to ask the driver to > look for and indicate the offset before the building of the > SKB is performed. > I understand your point of view. In any case, at first glance, I understand that the urb->transfer_buffer directly points to the preallocated skb data. These are filled by the HWs. I mean, the buffers are treated by the HW. So I guess, the meaning of the rx_fixup functions is just to solve this kind of situations. In fact, each usb net driver has an own fixup code according to their HW specifications. Peppe -- 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: Giuseppe CAVALLARO <peppe.cavallaro@st.com> Date: Thu, 26 Mar 2009 11:01:50 +0100 > In any case, at first glance, I understand that the urb->transfer_buffer > directly points to the preallocated skb data. > These are filled by the HWs. I mean, the buffers are treated by the HW. > So I guess, the meaning of the rx_fixup functions is just to solve this > kind of situations. > In fact, each usb net driver has an own fixup code according to their HW > specifications. Now I understand, thanks for the explanation. Please resubmit your patch for ASIX, and I will apply it. Thanks! -- 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
diff --git a/drivers/net/usb/asix.c b/drivers/net/usb/asix.c index 396f821..cea98be 100644 --- a/drivers/net/usb/asix.c +++ b/drivers/net/usb/asix.c @@ -298,26 +298,37 @@ asix_write_cmd_async(struct usbnet *dev, u8 cmd, u16 value, u16 index, static int asix_rx_fixup(struct usbnet *dev, struct sk_buff *skb) { - u8 *head; - u32 header; - char *packet; - struct sk_buff *ax_skb; - u16 size; - - head = (u8 *) skb->data; - memcpy(&header, head, sizeof(header)); + unsigned int header; + + memcpy(&header, skb->data, sizeof(header)); le32_to_cpus(&header); - packet = head + sizeof(header); skb_pull(skb, 4); while (skb->len > 0) { + struct sk_buff *ax_skb; + unsigned int size; + int offset; + if ((short)(header & 0x0000ffff) != ~((short)((header & 0xffff0000) >> 16))) { deverr(dev,"asix_rx_fixup() Bad Header Length"); } + /* get the packet length */ - size = (u16) (header & 0x0000ffff); + size = header & 0x0000ffff; + + /* Move in memory frames with incorrect alignment. + * This is to prevent unaligned memory accesses into + * the upper layers. */ + offset = NET_IP_ALIGN ? ((unsigned long)skb->data - + NET_IP_ALIGN) & 3 : 0; + + if (offset) { + skb->data -= offset; + skb->tail -= offset; + memmove(skb->data - offset, skb->data, skb->len); + } if ((skb->len) - ((size + 1) & 0xfffe) == 0) return 2; @@ -327,23 +338,18 @@ static int asix_rx_fixup(struct usbnet *dev, struct sk_buff *skb) } ax_skb = skb_clone(skb, GFP_ATOMIC); if (ax_skb) { - ax_skb->len = size; - ax_skb->data = packet; - skb_set_tail_pointer(ax_skb, size); + skb_trim(ax_skb, size); usbnet_skb_return(dev, ax_skb); - } else { + } else return 0; - } skb_pull(skb, (size + 1) & 0xfffe); if (skb->len == 0) break; - head = (u8 *) skb->data; - memcpy(&header, head, sizeof(header)); + memcpy(&header, skb->data, sizeof(header)); le32_to_cpus(&header); - packet = head + sizeof(header); skb_pull(skb, 4); }
Move in memory all the frames with an incorrect alignment. This is to prevent unaligned memory accesses into the upper layers. Note 1: this is a penalty for some architecture like SH. Note 2: indeed, this patch restores an old one posted three years ago (http://marc.info/?l=linux-usb-devel&m=116578791817830&w=2). Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com> --- drivers/net/usb/asix.c | 42 ++++++++++++++++++++++++------------------ 1 files changed, 24 insertions(+), 18 deletions(-)