Improve ata_reinit():

 - protect againtst recursions,
 - add new devices detection using ata_identify().

Improve ata_identify():
 - do not add duplicate device if device already exist.

Rework SATA hot-plug events handling. Instead of unsafe duplicate
implementation use common ata_reinit() to handle all state changes.

All together this gives quite stable and robust cold- and hot-plug operation,
invariant to false, lost and duplicate events.

5 files changed, 48 insertions, 89 deletions

diff --git a/sys/dev/ata/ata-all.c b/sys/dev/ata/ata-all.c
index bf33d1b..f0d1cf7 100644
--- a/sys/dev/ata/ata-all.c
+++ b/sys/dev/ata/ata-all.c
@@ -62,6 +62,7 @@ static struct cdevsw ata_cdevsw = {
 /* prototypes */
 static void ata_boot_attach(void);
 static device_t ata_add_child(device_t, struct ata_device *, int);
+static void ata_conn_event(void *, int);
 static void bswap(int8_t *, int);
 static void btrim(int8_t *, int);
 static void bpack(int8_t *, int8_t *, int);
@@ -127,6 +128,7 @@ ata_attach(device_t dev)
     bzero(&ch->queue_mtx, sizeof(struct mtx));
     mtx_init(&ch->queue_mtx, "ATA queue lock", NULL, MTX_DEF);
     TAILQ_INIT(&ch->ata_queue);
+    TASK_INIT(&ch->conntask, 0, ata_conn_event, dev);
 
     /* reset the controller HW, the channel and device(s) */
     while (ATA_LOCKING(dev, ATA_LF_LOCK) != ch->unit)
@@ -181,6 +183,7 @@ ata_detach(device_t dev)
 		device_delete_child(dev, children[i]);
 	free(children, M_TEMP);
     } 
+    taskqueue_drain(taskqueue_thread, &ch->conntask);
 
     /* release resources */
     bus_teardown_intr(dev, ch->r_irq, ch->ih);
@@ -196,6 +199,14 @@ ata_detach(device_t dev)
     return 0;
 }
 
