[RFC] MUSB: Workaround for Ethernet data alignment issue

[RFC] MUSB: Workaround for Ethernet data alignment issue

[Patra, Nilkesh]

On the latest version of MUSB, DMA is not working properly if the Data in the packet doesn't start at 32bit aligned address. This issue was found while using MUSB as g_ether. The basic ping does not work, if the data in the Ethernet  packet does not start at 32bit aligned address.

To overcome this issue, we found one solution mentioned in the below patch. This patch moves data (skb->data) by 2 bytes backwards in ethernet packet, which is nothing but skb->head.

I want to know, if there are any better approaches to fix this issue.

Signed-off-by: Nilkesh Patra <nilkesh.patra-l0cyMroinI0@public.gmane.org>
---
 drivers/usb/gadget/u_ether.c |   14 ++++++++++----
 1 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/gadget/u_ether.c b/drivers/usb/gadget/u_ether.c
index 2fc02bd..432b1bd 100644
--- a/drivers/usb/gadget/u_ether.c
+++ b/drivers/usb/gadget/u_ether.c
@@ -249,7 +249,6 @@ 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);
 
 	req->buf = skb->data;
 	req->length = size;
@@ -500,6 +499,8 @@ static netdev_tx_t eth_start_xmit(struct sk_buff *skb,
 	unsigned long		flags;
 	struct usb_ep		*in;
 	u16			cdc_filter;
+	int			i = 0;
+
 
 	spin_lock_irqsave(&dev->lock, flags);
 	if (dev->port_usb) {
@@ -573,9 +574,6 @@ static netdev_tx_t eth_start_xmit(struct sk_buff *skb,
 
 		length = skb->len;
 	}
-	req->buf = skb->data;
-	req->context = skb;
-	req->complete = tx_complete;
 
 	/* use zlp framing on tx for strict CDC-Ether conformance,
 	 * though any robust network rx path ignores extra padding.
@@ -585,6 +583,14 @@ static netdev_tx_t eth_start_xmit(struct sk_buff *skb,
 	if (!dev->zlp && (length % in->maxpacket) == 0)
 		length++;
 
+	if ((int)skb->data & 0x2) {
+		skb_push(skb, 2);
+		for (i = 0; i < length; i++)
+			skb->data[i] = skb->data[i+2];
+	}
+	req->buf = skb->data;
+	req->context = skb;
+	req->complete = tx_complete;
 	req->length = length;
 
 	/* throttle highspeed IRQ rate back slightly */
-- 
1.5.4.3
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [RFC] MUSB: Workaround for Ethernet data alignment issue

[Pandita, Vikram]

Nilkesh

>-----Original Message-----
>From: linux-omap-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org [mailto:linux-omap-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org] On Behalf Of Patra,
>Nilkesh
>Sent: Sunday, December 13, 2009 10:51 PM
>To: linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>Cc: linux-omap-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; David Brownell; Gadiyar, Anand
>Subject: [RFC] MUSB: Workaround for Ethernet data alignment issue
>
>On the latest version of MUSB, DMA is not working properly if the Data in the packet doesn't start at
>32bit aligned address. This issue was found while using MUSB as g_ether. The basic ping does not
>work, if the data in the Ethernet  packet does not start at 32bit aligned address.
>
>To overcome this issue, we found one solution mentioned in the below patch. This patch moves data
>(skb->data) by 2 bytes backwards in ethernet packet, which is nothing but skb->head.
>
>I want to know, if there are any better approaches to fix this issue.

I would NAK the first part of patch for following reasons (and suggestions are inlined):
a) this change is specific to OMAP hw only and so should not try to change generic files like usb/gadget/u_ether.c
that get compiled for all the architectures having usb.

b) there are two issues to be handled separately ( separate patches are good for review )
	RX side buffer alignments handling
	TX side buffer alignments handling

