Re: Broadcom bge(4) panics while shutting down

On Thu, May 14, 2009 at 6:52 PM, Xin LI <delphij_at_delphij.net> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Hi, Alexander,
>
> Alexander Sack wrote:
> [...]
>>> _at__at_ -3193,6 +3193,9 _at__at_ bge_rxeof(struct bge_softc *sc)
>>>                BGE_UNLOCK(sc);
>>>                (*ifp->if_input)(ifp, m);
>>>                BGE_LOCK(sc);
>>> +
>>> +               if (!(ifp->if_drv_flags & IFF_DRV_RUNNING))
>>> +                       return;
>>>        }
>>
>> Xin this looks fine by me, I actually put this up in the while loop as
>> I mentioned before which I think is functionality equivalent (can you
>> gain some optimizations by putting in the while loop though compiler
>> wise than a separate compilation unit?).
>
> I think the two is not semantically the same...  For this case an
> explicit 'return' would mean that no further actions, say the things
> right after the while loop, would be taken.  In my opinion that this is
> better since there is no protection over these DMA maps (which could
> have been released elsewhere).

Oh snap, I had a brain fart.  Ugh.  You are correct Xin.  Oh man, I
even said in my first post I want to AVOID touching the DMA maps
and/or hardware registers (notice the access in the jumbo case) as
soon as IFF_DRV_RUNNING gets reset.  I like the explicit return much
better that's why I had it that way but as John pointed out, tis
better to do it where you put it which is right after reacquiring the
lock.

> Note that I'm still not quite confident about the logic, we might just
> narrowed but not closed the race completely - these
> BGE_LOCK()/UNLOCK()'s could hit some problem if one thread has done
> bge_stop() very quickly.  That would require more work, though, I don't
> have a very good plan at this moment...

This has been a constant problem that I've seen in the labs.  I've
seen this with polling as well as tuning the number of RING
descriptors (there is a thread I had a while back ago about this when
I was dealing with BPF drops and bge(4)).  I think the patch as it
stands though will suffice for the majority of cases and again I thank
you and John for addressing it in short-order.

-aps