Tejun Heo wrote:
And this is the combined patch against ncq head of libata-dev-2.6
tree. Commit d032ec9048ff82a704b96b93cfd6f2e8e3a06b19.
Only ata_piix, sata_sil and ahci are converted and all other SATA
drivers are broken. So, enable only those three SATA drivers when
compiling with this patch.
Overall, the approach of adding a timeout to the 'special' commands is a
good, and necessary approach.
Comments follow...
Index: work/drivers/scsi/libata-core.c
===================================================================
--- work.orig/drivers/scsi/libata-core.c 2005-08-05 12:55:08.000000000
+0900
+++ work/drivers/scsi/libata-core.c 2005-08-05 12:55:50.000000000 +0900
@@ -49,6 +49,10 @@
#include "libata.h"
+#define ata_for_each_tag(tag, mask) \
+ for (tag = find_first_bit(&mask, ATA_MAX_CMDS); tag < ATA_MAX_CMDS; \
+ tag = find_next_bit(&mask, ATA_MAX_CMDS, tag + 1))
+
static unsigned int ata_busy_sleep (struct ata_port *ap,
unsigned long tmout_pat,
unsigned long tmout);
@@ -59,7 +63,6 @@ static int fgb(u32 bitmap);
static int ata_choose_xfer_mode(struct ata_port *ap,
u8 *xfer_mode_out,
unsigned int *xfer_shift_out);
-static int ata_qc_complete_noop(struct ata_queued_cmd *qc, u8 drv_stat);
static void __ata_qc_complete(struct ata_queued_cmd *qc);
static unsigned int ata_unique_id = 1;
@@ -70,6 +73,87 @@ MODULE_DESCRIPTION("Library module for A
MODULE_LICENSE("GPL");
MODULE_VERSION(DRV_VERSION);
+static void ata_qc_complete_noop(struct ata_queued_cmd *qc)
+{
+ /* noop */
+}
+
+static void ata_qc_exec_special_timeout(unsigned long data)
+{
+ struct completion *wait = (void *)data;
+ complete(wait);
+}
+
+/**
+ * ata_qc_exec_special - execute libata internal special command
+ * @qc: Command to execute
+ * @tmout: timeout in jiffies
+ *
+ * Executes libata internal command with timeout. Timeout and
+ * error conditions are reported via return value. No recovery
+ * action is taken after a command times out. It's caller's duto
+ * to clean up after timeout.
+ *
+ * Also, note that error condition is checked after the qc is
+ * completed, meaning that if another command is issued before
+ * checking the condition, we get the wrong values. As special
+ * cmds are used only for initialization and recovery, this
+ * won't cause any problem currently.
+ *
+ * LOCKING:
+ * None. Should be called with kernel context, might sleep.
+ */
+
+static int ata_qc_exec_special(struct ata_queued_cmd *qc, unsigned long tmout)
+{
+ struct ata_port *ap = qc->ap;
+ DECLARE_COMPLETION(wait);
+ struct timer_list timer;
+ int rc;
+
+ might_sleep();
+
+ if (ata_busy_sleep(ap, ATA_TMOUT_SPECIAL_QUICK, ATA_TMOUT_SPECIAL))
+ return -ETIMEDOUT;
+
+ qc->complete_fn = ata_qc_complete_noop;
+ qc->waiting = &wait;
+
+ if (tmout) {
+ init_timer(&timer);
+ timer.function = ata_qc_exec_special_timeout;
+ timer.data = (unsigned long)&wait;
+ timer.expires = jiffies + tmout;
+ add_timer(&timer);
Just use mod_timer(), and combine these last two lines into a single one.
+ spin_lock_irq(&ap->host_set->lock);
+ rc = ata_qc_issue(qc);
+ spin_unlock_irq(&ap->host_set->lock);
+
+ if (rc) {
+ if (tmout)
+ del_timer(&timer);
Use del_timer_sync() here, and for every case where you are not inside a
spinlock.
+ return -EAGAIN; /* any better value for issue failure? */
+ }
+
+ wait_for_completion(&wait);
+
+ if (tmout && !del_timer(&timer)) {
+ spin_lock_irq(&ap->host_set->lock);
+ if (qc->waiting == &wait) {
+ ata_qc_complete(qc);
+ rc = -ETIMEDOUT;
+ }
+ spin_unlock_irq(&ap->host_set->lock);
+ }
+
+ if (rc == 0 && !ata_ok(ata_chk_status(ap)))
+ rc = -EIO;
General comment on libata: we should move away from using the Status
register directly, and more towards an indication that the taskfile
described by the queue_cmd is in error. i.e. add an 'error' flag or
something like that.
The rest seems fairly sane.
General comment on libata error handling: as soon as SATA ATAPI is
complete, we can move libata to using SCSI's fine-grained error handling
hooks (eh_{abort,timeout,bus,host}_handler), which will make error
handling easier, and avoid the problems that keep cropping up with the
->eh_strategy_handler() approach.
Jeff
-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
