Re: [PATCH v5 07/14] net: pch_gbe: Fix handling of TX padding

[David Miller]

From: Paul Burton 
Date: Mon, 19 Feb 2018 08:42:23 -0800

> So whilst I totally agree that copying around the whole frame is awful,
> it's a separate problem to the length used for DMA mapping being
> incorrect which is what this patch addresses & I'd rather not start
> adding more & more fixes or cleanups into this initial series before the
> driver is even functional on my hardware.

Agreed.

Re: [PATCH v5 07/14] net: pch_gbe: Fix handling of TX padding

[Paul Burton]

Hi David,

On Mon, Feb 19, 2018 at 02:01:25PM +, David Laight wrote:
> From: Paul Burton
> > Sent: 17 February 2018 20:11
> > 
> > The ethernet controller found in the Intel EG20T Platform Controller
> > Hub requires that we place 2 bytes of padding between the ethernet
> > header & the packet payload. Our pch_gbe driver handles this by copying
> > packets to be transmitted to a temporary struct skb with the padding
> > bytes inserted
> ...
> 
> Uggg WFT is the driver doing that for?
> 
> I'd guess that the two byte pad is there so that a 4 byte aligned
> frame is still 4 byte aligned when the 14 byte ethernet header is added.
> So instead of copying the entire frame the MAC header should be built
> (or rebuilt?) two bytes further from the actual data.

I agree - the pch_gbe driver is pretty bad and does a lot of things
wrong. Frankly I'm amazed it's in tree, but it is & one patch series
isn't going to fix all of its shortcomings.

So whilst I totally agree that copying around the whole frame is awful,
it's a separate problem to the length used for DMA mapping being
incorrect which is what this patch addresses & I'd rather not start
adding more & more fixes or cleanups into this initial series before the
driver is even functional on my hardware.

Thanks,
Paul

RE: [PATCH v5 07/14] net: pch_gbe: Fix handling of TX padding

[David Laight]

From: Paul Burton
> Sent: 17 February 2018 20:11
> 
> The ethernet controller found in the Intel EG20T Platform Controller
> Hub requires that we place 2 bytes of padding between the ethernet
> header & the packet payload. Our pch_gbe driver handles this by copying
> packets to be transmitted to a temporary struct skb with the padding
> bytes inserted
...

Uggg WFT is the driver doing that for?

I'd guess that the two byte pad is there so that a 4 byte aligned
frame is still 4 byte aligned when the 14 byte ethernet header is added.
So instead of copying the entire frame the MAC header should be built
(or rebuilt?) two bytes further from the actual data.

David

[PATCH v5 07/14] net: pch_gbe: Fix handling of TX padding

[Paul Burton]

The ethernet controller found in the Intel EG20T Platform Controller
Hub requires that we place 2 bytes of padding between the ethernet
header & the packet payload. Our pch_gbe driver handles this by copying
packets to be transmitted to a temporary struct skb with the padding
bytes inserted, however it sets the length of this temporary skb to
equal that of the original, without the 2 padding bytes, and then uses
this length as that of the memory to map for DMA.

This is problematic on systems that don't have cache-coherent DMA, since
if the length of the original buffer is either a multiple of the
system's cache line size or one less than such a multiple then the size
we provide to dma_map_single() will not cover the last byte or two of
the data when rounded up to a cache line boundary. This may result in us
transmitting corrupt data in the last one or two bytes of the packet,
depending upon its length.

Fix this by setting the length of tmp_skb to include the 2 padding
bytes, which is actually the length of the data it holds. This is then
assigned to buffer_info->length & provided to dma_map_single() which
will operate on all of the data as desired. The EG20T datasheet
specifies that the padding bytes should not be included in the length
stored in the TX descriptor, so we switch that to use the length of the
original skb.

Whilst modifying this code we switch to using PCH_GBE_DMA_PADDING rather
than the magic number 2 to specify the size of the padding, making it
clearer what the code is doing, and fix a typo in the comment indicating
that padding is inserted.

Signed-off-by: Paul Burton 
Cc: David S. Miller 
Cc: linux-m...@linux-mips.org
Cc: netdev@vger.kernel.org
---

Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None

 drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c | 15 +++
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c 
b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
index 77f7fbd98e8f..60e91c0fc98b 100644
--- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
+++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
@@ -1223,13 +1223,12 @@ static void pch_gbe_tx_queue(struct pch_gbe_adapter 
*adapter,
buffer_info = _ring->buffer_info[ring_num];
tmp_skb = buffer_info->skb;
 
-   /* [Header:14][payload] ---> [Header:14][paddong:2][payload]*/
+   /* [Header:14][payload] ---> [Header:14][padding:2][payload] */
memcpy(tmp_skb->data, skb->data, ETH_HLEN);
-   tmp_skb->data[ETH_HLEN] = 0x00;
-   tmp_skb->data[ETH_HLEN + 1] = 0x00;
-   tmp_skb->len = skb->len;
-   memcpy(_skb->data[ETH_HLEN + 2], >data[ETH_HLEN],
-  (skb->len - ETH_HLEN));
+   memset(_skb->data[ETH_HLEN], 0, PCH_GBE_DMA_PADDING);
+   memcpy(_skb->data[ETH_HLEN + PCH_GBE_DMA_PADDING],
+  >data[ETH_HLEN], skb->len - ETH_HLEN);
+   tmp_skb->len = skb->len + PCH_GBE_DMA_PADDING;
/*-- Set Buffer information --*/
buffer_info->length = tmp_skb->len;
buffer_info->dma = dma_map_single(>pdev->dev, tmp_skb->data,
@@ -1248,8 +1247,8 @@ static void pch_gbe_tx_queue(struct pch_gbe_adapter 
*adapter,
/*-- Set Tx descriptor --*/
tx_desc = PCH_GBE_TX_DESC(*tx_ring, ring_num);
tx_desc->buffer_addr = (buffer_info->dma);
-   tx_desc->length = (tmp_skb->len);
-   tx_desc->tx_words_eob = ((tmp_skb->len + 3));
+   tx_desc->length = (skb->len);
+   tx_desc->tx_words_eob = ((skb->len + 3));
tx_desc->tx_frame_ctrl = (frame_ctrl);
tx_desc->gbec_status = (DSC_INIT16);
 
-- 
2.16.1