| Message ID | 4C642AA9.1060404@parrot.com |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
Matthieu CASTET wrote: > memmove is our friend : > the buffer allocated in usbnet got an offset. > All you have to do it remove this offset and memmove the data. That > what I did [1], and why it is better to do it in usb driver. > > > [1] http://article.gmane.org/gmane.linux.usb.general/28700 The problem I see with this approach is that it requires the usb driver to be aware of the device controller's quirks (hence the gadget_dma32()) your patch adds. That appears to be the norm (and maybe unavoidable) on the gadget side but on the host side things aren't supposed to work like that. The expectation for USB hosts is that any properly written usb driver will work with any properly written host controller driver. I don't think it's reasonable to expect driver developers to test their code with all the HCDs, nor expect HCD developers to test their code with all the drivers. So I think a policy needs to be defined to ensure this and enforced in the code. I can see two possible methods: 1) Require that usb drivers submit buffers obtained from kmalloc() and friends with no extra offsets. If they want some other alignment later they can use memmove in the completion handler. Enforce this in the core by checking the buffer pointers are aligned to ARCH_KMALLOC_MINALIGN or 2) Require that usb_submit_urb() accept byte aligned buffers. Enforce this by a new test in usbtest (which all HCDs are expected to pass). Implement it either in individual HCDs that require it or by letting HCDs inform the core of their requirements and have the core do the alignment (as it already does the dma mapping). Of course HCDs that can implement byte aligned transfers (either natively or using tricks such as the one Russell suggested) should do so. I think 2) is the better solution because: a) Solution 1 will impose a runtime overhead even on platforms / HCDs that don't need it (including the most common cases) b) There are more drivers than HCDs Martin -- 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
Am Freitag, 13. August 2010, 12:06:54 schrieb Martin Fuzzey: > So I think a policy needs to be defined to ensure this and enforced in > the code. I can see two possible methods: > > 1) Require that usb drivers submit buffers obtained from kmalloc() and > friends with no extra offsets. If they want some other alignment later > they can use memmove in the completion handler. Enforce this in the core > by checking the buffer pointers are aligned to ARCH_KMALLOC_MINALIGN > > or > > 2) Require that usb_submit_urb() accept byte aligned buffers. Enforce > this by a new test in usbtest (which all HCDs are expected to pass). > Implement it either in individual HCDs that require it or by letting > HCDs inform the core of their requirements and have the core do the > alignment (as it already does the dma mapping). Of course HCDs that can > implement byte aligned transfers (either natively or using tricks such > as the one Russell suggested) should do so. > > I think 2) is the better solution because: > a) Solution 1 will impose a runtime overhead even on platforms / HCDs > that don't need it (including the most common cases) > b) There are more drivers than HCDs Yes. But it doesn't prevent you from publishing this information so drivers can help the lower levels. Regards Oliver -- 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
> Subject: Re: Problem with non aligned DMA in usbnet on ARM I remain unconvinced the bug is anywhere except in whatever host controller is rejecting the buffers it's given. Does anyone have proof the bug is elsewhere? Think for a moment what chaos the kernel would be if arbitrary subsystems were allowed to introduce random DMA alignment "requirements". It would not be possible to pass buffers between subsystems with any success at all... > > 1) Require that usb drivers submit buffers obtained > from kmalloc() and > friends Requirements are already documented with the Description of what memory is DMA-able. It's not kmalloc() specifically that's required. with no extra offsets. Offsets have nothing to do with being DMA-able. They might have to do with working around hardware flaws though. (ISTR PXA255 also had a 32-bit goof, making its DMA engine unusable for most purposes.) > > 2) Require that usb_submit_urb() accept byte aligned buffers. I believe we already require that. Of course, the requirement is on HCDs, not on that call. -- 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
Am Freitag, 13. August 2010, 15:42:32 schrieb David Brownell: > > > 2) Require that usb_submit_urb() accept byte aligned buffers. > > I believe we already require that. Of course, > the requirement is on HCDs, not on th Divers fail if HCDs can't do it. But we've never explicitly said that HCDs need to be able to do that. We should probably document that requirement. Regards Oliver -- 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/usb/gadget/gadget_chips.h b/drivers/usb/gadget/gadget_chips.h index 1edbc12..ed3ee67 100644 --- a/drivers/usb/gadget/gadget_chips.h +++ b/drivers/usb/gadget/gadget_chips.h @@ -214,4 +214,14 @@ static inline bool gadget_supports_altsettings(struct usb_gadget *gadget) return true; } +/** + * gadget_dma32 - return true if we want buffer aligned on 32 bits (for dma) + * @gadget: the gadget in question + */ +static inline bool gadget_dma32(struct usb_gadget *gadget) +{ + if (gadget_is_musbhdrc(gadget)) + return true; + return false; +} #endif /* __GADGET_CHIPS_H */ diff --git a/drivers/usb/gadget/u_ether.c b/drivers/usb/gadget/u_ether.c index 84ca195..697af90 100644 --- a/drivers/usb/gadget/u_ether.c +++ b/drivers/usb/gadget/u_ether.c @@ -249,7 +249,12 @@ rx_submit(struct eth_dev *dev, struct usb_request *req, gfp_t gfp_flags) * but on at least one, checksumming fails otherwise. Note: * RNDIS headers involve variable numbers of LE32 values. */ - skb_reserve(skb, NET_IP_ALIGN); + /* + * RX: Do not move data by IP_ALIGN: + * if your DMA controller cannot handle it + */ + if (!gadget_dma32(dev->gadget)) + skb_reserve(skb, NET_IP_ALIGN); req->buf = skb->data; req->length = size; @@ -282,6 +287,12 @@ static void rx_complete(struct usb_ep *ep, struct usb_request *req) /* normal completion */ case 0: skb_put(skb, req->actual); + if (gadget_dma32(dev->gadget) && NET_IP_ALIGN) { + u8 *data = skb->data; + size_t len = skb_headlen(skb); + skb_reserve(skb, NET_IP_ALIGN); + memmove(skb->data, data, len); + } if (dev->unwrap) { unsigned long flags; @@ -573,6 +584,24 @@ static netdev_tx_t eth_start_xmit(struct sk_buff *skb, length = skb->len; } + + /* + * Align data to 32bit if the dma controller requires it + */ + if (gadget_dma32(dev->gadget)) { + unsigned long align = (unsigned long)skb->data & 3; + if (WARN_ON(skb_headroom(skb) < align)) { + dev_kfree_skb_any(skb); + goto drop; + } else if (align) { + u8 *data = skb->data; + size_t len = skb_headlen(skb); + skb->data -= align; + memmove(skb->data, data, len); + skb_set_tail_pointer(skb, len); + } + } + req->buf = skb->data; req->context = skb; req->complete = tx_complete;