On 1/11/07, Jarek Poplawski <[EMAIL PROTECTED]> wrote:

PS: alas I didn't even check compiling - I had no time to
find all compile dependencies of this driver
---

Signed-off-by: Jarek Poplawski <[EMAIL PROTECTED]>
---

diff -Nurp linux-2.6.20-rc4-/drivers/net/mv643xx_eth.c 
linux-2.6.20-rc4/drivers/net/mv643xx_eth.c
--- linux-2.6.20-rc4-/drivers/net/mv643xx_eth.c 2006-12-18 08:57:52.000000000 
+0100
+++ linux-2.6.20-rc4/drivers/net/mv643xx_eth.c  2007-01-11 08:55:34.000000000 
+0100
@@ -312,8 +312,8 @@ int mv643xx_eth_free_tx_descs(struct net
        int count;
        int released = 0;

+       spin_lock_irqsave(&mp->lock, flags);
        while (mp->tx_desc_count > 0) {
-               spin_lock_irqsave(&mp->lock, flags);
                tx_index = mp->tx_used_desc_q;
                desc = &mp->p_tx_desc_area[tx_index];
                cmd_sts = desc->cmd_sts;
@@ -348,8 +348,10 @@ int mv643xx_eth_free_tx_descs(struct net
                        dev_kfree_skb_irq(skb);

Hmm, I think this is guaranteed not to work. In between those lines
the lock is released, while data in the mp structure is still being
accessed. It seems that this bit of code is indeed not race-safe
though, I'm gonna try to figure something.

                released = 1;
+               spin_lock_irqsave(&mp->lock, flags);
        }

+       spin_unlock_irqrestore(&mp->lock, flags);
        return released;
 }

Ugh, this is really unclean... Taking a lock "for nothing" like that
has a perf cost.

HTH

T-Bone

--
Thibaut VARENE
http://www.parisc-linux.org/~varenet/
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to