On Mon, 26 Apr 2021, Michael Schmitz wrote:
Am 25.04.2021 um 21:06 schrieb Finn Thain:
This was tested on my Quadra 630. I haven't tested it on my PowerBook 150
because I don't have a RAM adapter board for it.
Apparently, the hardware I tested doesn't need macide_clear_irq() or
macide_test_irq() -- if it did, the generic driver would not have worked.
It's possible that those routines are needed for the PowerBook 150 but
we can cross that bridge if and when we come to it.
BTW, macide_clear_irq() appears to suffer from a race condition. The write
to the interrupt flags register could easily have unintended side effects
as it may alter other flag bits. Fortunately, all of the other bits are
unused by Linux. Moreover, when tested on my Quadra 630, that assignment
(*ide_ifr &= ~0x20) was observed to have no effect on bit 5.
You are worried that the bit clear might not be done atomic?
The edge-triggered interrupt flag bits are usually cleared by writing 1 to
the flag bit. Under this scheme, writing a 0 to a flag bit has no effect.
The assignment statement here is trying to clear bit 5 by writing 0. But
what about the other bits that we're writing 0 to? Some of them may also
be flag bits, and they may have been asserted in between the load and
store. AFAICS this scheme just can't work for edge-triggered interrupts.
So perhaps this is a level-triggered interrupt?
Regarding the missing effect of clearing bit 5, I suspect this has never
before been tested rigorously (I don't remember ever using a Quadra
630).
The logic attempted to replicate what the MacOS IDE driver did.
Fair enough. Maybe we have found a bug in the MacOS IDE driver.
The Linux IDE driver has its own way to test and clear a port's
interrupt flag, so this extra code can quite probably go.
Thanks for cleaning this up!
Reviewed-by: Michael Schmitz <schmitzmic@xxxxxxxxx>
Thanks for your review.