+static void
+ata_conn_event(void *context, int dummy)
+{
+    device_t dev = (device_t)context;
+
+    ata_reinit(dev);
+}
+
 int
 ata_reinit(device_t dev)
 {
@@ -217,6 +228,11 @@ ata_reinit(device_t dev)
 
     /* catch eventual request in ch->running */
     mtx_lock(&ch->state_mtx);
+    if (ch->state & ATA_STALL_QUEUE) {
+	/* Recursive reinits and reinits during detach prohobited. */
+	mtx_unlock(&ch->state_mtx);
+	return (ENXIO);
+    }
     if ((request = ch->running))
 	callout_stop(&request->callout);
     ch->running = NULL;
@@ -274,6 +290,9 @@ ata_reinit(device_t dev)
     mtx_unlock(&ch->state_mtx);
     ATA_LOCKING(dev, ATA_LF_UNLOCK);
 
+    /* Add new children. */
+    ata_identify(dev);
+
     if (bootverbose)
 	device_printf(dev, "reinit done ..\n");
 
@@ -684,14 +703,27 @@ ata_identify(device_t dev)
 {
     struct ata_channel *ch = device_get_softc(dev);
     struct ata_device *atadev;
+    device_t *children;
     device_t child;
-    int i;
+    int nchildren, i, n = ch->devices;
 
     if (bootverbose)
-	device_printf(dev, "identify ch->devices=%08x\n", ch->devices);
+	device_printf(dev, "Identifying devices: %08x\n", ch->devices);
 
+    mtx_lock(&Giant);
+    /* Skip existing devices. */
+    if (!device_get_children(dev, &children, &nchildren)) {
+	for (i = 0; i < nchildren; i++) {
+	    if (children[i] && (atadev = device_get_softc(children[i])))
+		n &= ~((ATA_ATA_MASTER | ATA_ATAPI_MASTER) << atadev->unit);
+	}
+	free(children, M_TEMP);
+    }
+    /* Create new devices. */
+    if (bootverbose)
+	device_printf(dev, "New devices: %08x\n", n);
     for (i = 0; i < ATA_PM; ++i) {
-	if (ch->devices & (((ATA_ATA_MASTER | ATA_ATAPI_MASTER) << i))) {
+	if (n & (((ATA_ATA_MASTER | ATA_ATAPI_MASTER) << i))) {
 	    int unit = -1;
 
 	    if (!(atadev = malloc(sizeof(struct ata_device),
@@ -701,7 +733,7 @@ ata_identify(device_t dev)
 	    }
 	    atadev->unit = i;
 #ifdef ATA_STATIC_ID
-	    if (ch->devices & ((ATA_ATA_MASTER << i)))
+	    if (n & (ATA_ATA_MASTER << i))
 		unit = (device_get_unit(dev) << 1) + i;
 #endif
 	    if ((child = ata_add_child(dev, atadev, unit))) {
@@ -716,6 +748,7 @@ ata_identify(device_t dev)
     }
     bus_generic_probe(dev);
     bus_generic_attach(dev);
+    mtx_unlock(&Giant);
     return 0;
 }
 
diff --git a/sys/dev/ata/ata-all.h b/sys/dev/ata/ata-all.h
index 4e47f82..a721d80 100644
--- a/sys/dev/ata/ata-all.h
+++ b/sys/dev/ata/ata-all.h
@@ -530,6 +530,7 @@ struct ata_channel {
     TAILQ_HEAD(, ata_request)   ata_queue;      /* head of ATA queue */
     struct ata_request          *freezepoint;   /* composite freezepoint */
     struct ata_request          *running;       /* currently running request */
+    struct task			conntask;	/* PHY events handling task */
 };
 
 /* disk bay/enclosure related */
diff --git a/sys/dev/ata/ata-pci.h b/sys/dev/ata/ata-pci.h
index 35de93a..27a341e 100644
--- a/sys/dev/ata/ata-pci.h
+++ b/sys/dev/ata/ata-pci.h
@@ -66,15 +66,6 @@ struct ata_pci_controller {
     } interrupt[8];     /* XXX SOS max ch# for now */
 };
 
-/* structure for SATA connection update hotplug/hotswap support */
-struct ata_connect_task {
-    struct task task;
-    device_t    dev;  
-    int         action;
-#define ATA_C_ATTACH    1
-#define ATA_C_DETACH    2
-};
-
 /* defines for known chipset PCI id's */
 #define ATA_ACARD_ID            0x1191
 #define ATA_ATP850              0x00021191
@@ -451,7 +442,6 @@ int ata_check_80pin(device_t dev, int mode);
 int ata_mode2idx(int mode);
 
 /* global prototypes ata-sata.c */
-void ata_sata_phy_event(void *context, int dummy);
 void ata_sata_phy_check_events(device_t dev);
 int ata_sata_phy_reset(device_t dev);
 void ata_sata_setmode(device_t dev, int mode);
diff --git a/sys/dev/ata/ata-sata.c b/sys/dev/ata/ata-sata.c
index b65bd56..559ed1e 100644
--- a/sys/dev/ata/ata-sata.c
+++ b/sys/dev/ata/ata-sata.c
@@ -50,41 +50,6 @@ __FBSDID("$FreeBSD$");
 #include <dev/ata/ata-pci.h>
 #include <ata_if.h>
 
-/*
- * SATA support functions
- */
-void
-ata_sata_phy_event(void *context, int dummy)
-{
-    struct ata_connect_task *tp = (struct ata_connect_task *)context;
-    struct ata_channel *ch = device_get_softc(tp->dev);
-    device_t *children;
-    int nchildren, i;
-
-    mtx_lock(&Giant);   /* newbus suckage it needs Giant */
-    if (tp->action == ATA_C_ATTACH) {
-	if (bootverbose)
-	    device_printf(tp->dev, "CONNECTED\n");
-	ATA_RESET(tp->dev);
-	ata_identify(tp->dev);
-    }
-    if (tp->action == ATA_C_DETACH) {
-	if (!device_get_children(tp->dev, &children, &nchildren)) {
-	    for (i = 0; i < nchildren; i++)
-		if (children[i])
-		    device_delete_child(tp->dev, children[i]);
-	    free(children, M_TEMP);
-	}    
-	mtx_lock(&ch->state_mtx);
-	ch->state = ATA_IDLE;
-	mtx_unlock(&ch->state_mtx);
-	if (bootverbose)
-	    device_printf(tp->dev, "DISCONNECTED\n");
-    }
-    mtx_unlock(&Giant); /* suckage code dealt with, release Giant */
-    free(tp, M_ATA);
-}
-
 void
 ata_sata_phy_check_events(device_t dev)
 {
@@ -94,32 +59,17 @@ ata_sata_phy_check_events(device_t dev)
     /* clear error bits/interrupt */
     ATA_IDX_OUTL(ch, ATA_SERROR, error);
 
-    /* do we have any events flagged ? */
-    if (error) {
-	struct ata_connect_task *tp;
-	u_int32_t status = ATA_IDX_INL(ch, ATA_SSTATUS);
-
-	/* if we have a connection event deal with it */
-	if ((error & ATA_SE_PHY_CHANGED) &&
-	    (tp = (struct ata_connect_task *)
-		  malloc(sizeof(struct ata_connect_task),
-			 M_ATA, M_NOWAIT | M_ZERO))) {
-
+    /* if we have a connection event deal with it */
+    if (error & ATA_SE_PHY_CHANGED) {
+	if (bootverbose) {
+	    u_int32_t status = ATA_IDX_INL(ch, ATA_SSTATUS);
 	    if (((status & ATA_SS_CONWELL_MASK) == ATA_SS_CONWELL_GEN1) ||
 		((status & ATA_SS_CONWELL_MASK) == ATA_SS_CONWELL_GEN2)) {
-		if (bootverbose)
 		    device_printf(dev, "CONNECT requested\n");
-		tp->action = ATA_C_ATTACH;
-	    }
-	    else {
-		if (bootverbose)
+	    } else
 		    device_printf(dev, "DISCONNECT requested\n");
-		tp->action = ATA_C_DETACH;
-	    }
-	    tp->dev = dev;
-	    TASK_INIT(&tp->task, 0, ata_sata_phy_event, tp);
-	    taskqueue_enqueue(taskqueue_thread, &tp->task);
 	}
+	taskqueue_enqueue(taskqueue_thread, &ch->conntask);
     }
 }
 
diff --git a/sys/dev/ata/chipsets/ata-promise.c b/sys/dev/ata/chipsets/ata-promise.c
index 0913df1..261e0be 100644
--- a/sys/dev/ata/chipsets/ata-promise.c
+++ b/sys/dev/ata/chipsets/ata-promise.c
@@ -637,7 +637,6 @@ ata_promise_mio_status(device_t dev)
 {
     struct ata_pci_controller *ctlr = device_get_softc(device_get_parent(dev));
     struct ata_channel *ch = device_get_softc(dev);
-    struct ata_connect_task *tp;
     u_int32_t fake_reg, stat_reg, vector, status;
 
     switch (ctlr->chip->cfg2) {
@@ -663,31 +662,17 @@ ata_promise_mio_status(device_t dev)
     ATA_OUTL(ctlr->r_res2, stat_reg, status & (0x00000011 << ch->unit));
 
     /* check for and handle disconnect events */
-    if ((status & (0x00000001 << ch->unit)) &&
-	(tp = (struct ata_connect_task *)
-	      malloc(sizeof(struct ata_connect_task),
-		     M_ATA, M_NOWAIT | M_ZERO))) {
-
+    if (status & (0x00000001 << ch->unit)) {
 	if (bootverbose)
 	    device_printf(dev, "DISCONNECT requested\n");
-	tp->action = ATA_C_DETACH;
-	tp->dev = dev;
-	TASK_INIT(&tp->task, 0, ata_sata_phy_event, tp);
-	taskqueue_enqueue(taskqueue_thread, &tp->task);
+	taskqueue_enqueue(taskqueue_thread, &ch->conntask);
     }
 
     /* check for and handle connect events */
-    if ((status & (0x00000010 << ch->unit)) &&
-	(tp = (struct ata_connect_task *)
-	      malloc(sizeof(struct ata_connect_task),
-		     M_ATA, M_NOWAIT | M_ZERO))) {
-
+    if (status & (0x00000010 << ch->unit)) {
 	if (bootverbose)
 	    device_printf(dev, "CONNECT requested\n");
-	tp->action = ATA_C_ATTACH;
-	tp->dev = dev;
-	TASK_INIT(&tp->task, 0, ata_sata_phy_event, tp);
-	taskqueue_enqueue(taskqueue_thread, &tp->task);
+	taskqueue_enqueue(taskqueue_thread, &ch->conntask);
     }
 
     /* do we have any device action ? */