Alignment issues with freescale FEC driver

Alignment issues with freescale FEC driver

[Eric Nelson]

Hello all,

We're seeing alignment issues from the ethernet stack on an i.MX6UL board:

root@mx6ul:~# cat /proc/cpu/alignment
User: 0
System: 470010 (inet_gro_receive+0x104/0x278)

This seems to be related to the ip header alignment, and there
was much discussion in mailing list threads [1] and [2].

In particular, Russell referred to a patch here, but I haven't been
able to find it:
https://lists.linaro.org/pipermail/linaro-toolchain/2012-October/002844.html

Eric Dumazet also suggested a path toward fixing it, but I don't quite
understand the suggestion:
http://www.spinics.net/lists/netdev/msg213166.htm

The immediate problem is addressed by just reading the id and frag_offs
fields in the iphdr structure as shown in this patch:

commit 98810abc911b1286a7e4a2ebdfbad66f12fae19d
Author: Eric Nelson <eric@nelint.com>
Date: Fri Sep 23 08:26:03 2016 -0700

net: ipv4: af_inet: don't read multiple 16-bit iphdr fields as a 32-bit
value

Change-Id: Idc7122c22c13ca078be31907d30ab1c3148ba807
Signed-off-by: Eric Nelson <eric@nelint.com>

diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
index 0cc98b1..c17ef6e 100644
--- a/net/ipv4/af_inet.c
+++ b/net/ipv4/af_inet.c
@@ -1301,6 +1301,7 @@ static struct sk_buff **inet_gro_receive(struct
sk_buff **head,
unsigned int hlen;
unsigned int off;
unsigned int id;
+ unsigned int frag;
int flush = 1;
int proto;

@@ -1326,9 +1327,9 @@ static struct sk_buff **inet_gro_receive(struct
sk_buff **head,
if (unlikely(ip_fast_csum((u8 *)iph, 5)))
goto out_unlock;

- id = ntohl(*(__be32 *)&iph->id);
- flush = (u16)((ntohl(*(__be32 *)iph) ^ skb_gro_len(skb)) | (id & ~IP_DF));
- id >>= 16;
+ id = ntohs(*(__be16 *)&iph->id);
+ frag = ntohs(*(__be16 *)&iph->frag_off);
+ flush = (u16)((ntohl(*(__be32 *)iph) ^ skb_gro_len(skb)) | (frag &
~IP_DF));

for (p = *head; p; p = p->next) {
struct iphdr *iph2;


The reading of both fields in one "ntohl" seems obfuscated at best and
certainly worthy of a comment about the optimization but I understand
from other notes that the fundamental problem is that the IP header should
be aligned on a 4-byte boundary and that's not possible without a memcpy.

I'd like to hear suggestions about how we can address this.

Regards,


Eric

[1] - http://www.spinics.net/lists/netdev/msg213114.html
[2] -
https://lists.linaro.org/pipermail/linaro-toolchain/2012-October/002828.html

Alignment issues with freescale FEC driver

[Eric Nelson]

Hello all,

We're seeing alignment issues from the ethernet stack on an i.MX6UL board:

root at mx6ul:~# cat /proc/cpu/alignment
User: 0
System: 470010 (inet_gro_receive+0x104/0x278)

This seems to be related to the ip header alignment, and there
was much discussion in mailing list threads [1] and [2].

In particular, Russell referred to a patch here, but I haven't been
able to find it:
https://lists.linaro.org/pipermail/linaro-toolchain/2012-October/002844.html

Eric Dumazet also suggested a path toward fixing it, but I don't quite
understand the suggestion:
http://www.spinics.net/lists/netdev/msg213166.htm

The immediate problem is addressed by just reading the id and frag_offs
fields in the iphdr structure as shown in this patch:

commit 98810abc911b1286a7e4a2ebdfbad66f12fae19d
Author: Eric Nelson <eric@nelint.com>
Date: Fri Sep 23 08:26:03 2016 -0700

net: ipv4: af_inet: don't read multiple 16-bit iphdr fields as a 32-bit
value

Change-Id: Idc7122c22c13ca078be31907d30ab1c3148ba807
Signed-off-by: Eric Nelson <eric@nelint.com>

diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
index 0cc98b1..c17ef6e 100644
--- a/net/ipv4/af_inet.c
+++ b/net/ipv4/af_inet.c
@@ -1301,6 +1301,7 @@ static struct sk_buff **inet_gro_receive(struct
sk_buff **head,
unsigned int hlen;
unsigned int off;
unsigned int id;
+ unsigned int frag;
int flush = 1;
int proto;

@@ -1326,9 +1327,9 @@ static struct sk_buff **inet_gro_receive(struct
sk_buff **head,
if (unlikely(ip_fast_csum((u8 *)iph, 5)))
goto out_unlock;

- id = ntohl(*(__be32 *)&iph->id);
- flush = (u16)((ntohl(*(__be32 *)iph) ^ skb_gro_len(skb)) | (id & ~IP_DF));
- id >>= 16;
+ id = ntohs(*(__be16 *)&iph->id);
+ frag = ntohs(*(__be16 *)&iph->frag_off);
+ flush = (u16)((ntohl(*(__be32 *)iph) ^ skb_gro_len(skb)) | (frag &
~IP_DF));

for (p = *head; p; p = p->next) {
struct iphdr *iph2;


The reading of both fields in one "ntohl" seems obfuscated at best and
certainly worthy of a comment about the optimization but I understand
from other notes that the fundamental problem is that the IP header should
be aligned on a 4-byte boundary and that's not possible without a memcpy.

I'd like to hear suggestions about how we can address this.

Regards,


Eric

[1] - http://www.spinics.net/lists/netdev/msg213114.html
[2] -
https://lists.linaro.org/pipermail/linaro-toolchain/2012-October/002828.html

Re: Alignment issues with freescale FEC driver

[Eric Dumazet]

On Fri, Sep 23, 2016 at 9:43 AM, Eric Nelson <eric@nelint.com> wrote:
>
> Hello all,
>
> We're seeing alignment issues from the ethernet stack on an i.MX6UL board:
>
> root@mx6ul:~# cat /proc/cpu/alignment
> User: 0
> System: 470010 (inet_gro_receive+0x104/0x278)
>
> This seems to be related to the ip header alignment, and there
> was much discussion in mailing list threads [1] and [2].
>
> In particular, Russell referred to a patch here, but I haven't been
> able to find it:
> https://lists.linaro.org/pipermail/linaro-toolchain/2012-October/002844.html
>
> Eric Dumazet also suggested a path toward fixing it, but I don't quite
> understand the suggestion:
> http://www.spinics.net/lists/netdev/msg213166.htm
>
> The immediate problem is addressed by just reading the id and frag_offs
> fields in the iphdr structure as shown in this patch:
>
> commit 98810abc911b1286a7e4a2ebdfbad66f12fae19d
> Author: Eric Nelson <eric@nelint.com>
> Date: Fri Sep 23 08:26:03 2016 -0700
>
> net: ipv4: af_inet: don't read multiple 16-bit iphdr fields as a 32-bit
> value
>
> Change-Id: Idc7122c22c13ca078be31907d30ab1c3148ba807
> Signed-off-by: Eric Nelson <eric@nelint.com>
>
> diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
> index 0cc98b1..c17ef6e 100644
> --- a/net/ipv4/af_inet.c
> +++ b/net/ipv4/af_inet.c
> @@ -1301,6 +1301,7 @@ static struct sk_buff **inet_gro_receive(struct
> sk_buff **head,
> unsigned int hlen;
> unsigned int off;
> unsigned int id;
> + unsigned int frag;
> int flush = 1;
> int proto;
>
> @@ -1326,9 +1327,9 @@ static struct sk_buff **inet_gro_receive(struct
> sk_buff **head,
> if (unlikely(ip_fast_csum((u8 *)iph, 5)))
> goto out_unlock;
>
> - id = ntohl(*(__be32 *)&iph->id);
> - flush = (u16)((ntohl(*(__be32 *)iph) ^ skb_gro_len(skb)) | (id & ~IP_DF));
> - id >>= 16;
> + id = ntohs(*(__be16 *)&iph->id);
> + frag = ntohs(*(__be16 *)&iph->frag_off);
> + flush = (u16)((ntohl(*(__be32 *)iph) ^ skb_gro_len(skb)) | (frag &
> ~IP_DF));
>
> for (p = *head; p; p = p->next) {
> struct iphdr *iph2;
>

This solves nothing, because a few lines after you'll have yet another
unaligned access :


((__force u32)iph->saddr ^ (__force u32)iph2->saddr) |
((__force u32)iph->daddr ^ (__force u32)iph2->daddr)) {

So you might have one less problematic access, out of hundreds of them
all over the places.

Really the problem is that whole stack depends on the assumption that
IP headers are aligned on arches that care
(ie where NET_IP_ALIGN == 2)

If your build does have NET_IP_ALIGN = 2 and you get a fault here, it
might be because of a buggy driver.

The other known case is some GRE encapsulations that break the
assumption, and this is discussed somewhere else.

Alignment issues with freescale FEC driver

[Eric Dumazet]

On Fri, Sep 23, 2016 at 9:43 AM, Eric Nelson <eric@nelint.com> wrote:
>
> Hello all,
>
> We're seeing alignment issues from the ethernet stack on an i.MX6UL board:
>
> root at mx6ul:~# cat /proc/cpu/alignment
> User: 0
> System: 470010 (inet_gro_receive+0x104/0x278)
>
> This seems to be related to the ip header alignment, and there
> was much discussion in mailing list threads [1] and [2].
>
> In particular, Russell referred to a patch here, but I haven't been
> able to find it:
> https://lists.linaro.org/pipermail/linaro-toolchain/2012-October/002844.html
>
> Eric Dumazet also suggested a path toward fixing it, but I don't quite
> understand the suggestion:
> http://www.spinics.net/lists/netdev/msg213166.htm
>
> The immediate problem is addressed by just reading the id and frag_offs
> fields in the iphdr structure as shown in this patch:
>
> commit 98810abc911b1286a7e4a2ebdfbad66f12fae19d
> Author: Eric Nelson <eric@nelint.com>
> Date: Fri Sep 23 08:26:03 2016 -0700
>
> net: ipv4: af_inet: don't read multiple 16-bit iphdr fields as a 32-bit
> value
>
> Change-Id: Idc7122c22c13ca078be31907d30ab1c3148ba807
> Signed-off-by: Eric Nelson <eric@nelint.com>
>
> diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
> index 0cc98b1..c17ef6e 100644
> --- a/net/ipv4/af_inet.c
> +++ b/net/ipv4/af_inet.c
> @@ -1301,6 +1301,7 @@ static struct sk_buff **inet_gro_receive(struct
> sk_buff **head,
> unsigned int hlen;
> unsigned int off;
> unsigned int id;
> + unsigned int frag;
> int flush = 1;
> int proto;
>
> @@ -1326,9 +1327,9 @@ static struct sk_buff **inet_gro_receive(struct
> sk_buff **head,
> if (unlikely(ip_fast_csum((u8 *)iph, 5)))
> goto out_unlock;
>
> - id = ntohl(*(__be32 *)&iph->id);
> - flush = (u16)((ntohl(*(__be32 *)iph) ^ skb_gro_len(skb)) | (id & ~IP_DF));
> - id >>= 16;
> + id = ntohs(*(__be16 *)&iph->id);
> + frag = ntohs(*(__be16 *)&iph->frag_off);
> + flush = (u16)((ntohl(*(__be32 *)iph) ^ skb_gro_len(skb)) | (frag &
> ~IP_DF));
>
> for (p = *head; p; p = p->next) {
> struct iphdr *iph2;
>

This solves nothing, because a few lines after you'll have yet another
unaligned access :


((__force u32)iph->saddr ^ (__force u32)iph2->saddr) |
((__force u32)iph->daddr ^ (__force u32)iph2->daddr)) {

So you might have one less problematic access, out of hundreds of them
all over the places.

Really the problem is that whole stack depends on the assumption that
IP headers are aligned on arches that care
(ie where NET_IP_ALIGN == 2)

If your build does have NET_IP_ALIGN = 2 and you get a fault here, it
might be because of a buggy driver.

The other known case is some GRE encapsulations that break the
assumption, and this is discussed somewhere else.

Re: Alignment issues with freescale FEC driver

[Eric Nelson]

Thanks Eric,

On 09/23/2016 09:54 AM, Eric Dumazet wrote:
> On Fri, Sep 23, 2016 at 9:43 AM, Eric Nelson <eric@nelint.com> wrote:
>>
>> Hello all,
>>
>> We're seeing alignment issues from the ethernet stack on an i.MX6UL board:
>>
>>

<snip>

>>
>> - id = ntohl(*(__be32 *)&iph->id);
>> - flush = (u16)((ntohl(*(__be32 *)iph) ^ skb_gro_len(skb)) | (id & ~IP_DF));
>> - id >>= 16;
>> + id = ntohs(*(__be16 *)&iph->id);
>> + frag = ntohs(*(__be16 *)&iph->frag_off);
>> + flush = (u16)((ntohl(*(__be32 *)iph) ^ skb_gro_len(skb)) | (frag &
>> ~IP_DF));
>>
>> for (p = *head; p; p = p->next) {
>> struct iphdr *iph2;
>>
> 
> This solves nothing, because a few lines after you'll have yet another
> unaligned access :
> 

Oddly, it does prevent the vast majority (90%+) of the alignment errors.

I believe this is because the compiler is generating an ldm instruction
when the ntohl() call is used, but I'm stumped about why these aren't
generating faults:

> ((__force u32)iph->saddr ^ (__force u32)iph2->saddr) |
> ((__force u32)iph->daddr ^ (__force u32)iph2->daddr)) {
> 
> So you might have one less problematic access, out of hundreds of them
> all over the places.
> 
> Really the problem is that whole stack depends on the assumption that
> IP headers are aligned on arches that care
> (ie where NET_IP_ALIGN == 2)
> 
> If your build does have NET_IP_ALIGN = 2 and you get a fault here, it
> might be because of a buggy driver.
> 

NET_IP_ALIGN is set to 2.

> The other known case is some GRE encapsulations that break the
> assumption, and this is discussed somewhere else.
> 
I don't think that's the case.

# CONFIG_IPV6_GRE is not set

Hmm... Instrumenting the kernel, it seems that iphdr **is** aligned on
a 4-byte boundary.

Does the ldm instruction require 8-byte alignment?

There's definitely a compiler-version dependency involved here,
since using gcc 4.9 also reduced the number of faults dramatically.

Alignment issues with freescale FEC driver

[Eric Nelson]

Thanks Eric,

On 09/23/2016 09:54 AM, Eric Dumazet wrote:
> On Fri, Sep 23, 2016 at 9:43 AM, Eric Nelson <eric@nelint.com> wrote:
>>
>> Hello all,
>>
>> We're seeing alignment issues from the ethernet stack on an i.MX6UL board:
>>
>>

<snip>

>>
>> - id = ntohl(*(__be32 *)&iph->id);
>> - flush = (u16)((ntohl(*(__be32 *)iph) ^ skb_gro_len(skb)) | (id & ~IP_DF));
>> - id >>= 16;
>> + id = ntohs(*(__be16 *)&iph->id);
>> + frag = ntohs(*(__be16 *)&iph->frag_off);
>> + flush = (u16)((ntohl(*(__be32 *)iph) ^ skb_gro_len(skb)) | (frag &
>> ~IP_DF));
>>
>> for (p = *head; p; p = p->next) {
>> struct iphdr *iph2;
>>
> 
> This solves nothing, because a few lines after you'll have yet another
> unaligned access :
> 

Oddly, it does prevent the vast majority (90%+) of the alignment errors.

I believe this is because the compiler is generating an ldm instruction
when the ntohl() call is used, but I'm stumped about why these aren't
generating faults:

> ((__force u32)iph->saddr ^ (__force u32)iph2->saddr) |
> ((__force u32)iph->daddr ^ (__force u32)iph2->daddr)) {
> 
> So you might have one less problematic access, out of hundreds of them
> all over the places.
> 
> Really the problem is that whole stack depends on the assumption that
> IP headers are aligned on arches that care
> (ie where NET_IP_ALIGN == 2)
> 
> If your build does have NET_IP_ALIGN = 2 and you get a fault here, it
> might be because of a buggy driver.
> 

NET_IP_ALIGN is set to 2.

> The other known case is some GRE encapsulations that break the
> assumption, and this is discussed somewhere else.
> 
I don't think that's the case.

# CONFIG_IPV6_GRE is not set

Hmm... Instrumenting the kernel, it seems that iphdr **is** aligned on
a 4-byte boundary.

Does the ldm instruction require 8-byte alignment?

There's definitely a compiler-version dependency involved here,
since using gcc 4.9 also reduced the number of faults dramatically.

Re: Alignment issues with freescale FEC driver

[Eric Nelson]

Hi Eric,

On 09/23/2016 10:19 AM, Eric Nelson wrote:
> Thanks Eric,
> 
> On 09/23/2016 09:54 AM, Eric Dumazet wrote:
>> On Fri, Sep 23, 2016 at 9:43 AM, Eric Nelson <eric@nelint.com> wrote:
>>>
>>> Hello all,
>>>
>>> We're seeing alignment issues from the ethernet stack on an i.MX6UL board:
>>>
>>>
> 
> <snip>
> 
>>>
>>> - id = ntohl(*(__be32 *)&iph->id);
>>> - flush = (u16)((ntohl(*(__be32 *)iph) ^ skb_gro_len(skb)) | (id & ~IP_DF));
>>> - id >>= 16;
>>> + id = ntohs(*(__be16 *)&iph->id);
>>> + frag = ntohs(*(__be16 *)&iph->frag_off);
>>> + flush = (u16)((ntohl(*(__be32 *)iph) ^ skb_gro_len(skb)) | (frag &
>>> ~IP_DF));
>>>
>>> for (p = *head; p; p = p->next) {
>>> struct iphdr *iph2;
>>>
>>
>> This solves nothing, because a few lines after you'll have yet another
>> unaligned access :
>>
> 
> Oddly, it does prevent the vast majority (90%+) of the alignment errors.
> 
> I believe this is because the compiler is generating an ldm instruction
> when the ntohl() call is used, but I'm stumped about why these aren't
> generating faults:
> 
>> ((__force u32)iph->saddr ^ (__force u32)iph2->saddr) |
>> ((__force u32)iph->daddr ^ (__force u32)iph2->daddr)) {
>>
>> So you might have one less problematic access, out of hundreds of them
>> all over the places.
>>
>> Really the problem is that whole stack depends on the assumption that
>> IP headers are aligned on arches that care
>> (ie where NET_IP_ALIGN == 2)
>>
>> If your build does have NET_IP_ALIGN = 2 and you get a fault here, it
>> might be because of a buggy driver.
>>
> 
> NET_IP_ALIGN is set to 2.
> 
>> The other known case is some GRE encapsulations that break the
>> assumption, and this is discussed somewhere else.
>>
> I don't think that's the case.
> 
> # CONFIG_IPV6_GRE is not set
> 
> Hmm... Instrumenting the kernel, it seems that iphdr **is** aligned on
> a 4-byte boundary.
> 

No. That was wrong.

The iphdr is aligned at offsets of 14 from the ethernet frame, which itself
is longword aligned.

I mistakenly tested before the call to skb_gro_header_slow(), when
iph was NULL.

After putting a test in the right place, I'm seeing an address of
888a364e for the first un-aligned packet.

Since the hardware requires longword alignment for its' DMA transfers,
aligning the IP header will require a memcpy, right?

You hinted at a solution in this post:

http://www.spinics.net/lists/netdev/msg213166.html

Are you aware of another driver doing this that could be used as
a reference?

Please advise,


Eric

Alignment issues with freescale FEC driver

[Eric Nelson]

Hi Eric,

On 09/23/2016 10:19 AM, Eric Nelson wrote:
> Thanks Eric,
> 
> On 09/23/2016 09:54 AM, Eric Dumazet wrote:
>> On Fri, Sep 23, 2016 at 9:43 AM, Eric Nelson <eric@nelint.com> wrote:
>>>
>>> Hello all,
>>>
>>> We're seeing alignment issues from the ethernet stack on an i.MX6UL board:
>>>
>>>
> 
> <snip>
> 
>>>
>>> - id = ntohl(*(__be32 *)&iph->id);
>>> - flush = (u16)((ntohl(*(__be32 *)iph) ^ skb_gro_len(skb)) | (id & ~IP_DF));
>>> - id >>= 16;
>>> + id = ntohs(*(__be16 *)&iph->id);
>>> + frag = ntohs(*(__be16 *)&iph->frag_off);
>>> + flush = (u16)((ntohl(*(__be32 *)iph) ^ skb_gro_len(skb)) | (frag &
>>> ~IP_DF));
>>>
>>> for (p = *head; p; p = p->next) {
>>> struct iphdr *iph2;
>>>
>>
>> This solves nothing, because a few lines after you'll have yet another
>> unaligned access :
>>
> 
> Oddly, it does prevent the vast majority (90%+) of the alignment errors.
> 
> I believe this is because the compiler is generating an ldm instruction
> when the ntohl() call is used, but I'm stumped about why these aren't
> generating faults:
> 
>> ((__force u32)iph->saddr ^ (__force u32)iph2->saddr) |
>> ((__force u32)iph->daddr ^ (__force u32)iph2->daddr)) {
>>
>> So you might have one less problematic access, out of hundreds of them
>> all over the places.
>>
>> Really the problem is that whole stack depends on the assumption that
>> IP headers are aligned on arches that care
>> (ie where NET_IP_ALIGN == 2)
>>
>> If your build does have NET_IP_ALIGN = 2 and you get a fault here, it
>> might be because of a buggy driver.
>>
> 
> NET_IP_ALIGN is set to 2.
> 
>> The other known case is some GRE encapsulations that break the
>> assumption, and this is discussed somewhere else.
>>
> I don't think that's the case.
> 
> # CONFIG_IPV6_GRE is not set
> 
> Hmm... Instrumenting the kernel, it seems that iphdr **is** aligned on
> a 4-byte boundary.
> 

No. That was wrong.

The iphdr is aligned at offsets of 14 from the ethernet frame, which itself
is longword aligned.

I mistakenly tested before the call to skb_gro_header_slow(), when
iph was NULL.

After putting a test in the right place, I'm seeing an address of
888a364e for the first un-aligned packet.

Since the hardware requires longword alignment for its' DMA transfers,
aligning the IP header will require a memcpy, right?

You hinted at a solution in this post:

http://www.spinics.net/lists/netdev/msg213166.html

Are you aware of another driver doing this that could be used as
a reference?

Please advise,


Eric

Re: Alignment issues with freescale FEC driver

[Russell King - ARM Linux]

On Fri, Sep 23, 2016 at 10:19:50AM -0700, Eric Nelson wrote:
> Oddly, it does prevent the vast majority (90%+) of the alignment errors.
> 
> I believe this is because the compiler is generating an ldm instruction
> when the ntohl() call is used, but I'm stumped about why these aren't
> generating faults:

ldm generates alignment faults when the address is not aligned to a
32-bit boundary.  ldr on ARMv6+ does not.

> I don't think that's the case.
> 
> # CONFIG_IPV6_GRE is not set
> 
> Hmm... Instrumenting the kernel, it seems that iphdr **is** aligned on
> a 4-byte boundary.
> 
> Does the ldm instruction require 8-byte alignment?
> 
> There's definitely a compiler-version dependency involved here,
> since using gcc 4.9 also reduced the number of faults dramatically.

Well, I don't think it's that gcc related:

User:           0
System:         312855 (ip6_route_input+0x6c/0x1e0)
Skipped:        0
Half:           0
Word:           0
DWord:          2
Multi:          312853

c06d8998 <ip6_route_input>:
c06d89ac:       e1a04000        mov     r4, r0
c06d89b0:       e1d489b4        ldrh    r8, [r4, #148]  ; 0x94
c06d89b8:       e594a0a0        ldr     sl, [r4, #160]  ; 0xa0
c06d89cc:       e08ac008        add     ip, sl, r8
c06d89d4:       e28c3018        add     r3, ip, #24
c06d89dc:       e28c7008        add     r7, ip, #8
c06d89e4:       e893000f        ldm     r3, {r0, r1, r2, r3}
c06d89ec:       e24be044        sub     lr, fp, #68     ; 0x44
c06d89f4:       e24b5054        sub     r5, fp, #84     ; 0x54
c06d89fc:       e885000f        stm     r5, {r0, r1, r2, r3}
c06d8a04:       e897000f        ldm     r7, {r0, r1, r2, r3}
c06d8a10:       e88e000f        stm     lr, {r0, r1, r2, r3}

This is from:

        struct flowi6 fl6 = {
                .flowi6_iif = l3mdev_fib_oif(skb->dev),
                .daddr = iph->daddr,
                .saddr = iph->saddr,
                .flowlabel = ip6_flowinfo(iph),
                .flowi6_mark = skb->mark,
                .flowi6_proto = iph->nexthdr,
        };

specifically, I suspect, the saddr and daddr initialisations.

There's not much to get away from this - the FEC on iMX requires a
16-byte alignment for DMA addresses, which violates the network
stack's requirement for the ethernet packet to be received with a
two byte offset.  So the IP header (and IPv6 headers) will always
be mis-aligned in memory, which leads to a huge number of alignment
faults.

There's not much getting away from this - the problem is not in the
networking stack, but the FEC hardware/network driver.  See:

        struct  fec_enet_private *fep = netdev_priv(ndev);
        int off;

        off = ((unsigned long)skb->data) & fep->rx_align;
        if (off)
                skb_reserve(skb, fep->rx_align + 1 - off);

        bdp->cbd_bufaddr = cpu_to_fec32(dma_map_single(&fep->pdev->dev, skb->data, FEC_ENET_RX_FRSIZE - fep->rx_align, DMA_FROM_DEVICE));

in fec_enet_new_rxbdp().

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Alignment issues with freescale FEC driver

[Russell King - ARM Linux]

On Fri, Sep 23, 2016 at 10:19:50AM -0700, Eric Nelson wrote:
> Oddly, it does prevent the vast majority (90%+) of the alignment errors.
> 
> I believe this is because the compiler is generating an ldm instruction
> when the ntohl() call is used, but I'm stumped about why these aren't
> generating faults:

ldm generates alignment faults when the address is not aligned to a
32-bit boundary.  ldr on ARMv6+ does not.

> I don't think that's the case.
> 
> # CONFIG_IPV6_GRE is not set
> 
> Hmm... Instrumenting the kernel, it seems that iphdr **is** aligned on
> a 4-byte boundary.
> 
> Does the ldm instruction require 8-byte alignment?
> 
> There's definitely a compiler-version dependency involved here,
> since using gcc 4.9 also reduced the number of faults dramatically.

Well, I don't think it's that gcc related:

User:           0
System:         312855 (ip6_route_input+0x6c/0x1e0)
Skipped:        0
Half:           0
Word:           0
DWord:          2
Multi:          312853

c06d8998 <ip6_route_input>:
c06d89ac:       e1a04000        mov     r4, r0
c06d89b0:       e1d489b4        ldrh    r8, [r4, #148]  ; 0x94
c06d89b8:       e594a0a0        ldr     sl, [r4, #160]  ; 0xa0
c06d89cc:       e08ac008        add     ip, sl, r8
c06d89d4:       e28c3018        add     r3, ip, #24
c06d89dc:       e28c7008        add     r7, ip, #8
c06d89e4:       e893000f        ldm     r3, {r0, r1, r2, r3}
c06d89ec:       e24be044        sub     lr, fp, #68     ; 0x44
c06d89f4:       e24b5054        sub     r5, fp, #84     ; 0x54
c06d89fc:       e885000f        stm     r5, {r0, r1, r2, r3}
c06d8a04:       e897000f        ldm     r7, {r0, r1, r2, r3}
c06d8a10:       e88e000f        stm     lr, {r0, r1, r2, r3}

This is from:

        struct flowi6 fl6 = {
                .flowi6_iif = l3mdev_fib_oif(skb->dev),
                .daddr = iph->daddr,
                .saddr = iph->saddr,
                .flowlabel = ip6_flowinfo(iph),
                .flowi6_mark = skb->mark,
                .flowi6_proto = iph->nexthdr,
        };

specifically, I suspect, the saddr and daddr initialisations.

There's not much to get away from this - the FEC on iMX requires a
16-byte alignment for DMA addresses, which violates the network
stack's requirement for the ethernet packet to be received with a
two byte offset.  So the IP header (and IPv6 headers) will always
be mis-aligned in memory, which leads to a huge number of alignment
faults.

There's not much getting away from this - the problem is not in the
networking stack, but the FEC hardware/network driver.  See:

        struct  fec_enet_private *fep = netdev_priv(ndev);
        int off;

        off = ((unsigned long)skb->data) & fep->rx_align;
        if (off)
                skb_reserve(skb, fep->rx_align + 1 - off);

        bdp->cbd_bufaddr = cpu_to_fec32(dma_map_single(&fep->pdev->dev, skb->data, FEC_ENET_RX_FRSIZE - fep->rx_align, DMA_FROM_DEVICE));

in fec_enet_new_rxbdp().

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: Alignment issues with freescale FEC driver

[Andrew Lunn]

> Since the hardware requires longword alignment for its' DMA transfers,
> aligning the IP header will require a memcpy, right?

The vf610 FEC has an SHIFT16 bit in register ENETx_TACC, which inserts
two padding bits on transmit. ENETx_RACC has the same.

What about your hardware?

     Andrew

Alignment issues with freescale FEC driver

[Andrew Lunn]

> Since the hardware requires longword alignment for its' DMA transfers,
> aligning the IP header will require a memcpy, right?

The vf610 FEC has an SHIFT16 bit in register ENETx_TACC, which inserts
two padding bits on transmit. ENETx_RACC has the same.

What about your hardware?

     Andrew

Re: Alignment issues with freescale FEC driver

[Eric Nelson]

Thanks Russell,

On 09/23/2016 10:37 AM, Russell King - ARM Linux wrote:
> On Fri, Sep 23, 2016 at 10:19:50AM -0700, Eric Nelson wrote:
>> Oddly, it does prevent the vast majority (90%+) of the alignment errors.
>>
>> I believe this is because the compiler is generating an ldm instruction
>> when the ntohl() call is used, but I'm stumped about why these aren't
>> generating faults:

After looking at it, I have to think that the code that reads iph->id
is just hit more frequently than the other code in this routine.

> 
> ldm generates alignment faults when the address is not aligned to a
> 32-bit boundary.  ldr on ARMv6+ does not.
> 
>> I don't think that's the case.
>>
>> # CONFIG_IPV6_GRE is not set
>>
>> Hmm... Instrumenting the kernel, it seems that iphdr **is** aligned on
>> a 4-byte boundary.
>>
>> Does the ldm instruction require 8-byte alignment?
>>
>> There's definitely a compiler-version dependency involved here,
>> since using gcc 4.9 also reduced the number of faults dramatically.
> 
> Well, I don't think it's that gcc related:
> 

I can only say that I noticed a dramatic drop in the number of faults, and
didn't see the inet_gro_receive reported in /proc/cpu/alignment with gcc 4.9
when trying to identify the issue.

> User:           0
> System:         312855 (ip6_route_input+0x6c/0x1e0)
> Skipped:        0
> Half:           0
> Word:           0
> DWord:          2
> Multi:          312853
> 
> c06d8998 <ip6_route_input>:
> c06d89ac:       e1a04000        mov     r4, r0
> c06d89b0:       e1d489b4        ldrh    r8, [r4, #148]  ; 0x94
> c06d89b8:       e594a0a0        ldr     sl, [r4, #160]  ; 0xa0
> c06d89cc:       e08ac008        add     ip, sl, r8
> c06d89d4:       e28c3018        add     r3, ip, #24
> c06d89dc:       e28c7008        add     r7, ip, #8
> c06d89e4:       e893000f        ldm     r3, {r0, r1, r2, r3}
> c06d89ec:       e24be044        sub     lr, fp, #68     ; 0x44
> c06d89f4:       e24b5054        sub     r5, fp, #84     ; 0x54
> c06d89fc:       e885000f        stm     r5, {r0, r1, r2, r3}
> c06d8a04:       e897000f        ldm     r7, {r0, r1, r2, r3}
> c06d8a10:       e88e000f        stm     lr, {r0, r1, r2, r3}
> 
> This is from:
> 
>         struct flowi6 fl6 = {
>                 .flowi6_iif = l3mdev_fib_oif(skb->dev),
>                 .daddr = iph->daddr,
>                 .saddr = iph->saddr,
>                 .flowlabel = ip6_flowinfo(iph),
>                 .flowi6_mark = skb->mark,
>                 .flowi6_proto = iph->nexthdr,
>         };
> 
> specifically, I suspect, the saddr and daddr initialisations.
> 
> There's not much to get away from this - the FEC on iMX requires a
> 16-byte alignment for DMA addresses, which violates the network
> stack's requirement for the ethernet packet to be received with a
> two byte offset.  So the IP header (and IPv6 headers) will always
> be mis-aligned in memory, which leads to a huge number of alignment
> faults.
> 
> There's not much getting away from this - the problem is not in the
> networking stack, but the FEC hardware/network driver.  See:
> 
>         struct  fec_enet_private *fep = netdev_priv(ndev);
>         int off;
> 
>         off = ((unsigned long)skb->data) & fep->rx_align;
>         if (off)
>                 skb_reserve(skb, fep->rx_align + 1 - off);
> 
>         bdp->cbd_bufaddr = cpu_to_fec32(dma_map_single(&fep->pdev->dev, skb->data, FEC_ENET_RX_FRSIZE - fep->rx_align, DMA_FROM_DEVICE));
> 
> in fec_enet_new_rxbdp().
> 

So the question is: should we just live with this and acknowledge a
performance penalty of bad alignment or do something about it?

I'm not sure the cost (or the details) of Eric's proposed fix of allocating
and copying the header to another skb.

The original report was of bad network performance, but I haven't
been able to see an impact doing some simple tests using wget
and SSH.

Alignment issues with freescale FEC driver

[Eric Nelson]

Thanks Russell,

On 09/23/2016 10:37 AM, Russell King - ARM Linux wrote:
> On Fri, Sep 23, 2016 at 10:19:50AM -0700, Eric Nelson wrote:
>> Oddly, it does prevent the vast majority (90%+) of the alignment errors.
>>
>> I believe this is because the compiler is generating an ldm instruction
>> when the ntohl() call is used, but I'm stumped about why these aren't
>> generating faults:

After looking at it, I have to think that the code that reads iph->id
is just hit more frequently than the other code in this routine.

> 
> ldm generates alignment faults when the address is not aligned to a
> 32-bit boundary.  ldr on ARMv6+ does not.
> 
>> I don't think that's the case.
>>
>> # CONFIG_IPV6_GRE is not set
>>
>> Hmm... Instrumenting the kernel, it seems that iphdr **is** aligned on
>> a 4-byte boundary.
>>
>> Does the ldm instruction require 8-byte alignment?
>>
>> There's definitely a compiler-version dependency involved here,
>> since using gcc 4.9 also reduced the number of faults dramatically.
> 
> Well, I don't think it's that gcc related:
> 

I can only say that I noticed a dramatic drop in the number of faults, and
didn't see the inet_gro_receive reported in /proc/cpu/alignment with gcc 4.9
when trying to identify the issue.

> User:           0
> System:         312855 (ip6_route_input+0x6c/0x1e0)
> Skipped:        0
> Half:           0
> Word:           0
> DWord:          2
> Multi:          312853
> 
> c06d8998 <ip6_route_input>:
> c06d89ac:       e1a04000        mov     r4, r0
> c06d89b0:       e1d489b4        ldrh    r8, [r4, #148]  ; 0x94
> c06d89b8:       e594a0a0        ldr     sl, [r4, #160]  ; 0xa0
> c06d89cc:       e08ac008        add     ip, sl, r8
> c06d89d4:       e28c3018        add     r3, ip, #24
> c06d89dc:       e28c7008        add     r7, ip, #8
> c06d89e4:       e893000f        ldm     r3, {r0, r1, r2, r3}
> c06d89ec:       e24be044        sub     lr, fp, #68     ; 0x44
> c06d89f4:       e24b5054        sub     r5, fp, #84     ; 0x54
> c06d89fc:       e885000f        stm     r5, {r0, r1, r2, r3}
> c06d8a04:       e897000f        ldm     r7, {r0, r1, r2, r3}
> c06d8a10:       e88e000f        stm     lr, {r0, r1, r2, r3}
> 
> This is from:
> 
>         struct flowi6 fl6 = {
>                 .flowi6_iif = l3mdev_fib_oif(skb->dev),
>                 .daddr = iph->daddr,
>                 .saddr = iph->saddr,
>                 .flowlabel = ip6_flowinfo(iph),
>                 .flowi6_mark = skb->mark,
>                 .flowi6_proto = iph->nexthdr,
>         };
> 
> specifically, I suspect, the saddr and daddr initialisations.
> 
> There's not much to get away from this - the FEC on iMX requires a
> 16-byte alignment for DMA addresses, which violates the network
> stack's requirement for the ethernet packet to be received with a
> two byte offset.  So the IP header (and IPv6 headers) will always
> be mis-aligned in memory, which leads to a huge number of alignment
> faults.
> 
> There's not much getting away from this - the problem is not in the
> networking stack, but the FEC hardware/network driver.  See:
> 
>         struct  fec_enet_private *fep = netdev_priv(ndev);
>         int off;
> 
>         off = ((unsigned long)skb->data) & fep->rx_align;
>         if (off)
>                 skb_reserve(skb, fep->rx_align + 1 - off);
> 
>         bdp->cbd_bufaddr = cpu_to_fec32(dma_map_single(&fep->pdev->dev, skb->data, FEC_ENET_RX_FRSIZE - fep->rx_align, DMA_FROM_DEVICE));
> 
> in fec_enet_new_rxbdp().
> 

So the question is: should we just live with this and acknowledge a
performance penalty of bad alignment or do something about it?

I'm not sure the cost (or the details) of Eric's proposed fix of allocating
and copying the header to another skb.

The original report was of bad network performance, but I haven't
been able to see an impact doing some simple tests using wget
and SSH.

Re: Alignment issues with freescale FEC driver

[Russell King - ARM Linux]

On Fri, Sep 23, 2016 at 08:13:01PM +0200, Andrew Lunn wrote:
> > Since the hardware requires longword alignment for its' DMA transfers,
> > aligning the IP header will require a memcpy, right?
> 
> The vf610 FEC has an SHIFT16 bit in register ENETx_TACC, which inserts
> two padding bits on transmit. ENETx_RACC has the same.
> 
> What about your hardware?

The iMX6 FEC also has that ability - as part of my FEC patch stack from
ages ago, I implemented support for it.

  "net:fec: implement almost zero-copy receive path"

in my public fec-testing branch.

That patch stack is sadly now totally dead and I've no interest in
reviving it myself.  There was some interest from others in taking my
patch stack over, but that went quiet.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Alignment issues with freescale FEC driver

[Russell King - ARM Linux]

On Fri, Sep 23, 2016 at 08:13:01PM +0200, Andrew Lunn wrote:
> > Since the hardware requires longword alignment for its' DMA transfers,
> > aligning the IP header will require a memcpy, right?
> 
> The vf610 FEC has an SHIFT16 bit in register ENETx_TACC, which inserts
> two padding bits on transmit. ENETx_RACC has the same.
> 
> What about your hardware?

The iMX6 FEC also has that ability - as part of my FEC patch stack from
ages ago, I implemented support for it.

  "net:fec: implement almost zero-copy receive path"

in my public fec-testing branch.

That patch stack is sadly now totally dead and I've no interest in
reviving it myself.  There was some interest from others in taking my
patch stack over, but that went quiet.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: Alignment issues with freescale FEC driver

[Eric Nelson]

Thanks Andrew.

On 09/23/2016 11:13 AM, Andrew Lunn wrote:
>> Since the hardware requires longword alignment for its' DMA transfers,
>> aligning the IP header will require a memcpy, right?
> 
> The vf610 FEC has an SHIFT16 bit in register ENETx_TACC, which inserts
> two padding bits on transmit. ENETx_RACC has the same.
> 
> What about your hardware?
> 

You got me with the RTFM!

>From the i.MX6DQ reference manual, bit 7 of ENET_RACC says this:

"RX FIFO Shift-16

When this field is set, the actual frame data starts at bit 16 of the first
word read from the RX FIFO aligning the Ethernet payload on a
32-bit boundary."

Same for the i.MX6UL.

I'm not sure what it will take to use this, but it seems to be exactly
what we're looking for.

Alignment issues with freescale FEC driver

[Eric Nelson]

Thanks Andrew.

On 09/23/2016 11:13 AM, Andrew Lunn wrote:
>> Since the hardware requires longword alignment for its' DMA transfers,
>> aligning the IP header will require a memcpy, right?
> 
> The vf610 FEC has an SHIFT16 bit in register ENETx_TACC, which inserts
> two padding bits on transmit. ENETx_RACC has the same.
> 
> What about your hardware?
> 

You got me with the RTFM!

>From the i.MX6DQ reference manual, bit 7 of ENET_RACC says this:

"RX FIFO Shift-16

When this field is set, the actual frame data starts at bit 16 of the first
word read from the RX FIFO aligning the Ethernet payload on a
32-bit boundary."

Same for the i.MX6UL.

I'm not sure what it will take to use this, but it seems to be exactly
what we're looking for.

Re: Alignment issues with freescale FEC driver

[Russell King - ARM Linux]

On Fri, Sep 23, 2016 at 11:26:18AM -0700, Eric Nelson wrote:
> So the question is: should we just live with this and acknowledge a
> performance penalty of bad alignment or do something about it?

Well, I've no interest in trying to do anything with the FEC driver
anymore, as I'll just generate another big patch stack which won't
make it into the kernel in a timely fashion - my last attempt at
improving the FEC driver was dogged with conflicting changes and I
gave up with it in the end.  I ended up spending a full cycle
rebasing, re-testing, and re-evaluating their performance only to find
that I'd missed the merge window again, and other conflicting changes
got merged which meant that I had to start from the beginning again.

> I'm not sure the cost (or the details) of Eric's proposed fix of allocating
> and copying the header to another skb.

I had a quick look at this, and although Eric's idea may be a good
idea, it doesn't contain enough details for me to be able to
implement it - eg, I've no idea how to attach the 128-byte skb to the
beginning of a previously allocated skb containing the rest of the
packet.  I've just looked through linux/skbuff.h and I can't see
anything that takes two sk_buff's that would do the job.

However, I don't think that's necessary in this case, because the
iMX6 FEC supports the 16-bit alignment of the packet, if only it was
enabled in hardware and the driver caters for it.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Alignment issues with freescale FEC driver

[Russell King - ARM Linux]

On Fri, Sep 23, 2016 at 11:26:18AM -0700, Eric Nelson wrote:
> So the question is: should we just live with this and acknowledge a
> performance penalty of bad alignment or do something about it?

Well, I've no interest in trying to do anything with the FEC driver
anymore, as I'll just generate another big patch stack which won't
make it into the kernel in a timely fashion - my last attempt at
improving the FEC driver was dogged with conflicting changes and I
gave up with it in the end.  I ended up spending a full cycle
rebasing, re-testing, and re-evaluating their performance only to find
that I'd missed the merge window again, and other conflicting changes
got merged which meant that I had to start from the beginning again.

> I'm not sure the cost (or the details) of Eric's proposed fix of allocating
> and copying the header to another skb.

I had a quick look at this, and although Eric's idea may be a good
idea, it doesn't contain enough details for me to be able to
implement it - eg, I've no idea how to attach the 128-byte skb to the
beginning of a previously allocated skb containing the rest of the
packet.  I've just looked through linux/skbuff.h and I can't see
anything that takes two sk_buff's that would do the job.

However, I don't think that's necessary in this case, because the
iMX6 FEC supports the 16-bit alignment of the packet, if only it was
enabled in hardware and the driver caters for it.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: Alignment issues with freescale FEC driver

[Eric Nelson]

Thanks Russell,

On 09/23/2016 11:30 AM, Russell King - ARM Linux wrote:
> On Fri, Sep 23, 2016 at 08:13:01PM +0200, Andrew Lunn wrote:
>>> Since the hardware requires longword alignment for its' DMA transfers,
>>> aligning the IP header will require a memcpy, right?
>>
>> The vf610 FEC has an SHIFT16 bit in register ENETx_TACC, which inserts
>> two padding bits on transmit. ENETx_RACC has the same.
>>
>> What about your hardware?
> 
> The iMX6 FEC also has that ability - as part of my FEC patch stack from
> ages ago, I implemented support for it.
> 
>   "net:fec: implement almost zero-copy receive path"
> 
> in my public fec-testing branch.
> 
> That patch stack is sadly now totally dead and I've no interest in
> reviving it myself.  There was some interest from others in taking my
> patch stack over, but that went quiet.
> 

I'll take a look and hopefully revive at least part of the patch set.

Alignment issues with freescale FEC driver

[Eric Nelson]

Thanks Russell,

On 09/23/2016 11:30 AM, Russell King - ARM Linux wrote:
> On Fri, Sep 23, 2016 at 08:13:01PM +0200, Andrew Lunn wrote:
>>> Since the hardware requires longword alignment for its' DMA transfers,
>>> aligning the IP header will require a memcpy, right?
>>
>> The vf610 FEC has an SHIFT16 bit in register ENETx_TACC, which inserts
>> two padding bits on transmit. ENETx_RACC has the same.
>>
>> What about your hardware?
> 
> The iMX6 FEC also has that ability - as part of my FEC patch stack from
> ages ago, I implemented support for it.
> 
>   "net:fec: implement almost zero-copy receive path"
> 
> in my public fec-testing branch.
> 
> That patch stack is sadly now totally dead and I've no interest in
> reviving it myself.  There was some interest from others in taking my
> patch stack over, but that went quiet.
> 

I'll take a look and hopefully revive at least part of the patch set.

Re: Alignment issues with freescale FEC driver

[Eric Nelson]

Thanks Russell,

On 09/23/2016 11:37 AM, Russell King - ARM Linux wrote:
> On Fri, Sep 23, 2016 at 11:26:18AM -0700, Eric Nelson wrote:
>> So the question is: should we just live with this and acknowledge a
>> performance penalty of bad alignment or do something about it?
> 
> Well, I've no interest in trying to do anything with the FEC driver
> anymore, as I'll just generate another big patch stack which won't
> make it into the kernel in a timely fashion - my last attempt at
> improving the FEC driver was dogged with conflicting changes and I
> gave up with it in the end.  I ended up spending a full cycle
> rebasing, re-testing, and re-evaluating their performance only to find
> that I'd missed the merge window again, and other conflicting changes
> got merged which meant that I had to start from the beginning again.
> 

That's sad. I recall reading your notes on that patch series and it was
a model for how to structure and document a patch set.

I hadn't noticed that you abandoned it and it's frustrating that the
merge process prevented your efforts from being used.

I'm also disheartened to hear your frustration about getting things
pushed up-stream and the entire Linux community should take note.

>> I'm not sure the cost (or the details) of Eric's proposed fix of allocating
>> and copying the header to another skb.
> 
> I had a quick look at this, and although Eric's idea may be a good
> idea, it doesn't contain enough details for me to be able to
> implement it - eg, I've no idea how to attach the 128-byte skb to the
> beginning of a previously allocated skb containing the rest of the
> packet.  I've just looked through linux/skbuff.h and I can't see
> anything that takes two sk_buff's that would do the job.
> 
> However, I don't think that's necessary in this case, because the
> iMX6 FEC supports the 16-bit alignment of the packet, if only it was
> enabled in hardware and the driver caters for it.
> 

Right. If the hardware supports placing things at a suitable address,
that's the right approach.

I'll try to review your earlier patch set and at least find a way to address
the alignment issues.

I'm a bit booked until LinuxCon but will try to get something out soon.

Regards,


Eric

Alignment issues with freescale FEC driver

[Eric Nelson]

Thanks Russell,

On 09/23/2016 11:37 AM, Russell King - ARM Linux wrote:
> On Fri, Sep 23, 2016 at 11:26:18AM -0700, Eric Nelson wrote:
>> So the question is: should we just live with this and acknowledge a
>> performance penalty of bad alignment or do something about it?
> 
> Well, I've no interest in trying to do anything with the FEC driver
> anymore, as I'll just generate another big patch stack which won't
> make it into the kernel in a timely fashion - my last attempt at
> improving the FEC driver was dogged with conflicting changes and I
> gave up with it in the end.  I ended up spending a full cycle
> rebasing, re-testing, and re-evaluating their performance only to find
> that I'd missed the merge window again, and other conflicting changes
> got merged which meant that I had to start from the beginning again.
> 

That's sad. I recall reading your notes on that patch series and it was
a model for how to structure and document a patch set.

I hadn't noticed that you abandoned it and it's frustrating that the
merge process prevented your efforts from being used.

I'm also disheartened to hear your frustration about getting things
pushed up-stream and the entire Linux community should take note.

>> I'm not sure the cost (or the details) of Eric's proposed fix of allocating
>> and copying the header to another skb.
> 
> I had a quick look at this, and although Eric's idea may be a good
> idea, it doesn't contain enough details for me to be able to
> implement it - eg, I've no idea how to attach the 128-byte skb to the
> beginning of a previously allocated skb containing the rest of the
> packet.  I've just looked through linux/skbuff.h and I can't see
> anything that takes two sk_buff's that would do the job.
> 
> However, I don't think that's necessary in this case, because the
> iMX6 FEC supports the 16-bit alignment of the packet, if only it was
> enabled in hardware and the driver caters for it.
> 

Right. If the hardware supports placing things at a suitable address,
that's the right approach.

I'll try to review your earlier patch set and at least find a way to address
the alignment issues.

I'm a bit booked until LinuxCon but will try to get something out soon.

Regards,


Eric

Re: Alignment issues with freescale FEC driver

[Uwe Kleine-König]

Hello Russell,

On Fri, Sep 23, 2016 at 07:37:25PM +0100, Russell King - ARM Linux wrote:
> On Fri, Sep 23, 2016 at 11:26:18AM -0700, Eric Nelson wrote:
> > So the question is: should we just live with this and acknowledge a
> > performance penalty of bad alignment or do something about it?
> 
> Well, I've no interest in trying to do anything with the FEC driver
> anymore, as I'll just generate another big patch stack which won't
> make it into the kernel in a timely fashion - my last attempt at
> improving the FEC driver was dogged with conflicting changes and I
> gave up with it in the end.  I ended up spending a full cycle
> rebasing, re-testing, and re-evaluating their performance only to find
> that I'd missed the merge window again, and other conflicting changes
> got merged which meant that I had to start from the beginning again.

I'm not included in the set of people who are responsible to review and
merge fec patches, but I'd be surprised if you couldn't get an exclusive
lock for that driver. Something like: After 4.X-rc1 the fec isn't
touched any more until you got your series ready for the 4.X+1 merge
window. Of course some fixes might have to go in, but these hopefully
won't disturb much.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Alignment issues with freescale FEC driver

[Uwe Kleine-König]

Hello Russell,

On Fri, Sep 23, 2016 at 07:37:25PM +0100, Russell King - ARM Linux wrote:
> On Fri, Sep 23, 2016 at 11:26:18AM -0700, Eric Nelson wrote:
> > So the question is: should we just live with this and acknowledge a
> > performance penalty of bad alignment or do something about it?
> 
> Well, I've no interest in trying to do anything with the FEC driver
> anymore, as I'll just generate another big patch stack which won't
> make it into the kernel in a timely fashion - my last attempt at
> improving the FEC driver was dogged with conflicting changes and I
> gave up with it in the end.  I ended up spending a full cycle
> rebasing, re-testing, and re-evaluating their performance only to find
> that I'd missed the merge window again, and other conflicting changes
> got merged which meant that I had to start from the beginning again.

I'm not included in the set of people who are responsible to review and
merge fec patches, but I'd be surprised if you couldn't get an exclusive
lock for that driver. Something like: After 4.X-rc1 the fec isn't
touched any more until you got your series ready for the 4.X+1 merge
window. Of course some fixes might have to go in, but these hopefully
won't disturb much.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: Alignment issues with freescale FEC driver

[David Miller]

From: Eric Nelson <eric@nelint.com>
Date: Fri, 23 Sep 2016 10:33:29 -0700

> Since the hardware requires longword alignment for its' DMA transfers,
> aligning the IP header will require a memcpy, right?

I wish hardware designers didn't do this.

There is no conflict between DMA alignment and properly offseting
the packet data by two bytes.

All hardware designers have to do is allow 2 padding bytes to be
emitted by the chip before the actual packet data.

Then the longword or whatever DMA transfer alignment is met
whilst still giving the necessary flexibility for where the
packet data lands.

Alignment issues with freescale FEC driver

[David Miller]

From: Eric Nelson <eric@nelint.com>
Date: Fri, 23 Sep 2016 10:33:29 -0700

> Since the hardware requires longword alignment for its' DMA transfers,
> aligning the IP header will require a memcpy, right?

I wish hardware designers didn't do this.

There is no conflict between DMA alignment and properly offseting
the packet data by two bytes.

All hardware designers have to do is allow 2 padding bytes to be
emitted by the chip before the actual packet data.

Then the longword or whatever DMA transfer alignment is met
whilst still giving the necessary flexibility for where the
packet data lands.

Re: Alignment issues with freescale FEC driver

[David Miller]

From: Eric Nelson <eric@nelint.com>
Date: Fri, 23 Sep 2016 11:35:17 -0700

> From the i.MX6DQ reference manual, bit 7 of ENET_RACC says this:
> 
> "RX FIFO Shift-16
> 
> When this field is set, the actual frame data starts at bit 16 of the first
> word read from the RX FIFO aligning the Ethernet payload on a
> 32-bit boundary."
> 
> Same for the i.MX6UL.
> 
> I'm not sure what it will take to use this, but it seems to be exactly
> what we're looking for.

+1

Alignment issues with freescale FEC driver

[David Miller]

From: Eric Nelson <eric@nelint.com>
Date: Fri, 23 Sep 2016 11:35:17 -0700

> From the i.MX6DQ reference manual, bit 7 of ENET_RACC says this:
> 
> "RX FIFO Shift-16
> 
> When this field is set, the actual frame data starts at bit 16 of the first
> word read from the RX FIFO aligning the Ethernet payload on a
> 32-bit boundary."
> 
> Same for the i.MX6UL.
> 
> I'm not sure what it will take to use this, but it seems to be exactly
> what we're looking for.

+1

RE: Alignment issues with freescale FEC driver

[Andy Duan]

From: David Miller <davem@davemloft.net> Sent: Saturday, September 24, 2016 10:46 AM
> To: eric@nelint.com
> Cc: andrew@lunn.ch; edumazet@google.com; Andy Duan
> <fugang.duan@nxp.com>; otavio@ossystems.com.br;
> netdev@vger.kernel.org; troy.kisky@boundarydevices.com;
> rmk+kernel@arm.linux.org.uk; cjb.sw.nospam@gmail.com; linux-arm-
> kernel@lists.infradead.org
> Subject: Re: Alignment issues with freescale FEC driver
> 
> From: Eric Nelson <eric@nelint.com>
> Date: Fri, 23 Sep 2016 11:35:17 -0700
> 
> > From the i.MX6DQ reference manual, bit 7 of ENET_RACC says this:
> >
> > "RX FIFO Shift-16
> >
> > When this field is set, the actual frame data starts at bit 16 of the
> > first word read from the RX FIFO aligning the Ethernet payload on a
> > 32-bit boundary."
> >
> > Same for the i.MX6UL.
> >
> > I'm not sure what it will take to use this, but it seems to be exactly
> > what we're looking for.
> 
> +1

RACC[SHIFT16] just instructs the MAC to write two additional bytes in front of each frame received into the RX FIFO to align
the Ethernet payload on a 32-bit boundary.
Eric's patch "net: fec: support RRACC_SHIFT16 to align IP header" works fine.

For the alignment issues, that is introduced by commit 1b7bde6d6 and c259c132a in net-next tree. Before these commits, no alignment issue.

How to fix the issue:
Solution1:  to enable HW RRACC_SHIFT16 feature (test pass):
	Eric's patch  "net: fec: support RRACC_SHIFT16 to align IP header".
Solution2: include the correct prefetch() header (test pass):
	--- a/drivers/net/ethernet/freescale/fec_main.c
	+++ b/drivers/net/ethernet/freescale/fec_main.c
	@@ -59,7 +59,7 @@
	 #include <linux/pinctrl/consumer.h>
 	#include <linux/pm_runtime.h>
 	#include <linux/busfreq-imx.h>
	-#include <linux/prefetch.h>
	+#include <asm/processor.h>
Solution3: use __netdev_alloc_skb_ip_align() instead of netdev_alloc_skb(). 
	     Or: still use the previous method before commit 1b7bde6d6:
		skb = netdev_alloc_skb(ndev, pkt_len - 4 + NET_IP_ALIGN);
		skb_reserve(skb, NET_IP_ALIGN);

Comparing these solutions:
	From sw effort and performance, I think these are the similar.  Enable RRACC_SHIFT16 doesn't take extra advantage.

Correct my if I am wrong. Thanks.

Regards,
Andy

Alignment issues with freescale FEC driver

[Andy Duan]

From: David Miller <davem@davemloft.net> Sent: Saturday, September 24, 2016 10:46 AM
> To: eric at nelint.com
> Cc: andrew at lunn.ch; edumazet at google.com; Andy Duan
> <fugang.duan@nxp.com>; otavio at ossystems.com.br;
> netdev at vger.kernel.org; troy.kisky at boundarydevices.com;
> rmk+kernel at arm.linux.org.uk; cjb.sw.nospam at gmail.com; linux-arm-
> kernel at lists.infradead.org
> Subject: Re: Alignment issues with freescale FEC driver
> 
> From: Eric Nelson <eric@nelint.com>
> Date: Fri, 23 Sep 2016 11:35:17 -0700
> 
> > From the i.MX6DQ reference manual, bit 7 of ENET_RACC says this:
> >
> > "RX FIFO Shift-16
> >
> > When this field is set, the actual frame data starts at bit 16 of the
> > first word read from the RX FIFO aligning the Ethernet payload on a
> > 32-bit boundary."
> >
> > Same for the i.MX6UL.
> >
> > I'm not sure what it will take to use this, but it seems to be exactly
> > what we're looking for.
> 
> +1

RACC[SHIFT16] just instructs the MAC to write two additional bytes in front of each frame received into the RX FIFO to align
the Ethernet payload on a 32-bit boundary.
Eric's patch "net: fec: support RRACC_SHIFT16 to align IP header" works fine.

For the alignment issues, that is introduced by commit 1b7bde6d6 and c259c132a in net-next tree. Before these commits, no alignment issue.

How to fix the issue:
Solution1:  to enable HW RRACC_SHIFT16 feature (test pass):
	Eric's patch  "net: fec: support RRACC_SHIFT16 to align IP header".
Solution2: include the correct prefetch() header (test pass):
	--- a/drivers/net/ethernet/freescale/fec_main.c
	+++ b/drivers/net/ethernet/freescale/fec_main.c
	@@ -59,7 +59,7 @@
	 #include <linux/pinctrl/consumer.h>
 	#include <linux/pm_runtime.h>
 	#include <linux/busfreq-imx.h>
	-#include <linux/prefetch.h>
	+#include <asm/processor.h>
Solution3: use __netdev_alloc_skb_ip_align() instead of netdev_alloc_skb(). 
	     Or: still use the previous method before commit 1b7bde6d6:
		skb = netdev_alloc_skb(ndev, pkt_len - 4 + NET_IP_ALIGN);
		skb_reserve(skb, NET_IP_ALIGN);

Comparing these solutions:
	From sw effort and performance, I think these are the similar.  Enable RRACC_SHIFT16 doesn't take extra advantage.

Correct my if I am wrong. Thanks.

Regards,
Andy

Re: Alignment issues with freescale FEC driver

[Eric Nelson]

Hi David,

On 09/23/2016 07:43 PM, David Miller wrote:
> From: Eric Nelson <eric@nelint.com>
> Date: Fri, 23 Sep 2016 10:33:29 -0700
> 
>> Since the hardware requires longword alignment for its' DMA transfers,
>> aligning the IP header will require a memcpy, right?
> 
> I wish hardware designers didn't do this.
> 
> There is no conflict between DMA alignment and properly offseting
> the packet data by two bytes.
> 
> All hardware designers have to do is allow 2 padding bytes to be
> emitted by the chip before the actual packet data.
> 

Andrew Lunn pointed out that the hardware does support this,
and I just pushed a patch for the vendor kernel to the meta-freescale
mailing list:

https://lists.yoctoproject.org/pipermail/meta-freescale/2016-September/019228.html

> Then the longword or whatever DMA transfer alignment is met
> whilst still giving the necessary flexibility for where the
> packet data lands.
> 

Right. A relatively small change fixes things right up.

Many thanks to Andrew for pointing this out and Russell for providing
the basis for my patch.

I'll re-work this for the up-stream kernel when I get out from
under a couple of unrelated things.

Alignment issues with freescale FEC driver

[Eric Nelson]

Hi David,

On 09/23/2016 07:43 PM, David Miller wrote:
> From: Eric Nelson <eric@nelint.com>
> Date: Fri, 23 Sep 2016 10:33:29 -0700
> 
>> Since the hardware requires longword alignment for its' DMA transfers,
>> aligning the IP header will require a memcpy, right?
> 
> I wish hardware designers didn't do this.
> 
> There is no conflict between DMA alignment and properly offseting
> the packet data by two bytes.
> 
> All hardware designers have to do is allow 2 padding bytes to be
> emitted by the chip before the actual packet data.
> 

Andrew Lunn pointed out that the hardware does support this,
and I just pushed a patch for the vendor kernel to the meta-freescale
mailing list:

https://lists.yoctoproject.org/pipermail/meta-freescale/2016-September/019228.html

> Then the longword or whatever DMA transfer alignment is met
> whilst still giving the necessary flexibility for where the
> packet data lands.
> 

Right. A relatively small change fixes things right up.

Many thanks to Andrew for pointing this out and Russell for providing
the basis for my patch.

I'll re-work this for the up-stream kernel when I get out from
under a couple of unrelated things.

[PATCH 0/3] net: fec: updates to align IP header

[Eric Nelson]

This patch series is the outcome of investigation into very high
numbers of alignment faults on kernel 4.1.33 from the linux-fslc
tree:
    https://github.com/freescale/linux-fslc/tree/4.1-1.0.x-imx

The first two patches remove support for the receive accelerator (RACC) from
the i.MX25 and i.MX27 SoCs which don't support the function.

The third patch enables hardware alignment of the ethernet packet payload
(and especially the IP header) to prevent alignment faults in the IP stack.

Testing on i.MX6UL on the 4.1.33 kernel showed that this patch removed
on the order of 70k alignment faults during a 100MiB transfer using 
wget.

Testing on an i.MX6Q (SABRE Lite) board on net-next (4.8.0-rc7) showed
a much more modest improvement from 10's of faults, and it's not clear
why that's the case.

Eric Nelson (3):
  net: fec: remove QUIRK_HAS_RACC from i.mx25
  net: fec: remove QUIRK_HAS_RACC from i.mx27
  net: fec: align IP header in hardware

 drivers/net/ethernet/freescale/fec_main.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

-- 
2.7.4

[PATCH 1/3] net: fec: remove QUIRK_HAS_RACC from i.mx25

[Eric Nelson]

According to the i.MX25 reference manual, this SoC does not have support
for the receive accelerator (RACC) register at offset 0x1C4.

http://www.nxp.com/files/dsp/doc/ref_manual/IMX25RM.pdf

Signed-off-by: Eric Nelson <eric@nelint.com>
---
 drivers/net/ethernet/freescale/fec_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index fb5c638..d193406 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -89,7 +89,7 @@ static struct platform_device_id fec_devtype[] = {
 		.driver_data = 0,
 	}, {
 		.name = "imx25-fec",
-		.driver_data = FEC_QUIRK_USE_GASKET | FEC_QUIRK_HAS_RACC,
+		.driver_data = FEC_QUIRK_USE_GASKET,
 	}, {
 		.name = "imx27-fec",
 		.driver_data = FEC_QUIRK_HAS_RACC,
-- 
2.7.4

[PATCH 2/3] net: fec: remove QUIRK_HAS_RACC from i.mx27

[Eric Nelson]

According to the i.MX27 reference manual, this SoC does not have support
for the receive accelerator (RACC) register at offset 0x1C4.

	http://cache.nxp.com/files/32bit/doc/ref_manual/MCIMX27RM.pdf

Signed-off-by: Eric Nelson <eric@nelint.com>
---
 drivers/net/ethernet/freescale/fec_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index d193406..0219e79 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -92,7 +92,7 @@ static struct platform_device_id fec_devtype[] = {
 		.driver_data = FEC_QUIRK_USE_GASKET,
 	}, {
 		.name = "imx27-fec",
-		.driver_data = FEC_QUIRK_HAS_RACC,
+		.driver_data = 0,
 	}, {
 		.name = "imx28-fec",
 		.driver_data = FEC_QUIRK_ENET_MAC | FEC_QUIRK_SWAP_FRAME |
-- 
2.7.4

[PATCH 3/3] net: fec: align IP header in hardware

[Eric Nelson]

The FEC receive accelerator (RACC) supports shifting the data payload of
received packets by 16-bits, which aligns the payload (IP header) on a
4-byte boundary, which is, if not required, at least strongly suggested
by the Linux networking layer.

Without this patch, a huge number of alignment faults will be taken by the
IP stack, as seen in /proc/cpu/alignment:

	~/$ cat /proc/cpu/alignment
	User:		0
	System:		72645 (inet_gro_receive+0x104/0x27c)
	Skipped:	0
	Half:		0
	Word:		0
	DWord:		0
	Multi:		72645
	User faults:	3 (fixup+warn)

This patch was suggested by Andrew Lunn in this message to linux-netdev:
	http://marc.info/?l=linux-arm-kernel&m=147465452108384&w=2

and adapted from a patch by Russell King from 2014:
	http://git.arm.linux.org.uk/cgit/linux-arm.git/commit/?id=70d8a8a

Signed-off-by: Eric Nelson <eric@nelint.com>
---
 drivers/net/ethernet/freescale/fec_main.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 0219e79..1fa2d87 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -180,6 +180,7 @@ MODULE_PARM_DESC(macaddr, "FEC Ethernet MAC address");
 /* FEC receive acceleration */
 #define FEC_RACC_IPDIS		(1 << 1)
 #define FEC_RACC_PRODIS		(1 << 2)
+#define FEC_RACC_SHIFT16	BIT(7)
 #define FEC_RACC_OPTIONS	(FEC_RACC_IPDIS | FEC_RACC_PRODIS)
 
 /*
@@ -945,9 +946,11 @@ fec_restart(struct net_device *ndev)
 
 #if !defined(CONFIG_M5272)
 	if (fep->quirks & FEC_QUIRK_HAS_RACC) {
-		/* set RX checksum */
 		val = readl(fep->hwp + FEC_RACC);
+		/* align IP header */
+		val |= FEC_RACC_SHIFT16;
 		if (fep->csum_flags & FLAG_RX_CSUM_ENABLED)
+			/* set RX checksum */
 			val |= FEC_RACC_OPTIONS;
 		else
 			val &= ~FEC_RACC_OPTIONS;
@@ -1428,6 +1431,12 @@ fec_enet_rx_queue(struct net_device *ndev, int budget, u16 queue_id)
 		prefetch(skb->data - NET_IP_ALIGN);
 		skb_put(skb, pkt_len - 4);
 		data = skb->data;
+
+#if !defined(CONFIG_M5272)
+		if (fep->quirks & FEC_QUIRK_HAS_RACC)
+			data = skb_pull_inline(skb, 2);
+#endif
+
 		if (!is_copybreak && need_swap)
 			swap_buffer(data, pkt_len);
 
-- 
2.7.4

RE: [PATCH 0/3] net: fec: updates to align IP header

[Andy Duan]

From: Eric Nelson <eric@nelint.com> Sent: Saturday, September 24, 2016 10:42 PM
> To: netdev@vger.kernel.org
> Cc: linux@arm.linux.org.uk; andrew@lunn.ch; Andy Duan
> <fugang.duan@nxp.com>; otavio@ossystems.com.br;
> edumazet@google.com; troy.kisky@boundarydevices.com;
> davem@davemloft.net; u.kleine-koenig@pengutronix.de; Eric Nelson
> <eric@nelint.com>
> Subject: [PATCH 0/3] net: fec: updates to align IP header
> 
> This patch series is the outcome of investigation into very high numbers of
> alignment faults on kernel 4.1.33 from the linux-fslc
> tree:
>     https://github.com/freescale/linux-fslc/tree/4.1-1.0.x-imx
> 
> The first two patches remove support for the receive accelerator (RACC)
> from the i.MX25 and i.MX27 SoCs which don't support the function.
> 
> The third patch enables hardware alignment of the ethernet packet payload
> (and especially the IP header) to prevent alignment faults in the IP stack.
> 
> Testing on i.MX6UL on the 4.1.33 kernel showed that this patch removed on
> the order of 70k alignment faults during a 100MiB transfer using wget.
> 
> Testing on an i.MX6Q (SABRE Lite) board on net-next (4.8.0-rc7) showed a
> much more modest improvement from 10's of faults, and it's not clear why
> that's the case.
> 
> Eric Nelson (3):
>   net: fec: remove QUIRK_HAS_RACC from i.mx25
>   net: fec: remove QUIRK_HAS_RACC from i.mx27
>   net: fec: align IP header in hardware
> 
>  drivers/net/ethernet/freescale/fec_main.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> --
> 2.7.4
I will investigate the diff between 4.1 and 4.8. Thanks.

Acked-by: Fugang Duan <fugang.duan@nxp.com>

Re: [PATCH 0/3] net: fec: updates to align IP header

[Eric Nelson]

On 09/24/2016 08:09 AM, Andy Duan wrote:
> From: Eric Nelson <eric@nelint.com> Sent: Saturday, September 24, 2016 10:42 PM
>> To: netdev@vger.kernel.org
>> Cc: linux@arm.linux.org.uk; andrew@lunn.ch; Andy Duan
>> <fugang.duan@nxp.com>; otavio@ossystems.com.br;
>> edumazet@google.com; troy.kisky@boundarydevices.com;
>> davem@davemloft.net; u.kleine-koenig@pengutronix.de; Eric Nelson
>> <eric@nelint.com>
>> Subject: [PATCH 0/3] net: fec: updates to align IP header
>>
>> This patch series is the outcome of investigation into very high numbers of
>> alignment faults on kernel 4.1.33 from the linux-fslc
>> tree:
>>     https://github.com/freescale/linux-fslc/tree/4.1-1.0.x-imx
>>
>> The first two patches remove support for the receive accelerator (RACC)
>> from the i.MX25 and i.MX27 SoCs which don't support the function.
>>
>> The third patch enables hardware alignment of the ethernet packet payload
>> (and especially the IP header) to prevent alignment faults in the IP stack.
>>
>> Testing on i.MX6UL on the 4.1.33 kernel showed that this patch removed on
>> the order of 70k alignment faults during a 100MiB transfer using wget.
>>
>> Testing on an i.MX6Q (SABRE Lite) board on net-next (4.8.0-rc7) showed a
>> much more modest improvement from 10's of faults, and it's not clear why
>> that's the case.
>>
>> Eric Nelson (3):
>>   net: fec: remove QUIRK_HAS_RACC from i.mx25
>>   net: fec: remove QUIRK_HAS_RACC from i.mx27
>>   net: fec: align IP header in hardware
>>
>>  drivers/net/ethernet/freescale/fec_main.c | 15 ++++++++++++---
>>  1 file changed, 12 insertions(+), 3 deletions(-)
>>
>> --
>> 2.7.4
> I will investigate the diff between 4.1 and 4.8. Thanks.
> 

Thanks. Note that I'm not sure if the difference is 4.1 vs. 4.8 or
i.MX6UL vs. i.MX6Q.

> Acked-by: Fugang Duan <fugang.duan@nxp.com>
> 

RE: [PATCH 3/3] net: fec: align IP header in hardware

[David Laight]

From: Eric Nelson
> Sent: 24 September 2016 15:42
> The FEC receive accelerator (RACC) supports shifting the data payload of
> received packets by 16-bits, which aligns the payload (IP header) on a
> 4-byte boundary, which is, if not required, at least strongly suggested
> by the Linux networking layer.
...
> +		/* align IP header */
> +		val |= FEC_RACC_SHIFT16;

I can't help feeling that there needs to be corresponding
changes to increase the buffer size by 2 (maybe for large mtu)
and to discard two bytes from the frame length.

If probably ought to be predicated on NET_IP_ALIGN as well.

	David

Re: [PATCH 3/3] net: fec: align IP header in hardware

[Eric Nelson]

Hi David,

On 09/26/2016 02:26 AM, David Laight wrote:
> From: Eric Nelson
>> Sent: 24 September 2016 15:42
>> The FEC receive accelerator (RACC) supports shifting the data payload of
>> received packets by 16-bits, which aligns the payload (IP header) on a
>> 4-byte boundary, which is, if not required, at least strongly suggested
>> by the Linux networking layer.
> ...
>> +		/* align IP header */
>> +		val |= FEC_RACC_SHIFT16;
> 
> I can't help feeling that there needs to be corresponding
> changes to increase the buffer size by 2 (maybe for large mtu)
> and to discard two bytes from the frame length.
> 

In the normal case, the fec driver over-allocates all receive packets to
be of size FEC_ENET_RX_FRSIZE (2048) minus the value of rx_align,
which is either 0x0f (ARM) or 0x03 (PPC).

If the frame length is less than rx_copybreak (typically 256), then
the frame length from the receive buffer descriptor is used to
control the allocation size for a copied buffer, and this will include
the two bytes of padding if RACC_SHIFT16 is set.

> If probably ought to be predicated on NET_IP_ALIGN as well.
> 
Can you elaborate?

Please advise,


Eric

Re: [PATCH 0/3] net: fec: updates to align IP header

[David Miller]

From: Eric Nelson <eric@nelint.com>
Date: Sat, 24 Sep 2016 07:42:16 -0700

> This patch series is the outcome of investigation into very high
> numbers of alignment faults on kernel 4.1.33 from the linux-fslc
> tree:
>     https://github.com/freescale/linux-fslc/tree/4.1-1.0.x-imx
> 
> The first two patches remove support for the receive accelerator (RACC) from
> the i.MX25 and i.MX27 SoCs which don't support the function.
> 
> The third patch enables hardware alignment of the ethernet packet payload
> (and especially the IP header) to prevent alignment faults in the IP stack.
> 
> Testing on i.MX6UL on the 4.1.33 kernel showed that this patch removed
> on the order of 70k alignment faults during a 100MiB transfer using 
> wget.
> 
> Testing on an i.MX6Q (SABRE Lite) board on net-next (4.8.0-rc7) showed
> a much more modest improvement from 10's of faults, and it's not clear
> why that's the case.

Series applied and queued up for -stable.

RE: [PATCH 3/3] net: fec: align IP header in hardware

[David Laight]

From: Eric Nelson
> Sent: 26 September 2016 19:40
> Hi David,
> 
> On 09/26/2016 02:26 AM, David Laight wrote:
> > From: Eric Nelson
> >> Sent: 24 September 2016 15:42
> >> The FEC receive accelerator (RACC) supports shifting the data payload of
> >> received packets by 16-bits, which aligns the payload (IP header) on a
> >> 4-byte boundary, which is, if not required, at least strongly suggested
> >> by the Linux networking layer.
> > ...
> >> +		/* align IP header */
> >> +		val |= FEC_RACC_SHIFT16;
> >
> > I can't help feeling that there needs to be corresponding
> > changes to increase the buffer size by 2 (maybe for large mtu)
> > and to discard two bytes from the frame length.
> >
> 
> In the normal case, the fec driver over-allocates all receive packets to
> be of size FEC_ENET_RX_FRSIZE (2048) minus the value of rx_align,
> which is either 0x0f (ARM) or 0x03 (PPC).
> 
> If the frame length is less than rx_copybreak (typically 256), then
> the frame length from the receive buffer descriptor is used to
> control the allocation size for a copied buffer, and this will include
> the two bytes of padding if RACC_SHIFT16 is set.
> 
> > If probably ought to be predicated on NET_IP_ALIGN as well.
> >
> Can you elaborate?

>From reading this it seems that the effect of FEC_RACC_SHIFT16 is to
add two bytes of 'junk' to the start of every receive frame.

In the 'copybreak' case the new skb would need to be 2 bytes shorter
than the length reported by the hardware, and the data copied from
2 bytes into the dma buffer.

The extra 2 bytes also mean the that maximum mtu that can be received
into a buffer is two bytes less.
If someone sets the mtu to (say) 9k for jumbo frames this might matter.
Even with fixed 2048 byte buffers it reduces the maximum value the mtu
can be set to by 2.

Now if NET_IP_ALIGN is zero then it is fine for the rx frame to start
on a 4n boundary, and the skb are likely to be allocated that way.
In this case you don't want to extra two bytes of 'junk'.

OTOH if NET_IP_ALIGN is 2 then you need to 'fiddle' things so that
the data is dma'd to offset -2 in the skb and then ensure that the
end of frame is set correctly.

	David

Re: [PATCH 3/3] net: fec: align IP header in hardware

[Eric Nelson]

Thanks David,

On 09/28/2016 09:42 AM, David Laight wrote:
> From: Eric Nelson
>> Sent: 26 September 2016 19:40
>> Hi David,
>>
>> On 09/26/2016 02:26 AM, David Laight wrote:
>>> From: Eric Nelson
>>>> Sent: 24 September 2016 15:42
>>>> The FEC receive accelerator (RACC) supports shifting the data payload of
>>>> received packets by 16-bits, which aligns the payload (IP header) on a
>>>> 4-byte boundary, which is, if not required, at least strongly suggested
>>>> by the Linux networking layer.
>>> ...
>>>> +		/* align IP header */
>>>> +		val |= FEC_RACC_SHIFT16;
>>>
>>> I can't help feeling that there needs to be corresponding
>>> changes to increase the buffer size by 2 (maybe for large mtu)
>>> and to discard two bytes from the frame length.
>>>
>>
>> In the normal case, the fec driver over-allocates all receive packets to
>> be of size FEC_ENET_RX_FRSIZE (2048) minus the value of rx_align,
>> which is either 0x0f (ARM) or 0x03 (PPC).
>>
>> If the frame length is less than rx_copybreak (typically 256), then
>> the frame length from the receive buffer descriptor is used to
>> control the allocation size for a copied buffer, and this will include
>> the two bytes of padding if RACC_SHIFT16 is set.
>>
>>> If probably ought to be predicated on NET_IP_ALIGN as well.
>>>
>> Can you elaborate?
> 
> From reading this it seems that the effect of FEC_RACC_SHIFT16 is to
> add two bytes of 'junk' to the start of every receive frame.
> 

That's right. Two bytes of junk between the MAC header and the
IP header.

> In the 'copybreak' case the new skb would need to be 2 bytes shorter
> than the length reported by the hardware, and the data copied from
> 2 bytes into the dma buffer.
> 

As it stands, the skb allocated by the copybreak routine will include
the two bytes of padding, and the call to skb_pull_inline will ignore
them.

> The extra 2 bytes also mean the that maximum mtu that can be received
> into a buffer is two bytes less.
>

Right, but I think the max is already high enough that this isn't a
problem.

> If someone sets the mtu to (say) 9k for jumbo frames this might matter.
> Even with fixed 2048 byte buffers it reduces the maximum value the mtu
> can be set to by 2.
> 

As far as I can tell, the fec driver doesn't support jumbo frames, and
the max frame length is currently hard-coded at PKT_MAXBUF_SIZE (1522).

This is well within the 2048-byte allocation, even with optional headers
for VLAN etc.

> Now if NET_IP_ALIGN is zero then it is fine for the rx frame to start
> on a 4n boundary, and the skb are likely to be allocated that way.
> In this case you don't want to extra two bytes of 'junk'.
> 
NET_IP_ALIGN is defaulting to 2 by the conditional in skbuff.h

> OTOH if NET_IP_ALIGN is 2 then you need to 'fiddle' things so that
> the data is dma'd to offset -2 in the skb and then ensure that the
> end of frame is set correctly.
> 

That's what the RACC SHIFT16 bit does.

The FEC hardware isn't capable of DMA'ing to an un-aligned address.
On ARM, it requires 64-bit alignment, but suggests 128-bit alignment.

On other (PPC?) architectures, it requires 32-bit alignment. This is
handled by the rx_align field.

Regards,


Eric

Re: [PATCH 3/3] net: fec: align IP header in hardware

[Russell King - ARM Linux]

On Wed, Sep 28, 2016 at 10:14:52AM -0700, Eric Nelson wrote:
> Thanks David,
> 
> On 09/28/2016 09:42 AM, David Laight wrote:
> > From reading this it seems that the effect of FEC_RACC_SHIFT16 is to
> > add two bytes of 'junk' to the start of every receive frame.
> 
> That's right. Two bytes of junk between the MAC header and the
> IP header.

That's wrong.  FEC_RACC_SHIFT16 adds two bytes to the _beginning_ of
the packet, not in the middle of the packet:

   7      RX FIFO Shift-16
 SHIFT16
          When this field is set, the actual frame data starts at bit 16
          of the first word read from the RX FIFO aligning the Ethernet
          payload on a 32-bit boundary.

          NOTE: This function only affects the FIFO storage and has no
                influence on the statistics, which use the actual length
                of the frame received.

          0    Disabled.
          1    Instructs the MAC to write two additional bytes in front
               of each frame received into the RX FIFO.

*in front* of the frame - that's before the Ethernet header.  Not between
the ethernet and IP headers.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 3/3] net: fec: align IP header in hardware

[Eric Nelson]

Thanks Russell,

On 09/28/2016 10:25 AM, Russell King - ARM Linux wrote:
> On Wed, Sep 28, 2016 at 10:14:52AM -0700, Eric Nelson wrote:
>> Thanks David,
>>
>> On 09/28/2016 09:42 AM, David Laight wrote:
>>> From reading this it seems that the effect of FEC_RACC_SHIFT16 is to
>>> add two bytes of 'junk' to the start of every receive frame.
>>
>> That's right. Two bytes of junk between the MAC header and the
>> IP header.
> 
> That's wrong.  FEC_RACC_SHIFT16 adds two bytes to the _beginning_ of
> the packet, not in the middle of the packet:
> 
>    7      RX FIFO Shift-16
>  SHIFT16
>           When this field is set, the actual frame data starts at bit 16
>           of the first word read from the RX FIFO aligning the Ethernet
>           payload on a 32-bit boundary.
> 
>           NOTE: This function only affects the FIFO storage and has no
>                 influence on the statistics, which use the actual length
>                 of the frame received.
> 
>           0    Disabled.
>           1    Instructs the MAC to write two additional bytes in front
>                of each frame received into the RX FIFO.
> 
> *in front* of the frame - that's before the Ethernet header.  Not between
> the ethernet and IP headers.
> 

I obviously mis-read this, and haven't dumped any packets to straighten
myself out.

RE: [PATCH 3/3] net: fec: align IP header in hardware

[David Laight]

From: Eric Nelson
> Sent: 28 September 2016 18:15
> On 09/28/2016 09:42 AM, David Laight wrote:
> > From: Eric Nelson
> >> Sent: 26 September 2016 19:40
> >> Hi David,
> >>
> >> On 09/26/2016 02:26 AM, David Laight wrote:
> >>> From: Eric Nelson
> >>>> Sent: 24 September 2016 15:42
> >>>> The FEC receive accelerator (RACC) supports shifting the data payload of
> >>>> received packets by 16-bits, which aligns the payload (IP header) on a
> >>>> 4-byte boundary, which is, if not required, at least strongly suggested
> >>>> by the Linux networking layer.
> >>> ...
> >>>> +		/* align IP header */
> >>>> +		val |= FEC_RACC_SHIFT16;
> >>>
> >>> I can't help feeling that there needs to be corresponding
> >>> changes to increase the buffer size by 2 (maybe for large mtu)
> >>> and to discard two bytes from the frame length.
> >>>
> >>
> >> In the normal case, the fec driver over-allocates all receive packets to
> >> be of size FEC_ENET_RX_FRSIZE (2048) minus the value of rx_align,
> >> which is either 0x0f (ARM) or 0x03 (PPC).
> >>
> >> If the frame length is less than rx_copybreak (typically 256), then
> >> the frame length from the receive buffer descriptor is used to
> >> control the allocation size for a copied buffer, and this will include
> >> the two bytes of padding if RACC_SHIFT16 is set.
> >>
> >>> If probably ought to be predicated on NET_IP_ALIGN as well.
> >>>
> >> Can you elaborate?
> >
> > From reading this it seems that the effect of FEC_RACC_SHIFT16 is to
> > add two bytes of 'junk' to the start of every receive frame.
> >
> 
> That's right. Two bytes of junk between the MAC header and the
> IP header.
> 
> > In the 'copybreak' case the new skb would need to be 2 bytes shorter
> > than the length reported by the hardware, and the data copied from
> > 2 bytes into the dma buffer.
> >
> 
> As it stands, the skb allocated by the copybreak routine will include
> the two bytes of padding, and the call to skb_pull_inline will ignore
> them.

Ok, I didn't see that call being added by this patch.

> > The extra 2 bytes also mean the that maximum mtu that can be received
> > into a buffer is two bytes less.
> >
> 
> Right, but I think the max is already high enough that this isn't a
> problem.
> 
> > If someone sets the mtu to (say) 9k for jumbo frames this might matter.
> > Even with fixed 2048 byte buffers it reduces the maximum value the mtu
> > can be set to by 2.
> >
> 
> As far as I can tell, the fec driver doesn't support jumbo frames, and
> the max frame length is currently hard-coded at PKT_MAXBUF_SIZE (1522).
> 
> This is well within the 2048-byte allocation, even with optional headers
> for VLAN etc.

Hmm...
That (probably) means all the skb the driver allocates are actually 4k.
It would be much better to reduce the size so that the entire skb
(with packet buffer) is less than 2k.

> > Now if NET_IP_ALIGN is zero then it is fine for the rx frame to start
> > on a 4n boundary, and the skb are likely to be allocated that way.
> > In this case you don't want to extra two bytes of 'junk'.
> >
> NET_IP_ALIGN is defaulting to 2 by the conditional in skbuff.h

Even though it is always currently set is isn't really ideal to have
a driver that breaks if it isn't set.
This could easily happen at some point in the future if the ethernet
logic is put with a different cpu.


> > OTOH if NET_IP_ALIGN is 2 then you need to 'fiddle' things so that
> > the data is dma'd to offset -2 in the skb and then ensure that the
> > end of frame is set correctly.
> >
> 
> That's what the RACC SHIFT16 bit does.

No, that causes the ethernet controller to add 2 bytes to the frame.
You then need to change the dma target address to match.
Otherwise if a new version of the silicon stops ignoring the low
address with the frame will be misaligned in the buffer.

The receive frame length will also (probably) be 2 larger than the
actual frame - so you need to set the end point correctly as well.
IP will probably ignore the 2 bytes of pad I think you are generating.

> The FEC hardware isn't capable of DMA'ing to an un-aligned address.
> On ARM, it requires 64-bit alignment, but suggests 128-bit alignment.
> 
> On other (PPC?) architectures, it requires 32-bit alignment. This is
> handled by the rx_align field.

That isn't entirely relevant.
If the kernel is being built with NET_IP_ALIGN set to 0 you should
align the destination mac address on a 4n boundary
(Or rather the skb are likely to be allocated that way).
If it causes misaligned memory reads later on that is a different problem.
The MAC driver has aligned the frames as it was told to.

	David

Re: [PATCH 3/3] net: fec: align IP header in hardware

[Eric Nelson]

Thanks for the feedback David,

On 09/29/2016 04:07 AM, David Laight wrote:
> From: Eric Nelson
>> Sent: 28 September 2016 18:15
>> On 09/28/2016 09:42 AM, David Laight wrote:
>>> From: Eric Nelson
>>>> Sent: 26 September 2016 19:40
>>>> Hi David,
>>>>
>>>> On 09/26/2016 02:26 AM, David Laight wrote:
>>>>> From: Eric Nelson
>>>>>> Sent: 24 September 2016 15:42
>>>>>> The FEC receive accelerator (RACC) supports shifting the data payload of
>>>>>> received packets by 16-bits, which aligns the payload (IP header) on a
>>>>>> 4-byte boundary, which is, if not required, at least strongly suggested
>>>>>> by the Linux networking layer.
>>>>> ...
>>>>>> +		/* align IP header */
>>>>>> +		val |= FEC_RACC_SHIFT16;
>>>>>
>>>>> I can't help feeling that there needs to be corresponding
>>>>> changes to increase the buffer size by 2 (maybe for large mtu)
>>>>> and to discard two bytes from the frame length.
>>>>>
>>>>
>>>> In the normal case, the fec driver over-allocates all receive packets to
>>>> be of size FEC_ENET_RX_FRSIZE (2048) minus the value of rx_align,
>>>> which is either 0x0f (ARM) or 0x03 (PPC).
>>>>
>>>> If the frame length is less than rx_copybreak (typically 256), then
>>>> the frame length from the receive buffer descriptor is used to
>>>> control the allocation size for a copied buffer, and this will include
>>>> the two bytes of padding if RACC_SHIFT16 is set.
>>>>
>>>>> If probably ought to be predicated on NET_IP_ALIGN as well.
>>>>>
>>>> Can you elaborate?
>>>
>>> From reading this it seems that the effect of FEC_RACC_SHIFT16 is to
>>> add two bytes of 'junk' to the start of every receive frame.
>>>
>>
>> That's right. Two bytes of junk between the MAC header and the
>> IP header.
>>
>>> In the 'copybreak' case the new skb would need to be 2 bytes shorter
>>> than the length reported by the hardware, and the data copied from
>>> 2 bytes into the dma buffer.
>>>
>>
>> As it stands, the skb allocated by the copybreak routine will include
>> the two bytes of padding, and the call to skb_pull_inline will ignore
>> them.
> 
> Ok, I didn't see that call being added by this patch.
> 
>>> The extra 2 bytes also mean the that maximum mtu that can be received
>>> into a buffer is two bytes less.
>>>
>>
>> Right, but I think the max is already high enough that this isn't a
>> problem.
>>
>>> If someone sets the mtu to (say) 9k for jumbo frames this might matter.
>>> Even with fixed 2048 byte buffers it reduces the maximum value the mtu
>>> can be set to by 2.
>>>
>>
>> As far as I can tell, the fec driver doesn't support jumbo frames, and
>> the max frame length is currently hard-coded at PKT_MAXBUF_SIZE (1522).
>>
>> This is well within the 2048-byte allocation, even with optional headers
>> for VLAN etc.
> 
> Hmm...
>
> That (probably) means all the skb the driver allocates are actually 4k.
> It would be much better to reduce the size so that the entire skb
> (with packet buffer) is less than 2k.
>

That seems worthwhile, but un-related to this patch.

It appears to me that the received packets could be allocated as

PKT_MAXBUF_SIZE+fep->rx_align+NET_IP_ALIGN

(+2 if FEC_RACC_SHIFT16 is used)

>>> Now if NET_IP_ALIGN is zero then it is fine for the rx frame to start
>>> on a 4n boundary, and the skb are likely to be allocated that way.
>>> In this case you don't want to extra two bytes of 'junk'.
>>>
>> NET_IP_ALIGN is defaulting to 2 by the conditional in skbuff.h
> 
> Even though it is always currently set is isn't really ideal to have
> a driver that breaks if it isn't set.
> This could easily happen at some point in the future if the ethernet
> logic is put with a different cpu.
> 

After multiple reads, I'm confused about the meaning of NET_IP_ALIGN
and how it should be used.

>From Documentation/unaligned-memory-access.txt, I take it that this
should be configured on a per-architecture basis, and it seems to be
set to zero on both PPC and x86.

I wonder if this is proper though. It seems that its' use might depend
on the I/O subsystem(s) in use as much as the architecture.

For example, it might be desirable to have a different value for a PCIe
interface than for an integrated MAC like the FEC.

Looking at the example of the 3c59x driver, I see a pattern of an
allocation that adds NET_IP_ALIGN followed by an skb->reserve()
of NET_IP_ALIGN before determining the target address to end
up with allocation with 4n+2 alignment.

This seems somewhat equivalent to this patch, except that we're
using the allocated address as the target and using skb_pull_inline
afterwards.

Andy, is the FEC used on any PPC SOCs?

If so, then this patch may cause a DMA of 2 extra bytes per frame
unecessarily although the driver doesn't special-case the allocation
to align the IP header, so this is still probably preferred.

>>> OTOH if NET_IP_ALIGN is 2 then you need to 'fiddle' things so that
>>> the data is dma'd to offset -2 in the skb and then ensure that the
>>> end of frame is set correctly.
>>>
>>
>> That's what the RACC SHIFT16 bit does.
> 
> No, that causes the ethernet controller to add 2 bytes to the frame.
> You then need to change the dma target address to match.
>

Or use skb_pull_inline to ignore the two bytes.

> Otherwise if a new version of the silicon stops ignoring the low
> address with the frame will be misaligned in the buffer.
> 

I'm not sure I understand this.

> The receive frame length will also (probably) be 2 larger than the
> actual frame - so you need to set the end point correctly as well.
> IP will probably ignore the 2 bytes of pad I think you are generating.
> 

The received frame length **is** 2 bytes longer, but these are
eaten by skb_pull_inline().

>> The FEC hardware isn't capable of DMA'ing to an un-aligned address.
>> On ARM, it requires 64-bit alignment, but suggests 128-bit alignment.
>>
>> On other (PPC?) architectures, it requires 32-bit alignment. This is
>> handled by the rx_align field.
> 
> That isn't entirely relevant.
>
> If the kernel is being built with NET_IP_ALIGN set to 0 you should
> align the destination mac address on a 4n boundary
> (Or rather the skb are likely to be allocated that way).

They're not currently allocated that way. The routine
fec_enet_alloc_rxq_buffers
forces the allocations to 32 or 128-bit alignment through the
routine fec_enet_new_rxbdp().

> If it causes misaligned memory reads later on that is a different problem.

That's the problem this patch is designed to address. Without this
patch, the IP header is always mis-aligned.

> The MAC driver has aligned the frames as it was told to.
> 
> 	David
> 
> 

Regards,


Eric

RE: [PATCH 3/3] net: fec: align IP header in hardware

[David Laight]

From: Eric Nelson
> Sent: 30 September 2016 14:27
> Thanks for the feedback David,
> 
> On 09/29/2016 04:07 AM, David Laight wrote:
> > From: Eric Nelson
> >> Sent: 28 September 2016 18:15
> >> On 09/28/2016 09:42 AM, David Laight wrote:
> >>> From: Eric Nelson
> >>>> Sent: 26 September 2016 19:40
> >>>> Hi David,
> >>>>
> >>>> On 09/26/2016 02:26 AM, David Laight wrote:
> >>>>> From: Eric Nelson
> >>>>>> Sent: 24 September 2016 15:42
> >>>>>> The FEC receive accelerator (RACC) supports shifting the data payload of
> >>>>>> received packets by 16-bits, which aligns the payload (IP header) on a
> >>>>>> 4-byte boundary, which is, if not required, at least strongly suggested
> >>>>>> by the Linux networking layer.
> >>>>> ...
> >>>>>> +		/* align IP header */
> >>>>>> +		val |= FEC_RACC_SHIFT16;
> >>>>>
> >>>>> I can't help feeling that there needs to be corresponding
> >>>>> changes to increase the buffer size by 2 (maybe for large mtu)
> >>>>> and to discard two bytes from the frame length.
> >>>>>
> >>>>
> >>>> In the normal case, the fec driver over-allocates all receive packets to
> >>>> be of size FEC_ENET_RX_FRSIZE (2048) minus the value of rx_align,
> >>>> which is either 0x0f (ARM) or 0x03 (PPC).
> >>>>
> >>>> If the frame length is less than rx_copybreak (typically 256), then
> >>>> the frame length from the receive buffer descriptor is used to
> >>>> control the allocation size for a copied buffer, and this will include
> >>>> the two bytes of padding if RACC_SHIFT16 is set.
> >>>>
> >>>>> If probably ought to be predicated on NET_IP_ALIGN as well.
> >>>>>
> >>>> Can you elaborate?
> >>>
> >>> From reading this it seems that the effect of FEC_RACC_SHIFT16 is to
> >>> add two bytes of 'junk' to the start of every receive frame.
> >>>
> >>
> >> That's right. Two bytes of junk between the MAC header and the
> >> IP header.
> >>
> >>> In the 'copybreak' case the new skb would need to be 2 bytes shorter
> >>> than the length reported by the hardware, and the data copied from
> >>> 2 bytes into the dma buffer.
> >>>
> >>
> >> As it stands, the skb allocated by the copybreak routine will include
> >> the two bytes of padding, and the call to skb_pull_inline will ignore
> >> them.
> >
> > Ok, I didn't see that call being added by this patch.
> >
> >>> The extra 2 bytes also mean the that maximum mtu that can be received
> >>> into a buffer is two bytes less.
> >>>
> >>
> >> Right, but I think the max is already high enough that this isn't a
> >> problem.
> >>
> >>> If someone sets the mtu to (say) 9k for jumbo frames this might matter.
> >>> Even with fixed 2048 byte buffers it reduces the maximum value the mtu
> >>> can be set to by 2.
> >>>
> >>
> >> As far as I can tell, the fec driver doesn't support jumbo frames, and
> >> the max frame length is currently hard-coded at PKT_MAXBUF_SIZE (1522).
> >>
> >> This is well within the 2048-byte allocation, even with optional headers
> >> for VLAN etc.
> >
> > Hmm...
> >
> > That (probably) means all the skb the driver allocates are actually 4k.
> > It would be much better to reduce the size so that the entire skb
> > (with packet buffer) is less than 2k.
> >
> 
> That seems worthwhile, but un-related to this patch.

Indeed.

> It appears to me that the received packets could be allocated as
> 
> PKT_MAXBUF_SIZE+fep->rx_align+NET_IP_ALIGN
> 
> (+2 if FEC_RACC_SHIFT16 is used)

No.
The packet buffers need to be allocated NET_IP_ALIGN + PKT_MAXBUF_SIZE
byte long and (I assume) aligned on a fep->rx_align byte boundary.

If NET_IP_ALIGN is set (to 2) then FEC_RACC_SHIFT16 must also me set
so that the ethernet frame itself is 4n+2 aligned.

> >>> Now if NET_IP_ALIGN is zero then it is fine for the rx frame to start
> >>> on a 4n boundary, and the skb are likely to be allocated that way.
> >>> In this case you don't want to extra two bytes of 'junk'.
> >>>
> >> NET_IP_ALIGN is defaulting to 2 by the conditional in skbuff.h
> >
> > Even though it is always currently set is isn't really ideal to have
> > a driver that breaks if it isn't set.
> > This could easily happen at some point in the future if the ethernet
> > logic is put with a different cpu.
> >
> 
> After multiple reads, I'm confused about the meaning of NET_IP_ALIGN
> and how it should be used.
> 
> From Documentation/unaligned-memory-access.txt, I take it that this
> should be configured on a per-architecture basis, and it seems to be
> set to zero on both PPC and x86.
> 
> I wonder if this is proper though. It seems that its' use might depend
> on the I/O subsystem(s) in use as much as the architecture.
...

If the cpu cannot do misaligned memory cycles then NET_IP_ALIGN must be 2
and all receive frames must be aligned like that.

If the cpu can do misaligned memory cycles then the alignment of receive
ethernet frames doesn't matter that much.
NET_IP_ALIGN is likely to be set to zero because the cost of the cpu
doing misaligned transfers it likely to be a lot less than that of
un-optimised dma accesses to misaligned memory [1] [2].

If NET_IP_ALIGN is zero then I believe that ethernet drivers are
allowed to build skb that have the frame on a 4n+2 alignment.
This is probably sensible if the hardware can write the two bytes.
(DM might correct me there.)

	David

[1] The original sparc sbus 'DMA' part did multiple 16bit transfers instead
  of a burst of 32bit transfers. This meant the buffer had to be misaligned
  and a software copy done to align the frames. Fixed in the DMA+ part.

[2] PCIe writes are likely to be much faster if they contain entire cache
  lines of data.

Re: [PATCH 3/3] net: fec: align IP header in hardware

[Eric Nelson]

Hi David,

On 09/30/2016 06:49 AM, David Laight wrote:
> From: Eric Nelson
>> Sent: 30 September 2016 14:27
>> Thanks for the feedback David,
>>
>> On 09/29/2016 04:07 AM, David Laight wrote:
>>> From: Eric Nelson
>>>> Sent: 28 September 2016 18:15
>>>> On 09/28/2016 09:42 AM, David Laight wrote:
>>>>> From: Eric Nelson
>>>>>> Sent: 26 September 2016 19:40
>>>>>> Hi David,
>>>>>>
>>>>>> On 09/26/2016 02:26 AM, David Laight wrote:
>>>>>>> From: Eric Nelson
>>>>>>>> Sent: 24 September 2016 15:42
>>>>>>>> The FEC receive accelerator (RACC) supports shifting the data payload of
>>>>>>>> received packets by 16-bits, which aligns the payload (IP header) on a
>>>>>>>> 4-byte boundary, which is, if not required, at least strongly suggested
>>>>>>>> by the Linux networking layer.
>>>>>>> ...
>>>>>>>> +		/* align IP header */
>>>>>>>> +		val |= FEC_RACC_SHIFT16;
>>>>>>>
>>>>>>> I can't help feeling that there needs to be corresponding
>>>>>>> changes to increase the buffer size by 2 (maybe for large mtu)
>>>>>>> and to discard two bytes from the frame length.
>>>>>>>
>>>>>>
>>>>>> In the normal case, the fec driver over-allocates all receive packets to
>>>>>> be of size FEC_ENET_RX_FRSIZE (2048) minus the value of rx_align,
>>>>>> which is either 0x0f (ARM) or 0x03 (PPC).
>>>>>>
>>>>>> If the frame length is less than rx_copybreak (typically 256), then
>>>>>> the frame length from the receive buffer descriptor is used to
>>>>>> control the allocation size for a copied buffer, and this will include
>>>>>> the two bytes of padding if RACC_SHIFT16 is set.
>>>>>>
>>>>>>> If probably ought to be predicated on NET_IP_ALIGN as well.
>>>>>>>
>>>>>> Can you elaborate?
>>>>>
>>>>> From reading this it seems that the effect of FEC_RACC_SHIFT16 is to
>>>>> add two bytes of 'junk' to the start of every receive frame.
>>>>>
>>>>
>>>> That's right. Two bytes of junk between the MAC header and the
>>>> IP header.
>>>>
>>>>> In the 'copybreak' case the new skb would need to be 2 bytes shorter
>>>>> than the length reported by the hardware, and the data copied from
>>>>> 2 bytes into the dma buffer.
>>>>>
>>>>
>>>> As it stands, the skb allocated by the copybreak routine will include
>>>> the two bytes of padding, and the call to skb_pull_inline will ignore
>>>> them.
>>>
>>> Ok, I didn't see that call being added by this patch.
>>>
>>>>> The extra 2 bytes also mean the that maximum mtu that can be received
>>>>> into a buffer is two bytes less.
>>>>>
>>>>
>>>> Right, but I think the max is already high enough that this isn't a
>>>> problem.
>>>>
>>>>> If someone sets the mtu to (say) 9k for jumbo frames this might matter.
>>>>> Even with fixed 2048 byte buffers it reduces the maximum value the mtu
>>>>> can be set to by 2.
>>>>>
>>>>
>>>> As far as I can tell, the fec driver doesn't support jumbo frames, and
>>>> the max frame length is currently hard-coded at PKT_MAXBUF_SIZE (1522).
>>>>
>>>> This is well within the 2048-byte allocation, even with optional headers
>>>> for VLAN etc.
>>>
>>> Hmm...
>>>
>>> That (probably) means all the skb the driver allocates are actually 4k.
>>> It would be much better to reduce the size so that the entire skb
>>> (with packet buffer) is less than 2k.
>>>
>>
>> That seems worthwhile, but un-related to this patch.
> 
> Indeed.
> 
>> It appears to me that the received packets could be allocated as
>>
>> PKT_MAXBUF_SIZE+fep->rx_align+NET_IP_ALIGN
>>
>> (+2 if FEC_RACC_SHIFT16 is used)
> 
> No.
> The packet buffers need to be allocated NET_IP_ALIGN + PKT_MAXBUF_SIZE
> byte long and (I assume) aligned on a fep->rx_align byte boundary.
> 

I think we're saying the same thing here, with the exception of the
+2 for FEC_RACC_SHIFT16.

> If NET_IP_ALIGN is set (to 2) then FEC_RACC_SHIFT16 must also me set
> so that the ethernet frame itself is 4n+2 aligned.
> 

This patch does this, but not with the beginning of the skb.

It also does this when NET_IP_ALIGN is zero though, and I believe this
is the right thing, so the IP header is aligned in a sensible way.

The driver can't handle a DMA to (4n+2) on any architecture.

>>>>> Now if NET_IP_ALIGN is zero then it is fine for the rx frame to start
>>>>> on a 4n boundary, and the skb are likely to be allocated that way.
>>>>> In this case you don't want to extra two bytes of 'junk'.
>>>>>
>>>> NET_IP_ALIGN is defaulting to 2 by the conditional in skbuff.h
>>>
>>> Even though it is always currently set is isn't really ideal to have
>>> a driver that breaks if it isn't set.
>>> This could easily happen at some point in the future if the ethernet
>>> logic is put with a different cpu.
>>>
>>
>> After multiple reads, I'm confused about the meaning of NET_IP_ALIGN
>> and how it should be used.
>>
>> From Documentation/unaligned-memory-access.txt, I take it that this
>> should be configured on a per-architecture basis, and it seems to be
>> set to zero on both PPC and x86.
>>
>> I wonder if this is proper though. It seems that its' use might depend
>> on the I/O subsystem(s) in use as much as the architecture.
> ...
> 
> If the cpu cannot do misaligned memory cycles then NET_IP_ALIGN must be 2
> and all receive frames must be aligned like that.
> 

On ARM, the CPU can't handle misaligned memory cycles without
taking an alignment fault and NET_IP_ALIGN is set to 2.

On PPC, NET_IP_ALIGN is set to zero.

I could use some help from NXP about whether the driver is used on
PPC, but I don't think it can DMA to 4n+2 addresses on any architecture
and the purpose of this patch is to align the frame on a (4n+2)
address.

> If the cpu can do misaligned memory cycles then the alignment of receive
> ethernet frames doesn't matter that much.
> NET_IP_ALIGN is likely to be set to zero because the cost of the cpu
> doing misaligned transfers it likely to be a lot less than that of
> un-optimised dma accesses to misaligned memory [1] [2].
> 

On ARM, we have CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS=y
but I find it hard to believe that taking alignment faults is more
efficient than adding two bytes to the start of the frame.

> If NET_IP_ALIGN is zero then I believe that ethernet drivers are
> allowed to build skb that have the frame on a 4n+2 alignment.
> This is probably sensible if the hardware can write the two bytes.
> (DM might correct me there.)
> 

Again, I don't think the FEC can do this, even if PPC does allow
DMA to 4n+2 addresses for other functions.

> 	David
> 
> [1] The original sparc sbus 'DMA' part did multiple 16bit transfers instead
>   of a burst of 32bit transfers. This meant the buffer had to be misaligned
>   and a software copy done to align the frames. Fixed in the DMA+ part.
> 
> [2] PCIe writes are likely to be much faster if they contain entire cache
>   lines of data.
> 

Re: [PATCH 3/3] net: fec: align IP header in hardware

[Russell King - ARM Linux]

On Fri, Sep 30, 2016 at 07:16:12AM -0700, Eric Nelson wrote:
> On ARM, the CPU can't handle misaligned memory cycles without
> taking an alignment fault and NET_IP_ALIGN is set to 2.

Let's get this right...  With Linux on MMU parts:

On ARMv6+, unaligned memory cycles using the LDR, LDRH and corresponding
store instructions are handled in hardware without any exception being
raised.

On pre-ARMv6, such instructions raise an alignment exception, and we fix
up the load/store manually.

Where things behave the same is with the LDM (load multiple) and STM
(store multiple) instructions.  Hardware does not fix these up if they
are unaligned: it is expected that the base address will always be
aligned to a 32-bit word.

For some reason, the compiler guys have decided it's okay to use these
instructions as an optimisation, and I see no way to disable this
behaviour.

> On ARM, we have CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS=y
> but I find it hard to believe that taking alignment faults is more
> efficient than adding two bytes to the start of the frame.

... see above, hence why CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS is set.
It's only when the compiler decides to do silly things that things go
wrong.  However, net code does not care about that configuration
setting, so it's irrelevant to this discussion.

The issue with the networking layer is that it passes around structure
pointers which may not be "naturally aligned" - technically it goes
against the C standard specs.  However, you'll find it hard to argue
against this, so we have to accept that the networking people expect
it to work.

The optimisation that the C compiler uses (using LDM to access multiple
32-bit consecutive words) is legal and efficient when the structure
pointers are aligned as it expects, but that all breaks if the pointer
is not so aligned.  So, raising it as a bug against the C compiler isn't
going to work either.

What may work is to raise a feature request with compiler people to have
a mechanism to disable the LDM/STM optimisation for code where we know
that pointers may not be naturally aligned.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

RE: [PATCH 3/3] net: fec: align IP header in hardware

[David Laight]

From: Russell King - ARM Linux
> Sent: 01 October 2016 20:52
> On Fri, Sep 30, 2016 at 07:16:12AM -0700, Eric Nelson wrote:
> > On ARM, the CPU can't handle misaligned memory cycles without
> > taking an alignment fault and NET_IP_ALIGN is set to 2.
> 
> Let's get this right...  With Linux on MMU parts:
> 
> On ARMv6+, unaligned memory cycles using the LDR, LDRH and corresponding
> store instructions are handled in hardware without any exception being
> raised.
> 
> On pre-ARMv6, such instructions raise an alignment exception, and we fix
> up the load/store manually.

I'm not sure that is a good idea but...

> Where things behave the same is with the LDM (load multiple) and STM
> (store multiple) instructions.  Hardware does not fix these up if they
> are unaligned: it is expected that the base address will always be
> aligned to a 32-bit word.
> 
> For some reason, the compiler guys have decided it's okay to use these
> instructions as an optimisation, and I see no way to disable this
> behaviour.
...
> The issue with the networking layer is that it passes around structure
> pointers which may not be "naturally aligned" - technically it goes
> against the C standard specs.  However, you'll find it hard to argue
> against this, so we have to accept that the networking people expect
> it to work.

I think it 'only' casts misaligned pointers to structure on systems
where unaligned accesses are allowed.
It is almost impossible to do a 'realignment copy' on (for example) sparc.

> The optimisation that the C compiler uses (using LDM to access multiple
> 32-bit consecutive words) is legal and efficient when the structure
> pointers are aligned as it expects, but that all breaks if the pointer
> is not so aligned.  So, raising it as a bug against the C compiler isn't
> going to work either.
> 
> What may work is to raise a feature request with compiler people to have
> a mechanism to disable the LDM/STM optimisation for code where we know
> that pointers may not be naturally aligned.

What happens is the relevant structures are marked 'packed'?
If the compiler still generates LDM/STM that is a bug.
Assuming the compiler is targeting v6+ it shouldn't generate byte accesses.

	David

Re: [PATCH 3/3] net: fec: align IP header in hardware

[Eric Nelson]

Hi Russell,

On 10/01/2016 09:52 PM, Russell King - ARM Linux wrote:
> On Fri, Sep 30, 2016 at 07:16:12AM -0700, Eric Nelson wrote:
>> On ARM, the CPU can't handle misaligned memory cycles without
>> taking an alignment fault and NET_IP_ALIGN is set to 2.
> 
> Let's get this right...  With Linux on MMU parts:
> 
> On ARMv6+, unaligned memory cycles using the LDR, LDRH and corresponding
> store instructions are handled in hardware without any exception being
> raised.
> 
> On pre-ARMv6, such instructions raise an alignment exception, and we fix
> up the load/store manually.
> 
> Where things behave the same is with the LDM (load multiple) and STM
> (store multiple) instructions.  Hardware does not fix these up if they
> are unaligned: it is expected that the base address will always be
> aligned to a 32-bit word.
> 

Thanks for the clarification. This helps me understand why I didn't
see the exceptions that Eric warned about:

https://www.spinics.net/lists/netdev/msg397012.html

> For some reason, the compiler guys have decided it's okay to use these
> instructions as an optimisation, and I see no way to disable this
> behaviour.
> 
>> On ARM, we have CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS=y
>> but I find it hard to believe that taking alignment faults is more
>> efficient than adding two bytes to the start of the frame.
> 
> ... see above, hence why CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS is set.
> It's only when the compiler decides to do silly things that things go
> wrong.  However, net code does not care about that configuration
> setting, so it's irrelevant to this discussion.
> 

The obfuscated optimization in ip_gro_receive doesn't help by
reading two 16-bit values as a __be32:

id = ntohl(*(__be32 *)&iph->id);
flush = (u16)((ntohl(*(__be32 *)iph) ^ skb_gro_len(skb)) | (id & ~IP_DF));
id >>= 16;

> The issue with the networking layer is that it passes around structure
> pointers which may not be "naturally aligned" - technically it goes
> against the C standard specs.  However, you'll find it hard to argue
> against this, so we have to accept that the networking people expect
> it to work.
> 
> The optimisation that the C compiler uses (using LDM to access multiple
> 32-bit consecutive words) is legal and efficient when the structure
> pointers are aligned as it expects, but that all breaks if the pointer
> is not so aligned.  So, raising it as a bug against the C compiler isn't
> going to work either.
> 
> What may work is to raise a feature request with compiler people to have
> a mechanism to disable the LDM/STM optimisation for code where we know
> that pointers may not be naturally aligned.
> 

Agreed, but that's a long path even if the compiler folks agree
immediately.

It's probably to just fix the driver with known issues for now.

Did you have any comments on the patch? I tried to pull what I found
from your patch set, but only the enabling of the SHIFT16 bit.

Please advise,


Eric

RE: [PATCH 3/3] net: fec: align IP header in hardware

[Andy Duan]

From: Eric Nelson <eric@nelint.com> Sent: Friday, September 30, 2016 9:27 PM
> To: David Laight <David.Laight@ACULAB.COM>; netdev@vger.kernel.org
> Cc: linux@arm.linux.org.uk; andrew@lunn.ch; Andy Duan
> <fugang.duan@nxp.com>; otavio@ossystems.com.br;
> edumazet@google.com; troy.kisky@boundarydevices.com;
> davem@davemloft.net; u.kleine-koenig@pengutronix.de
> Subject: Re: [PATCH 3/3] net: fec: align IP header in hardware
> 
> Thanks for the feedback David,
> 
> On 09/29/2016 04:07 AM, David Laight wrote:
> > From: Eric Nelson
> >> Sent: 28 September 2016 18:15
> >> On 09/28/2016 09:42 AM, David Laight wrote:
> >>> From: Eric Nelson
> >>>> Sent: 26 September 2016 19:40
> >>>> Hi David,
> >>>>
> >>>> On 09/26/2016 02:26 AM, David Laight wrote:
> >>>>> From: Eric Nelson
> >>>>>> Sent: 24 September 2016 15:42
> >>>>>> The FEC receive accelerator (RACC) supports shifting the data
> >>>>>> payload of received packets by 16-bits, which aligns the payload
> >>>>>> (IP header) on a 4-byte boundary, which is, if not required, at
> >>>>>> least strongly suggested by the Linux networking layer.
> >>>>> ...
> >>>>>> +		/* align IP header */
> >>>>>> +		val |= FEC_RACC_SHIFT16;
> >>>>>
> >>>>> I can't help feeling that there needs to be corresponding changes
> >>>>> to increase the buffer size by 2 (maybe for large mtu) and to
> >>>>> discard two bytes from the frame length.
> >>>>>
> >>>>
> >>>> In the normal case, the fec driver over-allocates all receive
> >>>> packets to be of size FEC_ENET_RX_FRSIZE (2048) minus the value of
> >>>> rx_align, which is either 0x0f (ARM) or 0x03 (PPC).
> >>>>
> >>>> If the frame length is less than rx_copybreak (typically 256), then
> >>>> the frame length from the receive buffer descriptor is used to
> >>>> control the allocation size for a copied buffer, and this will
> >>>> include the two bytes of padding if RACC_SHIFT16 is set.
> >>>>
> >>>>> If probably ought to be predicated on NET_IP_ALIGN as well.
> >>>>>
> >>>> Can you elaborate?
> >>>
> >>> From reading this it seems that the effect of FEC_RACC_SHIFT16 is to
> >>> add two bytes of 'junk' to the start of every receive frame.
> >>>
> >>
> >> That's right. Two bytes of junk between the MAC header and the IP
> >> header.
> >>
> >>> In the 'copybreak' case the new skb would need to be 2 bytes shorter
> >>> than the length reported by the hardware, and the data copied from
> >>> 2 bytes into the dma buffer.
> >>>
> >>
> >> As it stands, the skb allocated by the copybreak routine will include
> >> the two bytes of padding, and the call to skb_pull_inline will ignore
> >> them.
> >
> > Ok, I didn't see that call being added by this patch.
> >
> >>> The extra 2 bytes also mean the that maximum mtu that can be
> >>> received into a buffer is two bytes less.
> >>>
> >>
> >> Right, but I think the max is already high enough that this isn't a
> >> problem.
> >>
> >>> If someone sets the mtu to (say) 9k for jumbo frames this might matter.
> >>> Even with fixed 2048 byte buffers it reduces the maximum value the
> >>> mtu can be set to by 2.
> >>>
> >>
> >> As far as I can tell, the fec driver doesn't support jumbo frames,
> >> and the max frame length is currently hard-coded at PKT_MAXBUF_SIZE
> (1522).
> >>
> >> This is well within the 2048-byte allocation, even with optional
> >> headers for VLAN etc.
> >
> > Hmm...
> >
> > That (probably) means all the skb the driver allocates are actually 4k.
> > It would be much better to reduce the size so that the entire skb
> > (with packet buffer) is less than 2k.
> >
> 
> That seems worthwhile, but un-related to this patch.
> 
> It appears to me that the received packets could be allocated as
> 
> PKT_MAXBUF_SIZE+fep->rx_align+NET_IP_ALIGN
> 
> (+2 if FEC_RACC_SHIFT16 is used)
> 
> >>> Now if NET_IP_ALIGN is zero then it is fine for the rx frame to
> >>> start on a 4n boundary, and the skb are likely to be allocated that way.
> >>> In this case you don't want to extra two bytes of 'junk'.
> >>>
> >> NET_IP_ALIGN is defaulting to 2 by the conditional in skbuff.h
> >
> > Even though it is always currently set is isn't really ideal to have a
> > driver that breaks if it isn't set.
> > This could easily happen at some point in the future if the ethernet
> > logic is put with a different cpu.
> >
> 
> After multiple reads, I'm confused about the meaning of NET_IP_ALIGN and
> how it should be used.
> 
> From Documentation/unaligned-memory-access.txt, I take it that this should
> be configured on a per-architecture basis, and it seems to be set to zero on
> both PPC and x86.
> 
> I wonder if this is proper though. It seems that its' use might depend on the
> I/O subsystem(s) in use as much as the architecture.
> 
> For example, it might be desirable to have a different value for a PCIe
> interface than for an integrated MAC like the FEC.
> 
> Looking at the example of the 3c59x driver, I see a pattern of an allocation
> that adds NET_IP_ALIGN followed by an skb->reserve() of NET_IP_ALIGN
> before determining the target address to end up with allocation with 4n+2
> alignment.
> 
> This seems somewhat equivalent to this patch, except that we're using the
> allocated address as the target and using skb_pull_inline afterwards.
> 
> Andy, is the FEC used on any PPC SOCs?
> 
Sorry to reply the mail due to holiday.

Currently, i.MX and ColdFire like MCF5xxx series use the driver. And ColdFire series don't define FEC_QUIRK_HAS_RACC quirk flag, so the patch don't impact ColdFire.

The patch has no problem, has nothing related to DMA part.

> If so, then this patch may cause a DMA of 2 extra bytes per frame
> unecessarily although the driver doesn't special-case the allocation to align
> the IP header, so this is still probably preferred.
> 
> >>> OTOH if NET_IP_ALIGN is 2 then you need to 'fiddle' things so that
> >>> the data is dma'd to offset -2 in the skb and then ensure that the
> >>> end of frame is set correctly.
> >>>
> >>
> >> That's what the RACC SHIFT16 bit does.
> >
> > No, that causes the ethernet controller to add 2 bytes to the frame.
> > You then need to change the dma target address to match.
> >
> 
> Or use skb_pull_inline to ignore the two bytes.
> 
> > Otherwise if a new version of the silicon stops ignoring the low
> > address with the frame will be misaligned in the buffer.
> >
> 
> I'm not sure I understand this.
> 
> > The receive frame length will also (probably) be 2 larger than the
> > actual frame - so you need to set the end point correctly as well.
> > IP will probably ignore the 2 bytes of pad I think you are generating.
> >
> 
> The received frame length **is** 2 bytes longer, but these are eaten by
> skb_pull_inline().
> 
> >> The FEC hardware isn't capable of DMA'ing to an un-aligned address.
> >> On ARM, it requires 64-bit alignment, but suggests 128-bit alignment.
> >>
> >> On other (PPC?) architectures, it requires 32-bit alignment. This is
> >> handled by the rx_align field.
> >
> > That isn't entirely relevant.
> >
> > If the kernel is being built with NET_IP_ALIGN set to 0 you should
> > align the destination mac address on a 4n boundary (Or rather the skb
> > are likely to be allocated that way).
> 
> They're not currently allocated that way. The routine
> fec_enet_alloc_rxq_buffers forces the allocations to 32 or 128-bit alignment
> through the routine fec_enet_new_rxbdp().
> 
> > If it causes misaligned memory reads later on that is a different problem.
> 
> That's the problem this patch is designed to address. Without this patch, the
> IP header is always mis-aligned.
> 
> > The MAC driver has aligned the frames as it was told to.
> >
> > 	David
> >
> >
> 
> Regards,
> 
> 
> Eric