* [PATCH] pch_gbe: Fix the issue that the receiving data is not normal.
@ 2011-01-24 4:43 Toshiharu Okada
2011-01-25 21:32 ` David Miller
0 siblings, 1 reply; 7+ messages in thread
From: Toshiharu Okada @ 2011-01-24 4:43 UTC (permalink / raw)
To: ML netdev; +Cc: LKML, Wang, Qi, Wang, Yong Y, Andrew, Intel OTC, Ewe, Kok Howg
This PCH_GBE driver had an issue that the receiving data is not normal.
This driver had not removed correctly the padding data
which the DMA include in receiving data.
This patch fixed this issue.
Signed-off-by: Toshiharu Okada <toshiharu-linux@dsn.okisemi.com>
---
drivers/net/pch_gbe/pch_gbe_main.c | 21 ++++++++++++---------
1 files changed, 12 insertions(+), 9 deletions(-)
diff --git a/drivers/net/pch_gbe/pch_gbe_main.c b/drivers/net/pch_gbe/pch_gbe_main.c
index 03a1d28..1bf2626 100644
--- a/drivers/net/pch_gbe/pch_gbe_main.c
+++ b/drivers/net/pch_gbe/pch_gbe_main.c
@@ -29,6 +29,7 @@ const char pch_driver_version[] = DRV_VERSION;
#define PCH_GBE_SHORT_PKT 64
#define DSC_INIT16 0xC000
#define PCH_GBE_DMA_ALIGN 0
+#define PCH_GBE_DMA_PADDING 2
#define PCH_GBE_WATCHDOG_PERIOD (1 * HZ) /* watchdog time */
#define PCH_GBE_COPYBREAK_DEFAULT 256
#define PCH_GBE_PCI_BAR 1
@@ -1422,8 +1423,8 @@ pch_gbe_clean_rx(struct pch_gbe_adapter *adapter,
pr_err("Receive CRC Error\n");
} else {
/* get receive length */
- /* length convert[-3], padding[-2] */
- length = (rx_desc->rx_words_eob) - 3 - 2;
+ /* length convert[-3] */
+ length = (rx_desc->rx_words_eob) - 3;
/* Decide the data conversion method */
if (!adapter->rx_csum) {
@@ -1443,12 +1444,11 @@ pch_gbe_clean_rx(struct pch_gbe_adapter *adapter,
if (skb_copy_flag) { /* recycle skb */
struct sk_buff *new_skb;
new_skb =
- netdev_alloc_skb(netdev,
- length + NET_IP_ALIGN);
+ netdev_alloc_skb(netdev, length);
if (new_skb) {
if (!skb_padding_flag) {
skb_reserve(new_skb,
- NET_IP_ALIGN);
+ PCH_GBE_DMA_PADDING);
}
memcpy(new_skb->data, skb->data,
length);
@@ -1465,12 +1465,15 @@ pch_gbe_clean_rx(struct pch_gbe_adapter *adapter,
}
if (skb_padding_flag) {
memcpy(&tmp_packet[0], &skb->data[0], ETH_HLEN);
- memcpy(&skb->data[NET_IP_ALIGN], &tmp_packet[0],
- ETH_HLEN);
- skb_reserve(skb, NET_IP_ALIGN);
-
+ memcpy(&skb->data[PCH_GBE_DMA_PADDING],
+ &tmp_packet[0], ETH_HLEN);
+ skb_reserve(skb, PCH_GBE_DMA_PADDING);
+ /* The length includes padding length */
+ length = length - PCH_GBE_DMA_PADDING;
}
+ /* The length includes FCS length */
+ length = length - ETH_FCS_LEN;
/* update status of driver */
adapter->stats.rx_bytes += length;
adapter->stats.rx_packets++;
--
1.6.2.5
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] pch_gbe: Fix the issue that the receiving data is not normal.
2011-01-24 4:43 [PATCH] pch_gbe: Fix the issue that the receiving data is not normal Toshiharu Okada
@ 2011-01-25 21:32 ` David Miller
2011-01-27 8:45 ` Toshiharu Okada
0 siblings, 1 reply; 7+ messages in thread
From: David Miller @ 2011-01-25 21:32 UTC (permalink / raw)
To: toshiharu-linux
Cc: netdev, linux-kernel, qi.wang, yong.y.wang,
andrew.chih.howe.khor, joel.clark, kok.howg.ewe
From: Toshiharu Okada <toshiharu-linux@dsn.okisemi.com>
Date: Mon, 24 Jan 2011 13:43:31 +0900
> This PCH_GBE driver had an issue that the receiving data is not normal.
> This driver had not removed correctly the padding data
> which the DMA include in receiving data.
>
> This patch fixed this issue.
>
> Signed-off-by: Toshiharu Okada <toshiharu-linux@dsn.okisemi.com>
There are bugs in these changes:
> if (skb_copy_flag) { /* recycle skb */
> struct sk_buff *new_skb;
> new_skb =
> - netdev_alloc_skb(netdev,
> - length + NET_IP_ALIGN);
> + netdev_alloc_skb(netdev, length);
> if (new_skb) {
> if (!skb_padding_flag) {
> skb_reserve(new_skb,
> - NET_IP_ALIGN);
> + PCH_GBE_DMA_PADDING);
> }
> memcpy(new_skb->data, skb->data,
> length);
If "!skb_padding_flag" then you will write past the end of the SKB
data in that memcpy.
You cannot allocate only "length" then proceed to reserve PCH_GBE_DMA_PADDING
and then add "length" worth of data on top of that. In such a cause you
must allocate at least "length + PCH_GBE_DMA_PADDING".
Furthermore you _MUST_ respect NET_IP_ALIGN. Some platforms set this value
to "0", because otherwise performance suffers greatly.
There are two seperate issues, removing the padding bytes provided by
the device, and aligning the IP headers as wanted by the cpu
architecutre. Therefore they should be handled seperately, and we
therefore should still see references to NET_IP_ALIGN in your patch.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] pch_gbe: Fix the issue that the receiving data is not normal.
2011-01-25 21:32 ` David Miller
@ 2011-01-27 8:45 ` Toshiharu Okada
0 siblings, 0 replies; 7+ messages in thread
From: Toshiharu Okada @ 2011-01-27 8:45 UTC (permalink / raw)
To: David Miller
Cc: netdev, linux-kernel, qi.wang, yong.y.wang,
andrew.chih.howe.khor, joel.clark, kok.howg.ewe
Hi David
Thank you for your comment.
I will confirm them and will submit the patch modified.
Best regards
Toshiharu Okada(OKI semiconductor)
----- Original Message -----
From: "David Miller" <davem@davemloft.net>
To: <toshiharu-linux@dsn.okisemi.com>
Cc: <netdev@vger.kernel.org>; <linux-kernel@vger.kernel.org>;
<qi.wang@intel.com>; <yong.y.wang@intel.com>;
<andrew.chih.howe.khor@intel.com>; <joel.clark@intel.com>;
<kok.howg.ewe@intel.com>
Sent: Wednesday, January 26, 2011 6:32 AM
Subject: Re: [PATCH] pch_gbe: Fix the issue that the receiving data is not
normal.
From: Toshiharu Okada <toshiharu-linux@dsn.okisemi.com>
Date: Mon, 24 Jan 2011 13:43:31 +0900
> This PCH_GBE driver had an issue that the receiving data is not normal.
> This driver had not removed correctly the padding data
> which the DMA include in receiving data.
>
> This patch fixed this issue.
>
> Signed-off-by: Toshiharu Okada <toshiharu-linux@dsn.okisemi.com>
There are bugs in these changes:
> if (skb_copy_flag) { /* recycle skb */
> struct sk_buff *new_skb;
> new_skb =
> - netdev_alloc_skb(netdev,
> - length + NET_IP_ALIGN);
> + netdev_alloc_skb(netdev, length);
> if (new_skb) {
> if (!skb_padding_flag) {
> skb_reserve(new_skb,
> - NET_IP_ALIGN);
> + PCH_GBE_DMA_PADDING);
> }
> memcpy(new_skb->data, skb->data,
> length);
If "!skb_padding_flag" then you will write past the end of the SKB
data in that memcpy.
You cannot allocate only "length" then proceed to reserve
PCH_GBE_DMA_PADDING
and then add "length" worth of data on top of that. In such a cause you
must allocate at least "length + PCH_GBE_DMA_PADDING".
Furthermore you _MUST_ respect NET_IP_ALIGN. Some platforms set this value
to "0", because otherwise performance suffers greatly.
There are two seperate issues, removing the padding bytes provided by
the device, and aligning the IP headers as wanted by the cpu
architecutre. Therefore they should be handled seperately, and we
therefore should still see references to NET_IP_ALIGN in your patch.
--
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] pch_gbe: Fix the issue that the receiving data is not normal.
@ 2011-01-27 8:45 ` Toshiharu Okada
0 siblings, 0 replies; 7+ messages in thread
From: Toshiharu Okada @ 2011-01-27 8:45 UTC (permalink / raw)
To: David Miller
Cc: netdev, linux-kernel, qi.wang, yong.y.wang,
andrew.chih.howe.khor, joel.clark, kok.howg.ewe
Hi David
Thank you for your comment.
I will confirm them and will submit the patch modified.
Best regards
Toshiharu Okada(OKI semiconductor)
----- Original Message -----
From: "David Miller" <davem@davemloft.net>
To: <toshiharu-linux@dsn.okisemi.com>
Cc: <netdev@vger.kernel.org>; <linux-kernel@vger.kernel.org>;
<qi.wang@intel.com>; <yong.y.wang@intel.com>;
<andrew.chih.howe.khor@intel.com>; <joel.clark@intel.com>;
<kok.howg.ewe@intel.com>
Sent: Wednesday, January 26, 2011 6:32 AM
Subject: Re: [PATCH] pch_gbe: Fix the issue that the receiving data is not
normal.
From: Toshiharu Okada <toshiharu-linux@dsn.okisemi.com>
Date: Mon, 24 Jan 2011 13:43:31 +0900
> This PCH_GBE driver had an issue that the receiving data is not normal.
> This driver had not removed correctly the padding data
> which the DMA include in receiving data.
>
> This patch fixed this issue.
>
> Signed-off-by: Toshiharu Okada <toshiharu-linux@dsn.okisemi.com>
There are bugs in these changes:
> if (skb_copy_flag) { /* recycle skb */
> struct sk_buff *new_skb;
> new_skb =
> - netdev_alloc_skb(netdev,
> - length + NET_IP_ALIGN);
> + netdev_alloc_skb(netdev, length);
> if (new_skb) {
> if (!skb_padding_flag) {
> skb_reserve(new_skb,
> - NET_IP_ALIGN);
> + PCH_GBE_DMA_PADDING);
> }
> memcpy(new_skb->data, skb->data,
> length);
If "!skb_padding_flag" then you will write past the end of the SKB
data in that memcpy.
You cannot allocate only "length" then proceed to reserve
PCH_GBE_DMA_PADDING
and then add "length" worth of data on top of that. In such a cause you
must allocate at least "length + PCH_GBE_DMA_PADDING".
Furthermore you _MUST_ respect NET_IP_ALIGN. Some platforms set this value
to "0", because otherwise performance suffers greatly.
There are two seperate issues, removing the padding bytes provided by
the device, and aligning the IP headers as wanted by the cpu
architecutre. Therefore they should be handled seperately, and we
therefore should still see references to NET_IP_ALIGN in your patch.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] pch_gbe: Fix the issue that the receiving data is not normal.
2011-02-09 0:35 ` David Miller
@ 2011-02-09 4:58 ` Toshiharu Okada
0 siblings, 0 replies; 7+ messages in thread
From: Toshiharu Okada @ 2011-02-09 4:58 UTC (permalink / raw)
To: David Miller
Cc: netdev, linux-kernel, qi.wang, yong.y.wang,
andrew.chih.howe.khor, joel.clark, kok.howg.ewe, tomoya-linux
From: David Miller <davem@davemloft.net>
Date: Tue, 08 Feb 2011 16:35:22 -0800 (PST)
>> Hi Devid
>>
>> I resubmit this patch modified.
>> Please check them.
>
>Your memcpy+memcpy sequences are equivalent to memmove(), please
>use that instead.
>
I use memmove().
>I have to say this function is insanely complicated. There seems
>to be 16 different ways RX packets are processed. I can't believe
>that is needs to be like this.
If processing is arranged, There are 4 different ways RX packets are
processed.
[Case1]
"rx_csum is enable " and
"NET_IP_ALIGN == 0"
DMA buffer is used as SKB as it is.
[Case2]
"rx_csum is enable " and
"NET_IP_ALIGN != 0"
Because alignment differs, the new_skb is newly allocated, and data is
copied to new_skb.
[Case3]
"rx_csum is disable " and
"length < copybreak" or "NET_IP_ALIGN != PCH_GBE_DMA_PADDING"
Because alignment differs, the new_skb is newly allocated, and data is
copied to new_skb.
Padding data is deleted at the time of a copy.
[Case4]
"rx_csum is disable " and
"length >= copybreak" and "NET_IP_ALIGN == PCH_GBE_DMA_PADDING"
Padding data is deleted by moving header data.
I rewrite without using skb_copy_flag and skb_padding simply.
Best regards
Toshiharu Okada(OKI semiconductor)
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] pch_gbe: Fix the issue that the receiving data is not normal.
2011-02-08 8:24 Toshiharu Okada
@ 2011-02-09 0:35 ` David Miller
2011-02-09 4:58 ` Toshiharu Okada
0 siblings, 1 reply; 7+ messages in thread
From: David Miller @ 2011-02-09 0:35 UTC (permalink / raw)
To: toshiharu-linux
Cc: netdev, linux-kernel, qi.wang, yong.y.wang,
andrew.chih.howe.khor, joel.clark, kok.howg.ewe, tomoya-linux
From: Toshiharu Okada <toshiharu-linux@dsn.okisemi.com>
Date: Tue, 08 Feb 2011 17:24:10 +0900
> Hi Devid
>
> I resubmit this patch modified.
> Please check them.
Your memcpy+memcpy sequences are equivalent to memmove(), please
use that instead.
I have to say this function is insanely complicated. There seems
to be 16 different ways RX packets are processed. I can't believe
that is needs to be like this.
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] pch_gbe: Fix the issue that the receiving data is not normal.
@ 2011-02-08 8:24 Toshiharu Okada
2011-02-09 0:35 ` David Miller
0 siblings, 1 reply; 7+ messages in thread
From: Toshiharu Okada @ 2011-02-08 8:24 UTC (permalink / raw)
To: ML netdev, David S. Miller
Cc: LKML, Wang, Qi, Wang, Yong Y, Andrew, Intel OTC, Ewe, Kok Howg,
Tomoya Morinaga
Hi Devid
I resubmit this patch modified.
Please check them.
Best regards
Toshiharu Okada(OKI semiconductor)
---
This PCH_GBE driver had an issue that the receiving data is not normal.
This driver had not removed correctly the padding data
which the DMA include in receiving data.
This patch fixed this issue.
Signed-off-by: Toshiharu Okada <toshiharu-linux@dsn.okisemi.com>
---
drivers/net/pch_gbe/pch_gbe_main.c | 77 +++++++++++++++++++++--------------
1 files changed, 46 insertions(+), 31 deletions(-)
diff --git a/drivers/net/pch_gbe/pch_gbe_main.c b/drivers/net/pch_gbe/pch_gbe_main.c
index 03a1d28..3248313 100644
--- a/drivers/net/pch_gbe/pch_gbe_main.c
+++ b/drivers/net/pch_gbe/pch_gbe_main.c
@@ -29,6 +29,7 @@ const char pch_driver_version[] = DRV_VERSION;
#define PCH_GBE_SHORT_PKT 64
#define DSC_INIT16 0xC000
#define PCH_GBE_DMA_ALIGN 0
+#define PCH_GBE_DMA_PADDING 2
#define PCH_GBE_WATCHDOG_PERIOD (1 * HZ) /* watchdog time */
#define PCH_GBE_COPYBREAK_DEFAULT 256
#define PCH_GBE_PCI_BAR 1
@@ -1373,7 +1374,7 @@ pch_gbe_clean_rx(struct pch_gbe_adapter *adapter,
unsigned int i;
unsigned int cleaned_count = 0;
bool cleaned = false;
- struct sk_buff *skb;
+ struct sk_buff *skb, *new_skb;
u8 dma_status;
u16 gbec_status;
u32 tcp_ip_status;
@@ -1422,55 +1423,69 @@ pch_gbe_clean_rx(struct pch_gbe_adapter *adapter,
pr_err("Receive CRC Error\n");
} else {
/* get receive length */
- /* length convert[-3], padding[-2] */
- length = (rx_desc->rx_words_eob) - 3 - 2;
+ /* length convert[-3] */
+ length = (rx_desc->rx_words_eob) - 3;
/* Decide the data conversion method */
if (!adapter->rx_csum) {
/* [Header:14][payload] */
skb_padding_flag = 0;
- skb_copy_flag = 1;
+ if (NET_IP_ALIGN)
+ /* [NET_IP_ALIGN][Header:14][payload] */
+ skb_copy_flag = 1;
+ else
+ skb_copy_flag = 0;
} else {
/* [Header:14][padding:2][payload] */
skb_padding_flag = 1;
+ /* The length includes padding length */
+ length = length - PCH_GBE_DMA_PADDING;
if (length < copybreak)
skb_copy_flag = 1;
- else
- skb_copy_flag = 0;
+ else {
+ /* [Header:14][padding:2][payload]
+ * Chenge to the following
+ * [NET_IP_ALIGN][Header:14][payload] */
+ if (NET_IP_ALIGN == PCH_GBE_DMA_PADDING)
+ skb_copy_flag = 0;
+ else
+ skb_copy_flag = 1;
+ }
}
-
/* Data conversion */
- if (skb_copy_flag) { /* recycle skb */
- struct sk_buff *new_skb;
- new_skb =
- netdev_alloc_skb(netdev,
- length + NET_IP_ALIGN);
- if (new_skb) {
- if (!skb_padding_flag) {
- skb_reserve(new_skb,
- NET_IP_ALIGN);
- }
- memcpy(new_skb->data, skb->data,
- length);
- /* save the skb
- * in buffer_info as good */
- skb = new_skb;
- } else if (!skb_padding_flag) {
+ if (skb_copy_flag) {
+ new_skb = netdev_alloc_skb(netdev,
+ length + NET_IP_ALIGN);
+ if (!new_skb) {
/* dorrop error */
pr_err("New skb allocation Error\n");
goto dorrop;
}
+ skb_reserve(new_skb, NET_IP_ALIGN);
+ if (skb_padding_flag) {
+ memcpy(new_skb->data, skb->data,
+ ETH_HLEN);
+ memcpy(&new_skb->data[ETH_HLEN],
+ &skb->data[ETH_HLEN +
+ PCH_GBE_DMA_PADDING],
+ length - ETH_HLEN);
+ } else {
+ memcpy(new_skb->data, skb->data,
+ length);
+ }
+ skb = new_skb;
} else {
+ if (skb_padding_flag) {
+ memcpy(&tmp_packet[0], &skb->data[0],
+ ETH_HLEN);
+ memcpy(&skb->data[PCH_GBE_DMA_PADDING],
+ &tmp_packet[0], ETH_HLEN);
+ skb_reserve(skb, PCH_GBE_DMA_PADDING);
+ }
buffer_info->skb = NULL;
}
- if (skb_padding_flag) {
- memcpy(&tmp_packet[0], &skb->data[0], ETH_HLEN);
- memcpy(&skb->data[NET_IP_ALIGN], &tmp_packet[0],
- ETH_HLEN);
- skb_reserve(skb, NET_IP_ALIGN);
-
- }
-
+ /* The length includes FCS length */
+ length = length - ETH_FCS_LEN;
/* update status of driver */
adapter->stats.rx_bytes += length;
adapter->stats.rx_packets++;
--
1.6.2.5
^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2011-02-09 5:00 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-01-24 4:43 [PATCH] pch_gbe: Fix the issue that the receiving data is not normal Toshiharu Okada
2011-01-25 21:32 ` David Miller
2011-01-27 8:45 ` Toshiharu Okada
2011-01-27 8:45 ` Toshiharu Okada
2011-02-08 8:24 Toshiharu Okada
2011-02-09 0:35 ` David Miller
2011-02-09 4:58 ` Toshiharu Okada
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.