[ sorry, for the record I'm not trimming Val's comments - mine are much below]
On Wed, Jun 21, 2006 at 05:43:40PM -0700, Valerie Henson wrote:
...
> Hi folks,
>
> The quick summary of my thoughts on this patch is that it isn't the
> ideal patch, but it works and it's well-tested. Doing my preferred
> fix (adding a shutdown flag) would be invasive and take many weeks to
> reach the level of stability of Grant's patch. So I'm taking this
> patch but adding a comment describing my preferred fix.
>
> Here's the long version. The obvious ordering of bringing up the card
> is:
>
> request_irq()
> setup DMA resources
> enable interrupts
> start DMA engine
The problem here is request_irq() enables interrupts.
SO the third step happens as part of the first step.
And if an IRQ happens to be pending, it will blow up.
But since we reset the chip at init time, that should
never happen.
>
> And the obvious ordering of shutting it down is:
>
> stop DMA engine
> disable interrupts
> remove DMA resources
> free_irq()
This attempts to be symetrical with the init sequence and
I like that approach where possible. It's not symetrical
in practice because the interrupt code restarts DMA.
(which is what you've noted below)
> The problem with the above shutdown order is that we can receive an
> interrupt in between stopping DMA and disabling interrupts, and the
> tulip irq handler will reenable DMA =><= Boom! The solution I prefer
> is to make the irq handler aware of whether we are shutting down or
> not, and not reenable DMA in that case.
While I believe this will work too, I don't advocate this approach
because I don't like to add code to interrupt handlers unless
it's the _only_ way to fix a problem.
> However, it is a non-trivial
> patch to get right, and I would rather have the bug fixed short-term
> with the ordering Grant uses:
>
> disable interrupts
> free_irq()
> stop rxtx
> remove DMA resources
>
> Make sense to everyone? I'll redo the patch with the comment and get
> Grant's sign-off.
Yes. I'm ok with anything similar to what you posted above:
Signed-off-by: Grant Grundler <[EMAIL PROTECTED]>
And Val, thanks for accepting the patch and taking the time to
explain where you want to go next with the code in this case.
I'll be happy to setup remote access to my test environment when
you are ready to test that next step.
thanks,
grant
>
> -VAL
-
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
