Date: Wed, 20 Jul 2011 10:44:42 -0400 From: Andrew Boyer <aboyer@averesystems.com> To: mav@freebsd.org Cc: freebsd-current@freebsd.org, Jack Vogel <jack.vogel@intel.com> Subject: [patch] Intel SATA controller hiccups, locking Message-ID: <D0BF9B62-9D23-46DF-9295-95CC0C6AEBB6@averesystems.com>
next in thread | raw e-mail | index | archive | help
[-- Attachment #1 --]
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=0x01018f card=0x060d15d9 chip=0x3a208086 rev=0x00 hdr=0x00
> atapci1@pci0:0:31:5: class=0x010185 card=0x060d15d9 chip=0x3a268086 rev=0x00 hdr=0x00
> atapci0: <Intel ICH10 SATA300 controller> port 0xbff0-0xbff7,0xbf7c-0xbf7f,0xbfe0-0xbfe7,0xbef4-0xbef7,0xbfa0-0xbfaf,0xbf60-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: <ATA channel 0> 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=0x00
> ata2: p0: SATA connect timeout status=00000000
> ata2: p1: SATA connect timeout status=00000000
> ata2: [MPSAFE]
> ata2: [ITHREAD]
> ata3: <ATA channel 1> 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=0x08
> ata3: p0: SATA connect timeout status=00000000
> ata3: p1: SATA connect time=0ms status=00000123
> ata3: reset tp1 mask=03 ostat0=7f ostat1=50
> ata3: stat0=0x7f err=0x00 lsb=0xff msb=0xff
> ata3: stat1=0x50 err=0x01 lsb=0x00 msb=0x00
> ata3: reset tp2 stat0=7f stat1=50 devices=0x2
> 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.
Thank you,
Andrew
[-- Attachment #2 --]
--- 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);
}
[-- Attachment #3 --]
--------------------------------------------------
Andrew Boyer aboyer@averesystems.com
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?D0BF9B62-9D23-46DF-9295-95CC0C6AEBB6>