Re: patch "libata-sff: Fix oops for pnp devices with no ctl" causes regression

[Nick Piggin <npiggin@suse.de>]

On Thu, Jun 05, 2008 at 12:21:29PM +0200, Nick Piggin wrote:
> On Thu, Jun 05, 2008 at 10:24:24AM +0100, Alan Cox wrote:
> > > And when booting with the commit applied, I instead get a whole lot of
> > > messages like this (this is the first one, copied by hand):
> > > 
> > > ata2.00: exception Emask 0x0 SAct 0x0 Serr 0x10000000 action 0x6 frozen
> > > ata2: SError: { }
> > > ata2.00: cmd c8/00:02:42:08:20/00:00:00:00:00/e0 tag 0 dma 1024 in
> > >          res 40/00:00:00:00:00/00:00:00:00:00/00 Emask 0x4 (timeout)
> > > ata2.00: status: { DRDY }
> > > ata2: hard resetting link
> > > ata2: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
> > > ata2.00: configured for UDMA/133
> > > ata2: EH complete
> > 
> > Well I've been over the patch twice now and I cannot see a single point
> > at which the sequence of code that *should* be executed is any different.
> 
> If it is of any help to you, doing this:
> diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
> index 90d20c6..f6cb8d7 100644
> --- a/drivers/ata/libata-sff.c
> +++ b/drivers/ata/libata-sff.c
> @@ -1583,6 +1583,12 @@ inline unsigned int ata_sff_host_intr(struct ata_port *ap
>         if (status & ATA_BUSY)
>                 goto idle_irq;
> 
> +       /* check main status, clearing INTRQ */
> +       status = ap->ops->sff_check_status(ap);
> +       if (unlikely(status & ATA_BUSY))
> +               goto idle_irq;
> +
> +
>         /* ack bmdma irq events */
>         ap->ops->sff_irq_clear(ap);
> 
> Gets it working again...
> 
> 
> > Stick wmb();rmb(); (or similar barriers to compiler optimisation and I/O
> > fencing) at the start and end of your ata_sff_altstatus() and see what
> > happens, if it suddenly decides to behave or forcing it no inline makes
> > it behave then that would be useful info.
> 
> Will give that a try next

And doing this made no difference

diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
index 90d20c6..db6be15 100644
--- a/drivers/ata/libata-sff.c
+++ b/drivers/ata/libata-sff.c
@@ -249,10 +249,18 @@ u8 ata_sff_check_status(struct ata_port *ap)
  */
 static u8 ata_sff_altstatus(struct ata_port *ap)
 {
+       u8 ret;
+
+       mb();
+
        if (ap->ops->sff_check_altstatus)
-               return ap->ops->sff_check_altstatus(ap);
+               ret = ap->ops->sff_check_altstatus(ap);
+
+       ret = ioread8(ap->ioaddr.altstatus_addr);

-       return ioread8(ap->ioaddr.altstatus_addr);
+       mb();
+
+       return ret;
 }

 /**