Re: [PATCH 2.6.14.3] Necessary evil to get sata_vsc to initialize with Intel iq3124h hba

[Jeremy Higdon <jeremy@sgi.com>]

On Tue, Dec 27, 2005 at 12:27:57PM -0700, Dan Williams wrote:
> On 12/18/05, Jeremy Higdon <jeremy@sgi.com> wrote:
> >
> > I don't think I fully understand the problem you're trying to solve, but
> > the gist seems to be that you're taking endless interrupts even when
> > ATA_NIEN is set.
> Yes...
> 
> > So you add the dummy ata_chk_status call to clear
> > interrupts that the libata core isn't interested in.  But ata_chk_status
> > doesn't actually clear it unless you first write to the SCR_ERROR
> > register above?
> 
> Actually I added the dummy ata_chk_status because just clearing the
> SCR_ERROR bits did not stop the stream of interrupts.  Upon further
> investigation it appears that clearing the SCR_ERRORS is not
> _required_ and all that is necessary is the ata_chk_status() call. 
> however not clearing the SCR_ERROR results in the error interrupt
> recurring everytime the disk is accessed.
> 
> For example:
> vsc_sata_interrupt: clearing interrupt, status 30383; sata err status 50002
> vsc_sata_interrupt: clearing interrupt, status 30383; sata err status 50002
> Adding 136512k swap on /dev/sda1.  Priority:-1 extents:1 across:136512k
> vsc_sata_interrupt: clearing interrupt, status 38303; sata err status 50002
> vsc_sata_interrupt: clearing interrupt, status 38303; sata err status 50002
> Adding 125456k swap on /dev/sdb1.  Priority:-2 extents:1 across:125456k
> vsc_sata_interrupt: clearing interrupt, status 830303; sata err status 50002
> vsc_sata_interrupt: clearing interrupt, status 830303; sata err status 50002
> Adding 136512k swap on /dev/sdc1.  Priority:-3 extents:1 across:136512k
> 
> > FWIW, on my Vitesse chip, the int_status and SCR_ERROR bits that are set and
> > when this code is called this code are 0x83 and 0x50002, respectively.  But
> > not clearing them doesn't seem to cause a problem . . . this must be different
> > behavior in your chip (presumably the Intel branch).
> 
> Yes, with your patch I get the same output, perhaps the Vitesse part
> does not tie these bits to the error interrupt, or they are masked by
> default?

Dan, please trim trailing whitespace from your patch, if you would.  Other
than that, your patch to sata_vsc.c and Kconfig looks fine to me.  I'm not sure
about the patch to libata-scsi.c, but jgarzik can review that one.

I checked both the Intel and Vitesse documentation, and they both indicate
that the error interrupts are masked by default.  Perhaps your PROM/BIOS
software is enabling them.  Ours does not, and the sata_vsc driver doesn't
either.  Perhaps it should -- though since we're just ignoring them, that
doesn't make much sense.  Maybe we should explicitly mask them off?  Or
explicitly enable them, rather than leaving the chip in whatever state
the PROM/BIOS leaves it.  Perhaps this patch is all you really need.
Can you try it?

--- linux-2.6.15/drivers/scsi/sata_vsc.c	2006-01-02 19:21:10.000000000 -0800
+++ linux-2.6.15-mask/drivers/scsi/sata_vsc.c	2006-01-04 23:55:16.000000000 -0800
@@ -108,9 +108,9 @@
 		VSC_SATA_INT_MASK_OFFSET + ap->port_no;
 	mask = readb(mask_addr);
 	if (ctl & ATA_NIEN)
-		mask |= 0x80;
+		mask = 0x80;
 	else
-		mask &= 0x7F;
+		mask = 0;
 	writeb(mask, mask_addr);
 }
 

jeremy