| Message ID | 4CF47B2C.4010507@dsn.okisemi.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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(). -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);
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(-)