[PATCH net-next-2.6 v6 04/20] can: EG20T PCH: Add Tx Flow Control

[PATCH net-next-2.6 v6 04/20] can: EG20T PCH: Add Tx Flow Control

[Tomoya MORINAGA]

Currently, there is no flow control processing.
Thus, Add flow control processing as
when there is no empty of tx buffer,
netif_stop_queue is called.
When there is empty buffer, netif_wake_queue is called.

Signed-off-by: Tomoya MORINAGA <tomoya-linux@dsn.okisemi.com>
---
 drivers/net/can/pch_can.c |   27 ++++++++-------------------
 1 files changed, 8 insertions(+), 19 deletions(-)

diff --git a/drivers/net/can/pch_can.c b/drivers/net/can/pch_can.c
index 2d4ab0f..8686d93 100644
--- a/drivers/net/can/pch_can.c
+++ b/drivers/net/can/pch_can.c
@@ -899,6 +899,8 @@ MSG_OBJ:
 			dlc = 8;
 		stats->tx_bytes += dlc;
 		stats->tx_packets++;
+		if (int_stat == PCH_TX_OBJ_END)
+			netif_wake_queue(ndev);
 	}
 
 	int_stat = pch_can_int_pending(priv);
@@ -1037,18 +1039,6 @@ static int pch_close(struct net_device *ndev)
 	return 0;
 }
 
-static int pch_get_msg_obj_sts(struct net_device *ndev, u32 obj_id)
-{
-	u32 buffer_status = 0;
-	struct pch_can_priv *priv = netdev_priv(ndev);
-
-	/* Getting the message object status. */
-	buffer_status = (u32) pch_can_get_buffer_status(priv);
-
-	return buffer_status & obj_id;
-}
-
-
 static netdev_tx_t pch_xmit(struct sk_buff *skb, struct net_device *ndev)
 {
 	int i, j;
@@ -1060,17 +1050,16 @@ static netdev_tx_t pch_xmit(struct sk_buff *skb, struct net_device *ndev)
 	if (can_dropped_invalid_skb(ndev, skb))
 		return NETDEV_TX_OK;
 
-	if (priv->tx_obj == PCH_TX_OBJ_END) { /* Point tail Obj */
-		while (pch_get_msg_obj_sts(ndev, (((1 << PCH_TX_OBJ_NUM)-1) <<
-					   PCH_RX_OBJ_NUM)))
-			udelay(500);
+	if (priv->tx_obj == PCH_TX_OBJ_END) {
+		if (ioread32(&priv->regs->treq2) & 0xfc00)
+			netif_stop_queue(ndev);
 
-		priv->tx_obj = PCH_TX_OBJ_START; /* Point head of Tx Obj ID */
-		tx_buffer_avail = priv->tx_obj; /* Point Tail of Tx Obj */
+		tx_buffer_avail = priv->tx_obj;
+		priv->tx_obj = PCH_TX_OBJ_START;
 	} else {
 		tx_buffer_avail = priv->tx_obj;
+		priv->tx_obj++;
 	}
-	priv->tx_obj++;
 
 	/* Attaining the lock. */
 	spin_lock_irqsave(&priv->msgif_reg_lock, flags);
-- 
1.6.0.6

Re: [PATCH net-next-2.6 v6 04/20] can: EG20T PCH: Add Tx Flow Control

[David Miller]

From: Tomoya MORINAGA <tomoya-linux@dsn.okisemi.com>
Date: Tue, 30 Nov 2010 13:18:52 +0900

> Currently, there is no flow control processing.
> Thus, Add flow control processing as
> when there is no empty of tx buffer,
> netif_stop_queue is called.
> When there is empty buffer, netif_wake_queue is called.
> 
> Signed-off-by: Tomoya MORINAGA <tomoya-linux@dsn.okisemi.com>

When implementing functionality like this it is better to use other
existing well tested network drivers as a guide rather then trying
to be unique and clever, as you are doing here.

First of all, your netif_wake_queue() call is racy.  Because another
thread can be queueing up packets, fill the queue, and execute a
stop queue right when you have made the decision to invoke
netif_wake_queue().

Second of all, checking the state of the device to determine if a
stop queue should be performed has two problems:

1) The test uses a magic constant mask, which is undocumented.

2) It causes the race you have on the wake queue side

Use pure software state to guide your actions, and let the hardware
interrupt trigger the wake queue.

Also, you don't implement this as a true ring buffer, you only
consider to stop the queue when you hit the last TX object entry.  But
all the previous slots could be available.

Your head and tail pointer need to be maintained by advancing the
head pointer only during pch_xmit(), and advancing the tail pointer
only in the NAPI code as you get indications from the hardware.

Then, after the NAPI TX code advances the tail pointer, you see if
1) the queue is stopped and 2) TX space is now available.  If both
are true you wake the queue.

Use a well tested and mature driver like drivers/net/tg3.c as a
guide.  Search for netif_tx_{stop,wake}_queue().

Re: [PATCH net-next-2.6 v6 04/20] can: EG20T PCH: Add Tx Flow Control

[David Miller]

From: Tomoya MORINAGA <tomoya-linux-ECg8zkTtlr0C6LszWs/t0g@public.gmane.org>
Date: Tue, 30 Nov 2010 13:18:52 +0900

> Currently, there is no flow control processing.
> Thus, Add flow control processing as
> when there is no empty of tx buffer,
> netif_stop_queue is called.
> When there is empty buffer, netif_wake_queue is called.
> 
> Signed-off-by: Tomoya MORINAGA <tomoya-linux-ECg8zkTtlr0C6LszWs/t0g@public.gmane.org>

