Skip site navigation (1)Skip section navigation (2)
Date:      Sat, 28 Feb 2009 22:07:15 +0000 (UTC)
From:      Alexander Motin <mav@FreeBSD.org>
To:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org
Subject:   svn commit: r189195 - head/sys/dev/ata
Message-ID:  <200902282207.n1SM7Fug022809@svn.freebsd.org>

next in thread | raw e-mail | index | archive | help
Author: mav
Date: Sat Feb 28 22:07:15 2009
New Revision: 189195
URL: http://svn.freebsd.org/changeset/base/189195

Log:
  Revert my ata_identify()/ata_reinit() related changes: r189166, r189091
  and partially r188903. Revert breaks new drives detection on reinit to the
  state as it was before me, but fixes series of new bugs reported by some
  people.
  
  Unconditional queueing of ata_completed() calls can lead to deadlock if
  due to timeout ata_reinit() was called at the same thread by previous
  ata_completed(). Calling of ata_identify() on ata_reinit() in current
  implementation opens numerous races and deadlocks.
  
  Problems I was touching here are still exist and should be addresed, but
  probably in different way.

Modified:
  head/sys/dev/ata/ata-all.c
  head/sys/dev/ata/ata-all.h
  head/sys/dev/ata/ata-disk.c
  head/sys/dev/ata/ata-queue.c
  head/sys/dev/ata/ata-raid.c
  head/sys/dev/ata/atapi-cam.c
  head/sys/dev/ata/atapi-cd.c
  head/sys/dev/ata/atapi-fd.c
  head/sys/dev/ata/atapi-tape.c

Modified: head/sys/dev/ata/ata-all.c
==============================================================================
--- head/sys/dev/ata/ata-all.c	Sat Feb 28 19:10:43 2009	(r189194)
+++ head/sys/dev/ata/ata-all.c	Sat Feb 28 22:07:15 2009	(r189195)
@@ -61,6 +61,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);
@@ -179,7 +180,7 @@ ata_detach(device_t dev)
     if (!device_get_children(dev, &children, &nchildren)) {
 	for (i = 0; i < nchildren; i++)
 	    if (children[i])
-		ata_delete_child(dev, children[i]);
+		device_delete_child(dev, children[i]);
 	free(children, M_TEMP);
     } 
     taskqueue_drain(taskqueue_thread, &ch->conntask);
@@ -264,7 +265,7 @@ ata_reinit(device_t dev)
 			    ata_finish(request);
 		    request = NULL;
 		}
-		ata_delete_child(dev, children[i]);
+		device_delete_child(dev, children[i]);
 	    }
 	}
 	free(children, M_TEMP);
@@ -290,7 +291,7 @@ ata_reinit(device_t dev)
     ATA_LOCKING(dev, ATA_LF_UNLOCK);
 
     /* Add new children. */
-    ata_identify(dev);
+/*    ata_identify(dev); */
 
     if (bootverbose)
 	device_printf(dev, "reinit done ..\n");
@@ -589,44 +590,19 @@ ata_boot_attach(void)
 /*
  * misc support functions
  */
