[PATCH] libata: cosmetic change in ata_bus_softreset()

[PATCH] libata: cosmetic change in ata_bus_softreset()

[Tejun Heo]

ata_bus_softreset() should return AC_ERR_* on failure not arbitrary
positive number.  While at it, kill trailing indentations and reformat
comment.

Signed-off-by: Tejun Heo <htejun@gmail.com>

diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index f3c115b..200198c 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -1999,13 +1999,12 @@ static unsigned int ata_bus_softreset(st
 	 */
 	msleep(150);
 
-	
-	/* Before we perform post reset processing we want to see if 
-	   the bus shows 0xFF because the odd clown forgets the D7 pulldown
-	   resistor */
-	
+	/* Before we perform post reset processing we want to see if
+	 * the bus shows 0xFF because the odd clown forgets the D7
+	 * pulldown resistor
+	 */
 	if (ata_check_status(ap) == 0xFF)
-		return 1;	/* Positive is failure for some reason */
+		return AC_ERR_OTHER;
 
 	ata_bus_post_reset(ap, devmask);
 

Re: [PATCH] libata: cosmetic change in ata_bus_softreset()

[Jeff Garzik]

Tejun Heo wrote:
> ata_bus_softreset() should return AC_ERR_* on failure not arbitrary
> positive number.  While at it, kill trailing indentations and reformat
> comment.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> 
> diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
> index f3c115b..200198c 100644
> --- a/drivers/scsi/libata-core.c
> +++ b/drivers/scsi/libata-core.c
> @@ -1999,13 +1999,12 @@ static unsigned int ata_bus_softreset(st
>  	 */
>  	msleep(150);
>  
> -	
> -	/* Before we perform post reset processing we want to see if 
> -	   the bus shows 0xFF because the odd clown forgets the D7 pulldown
> -	   resistor */
> -	
> +	/* Before we perform post reset processing we want to see if
> +	 * the bus shows 0xFF because the odd clown forgets the D7
> +	 * pulldown resistor
> +	 */
>  	if (ata_check_status(ap) == 0xFF)
> -		return 1;	/* Positive is failure for some reason */
> +		return AC_ERR_OTHER;

agreed and will apply, though note:

The reason Alan did this is most likely because the only other function 
that produces a non-zero return code is ata_bus_edd->ata_busy_sleep, 
which returns 1.

Though really, we should rip out E.D.D. anyway....

	Jeff

[PATCH] libata: cosmetic changes in ata_bus_softreset()

[Tejun Heo]

ata_bus_softreset() should return AC_ERR_* on failure not arbitrary
positive number.  While at it, reformat comment above it.

Signed-off-by: Tejun Heo <htejun@gmail.com>
Acked-by: Alan Cox <alan@redhat.com>

---

 libata-core.c |   11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index f3c115b..200198c 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -1999,13 +1999,12 @@ static unsigned int ata_bus_softreset(st
 	 */
 	msleep(150);
 
-	
-	/* Before we perform post reset processing we want to see if 
-	   the bus shows 0xFF because the odd clown forgets the D7 pulldown
-	   resistor */
-	
+	/* Before we perform post reset processing we want to see if
+	 * the bus shows 0xFF because the odd clown forgets the D7
+	 * pulldown resistor
+	 */
 	if (ata_check_status(ap) == 0xFF)
-		return 1;	/* Positive is failure for some reason */
+		return AC_ERR_OTHER;
 
 	ata_bus_post_reset(ap, devmask);
 

Re: [PATCH] libata: cosmetic changes in ata_bus_softreset()

[Jeff Garzik]

Tejun Heo wrote:
> ata_bus_softreset() should return AC_ERR_* on failure not arbitrary
> positive number.  While at it, reformat comment above it.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> Acked-by: Alan Cox <alan@redhat.com>

ACK but doesn't apply to #upstream (now linux-2.6.git as well)

[PATCH] libata: cosmetic changes in ata_bus_softreset()

[Tejun Heo]

ata_bus_softreset() should return AC_ERR_* on failure not arbitrary
positive number.  While at it, reformat comment above it.

Signed-off-by: Tejun Heo <htejun@gmail.com>
Acked-by: Alan Cox <alan@redhat.com>

---

Hmmm... weird.  Here's the regenerated patch against the current
upstream (aec5c3c1a929d7d79a420e943285cf3ba26a7c0d).

 libata-core.c |    9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index 0aff888..64f71df 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -2008,13 +2008,12 @@ static unsigned int ata_bus_softreset(st
 	 */
 	msleep(150);
 
-
 	/* Before we perform post reset processing we want to see if
-	   the bus shows 0xFF because the odd clown forgets the D7 pulldown
-	   resistor */
-
+	 * the bus shows 0xFF because the odd clown forgets the D7
+	 * pulldown resistor.
+	 */
 	if (ata_check_status(ap) == 0xFF)
-		return 1;	/* Positive is failure for some reason */
+		return AC_ERR_OTHER;
 
 	ata_bus_post_reset(ap, devmask);
 

Re: [PATCH] libata: cosmetic changes in ata_bus_softreset()

[Jeff Garzik]

Tejun Heo wrote:
> ata_bus_softreset() should return AC_ERR_* on failure not arbitrary
> positive number.  While at it, reformat comment above it.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> Acked-by: Alan Cox <alan@redhat.com>

applied

[PATCH] libata: kill E.D.D.

[Tejun Heo]

E.D.D. has no user in-tree and mostly useless.  Kill it.  For possible
out-of-tree users, add a nice warning message and error handling if
LLDD doesn't report any useable reset mechanism (and thus tries to use
E.D.D.).

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

I left BUS_EDD constant alone as I have no idea know how those BUS_*
constants are used.  Tested on sata_sil, sata_promise, ata_piix and
ahci.

 drivers/scsi/libata-core.c |   98 +++++----------------------------------------
 include/linux/libata.h     |    1
 2 files changed, 13 insertions(+), 86 deletions(-)

diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index eda4c0c..516b1a6 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -1081,9 +1081,8 @@ unsigned int ata_pio_need_iordy(const st
  *
  *	Read ID data from the specified device.  ATA_CMD_ID_ATA is
  *	performed on ATA devices and ATA_CMD_ID_ATAPI on ATAPI
- *	devices.  This function also takes care of EDD signature
- *	misreporting (to be removed once EDD support is gone) and
- *	issues ATA_CMD_INIT_DEV_PARAMS for pre-ATA4 drives.
+ *	devices.  This function also issues ATA_CMD_INIT_DEV_PARAMS
+ *	for pre-ATA4 drives.
  *
  *	LOCKING:
  *	Kernel thread context (may sleep)
@@ -1095,7 +1094,6 @@ static int ata_dev_read_id(struct ata_po
 			   unsigned int *p_class, int post_reset, u16 **p_id)
 {
 	unsigned int class = *p_class;
-	unsigned int using_edd;
 	struct ata_taskfile tf;
 	unsigned int err_mask = 0;
 	u16 *id;
@@ -1104,12 +1102,6 @@ static int ata_dev_read_id(struct ata_po
 
 	DPRINTK("ENTER, host %u, dev %u\n", ap->id, dev->devno);
 
-	if (ap->ops->probe_reset ||
-	    ap->flags & (ATA_FLAG_SRST | ATA_FLAG_SATA_RESET))
-		using_edd = 0;
-	else
-		using_edd = 1;
-
 	ata_dev_select(ap, dev->devno, 1, 1); /* select device 0/1 */
 
 	id = kmalloc(sizeof(id[0]) * ATA_ID_WORDS, GFP_KERNEL);
@@ -1139,32 +1131,9 @@ static int ata_dev_read_id(struct ata_po
 
 	err_mask = ata_exec_internal(ap, dev, &tf, DMA_FROM_DEVICE,
 				     id, sizeof(id[0]) * ATA_ID_WORDS);
-
 	if (err_mask) {
 		rc = -EIO;
 		reason = "I/O error";
-
-		if (err_mask & ~AC_ERR_DEV)
-			goto err_out;
-
-		/*
-		 * arg!  EDD works for all test cases, but seems to return
-		 * the ATA signature for some ATAPI devices.  Until the
-		 * reason for this is found and fixed, we fix up the mess
-		 * here.  If IDENTIFY DEVICE returns command aborted
-		 * (as ATAPI devices do), then we issue an
-		 * IDENTIFY PACKET DEVICE.
-		 *
-		 * ATA software reset (SRST, the default) does not appear
-		 * to have this problem.
-		 */
-		if ((using_edd) && (class == ATA_DEV_ATA)) {
-			u8 err = tf.feature;
-			if (err & ATA_ABORTED) {
-				class = ATA_DEV_ATAPI;
-				goto retry;
-			}
-		}
 		goto err_out;
 	}
 
@@ -2005,45 +1974,6 @@ static void ata_bus_post_reset(struct at
 		ap->ops->dev_select(ap, 0);
 }
 
-/**
- *	ata_bus_edd - Issue EXECUTE DEVICE DIAGNOSTIC command.
- *	@ap: Port to reset and probe
- *
- *	Use the EXECUTE DEVICE DIAGNOSTIC command to reset and
- *	probe the bus.  Not often used these days.
- *
- *	LOCKING:
- *	PCI/etc. bus probe sem.
- *	Obtains host_set lock.
- *
- */
-
-static unsigned int ata_bus_edd(struct ata_port *ap)
-{
-	struct ata_taskfile tf;
-	unsigned long flags;
-
-	/* set up execute-device-diag (bus reset) taskfile */
-	/* also, take interrupts to a known state (disabled) */
-	DPRINTK("execute-device-diag\n");
-	ata_tf_init(ap, &tf, 0);
-	tf.ctl |= ATA_NIEN;
-	tf.command = ATA_CMD_EDD;
-	tf.protocol = ATA_PROT_NODATA;
-
-	/* do bus reset */
-	spin_lock_irqsave(&ap->host_set->lock, flags);
-	ata_tf_to_host(ap, &tf);
-	spin_unlock_irqrestore(&ap->host_set->lock, flags);
-
-	/* spec says at least 2ms.  but who knows with those
-	 * crazy ATAPI devices...
-	 */
-	msleep(150);
-
-	return ata_busy_sleep(ap, ATA_TMOUT_BOOT_QUICK, ATA_TMOUT_BOOT);
-}
-
 static unsigned int ata_bus_softreset(struct ata_port *ap,
 				      unsigned int devmask)
 {
@@ -2115,7 +2045,7 @@ void ata_bus_reset(struct ata_port *ap)
 	struct ata_ioports *ioaddr = &ap->ioaddr;
 	unsigned int slave_possible = ap->flags & ATA_FLAG_SLAVE_POSS;
 	u8 err;
-	unsigned int dev0, dev1 = 0, rc = 0, devmask = 0;
+	unsigned int dev0, dev1 = 0, devmask = 0;
 
 	DPRINTK("ENTER, host %u, port %u\n", ap->id, ap->port_no);
 
@@ -2138,18 +2068,8 @@ void ata_bus_reset(struct ata_port *ap)
 
 	/* issue bus reset */
 	if (ap->flags & ATA_FLAG_SRST)
-		rc = ata_bus_softreset(ap, devmask);
-	else if ((ap->flags & ATA_FLAG_SATA_RESET) == 0) {
-		/* set up device control */
-		if (ap->flags & ATA_FLAG_MMIO)
-			writeb(ap->ctl, (void __iomem *) ioaddr->ctl_addr);
-		else
-			outb(ap->ctl, ioaddr->ctl_addr);
-		rc = ata_bus_edd(ap);
-	}
-
-	if (rc)
-		goto err_out;
+		if (ata_bus_softreset(ap, devmask))
+			goto err_out;
 
 	/*
 	 * determine by signature whether we have ATA or ATAPI devices
@@ -4535,6 +4455,14 @@ static struct ata_port * ata_host_add(co
 	int rc;
 
 	DPRINTK("ENTER\n");
+
+	if (!ent->port_ops->probe_reset &&
+	    !(ent->host_flags & (ATA_FLAG_SATA_RESET | ATA_FLAG_SRST))) {
+		printk(KERN_ERR "ata%u: no reset mechanism available\n",
+		       port_no);
+		return NULL;
+	}
+
 	host = scsi_host_alloc(ent->sht, sizeof(struct ata_port));
 	if (!host)
 		return NULL;
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 0471922..9fcc061 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -161,7 +161,6 @@ enum {
 	ATA_QCFLAG_EH_SCHEDULED = (1 << 5), /* EH scheduled */
 
 	/* various lengths of time */
-	ATA_TMOUT_EDD		= 5 * HZ,	/* heuristic */
 	ATA_TMOUT_PIO		= 30 * HZ,
 	ATA_TMOUT_BOOT		= 30 * HZ,	/* heuristic */
 	ATA_TMOUT_BOOT_QUICK	= 7 * HZ,	/* heuristic */

Re: [PATCH] libata: kill E.D.D.

[Jeff Garzik]

Tejun Heo wrote:
> E.D.D. has no user in-tree and mostly useless.  Kill it.  For possible
> out-of-tree users, add a nice warning message and error handling if
> LLDD doesn't report any useable reset mechanism (and thus tries to use
> E.D.D.).
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

applied

Re: [PATCH] libata: kill E.D.D.

[Alan Cox]

On Sad, 2006-03-25 at 01:33 +0900, Tejun Heo wrote:
> E.D.D. has no user in-tree and mostly useless.  Kill it.  For possible
> out-of-tree users, add a nice warning message and error handling if
> LLDD doesn't report any useable reset mechanism (and thus tries to use
> E.D.D.).

We made need this for some ISAPnP, M68K and embedded devices but its
easy enough to put back when actual hardware is found.

Re: [PATCH] libata: cosmetic change in ata_bus_softreset()

[Jeff Garzik]

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

Tejun Heo wrote:
> ata_bus_softreset() should return AC_ERR_* on failure not arbitrary
> positive number.  While at it, kill trailing indentations and reformat
> comment.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

Please pull #upstream, make sure you have
2e9edbf815e42f93dd29a9981f27bd421acc47df, and then resend.  I ran chomp 
(attached) on a bunch of files.

	Jeff



[-- Attachment #2: chomp.pl --]
[-- Type: application/x-perl, Size: 806 bytes --]