* [PATCH #upstream 1/2] libata: move command post processing to __ata_qc_complete() @ 2007-10-12 11:56 Tejun Heo 2007-10-12 11:56 ` [PATCH #upstream 2/2] libata: track SLEEP state and issue SRST to wake it up Tejun Heo 2007-10-12 12:13 ` [PATCH #upstream 1/2] libata: move command post processing to __ata_qc_complete() Jeff Garzik 0 siblings, 2 replies; 9+ messages in thread From: Tejun Heo @ 2007-10-12 11:56 UTC (permalink / raw) To: Jeff Garzik, linux-ide, ballen, andrew 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH #upstream 2/2] libata: track SLEEP state and issue SRST to wake it up 2007-10-12 11:56 [PATCH #upstream 1/2] libata: move command post processing to __ata_qc_complete() Tejun Heo @ 2007-10-12 11:56 ` Tejun Heo 2007-10-13 4:57 ` Andrew Paprocki 2007-10-12 12:13 ` [PATCH #upstream 1/2] libata: move command post processing to __ata_qc_complete() Jeff Garzik 1 sibling, 1 reply; 9+ messages in thread From: Tejun Heo @ 2007-10-12 11:56 UTC (permalink / raw) To: Jeff Garzik, linux-ide, ballen, andrew 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; ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH #upstream 2/2] libata: track SLEEP state and issue SRST to wake it up 2007-10-12 11:56 ` [PATCH #upstream 2/2] libata: track SLEEP state and issue SRST to wake it up Tejun Heo @ 2007-10-13 4:57 ` Andrew Paprocki 2007-10-13 13:55 ` Tejun Heo 0 siblings, 1 reply; 9+ messages in thread From: Andrew Paprocki @ 2007-10-13 4:57 UTC (permalink / raw) To: Tejun Heo; +Cc: Jeff Garzik, linux-ide, Bruce Allen 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; > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH #upstream 2/2] libata: track SLEEP state and issue SRST to wake it up 2007-10-13 4:57 ` Andrew Paprocki @ 2007-10-13 13:55 ` Tejun Heo 2007-10-14 1:02 ` Andrew Paprocki 0 siblings, 1 reply; 9+ messages in thread From: Tejun Heo @ 2007-10-13 13:55 UTC (permalink / raw) To: Andrew Paprocki; +Cc: Jeff Garzik, linux-ide, Bruce Allen 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH #upstream 2/2] libata: track SLEEP state and issue SRST to wake it up 2007-10-13 13:55 ` Tejun Heo @ 2007-10-14 1:02 ` Andrew Paprocki 0 siblings, 0 replies; 9+ messages in thread From: Andrew Paprocki @ 2007-10-14 1:02 UTC (permalink / raw) To: Tejun Heo; +Cc: Jeff Garzik, linux-ide, Bruce Allen 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. ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH #upstream 1/2] libata: move command post processing to __ata_qc_complete() 2007-10-12 11:56 [PATCH #upstream 1/2] libata: move command post processing to __ata_qc_complete() Tejun Heo 2007-10-12 11:56 ` [PATCH #upstream 2/2] libata: track SLEEP state and issue SRST to wake it up Tejun Heo @ 2007-10-12 12:13 ` Jeff Garzik 2007-10-13 13:09 ` Tejun Heo 1 sibling, 1 reply; 9+ messages in thread From: Jeff Garzik @ 2007-10-12 12:13 UTC (permalink / raw) To: Tejun Heo; +Cc: linux-ide, ballen, andrew 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH #upstream 1/2] libata: move command post processing to __ata_qc_complete() 2007-10-12 12:13 ` [PATCH #upstream 1/2] libata: move command post processing to __ata_qc_complete() Jeff Garzik @ 2007-10-13 13:09 ` Tejun Heo 2007-10-18 1:23 ` Jeff Garzik 0 siblings, 1 reply; 9+ messages in thread From: Tejun Heo @ 2007-10-13 13:09 UTC (permalink / raw) To: Jeff Garzik; +Cc: linux-ide, ballen, andrew 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH #upstream 1/2] libata: move command post processing to __ata_qc_complete() 2007-10-13 13:09 ` Tejun Heo @ 2007-10-18 1:23 ` Jeff Garzik 2007-10-18 3:55 ` Bruce Allen 0 siblings, 1 reply; 9+ messages in thread From: Jeff Garzik @ 2007-10-18 1:23 UTC (permalink / raw) To: Tejun Heo; +Cc: linux-ide, ballen, andrew 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH #upstream 1/2] libata: move command post processing to __ata_qc_complete() 2007-10-18 1:23 ` Jeff Garzik @ 2007-10-18 3:55 ` Bruce Allen 0 siblings, 0 replies; 9+ messages in thread From: Bruce Allen @ 2007-10-18 3:55 UTC (permalink / raw) To: Jeff Garzik; +Cc: Tejun Heo, linux-ide, andrew >>> 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 > > ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2007-10-18 4:51 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2007-10-12 11:56 [PATCH #upstream 1/2] libata: move command post processing to __ata_qc_complete() Tejun Heo 2007-10-12 11:56 ` [PATCH #upstream 2/2] libata: track SLEEP state and issue SRST to wake it up Tejun Heo 2007-10-13 4:57 ` Andrew Paprocki 2007-10-13 13:55 ` Tejun Heo 2007-10-14 1:02 ` Andrew Paprocki 2007-10-12 12:13 ` [PATCH #upstream 1/2] libata: move command post processing to __ata_qc_complete() Jeff Garzik 2007-10-13 13:09 ` Tejun Heo 2007-10-18 1:23 ` Jeff Garzik 2007-10-18 3:55 ` Bruce Allen
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.