[PATCH #upstream 1/2] libata: move command post processing to __ata_qc_complete()

[PATCH #upstream 1/2] libata: move command post processing to __ata_qc_complete()

[Tejun Heo]

Some commands need post-processing after successful completion.  This
was done in ata_scsi_qc_complete() till now but command post
processing doesn't belong to SAT layer.  Move them to
__ata_qc_complete() and, while at it, restructure a bit to ease adding
post-processing for other commands.

Signed-off-by: Tejun Heo <htejun@gmail.com>
---
 drivers/ata/libata-core.c |   25 +++++++++++++++++++++++++
 drivers/ata/libata-scsi.c |   23 -----------------------
 2 files changed, 25 insertions(+), 23 deletions(-)

Index: work/drivers/ata/libata-core.c
===================================================================
--- work.orig/drivers/ata/libata-core.c
+++ work/drivers/ata/libata-core.c
@@ -5536,6 +5536,31 @@ void __ata_qc_complete(struct ata_queued
 	qc->flags &= ~ATA_QCFLAG_ACTIVE;
 	ap->qc_active &= ~(1 << qc->tag);
 
+	/* some commands need post-processing after successful completion */
+	if (likely(qc->err_mask == 0)) {
+		unsigned int eh_action = 0;
+
+		switch (qc->tf.command) {
+		case ATA_CMD_SET_FEATURES:
+			/* cache configuration changed? */
+			if (ap->ops->error_handler &&
+			    ((qc->tf.feature == SETFEATURES_WC_ON) ||
+			     (qc->tf.feature == SETFEATURES_WC_OFF)))
+				eh_action |= ATA_EH_REVALIDATE;
+			break;
+
+		case ATA_CMD_INIT_DEV_PARAMS: /* CHS translation changed */
+		case ATA_CMD_SET_MULTI: /* multi_count changed */
+			eh_action |= ATA_EH_REVALIDATE;
+			break;
+		}
+
+		if (unlikely(eh_action) && ap->ops->error_handler) {
+			link->eh_info.action |= eh_action;
+			ata_port_schedule_eh(ap);
+		}
+	}
+
 	/* call completion callback */
 	qc->complete_fn(qc);
 }
Index: work/drivers/ata/libata-scsi.c
===================================================================
--- work.orig/drivers/ata/libata-scsi.c
+++ work/drivers/ata/libata-scsi.c
@@ -1363,33 +1363,10 @@ nothing_to_do:
 static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
 {
 	struct ata_port *ap = qc->ap;
-	struct ata_eh_info *ehi = &qc->dev->link->eh_info;
 	struct scsi_cmnd *cmd = qc->scsicmd;
 	u8 *cdb = cmd->cmnd;
  	int need_sense = (qc->err_mask != 0);
 
-	/* We snoop the SET_FEATURES - Write Cache ON/OFF command, and
-	 * schedule EH_REVALIDATE operation to update the IDENTIFY DEVICE
-	 * cache
-	 */
-	if (ap->ops->error_handler && !need_sense) {
-		switch (qc->tf.command) {
-		case ATA_CMD_SET_FEATURES:
-			if ((qc->tf.feature == SETFEATURES_WC_ON) ||
-			    (qc->tf.feature == SETFEATURES_WC_OFF)) {
-				ehi->action |= ATA_EH_REVALIDATE;
-				ata_port_schedule_eh(ap);
-			}
-			break;
-
-		case ATA_CMD_INIT_DEV_PARAMS: /* CHS translation changed */
-		case ATA_CMD_SET_MULTI: /* multi_count changed */
-			ehi->action |= ATA_EH_REVALIDATE;
-			ata_port_schedule_eh(ap);
-			break;
-		}
-	}
-
 	/* For ATA pass thru (SAT) commands, generate a sense block if
 	 * user mandated it or if there's an error.  Note that if we
 	 * generate because the user forced us to, a check condition

[PATCH #upstream 2/2] libata: track SLEEP state and issue SRST to wake it up

[Tejun Heo]

ATA devices in SLEEP mode don't respond to any commands.  SRST is
necessary to wake it up.  Till now, when a command is issued to a
device in SLEEP mode, the command times out, which makes EH reset the
device and retry the command after that, causing a long delay.

This patch makes libata track SLEEP state and issue SRST automatically
if a command is about to be issued to a device in SLEEP.

Signed-off-by: Tejun Heo <htejun@gmail.com>
Cc: Bruce Allen <ballen@gravity.phys.uwm.edu>
Cc: Andrew Paprocki <andrew@ishiboo.com>
---
 drivers/ata/libata-core.c |   12 ++++++++++++
 drivers/ata/libata-eh.c   |    4 +++-
 include/linux/ata.h       |    1 +
 include/linux/libata.h    |    1 +
 4 files changed, 17 insertions(+), 1 deletion(-)

Index: work/include/linux/ata.h
===================================================================
--- work.orig/include/linux/ata.h
+++ work/include/linux/ata.h
@@ -179,6 +179,7 @@ enum {
 	ATA_CMD_VERIFY		= 0x40,
 	ATA_CMD_VERIFY_EXT	= 0x42,
  	ATA_CMD_STANDBYNOW1	= 0xE0,
+	ATA_CMD_SLEEP		= 0xE6,
  	ATA_CMD_IDLEIMMEDIATE	= 0xE1,
 	ATA_CMD_INIT_DEV_PARAMS	= 0x91,
 	ATA_CMD_READ_NATIVE_MAX	= 0xF8,
Index: work/include/linux/libata.h
===================================================================
--- work.orig/include/linux/libata.h
+++ work/include/linux/libata.h
@@ -145,6 +145,7 @@ enum {
 	ATA_DFLAG_PIO		= (1 << 12), /* device limited to PIO mode */
 	ATA_DFLAG_NCQ_OFF	= (1 << 13), /* device limited to non-NCQ mode */
 	ATA_DFLAG_SPUNDOWN	= (1 << 14), /* XXX: for spindown_compat */
+	ATA_DFLAG_SLEEPING	= (1 << 15), /* device is sleeping */
 	ATA_DFLAG_INIT_MASK	= (1 << 16) - 1,
 
 	ATA_DFLAG_DETACH	= (1 << 16),
Index: work/drivers/ata/libata-core.c
===================================================================
--- work.orig/drivers/ata/libata-core.c
+++ work/drivers/ata/libata-core.c
@@ -5553,6 +5553,10 @@ void __ata_qc_complete(struct ata_queued
 		case ATA_CMD_SET_MULTI: /* multi_count changed */
 			eh_action |= ATA_EH_REVALIDATE;
 			break;
+
+		case ATA_CMD_SLEEP:
+			qc->dev->flags |= ATA_DFLAG_SLEEPING;
+			break;
 		}
 
 		if (unlikely(eh_action) && ap->ops->error_handler) {
@@ -5757,6 +5761,14 @@ void ata_qc_issue(struct ata_queued_cmd 
 		qc->flags &= ~ATA_QCFLAG_DMAMAP;
 	}
 
+	/* if device is sleeping, schedule softreset and abort the link */
+	if (unlikely(qc->dev->flags & ATA_DFLAG_SLEEPING)) {
+		link->eh_info.action |= ATA_EH_SOFTRESET;
+		ata_ehi_push_desc(&link->eh_info, "waking up from sleep");
+		ata_link_abort(link);
+		return;
+	}
+
 	ap->ops->qc_prep(qc);
 
 	qc->err_mask |= ap->ops->qc_issue(qc);
Index: work/drivers/ata/libata-eh.c
===================================================================
--- work.orig/drivers/ata/libata-eh.c
+++ work/drivers/ata/libata-eh.c
@@ -2207,9 +2207,11 @@ int ata_eh_reset(struct ata_link *link, 
 		ata_link_for_each_dev(dev, link) {
 			/* After the reset, the device state is PIO 0
 			 * and the controller state is undefined.
-			 * Record the mode.
+			 * Reset also wakes up drives from sleeping
+			 * mode.
 			 */
 			dev->pio_mode = XFER_PIO_0;
+			dev->flags &= ~ATA_DFLAG_SLEEPING;
 
 			if (ata_link_offline(link))
 				continue;

Re: [PATCH #upstream 2/2] libata: track SLEEP state and issue SRST to wake it up

[Andrew Paprocki]

Tejun,

I'm able to break my system using this patch. I had a hunch this might
be possible.. :) In short, if you issue a sleep command while the
drive is already sleeping, it puts libata into an infinite loop
resetting the port. I've illustrated the working test and the evil
hunch below. The sleep command itself will need a short-circuit out of
this logic in order to prevent this loop.

Also, in the working case below the hddtemp command actually blocked
until the drive was spun up before returning a valid temp. While
testing, I was able to get hddtemp to trigger the drive wake-up when
it was sleeping, but hddtemp then returned stating the drive was
sleeping. Re-running hddtemp until the drive was fully spun up
(another 5 seconds) kept returning that it was sleeping. I'll see if I
can reproduce this reliably. Am I correct in assuming the process
which triggers the wake-up should block?

-Andrew

Working case:

# hddtemp /dev/sdb
/dev/sdb: Hitachi HDS721010KLA330                 : 35 C
# hdparm -Y /dev/sdb

/dev/sdb:
 issuing sleep command
# time hddtemp /dev/sdb
ata2.00: exception Emask 0x0 SAct 0x0 SErr 0x0 action 0x2
ata2.00: waking up from sleep
ata2: soft resetting link
ata2: SATA link up 1.5 Gbps (SStatus 113 SControl 310)
ata2.00: configured for UDMA/100
ata2: EH complete
sd 1:0:0:0: [sdb] 1953525168 512-byte hardware sectors (1000205 MB)
sd 1:0:0:0: [sdb] Write Protect is off
sd 1:0:0:0: [sdb] Mode Sense: 00 3a 00 00
sd 1:0:0:0: [sdb] Write cache: enabled, read cache: enabled, doesn't
support DPO or FUA
/dev/sdb: Hitachi HDS721010KLA330                 : 34 C
real    0m 10.89s
user    0m 0.00s
sys     0m 0.00s
# time hddtemp /dev/sdb
/dev/sdb: Hitachi HDS721010KLA330                 : 34 C
real    0m 0.26s
user    0m 0.00s
sys     0m 0.00s

Evil DoS case:

# hddtemp /dev/sdb
/dev/sdb: Hitachi HDS721010KLA330                 : 35 C
# hdparm -Y /dev/sdb

/dev/sdb:
 issuing sleep command
# hdparm -Y /dev/sdb
ata2.00: exception Emask 0x0 SAct 0x0 SErr 0x0 action 0x2
ata2.00: waking up from sleep
ata2: soft resetting link
ata2: SATA link up 1.5 Gbps (SStatus 113 SControl 310)
ata2.00: configured for UDMA/100
ata2: EH complete
ata2.00: exception Emask 0x0 SAct 0x0 SErr 0x0 action 0x2
ata2.00: waking up from sleep
ata2: soft resetting link
ata2: SATA link up 1.5 Gbps (SStatus 113 SControl 310)
ata2.00: configured for UDMA/100
ata2: EH complete
....
to infinity

On 10/12/07, Tejun Heo <htejun@gmail.com> wrote:
> ATA devices in SLEEP mode don't respond to any commands.  SRST is
> necessary to wake it up.  Till now, when a command is issued to a
> device in SLEEP mode, the command times out, which makes EH reset the
> device and retry the command after that, causing a long delay.
>
> This patch makes libata track SLEEP state and issue SRST automatically
> if a command is about to be issued to a device in SLEEP.
>
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> Cc: Bruce Allen <ballen@gravity.phys.uwm.edu>
> Cc: Andrew Paprocki <andrew@ishiboo.com>
> ---
>  drivers/ata/libata-core.c |   12 ++++++++++++
>  drivers/ata/libata-eh.c   |    4 +++-
>  include/linux/ata.h       |    1 +
>  include/linux/libata.h    |    1 +
>  4 files changed, 17 insertions(+), 1 deletion(-)
>
> Index: work/include/linux/ata.h
> ===================================================================
> --- work.orig/include/linux/ata.h
> +++ work/include/linux/ata.h
> @@ -179,6 +179,7 @@ enum {
>         ATA_CMD_VERIFY          = 0x40,
>         ATA_CMD_VERIFY_EXT      = 0x42,
>         ATA_CMD_STANDBYNOW1     = 0xE0,
> +       ATA_CMD_SLEEP           = 0xE6,
>         ATA_CMD_IDLEIMMEDIATE   = 0xE1,
>         ATA_CMD_INIT_DEV_PARAMS = 0x91,
>         ATA_CMD_READ_NATIVE_MAX = 0xF8,
> Index: work/include/linux/libata.h
> ===================================================================
> --- work.orig/include/linux/libata.h
> +++ work/include/linux/libata.h
> @@ -145,6 +145,7 @@ enum {
>         ATA_DFLAG_PIO           = (1 << 12), /* device limited to PIO mode */
>         ATA_DFLAG_NCQ_OFF       = (1 << 13), /* device limited to non-NCQ mode */
>         ATA_DFLAG_SPUNDOWN      = (1 << 14), /* XXX: for spindown_compat */
> +       ATA_DFLAG_SLEEPING      = (1 << 15), /* device is sleeping */
>         ATA_DFLAG_INIT_MASK     = (1 << 16) - 1,
>
>         ATA_DFLAG_DETACH        = (1 << 16),
> Index: work/drivers/ata/libata-core.c
> ===================================================================
> --- work.orig/drivers/ata/libata-core.c
> +++ work/drivers/ata/libata-core.c
> @@ -5553,6 +5553,10 @@ void __ata_qc_complete(struct ata_queued
>                 case ATA_CMD_SET_MULTI: /* multi_count changed */
>                         eh_action |= ATA_EH_REVALIDATE;
>                         break;
> +
> +               case ATA_CMD_SLEEP:
> +                       qc->dev->flags |= ATA_DFLAG_SLEEPING;
> +                       break;
>                 }
>
>                 if (unlikely(eh_action) && ap->ops->error_handler) {
> @@ -5757,6 +5761,14 @@ void ata_qc_issue(struct ata_queued_cmd
>                 qc->flags &= ~ATA_QCFLAG_DMAMAP;
>         }
>
> +       /* if device is sleeping, schedule softreset and abort the link */
> +       if (unlikely(qc->dev->flags & ATA_DFLAG_SLEEPING)) {
> +               link->eh_info.action |= ATA_EH_SOFTRESET;
> +               ata_ehi_push_desc(&link->eh_info, "waking up from sleep");
> +               ata_link_abort(link);
> +               return;
> +       }
> +
>         ap->ops->qc_prep(qc);
>
>         qc->err_mask |= ap->ops->qc_issue(qc);
> Index: work/drivers/ata/libata-eh.c
> ===================================================================
> --- work.orig/drivers/ata/libata-eh.c
> +++ work/drivers/ata/libata-eh.c
> @@ -2207,9 +2207,11 @@ int ata_eh_reset(struct ata_link *link,
>                 ata_link_for_each_dev(dev, link) {
>                         /* After the reset, the device state is PIO 0
>                          * and the controller state is undefined.
> -                        * Record the mode.
> +                        * Reset also wakes up drives from sleeping
> +                        * mode.
>                          */
>                         dev->pio_mode = XFER_PIO_0;
> +                       dev->flags &= ~ATA_DFLAG_SLEEPING;
>
>                         if (ata_link_offline(link))
>                                 continue;
>
>

Re: [PATCH #upstream 2/2] libata: track SLEEP state and issue SRST to wake it up

[Tejun Heo]

Andrew Paprocki wrote:
> Tejun,
> 
> I'm able to break my system using this patch. I had a hunch this might
> be possible.. :) In short, if you issue a sleep command while the
> drive is already sleeping, it puts libata into an infinite loop
> resetting the port. I've illustrated the working test and the evil
> hunch below. The sleep command itself will need a short-circuit out of
> this logic in order to prevent this loop.

Heh... I guess you're much better a software engineer than I am.  Thanks
for finding it out.  It was stupid of me.  :-)

> Also, in the working case below the hddtemp command actually blocked
> until the drive was spun up before returning a valid temp. While
> testing, I was able to get hddtemp to trigger the drive wake-up when
> it was sleeping, but hddtemp then returned stating the drive was
> sleeping. Re-running hddtemp until the drive was fully spun up
> (another 5 seconds) kept returning that it was sleeping. I'll see if I
> can reproduce this reliably. Am I correct in assuming the process
> which triggers the wake-up should block?

Yeah, it should.  I'll test with hddtemp myself.

Jeff, please forget about this patchset.  I'll re-post updated version.

-- 
tejun

Re: [PATCH #upstream 2/2] libata: track SLEEP state and issue SRST to wake it up

[Andrew Paprocki]

Tejun,

This patch applied on top of your set works for me. It clears the
error mask and completes any ATA_CMD_SLEEP when the drive is already
sleeping. I tried `hdparm -Y` twice and it didn't loop like before.

Thanks,
-Andrew

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 45b781b..7e0627f 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -5763,6 +5763,16 @@ void ata_qc_issue(struct ata_queued_cmd *qc)

        /* if device is sleeping, schedule softreset and abort the link */
        if (unlikely(qc->dev->flags & ATA_DFLAG_SLEEPING)) {
+               if (unlikely(qc->tf.command == ATA_CMD_SLEEP)) {
+                       /* to prevent a loop, do not wake up if sleeping
+                        * and a sleep cmd is sent. instead, simply clear
+                        * the error mask and complete as if it was
+                        * successful.
+                        */
+                       qc->err_mask = 0;
+                       ata_qc_complete(qc);
+                       return;
+               }
                link->eh_info.action |= ATA_EH_SOFTRESET;
                ata_ehi_push_desc(&link->eh_info, "waking up from sleep");
                ata_link_abort(link);

On 10/13/07, Tejun Heo <htejun@gmail.com> wrote:
> Jeff, please forget about this patchset.  I'll re-post updated version.

Re: [PATCH #upstream 1/2] libata: move command post processing to __ata_qc_complete()

[Jeff Garzik]

Tejun Heo wrote:
> Some commands need post-processing after successful completion.  This
> was done in ata_scsi_qc_complete() till now but command post
> processing doesn't belong to SAT layer.  Move them to
> __ata_qc_complete() and, while at it, restructure a bit to ease adding
> post-processing for other commands.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

BTW, while doing the TEST UNIT READY emulation patch for ATA (recently 
withdrawn from libata-dev.git#upstream), I found a problem with the 
interface that was difficult to get around:  TEST UNIT READY simulation 
code really wants to look at the result TF of CHECK POWER MODE, even if 
ATA_ERR is asserted, before determining whether or not to call that 
command an error.

Maybe the EH scheduling could be moved until after ->complete_fn, to 
permit ->complete_fn users to manipulate qc->err_mask etc.?

	Jeff

Re: [PATCH #upstream 1/2] libata: move command post processing to __ata_qc_complete()

[Tejun Heo]

Jeff Garzik wrote:
> Tejun Heo wrote:
>> Some commands need post-processing after successful completion.  This
>> was done in ata_scsi_qc_complete() till now but command post
>> processing doesn't belong to SAT layer.  Move them to
>> __ata_qc_complete() and, while at it, restructure a bit to ease adding
>> post-processing for other commands.
>>
>> Signed-off-by: Tejun Heo <htejun@gmail.com>
> 
> BTW, while doing the TEST UNIT READY emulation patch for ATA (recently
> withdrawn from libata-dev.git#upstream), I found a problem with the
> interface that was difficult to get around:  TEST UNIT READY simulation
> code really wants to look at the result TF of CHECK POWER MODE, even if
> ATA_ERR is asserted, before determining whether or not to call that
> command an error.
> 
> Maybe the EH scheduling could be moved until after ->complete_fn, to
> permit ->complete_fn users to manipulate qc->err_mask etc.?

Yeah, right.  Device error is a special case.  In many cases, devices
can operate without any problem after asserting error and for some
commands error is used to signal certain conditions.  I think the least
intrusive way would be a qc flag - ATA_QCFLAG_ALLOW_DEVERR, maybe.
Also, I'm not sure whether EH should kick in when passthru commands fail
with a device error.  Maybe libata should just report the error to the
issuer and continue operation?

-- 
tejun

Re: [PATCH #upstream 1/2] libata: move command post processing to __ata_qc_complete()

[Jeff Garzik]

Tejun Heo wrote:
> Jeff Garzik wrote:
>> Tejun Heo wrote:
>>> Some commands need post-processing after successful completion.  This
>>> was done in ata_scsi_qc_complete() till now but command post
>>> processing doesn't belong to SAT layer.  Move them to
>>> __ata_qc_complete() and, while at it, restructure a bit to ease adding
>>> post-processing for other commands.
>>>
>>> Signed-off-by: Tejun Heo <htejun@gmail.com>
>> BTW, while doing the TEST UNIT READY emulation patch for ATA (recently
>> withdrawn from libata-dev.git#upstream), I found a problem with the
>> interface that was difficult to get around:  TEST UNIT READY simulation
>> code really wants to look at the result TF of CHECK POWER MODE, even if
>> ATA_ERR is asserted, before determining whether or not to call that
>> command an error.
>>
>> Maybe the EH scheduling could be moved until after ->complete_fn, to
>> permit ->complete_fn users to manipulate qc->err_mask etc.?
> 
> Yeah, right.  Device error is a special case.  In many cases, devices
> can operate without any problem after asserting error and for some
> commands error is used to signal certain conditions.  I think the least
> intrusive way would be a qc flag - ATA_QCFLAG_ALLOW_DEVERR, maybe.
> Also, I'm not sure whether EH should kick in when passthru commands fail
> with a device error.  Maybe libata should just report the error to the
> issuer and continue operation?

Good point and agreed -- I definitely think passthru commands want 
device errors immediately.

That vastly increases the utility of passthru to be used in test suites 
and stuff, where you know a lot of operations should fail, by design. 
(i.e. intentionally submitting an invalid command, to test that 'command 
aborted' is returned)

	Jeff

Re: [PATCH #upstream 1/2] libata: move command post processing to __ata_qc_complete()

[Bruce Allen]

>>> BTW, while doing the TEST UNIT READY emulation patch for ATA (recently
>>> withdrawn from libata-dev.git#upstream), I found a problem with the
>>> interface that was difficult to get around:  TEST UNIT READY simulation
>>> code really wants to look at the result TF of CHECK POWER MODE, even if
>>> ATA_ERR is asserted, before determining whether or not to call that
>>> command an error.
>>> 
>>> Maybe the EH scheduling could be moved until after ->complete_fn, to
>>> permit ->complete_fn users to manipulate qc->err_mask etc.?
>> 
>> Yeah, right.  Device error is a special case.  In many cases, devices
>> can operate without any problem after asserting error and for some
>> commands error is used to signal certain conditions.  I think the least
>> intrusive way would be a qc flag - ATA_QCFLAG_ALLOW_DEVERR, maybe.
>> Also, I'm not sure whether EH should kick in when passthru commands fail
>> with a device error.  Maybe libata should just report the error to the
>> issuer and continue operation?
>
> Good point and agreed -- I definitely think passthru commands want device 
> errors immediately.
>
> That vastly increases the utility of passthru to be used in test suites 
> and stuff, where you know a lot of operations should fail, by design. 
> (i.e. intentionally submitting an invalid command, to test that 'command 
> aborted' is returned)

Absolutely!  Speaking as an 'application programmer', much of the utility 
of passthru is lost if the device errors are not immediately returned to 
the caller.

Cheers,
 	Bruce




>
> 	Jeff
>
>