[PATCH 00/12] net: mediatek: various small fixes

[PATCH 00/12] net: mediatek: various small fixes

[John Crispin]

This series contains various small fixes that we stumbled across while
doing thorough testing and code level reviewing of the driver. The only
patch that sticks out is the first one, which addresses a DQL related
issue. The rest are just minor fixes.

John Crispin (12):
  net: mediatek: fix DQL support
  net: mediatek: add missing return code check
  net: mediatek: fix missing free of scratch memory
  net: mediatek: invalid buffer lookup in mtk_tx_map()
  net: mediatek: dropped rx packets are not being counted properly
  net: mediatek: add next data pointer coherency protection
  net: mediatek: disable all interrupts during probe
  net: mediatek: fix threshold value
  net: mediatek: increase watchdog_timeo
  net: mediatek: fix off by one in the TX ring allocation
  net: mediatek: only wake the queue if it is stopped
  net: mediatek: remove superfluous queue wake up call

 drivers/net/ethernet/mediatek/mtk_eth_soc.c |   94 ++++++++++++++++++---------
 drivers/net/ethernet/mediatek/mtk_eth_soc.h |    3 +
 2 files changed, 65 insertions(+), 32 deletions(-)

-- 
1.7.10.4

[PATCH 01/12] net: mediatek: fix DQL support

[John Crispin]

The MTK ethernet core has 2 MACs both sitting on the same DMA ring. For
DQL to be deterministic it needs to track the amount of data in the DMA
ring and not the amount of data enqueued on each device. The current code
is incorrect, fix it by making it each device track its own traffic aswell
as the traffic of the other device.

Signed-off-by: John Crispin <john-Pj+rj9U5foFAfugRpC6u6w@public.gmane.org>
---
 drivers/net/ethernet/mediatek/mtk_eth_soc.c |   33 ++++++++++++++++-----------
 1 file changed, 20 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
