Re: sata_mv oops on module removal

[Tejun Heo <htejun@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 499 bytes --]

Mark Lord wrote:
> Tejun,
> 
> This Oops (2.6.23.13) looks very much like the same bug
> you fixed recently for me in 2.6.24.
> 
> The bug was with sata_qstor and other drivers, in that
> devres/libata were freeing the I/O resources before invoking
> the LLD's host/port_stop routines .. which still need the I/O.
> 
> Did that patch get backported to 2.6.23 yet ?

No, it's not but yeah it probably should have.  Tomasz, can you please
test whether the attached patch fixes the problem?

-- 
tejun

[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 2301 bytes --]

Index: tree0/drivers/ata/libata-core.c
===================================================================
--- tree0.orig/drivers/ata/libata-core.c
+++ tree0/drivers/ata/libata-core.c
@@ -6121,19 +6121,6 @@ static void ata_host_release(struct devi
 		if (!ap)
 			continue;
 
-		if ((host->flags & ATA_HOST_STARTED) && ap->ops->port_stop)
-			ap->ops->port_stop(ap);
-	}
-
-	if ((host->flags & ATA_HOST_STARTED) && host->ops->host_stop)
-		host->ops->host_stop(host);
-
-	for (i = 0; i < host->n_ports; i++) {
-		struct ata_port *ap = host->ports[i];
-
-		if (!ap)
-			continue;
-
 		if (ap->scsi_host)
 			scsi_host_put(ap->scsi_host);
 
@@ -6258,6 +6245,24 @@ struct ata_host *ata_host_alloc_pinfo(st
 	return host;
 }
 
+static void ata_host_stop(struct device *gendev, void *res)
+{
+	struct ata_host *host = dev_get_drvdata(gendev);
+	int i;
+
+	WARN_ON(!(host->flags & ATA_HOST_STARTED));
+
+	for (i = 0; i < host->n_ports; i++) {
+		struct ata_port *ap = host->ports[i];
+
+		if (ap->ops->port_stop)
+			ap->ops->port_stop(ap);
+	}
+
+	if (host->ops->host_stop)
+		host->ops->host_stop(host);
+}
+
 /**
  *	ata_host_start - start and freeze ports of an ATA host
  *	@host: ATA host to start ports for
@@ -6276,6 +6281,8 @@ struct ata_host *ata_host_alloc_pinfo(st
  */
 int ata_host_start(struct ata_host *host)
 {
+	int have_stop = 0;
+	void *start_dr = NULL;
 	int i, rc;
 
 	if (host->flags & ATA_HOST_STARTED)
@@ -6287,6 +6294,22 @@ int ata_host_start(struct ata_host *host
 		if (!host->ops && !ata_port_is_dummy(ap))
 			host->ops = ap->ops;
 
+		if (ap->ops->port_stop)
+			have_stop = 1;
+	}
+
+	if (host->ops->host_stop)
+		have_stop = 1;
+
+	if (have_stop) {
+		start_dr = devres_alloc(ata_host_stop, 0, GFP_KERNEL);
+		if (!start_dr)
+			return -ENOMEM;
+	}
+
+	for (i = 0; i < host->n_ports; i++) {
+		struct ata_port *ap = host->ports[i];
+
 		if (ap->ops->port_start) {
 			rc = ap->ops->port_start(ap);
 			if (rc) {
@@ -6299,6 +6322,8 @@ int ata_host_start(struct ata_host *host
 		ata_eh_freeze_port(ap);
 	}
 
+	if (start_dr)
+		devres_add(host->dev, start_dr);
 	host->flags |= ATA_HOST_STARTED;
 	return 0;
 
@@ -6309,6 +6334,7 @@ int ata_host_start(struct ata_host *host
 		if (ap->ops->port_stop)
 			ap->ops->port_stop(ap);
 	}
+	devres_free(start_dr);
 	return rc;
 }