[PATCH] net: ftgmac100: Fix missing TX-poll issue

The cpu accesses the register and the memory via different bus/path on
aspeed soc.  So we can not guarantee that the tx-poll command
(register access) is always behind the tx descriptor (memory).  In other
words, the HW may start working even the data is not yet ready.  By
adding a dummy read after the last data write, we can ensure the data
are pushed to the memory, then guarantee the processing sequence

Signed-off-by: Dylan Hung <redacted>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
index 00024dd41147..9a99a87f29f3 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c

@@ -804,7 +804,8 @@ static netdev_tx_t ftgmac100_hard_start_xmit(struct sk_buff *skb,
 	 * before setting the OWN bit on the first descriptor.
 	 */
 	dma_wmb();
-	first->txdes0 = cpu_to_le32(f_ctl_stat);
+	WRITE_ONCE(first->txdes0, cpu_to_le32(f_ctl_stat));
+	READ_ONCE(first->txdes0);
 
 	/* Update next TX pointer */
 	priv->tx_pointer = pointer;

-- 
2.17.1

On Mon, 19 Oct 2020 at 07:39, Dylan Hung [off-list ref] wrote:

The cpu accesses the register and the memory via different bus/path on
aspeed soc.  So we can not guarantee that the tx-poll command

Just the 2600, or other versions too?

(register access) is always behind the tx descriptor (memory).  In other
words, the HW may start working even the data is not yet ready.  By

even if the

I understand what you're trying to do here, but I'm not sure that this
is the correct way to go about it.

It does cause the compiler to produce a store and then a load.

        /* Update next TX pointer */
        priv->tx_pointer = pointer;
--
2.17.1

Hi Joel,

-----Original Message-----
From: Joel Stanley [mailto:joel@jms.id.au]
Sent: Monday, October 19, 2020 4:57 PM
To: Dylan Hung <redacted>; Benjamin Herrenschmidt
[off-list ref]
Cc: David S . Miller <davem@davemloft.net>; Jakub Kicinski
[off-list ref]; netdev@vger.kernel.org; Linux Kernel Mailing List
[off-list ref]; Po-Yu Chuang [off-list ref];
linux-aspeed [off-list ref]; OpenBMC Maillist
[off-list ref]; BMC-SW [off-list ref]
Subject: Re: [PATCH] net: ftgmac100: Fix missing TX-poll issue

On Mon, 19 Oct 2020 at 07:39, Dylan Hung [off-list ref]
wrote:

quoted

The cpu accesses the register and the memory via different bus/path on
aspeed soc.  So we can not guarantee that the tx-poll command

Just the 2600, or other versions too?

Just the 2600.  And this issue only occurred on Ethernet mac.

quoted

(register access) is always behind the tx descriptor (memory).  In
other words, the HW may start working even the data is not yet ready.
By

even if the

quoted

adding a dummy read after the last data write, we can ensure the data
are pushed to the memory, then guarantee the processing sequence

Signed-off-by: Dylan Hung <redacted>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c

b/drivers/net/ethernet/faraday/ftgmac100.c
index 00024dd41147..9a99a87f29f3 100644

--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c

@@ -804,7 +804,8 @@ static netdev_tx_t

ftgmac100_hard_start_xmit(struct sk_buff *skb,

quoted

         * before setting the OWN bit on the first descriptor.
         */
        dma_wmb();
-       first->txdes0 = cpu_to_le32(f_ctl_stat);
+       WRITE_ONCE(first->txdes0, cpu_to_le32(f_ctl_stat));
+       READ_ONCE(first->txdes0);

I understand what you're trying to do here, but I'm not sure that this is the
correct way to go about it.

It does cause the compiler to produce a store and then a load.

quoted

        /* Update next TX pointer */
        priv->tx_pointer = pointer;
--
2.17.1

On Mon, 19 Oct 2020 08:57:03 +0000 Joel Stanley wrote:

quoted

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
index 00024dd41147..9a99a87f29f3 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c

@@ -804,7 +804,8 @@ static netdev_tx_t ftgmac100_hard_start_xmit(struct sk_buff *skb,
         * before setting the OWN bit on the first descriptor.
         */
        dma_wmb();
-       first->txdes0 = cpu_to_le32(f_ctl_stat);
+       WRITE_ONCE(first->txdes0, cpu_to_le32(f_ctl_stat));
+       READ_ONCE(first->txdes0);  

I understand what you're trying to do here, but I'm not sure that this
is the correct way to go about it.

It does cause the compiler to produce a store and then a load.

+1 @first is system memory from dma_alloc_coherent(), right?

You shouldn't have to do this. Is coherent DMA memory broken 
on your platform?

On Mon, 2020-10-19 at 12:00 -0700, Jakub Kicinski wrote:

On Mon, 19 Oct 2020 08:57:03 +0000 Joel Stanley wrote:

quoted

quoted

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c

b/drivers/net/ethernet/faraday/ftgmac100.c
index 00024dd41147..9a99a87f29f3 100644

--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c

@@ -804,7 +804,8 @@ static netdev_tx_t

ftgmac100_hard_start_xmit(struct sk_buff *skb,
         * before setting the OWN bit on the first descriptor.
         */
        dma_wmb();
-       first->txdes0 = cpu_to_le32(f_ctl_stat);
+       WRITE_ONCE(first->txdes0, cpu_to_le32(f_ctl_stat));
+       READ_ONCE(first->txdes0);  

I understand what you're trying to do here, but I'm not sure that
this
is the correct way to go about it.

It does cause the compiler to produce a store and then a load.

+1 @first is system memory from dma_alloc_coherent(), right?

You shouldn't have to do this. Is coherent DMA memory broken 
on your platform?

I suspect the problem is that the HW (and yes this would be a HW bug)
doesn't order the CPU -> memory and the CPU -> MMIO path.

What I think happens is that the store to txde0 is potentially still in
a buffer somewhere on its way to memory, gets bypassed by the store to
MMIO, causing the MAC to try to read the descriptor, and getting the
"old" data from memory.

It's ... fishy, but that isn't the first time an Aspeed chip has that
type of bug (there's a similar one in the USB device controler iirc).

Cheers,
Ben.

On Tue, 20 Oct 2020 10:23:41 +1100 Benjamin Herrenschmidt wrote:

On Mon, 2020-10-19 at 12:00 -0700, Jakub Kicinski wrote:

quoted

On Mon, 19 Oct 2020 08:57:03 +0000 Joel Stanley wrote:  

quoted

quoted

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c

b/drivers/net/ethernet/faraday/ftgmac100.c
index 00024dd41147..9a99a87f29f3 100644

--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c

@@ -804,7 +804,8 @@ static netdev_tx_t

ftgmac100_hard_start_xmit(struct sk_buff *skb,
         * before setting the OWN bit on the first descriptor.
         */
        dma_wmb();
-       first->txdes0 = cpu_to_le32(f_ctl_stat);
+       WRITE_ONCE(first->txdes0, cpu_to_le32(f_ctl_stat));
+       READ_ONCE(first->txdes0);    

I understand what you're trying to do here, but I'm not sure that
this
is the correct way to go about it.

It does cause the compiler to produce a store and then a load.  

+1 @first is system memory from dma_alloc_coherent(), right?

You shouldn't have to do this. Is coherent DMA memory broken 
on your platform?  

I suspect the problem is that the HW (and yes this would be a HW bug)
doesn't order the CPU -> memory and the CPU -> MMIO path.

What I think happens is that the store to txde0 is potentially still in
a buffer somewhere on its way to memory, gets bypassed by the store to
MMIO, causing the MAC to try to read the descriptor, and getting the
"old" data from memory.

I see, but in general this sort of a problem should be resolved by
adding an appropriate memory barrier. And in fact such barrier should
(these days) be implied by a writel (I'm not 100% clear on why this
driver uses iowrite, and if it matters).

It's ... fishy, but that isn't the first time an Aspeed chip has that
type of bug (there's a similar one in the USB device controler iirc).

Argh.

-----Original Message-----
From: Jakub Kicinski [mailto:kuba@kernel.org]
Sent: Tuesday, October 20, 2020 3:01 AM
To: Joel Stanley <joel@jms.id.au>
Cc: Dylan Hung <redacted>; Benjamin Herrenschmidt
[off-list ref]; David S . Miller [off-list ref];
netdev@vger.kernel.org; Linux Kernel Mailing List
[off-list ref]; Po-Yu Chuang [off-list ref];
linux-aspeed [off-list ref]; OpenBMC Maillist
[off-list ref]; BMC-SW [off-list ref]
Subject: Re: [PATCH] net: ftgmac100: Fix missing TX-poll issue

On Mon, 19 Oct 2020 08:57:03 +0000 Joel Stanley wrote:

quoted

quoted

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c

b/drivers/net/ethernet/faraday/ftgmac100.c
index 00024dd41147..9a99a87f29f3 100644

--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c

@@ -804,7 +804,8 @@ static netdev_tx_t

ftgmac100_hard_start_xmit(struct sk_buff *skb,

quoted

quoted

         * before setting the OWN bit on the first descriptor.
         */
        dma_wmb();
-       first->txdes0 = cpu_to_le32(f_ctl_stat);
+       WRITE_ONCE(first->txdes0, cpu_to_le32(f_ctl_stat));
+       READ_ONCE(first->txdes0);

I understand what you're trying to do here, but I'm not sure that this
is the correct way to go about it.

It does cause the compiler to produce a store and then a load.

Yes, the load instruction here is to guarantee the previous store is indeed pushed onto the physical memory.

+1 @first is system memory from dma_alloc_coherent(), right?

You shouldn't have to do this. Is coherent DMA memory broken on your
platform?

It is about the arbitration on the DRAM controller.  There are two queues in the dram controller, one is for the CPU access and the other is for the HW engines.
When CPU issues a store command, the dram controller just acknowledges cpu's request and pushes the request into the queue.  Then CPU triggers the HW MAC engine, the HW engine starts to fetch the DMA memory.
But since the cpu's request may still stay in the queue, the HW engine may fetch the wrong data.

On Mon, 2020-10-19 at 19:57 -0700, Jakub Kicinski wrote:

quoted

I suspect the problem is that the HW (and yes this would be a HW bug)
doesn't order the CPU -> memory and the CPU -> MMIO path.

What I think happens is that the store to txde0 is potentially still in
a buffer somewhere on its way to memory, gets bypassed by the store to
MMIO, causing the MAC to try to read the descriptor, and getting the
"old" data from memory.

I see, but in general this sort of a problem should be resolved by
adding an appropriate memory barrier. And in fact such barrier should
(these days) be implied by a writel (I'm not 100% clear on why this
driver uses iowrite, and if it matters).

No, a barrier won't solve this I think.

This is a coherency problem at the fabric/interconnect level. I has to
do with the way they implemented the DMA path from memory to the
ethernet controller using a different "port" of the memory controller
than the one used by the CPU, separately from the MMIO path, with no
proper ordering between those busses. Old school design .... and
broken.

By doing a read back, they probably force the previous write to memory
to get past the point where it will be visible to a subsequent DMA read
by the ethernet controller.

quoted

It's ... fishy, but that isn't the first time an Aspeed chip has that
type of bug (there's a similar one in the USB device controler iirc).

Cheers,
Ben.

From: Dylan Hung

Sent: 20 October 2020 07:15

quoted

-----Original Message-----
From: Jakub Kicinski [mailto:kuba@kernel.org]

On Mon, 19 Oct 2020 08:57:03 +0000 Joel Stanley wrote:

quoted

quoted

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c

b/drivers/net/ethernet/faraday/ftgmac100.c
index 00024dd41147..9a99a87f29f3 100644

--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c

@@ -804,7 +804,8 @@ static netdev_tx_t

ftgmac100_hard_start_xmit(struct sk_buff *skb,

quoted

quoted

         * before setting the OWN bit on the first descriptor.
         */
        dma_wmb();
-       first->txdes0 = cpu_to_le32(f_ctl_stat);
+       WRITE_ONCE(first->txdes0, cpu_to_le32(f_ctl_stat));
+       READ_ONCE(first->txdes0);

I understand what you're trying to do here, but I'm not sure that this
is the correct way to go about it.

It does cause the compiler to produce a store and then a load.

Yes, the load instruction here is to guarantee the previous store is indeed
pushed onto the physical memory.

That rather depends where the data is 'stuck'.

An old sparc cpu would flush the cpu store buffer before the read.
But a modern x86 cpu will satisfy the read from the store buffer
for cached data.

If the write is 'posted' on a PCI(e) bus then the read can't overtake it.
But that is a memory access so shouldn't be to a PCI(e) address.

Shouldn't dma_wb() actually force your 'cpu to dram' queue be flushed?
In which case you need one after writing the ring descriptor and
before the poke of the mac engine.

The barrier before the descriptor write only needs to guarantee
ordering of the writes - it can probably be a lighter barrier?

It might be that your dma_wmb() needs to do a write+read of
an uncached DRAM location in order to empty the cpu to dram queue.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

On Tue, 20 Oct 2020 17:15:42 +1100 Benjamin Herrenschmidt wrote:

On Mon, 2020-10-19 at 19:57 -0700, Jakub Kicinski wrote:

quoted

quoted

I suspect the problem is that the HW (and yes this would be a HW bug)
doesn't order the CPU -> memory and the CPU -> MMIO path.

What I think happens is that the store to txde0 is potentially still in
a buffer somewhere on its way to memory, gets bypassed by the store to
MMIO, causing the MAC to try to read the descriptor, and getting the
"old" data from memory.  

I see, but in general this sort of a problem should be resolved by
adding an appropriate memory barrier. And in fact such barrier should
(these days) be implied by a writel (I'm not 100% clear on why this
driver uses iowrite, and if it matters).  

No, a barrier won't solve this I think.

This is a coherency problem at the fabric/interconnect level. I has to
do with the way they implemented the DMA path from memory to the
ethernet controller using a different "port" of the memory controller
than the one used by the CPU, separately from the MMIO path, with no
proper ordering between those busses. Old school design .... and
broken.

By doing a read back, they probably force the previous write to memory
to get past the point where it will be visible to a subsequent DMA read
by the ethernet controller.

Thanks for the explanation. How wonderful :/

It'd still be highly, highly preferable if the platform was conforming
to the Linux memory model. IO successors (iowrite32 / writel) must
ensure previous DRAM writes had completed. For performance sensitive
ops, which don't require ordering we have writel_relaxed etc.

I assume the DRAM controller queue is a straight FIFO and we don't have
to worry about hitting the same address, so how about we add a read
of some known uncached address in iowrite32 / writel?

On Tue, 2020-10-20 at 13:15 +0000, David Laight wrote:

That rather depends where the data is 'stuck'.

An old sparc cpu would flush the cpu store buffer before the read.
But a modern x86 cpu will satisfy the read from the store buffer
for cached data.

If the write is 'posted' on a PCI(e) bus then the read can't overtake it.
But that is a memory access so shouldn't be to a PCI(e) address.

Shouldn't dma_wb() actually force your 'cpu to dram' queue be flushed?
In which case you need one after writing the ring descriptor and
before the poke of the mac engine.

The barrier before the descriptor write only needs to guarantee
ordering of the writes - it can probably be a lighter barrier?

It might be that your dma_wmb() needs to do a write+read of
an uncached DRAM location in order to empty the cpu to dram queue.

This is a specific bug with how a specific IP block is hooked up in
those SOCs, I wouldn't bloat the global dma_wmb for that. The read back
in the driver with appropriate comment should be enough.

Cheers,
Ben.