index c984462..45f8dbf 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
@@ -625,7 +625,16 @@ static int mtk_tx_map(struct sk_buff *skb, struct net_device *dev,
 	WRITE_ONCE(itxd->txd3, (TX_DMA_SWC | TX_DMA_PLEN0(skb_headlen(skb)) |
 				(!nr_frags * TX_DMA_LS0)));
 
-	netdev_sent_queue(dev, skb->len);
+	/* we have a single DMA ring so BQL needs to be updated for all devices
+	 * sitting on this ring
+	 */
+	for (i = 0; i < MTK_MAC_COUNT; i++) {
+		if (!eth->netdev[i])
+			continue;
+
+		netdev_sent_queue(eth->netdev[i], skb->len);
+	}
+
 	skb_tx_timestamp(skb);
 
 	ring->next_free = mtk_qdma_phys_to_virt(ring, txd->txd2);
@@ -853,21 +862,18 @@ static int mtk_poll_tx(struct mtk_eth *eth, int budget, bool *tx_again)
 	struct mtk_tx_dma *desc;
 	struct sk_buff *skb;
 	struct mtk_tx_buf *tx_buf;
-	int total = 0, done[MTK_MAX_DEVS];
-	unsigned int bytes[MTK_MAX_DEVS];
+	int total = 0, done = 0;
+	unsigned int bytes = 0;
 	u32 cpu, dma;
 	static int condition;
 	int i;
 
-	memset(done, 0, sizeof(done));
-	memset(bytes, 0, sizeof(bytes));
-
 	cpu = mtk_r32(eth, MTK_QTX_CRX_PTR);
 	dma = mtk_r32(eth, MTK_QTX_DRX_PTR);
 
 	desc = mtk_qdma_phys_to_virt(ring, cpu);
 
-	while ((cpu != dma) && budget) {
+	while ((cpu != dma) && done < budget) {
 		u32 next_cpu = desc->txd2;
 		int mac;
 
@@ -887,9 +893,8 @@ static int mtk_poll_tx(struct mtk_eth *eth, int budget, bool *tx_again)
 		}
 
 		if (skb != (struct sk_buff *)MTK_DMA_DUMMY_DESC) {
-			bytes[mac] += skb->len;
-			done[mac]++;
-			budget--;
+			bytes += skb->len;
+			done++;
 		}
 		mtk_tx_unmap(eth->dev, tx_buf);
 
@@ -902,11 +907,13 @@ static int mtk_poll_tx(struct mtk_eth *eth, int budget, bool *tx_again)
 
 	mtk_w32(eth, cpu, MTK_QTX_CRX_PTR);
 
+	/* we have a single DMA ring so BQL needs to be updated for all devices
+	 * sitting on this ring
+	 */
 	for (i = 0; i < MTK_MAC_COUNT; i++) {
-		if (!eth->netdev[i] || !done[i])
+		if (!eth->netdev[i])
 			continue;
-		netdev_completed_queue(eth->netdev[i], done[i], bytes[i]);
-		total += done[i];
+		netdev_completed_queue(eth->netdev[i], done, bytes);
 	}
 
 	/* read hw index again make sure no new tx packet */
-- 
1.7.10.4

Re: [PATCH 01/12] net: mediatek: fix DQL support

[David Miller]

From: John Crispin <john@phrozen.org>
Date: Sun,  5 Jun 2016 08:32:54 +0200

> @@ -625,7 +625,16 @@ static int mtk_tx_map(struct sk_buff *skb, struct net_device *dev,
>  	WRITE_ONCE(itxd->txd3, (TX_DMA_SWC | TX_DMA_PLEN0(skb_headlen(skb)) |
>  				(!nr_frags * TX_DMA_LS0)));
>  
> -	netdev_sent_queue(dev, skb->len);
> +	/* we have a single DMA ring so BQL needs to be updated for all devices
> +	 * sitting on this ring
> +	 */
> +	for (i = 0; i < MTK_MAC_COUNT; i++) {
> +		if (!eth->netdev[i])
> +			continue;
> +
> +		netdev_sent_queue(eth->netdev[i], skb->len);
> +	}
> +
>  	skb_tx_timestamp(skb);

Sorry, this is very far from working.

You cannot asynchronously touch the DQL state of another netdevice.

You have to hold the TX lock of a queue while changing it's DQL state,
otherwise you'll corrupt the state.

This "loop over all possible devices on this DMA ring" is pretty
expensive for the problem you're trying to solve.

You'll have to find another way to fix this bug, which BTW I'm not too
clear about.  The commit message doesn't explain sufficiently what the
actual problem is.  "not deterministic" doesn't give enough details.

Re: [PATCH 01/12] net: mediatek: fix DQL support

[John Crispin]

On 05/06/2016 09:32, David Miller wrote:
> From: John Crispin <john@phrozen.org>
> Date: Sun,  5 Jun 2016 08:32:54 +0200
> 
>> @@ -625,7 +625,16 @@ static int mtk_tx_map(struct sk_buff *skb, struct net_device *dev,
>>  	WRITE_ONCE(itxd->txd3, (TX_DMA_SWC | TX_DMA_PLEN0(skb_headlen(skb)) |
>>  				(!nr_frags * TX_DMA_LS0)));
>>  
>> -	netdev_sent_queue(dev, skb->len);
>> +	/* we have a single DMA ring so BQL needs to be updated for all devices
>> +	 * sitting on this ring
>> +	 */
>> +	for (i = 0; i < MTK_MAC_COUNT; i++) {
>> +		if (!eth->netdev[i])
>> +			continue;
>> +
>> +		netdev_sent_queue(eth->netdev[i], skb->len);
>> +	}
>> +
>>  	skb_tx_timestamp(skb);
> 
> Sorry, this is very far from working.
> 
> You cannot asynchronously touch the DQL state of another netdevice.
> 
> You have to hold the TX lock of a queue while changing it's DQL state,
> otherwise you'll corrupt the state.
> 
> This "loop over all possible devices on this DMA ring" is pretty
> expensive for the problem you're trying to solve.
> 
> You'll have to find another way to fix this bug, which BTW I'm not too
> clear about.  The commit message doesn't explain sufficiently what the
> actual problem is.  "not deterministic" doesn't give enough details.
> 

Hi David,

DQL is supposed to measure how much data is enqueued on a netdev. the
problem here is that two devices share the same hardware queue. fq_codel
for example uses the values from dql to base its QoS judgement on. if we
track the dql of the 2 devices separately then the values will not take
the actual amount of data enqueued into account but only parts of it.
this will make the queue length used as a basis for fq_codel
calculations non deterministic thus breaking qos. dql needs to track the
amount of data in the physical queue underlying the netdev to be useful.
hope that explanation is better to understand.

i think one solution would be to add some code to have 2 devices share
the same dql instance. would that be an acceptable solution ?

anyhow, i will resend the series without the dql patch today and then
worry about it afterwards.

	John

Re: [PATCH 01/12] net: mediatek: fix DQL support

[David Miller]

From: John Crispin <john@phrozen.org>
Date: Mon, 6 Jun 2016 08:43:13 +0200

> i think one solution would be to add some code to have 2 devices share
> the same dql instance. would that be an acceptable solution ?

You still need to address the issue of synchronization.

dql purposefully doesn't use locking, always because a higher level
object (in this case the netdev TX queue) it is contained within
provides the synchronization.

That breaks apart once you share the dql between two netdevs, as you
are proposing here.  You'll have to add locking, which is expensive.

That's why I'm trying to encourage you to think out of the box and
find some way to solve the issue without having to access shared
state shared between multiple devices.

Thanks.

Re: [PATCH 01/12] net: mediatek: fix DQL support

[Tom Herbert]

On Tue, Jun 7, 2016 at 4:01 PM, David Miller <davem@davemloft.net> wrote:
> From: John Crispin <john@phrozen.org>
> Date: Mon, 6 Jun 2016 08:43:13 +0200
>
>> i think one solution would be to add some code to have 2 devices share
>> the same dql instance. would that be an acceptable solution ?
>
> You still need to address the issue of synchronization.
>
> dql purposefully doesn't use locking, always because a higher level
> object (in this case the netdev TX queue) it is contained within
> provides the synchronization.
>
> That breaks apart once you share the dql between two netdevs, as you
> are proposing here.  You'll have to add locking, which is expensive.
>
> That's why I'm trying to encourage you to think out of the box and
> find some way to solve the issue without having to access shared
> state shared between multiple devices.
>

I think you guys mean mean BQL not DQL :-)

If two netdevs share the same DMA ring then is using two netdevs the
right approach. Seems like this would have other consequences beyond
BQL...

Tom

> Thanks.
>

[PATCH 02/12] net: mediatek: add missing return code check

[John Crispin]

The code fails to check if the scratch memory was properly allocated. Add
this check and return with an error if the allocation failed.

Signed-off-by: John Crispin <john-Pj+rj9U5foFAfugRpC6u6w@public.gmane.org>
---
 drivers/net/ethernet/mediatek/mtk_eth_soc.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
index 45f8dbf..6780886 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
@@ -467,6 +467,9 @@ static int mtk_init_fq_dma(struct mtk_eth *eth)
 
 	eth->scratch_head = kcalloc(cnt, MTK_QDMA_PAGE_SIZE,
 				    GFP_KERNEL);
+	if (unlikely(!eth->scratch_head))
+		return -ENOMEM;
+
 	dma_addr = dma_map_single(eth->dev,
 				  eth->scratch_head, cnt * MTK_QDMA_PAGE_SIZE,
 				  DMA_FROM_DEVICE);
-- 
1.7.10.4

[PATCH 04/12] net: mediatek: invalid buffer lookup in mtk_tx_map()

[John Crispin]

The lookup of the tx_buffer in the error path inside mtk_tx_map() uses the
wrong descriptor pointer. This looks like a copy & paste error. Change the
code to use the correct pointer.

Signed-off-by: John Crispin <john-Pj+rj9U5foFAfugRpC6u6w@public.gmane.org>
---
 drivers/net/ethernet/mediatek/mtk_eth_soc.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
index 8d79bbf..4f22ee9 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
@@ -655,7 +655,7 @@ static int mtk_tx_map(struct sk_buff *skb, struct net_device *dev,
 
 err_dma:
 	do {
-		tx_buf = mtk_desc_to_tx_buf(ring, txd);
+		tx_buf = mtk_desc_to_tx_buf(ring, itxd);
 
 		/* unmap dma */
 		mtk_tx_unmap(&dev->dev, tx_buf);
-- 
1.7.10.4

[PATCH 05/12] net: mediatek: dropped rx packets are not being counted properly

[John Crispin]

There are two places inside mtk_poll_rx where rx_dropped is not being
incremented properly. Fix this by adding the missing code to increment
the counter.

Signed-off-by: John Crispin <john-Pj+rj9U5foFAfugRpC6u6w@public.gmane.org>
---
 drivers/net/ethernet/mediatek/mtk_eth_soc.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
index 4f22ee9..0610262 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
@@ -810,6 +810,7 @@ static int mtk_poll_rx(struct napi_struct *napi, int budget,
 					  DMA_FROM_DEVICE);
 		if (unlikely(dma_mapping_error(&netdev->dev, dma_addr))) {
 			skb_free_frag(new_data);
+			netdev->stats.rx_dropped++;
 			goto release_desc;
 		}
 
@@ -817,6 +818,7 @@ static int mtk_poll_rx(struct napi_struct *napi, int budget,
 		skb = build_skb(data, ring->frag_size);
 		if (unlikely(!skb)) {
 			put_page(virt_to_head_page(new_data));
+			netdev->stats.rx_dropped++;
 			goto release_desc;
 		}
 		skb_reserve(skb, NET_SKB_PAD + NET_IP_ALIGN);
-- 
1.7.10.4

[PATCH 06/12] net: mediatek: add next data pointer coherency protection

[John Crispin]

The QDMA engine can fail to update the register pointing to the next TX
descriptor if this bit does not get set in the QDMA configuration register.
Not setting this bit can result in invalid values inside the TX rings
registers which will causes TX stalls.

Signed-off-by: John Crispin <john-Pj+rj9U5foFAfugRpC6u6w@public.gmane.org>
---
 drivers/net/ethernet/mediatek/mtk_eth_soc.c |    2 +-
 drivers/net/ethernet/mediatek/mtk_eth_soc.h |    1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
index 0610262..bfac376 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
@@ -1261,7 +1261,7 @@ static int mtk_start_dma(struct mtk_eth *eth)
 	mtk_w32(eth,
 		MTK_TX_WB_DDONE | MTK_RX_DMA_EN | MTK_TX_DMA_EN |
 		MTK_RX_2B_OFFSET | MTK_DMA_SIZE_16DWORDS |
-		MTK_RX_BT_32DWORDS,
+		MTK_RX_BT_32DWORDS | MTK_NDP_CO_PRO,
 		MTK_QDMA_GLO_CFG);
 
 	return 0;
diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.h b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
index 57f7e8a..a5eb7c6 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.h
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
@@ -91,6 +91,7 @@
 #define MTK_QDMA_GLO_CFG	0x1A04
 #define MTK_RX_2B_OFFSET	BIT(31)
 #define MTK_RX_BT_32DWORDS	(3 << 11)
+#define MTK_NDP_CO_PRO		BIT(10)
 #define MTK_TX_WB_DDONE		BIT(6)
 #define MTK_DMA_SIZE_16DWORDS	(2 << 4)
 #define MTK_RX_DMA_BUSY		BIT(3)
-- 
1.7.10.4

[PATCH 07/12] net: mediatek: disable all interrupts during probe

[John Crispin]

The current code only disables those IRQs that we will later use. To
ensure that we have a predefined state, we really want to disable all IRQs.
Change the code to disable all IRQs to achieve this.

Signed-off-by: John Crispin <john-Pj+rj9U5foFAfugRpC6u6w@public.gmane.org>
---
 drivers/net/ethernet/mediatek/mtk_eth_soc.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
index bfac376..93af4e3 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
@@ -1375,7 +1375,7 @@ static int __init mtk_hw_init(struct mtk_eth *eth)
 
 	/* disable delay and normal interrupt */
 	mtk_w32(eth, 0, MTK_QDMA_DELAY_INT);
-	mtk_irq_disable(eth, MTK_TX_DONE_INT | MTK_RX_DONE_INT);
+	mtk_irq_disable(eth, ~0);
 	mtk_w32(eth, RST_GL_PSE, MTK_RST_GL);
 	mtk_w32(eth, 0, MTK_RST_GL);
 
-- 
1.7.10.4

[PATCH 08/12] net: mediatek: fix threshold value

[John Crispin]

The logic to calculate the threshold value for stopping the TX queue is
bad. Currently it will always use 1/2 of the rings size, which is way too
much. Set the threshold to MAX_SKB_FRAGS. This makes sure that the queue
is stopped when there is not enough room to accept an additional segment. 

Signed-off-by: John Crispin <john@phrozen.org>
---
 drivers/net/ethernet/mediatek/mtk_eth_soc.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
index 93af4e3..8b289e1 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
@@ -1012,8 +1012,7 @@ static int mtk_tx_alloc(struct mtk_eth *eth)
 	atomic_set(&ring->free_count, MTK_DMA_SIZE - 2);
 	ring->next_free = &ring->dma[0];
 	ring->last_free = &ring->dma[MTK_DMA_SIZE - 2];
-	ring->thresh = max((unsigned long)MTK_DMA_SIZE >> 2,
-			      MAX_SKB_FRAGS);
+	ring->thresh = MAX_SKB_FRAGS;
 
 	/* make sure that all changes to the dma ring are flushed before we
 	 * continue
-- 
1.7.10.4


_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

[PATCH 09/12] net: mediatek: increase watchdog_timeo

[John Crispin]

During stress testing, after reducing the threshold value, we have seen
TX timeouts that were caused by the watchdog_timeo value being too low.
Increase the value to 5 * HZ which is a value commonly used by many other
drivers.

Signed-off-by: John Crispin <john-Pj+rj9U5foFAfugRpC6u6w@public.gmane.org>
---
 drivers/net/ethernet/mediatek/mtk_eth_soc.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
index 8b289e1..24714ab 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
@@ -1688,7 +1688,7 @@ static int mtk_add_mac(struct mtk_eth *eth, struct device_node *np)
 	mac->hw_stats->reg_offset = id * MTK_STAT_OFFSET;
 
 	SET_NETDEV_DEV(eth->netdev[id], eth->dev);
-	eth->netdev[id]->watchdog_timeo = HZ;
+	eth->netdev[id]->watchdog_timeo = 5 * HZ;
 	eth->netdev[id]->netdev_ops = &mtk_netdev_ops;
 	eth->netdev[id]->base_addr = (unsigned long)eth->base;
 	eth->netdev[id]->vlan_features = MTK_HW_FEATURES &
-- 
1.7.10.4

Re: [PATCH 09/12] net: mediatek: increase watchdog_timeo

[Andrew Lunn]

On Sun, Jun 05, 2016 at 08:33:02AM +0200, John Crispin wrote:
> During stress testing, after reducing the threshold value, we have seen
> TX timeouts that were caused by the watchdog_timeo value being too low.
> Increase the value to 5 * HZ which is a value commonly used by many other
> drivers.

I've never studied what watchdog_timeo actually means. Does it mean a
transmit has not completed in that amount of time? Would this imply
you have 5 seconds worth of packets in your transmit queue? Do you
know what the driver is doing during this 5 seconds?

Thanks
    Andrew

Re: [PATCH 09/12] net: mediatek: increase watchdog_timeo

[John Crispin]

On 05/06/2016 16:56, Andrew Lunn wrote:
> On Sun, Jun 05, 2016 at 08:33:02AM +0200, John Crispin wrote:
>> During stress testing, after reducing the threshold value, we have seen
>> TX timeouts that were caused by the watchdog_timeo value being too low.
>> Increase the value to 5 * HZ which is a value commonly used by many other
>> drivers.
> 
> I've never studied what watchdog_timeo actually means. Does it mean a
> transmit has not completed in that amount of time? Would this imply
> you have 5 seconds worth of packets in your transmit queue? Do you
> know what the driver is doing during this 5 seconds?

Hi Andrew,

it is waiting for the watchdog to trigger :-) TBH the 1s seems to be too
short to for the dma ring length to be flushed and i had to pick some
value and 5 is used most places.

it really depends on the amount of packets in the queue, their length
and the mac setting. the timeout needs to be large enough that it would
not trigger incorrectly even if the mac is on 10mbit half duplex and all
frames in the queue were maximum size.

	John

Re: [PATCH 09/12] net: mediatek: increase watchdog_timeo

[Andrew Lunn]

> Hi Andrew,
> 
> it is waiting for the watchdog to trigger :-) TBH the 1s seems to be too
> short to for the dma ring length to be flushed and i had to pick some
> value and 5 is used most places.
> 
> it really depends on the amount of packets in the queue, their length
> and the mac setting. the timeout needs to be large enough that it would
> not trigger incorrectly even if the mac is on 10mbit half duplex and all
> frames in the queue were maximum size.

So you are saying there is 5 seconds worth of traffic in the transmit ring.

As a general point, not specific to this driver, is that wise? Isn't
that really bad buffer bloat?

I just wondered what happened to cause it to have 5 seconds worth of
traffic in the transmit ring. Did downstream signal a pause?  But i
thought the byte queue limit was designed to prevent a big backlog in
the transmit queue? At 10/Half, is it not reacting fast enough?  Since
it is half duplex, do you have a lot of traffic coming the other way
and something is not being fair at distributing up and down traffic?

I'm just wondering if by increasing the watchdog to 5 seconds, you are
just hiding a problem.

    Andrew

Re: [PATCH 09/12] net: mediatek: increase watchdog_timeo

[John Crispin]

On 06/06/2016 14:21, Andrew Lunn wrote:
>> Hi Andrew,
>>
>> it is waiting for the watchdog to trigger :-) TBH the 1s seems to be too
>> short to for the dma ring length to be flushed and i had to pick some
>> value and 5 is used most places.
>>
>> it really depends on the amount of packets in the queue, their length
>> and the mac setting. the timeout needs to be large enough that it would
>> not trigger incorrectly even if the mac is on 10mbit half duplex and all
>> frames in the queue were maximum size.
> 
> So you are saying there is 5 seconds worth of traffic in the transmit ring.
> 
> As a general point, not specific to this driver, is that wise? Isn't
> that really bad buffer bloat?
> 
> I just wondered what happened to cause it to have 5 seconds worth of
> traffic in the transmit ring. Did downstream signal a pause?  But i
> thought the byte queue limit was designed to prevent a big backlog in
> the transmit queue? At 10/Half, is it not reacting fast enough?  Since
> it is half duplex, do you have a lot of traffic coming the other way
> and something is not being fair at distributing up and down traffic?
> 
> I'm just wondering if by increasing the watchdog to 5 seconds, you are
> just hiding a problem.
> 
>     Andrew

Hi Andrew,

running the driver without any QoS and using the typical ringsize for
gigabit devices, 1s is not enough. we were seeing false positive
watchdog events. then i grepped to see what other drivers do and most
set 5seconds. ideally the watchdog never triggers as the driver is
functional an does not suffer from deadlocks.

at gbit ethernet can transmit 83 packets that are 1500 bytes long /
second, if there are no pause gaps. at 10Mbit that would be 6 packets.

so assuming we have a ring of 128 and napi set to 64, we would want at
least 2 seconds, 3 if there are a lot of pause gaps, 4-5 if it is half
duplex ... so i took the value commonly used, which is 5 according to grep.

figuring out when the queue is stuck seems to be a little bit more
complicated. imho the trigger should not be based on how long it took to
send a packet, but how long since the last packet was dequeued from dma.

personally i'd rather fix the deadlocks that can happen, which is what
we did, than rely on the watchdog to reset the queue. right now we can
hammer the driver with several streams on both macs utilizing all 4 cpu
cores for several days without seeing any hickups.

		John

[PATCH 03/12] net: mediatek: fix missing free of scratch memory

[John Crispin]

Scratch memory gets allocated in mtk_init_fq_dma() but the corresponding
code to free it is missing inside mtk_dma_free() causing a memory leak.
With this patch applied, we can run ifconfig up/down several thousand
times without any problems.

Signed-off-by: John Crispin <john@phrozen.org>
---
 drivers/net/ethernet/mediatek/mtk_eth_soc.c |   18 +++++++++++++-----
 drivers/net/ethernet/mediatek/mtk_eth_soc.h |    2 ++
 2 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
index 6780886..8d79bbf 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
@@ -453,14 +453,14 @@ static inline void mtk_rx_get_desc(struct mtk_rx_dma *rxd,
 /* the qdma core needs scratch memory to be setup */
 static int mtk_init_fq_dma(struct mtk_eth *eth)
 {
-	dma_addr_t phy_ring_head, phy_ring_tail;
+	dma_addr_t phy_ring_tail;
 	int cnt = MTK_DMA_SIZE;
 	dma_addr_t dma_addr;
 	int i;
 
 	eth->scratch_ring = dma_alloc_coherent(eth->dev,
 					       cnt * sizeof(struct mtk_tx_dma),
-					       &phy_ring_head,
+					       &eth->phy_scratch_ring,
 					       GFP_ATOMIC | __GFP_ZERO);
 	if (unlikely(!eth->scratch_ring))
 		return -ENOMEM;
@@ -477,19 +477,19 @@ static int mtk_init_fq_dma(struct mtk_eth *eth)
 		return -ENOMEM;
 
 	memset(eth->scratch_ring, 0x0, sizeof(struct mtk_tx_dma) * cnt);
-	phy_ring_tail = phy_ring_head +
+	phy_ring_tail = eth->phy_scratch_ring +
 			(sizeof(struct mtk_tx_dma) * (cnt - 1));
 
 	for (i = 0; i < cnt; i++) {
 		eth->scratch_ring[i].txd1 =
 					(dma_addr + (i * MTK_QDMA_PAGE_SIZE));
 		if (i < cnt - 1)
-			eth->scratch_ring[i].txd2 = (phy_ring_head +
+			eth->scratch_ring[i].txd2 = (eth->phy_scratch_ring +
 				((i + 1) * sizeof(struct mtk_tx_dma)));
 		eth->scratch_ring[i].txd3 = TX_DMA_SDL(MTK_QDMA_PAGE_SIZE);
 	}
 
-	mtk_w32(eth, phy_ring_head, MTK_QDMA_FQ_HEAD);
+	mtk_w32(eth, eth->phy_scratch_ring, MTK_QDMA_FQ_HEAD);
 	mtk_w32(eth, phy_ring_tail, MTK_QDMA_FQ_TAIL);
 	mtk_w32(eth, (cnt << 16) | cnt, MTK_QDMA_FQ_CNT);
 	mtk_w32(eth, MTK_QDMA_PAGE_SIZE << 16, MTK_QDMA_FQ_BLEN);
@@ -1189,6 +1189,14 @@ static void mtk_dma_free(struct mtk_eth *eth)
 	for (i = 0; i < MTK_MAC_COUNT; i++)
 		if (eth->netdev[i])
 			netdev_reset_queue(eth->netdev[i]);
+	if (eth->scratch_ring) {
+		dma_free_coherent(eth->dev,
+				  MTK_DMA_SIZE * sizeof(struct mtk_tx_dma),
+				  eth->scratch_ring,
+				  eth->phy_scratch_ring);
+		eth->scratch_ring = NULL;
+		eth->phy_scratch_ring = 0;
+	}
 	mtk_tx_clean(eth);
 	mtk_rx_clean(eth);
 	kfree(eth->scratch_head);
diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.h b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
index eed626d..57f7e8a 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.h
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
@@ -357,6 +357,7 @@ struct mtk_rx_ring {
  * @rx_ring:		Pointer to the memore holding info about the RX ring
  * @rx_napi:		The NAPI struct
  * @scratch_ring:	Newer SoCs need memory for a second HW managed TX ring
+ * @phy_scratch_ring:	physical address of scratch_ring
  * @scratch_head:	The scratch memory that scratch_ring points to.
  * @clk_ethif:		The ethif clock
  * @clk_esw:		The switch clock
@@ -384,6 +385,7 @@ struct mtk_eth {
 	struct mtk_rx_ring		rx_ring;
 	struct napi_struct		rx_napi;
 	struct mtk_tx_dma		*scratch_ring;
+	dma_addr_t			phy_scratch_ring;
 	void				*scratch_head;
 	struct clk			*clk_ethif;
 	struct clk			*clk_esw;
-- 
1.7.10.4

[PATCH 10/12] net: mediatek: fix off by one in the TX ring allocation

[John Crispin]

The TX ring setup has an off by one error causing it to not utilise all
descriptors. This has the side effect that we need to reset the next
pointer at runtime to make it work. Fix the off by one and remove the
code fixing the ring at runtime.

Signed-off-by: John Crispin <john@phrozen.org>
---
 drivers/net/ethernet/mediatek/mtk_eth_soc.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
index 24714ab..6daf48b 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
@@ -903,7 +903,6 @@ static int mtk_poll_tx(struct mtk_eth *eth, int budget, bool *tx_again)
 		}
 		mtk_tx_unmap(eth->dev, tx_buf);
 
-		ring->last_free->txd2 = next_cpu;
 		ring->last_free = desc;
 		atomic_inc(&ring->free_count);
 
@@ -1011,7 +1010,7 @@ static int mtk_tx_alloc(struct mtk_eth *eth)
 
 	atomic_set(&ring->free_count, MTK_DMA_SIZE - 2);
 	ring->next_free = &ring->dma[0];
-	ring->last_free = &ring->dma[MTK_DMA_SIZE - 2];
+	ring->last_free = &ring->dma[MTK_DMA_SIZE - 1];
 	ring->thresh = MAX_SKB_FRAGS;
 
 	/* make sure that all changes to the dma ring are flushed before we
-- 
1.7.10.4

[PATCH 11/12] net: mediatek: only wake the queue if it is stopped

[John Crispin]

The current code unconditionally wakes up the queue at the end of each
tx_poll action. Change the code to only wake up the queues if any of
them have actually been stopped before.

Signed-off-by: John Crispin <john@phrozen.org>
---
 drivers/net/ethernet/mediatek/mtk_eth_soc.c |   17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
index 6daf48b..40d3cfd 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
@@ -685,6 +685,20 @@ static inline int mtk_cal_txd_req(struct sk_buff *skb)
 	return nfrags;
 }
 
+static int mtk_queue_stopped(struct mtk_eth *eth)
+{
+	int i;
+
+	for (i = 0; i < MTK_MAC_COUNT; i++) {
+		if (!eth->netdev[i])
+			continue;
+		if (netif_queue_stopped(eth->netdev[i]))
+			return 1;
+	}
+
+	return 0;
+}
+
 static void mtk_wake_queue(struct mtk_eth *eth)
 {
 	int i;
@@ -929,7 +943,8 @@ static int mtk_poll_tx(struct mtk_eth *eth, int budget, bool *tx_again)
 	if (!total)
 		return 0;
 
-	if (atomic_read(&ring->free_count) > ring->thresh)
+	if (mtk_queue_stopped(eth) &&
+	    (atomic_read(&ring->free_count) > ring->thresh))
 		mtk_wake_queue(eth);
 
 	return total;
-- 
1.7.10.4

[PATCH 12/12] net: mediatek: remove superfluous queue wake up call

[John Crispin]

The code checks if the queue should be stopped because we are below the
threshold of free descriptors only to check if it should be started again.
If we do end up in a state where we are at the threshold limit, it makes
more sense to just stop the queue and wait for the next IRQ to trigger the
TX housekeeping again. There is no rush in enqueuing the next packet, it
needs to wait for all the others in the queue to be dispatched first
anyway.

Signed-off-by: John Crispin <john@phrozen.org>
---
 drivers/net/ethernet/mediatek/mtk_eth_soc.c |    7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
index 40d3cfd..ebfca7d 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
@@ -764,12 +764,9 @@ static int mtk_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	if (mtk_tx_map(skb, dev, tx_num, ring, gso) < 0)
 		goto drop;
 
-	if (unlikely(atomic_read(&ring->free_count) <= ring->thresh)) {
+	if (unlikely(atomic_read(&ring->free_count) <= ring->thresh))
 		mtk_stop_queue(eth);
-		if (unlikely(atomic_read(&ring->free_count) >
-			     ring->thresh))
-			mtk_wake_queue(eth);
-	}
+
 	spin_unlock_irqrestore(&eth->page_lock, flags);
 
 	return NETDEV_TX_OK;
-- 
1.7.10.4