>
>Signed-off-by: Nilkesh Patra <nilkesh.patra-l0cyMroinI0@public.gmane.org>
>---
> drivers/usb/gadget/u_ether.c |   14 ++++++++++----
> 1 files changed, 10 insertions(+), 4 deletions(-)
>
>diff --git a/drivers/usb/gadget/u_ether.c b/drivers/usb/gadget/u_ether.c
>index 2fc02bd..432b1bd 100644
>--- a/drivers/usb/gadget/u_ether.c
>+++ b/drivers/usb/gadget/u_ether.c
>@@ -249,7 +249,6 @@ 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);

NAK. Do not remove this line, 
Instead over-ride the NET_IP_ALIGN macro for your architecture. 

One suggestion to do so:

diff --git a/arch/arm/include/asm/system.h b/arch/arm/include/asm/system.h
index 058e7e9..b16c5f7 100644
--- a/arch/arm/include/asm/system.h
+++ b/arch/arm/include/asm/system.h
@@ -514,6 +514,10 @@ static inline unsigned long long __cmpxchg64_mb(volatile void *ptr,

 #define arch_align_stack(x) (x)

+#if defined(CONFIG_ARCH_OMAP3) || defined(CONFIG_ARCH_OMAP4)
+#define NET_IP_ALIGN 0
+#endif
+
 #endif /* __KERNEL__ */

 #endif

This has been verified to work for OMAP3630.

>
> 	req->buf = skb->data;
> 	req->length = size;
>@@ -500,6 +499,8 @@ static netdev_tx_t eth_start_xmit(struct sk_buff *skb,
> 	unsigned long		flags;
> 	struct usb_ep		*in;
> 	u16			cdc_filter;
>+	int			i = 0;
>+
>
> 	spin_lock_irqsave(&dev->lock, flags);
> 	if (dev->port_usb) {
>@@ -573,9 +574,6 @@ static netdev_tx_t eth_start_xmit(struct sk_buff *skb,
>
> 		length = skb->len;
> 	}
>-	req->buf = skb->data;
>-	req->context = skb;
>-	req->complete = tx_complete;

This change would be to handle TX un-alignments. 
So I would prefer to handle it separately ( a separate patch)
Have you explored the option of over-riding #define NET_SKB_PAD ??

>
> 	/* use zlp framing on tx for strict CDC-Ether conformance,
> 	 * though any robust network rx path ignores extra padding.
>@@ -585,6 +583,14 @@ static netdev_tx_t eth_start_xmit(struct sk_buff *skb,
> 	if (!dev->zlp && (length % in->maxpacket) == 0)
> 		length++;
>
>+	if ((int)skb->data & 0x2) {
>+		skb_push(skb, 2);

Where have you made sure that skb->data allocation has this extra space of two bytes? 

>+		for (i = 0; i < length; i++)
>+			skb->data[i] = skb->data[i+2];
>+	}
>+	req->buf = skb->data;
>+	req->context = skb;
>+	req->complete = tx_complete;
> 	req->length = length;
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [RFC] MUSB: Workaround for Ethernet data alignment issue

[Sonasath, Moiz]

Nilkesh,

> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> owner@vger.kernel.org] On Behalf Of Pandita, Vikram
> Sent: Monday, December 14, 2009 11:39 AM
> To: Patra, Nilkesh; linux-usb@vger.kernel.org
> Cc: linux-omap@vger.kernel.org; David Brownell; Gadiyar, Anand
> Subject: RE: [RFC] MUSB: Workaround for Ethernet data alignment issue
> 
> 
> Nilkesh
> 
> >-----Original Message-----
> >From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> owner@vger.kernel.org] On Behalf Of Patra,
> >Nilkesh
> >Sent: Sunday, December 13, 2009 10:51 PM
> >To: linux-usb@vger.kernel.org
> >Cc: linux-omap@vger.kernel.org; David Brownell; Gadiyar, Anand
> >Subject: [RFC] MUSB: Workaround for Ethernet data alignment issue
> >
> >On the latest version of MUSB, DMA is not working properly if the Data in
> the packet doesn't start at
> >32bit aligned address. This issue was found while using MUSB as g_ether.
> The basic ping does not
> >work, if the data in the Ethernet  packet does not start at 32bit aligned
> address.
> >
> >To overcome this issue, we found one solution mentioned in the below
> patch. This patch moves data
> >(skb->data) by 2 bytes backwards in ethernet packet, which is nothing but
> skb->head.
> >
> >I want to know, if there are any better approaches to fix this issue.
> 
> I would NAK the first part of patch for following reasons (and suggestions
> are inlined):
> a) this change is specific to OMAP hw only and so should not try to change
> generic files like usb/gadget/u_ether.c
> that get compiled for all the architectures having usb.
> 
> b) there are two issues to be handled separately ( separate patches are
> good for review )
> 	RX side buffer alignments handling
> 	TX side buffer alignments handling
> 
> >
> >Signed-off-by: Nilkesh Patra <nilkesh.patra@ti.com>
> >---
> > drivers/usb/gadget/u_ether.c |   14 ++++++++++----
> > 1 files changed, 10 insertions(+), 4 deletions(-)
> >
> >diff --git a/drivers/usb/gadget/u_ether.c b/drivers/usb/gadget/u_ether.c
> >index 2fc02bd..432b1bd 100644
> >--- a/drivers/usb/gadget/u_ether.c
> >+++ b/drivers/usb/gadget/u_ether.c
> >@@ -249,7 +249,6 @@ 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);
> 
> NAK. Do not remove this line,
> Instead over-ride the NET_IP_ALIGN macro for your architecture.
> 
> One suggestion to do so:
> 
> diff --git a/arch/arm/include/asm/system.h b/arch/arm/include/asm/system.h
> index 058e7e9..b16c5f7 100644
> --- a/arch/arm/include/asm/system.h
> +++ b/arch/arm/include/asm/system.h
> @@ -514,6 +514,10 @@ static inline unsigned long long
> __cmpxchg64_mb(volatile void *ptr,
> 
>  #define arch_align_stack(x) (x)
> 
> +#if defined(CONFIG_ARCH_OMAP3) || defined(CONFIG_ARCH_OMAP4)
> +#define NET_IP_ALIGN 0
> +#endif
> +
>  #endif /* __KERNEL__ */
> 
>  #endif
> 
> This has been verified to work for OMAP3630.
> 
> >
> > 	req->buf = skb->data;
> > 	req->length = size;
> >@@ -500,6 +499,8 @@ static netdev_tx_t eth_start_xmit(struct sk_buff
> *skb,
> > 	unsigned long		flags;
> > 	struct usb_ep		*in;
> > 	u16			cdc_filter;
> >+	int			i = 0;
> >+
> >
> > 	spin_lock_irqsave(&dev->lock, flags);
> > 	if (dev->port_usb) {
> >@@ -573,9 +574,6 @@ static netdev_tx_t eth_start_xmit(struct sk_buff
> *skb,
> >
> > 		length = skb->len;
> > 	}
> >-	req->buf = skb->data;
> >-	req->context = skb;
> >-	req->complete = tx_complete;
> 
> This change would be to handle TX un-alignments.
> So I would prefer to handle it separately ( a separate patch)
> Have you explored the option of over-riding #define NET_SKB_PAD ??
> 
> >
> > 	/* use zlp framing on tx for strict CDC-Ether conformance,
> > 	 * though any robust network rx path ignores extra padding.
> >@@ -585,6 +583,14 @@ static netdev_tx_t eth_start_xmit(struct sk_buff
> *skb,
> > 	if (!dev->zlp && (length % in->maxpacket) == 0)
> > 		length++;
> >
> >+	if ((int)skb->data & 0x2) {

You can use this wrapper instead:
!IS_ALIGNED(skb->data, 4) 


> >+		skb_push(skb, 2);
> 
> Where have you made sure that skb->data allocation has this extra space of
> two bytes?
> 
> >+		for (i = 0; i < length; i++)
> >+			skb->data[i] = skb->data[i+2];
> >+	}
> >+	req->buf = skb->data;
> >+	req->context = skb;
> >+	req->complete = tx_complete;
> > 	req->length = length;
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-
Moiz

RE: [RFC] MUSB: Workaround for Ethernet data alignment issue

[Patra, Nilkesh]

Vikram

> -----Original Message-----
> From: Pandita, Vikram
> Sent: Monday, December 14, 2009 11:09 PM
> To: Patra, Nilkesh; linux-usb@vger.kernel.org
> Cc: linux-omap@vger.kernel.org; David Brownell; Gadiyar, Anand
> Subject: RE: [RFC] MUSB: Workaround for Ethernet data alignment issue
> 
> 
> Nilkesh
> 
> >-----Original Message-----
> >From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> owner@vger.kernel.org] On Behalf Of Patra,
> >Nilkesh
> >Sent: Sunday, December 13, 2009 10:51 PM
> >To: linux-usb@vger.kernel.org
> >Cc: linux-omap@vger.kernel.org; David Brownell; Gadiyar, Anand
> >Subject: [RFC] MUSB: Workaround for Ethernet data alignment issue
> >
> >On the latest version of MUSB, DMA is not working properly if the Data in
> the packet doesn't start at
> >32bit aligned address. This issue was found while using MUSB as g_ether.
> The basic ping does not
> >work, if the data in the Ethernet  packet does not start at 32bit aligned
> address.
> >
> >To overcome this issue, we found one solution mentioned in the below
> patch. This patch moves data
> >(skb->data) by 2 bytes backwards in ethernet packet, which is nothing but
> skb->head.
> >
> >I want to know, if there are any better approaches to fix this issue.
> 
> I would NAK the first part of patch for following reasons (and suggestions
> are inlined):
> a) this change is specific to OMAP hw only and so should not try to change
> generic files like usb/gadget/u_ether.c
> that get compiled for all the architectures having usb.

This may not be specific to OMAP hw, the same issue may be there in other hw also. As this modification takes care of both aligned and unaligned  addresses, so this can be in usb/gadget/u_ether.c . 

Only disadvantage in this logic is, if the hw is capable of handling unaligned address, then there will be unnecessary shifting of data by 2 bytes. To avoid this, this logic has to be in gadget driver. But there will more performance impact for allocating a fresh buffer, doing memcpy and freeing original buffer.

> 
> b) there are two issues to be handled separately ( separate patches are
> good for review )
> 	RX side buffer alignments handling
> 	TX side buffer alignments handling
> 
> >
> >Signed-off-by: Nilkesh Patra <nilkesh.patra@ti.com>
> >---
> > drivers/usb/gadget/u_ether.c |   14 ++++++++++----
> > 1 files changed, 10 insertions(+), 4 deletions(-)
> >
> >diff --git a/drivers/usb/gadget/u_ether.c b/drivers/usb/gadget/u_ether.c
> >index 2fc02bd..432b1bd 100644
> >--- a/drivers/usb/gadget/u_ether.c
> >+++ b/drivers/usb/gadget/u_ether.c
> >@@ -249,7 +249,6 @@ 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);
> 
> NAK. Do not remove this line,
> Instead over-ride the NET_IP_ALIGN macro for your architecture.
> 
> One suggestion to do so:
> 
> diff --git a/arch/arm/include/asm/system.h b/arch/arm/include/asm/system.h
> index 058e7e9..b16c5f7 100644
> --- a/arch/arm/include/asm/system.h
> +++ b/arch/arm/include/asm/system.h
> @@ -514,6 +514,10 @@ static inline unsigned long long
> __cmpxchg64_mb(volatile void *ptr,
> 
>  #define arch_align_stack(x) (x)
> 
> +#if defined(CONFIG_ARCH_OMAP3) || defined(CONFIG_ARCH_OMAP4)
> +#define NET_IP_ALIGN 0
> +#endif
> +
>  #endif /* __KERNEL__ */
> 
>  #endif
> 
> This has been verified to work for OMAP3630.
> 

By assigning 0 to NET_IP_ALIGN, basic Ethernet is not working. Because the same macro is used in other places also.

> >
> > 	req->buf = skb->data;
> > 	req->length = size;
> >@@ -500,6 +499,8 @@ static netdev_tx_t eth_start_xmit(struct sk_buff
> *skb,
> > 	unsigned long		flags;
> > 	struct usb_ep		*in;
> > 	u16			cdc_filter;
> >+	int			i = 0;
> >+
> >
> > 	spin_lock_irqsave(&dev->lock, flags);
> > 	if (dev->port_usb) {
> >@@ -573,9 +574,6 @@ static netdev_tx_t eth_start_xmit(struct sk_buff
> *skb,
> >
> > 		length = skb->len;
> > 	}
> >-	req->buf = skb->data;
> >-	req->context = skb;
> >-	req->complete = tx_complete;
> 
> This change would be to handle TX un-alignments.
> So I would prefer to handle it separately ( a separate patch)
> Have you explored the option of over-riding #define NET_SKB_PAD ??

By changing NET_SKB_PAD, skb->data will get aligned. But there may be impact in other places. I tried with NET_SKB_PAD 34, but it didn't work. 
 
> 
> >
> > 	/* use zlp framing on tx for strict CDC-Ether conformance,
> > 	 * though any robust network rx path ignores extra padding.
> >@@ -585,6 +583,14 @@ static netdev_tx_t eth_start_xmit(struct sk_buff
> *skb,
> > 	if (!dev->zlp && (length % in->maxpacket) == 0)
> > 		length++;
> >
> >+	if ((int)skb->data & 0x2) {
> >+		skb_push(skb, 2);
> 
> Where have you made sure that skb->data allocation has this extra space of
> two bytes?

In SKB, there is a head ptr (4 bytes) just before actual data starts. This space is not used at all in this scenario. So, skb->data can be shifted 2 bytes backward.
 
> 
> >+		for (i = 0; i < length; i++)
> >+			skb->data[i] = skb->data[i+2];
> >+	}
> >+	req->buf = skb->data;
> >+	req->context = skb;
> >+	req->complete = tx_complete;
> > 	req->length = length;
> >

Re: [RFC] MUSB: Workaround for Ethernet data alignment issue

[Matthieu CASTET]

Hi,

Patra, Nilkesh a écrit :
> On the latest version of MUSB, DMA is not working properly if the Data in the packet doesn't start at 32bit aligned address. This issue was found while using MUSB as g_ether. The basic ping does not work, if the data in the Ethernet  packet does not start at 32bit aligned address.
> 
> To overcome this issue, we found one solution mentioned in the below patch. This patch moves data (skb->data) by 2 bytes backwards in ethernet packet, which is nothing but skb->head.
> 
> I want to know, if there are any better approaches to fix this issue.
>
What dma_mask are you using ?

From what I understand, dma_map_single will use it to know if the dma
buffer need to be aligned [1].
So you need to set the last 2 bits to 0 and it should do everything for you.


Matthieu

[1]
       /*
         * Figure out if we need to bounce from the DMA mask.
         */
        needs_bounce = (dma_addr | (dma_addr + size - 1)) & ~mask;
    }

    if (device_info && (needs_bounce || dma_needs_bounce(dev, dma_addr,
size))) {
        struct safe_buffer *buf;

        buf = alloc_safe_buffer(device_info, ptr, size, dir);
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] MUSB: Workaround for Ethernet data alignment issue

[Matthieu CASTET]

Matthieu CASTET a écrit :
> Hi,
> 
> Patra, Nilkesh a écrit :
>> On the latest version of MUSB, DMA is not working properly if the Data in the packet doesn't start at 32bit aligned address. This issue was found while using MUSB as g_ether. The basic ping does not work, if the data in the Ethernet  packet does not start at 32bit aligned address.
>>
>> To overcome this issue, we found one solution mentioned in the below patch. This patch moves data (skb->data) by 2 bytes backwards in ethernet packet, which is nothing but skb->head.
>>
>> I want to know, if there are any better approaches to fix this issue.
>>
> What dma_mask are you using ?
> 
>>From what I understand, dma_map_single will use it to know if the dma
> buffer need to be aligned [1].
> So you need to set the last 2 bits to 0 and it should do everything for you.
> 
Forget what I said it is only true in CONFIG_DMABOUNCE case...
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html