Messages in this thread | | | Date | Mon, 26 Apr 2021 17:35:45 +1000 (AEST) | From | Finn Thain <> | Subject | Re: [PATCH] m68k/mac: Replace macide driver with generic platform driver |
| |
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@gmail.com> >
Thanks for your review.
| |