linux-kernel - Re: [PATCH] m68k/mac: Replace macide driver with generic platform driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <3de9a93d-2f7-a650-1fa-c2129a4a765b@nippy.intranet>
Date:   Mon, 26 Apr 2021 17:35:45 +1000 (AEST)
From:   Finn Thain <fthain@...tmail.com.au>
To:     Michael Schmitz <schmitzmic@...il.com>
cc:     Finn Thain <fthain@...egraphics.com.au>,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        Christoph Hellwig <hch@....de>,
        Joshua Thompson <funaho@...ai.org>,
        "David S. Miller" <davem@...emloft.net>,
        linux-m68k@...ts.linux-m68k.org, linux-kernel@...r.kernel.org,
        linux-ide@...r.kernel.org
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@...il.com>
> 

Thanks for your review.