From owner-freebsd-current@FreeBSD.ORG Wed Jul 20 15:02:38 2011 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id EBC53106566B for ; Wed, 20 Jul 2011 15:02:38 +0000 (UTC) (envelope-from aboyer@averesystems.com) Received: from zimbra.averesystems.com (75-149-8-245-Pennsylvania.hfc.comcastbusiness.net [75.149.8.245]) by mx1.freebsd.org (Postfix) with ESMTP id 6E0E18FC12 for ; Wed, 20 Jul 2011 15:02:38 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by zimbra.averesystems.com (Postfix) with ESMTP id DFA2F8BC023; Wed, 20 Jul 2011 10:47:21 -0400 (EDT) X-Virus-Scanned: amavisd-new at averesystems.com Received: from zimbra.averesystems.com ([127.0.0.1]) by localhost (zimbra.averesystems.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id cTucudtWFQHR; Wed, 20 Jul 2011 10:47:15 -0400 (EDT) Received: from riven.arriad.com (fw.arriad.com [10.0.0.16]) by zimbra.averesystems.com (Postfix) with ESMTPSA id C2A988BC003; Wed, 20 Jul 2011 10:47:15 -0400 (EDT) From: Andrew Boyer Content-Type: multipart/mixed; boundary=Apple-Mail-10-5916955 Date: Wed, 20 Jul 2011 10:44:42 -0400 Message-Id: To: mav@freebsd.org Mime-Version: 1.0 (Apple Message framework v1084) X-Mailer: Apple Mail (2.1084) Cc: freebsd-current@freebsd.org, Jack Vogel Subject: [patch] Intel SATA controller hiccups, locking X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 20 Jul 2011 15:02:39 -0000 --Apple-Mail-10-5916955 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=us-ascii Hello Alexander, I am using the latest ata driver from stable/8 on a system with an Intel = ICH10 controller. ATA_CAM and ATA_STATIC_ID are both off. There is one = drive connected to port 3. SATA is set to Enhanced / IDE mode (not = AHCI) in the BIOS. > atapci0@pci0:0:31:2: class=3D0x01018f card=3D0x060d15d9 = chip=3D0x3a208086 rev=3D0x00 hdr=3D0x00 > atapci1@pci0:0:31:5: class=3D0x010185 card=3D0x060d15d9 = chip=3D0x3a268086 rev=3D0x00 hdr=3D0x00 > atapci0: port = 0xbff0-0xbff7,0xbf7c-0xbf7f,0xbfe0-0xbfe7,0xbef4-0xbef7,0xbfa0-0xbfaf,0xbf= 60-0xbf6f irq 19 at device 31.2 on pci0 > atapci0: Reserved 0x10 bytes for rid 0x20 type 4 at 0xbfa0 > atapci0: [MPSAFE] > atapci0: [ITHREAD] > atapci0: Reserved 0x10 bytes for rid 0x24 type 4 at 0xbf60 > ata2: on atapci0 > atapci0: Reserved 0x8 bytes for rid 0x10 type 4 at 0xbff0 > atapci0: Reserved 0x4 bytes for rid 0x14 type 4 at 0xbf7c > ata2: SATA reset: ports status=3D0x00 > ata2: p0: SATA connect timeout status=3D00000000 > ata2: p1: SATA connect timeout status=3D00000000 > ata2: [MPSAFE] > ata2: [ITHREAD] > ata3: on atapci0 > atapci0: Reserved 0x8 bytes for rid 0x18 type 4 at 0xbfe0 > atapci0: Reserved 0x4 bytes for rid 0x1c type 4 at 0xbef4 > ata3: SATA reset: ports status=3D0x08 > ata3: p0: SATA connect timeout status=3D00000000 > ata3: p1: SATA connect time=3D0ms status=3D00000123 > ata3: reset tp1 mask=3D03 ostat0=3D7f ostat1=3D50 > ata3: stat0=3D0x7f err=3D0x00 lsb=3D0xff msb=3D0xff > ata3: stat1=3D0x50 err=3D0x01 lsb=3D0x00 msb=3D0x00 > ata3: reset tp2 stat0=3D7f stat1=3D50 devices=3D0x2 > ata3: [MPSAFE] > ata3: [ITHREAD] When under heavy load, the 'atacontrol mode ad0' command sometimes fails = to determine the SATA speed; the drive appears to be missing. I think = the root cause is that chipsets/ata-intel.c does not do any locking on = the ata_intel_sata_sidpr_* routines. The (write address register) + = (access data register) model isn't safe without locking because two = channels share the registers. The ata_intel_sata_cscr_* routines have = the same problem. Adding a mutex to a structure stored in ctlr->chipset_data makes the = hiccups go away; see the attached patch. Please advise if this is = something you would like to fix. =20 Thank you, Andrew --Apple-Mail-10-5916955 Content-Disposition: attachment; filename=ata_intel_sata.diff Content-Type: application/octet-stream; x-unix-mode=0644; name="ata_intel_sata.diff" Content-Transfer-Encoding: 7bit --- sys/dev/ata/chipsets/ata-intel.c +++ sys/dev/ata/chipsets/ata-intel.c @@ -85,6 +85,18 @@ static void ata_intel_31244_reset(device_t dev); #define INTEL_6CH2 8 #define INTEL_ICH7 16 +struct ata_intel_data { + u_char smap[8]; /* was a void* */ + struct mtx lock; +}; + +#define ATA_INTEL_SMAP(_ctlr, _ch) \ + &((struct ata_intel_data *)(_ctlr->chipset_data))->smap[_ch->unit * 2] +#define ATA_INTEL_LOCK(_ctlr) \ + mtx_lock(&((struct ata_intel_data *)(_ctlr->chipset_data))->lock) +#define ATA_INTEL_UNLOCK(_ctlr) \ + mtx_unlock(&((struct ata_intel_data *)(_ctlr->chipset_data))->lock) + /* * Intel chipset support functions */ @@ -217,7 +229,10 @@ ata_intel_chipinit(device_t dev) if (ata_setup_interrupt(dev, ata_generic_intr)) return ENXIO; - ctlr->chipset_data = NULL; + struct ata_intel_data *data = malloc(sizeof(struct ata_intel_data), + M_DEVBUF, M_WAITOK | M_ZERO); + mtx_init(&data->lock, "Intel SATA lock", NULL, MTX_DEF); + ctlr->chipset_data = (void *)data; /* good old PIIX needs special treatment (not implemented) */ if (ctlr->chip->chipid == ATA_I82371FB) { @@ -329,7 +344,7 @@ ata_intel_ch_attach(device_t dev) ch->flags |= ATA_ALWAYS_DMASTAT; if (ctlr->chip->max_dma >= ATA_SA150) { - smap = (u_char *)&ctlr->chipset_data + ch->unit * 2; + smap = ATA_INTEL_SMAP(ctlr, ch); map = pci_read_config(device_get_parent(dev), 0x90, 1); if (ctlr->chip->cfg1 & INTEL_ICH5) { map &= 0x07; @@ -415,7 +430,7 @@ ata_intel_reset(device_t dev) return (ata_generic_reset(dev)); /* Do hard-reset on respective SATA ports. */ - smap = (u_char *)&ctlr->chipset_data + ch->unit * 2; + smap = ATA_INTEL_SMAP(ctlr, ch); mask = 1 << smap[0]; if ((ch->flags & ATA_NO_SLAVE) == 0) mask |= (1 << smap[1]); @@ -605,7 +620,7 @@ ata_intel_sata_ahci_read(device_t dev, int port, int reg, u_int32_t *result) ctlr = device_get_softc(parent); ch = device_get_softc(dev); port = (port == 1) ? 1 : 0; - smap = (u_char *)&ctlr->chipset_data + ch->unit * 2; + smap = ATA_INTEL_SMAP(ctlr, ch); offset = 0x100 + smap[port] * 0x80; switch (reg) { case ATA_SSTATUS: @@ -635,7 +650,7 @@ ata_intel_sata_cscr_read(device_t dev, int port, int reg, u_int32_t *result) parent = device_get_parent(dev); ctlr = device_get_softc(parent); ch = device_get_softc(dev); - smap = (u_char *)&ctlr->chipset_data + ch->unit * 2; + smap = ATA_INTEL_SMAP(ctlr, ch); port = (port == 1) ? 1 : 0; switch (reg) { case ATA_SSTATUS: @@ -650,9 +665,11 @@ ata_intel_sata_cscr_read(device_t dev, int port, int reg, u_int32_t *result) default: return (EINVAL); } + ATA_INTEL_LOCK(ctlr); pci_write_config(parent, 0xa0, 0x50 + smap[port] * 0x10 + reg * 4, 4); *result = pci_read_config(parent, 0xa4, 4); + ATA_INTEL_UNLOCK(ctlr); return (0); } @@ -680,8 +697,10 @@ ata_intel_sata_sidpr_read(device_t dev, int port, int reg, u_int32_t *result) default: return (EINVAL); } + ATA_INTEL_LOCK(ctlr); ATA_IDX_OUTL(ch, ATA_IDX_ADDR, ((ch->unit * 2 + port) << 8) + reg); *result = ATA_IDX_INL(ch, ATA_IDX_DATA); + ATA_INTEL_UNLOCK(ctlr); return (0); } @@ -698,7 +717,7 @@ ata_intel_sata_ahci_write(device_t dev, int port, int reg, u_int32_t value) ctlr = device_get_softc(parent); ch = device_get_softc(dev); port = (port == 1) ? 1 : 0; - smap = (u_char *)&ctlr->chipset_data + ch->unit * 2; + smap = ATA_INTEL_SMAP(ctlr, ch); offset = 0x100 + smap[port] * 0x80; switch (reg) { case ATA_SSTATUS: @@ -728,7 +747,7 @@ ata_intel_sata_cscr_write(device_t dev, int port, int reg, u_int32_t value) parent = device_get_parent(dev); ctlr = device_get_softc(parent); ch = device_get_softc(dev); - smap = (u_char *)&ctlr->chipset_data + ch->unit * 2; + smap = ATA_INTEL_SMAP(ctlr, ch); port = (port == 1) ? 1 : 0; switch (reg) { case ATA_SSTATUS: @@ -743,9 +762,11 @@ ata_intel_sata_cscr_write(device_t dev, int port, int reg, u_int32_t value) default: return (EINVAL); } + ATA_INTEL_LOCK(ctlr); pci_write_config(parent, 0xa0, 0x50 + smap[port] * 0x10 + reg * 4, 4); pci_write_config(parent, 0xa4, value, 4); + ATA_INTEL_UNLOCK(ctlr); return (0); } @@ -773,8 +794,10 @@ ata_intel_sata_sidpr_write(device_t dev, int port, int reg, u_int32_t value) default: return (EINVAL); } + ATA_INTEL_LOCK(ctlr); ATA_IDX_OUTL(ch, ATA_IDX_ADDR, ((ch->unit * 2 + port) << 8) + reg); ATA_IDX_OUTL(ch, ATA_IDX_DATA, value); + ATA_INTEL_UNLOCK(ctlr); return (0); } --Apple-Mail-10-5916955 Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset=us-ascii -------------------------------------------------- Andrew Boyer aboyer@averesystems.com --Apple-Mail-10-5916955--