Re: [PATCH] Re: kernel BUG in eth_alloc_tx_desc_index at drivers/net/mv643xx_eth.c:1069!

["Thibaut VARENE" <T-Bone@parisc-linux.org>]

On 1/11/07, Jarek Poplawski <jarkao2@o2.pl> 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 <jarkao2@o2.pl>
> ---
>
> 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/