-device_t
-ata_add_child(device_t parent, int unit, int atapi)
+static device_t
+ata_add_child(device_t parent, struct ata_device *atadev, int unit)
 {
-    struct ata_device *atadev;
     device_t child;
-    int dev_unit = -1;
 
-#ifdef ATA_STATIC_ID
-    if (!atapi)
-	dev_unit = (device_get_unit(parent) << 1) + unit;
-#endif
-    if ((child = device_add_child(parent, NULL, dev_unit))) {
-	if (!(atadev = malloc(sizeof(struct ata_device),
-			  M_ATA, M_NOWAIT | M_ZERO))) {
-	    device_printf(parent, "out of memory\n");
-	    device_delete_child(parent, child);
-	    return (NULL);
-	}
+    if ((child = device_add_child(parent, NULL, unit))) {
 	device_set_softc(child, atadev);
 	device_quiet(child);
 	atadev->dev = child;
-	atadev->unit = unit;
-	atadev->type = atapi ? ATA_T_ATAPI : ATA_T_ATA;
 	atadev->max_iosize = DEV_BSIZE;
 	atadev->mode = ATA_PIO_MAX;
     }
-    return (child);
-}
-
-int
-ata_delete_child(device_t parent, device_t child)
-{
-    struct ata_device *atadev = device_get_softc(child);
-    int res;
-
-    res = device_delete_child(parent, child);
-    free(atadev, M_ATA);
-    return (res);
+    return child;
 }
 
 int
@@ -637,11 +613,11 @@ ata_getparam(struct ata_device *atadev, 
     u_int8_t command = 0;
     int error = ENOMEM, retries = 2;
 
-    if (atadev->type == ATA_T_ATA)
+    if (ch->devices & (ATA_ATA_MASTER << atadev->unit))
 	command = ATA_ATA_IDENTIFY;
-    else if (atadev->type == ATA_T_ATAPI)
+    if (ch->devices & (ATA_ATAPI_MASTER << atadev->unit))
 	command = ATA_ATAPI_IDENTIFY;
-    else
+    if (!command)
 	return ENXIO;
 
     while (retries-- > 0 && error) {
@@ -651,7 +627,7 @@ ata_getparam(struct ata_device *atadev, 
 	request->timeout = 1;
 	request->retries = 0;
 	request->u.ata.command = command;
-	request->flags = (ATA_R_READ|ATA_R_AT_HEAD|ATA_R_THREAD);
+	request->flags = (ATA_R_READ|ATA_R_AT_HEAD|ATA_R_DIRECT);
 	if (!bootverbose)
 	    request->flags |= ATA_R_QUIET;
 	request->data = (void *)&atadev->param;
@@ -687,18 +663,17 @@ ata_getparam(struct ata_device *atadev, 
 	btrim(atacap->serial, sizeof(atacap->serial));
 	bpack(atacap->serial, atacap->serial, sizeof(atacap->serial));
 
-	if (init) {
-	    char buffer[64];
-
-	    if (bootverbose) {
-		printf("ata%d-%s: pio=%s wdma=%s udma=%s cable=%s wire\n",
+	if (bootverbose)
+	    printf("ata%d-%s: pio=%s wdma=%s udma=%s cable=%s wire\n",
 		   device_get_unit(ch->dev),
 		   ata_unit2str(atadev),
 		   ata_mode2str(ata_pmode(atacap)),
 		   ata_mode2str(ata_wmode(atacap)),
 		   ata_mode2str(ata_umode(atacap)),
 		   (atacap->hwres & ATA_CABLE_ID) ? "80":"40");
-	    }
+
+	if (init) {
+	    char buffer[64];
 
 	    sprintf(buffer, "%.40s/%.8s", atacap->model, atacap->revision);
 	    device_set_desc_copy(atadev->dev, buffer);
@@ -731,6 +706,7 @@ ata_identify(device_t dev)
     struct ata_channel *ch = device_get_softc(dev);
     struct ata_device *atadev;
     device_t *children;
+    device_t child;
     int nchildren, i, n = ch->devices;
 
     if (bootverbose)
@@ -745,18 +721,37 @@ ata_identify(device_t dev)
 	}
 	free(children, M_TEMP);
     }
+    /* Create new devices. */
     if (bootverbose)
 	device_printf(dev, "New devices: %08x\n", n);
     if (n == 0) {
 	mtx_unlock(&Giant);
 	return (0);
     }
-    /* Create new devices. */
     for (i = 0; i < ATA_PM; ++i) {
-	if (n & (((ATA_ATA_MASTER | ATA_ATAPI_MASTER) << i)))
-	    ata_add_child(dev, i, n & (ATA_ATAPI_MASTER << i));
-    }
+	if (n & (((ATA_ATA_MASTER | ATA_ATAPI_MASTER) << i))) {
+	    int unit = -1;
 
+	    if (!(atadev = malloc(sizeof(struct ata_device),
+				  M_ATA, M_NOWAIT | M_ZERO))) {
+		device_printf(dev, "out of memory\n");
+		return ENOMEM;
+	    }
+	    atadev->unit = i;
+#ifdef ATA_STATIC_ID
+	    if (n & (ATA_ATA_MASTER << i))
+		unit = (device_get_unit(dev) << 1) + i;
+#endif
+	    if ((child = ata_add_child(dev, atadev, unit))) {
+		if (ata_getparam(atadev, 1)) {
+		    device_delete_child(dev, child);
+		    free(atadev, M_ATA);
+		}
+	    }
+	    else
+		free(atadev, M_ATA);
+	}
+    }
     bus_generic_probe(dev);
     bus_generic_attach(dev);
     mtx_unlock(&Giant);

Modified: head/sys/dev/ata/ata-all.h
==============================================================================
--- head/sys/dev/ata/ata-all.h	Sat Feb 28 19:10:43 2009	(r189194)
+++ head/sys/dev/ata/ata-all.h	Sat Feb 28 22:07:15 2009	(r189195)
@@ -367,6 +367,7 @@ struct ata_request {
 #define         ATA_R_AT_HEAD           0x00000200
 #define         ATA_R_REQUEUE           0x00000400
 #define         ATA_R_THREAD            0x00000800
+#define         ATA_R_DIRECT            0x00001000
 
 #define         ATA_R_DEBUG             0x10000000
 #define         ATA_R_DANGER1           0x20000000
@@ -410,10 +411,6 @@ struct ata_device {
 #define         ATA_MASTER              0x00
 #define         ATA_SLAVE               0x01
 #define         ATA_PM                  0x0f
-    int                         type;           /* device type */
-#define         ATA_T_ATA               0x00
-#define         ATA_T_ATAPI             0x01
-#define         ATA_T_ATAPI_CAM         0x02
 
     struct ata_params           param;          /* ata param structure */
     int                         mode;           /* current transfermode */
@@ -426,8 +423,6 @@ struct ata_device {
 #define         ATA_D_MEDIA_CHANGED     0x0002
 #define         ATA_D_ENC_PRESENT       0x0004
 #define         ATA_D_48BIT_ACTIVE      0x0008
-#define         ATA_D_PROBED            0x0010
-#define         ATA_D_VALID             0x0020
 };
 
 /* structure for holding DMA Physical Region Descriptors (PRD) entries */
@@ -565,8 +560,6 @@ void ata_interrupt(void *data);
 int ata_device_ioctl(device_t dev, u_long cmd, caddr_t data);
 int ata_getparam(struct ata_device *atadev, int init);
 int ata_identify(device_t dev);
-device_t ata_add_child(device_t, int, int);
-int ata_delete_child(device_t , device_t);
 void ata_default_registers(device_t dev);
 void ata_modify_if_48bit(struct ata_request *request);
 void ata_udelay(int interval);

Modified: head/sys/dev/ata/ata-disk.c
==============================================================================
--- head/sys/dev/ata/ata-disk.c	Sat Feb 28 19:10:43 2009	(r189194)
+++ head/sys/dev/ata/ata-disk.c	Sat Feb 28 22:07:15 2009	(r189195)
@@ -79,18 +79,6 @@ ad_probe(device_t dev)
 {
     struct ata_device *atadev = device_get_softc(dev);
 
-    if (atadev->type != ATA_T_ATA)
-	return (ENXIO);
-
-    if (!(atadev->flags & ATA_D_PROBED)) {
-	atadev->flags |= ATA_D_PROBED;
-	if (ata_getparam(atadev, 1) == 0)
-	    atadev->flags |= ATA_D_VALID;
-    }
-
-    if (!(atadev->flags & ATA_D_VALID))
-	return (ENXIO);
-
     if (!(atadev->param.config & ATA_PROTO_ATAPI) ||
 	(atadev->param.config == ATA_CFA_MAGIC1) ||
 	(atadev->param.config == ATA_CFA_MAGIC2) ||

Modified: head/sys/dev/ata/ata-queue.c
==============================================================================
--- head/sys/dev/ata/ata-queue.c	Sat Feb 28 19:10:43 2009	(r189194)
+++ head/sys/dev/ata/ata-queue.c	Sat Feb 28 22:07:15 2009	(r189195)
@@ -237,8 +237,14 @@ ata_start(device_t dev)
 void
 ata_finish(struct ata_request *request)
 {
+    struct ata_channel *ch = device_get_softc(request->parent);
 
-    if (dumping) {
+    /*
+     * if in ATA_STALL_QUEUE state or request has ATA_R_DIRECT flags set
+     * we need to call ata_complete() directly here (no taskqueue involvement)
+     */
+    if (dumping ||
+	(ch->state & ATA_STALL_QUEUE) || (request->flags & ATA_R_DIRECT)) {
 	ATA_DEBUG_RQ(request, "finish directly");
 	ata_completed(request, 0);
     }

Modified: head/sys/dev/ata/ata-raid.c
==============================================================================
--- head/sys/dev/ata/ata-raid.c	Sat Feb 28 19:10:43 2009	(r189194)
+++ head/sys/dev/ata/ata-raid.c	Sat Feb 28 22:07:15 2009	(r189195)
@@ -275,7 +275,7 @@ ata_raid_flush(struct bio *bp)
 	request->u.ata.feature = 0;
 	request->timeout = 1;
 	request->retries = 0;
-	request->flags |= ATA_R_ORDERED | ATA_R_THREAD;
+	request->flags |= ATA_R_ORDERED | ATA_R_DIRECT;
 	ata_queue_request(request);
     }
     return 0;
@@ -1570,7 +1570,7 @@ ata_raid_wipe_metadata(struct ar_softc *
 	    if (!(meta = malloc(size, M_AR, M_NOWAIT | M_ZERO)))
 		return ENOMEM;
 	    if (ata_raid_rw(rdp->disks[disk].dev, lba, meta, size,
-			    ATA_R_WRITE | ATA_R_THREAD)) {
+			    ATA_R_WRITE | ATA_R_DIRECT)) {
 		device_printf(rdp->disks[disk].dev, "wipe metadata failed\n");
 		error = EIO;
 	    }
@@ -2264,7 +2264,7 @@ ata_raid_hptv2_write_meta(struct ar_soft
 	    if (ata_raid_rw(rdp->disks[disk].dev,
 			    HPTV2_LBA(rdp->disks[disk].dev), meta,
 			    sizeof(struct promise_raid_conf),
-			    ATA_R_WRITE | ATA_R_THREAD)) {
+			    ATA_R_WRITE | ATA_R_DIRECT)) {
 		device_printf(rdp->disks[disk].dev, "write metadata failed\n");
 		error = EIO;
 	    }
@@ -2710,7 +2710,7 @@ ata_raid_intel_write_meta(struct ar_soft
 	if (rdp->disks[disk].dev) {
 	    if (ata_raid_rw(rdp->disks[disk].dev,
 			    INTEL_LBA(rdp->disks[disk].dev),
-			    meta, 1024, ATA_R_WRITE | ATA_R_THREAD)) {
+			    meta, 1024, ATA_R_WRITE | ATA_R_DIRECT)) {
 		device_printf(rdp->disks[disk].dev, "write metadata failed\n");
 		error = EIO;
 	    }
@@ -3055,7 +3055,7 @@ ata_raid_jmicron_write_meta(struct ar_so
 	    if (ata_raid_rw(rdp->disks[disk].dev,
 			    JMICRON_LBA(rdp->disks[disk].dev),
 			    meta, sizeof(struct jmicron_raid_conf),
-			    ATA_R_WRITE | ATA_R_THREAD)) {
+			    ATA_R_WRITE | ATA_R_DIRECT)) {
 		device_printf(rdp->disks[disk].dev, "write metadata failed\n");
 		error = EIO;
 	    }
@@ -3778,7 +3778,7 @@ ata_raid_promise_write_meta(struct ar_so
 	    if (ata_raid_rw(rdp->disks[disk].dev,
 			    PROMISE_LBA(rdp->disks[disk].dev),
 			    meta, sizeof(struct promise_raid_conf),
-			    ATA_R_WRITE | ATA_R_THREAD)) {
+			    ATA_R_WRITE | ATA_R_DIRECT)) {
 		device_printf(rdp->disks[disk].dev, "write metadata failed\n");
 		error = EIO;
 	    }
@@ -4126,7 +4126,7 @@ ata_raid_sis_write_meta(struct ar_softc 
 	    if (ata_raid_rw(rdp->disks[disk].dev,
 			    SIS_LBA(rdp->disks[disk].dev),
 			    meta, sizeof(struct sis_raid_conf),
-			    ATA_R_WRITE | ATA_R_THREAD)) {
+			    ATA_R_WRITE | ATA_R_DIRECT)) {
 		device_printf(rdp->disks[disk].dev, "write metadata failed\n");
 		error = EIO;
 	    }
@@ -4351,7 +4351,7 @@ ata_raid_via_write_meta(struct ar_softc 
 	    if (ata_raid_rw(rdp->disks[disk].dev,
 			    VIA_LBA(rdp->disks[disk].dev),
 			    meta, sizeof(struct via_raid_conf),
-			    ATA_R_WRITE | ATA_R_THREAD)) {
+			    ATA_R_WRITE | ATA_R_DIRECT)) {
 		device_printf(rdp->disks[disk].dev, "write metadata failed\n");
 		error = EIO;
 	    }

Modified: head/sys/dev/ata/atapi-cam.c
==============================================================================
--- head/sys/dev/ata/atapi-cam.c	Sat Feb 28 19:10:43 2009	(r189194)
+++ head/sys/dev/ata/atapi-cam.c	Sat Feb 28 22:07:15 2009	(r189195)
@@ -147,7 +147,7 @@ static void
 atapi_cam_identify(driver_t *driver, device_t parent)
 {
 	struct atapi_xpt_softc *scp =
-	    malloc (sizeof (struct atapi_xpt_softc), M_ATA, M_NOWAIT|M_ZERO);
+	    malloc (sizeof (struct atapi_xpt_softc), M_ATACAM, M_NOWAIT|M_ZERO);
 	device_t child;
 
 	if (scp == NULL) {
@@ -159,11 +159,10 @@ atapi_cam_identify(driver_t *driver, dev
 	child = device_add_child(parent, "atapicam", -1);
 	if (child == NULL) {
 		printf ("atapi_cam_identify: out of memory, can't add child");
-		free (scp, M_ATA);
+		free (scp, M_ATACAM);
 		return;
 	}
 	scp->atapi_cam_dev.unit = -1;
-	scp->atapi_cam_dev.type = ATA_T_ATAPI_CAM;
 	scp->atapi_cam_dev.dev  = child;
 	device_quiet(child);
 	device_set_softc(child, scp);
@@ -175,10 +174,12 @@ atapi_cam_probe(device_t dev)
 	struct ata_device *atadev = device_get_softc (dev);
 
 	KASSERT(atadev != NULL, ("expect valid struct ata_device"));
-	if (atadev->type != ATA_T_ATAPI_CAM)
-		return (ENXIO);
-	device_set_desc(dev, "ATAPI CAM Attachment");
-	return (0);
+	if (atadev->unit < 0) {
+		device_set_desc(dev, "ATAPI CAM Attachment");
+		return (0);
+	} else {
+		return ENXIO;
+	}
 }
 
 static int
@@ -924,8 +925,11 @@ atapi_cam_event_handler(module_t mod, in
 	    if (devlist != NULL) {
 		while (devlist != NULL && devcount > 0) {
 		    device_t child = devlist[--devcount];
+		    struct atapi_xpt_softc *scp = device_get_softc(child);
 
-		    ata_delete_child(device_get_parent(child),child);
+		    device_delete_child(device_get_parent(child),child);
+		    if (scp != NULL)
+			free(scp, M_ATACAM);
 		}
 		free(devlist, M_TEMP);
 	    }

Modified: head/sys/dev/ata/atapi-cd.c
==============================================================================
--- head/sys/dev/ata/atapi-cd.c	Sat Feb 28 19:10:43 2009	(r189194)
+++ head/sys/dev/ata/atapi-cd.c	Sat Feb 28 22:07:15 2009	(r189195)
@@ -108,18 +108,6 @@ acd_probe(device_t dev)
 {
     struct ata_device *atadev = device_get_softc(dev);
 
-    if (atadev->type != ATA_T_ATAPI)
-	return (ENXIO);
-
-    if (!(atadev->flags & ATA_D_PROBED)) {
-	atadev->flags |= ATA_D_PROBED;
-	if (ata_getparam(atadev, 1) == 0)
-	    atadev->flags |= ATA_D_VALID;
-    }
-
-    if (!(atadev->flags & ATA_D_VALID))
-	return (ENXIO);
-
     if ((atadev->param.config & ATA_PROTO_ATAPI) &&
 	(atadev->param.config & ATA_ATAPI_TYPE_MASK) == ATA_ATAPI_TYPE_CDROM)
 	return 0;

Modified: head/sys/dev/ata/atapi-fd.c
==============================================================================
--- head/sys/dev/ata/atapi-fd.c	Sat Feb 28 19:10:43 2009	(r189194)
+++ head/sys/dev/ata/atapi-fd.c	Sat Feb 28 22:07:15 2009	(r189195)
@@ -66,19 +66,6 @@ static int 
 afd_probe(device_t dev)
 {
     struct ata_device *atadev = device_get_softc(dev);
-
-    if (atadev->type != ATA_T_ATAPI)
-	return (ENXIO);
-
-    if (!(atadev->flags & ATA_D_PROBED)) {
-	atadev->flags |= ATA_D_PROBED;
-	if (ata_getparam(atadev, 1) == 0)
-	    atadev->flags |= ATA_D_VALID;
-    }
-
-    if (!(atadev->flags & ATA_D_VALID))
-	return (ENXIO);
-
     if ((atadev->param.config & ATA_PROTO_ATAPI) &&
 	(atadev->param.config & ATA_ATAPI_TYPE_MASK) == ATA_ATAPI_TYPE_DIRECT)
 	return 0;  

Modified: head/sys/dev/ata/atapi-tape.c
==============================================================================
--- head/sys/dev/ata/atapi-tape.c	Sat Feb 28 19:10:43 2009	(r189194)
+++ head/sys/dev/ata/atapi-tape.c	Sat Feb 28 22:07:15 2009	(r189195)
@@ -90,18 +90,6 @@ ast_probe(device_t dev)
 {
     struct ata_device *atadev = device_get_softc(dev);
 
-    if (atadev->type != ATA_T_ATAPI)
-	return (ENXIO);
-
-    if (!(atadev->flags & ATA_D_PROBED)) {
-	atadev->flags |= ATA_D_PROBED;
-	if (ata_getparam(atadev, 1) == 0)
-	    atadev->flags |= ATA_D_VALID;
-    }
-
-    if (!(atadev->flags & ATA_D_VALID))
-	return (ENXIO);
-
     if ((atadev->param.config & ATA_PROTO_ATAPI) &&
 	(atadev->param.config & ATA_ATAPI_TYPE_MASK) == ATA_ATAPI_TYPE_TAPE)
 	return 0;



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200902282207.n1SM7Fug022809>