Messages in this thread | ![/](/images/icornerl.gif) | | From | "Corbett, David" <> | Subject | RE: sis900 driver in 2.4.20 - "start_xmit" function - buffer full ? | Date | Fri, 4 Apr 2003 10:41:08 -0600 |
| |
I am looking at the sis900.c in the 2.4.20 kernel. I ran across something that doesn't look right. (Yes, I plan to fix it and test it, but I haven't had time to do that yet - I've been busy!!!) I thought that I would present the issue here. Perhaps the author of the code for this area will be able to shed some light on whether there is something wrong with this code (or something wrong with me?! :-)
It appears that in sis900_start_xmit(), we erroneously detect the transmit buffer full condition. Moreover, this function *always* thinks that the transmit buffer is full.
Here is the block from sis900_start_xmit, starting at line 1525:
sis_priv->cur_tx ++; index_cur_tx = sis_priv->cur_tx; index_dirty_tx = sis_priv->dirty_tx;
for (count_dirty_tx = 0; index_cur_tx != index_dirty_tx; index_dirty_tx++) count_dirty_tx ++;
if (index_cur_tx == index_dirty_tx) { /* dirty_tx is met in the cycle of cur_tx, buffer full */ sis_priv->tx_full = 1; netif_stop_queue(net_dev); } else if (count_dirty_tx < NUM_TX_DESC) { /* Typical path, tell upper layer that more transmission is possible */ netif_start_queue(net_dev); } else { /* buffer full, tell upper layer no more transmission */ sis_priv->tx_full = 1; netif_stop_queue(net_dev); }
==================================================================
Here is a copy of the block above, where I have added some big ugly comments:
static int sis900_start_xmit(struct sk_buff *skb, struct net_device *net_dev) { {snip...} sis_priv->cur_tx ++; index_cur_tx = sis_priv->cur_tx; index_dirty_tx = sis_priv->dirty_tx;
for (count_dirty_tx = 0; index_cur_tx != index_dirty_tx; index_dirty_tx++) count_dirty_tx ++; // MY BIG FAT UGLY COMMENTS: Here, we have completed the for loop. // That for loop made sure that index_cur_tx is equal to // index_dirty_tx. Thus, the condition in the "if" below will // always be true. The other clauses are never used. We // will never take the "Typical path". Isn't this a problem? if (index_cur_tx == index_dirty_tx) { /* dirty_tx is met in the cycle of cur_tx, buffer full */ sis_priv->tx_full = 1; netif_stop_queue(net_dev); } else if (count_dirty_tx < NUM_TX_DESC) { /* Typical path, tell upper layer that more transmission is possible */ netif_start_queue(net_dev); } else { /* buffer full, tell upper layer no more transmission */ sis_priv->tx_full = 1; netif_stop_queue(net_dev); }
==================================================================
I believe I know how this should be fixed; however, I will defer to the masters. I wanted to inform the community of this problem ASAP. I hope that this helps.
Regards,
David Corbett - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| ![\](/images/icornerr.gif) |