When implementing functionality like this it is better to use other
existing well tested network drivers as a guide rather then trying
to be unique and clever, as you are doing here.

First of all, your netif_wake_queue() call is racy.  Because another
thread can be queueing up packets, fill the queue, and execute a
stop queue right when you have made the decision to invoke
netif_wake_queue().

Second of all, checking the state of the device to determine if a
stop queue should be performed has two problems:

1) The test uses a magic constant mask, which is undocumented.

2) It causes the race you have on the wake queue side

Use pure software state to guide your actions, and let the hardware
interrupt trigger the wake queue.

Also, you don't implement this as a true ring buffer, you only
consider to stop the queue when you hit the last TX object entry.  But
all the previous slots could be available.

Your head and tail pointer need to be maintained by advancing the
head pointer only during pch_xmit(), and advancing the tail pointer
only in the NAPI code as you get indications from the hardware.

Then, after the NAPI TX code advances the tail pointer, you see if
1) the queue is stopped and 2) TX space is now available.  If both
are true you wake the queue.

Use a well tested and mature driver like drivers/net/tg3.c as a
guide.  Search for netif_tx_{stop,wake}_queue().

Re: [PATCH net-next-2.6 v6 04/20] can: EG20T PCH: Add Tx Flow Control

[Tomoya MORINAGA]

On Friday, December 03, 2010 6:12 AM, David Miller wrote :

> 
> When implementing functionality like this it is better to use other
> existing well tested network drivers as a guide rather then trying
> to be unique and clever, as you are doing here.

Sure. I will do so.

> 
> Second of all, checking the state of the device to determine if a
> stop queue should be performed has two problems:
> 
> 1) The test uses a magic constant mask, which is undocumented.

Do you above "magic constant" mean "0xfc00" ?
If so, this constant is replaced to MACRO in 08/20 patch. 

> 
> 2) It causes the race you have on the wake queue side
> 
> Use pure software state to guide your actions, and let the hardware
> interrupt trigger the wake queue.
> 
> Also, you don't implement this as a true ring buffer, you only
> consider to stop the queue when you hit the last TX object entry.  But
> all the previous slots could be available.

We can't use ring buffer.
Because ring buffer causes issue of tx packet out-of-order.

I show EG20T CAN HW spec below
According to priority, EG20T CAN HW sends packet from message object.
The priority is Message object number.
Lower number is high-priority, Higher is low priority

Thus, I think we can't use ring buffer.

Still, do you think I can use ring buffer ?
------
Thanks,

Tomoya MORINAGA
OKI SEMICONDUCTOR CO., LTD.

[PATCH net-next-2.6 v6 04/20] can: EG20T PCH: Add Tx Flow Control

[Tomoya MORINAGA]

Currently, there is no flow control processing.
Thus, Add flow control processing as
when there is no empty of tx buffer,
netif_stop_queue is called.
When there is empty buffer, netif_wake_queue is called.

Signed-off-by: Tomoya MORINAGA <tomoya-linux@dsn.okisemi.com>
---
 drivers/net/can/pch_can.c |   27 ++++++++-------------------
 1 files changed, 8 insertions(+), 19 deletions(-)

diff --git a/drivers/net/can/pch_can.c b/drivers/net/can/pch_can.c
index 2d4ab0f..8686d93 100644
--- a/drivers/net/can/pch_can.c
+++ b/drivers/net/can/pch_can.c
@@ -899,6 +899,8 @@ MSG_OBJ:
 			dlc = 8;
 		stats->tx_bytes += dlc;
 		stats->tx_packets++;
+		if (int_stat == PCH_TX_OBJ_END)
+			netif_wake_queue(ndev);
 	}
 
 	int_stat = pch_can_int_pending(priv);
@@ -1037,18 +1039,6 @@ static int pch_close(struct net_device *ndev)
 	return 0;
 }
 
-static int pch_get_msg_obj_sts(struct net_device *ndev, u32 obj_id)
-{
-	u32 buffer_status = 0;
-	struct pch_can_priv *priv = netdev_priv(ndev);
-
-	/* Getting the message object status. */
-	buffer_status = (u32) pch_can_get_buffer_status(priv);
-
-	return buffer_status & obj_id;
-}
-
-
 static netdev_tx_t pch_xmit(struct sk_buff *skb, struct net_device *ndev)
 {
 	int i, j;
@@ -1060,17 +1050,16 @@ static netdev_tx_t pch_xmit(struct sk_buff *skb, struct net_device *ndev)
 	if (can_dropped_invalid_skb(ndev, skb))
 		return NETDEV_TX_OK;
 
-	if (priv->tx_obj == PCH_TX_OBJ_END) { /* Point tail Obj */
-		while (pch_get_msg_obj_sts(ndev, (((1 << PCH_TX_OBJ_NUM)-1) <<
-					   PCH_RX_OBJ_NUM)))
-			udelay(500);
+	if (priv->tx_obj == PCH_TX_OBJ_END) {
+		if (ioread32(&priv->regs->treq2) & 0xfc00)
+			netif_stop_queue(ndev);
 
-		priv->tx_obj = PCH_TX_OBJ_START; /* Point head of Tx Obj ID */
-		tx_buffer_avail = priv->tx_obj; /* Point Tail of Tx Obj */
+		tx_buffer_avail = priv->tx_obj;
+		priv->tx_obj = PCH_TX_OBJ_START;
 	} else {
 		tx_buffer_avail = priv->tx_obj;
+		priv->tx_obj++;
 	}
-	priv->tx_obj++;
 
 	/* Attaining the lock. */
 	spin_lock_irqsave(&priv->msgif_reg_lock, flags);
-- 
1.6.0.6