tg3 reset_task question

tg3 reset_task question

[Gagan Arneja]

Can't this BUG_ON be hit very easily:

static void tg3_irq_quiesce(struct tg3 *tp)
{
        BUG_ON(tp->irq_sync);
...
}

tg3_reset_task could easily be racing with another thread, that calls
tg3_full_lock(tp, 1); e.g tg3_change_mtu. Maybe I'm missing something
obvious.

--
Gagan

Re: tg3 reset_task question

[Gagan Arneja]

I like the counting semaphore idea.

--
Gagan

Michael Chan wrote:
> On Wed, 2007-04-04 at 13:34 -0700, Gagan Arneja wrote:
>> Can't this BUG_ON be hit very easily:
>>
>> static void tg3_irq_quiesce(struct tg3 *tp)
>> {
>>         BUG_ON(tp->irq_sync);
>> ...
>> }
>>
>> tg3_reset_task could easily be racing with another thread, that calls
>> tg3_full_lock(tp, 1); e.g tg3_change_mtu. Maybe I'm missing something
>> obvious.
>>
> 
> Yes, you're right.  Perhaps we should get the rtnl first before
> tg3_full_lock(), or turn irq_sync into an atomic counter that allows
> nesting.
> 

Re: tg3 reset_task question

[Michael Chan]

On Wed, 2007-04-04 at 13:34 -0700, Gagan Arneja wrote:
> Can't this BUG_ON be hit very easily:
> 
> static void tg3_irq_quiesce(struct tg3 *tp)
> {
>         BUG_ON(tp->irq_sync);
> ...
> }
> 
> tg3_reset_task could easily be racing with another thread, that calls
> tg3_full_lock(tp, 1); e.g tg3_change_mtu. Maybe I'm missing something
> obvious.
> 

Yes, you're right.  Perhaps we should get the rtnl first before
tg3_full_lock(), or turn irq_sync into an atomic counter that allows
nesting.

Re: tg3 reset_task question

[Stephen Hemminger]

On Wed, 04 Apr 2007 13:53:47 -0700
Gagan Arneja <gaagaan@gmail.com> wrote:

> I like the counting semaphore idea.
> 
> --
> Gagan
> 
> Michael Chan wrote:
> > On Wed, 2007-04-04 at 13:34 -0700, Gagan Arneja wrote:
> >> Can't this BUG_ON be hit very easily:
> >>
> >> static void tg3_irq_quiesce(struct tg3 *tp)
> >> {
> >>         BUG_ON(tp->irq_sync);
> >> ...
> >> }
> >>
> >> tg3_reset_task could easily be racing with another thread, that calls
> >> tg3_full_lock(tp, 1); e.g tg3_change_mtu. Maybe I'm missing something
> >> obvious.
> >>
> > 
> > Yes, you're right.  Perhaps we should get the rtnl first before
> > tg3_full_lock(), or turn irq_sync into an atomic counter that allows
> > nesting.

When you start reinventing windows locks or the BKL, you know
you are going down the wrong path

Re: tg3 reset_task question

[Herbert Xu]

On Wed, Apr 04, 2007 at 10:10:07PM +0000, Stephen Hemminger wrote:
>
> > > Yes, you're right.  Perhaps we should get the rtnl first before
> > > tg3_full_lock(), or turn irq_sync into an atomic counter that allows
> > > nesting.
> 
> When you start reinventing windows locks or the BKL, you know
> you are going down the wrong path

Actually, I think what Michael's suggesting is quite different.
This would be a simple counter that tells the IRQ handlers to
not process any events.  So this isn't really a lock as such.

FWIW I think the counter sounds better than using the RTNL since
with the latter you'd have to figure out whether you're in an
RTNL context or not (e.g., tg3_suspend would also need to grab it).

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt