libata: implement ata_qc_exec_internal()
PATCH 1/1 libata: implement ata_qc_exec_internal()
Each user libata internal commands initializes a qc, issues it, waits for its completion and checks for errors. This patch factors internal command execution sequence into ata_qc_exec_internal(). The function also removes race condition between irq and timeout handling.
drivers/scsi/libata-core.c +76 -66
@@ -1047,28 +1047,73 @@ static unsigned int ata_pio_modes(constreturn modes; } -static int ata_qc_wait_err (struct ata_queued_cmd *qc, -struct completion *wait) +/** +* ata_qc_exec_internal - execute libata internal command +* @qc: Command to execute +* +* 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 duty +* 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 internal +* cmds are used only for initialization and recovery, this +* shouldn't cause any problem. +* +* LOCKING: +* None. Should be called with kernel context, might sleep. +*/ + +static unsigned ata_qc_exec_internal (struct ata_queued_cmd *qc) { -int rc =0 ; +struct ata_port *ap = qc->ap; +DECLARE_COMPLETION (wait); +unsigned long tmout_left; +unsigned int err_mask; +int rc; -if (wait_for_completion_timeout (wait,30 *HZ ) <1 ) { -/* timeout handling */ -unsigned int err_mask =ac_err_mask (ata_chk_status (qc->ap)); +if (ata_busy_sleep (ap,ATA_TMOUT_INTERNAL_QUICK ,ATA_TMOUT_INTERNAL )) +return -ETIMEDOUT ; -if (!err_mask) { + qc->complete_fn = ata_qc_complete_noop; + qc->waiting = &wait; + +spin_lock_irq (&ap->host_set->lock); + rc =ata_qc_issue (qc); +spin_unlock_irq (&ap->host_set->lock); + +if (rc) +return AC_ERR_OTHER ; + + tmout_left =wait_for_completion_timeout (&wait,ATA_TMOUT_INTERNAL ); + err_mask =ac_err_mask (ata_chk_status (qc->ap)); + +if (!tmout_left) { +/* timeout handling */ +if (!err_mask)printk (KERN_WARNING "ata%u: slow completion (cmd %x)\n" , qc->ap->id, qc->tf.command); - }else { +else printk (KERN_WARNING "ata%u: qc timeout (cmd %x)\n" , qc->ap->id, qc->tf.command); - rc = -EIO ; - } -ata_qc_complete (qc, err_mask); +spin_lock_irq (&ap->host_set->lock); + +/* We're racing with irq here. If we lose, the +* following test prevents us from completing the qc +* again. If completion irq occurs after here but +* before the caller cleans up, it will result in a +* spurious interrupt. We can live with that. +*/ +if (qc->flags &ATA_QCFLAG_ACTIVE ) +ata_qc_complete (qc, err_mask); + +spin_unlock_irq (&ap->host_set->lock); } -return rc; +return err_mask; }/**
@@ -1100,9 +1145,8 @@ static void ata_dev_identify(struct ata_u16 tmp;unsigned long xfer_modes;unsigned int using_edd; -DECLARE_COMPLETION (wait);struct ata_queued_cmd *qc; -unsigned long flags; +unsigned int err_mask;int rc;if (!ata_dev_present (dev)) {
@@ -1140,23 +1184,18 @@ retry:DPRINTK ("do ATAPI identify\n" ); } - qc->waiting = &wait; - qc->complete_fn = ata_qc_complete_noop; + err_mask =ata_qc_exec_internal (qc); -spin_lock_irqsave (&ap->host_set->lock, flags); - rc =ata_qc_issue (qc); -spin_unlock_irqrestore (&ap->host_set->lock, flags); +if (err_mask) { +unsigned long flags; -if (rc) - goto err_out; -else -ata_qc_wait_err (qc, &wait); +if (err_mask & ~AC_ERR_DEV ) + goto err_out; -spin_lock_irqsave (&ap->host_set->lock, flags); - ap->ops->tf_read (ap, &qc->tf); -spin_unlock_irqrestore (&ap->host_set->lock, flags); +spin_lock_irqsave (&ap->host_set->lock, flags); + ap->ops->tf_read (ap, &qc->tf); +spin_unlock_irqrestore (&ap->host_set->lock, flags); -if (qc->tf.command &ATA_ERR ) {/* * arg! EDD works for all test cases, but seems to return * the ATA signature for some ATAPI devices. Until the
@@ -2288,10 +2327,7 @@ static int ata_choose_xfer_mode(const ststatic void ata_dev_set_xfermode (struct ata_port *ap,struct ata_device *dev) { -DECLARE_COMPLETION (wait);struct ata_queued_cmd *qc; -int rc; -unsigned long flags;/* set up set-features taskfile */ DPRINTK ("set features - xfer mode\n" );
@@ -2305,17 +2341,11 @@ static void ata_dev_set_xfermode(struct qc->tf.protocol =ATA_PROT_NODATA ; qc->tf.nsect = dev->xfer_mode; - qc->waiting = &wait; - qc->complete_fn = ata_qc_complete_noop; - -spin_lock_irqsave (&ap->host_set->lock, flags); - rc =ata_qc_issue (qc); -spin_unlock_irqrestore (&ap->host_set->lock, flags); - -if (rc) +if (ata_qc_exec_internal (qc)) { +printk (KERN_ERR "ata%u: failed to set xfermode, disabled\n" , + ap->id);ata_port_disable (ap); -else -ata_qc_wait_err (qc, &wait); + }DPRINTK ("EXIT\n" ); }
@@ -2330,10 +2360,7 @@ static void ata_dev_set_xfermode(structstatic void ata_dev_reread_id (struct ata_port *ap,struct ata_device *dev) { -DECLARE_COMPLETION (wait);struct ata_queued_cmd *qc; -unsigned long flags; -int rc; qc =ata_qc_new_init (ap, dev);BUG_ON (qc ==NULL );
@@ -2353,18 +2380,9 @@ static void ata_dev_reread_id(struct ata qc->tf.protocol =ATA_PROT_PIO ; qc->nsect =1 ; - qc->waiting = &wait; - qc->complete_fn = ata_qc_complete_noop; - -spin_lock_irqsave (&ap->host_set->lock, flags); - rc =ata_qc_issue (qc); -spin_unlock_irqrestore (&ap->host_set->lock, flags); - -if (rc) +if (ata_qc_exec_internal (qc)) goto err_out; -ata_qc_wait_err (qc, &wait); -swap_buf_le16 (dev->id,ATA_ID_WORDS );ata_dump_id (dev);
@@ -2373,6 +2391,7 @@ static void ata_dev_reread_id(struct atareturn ; err_out: +printk (KERN_ERR "ata%u: failed to reread ID, disabled\n" , ap->id);ata_port_disable (ap); }
@@ -2386,10 +2405,7 @@ err_out:static void ata_dev_init_params (struct ata_port *ap,struct ata_device *dev) { -DECLARE_COMPLETION (wait);struct ata_queued_cmd *qc; -int rc; -unsigned long flags;u16 sectors = dev->id[6 ];u16 heads = dev->id[3 ];
@@ -2409,17 +2425,11 @@ static void ata_dev_init_params(struct a qc->tf.nsect = sectors; qc->tf.device |= (heads -1 ) &0x0f ;/* max head = num. of heads - 1 */ - qc->waiting = &wait; - qc->complete_fn = ata_qc_complete_noop; - -spin_lock_irqsave (&ap->host_set->lock, flags); - rc =ata_qc_issue (qc); -spin_unlock_irqrestore (&ap->host_set->lock, flags); - -if (rc) +if (ata_qc_exec_internal (qc)) { +printk (KERN_ERR "ata%u: failed to init parameters, disabled\n" , + ap->id);ata_port_disable (ap); -else -ata_qc_wait_err (qc, &wait); + }DPRINTK ("EXIT\n" ); }
drivers/scsi/libata.h +0 -0
include/linux/libata.h +2 -0
@@ -136,6 +136,8 @@ enum {ATA_TMOUT_BOOT_QUICK =7 *HZ ,/* hueristic */ ATA_TMOUT_DATAOUT =30 *HZ ,ATA_TMOUT_DATAOUT_QUICK =5 *HZ , +ATA_TMOUT_INTERNAL =30 *HZ , +ATA_TMOUT_INTERNAL_QUICK =5 *HZ ,/* ATA bus states */ BUS_UNKNOWN = 0,
does it solve the cpu usage ?
Quoted 260 lines · Tejun Heo
Each user libata internal commands initializes a qc, issues it, waits for its completion and checks for errors. This patch factors internal command execution sequence into
ata_qc_exec_internal(). The function also removes race condition between irq and timeout handling.Signed-off-by Tejun Heo <[email protected]>drivers/scsi/libata-core.c +76
-66@@ -1047,28 +1047,73 @@ static unsigned int ata_pio_modes(constreturn modes; } -static int ata_qc_wait_err (struct ata_queued_cmd *qc, -struct completion *wait) +/** +* ata_qc_exec_internal - execute libata internal command +* @qc: Command to execute +* +* 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 duty +* 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 internal +* cmds are used only for initialization and recovery, this +* shouldn't cause any problem. +* +* LOCKING: +* None. Should be called with kernel context, might sleep. +*/ + +static unsigned ata_qc_exec_internal (struct ata_queued_cmd *qc) { -int rc =0 ; +struct ata_port *ap = qc->ap; +DECLARE_COMPLETION (wait); +unsigned long tmout_left; +unsigned int err_mask; +int rc; -if (wait_for_completion_timeout (wait,30 *HZ ) <1 ) { -/* timeout handling */ -unsigned int err_mask =ac_err_mask (ata_chk_status (qc->ap)); +if (ata_busy_sleep (ap,ATA_TMOUT_INTERNAL_QUICK ,ATA_TMOUT_INTERNAL )) +return -ETIMEDOUT ; -if (!err_mask) { + qc->complete_fn = ata_qc_complete_noop; + qc->waiting = &wait; + +spin_lock_irq (&ap->host_set->lock); + rc =ata_qc_issue (qc); +spin_unlock_irq (&ap->host_set->lock); + +if (rc) +return AC_ERR_OTHER ; + + tmout_left =wait_for_completion_timeout (&wait,ATA_TMOUT_INTERNAL ); + err_mask =ac_err_mask (ata_chk_status (qc->ap)); + +if (!tmout_left) { +/* timeout handling */ +if (!err_mask)printk (KERN_WARNING "ata%u: slow completion (cmd %x)\n" , qc->ap->id, qc->tf.command); - }else { +else printk (KERN_WARNING "ata%u: qc timeout (cmd %x)\n" , qc->ap->id, qc->tf.command); - rc = -EIO ; - } -ata_qc_complete (qc, err_mask); +spin_lock_irq (&ap->host_set->lock); + +/* We're racing with irq here. If we lose, the +* following test prevents us from completing the qc +* again. If completion irq occurs after here but +* before the caller cleans up, it will result in a +* spurious interrupt. We can live with that. +*/ +if (qc->flags &ATA_QCFLAG_ACTIVE ) +ata_qc_complete (qc, err_mask); + +spin_unlock_irq (&ap->host_set->lock); } -return rc; +return err_mask; }/** @@ -1100,9 +1145,8 @@ static void ata_dev_identify(struct ata_u16 tmp;unsigned long xfer_modes;unsigned int using_edd; -DECLARE_COMPLETION (wait);struct ata_queued_cmd *qc; -unsigned long flags; +unsigned int err_mask;int rc;if (!ata_dev_present (dev)) {@@ -1140,23 +1184,18 @@ retry:DPRINTK ("do ATAPI identify\n" ); } - qc->waiting = &wait; - qc->complete_fn = ata_qc_complete_noop; + err_mask =ata_qc_exec_internal (qc); -spin_lock_irqsave (&ap->host_set->lock, flags); - rc =ata_qc_issue (qc); -spin_unlock_irqrestore (&ap->host_set->lock, flags); +if (err_mask) { +unsigned long flags; -if (rc) - goto err_out; -else -ata_qc_wait_err (qc, &wait); +if (err_mask & ~AC_ERR_DEV ) + goto err_out; -spin_lock_irqsave (&ap->host_set->lock, flags); - ap->ops->tf_read (ap, &qc->tf); -spin_unlock_irqrestore (&ap->host_set->lock, flags); +spin_lock_irqsave (&ap->host_set->lock, flags); + ap->ops->tf_read (ap, &qc->tf); +spin_unlock_irqrestore (&ap->host_set->lock, flags); -if (qc->tf.command &ATA_ERR ) {/* * arg! EDD works for all test cases, but seems to return * the ATA signature for some ATAPI devices. Until the @@ -2288,10 +2327,7 @@ static int ata_choose_xfer_mode(const ststatic void ata_dev_set_xfermode (struct ata_port *ap,struct ata_device *dev) { -DECLARE_COMPLETION (wait);struct ata_queued_cmd *qc; -int rc; -unsigned long flags;/* set up set-features taskfile */ DPRINTK ("set features - xfer mode\n" );@@ -2305,17 +2341,11 @@ static void ata_dev_set_xfermode(struct qc->tf.protocol =ATA_PROT_NODATA ; qc->tf.nsect = dev->xfer_mode; - qc->waiting = &wait; - qc->complete_fn = ata_qc_complete_noop; - -spin_lock_irqsave (&ap->host_set->lock, flags); - rc =ata_qc_issue (qc); -spin_unlock_irqrestore (&ap->host_set->lock, flags); - -if (rc) +if (ata_qc_exec_internal (qc)) { +printk (KERN_ERR "ata%u: failed to set xfermode, disabled\n" , + ap->id);ata_port_disable (ap); -else -ata_qc_wait_err (qc, &wait); + }DPRINTK ("EXIT\n" ); }@@ -2330,10 +2360,7 @@ static void ata_dev_set_xfermode(structstatic void ata_dev_reread_id (struct ata_port *ap,struct ata_device *dev) { -DECLARE_COMPLETION (wait);struct ata_queued_cmd *qc; -unsigned long flags; -int rc; qc =ata_qc_new_init (ap, dev);BUG_ON (qc ==NULL );@@ -2353,18 +2380,9 @@ static void ata_dev_reread_id(struct ata qc->tf.protocol =ATA_PROT_PIO ; qc->nsect =1 ; - qc->waiting = &wait; - qc->complete_fn = ata_qc_complete_noop; - -spin_lock_irqsave (&ap->host_set->lock, flags); - rc =ata_qc_issue (qc); -spin_unlock_irqrestore (&ap->host_set->lock, flags); - -if (rc) +if (ata_qc_exec_internal (qc)) goto err_out; -ata_qc_wait_err (qc, &wait); -swap_buf_le16 (dev->id,ATA_ID_WORDS );ata_dump_id (dev);@@ -2373,6 +2391,7 @@ static void ata_dev_reread_id(struct atareturn ; err_out: +printk (KERN_ERR "ata%u: failed to reread ID, disabled\n" , ap->id);ata_port_disable (ap); }@@ -2386,10 +2405,7 @@ err_out:static void ata_dev_init_params (struct ata_port *ap,struct ata_device *dev) { -DECLARE_COMPLETION (wait);struct ata_queued_cmd *qc; -int rc; -unsigned long flags;u16 sectors = dev->id[6 ];u16 heads = dev->id[3 ];@@ -2409,17 +2425,11 @@ static void ata_dev_init_params(struct a qc->tf.nsect = sectors; qc->tf.device |= (heads -1 ) &0x0f ;/* max head = num. of heads - 1 */ - qc->waiting = &wait; - qc->complete_fn = ata_qc_complete_noop; - -spin_lock_irqsave (&ap->host_set->lock, flags); - rc =ata_qc_issue (qc); -spin_unlock_irqrestore (&ap->host_set->lock, flags); - -if (rc) +if (ata_qc_exec_internal (qc)) { +printk (KERN_ERR "ata%u: failed to init parameters, disabled\n" , + ap->id);ata_port_disable (ap); -else -ata_qc_wait_err (qc, &wait); + }DPRINTK ("EXIT\n" ); }drivers/scsi/libata.h +0
-0include/linux/libata.h +2
-1@@ -136,6 +136,8 @@ enum {ATA_TMOUT_BOOT_QUICK =7 *HZ ,/* hueristic */ ATA_TMOUT_DATAOUT =30 *HZ ,ATA_TMOUT_DATAOUT_QUICK =5 *HZ , +ATA_TMOUT_INTERNAL =30 *HZ , +ATA_TMOUT_INTERNAL_QUICK =5 *HZ ,/* ATA bus states */ BUS_UNKNOWN = 0, -
Quoted 4 lines
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
--
Raz
Quoted 11 lines · Raz Ben-Jehuda(caro)
does it solve the cpu usage ?
Quoted 7 lines · Tejun Heo
Each user libata internal commands initializes a qc, issues it, waits for its completion and checks for errors. This patch factors internal command execution sequence into
ata_qc_exec_internal(). The function also removes race condition between irq and timeout handling.Signed-off-by Tejun Heo <[email protected]>
Hi, Raz.
It propabably has nothing to do with the cpu usage problem on piix you reported. I think you need to post more data to get people on the list interested in your problem. The followings may help.
1. Please report exactly which kernel you're using and more information on the hardware will also help.
2. Don't use raid or any fs on it. Just perform tests on raw /dev/sdx devices. Do something like 'time dd if=/dev/sdx of=/dev/zero bs=1M count=1024' and report how much user/system time it uses.
3. If above shows large system time consumption, build your kernel with oprofile. Start recording non-idle cpu cycles (the default) including the kernel. Repeat test in #2. Dump oprofile result and post that here.
Hope it helps.
Signature · 1 line
tejun
Quoted 14 lines · Tejun Heo
Quoted 11 lines · Raz Ben-Jehuda(caro)
does it solve the cpu usage ?
Quoted 7 lines · Tejun Heo
Each user libata internal commands initializes a qc, issues it, waits for its completion and checks for errors. This patch factors internal command execution sequence into
ata_qc_exec_internal(). The function also removes race condition between irq and timeout handling.Signed-off-by Tejun Heo <[email protected]>
Quoted 7 lines
Hi, Raz.
It propabably has nothing to do with the cpu usage problem on piix you reported. I think you need to post more data to get people on the list interested in your problem. The followings may help.
1. Please report exactly which kernel you're using and more information on the hardware will also help.
2. Don't use raid or any fs on it. Just perform tests on raw
/dev/sdxdevices. Do something like 'time dd if=/dev/sdx of=/dev/zero bs=1M count=1024' and report how much user/system time it uses.
Oops, count=1024 is way too small. Do something like bs=1M count=1M or maybe count=10M.
Quoted 5 lines
3. If above shows large system time consumption, build your kernel with oprofile. Start recording non-idle cpu cycles (the default) including the kernel. Repeat test in #2. Dump oprofile result and post that here.
Hope it helps.
Signature · 1 line
tejun
Quoted 18 lines · Tejun Heo
Quoted 16 lines · Tejun Heo
Quoted 11 lines · Raz Ben-Jehuda(caro)
does it solve the cpu usage ?
Quoted 7 lines · Tejun Heo
Each user libata internal commands initializes a qc, issues it, waits for its completion and checks for errors. This patch factors internal command execution sequence into
ata_qc_exec_internal(). The function also removes race condition between irq and timeout handling.Signed-off-by Tejun Heo <[email protected]>Hi, Raz.
Quoted 8 lines
Quoted 5 lines
It propabably has nothing to do with the cpu usage problem on piix you reported. I think you need to post more data to get people on the list interested in your problem. The followings may help.
1. Please report exactly which kernel you're using and more information on the hardware will also help.
2. Don't use raid or any fs on it. Just perform tests on raw
/dev/sdxdevices. Do something like 'time dd if=/dev/sdx of=/dev/zero bs=1M count=1024' and report how much user/system time it uses.Oops, count=1024 is way too small. Do something like bs=1M count=1M or maybe count=10M.
I hate myself. I must be on crack or something now. count=1024 should be fine (count=2k or 4k should be fine too). Don't do count=1M or 10M.
Sorry guys. :-)
Signature · 1 line
tejun
Quoted 54 lines · Tejun Heo
Each user libata internal commands initializes a qc, issues it, waits for its completion and checks for errors. This patch factors internal command execution sequence into
ata_qc_exec_internal(). The function also removes race condition between irq and timeout handling.Signed-off-by Tejun Heo <[email protected]>drivers/scsi/libata-core.c +29
-7@@ -1047,28 +1047,73 @@ static unsigned int ata_pio_modes(constreturn modes; } -static int ata_qc_wait_err (struct ata_queued_cmd *qc, -struct completion *wait) +/** +* ata_qc_exec_internal - execute libata internal command +* @qc: Command to execute +* +* 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 duty +* 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 internal +* cmds are used only for initialization and recovery, this +* shouldn't cause any problem. +* +* LOCKING: +* None. Should be called with kernel context, might sleep. +*/ + +static unsigned ata_qc_exec_internal (struct ata_queued_cmd *qc) { -int rc =0 ; +struct ata_port *ap = qc->ap; +DECLARE_COMPLETION (wait); +unsigned long tmout_left; +unsigned int err_mask; +int rc; -if (wait_for_completion_timeout (wait,30 *HZ ) <1 ) { -/* timeout handling */ -unsigned int err_mask =ac_err_mask (ata_chk_status (qc->ap)); +if (ata_busy_sleep (ap,ATA_TMOUT_INTERNAL_QUICK ,ATA_TMOUT_INTERNAL )) +return -ETIMEDOUT ; -if (!err_mask) { + qc->complete_fn = ata_qc_complete_noop; + qc->waiting = &wait;
drivers/scsi/libata-core.c static unsigned int ata_pio_modes(const · line 1091
Quoted 10 lines
drivers/scsi/libata-core.c +10
-0+ +spin_lock_irq (&ap->host_set->lock); + rc =ata_qc_issue (qc); +spin_unlock_irq (&ap->host_set->lock); + +if (rc) +return AC_ERR_OTHER ; + + tmout_left =wait_for_completion_timeout (&wait,ATA_TMOUT_INTERNAL ); + err_mask =ac_err_mask (ata_chk_status (qc->ap));
Could we get the err_mask from the qc->complete_fn?
(For some case where the software finds something wrong, but the device ata_chk_status(qc->ap) is zero.)
Albert
Quoted 32 lines
drivers/scsi/libata-core.c +18
-5+ +if (!tmout_left) { +/* timeout handling */ +if (!err_mask)printk (KERN_WARNING "ata%u: slow completion (cmd %x)\n" , qc->ap->id, qc->tf.command); - }else { +else printk (KERN_WARNING "ata%u: qc timeout (cmd %x)\n" , qc->ap->id, qc->tf.command); - rc = -EIO ; - } -ata_qc_complete (qc, err_mask); +spin_lock_irq (&ap->host_set->lock); + +/* We're racing with irq here. If we lose, the +* following test prevents us from completing the qc +* again. If completion irq occurs after here but +* before the caller cleans up, it will result in a +* spurious interrupt. We can live with that. +*/ +if (qc->flags &ATA_QCFLAG_ACTIVE ) +ata_qc_complete (qc, err_mask); + +spin_unlock_irq (&ap->host_set->lock); } -return rc; +return err_mask; }
PATCH 1/2 libata: implement ata_exec_internal()
Each user libata internal commands initializes a qc, issues it, waits for its completion and checks for errors. This patch factors internal command execution sequence into ata_qc_exec_internal(). This change fixes the following bugs.
* qc not freed on issue failure
* ap->qactive clearing could race with the next internal command
* race between timeout handling and irq
* ignoring error condition not represented in tf->status
Also, qc & hardware are not accessed anymore once it's completed, making internal commands more conformant with general semantics. This change also makes it easy to issue internal commands from multiple threads if that becomes necessary.
--
This is take #2 of ata_exec_internal(). qc allocation and tf reading-back have also been moved into ata_exec_internal(). Now all qc handling is done while properly holding host_set lock removing above mentioned rare race conditon (the second item).
drivers/scsi/libata-core.c +152 -122
@@ -1047,28 +1047,119 @@ static unsigned int ata_pio_modes(constreturn modes; } -static int ata_qc_wait_err (struct ata_queued_cmd *qc, -struct completion *wait) +struct ata_exec_internal_arg { +unsigned int err_mask; +struct ata_taskfile *tf; +struct completion *waiting; +}; + +int ata_qc_complete_internal (struct ata_queued_cmd *qc,unsigned int err_mask) { -int rc =0 ; +struct ata_exec_internal_arg *arg = qc->complete_data; +struct completion *waiting = arg->waiting; -if (wait_for_completion_timeout (wait,30 *HZ ) <1 ) { -/* timeout handling */ -unsigned int err_mask =ac_err_mask (ata_chk_status (qc->ap)); - -if (!err_mask) { -printk (KERN_WARNING "ata%u: slow completion (cmd %x)\n" , - qc->ap->id, qc->tf.command); - }else { -printk (KERN_WARNING "ata%u: qc timeout (cmd %x)\n" , - qc->ap->id, qc->tf.command); - rc = -EIO ; - } +if (!(err_mask & ~AC_ERR_DEV )) + qc->ap->ops->tf_read (qc->ap, arg->tf); + arg->err_mask = err_mask; + arg->waiting =NULL ; +complete (waiting); + +return 0 ; +} + +/** +* ata_exec_internal - execute libata internal command +* @ap: Port to which the command is sent +* @dev: Device to which the command is sent +* @tf: Taskfile registers for the command and the result +* @dma_dir: Data tranfer direction of the command +* @buf: Data buffer of the command +* @buflen: Length of data buffer +* +* Executes libata internal command with timeout. @tf contains +* command on entry and result on return. Timeout and error +* conditions are reported via return value. No recovery action +* is taken after a command times out. It's caller's duty to +* clean up after timeout. +* +* LOCKING: +* None. Should be called with kernel context, might sleep. +*/ + +static unsigned +ata_exec_internal (struct ata_port *ap,struct ata_device *dev, +struct ata_taskfile *tf, +int dma_dir,void *buf,unsigned int buflen) +{ +u8 command = tf->command; +struct ata_queued_cmd *qc; +DECLARE_COMPLETION (wait); +unsigned long flags; +struct ata_exec_internal_arg arg; + +if (ata_busy_sleep (ap,ATA_TMOUT_INTERNAL_QUICK ,ATA_TMOUT_INTERNAL )) +return AC_ERR_OTHER ; + +spin_lock_irqsave (&ap->host_set->lock, flags); + + qc =ata_qc_new_init (ap, dev); +BUG_ON (qc ==NULL ); + + qc->tf = *tf; + qc->dma_dir = dma_dir; +if (dma_dir !=DMA_NONE ) { +ata_sg_init_one (qc, buf, buflen); + qc->nsect = buflen /ATA_SECT_SIZE ; +printk ("dma_dir=%d buflen=%u nsect=%d\n" , + dma_dir, buflen, qc->nsect); + } + + arg.waiting = &wait; + arg.tf = tf; + qc->complete_data = &arg; + qc->complete_fn = ata_qc_complete_internal; + +if (ata_qc_issue (qc)) + goto issue_fail; + +spin_unlock_irqrestore (&ap->host_set->lock, flags); + +if (!wait_for_completion_timeout (&wait,ATA_TMOUT_INTERNAL )) { +int timedout; + +spin_lock_irqsave (&ap->host_set->lock, flags); + +/* We're racing with irq here. If we lose, the +* following test prevents us from completing the qc +* again. If completion irq occurs after here but +* before the caller cleans up, it will result in a +* spurious interrupt. We can live with that. +*/ + timedout = arg.waiting !=NULL ; +if (timedout) +ata_qc_complete (qc,0 ); + +spin_unlock_irqrestore (&ap->host_set->lock, flags); -ata_qc_complete (qc, err_mask); +if (timedout) { + arg.err_mask =ac_err_mask (tf->command); +if (!arg.err_mask) +printk (KERN_WARNING +"ata%u: slow completion (cmd %x)\n" , + ap->id, command); +else +printk (KERN_WARNING +"ata%u: qc timeout (cmd %x)\n" , + ap->id, command); + } } -return rc; +return arg.err_mask; + + issue_fail: +ata_qc_free (qc); +spin_unlock_irqrestore (&ap->host_set->lock, flags); +return AC_ERR_OTHER ; }/**
@@ -1100,9 +1191,8 @@ static void ata_dev_identify(struct ata_u16 tmp;unsigned long xfer_modes;unsigned int using_edd; -DECLARE_COMPLETION (wait); -struct ata_queued_cmd *qc; -unsigned long flags; +struct ata_taskfile tf; +unsigned int err_mask;int rc;if (!ata_dev_present (dev)) {
@@ -1123,40 +1213,26 @@ static void ata_dev_identify(struct ata_ata_dev_select (ap, device,1 ,1 );/* select device 0/1 */ - qc =ata_qc_new_init (ap, dev); -BUG_ON (qc ==NULL ); - -ata_sg_init_one (qc, dev->id,sizeof (dev->id)); - qc->dma_dir =DMA_FROM_DEVICE ; - qc->tf.protocol =ATA_PROT_PIO ; - qc->nsect =1 ; - retry: +ata_tf_init (ap, &tf, device); +if (dev->class ==ATA_DEV_ATA ) { - qc->tf.command =ATA_CMD_ID_ATA ; + tf.command =ATA_CMD_ID_ATA ;DPRINTK ("do ATA identify\n" ); }else { - qc->tf.command =ATA_CMD_ID_ATAPI ; + tf.command =ATA_CMD_ID_ATAPI ;DPRINTK ("do ATAPI identify\n" ); } - qc->waiting = &wait; - qc->complete_fn = ata_qc_complete_noop; + tf.protocol =ATA_PROT_PIO ; -spin_lock_irqsave (&ap->host_set->lock, flags); - rc =ata_qc_issue (qc); -spin_unlock_irqrestore (&ap->host_set->lock, flags); + err_mask =ata_exec_internal (ap, dev, &tf,DMA_FROM_DEVICE , + dev->id,sizeof (dev->id)); -if (rc) - goto err_out; -else -ata_qc_wait_err (qc, &wait); - -spin_lock_irqsave (&ap->host_set->lock, flags); - ap->ops->tf_read (ap, &qc->tf); -spin_unlock_irqrestore (&ap->host_set->lock, flags); +if (err_mask) { +if (err_mask & ~AC_ERR_DEV ) + goto err_out; -if (qc->tf.command &ATA_ERR ) {/* * arg! EDD works for all test cases, but seems to return * the ATA signature for some ATAPI devices. Until the
@@ -1169,13 +1245,9 @@ retry:* to have this problem. */ if ((using_edd) && (dev->class ==ATA_DEV_ATA )) { -u8 err = qc->tf.feature; +u8 err = tf.feature;if (err &ATA_ABORTED ) { dev->class =ATA_DEV_ATAPI ; - qc->cursg =0 ; - qc->cursg_ofs =0 ; - qc->cursect =0 ; - qc->nsect =1 ; goto retry; } }
@@ -2288,34 +2360,23 @@ static int ata_choose_xfer_mode(const ststatic void ata_dev_set_xfermode (struct ata_port *ap,struct ata_device *dev) { -DECLARE_COMPLETION (wait); -struct ata_queued_cmd *qc; -int rc; -unsigned long flags; +struct ata_taskfile tf;/* set up set-features taskfile */ DPRINTK ("set features - xfer mode\n" ); - qc =ata_qc_new_init (ap, dev); -BUG_ON (qc ==NULL ); - - qc->tf.command =ATA_CMD_SET_FEATURES ; - qc->tf.feature =SETFEATURES_XFER ; - qc->tf.flags |=ATA_TFLAG_ISADDR |ATA_TFLAG_DEVICE ; - qc->tf.protocol =ATA_PROT_NODATA ; - qc->tf.nsect = dev->xfer_mode; - - qc->waiting = &wait; - qc->complete_fn = ata_qc_complete_noop; - -spin_lock_irqsave (&ap->host_set->lock, flags); - rc =ata_qc_issue (qc); -spin_unlock_irqrestore (&ap->host_set->lock, flags); +ata_tf_init (ap, &tf, dev->devno); + tf.command =ATA_CMD_SET_FEATURES ; + tf.feature =SETFEATURES_XFER ; + tf.flags |=ATA_TFLAG_ISADDR |ATA_TFLAG_DEVICE ; + tf.protocol =ATA_PROT_NODATA ; + tf.nsect = dev->xfer_mode; -if (rc) +if (ata_exec_internal (ap, dev, &tf,DMA_NONE ,NULL ,0 )) { +printk (KERN_ERR "ata%u: failed to set xfermode, disabled\n" , + ap->id);ata_port_disable (ap); -else -ata_qc_wait_err (qc, &wait); + }DPRINTK ("EXIT\n" ); }
@@ -2330,41 +2391,25 @@ static void ata_dev_set_xfermode(structstatic void ata_dev_reread_id (struct ata_port *ap,struct ata_device *dev) { -DECLARE_COMPLETION (wait); -struct ata_queued_cmd *qc; -unsigned long flags; -int rc; - - qc =ata_qc_new_init (ap, dev); -BUG_ON (qc ==NULL ); +struct ata_taskfile tf; -ata_sg_init_one (qc, dev->id,sizeof (dev->id)); - qc->dma_dir =DMA_FROM_DEVICE ; +ata_tf_init (ap, &tf, dev->devno);if (dev->class ==ATA_DEV_ATA ) { - qc->tf.command =ATA_CMD_ID_ATA ; + tf.command =ATA_CMD_ID_ATA ;DPRINTK ("do ATA identify\n" ); }else { - qc->tf.command =ATA_CMD_ID_ATAPI ; + tf.command =ATA_CMD_ID_ATAPI ;DPRINTK ("do ATAPI identify\n" ); } - qc->tf.flags |=ATA_TFLAG_DEVICE ; - qc->tf.protocol =ATA_PROT_PIO ; - qc->nsect =1 ; - - qc->waiting = &wait; - qc->complete_fn = ata_qc_complete_noop; + tf.flags |=ATA_TFLAG_DEVICE ; + tf.protocol =ATA_PROT_PIO ; -spin_lock_irqsave (&ap->host_set->lock, flags); - rc =ata_qc_issue (qc); -spin_unlock_irqrestore (&ap->host_set->lock, flags); - -if (rc) +if (ata_exec_internal (ap, dev, &tf,DMA_FROM_DEVICE , + dev->id,sizeof (dev->id))) goto err_out; -ata_qc_wait_err (qc, &wait); -swap_buf_le16 (dev->id,ATA_ID_WORDS );ata_dump_id (dev);
@@ -2373,6 +2418,7 @@ static void ata_dev_reread_id(struct atareturn ; err_out: +printk (KERN_ERR "ata%u: failed to reread ID, disabled\n" , ap->id);ata_port_disable (ap); }
@@ -2386,10 +2432,7 @@ err_out:static void ata_dev_init_params (struct ata_port *ap,struct ata_device *dev) { -DECLARE_COMPLETION (wait); -struct ata_queued_cmd *qc; -int rc; -unsigned long flags; +struct ata_taskfile tf;u16 sectors = dev->id[6 ];u16 heads = dev->id[3 ];
@@ -2400,26 +2443,18 @@ static void ata_dev_init_params(struct a/* set up init dev params taskfile */ DPRINTK ("init dev params \n" ); - qc =ata_qc_new_init (ap, dev); -BUG_ON (qc ==NULL ); - - qc->tf.command =ATA_CMD_INIT_DEV_PARAMS ; - qc->tf.flags |=ATA_TFLAG_ISADDR |ATA_TFLAG_DEVICE ; - qc->tf.protocol =ATA_PROT_NODATA ; - qc->tf.nsect = sectors; - qc->tf.device |= (heads -1 ) &0x0f ;/* max head = num. of heads - 1 */ - - qc->waiting = &wait; - qc->complete_fn = ata_qc_complete_noop; - -spin_lock_irqsave (&ap->host_set->lock, flags); - rc =ata_qc_issue (qc); -spin_unlock_irqrestore (&ap->host_set->lock, flags); +ata_tf_init (ap, &tf, dev->devno); + tf.command =ATA_CMD_INIT_DEV_PARAMS ; + tf.flags |=ATA_TFLAG_ISADDR |ATA_TFLAG_DEVICE ; + tf.protocol =ATA_PROT_NODATA ; + tf.nsect = sectors; + tf.device |= (heads -1 ) &0x0f ;/* max head = num. of heads - 1 */ -if (rc) +if (ata_exec_internal (ap, dev, &tf,DMA_NONE ,NULL ,0 )) { +printk (KERN_ERR "ata%u: failed to init parameters, disabled\n" , + ap->id);ata_port_disable (ap); -else -ata_qc_wait_err (qc, &wait); + }DPRINTK ("EXIT\n" ); }
@@ -3641,11 +3676,6 @@ struct ata_queued_cmd *ata_qc_new_init(sreturn qc; } -int ata_qc_complete_noop (struct ata_queued_cmd *qc,unsigned int err_mask) -{ -return 0 ; -} -static void __ata_qc_complete (struct ata_queued_cmd *qc) {struct ata_port *ap = qc->ap;
drivers/scsi/libata.h +0 -1
@@ -39,7 +39,6 @@ struct ata_scsi_args {/* libata-core.c */ extern int atapi_enabled; -extern int ata_qc_complete_noop (struct ata_queued_cmd *qc,unsigned int err_mask);extern struct ata_queued_cmd *ata_qc_new_init (struct ata_port *ap,struct ata_device *dev);extern void ata_rwcmd_protocol (struct ata_queued_cmd *qc);
include/linux/libata.h +3 -0
@@ -136,6 +136,8 @@ enum {ATA_TMOUT_BOOT_QUICK =7 *HZ ,/* hueristic */ ATA_TMOUT_DATAOUT =30 *HZ ,ATA_TMOUT_DATAOUT_QUICK =5 *HZ , +ATA_TMOUT_INTERNAL =30 *HZ , +ATA_TMOUT_INTERNAL_QUICK =5 *HZ ,/* ATA bus states */ BUS_UNKNOWN = 0,
@@ -284,6 +286,7 @@ struct ata_queued_cmd {struct scatterlist *__sg;ata_qc_cb_t complete_fn; +void *complete_data;struct completion *waiting;
Re: [PATCH 1/2] libata: implement ata_exec_internal()
Quoted 10 lines · Tejun Heo
Each user libata internal commands initializes a qc, issues it, waits for its completion and checks for errors. This patch factors internal command execution sequence into
ata_qc_exec_internal(). This change fixes the following bugs.* qc not freed on issue failure
*ap->qactiveclearing could race with the next internal command
* race between timeout handling and irq
* ignoring error condition not represented intf->status
Quoted 6 lines
Also, qc & hardware are not accessed anymore once it's completed, making internal commands more conformant with general semantics. This change also makes it easy to issue internal commands from multiple threads if that becomes necessary.
Signed-off-by Tejun Heo <[email protected]>
This is OK in concept, but there are problems in implementation...
General comment: would be nice split up as 'implement ata.exec.internal' and 'use ata.exec.internal' patches.
Quoted 15 lines
This is take #2 of
ata_exec_internal(). qc allocation and tf reading-back have also been moved intoata_exec_internal(). Now all qc handling is done while properly holdinghost_setlock removing above mentioned rare race conditon (the second item).Index:
work/drivers/scsi/libata-core.c
===================================================================drivers/scsi/libata-core.c +0
-2@@ -1047,28 +1047,119 @@ static unsigned int ata_pio_modes(constreturn modes; } -static int ata_qc_wait_err (struct ata_queued_cmd *qc, -struct completion *wait)
drivers/scsi/libata-core.c static unsigned int ata_pio_modes(const · line 1058
Quoted 10 lines
drivers/scsi/libata-core.c +8
-1+struct ata_exec_internal_arg { +unsigned int err_mask; +struct ata_taskfile *tf; +struct completion *waiting; +}; + +int ata_qc_complete_internal (struct ata_queued_cmd *qc,unsigned int err_mask) { -int rc =0 ; +struct ata_exec_internal_arg *arg = qc->complete_data;
This is what private_data is for. No need to add 'complete_data'.
Quoted 45 lines
drivers/scsi/libata-core.c +32
-12+struct completion *waiting = arg->waiting; -if (wait_for_completion_timeout (wait,30 *HZ ) <1 ) { -/* timeout handling */ -unsigned int err_mask =ac_err_mask (ata_chk_status (qc->ap)); - -if (!err_mask) { -printk (KERN_WARNING "ata%u: slow completion (cmd %x)\n" , - qc->ap->id, qc->tf.command); - }else { -printk (KERN_WARNING "ata%u: qc timeout (cmd %x)\n" , - qc->ap->id, qc->tf.command); - rc = -EIO ; - } +if (!(err_mask & ~AC_ERR_DEV )) + qc->ap->ops->tf_read (qc->ap, arg->tf); + arg->err_mask = err_mask; + arg->waiting =NULL ; +complete (waiting); + +return 0 ; +} + +/** +* ata_exec_internal - execute libata internal command +* @ap: Port to which the command is sent +* @dev: Device to which the command is sent +* @tf: Taskfile registers for the command and the result +* @dma_dir: Data tranfer direction of the command +* @buf: Data buffer of the command +* @buflen: Length of data buffer +* +* Executes libata internal command with timeout. @tf contains +* command on entry and result on return. Timeout and error +* conditions are reported via return value. No recovery action +* is taken after a command times out. It's caller's duty to +* clean up after timeout. +* +* LOCKING: +* None. Should be called with kernel context, might sleep. +*/ + +static unsigned +ata_exec_internal(struct ata_port *ap, struct ata_device *dev, + struct ata_taskfile *tf,
drivers/scsi/libata-core.c static unsigned int ata_pio_modes(const · line 1101
Quoted 10 lines
drivers/scsi/libata-core.c +10
-0+ int dma_dir, void *buf, unsigned int buflen) +{ + u8 command = tf->command; +struct ata_queued_cmd *qc; +DECLARE_COMPLETION (wait); +unsigned long flags; +struct ata_exec_internal_arg arg; + +if (ata_busy_sleep (ap,ATA_TMOUT_INTERNAL_QUICK ,ATA_TMOUT_INTERNAL )) +return AC_ERR_OTHER ;
NAK. This sort of thing really belongs in the execution engine for each driver. Advanced controllers have zero need for this.
Moreover, why are you adding this, anyway? This is a distinct regression from the nice "do everything inside ata_qc_issue" model that's been sprouting.
Quoted 28 lines
drivers/scsi/libata-core.c +28
-0+spin_lock_irqsave (&ap->host_set->lock, flags); + + qc =ata_qc_new_init (ap, dev); +BUG_ON (qc ==NULL ); + + qc->tf = *tf; + qc->dma_dir = dma_dir; +if (dma_dir !=DMA_NONE ) { +ata_sg_init_one (qc, buf, buflen); + qc->nsect = buflen /ATA_SECT_SIZE ; +printk ("dma_dir=%d buflen=%u nsect=%d\n" , + dma_dir, buflen, qc->nsect); + } + + arg.waiting = &wait; + arg.tf = tf; + qc->complete_data = &arg; + qc->complete_fn = ata_qc_complete_internal; + +if (ata_qc_issue (qc)) + goto issue_fail; + +spin_unlock_irqrestore (&ap->host_set->lock, flags); + +if (!wait_for_completion_timeout (&wait,ATA_TMOUT_INTERNAL )) { +int timedout; + +spin_lock_irqsave (&ap->host_set->lock, flags);
drivers/scsi/libata-core.c static unsigned int ata_pio_modes(const · line 1140
Quoted 10 lines
drivers/scsi/libata-core.c +10
-0+ +/* We're racing with irq here. If we lose, the +* following test prevents us from completing the qc +* again. If completion irq occurs after here but +* before the caller cleans up, it will result in a +* spurious interrupt. We can live with that. +*/ + timedout = arg.waiting !=NULL ; +if (timedout) +ata_qc_complete (qc,0 );
This is not very wise, to pass 'nothing is wrong' to ata_qc_complete() upon timeout.
Quoted 255 lines
drivers/scsi/libata-core.c +61
-102+spin_unlock_irqrestore (&ap->host_set->lock, flags); -ata_qc_complete (qc, err_mask); +if (timedout) { + arg.err_mask =ac_err_mask (tf->command); +if (!arg.err_mask) +printk (KERN_WARNING +"ata%u: slow completion (cmd %x)\n" , + ap->id, command); +else +printk (KERN_WARNING +"ata%u: qc timeout (cmd %x)\n" , + ap->id, command); + } } -return rc; +return arg.err_mask; + + issue_fail: +ata_qc_free (qc); +spin_unlock_irqrestore (&ap->host_set->lock, flags); +return AC_ERR_OTHER ; }/** @@ -1100,9 +1191,8 @@ static void ata_dev_identify(struct ata_u16 tmp;unsigned long xfer_modes;unsigned int using_edd; -DECLARE_COMPLETION (wait); -struct ata_queued_cmd *qc; -unsigned long flags; +struct ata_taskfile tf; +unsigned int err_mask;int rc;if (!ata_dev_present (dev)) {@@ -1123,40 +1213,26 @@ static void ata_dev_identify(struct ata_ata_dev_select (ap, device,1 ,1 );/* select device 0/1 */ - qc =ata_qc_new_init (ap, dev); -BUG_ON (qc ==NULL ); - -ata_sg_init_one (qc, dev->id,sizeof (dev->id)); - qc->dma_dir =DMA_FROM_DEVICE ; - qc->tf.protocol =ATA_PROT_PIO ; - qc->nsect =1 ; - retry: +ata_tf_init (ap, &tf, device);if (dev->class ==ATA_DEV_ATA ) { - qc->tf.command =ATA_CMD_ID_ATA ; + tf.command =ATA_CMD_ID_ATA ;DPRINTK ("do ATA identify\n" ); }else { - qc->tf.command =ATA_CMD_ID_ATAPI ; + tf.command =ATA_CMD_ID_ATAPI ;DPRINTK ("do ATAPI identify\n" ); } - qc->waiting = &wait; - qc->complete_fn = ata_qc_complete_noop; + tf.protocol =ATA_PROT_PIO ; -spin_lock_irqsave (&ap->host_set->lock, flags); - rc =ata_qc_issue (qc); -spin_unlock_irqrestore (&ap->host_set->lock, flags); + err_mask =ata_exec_internal (ap, dev, &tf,DMA_FROM_DEVICE , + dev->id,sizeof (dev->id)); -if (rc) - goto err_out; -else -ata_qc_wait_err (qc, &wait); - -spin_lock_irqsave (&ap->host_set->lock, flags); - ap->ops->tf_read (ap, &qc->tf); -spin_unlock_irqrestore (&ap->host_set->lock, flags); +if (err_mask) { +if (err_mask & ~AC_ERR_DEV ) + goto err_out; -if (qc->tf.command &ATA_ERR ) {/* * arg! EDD works for all test cases, but seems to return * the ATA signature for some ATAPI devices. Until the @@ -1169,13 +1245,9 @@ retry:* to have this problem. */ if ((using_edd) && (dev->class ==ATA_DEV_ATA )) { -u8 err = qc->tf.feature; +u8 err = tf.feature;if (err &ATA_ABORTED ) { dev->class =ATA_DEV_ATAPI ; - qc->cursg =0 ; - qc->cursg_ofs =0 ; - qc->cursect =0 ; - qc->nsect =1 ; goto retry; } }@@ -2288,34 +2360,23 @@ static int ata_choose_xfer_mode(const ststatic void ata_dev_set_xfermode (struct ata_port *ap,struct ata_device *dev) { -DECLARE_COMPLETION (wait); -struct ata_queued_cmd *qc; -int rc; -unsigned long flags; +struct ata_taskfile tf;/* set up set-features taskfile */ DPRINTK ("set features - xfer mode\n" ); - qc =ata_qc_new_init (ap, dev); -BUG_ON (qc ==NULL ); - - qc->tf.command =ATA_CMD_SET_FEATURES ; - qc->tf.feature =SETFEATURES_XFER ; - qc->tf.flags |=ATA_TFLAG_ISADDR |ATA_TFLAG_DEVICE ; - qc->tf.protocol =ATA_PROT_NODATA ; - qc->tf.nsect = dev->xfer_mode; - - qc->waiting = &wait; - qc->complete_fn = ata_qc_complete_noop; - -spin_lock_irqsave (&ap->host_set->lock, flags); - rc =ata_qc_issue (qc); -spin_unlock_irqrestore (&ap->host_set->lock, flags); +ata_tf_init (ap, &tf, dev->devno); + tf.command =ATA_CMD_SET_FEATURES ; + tf.feature =SETFEATURES_XFER ; + tf.flags |=ATA_TFLAG_ISADDR |ATA_TFLAG_DEVICE ; + tf.protocol =ATA_PROT_NODATA ; + tf.nsect = dev->xfer_mode; -if (rc) +if (ata_exec_internal (ap, dev, &tf,DMA_NONE ,NULL ,0 )) { +printk (KERN_ERR "ata%u: failed to set xfermode, disabled\n" , + ap->id);ata_port_disable (ap); -else -ata_qc_wait_err (qc, &wait); + }DPRINTK ("EXIT\n" ); }@@ -2330,41 +2391,25 @@ static void ata_dev_set_xfermode(structstatic void ata_dev_reread_id (struct ata_port *ap,struct ata_device *dev) { -DECLARE_COMPLETION (wait); -struct ata_queued_cmd *qc; -unsigned long flags; -int rc; - - qc =ata_qc_new_init (ap, dev); -BUG_ON (qc ==NULL ); +struct ata_taskfile tf; -ata_sg_init_one (qc, dev->id,sizeof (dev->id)); - qc->dma_dir =DMA_FROM_DEVICE ; +ata_tf_init (ap, &tf, dev->devno);if (dev->class ==ATA_DEV_ATA ) { - qc->tf.command =ATA_CMD_ID_ATA ; + tf.command =ATA_CMD_ID_ATA ;DPRINTK ("do ATA identify\n" ); }else { - qc->tf.command =ATA_CMD_ID_ATAPI ; + tf.command =ATA_CMD_ID_ATAPI ;DPRINTK ("do ATAPI identify\n" ); } - qc->tf.flags |=ATA_TFLAG_DEVICE ; - qc->tf.protocol =ATA_PROT_PIO ; - qc->nsect =1 ; - - qc->waiting = &wait; - qc->complete_fn = ata_qc_complete_noop; + tf.flags |=ATA_TFLAG_DEVICE ; + tf.protocol =ATA_PROT_PIO ; -spin_lock_irqsave (&ap->host_set->lock, flags); - rc =ata_qc_issue (qc); -spin_unlock_irqrestore (&ap->host_set->lock, flags); - -if (rc) +if (ata_exec_internal (ap, dev, &tf,DMA_FROM_DEVICE , + dev->id,sizeof (dev->id))) goto err_out; -ata_qc_wait_err (qc, &wait); -swap_buf_le16 (dev->id,ATA_ID_WORDS );ata_dump_id (dev);@@ -2373,6 +2418,7 @@ static void ata_dev_reread_id(struct atareturn ; err_out: +printk (KERN_ERR "ata%u: failed to reread ID, disabled\n" , ap->id);ata_port_disable (ap); }@@ -2386,10 +2432,7 @@ err_out:static void ata_dev_init_params (struct ata_port *ap,struct ata_device *dev) { -DECLARE_COMPLETION (wait); -struct ata_queued_cmd *qc; -int rc; -unsigned long flags; +struct ata_taskfile tf;u16 sectors = dev->id[6 ];u16 heads = dev->id[3 ];@@ -2400,26 +2443,18 @@ static void ata_dev_init_params(struct a/* set up init dev params taskfile */ DPRINTK ("init dev params \n" ); - qc =ata_qc_new_init (ap, dev); -BUG_ON (qc ==NULL ); - - qc->tf.command =ATA_CMD_INIT_DEV_PARAMS ; - qc->tf.flags |=ATA_TFLAG_ISADDR |ATA_TFLAG_DEVICE ; - qc->tf.protocol =ATA_PROT_NODATA ; - qc->tf.nsect = sectors; - qc->tf.device |= (heads -1 ) &0x0f ;/* max head = num. of heads - 1 */ - - qc->waiting = &wait; - qc->complete_fn = ata_qc_complete_noop; - -spin_lock_irqsave (&ap->host_set->lock, flags); - rc =ata_qc_issue (qc); -spin_unlock_irqrestore (&ap->host_set->lock, flags); +ata_tf_init (ap, &tf, dev->devno); + tf.command =ATA_CMD_INIT_DEV_PARAMS ; + tf.flags |=ATA_TFLAG_ISADDR |ATA_TFLAG_DEVICE ; + tf.protocol =ATA_PROT_NODATA ; + tf.nsect = sectors; + tf.device |= (heads -1 ) &0x0f ;/* max head = num. of heads - 1 */ -if (rc) +if (ata_exec_internal (ap, dev, &tf,DMA_NONE ,NULL ,0 )) { +printk (KERN_ERR "ata%u: failed to init parameters, disabled\n" , + ap->id);ata_port_disable (ap); -else -ata_qc_wait_err (qc, &wait); + }DPRINTK ("EXIT\n" );
Re: [PATCH 1/2] libata: implement ata_exec_internal()
Hello, Jeff.
Quoted 17 lines · Jeff Garzik
Quoted 15 lines · Tejun Heo
Each user libata internal commands initializes a qc, issues it, waits for its completion and checks for errors. This patch factors internal command execution sequence into
ata_qc_exec_internal(). This change fixes the following bugs.* qc not freed on issue failure
*ap->qactiveclearing could race with the next internal command
* race between timeout handling and irq
* ignoring error condition not represented intf->statusAlso, qc & hardware are not accessed anymore once it's completed, making internal commands more conformant with general semantics. This change also makes it easy to issue internal commands from multiple threads if that becomes necessary.
Quoted 7 lines
Quoted 1 line
Signed-off-by Tejun Heo <[email protected]>This is OK in concept, but there are problems in implementation...
General comment: would be nice split up as 'implement ata.exec.internal' and 'use ata.exec.internal' patches.
Sure.
Quoted 16 lines
Quoted 16 lines
This is take #2 of
ata_exec_internal(). qc allocation and tf reading-back have also been moved intoata_exec_internal(). Now all qc handling is done while properly holdinghost_setlock removing above mentioned rare race conditon (the second item).Index:
work/drivers/scsi/libata-core.c
===================================================================
---work.orig/drivers/scsi/libata-core.c2005-11-29
22:04:15.000000000 +0900
+++work/drivers/scsi/libata-core.c2005-11-29 22:06:20.000000000
+0900drivers/scsi/libata-core.c +0
-0@@ -1047,28 +1047,119 @@ static unsigned int ata_pio_modes(constreturn modes; }
Quoted 17 lines
drivers/scsi/libata-core.c static unsigned int ata_pio_modes(const · line 1058
Quoted 13 lines
drivers/scsi/libata-core.c +8
-3-static int ata_qc_wait_err (struct ata_queued_cmd *qc, -struct completion *wait) +struct ata_exec_internal_arg { +unsigned int err_mask; +struct ata_taskfile *tf; +struct completion *waiting; +}; + +int ata_qc_complete_internal (struct ata_queued_cmd *qc,unsigned int err_mask) { -int rc =0 ; +struct ata_exec_internal_arg *arg = qc->complete_data;This is what
private_datais for. No need to add 'complete_data'.
Hmmm... As there currently isn't any in-kernel user of qc->private_data, its ownership status isn't very clear. However, ap->private_data is used by most low-level drivers to store its private data, so I assumed that qc->private_data belongs to low-level driver too.
Actually, the m15w workaround I'm maintaining uses qc->private_data to store context for qc iteration. This can be substitued easily but I think it would be nice / more consistent to reserve private_data fields for low-level drivers.
Quoted 61 lines
Quoted 47 lines
drivers/scsi/libata-core.c +33
-12+struct completion *waiting = arg->waiting; -if (wait_for_completion_timeout (wait,30 *HZ ) <1 ) { -/* timeout handling */ -unsigned int err_mask =ac_err_mask (ata_chk_status (qc->ap)); - -if (!err_mask) { -printk (KERN_WARNING "ata%u: slow completion (cmd %x)\n" , - qc->ap->id, qc->tf.command); - }else { -printk (KERN_WARNING "ata%u: qc timeout (cmd %x)\n" , - qc->ap->id, qc->tf.command); - rc = -EIO ; - } +if (!(err_mask & ~AC_ERR_DEV )) + qc->ap->ops->tf_read (qc->ap, arg->tf); + arg->err_mask = err_mask; + arg->waiting =NULL ; +complete (waiting); + +return 0 ; +} + +/** +* ata_exec_internal - execute libata internal command +* @ap: Port to which the command is sent +* @dev: Device to which the command is sent +* @tf: Taskfile registers for the command and the result +* @dma_dir: Data tranfer direction of the command +* @buf: Data buffer of the command +* @buflen: Length of data buffer +* +* Executes libata internal command with timeout. @tf contains +* command on entry and result on return. Timeout and error +* conditions are reported via return value. No recovery action +* is taken after a command times out. It's caller's duty to +* clean up after timeout. +* +* LOCKING: +* None. Should be called with kernel context, might sleep. +*/ + +static unsigned +ata_exec_internal (struct ata_port *ap,struct ata_device *dev, +struct ata_taskfile *tf, +int dma_dir,void *buf,unsigned int buflen)drivers/scsi/libata-core.c static unsigned int ata_pio_modes(const · line 1101
Quoted 10 lines
drivers/scsi/libata-core.c +9
-0+{ +u8 command = tf->command; +struct ata_queued_cmd *qc; +DECLARE_COMPLETION (wait); +unsigned long flags; +struct ata_exec_internal_arg arg; + +if (ata_busy_sleep (ap,ATA_TMOUT_INTERNAL_QUICK ,ATA_TMOUT_INTERNAL )) +return AC_ERR_OTHER ;NAK. This sort of thing really belongs in the execution engine for each driver. Advanced controllers have zero need for this.
Quoted 2 lines
Moreover, why are you adding this, anyway? This is a distinct regression from the nice "do everything inside
ata_qc_issue" model that's been sprouting.
I was thinking that as these internal commands are used for initialization and possibly eh in the future, it would be better to give drives longer time to get ready for commands. So, the ata_busy_sleep with ATA_TMOUT_INTERNAL_QUICK, ATA_TMOUT_INTERNAL timeouts. Too paranoid / unnecessary?
Quoted 34 lines
Quoted 34 lines
drivers/scsi/libata-core.c +33
-0+spin_lock_irqsave (&ap->host_set->lock, flags); + + qc =ata_qc_new_init (ap, dev); +BUG_ON (qc ==NULL ); + + qc->tf = *tf; + qc->dma_dir = dma_dir; +if (dma_dir !=DMA_NONE ) { +ata_sg_init_one (qc, buf, buflen); + qc->nsect = buflen /ATA_SECT_SIZE ; +printk ("dma_dir=%d buflen=%u nsect=%d\n" , + dma_dir, buflen, qc->nsect); + } + + arg.waiting = &wait; + arg.tf = tf; + qc->complete_data = &arg; + qc->complete_fn = ata_qc_complete_internal; + +if (ata_qc_issue (qc)) + goto issue_fail; + +spin_unlock_irqrestore (&ap->host_set->lock, flags); + +if (!wait_for_completion_timeout (&wait,ATA_TMOUT_INTERNAL )) { +int timedout; + +spin_lock_irqsave (&ap->host_set->lock, flags); + +/* We're racing with irq here. If we lose, the +* following test prevents us from completing the qc +* again. If completion irq occurs after here but +* before the caller cleans up, it will result in a
Quoted 9 lines
drivers/scsi/libata-core.c static unsigned int ata_pio_modes(const · line 1140
Quoted 5 lines
drivers/scsi/libata-core.c +5
-0+* spurious interrupt. We can live with that. +*/ + timedout = arg.waiting !=NULL ; +if (timedout) +ata_qc_complete (qc,0 );This is not very wise, to pass 'nothing is wrong' to
ata_qc_complete()upon timeout.
The problem is that we may or may not complete the command successfully due to the following slow completion handling. So, completing with AC_ERR_OTHER isn't very correct either. This matters because the tf registers are read from the completion callback and the completion callback does so only on device errors. The solutions are....
1. leaving as it is with more comments
2. completing with AC_ERR_OTHER but always reading back tf regs in the completion callback
I prefer #1, but I'm okay with #2, too.
On a side note, I'm sort of doubtful about slow completion handling in ata_exec_internal() and also in eng_timeout(). For example, if some kind of electrical / cabling glitch occurs during command execution and results in device internal reset, the device's reset status can easily be interpreted as successful completion by slow completion handling.
Actually, in the current libata, this can be easily triggered by just unplugging and replugging the cable on a busy drive as we don't handle those events.
IMHO, timed out commands should always be finished with AC_ERR_OTHER. I don't really see a lot of benefits in successfully completing a command after 30sec timeout.
Thanks for reviewing this.
Signature · 1 line
tejun
Re: [PATCH 1/2] libata: implement ata_exec_internal()
Quoted 37 lines · Tejun Heo
Quoted 26 lines · Jeff Garzik
Quoted 24 lines · Tejun Heo
This is take #2 of
ata_exec_internal(). qc allocation and tf reading-back have also been moved intoata_exec_internal(). Now all qc handling is done while properly holdinghost_setlock removing above mentioned rare race conditon (the second item).Index:
work/drivers/scsi/libata-core.c
===================================================================
---work.orig/drivers/scsi/libata-core.c2005-11-29
22:04:15.000000000 +0900
+++work/drivers/scsi/libata-core.c2005-11-29 22:06:20.000000000
+0900drivers/scsi/libata-core.c +7
-2@@ -1047,28 +1047,119 @@ static unsigned int ata_pio_modes(constreturn modes; } -static int ata_qc_wait_err (struct ata_queued_cmd *qc, -struct completion *wait) +struct ata_exec_internal_arg { +unsigned int err_mask; +struct ata_taskfile *tf; +struct completion *waiting; +}; + +int ata_qc_complete_internal(struct ata_queued_cmd *qc, unsigned intQuoted 9 lines
drivers/scsi/libata-core.c static unsigned int ata_pio_modes(const · line 1058
Quoted 4 lines
drivers/scsi/libata-core.c +1
-1err_mask) { -int rc =0 ; +struct ata_exec_internal_arg *arg = qc->complete_data;This is what
private_datais for. No need to add 'complete_data'.
Quoted 3 lines
Hmmm... As there currently isn't any in-kernel user of
qc->private_data, its ownership status isn't very clear. However,ap->private_datais used by most low-level drivers to store its private data, so I assumed thatqc->private_databelongs to low-level driver too.Actually, the m15w workaround I'm maintaining uses
qc->private_datato store context for qc iteration. This can be substitued easily but I think it would be nice / more consistent to reserveprivate_datafields for low-level drivers.
qc->private_data is for the creator/owner of the ata_queued_cmd, not for low-level drivers.
Quoted 61 lines
Quoted 61 lines
Quoted 46 lines
drivers/scsi/libata-core.c +33
-12+struct completion *waiting = arg->waiting; -if (wait_for_completion_timeout (wait,30 *HZ ) <1 ) { -/* timeout handling */ -unsigned int err_mask =ac_err_mask (ata_chk_status (qc->ap)); - -if (!err_mask) { -printk (KERN_WARNING "ata%u: slow completion (cmd %x)\n" , - qc->ap->id, qc->tf.command); - }else { -printk (KERN_WARNING "ata%u: qc timeout (cmd %x)\n" , - qc->ap->id, qc->tf.command); - rc = -EIO ; - } +if (!(err_mask & ~AC_ERR_DEV )) + qc->ap->ops->tf_read (qc->ap, arg->tf); + arg->err_mask = err_mask; + arg->waiting =NULL ; +complete (waiting); + +return 0 ; +} + +/** +* ata_exec_internal - execute libata internal command +* @ap: Port to which the command is sent +* @dev: Device to which the command is sent +* @tf: Taskfile registers for the command and the result +* @dma_dir: Data tranfer direction of the command +* @buf: Data buffer of the command +* @buflen: Length of data buffer +* +* Executes libata internal command with timeout. @tf contains +* command on entry and result on return. Timeout and error +* conditions are reported via return value. No recovery action +* is taken after a command times out. It's caller's duty to +* clean up after timeout. +* +* LOCKING: +* None. Should be called with kernel context, might sleep. +*/ + +static unsigned +ata_exec_internal (struct ata_port *ap,struct ata_device *dev, +struct ata_taskfile *tf, +int dma_dir,void *buf,unsigned int buflen)drivers/scsi/libata-core.c static unsigned int ata_pio_modes(const · line 1101
Quoted 10 lines
drivers/scsi/libata-core.c +9
-0+{ +u8 command = tf->command; +struct ata_queued_cmd *qc; +DECLARE_COMPLETION (wait); +unsigned long flags; +struct ata_exec_internal_arg arg; + +if (ata_busy_sleep (ap,ATA_TMOUT_INTERNAL_QUICK ,ATA_TMOUT_INTERNAL )) +return AC_ERR_OTHER ;NAK. This sort of thing really belongs in the execution engine for each driver. Advanced controllers have zero need for this.
Quoted 4 lines
Quoted 2 lines
Moreover, why are you adding this, anyway? This is a distinct regression from the nice "do everything inside
ata_qc_issue" model that's been sprouting.I was thinking that as these internal commands are used for initialization and possibly eh in the future, it would be better to give drives longer time to get ready for commands. So, the
ata_busy_sleepwithATA_TMOUT_INTERNAL_QUICK,ATA_TMOUT_INTERNALtimeouts. Too paranoid / unnecessary?
Yes, and it breaks the "use ata_queued_cmd for everything" model. Polling for BSY, etc. should happen in the time between the call to ata_qc_issue() and the call to ata_qc_complete().
There are some violations of this model in the SRST and IDENTIFY DEVICE code, but those need to be eliminated, not expanded :)
Quoted 44 lines
Quoted 34 lines
Quoted 34 lines
drivers/scsi/libata-core.c +34
-0+spin_lock_irqsave (&ap->host_set->lock, flags); + + qc =ata_qc_new_init (ap, dev); +BUG_ON (qc ==NULL ); + + qc->tf = *tf; + qc->dma_dir = dma_dir; +if (dma_dir !=DMA_NONE ) { +ata_sg_init_one (qc, buf, buflen); + qc->nsect = buflen /ATA_SECT_SIZE ; +printk ("dma_dir=%d buflen=%u nsect=%d\n" , + dma_dir, buflen, qc->nsect); + } + + arg.waiting = &wait; + arg.tf = tf; + qc->complete_data = &arg; + qc->complete_fn = ata_qc_complete_internal; + +if (ata_qc_issue (qc)) + goto issue_fail; + +spin_unlock_irqrestore (&ap->host_set->lock, flags); + +if (!wait_for_completion_timeout (&wait,ATA_TMOUT_INTERNAL )) { +int timedout; + +spin_lock_irqsave (&ap->host_set->lock, flags); + +/* We're racing with irq here. If we lose, the +* following test prevents us from completing the qc +* again. If completion irq occurs after here but +* before the caller cleans up, it will result in a +* spurious interrupt. We can live with that. Quoted 9 lines
drivers/scsi/libata-core.c static unsigned int ata_pio_modes(const · line 1140
Quoted 4 lines
drivers/scsi/libata-core.c +4
-0+*/ + timedout = arg.waiting !=NULL ; +if (timedout) +ata_qc_complete (qc,0 );This is not very wise, to pass 'nothing is wrong' to
ata_qc_complete()upon timeout.
Quoted 6 lines
The problem is that we may or may not complete the command successfully due to the following slow completion handling. So, completing with
AC_ERR_OTHERisn't very correct either. This matters because the tf registers are read from the completion callback and the completion callback does so only on device errors. The solutions are....1. leaving as it is with more comments
2. completing withAC_ERR_OTHERbut always reading back tf regs in the completion callbackI prefer #1, but I'm okay with #2, too.
If it times out, call it an error. Easy enough.
Quoted 3 lines
On a side note, I'm sort of doubtful about slow completion handling in
ata_exec_internal()and also ineng_timeout(). For example, if some kind of electrical / cabling glitch occurs during command execution and results in device internal reset, the device's reset status can easily be interpreted as successful completion by slow completion handling.Actually, in the current libata, this can be easily triggered by just unplugging and replugging the cable on a busy drive as we don't handle those events.
You're welcome to improve the libata error handling code ;-)
Jeff
PATCH 1/3 libata: implement ata_exec_internal()
This patch implements ata_exec_internal() function which performs libata internal command execution. Previously, this was done by each user by manually initializing a qc, issueing it, waiting for its completion and handling errors. In addition to obvious code factoring, using ata_exec_internal() fixes the following bugs.
* qc not freed on issue failure
* ap->qactive clearing could race with the next internal command
* race between timeout handling and irq
* ignoring error condition not represented in tf->status
Also, qc & hardware are not accessed anymore once it's completed, making internal commands more conformant with general semantics. ata_exec_internal() also makes it easy to issue internal commands from multiple threads if that becomes necessary.
This patch only implements ata_exec_internal(). A following patch will convert all users.
--
Jeff, changes from the previous posting are...
* no ata_busy_sleep
* no ->complete_data
* timeout is always an AC_ERR_OTHER error
* patches splitted
Thanks.
drivers/scsi/libata-core.c +100 -0
@@ -1047,6 +1047,106 @@ static unsigned int ata_pio_modes(constreturn modes; } +struct ata_exec_internal_arg { +unsigned int err_mask; +struct ata_taskfile *tf; +struct completion *waiting; +}; + +int ata_qc_complete_internal (struct ata_queued_cmd *qc,unsigned int err_mask) +{ +struct ata_exec_internal_arg *arg = qc->private_data; +struct completion *waiting = arg->waiting; + +if (!(err_mask & ~AC_ERR_DEV )) + qc->ap->ops->tf_read (qc->ap, arg->tf); + arg->err_mask = err_mask; + arg->waiting =NULL ; +complete (waiting); + +return 0 ; +} + +/** +* ata_exec_internal - execute libata internal command +* @ap: Port to which the command is sent +* @dev: Device to which the command is sent +* @tf: Taskfile registers for the command and the result +* @dma_dir: Data tranfer direction of the command +* @buf: Data buffer of the command +* @buflen: Length of data buffer +* +* Executes libata internal command with timeout. @tf contains +* command on entry and result on return. Timeout and error +* conditions are reported via return value. No recovery action +* is taken after a command times out. It's caller's duty to +* clean up after timeout. +* +* LOCKING: +* None. Should be called with kernel context, might sleep. +*/ + +static unsigned +ata_exec_internal (struct ata_port *ap,struct ata_device *dev, +struct ata_taskfile *tf, +int dma_dir,void *buf,unsigned int buflen) +{ +u8 command = tf->command; +struct ata_queued_cmd *qc; +DECLARE_COMPLETION (wait); +unsigned long flags; +struct ata_exec_internal_arg arg; + +spin_lock_irqsave (&ap->host_set->lock, flags); + + qc =ata_qc_new_init (ap, dev); +BUG_ON (qc ==NULL ); + + qc->tf = *tf; + qc->dma_dir = dma_dir; +if (dma_dir !=DMA_NONE ) { +ata_sg_init_one (qc, buf, buflen); + qc->nsect = buflen /ATA_SECT_SIZE ; +printk ("dma_dir=%d buflen=%u nsect=%d\n" , + dma_dir, buflen, qc->nsect); + } + + arg.waiting = &wait; + arg.tf = tf; + qc->private_data = &arg; + qc->complete_fn = ata_qc_complete_internal; + +if (ata_qc_issue (qc)) + goto issue_fail; + +spin_unlock_irqrestore (&ap->host_set->lock, flags); + +if (!wait_for_completion_timeout (&wait,ATA_TMOUT_INTERNAL )) { +spin_lock_irqsave (&ap->host_set->lock, flags); + +/* We're racing with irq here. If we lose, the +* following test prevents us from completing the qc +* again. If completion irq occurs after here but +* before the caller cleans up, it will result in a +* spurious interrupt. We can live with that. +*/ +if (arg.waiting) { +ata_qc_complete (qc,AC_ERR_OTHER ); +printk (KERN_WARNING "ata%u: qc timeout (cmd 0x%x)\n" , + ap->id, command); + } + +spin_unlock_irqrestore (&ap->host_set->lock, flags); + } + +return arg.err_mask; + + issue_fail: +ata_qc_free (qc); +spin_unlock_irqrestore (&ap->host_set->lock, flags); +return AC_ERR_OTHER ; +} +static int ata_qc_wait_err (struct ata_queued_cmd *qc,struct completion *wait) {
include/linux/libata.h +2 -0
@@ -136,6 +136,8 @@ enum {ATA_TMOUT_BOOT_QUICK =7 *HZ ,/* hueristic */ ATA_TMOUT_DATAOUT =30 *HZ ,ATA_TMOUT_DATAOUT_QUICK =5 *HZ , +ATA_TMOUT_INTERNAL =30 *HZ , +ATA_TMOUT_INTERNAL_QUICK =5 *HZ ,/* ATA bus states */ BUS_UNKNOWN = 0,
PATCH 2/4 libata: use ata_exec_internal()
In reply to Jeff GarzikThis patch converts all users of libata internal commands to use ata_exec_internal().
--
Jeff, I made a mistake while posting the previous patch. The subject line should say [PATCH 1/4] not [PATCH 1/3]. Sorry.
drivers/scsi/libata-core.c +44 -100
@@ -1200,9 +1200,8 @@ static void ata_dev_identify(struct ata_u16 tmp;unsigned long xfer_modes;unsigned int using_edd; -DECLARE_COMPLETION (wait); -struct ata_queued_cmd *qc; -unsigned long flags; +struct ata_taskfile tf; +unsigned int err_mask;int rc;if (!ata_dev_present (dev)) {
@@ -1223,40 +1222,26 @@ static void ata_dev_identify(struct ata_ata_dev_select (ap, device,1 ,1 );/* select device 0/1 */ - qc =ata_qc_new_init (ap, dev); -BUG_ON (qc ==NULL ); - -ata_sg_init_one (qc, dev->id,sizeof (dev->id)); - qc->dma_dir =DMA_FROM_DEVICE ; - qc->tf.protocol =ATA_PROT_PIO ; - qc->nsect =1 ; - retry: +ata_tf_init (ap, &tf, device); +if (dev->class ==ATA_DEV_ATA ) { - qc->tf.command =ATA_CMD_ID_ATA ; + tf.command =ATA_CMD_ID_ATA ;DPRINTK ("do ATA identify\n" ); }else { - qc->tf.command =ATA_CMD_ID_ATAPI ; + tf.command =ATA_CMD_ID_ATAPI ;DPRINTK ("do ATAPI identify\n" ); } - qc->waiting = &wait; - qc->complete_fn = ata_qc_complete_noop; - -spin_lock_irqsave (&ap->host_set->lock, flags); - rc =ata_qc_issue (qc); -spin_unlock_irqrestore (&ap->host_set->lock, flags); + tf.protocol =ATA_PROT_PIO ; -if (rc) - goto err_out; -else -ata_qc_wait_err (qc, &wait); + err_mask =ata_exec_internal (ap, dev, &tf,DMA_FROM_DEVICE , + dev->id,sizeof (dev->id)); -spin_lock_irqsave (&ap->host_set->lock, flags); - ap->ops->tf_read (ap, &qc->tf); -spin_unlock_irqrestore (&ap->host_set->lock, flags); +if (err_mask) { +if (err_mask & ~AC_ERR_DEV ) + goto err_out; -if (qc->tf.command &ATA_ERR ) {/* * arg! EDD works for all test cases, but seems to return * the ATA signature for some ATAPI devices. Until the
@@ -1269,13 +1254,9 @@ retry:* to have this problem. */ if ((using_edd) && (dev->class ==ATA_DEV_ATA )) { -u8 err = qc->tf.feature; +u8 err = tf.feature;if (err &ATA_ABORTED ) { dev->class =ATA_DEV_ATAPI ; - qc->cursg =0 ; - qc->cursg_ofs =0 ; - qc->cursect =0 ; - qc->nsect =1 ; goto retry; } }
@@ -2388,34 +2369,23 @@ static int ata_choose_xfer_mode(const ststatic void ata_dev_set_xfermode (struct ata_port *ap,struct ata_device *dev) { -DECLARE_COMPLETION (wait); -struct ata_queued_cmd *qc; -int rc; -unsigned long flags; +struct ata_taskfile tf;/* set up set-features taskfile */ DPRINTK ("set features - xfer mode\n" ); - qc =ata_qc_new_init (ap, dev); -BUG_ON (qc ==NULL ); - - qc->tf.command =ATA_CMD_SET_FEATURES ; - qc->tf.feature =SETFEATURES_XFER ; - qc->tf.flags |=ATA_TFLAG_ISADDR |ATA_TFLAG_DEVICE ; - qc->tf.protocol =ATA_PROT_NODATA ; - qc->tf.nsect = dev->xfer_mode; - - qc->waiting = &wait; - qc->complete_fn = ata_qc_complete_noop; - -spin_lock_irqsave (&ap->host_set->lock, flags); - rc =ata_qc_issue (qc); -spin_unlock_irqrestore (&ap->host_set->lock, flags); +ata_tf_init (ap, &tf, dev->devno); + tf.command =ATA_CMD_SET_FEATURES ; + tf.feature =SETFEATURES_XFER ; + tf.flags |=ATA_TFLAG_ISADDR |ATA_TFLAG_DEVICE ; + tf.protocol =ATA_PROT_NODATA ; + tf.nsect = dev->xfer_mode; -if (rc) +if (ata_exec_internal (ap, dev, &tf,DMA_NONE ,NULL ,0 )) { +printk (KERN_ERR "ata%u: failed to set xfermode, disabled\n" , + ap->id);ata_port_disable (ap); -else -ata_qc_wait_err (qc, &wait); + }DPRINTK ("EXIT\n" ); }
@@ -2430,41 +2400,25 @@ static void ata_dev_set_xfermode(structstatic void ata_dev_reread_id (struct ata_port *ap,struct ata_device *dev) { -DECLARE_COMPLETION (wait); -struct ata_queued_cmd *qc; -unsigned long flags; -int rc; - - qc =ata_qc_new_init (ap, dev); -BUG_ON (qc ==NULL ); +struct ata_taskfile tf; -ata_sg_init_one (qc, dev->id,sizeof (dev->id)); - qc->dma_dir =DMA_FROM_DEVICE ; +ata_tf_init (ap, &tf, dev->devno);if (dev->class ==ATA_DEV_ATA ) { - qc->tf.command =ATA_CMD_ID_ATA ; + tf.command =ATA_CMD_ID_ATA ;DPRINTK ("do ATA identify\n" ); }else { - qc->tf.command =ATA_CMD_ID_ATAPI ; + tf.command =ATA_CMD_ID_ATAPI ;DPRINTK ("do ATAPI identify\n" ); } - qc->tf.flags |=ATA_TFLAG_DEVICE ; - qc->tf.protocol =ATA_PROT_PIO ; - qc->nsect =1 ; + tf.flags |=ATA_TFLAG_DEVICE ; + tf.protocol =ATA_PROT_PIO ; - qc->waiting = &wait; - qc->complete_fn = ata_qc_complete_noop; - -spin_lock_irqsave (&ap->host_set->lock, flags); - rc =ata_qc_issue (qc); -spin_unlock_irqrestore (&ap->host_set->lock, flags); - -if (rc) +if (ata_exec_internal (ap, dev, &tf,DMA_FROM_DEVICE , + dev->id,sizeof (dev->id))) goto err_out; -ata_qc_wait_err (qc, &wait); -swap_buf_le16 (dev->id,ATA_ID_WORDS );ata_dump_id (dev);
@@ -2473,6 +2427,7 @@ static void ata_dev_reread_id(struct atareturn ; err_out: +printk (KERN_ERR "ata%u: failed to reread ID, disabled\n" , ap->id);ata_port_disable (ap); }
@@ -2486,10 +2441,7 @@ err_out:static void ata_dev_init_params (struct ata_port *ap,struct ata_device *dev) { -DECLARE_COMPLETION (wait); -struct ata_queued_cmd *qc; -int rc; -unsigned long flags; +struct ata_taskfile tf;u16 sectors = dev->id[6 ];u16 heads = dev->id[3 ];
@@ -2500,26 +2452,18 @@ static void ata_dev_init_params(struct a/* set up init dev params taskfile */ DPRINTK ("init dev params \n" ); - qc =ata_qc_new_init (ap, dev); -BUG_ON (qc ==NULL ); - - qc->tf.command =ATA_CMD_INIT_DEV_PARAMS ; - qc->tf.flags |=ATA_TFLAG_ISADDR |ATA_TFLAG_DEVICE ; - qc->tf.protocol =ATA_PROT_NODATA ; - qc->tf.nsect = sectors; - qc->tf.device |= (heads -1 ) &0x0f ;/* max head = num. of heads - 1 */ - - qc->waiting = &wait; - qc->complete_fn = ata_qc_complete_noop; - -spin_lock_irqsave (&ap->host_set->lock, flags); - rc =ata_qc_issue (qc); -spin_unlock_irqrestore (&ap->host_set->lock, flags); +ata_tf_init (ap, &tf, dev->devno); + tf.command =ATA_CMD_INIT_DEV_PARAMS ; + tf.flags |=ATA_TFLAG_ISADDR |ATA_TFLAG_DEVICE ; + tf.protocol =ATA_PROT_NODATA ; + tf.nsect = sectors; + tf.device |= (heads -1 ) &0x0f ;/* max head = num. of heads - 1 */ -if (rc) +if (ata_exec_internal (ap, dev, &tf,DMA_NONE ,NULL ,0 )) { +printk (KERN_ERR "ata%u: failed to init parameters, disabled\n" , + ap->id);ata_port_disable (ap); -else -ata_qc_wait_err (qc, &wait); + }DPRINTK ("EXIT\n" ); }
PATCH 3/4 libata: remove unused functions
In reply to Jeff GarzikThere is no user of ata_qc_wait_err() and ata_qc_complete_noop() after ata_qc_exec_internal() change. Remove unused functions.
drivers/scsi/libata-core.c +0 -29
@@ -1147,30 +1147,6 @@ ata_exec_internal(struct ata_port *ap, sreturn AC_ERR_OTHER ; } -static int ata_qc_wait_err (struct ata_queued_cmd *qc, -struct completion *wait) -{ -int rc =0 ; - -if (wait_for_completion_timeout (wait,30 *HZ ) <1 ) { -/* timeout handling */ -unsigned int err_mask =ac_err_mask (ata_chk_status (qc->ap)); - -if (!err_mask) { -printk (KERN_WARNING "ata%u: slow completion (cmd %x)\n" , - qc->ap->id, qc->tf.command); - }else { -printk (KERN_WARNING "ata%u: qc timeout (cmd %x)\n" , - qc->ap->id, qc->tf.command); - rc = -EIO ; - } - -ata_qc_complete (qc, err_mask); - } - -return rc; -} -/** * ata_dev_identify - obtain IDENTIFY x DEVICE page * @ap: port on which device we wish to probe resides
@@ -3685,11 +3661,6 @@ struct ata_queued_cmd *ata_qc_new_init(sreturn qc; } -int ata_qc_complete_noop (struct ata_queued_cmd *qc,unsigned int err_mask) -{ -return 0 ; -} -static void __ata_qc_complete (struct ata_queued_cmd *qc) {struct ata_port *ap = qc->ap;
drivers/scsi/libata.h +0 -1
@@ -39,7 +39,6 @@ struct ata_scsi_args {/* libata-core.c */ extern int atapi_enabled; -extern int ata_qc_complete_noop (struct ata_queued_cmd *qc,unsigned int err_mask);extern struct ata_queued_cmd *ata_qc_new_init (struct ata_port *ap,struct ata_device *dev);extern void ata_rwcmd_protocol (struct ata_queued_cmd *qc);
PATCH 4/4 libata: remove unused qc->waiting
In reply to Jeff GarzikThere is no user of qc->waiting left after ata_exec_internal() change. Kill the field.
drivers/scsi/libata-core.c +2 -12
@@ -3664,7 +3664,7 @@ struct ata_queued_cmd *ata_qc_new_init(sstatic void __ata_qc_complete (struct ata_queued_cmd *qc) {struct ata_port *ap = qc->ap; -unsigned int tag, do_clear =0 ; +unsigned int tag; qc->flags =0 ; tag = qc->tag;
@@ -3672,17 +3672,8 @@ static void __ata_qc_complete(struct ataif (tag == ap->active_tag) ap->active_tag =ATA_TAG_POISON ; qc->tag =ATA_TAG_POISON ; - do_clear =1 ; - } - -if (qc->waiting) { -struct completion *waiting = qc->waiting; - qc->waiting =NULL ; -complete (waiting); - } - -if (likely (do_clear))clear_bit (tag, &ap->qactive); + } }/**
@@ -3698,7 +3689,6 @@ static void __ata_qc_complete(struct atavoid ata_qc_free (struct ata_queued_cmd *qc) {assert (qc !=NULL );/* ata_qc_from_tag _might_ return NULL */ -assert (qc->waiting ==NULL );/* nothing should be waiting */ __ata_qc_complete (qc); }
include/linux/libata.h +0 -2
@@ -287,8 +287,6 @@ struct ata_queued_cmd {ata_qc_cb_t complete_fn; -struct completion *waiting; -void *private_data; };
Re: [PATCH 1/2] libata: implement ata_exec_internal()
In reply to Jeff GarzikJeff, can you please ack or nack it?
Thanks.
Signature · 1 line
tejun
PATCH 1/4 libata: implement ata_exec_internal()
This patch implements ata_exec_internal() function which performs libata internal command execution. Previously, this was done by each user by manually initializing a qc, issueing it, waiting for its completion and handling errors. In addition to obvious code factoring, using ata_exec_internal() fixes the following bugs.
* qc not freed on issue failure
* ap->qactive clearing could race with the next internal command
* race between timeout handling and irq
* ignoring error condition not represented in tf->status
Also, qc & hardware are not accessed anymore once it's completed, making internal commands more conformant with general semantics. ata_exec_internal() also makes it easy to issue internal commands from multiple threads if that becomes necessary.
This patch only implements ata_exec_internal(). A following patch will convert all users.
--
Jeff, this is the third posting of this patchset. Changes from the second posting are
* adapted to late qc->err_mask change
Changes to the second posting from the first are
* no ata_busy_sleep
* no ->complete_data
* timeout is always an AC_ERR_OTHER error
* patches splitted
Thanks.
drivers/scsi/libata-core.c +101 -0
@@ -1047,6 +1047,107 @@ static unsigned int ata_pio_modes(constreturn modes; } +struct ata_exec_internal_arg { +unsigned int err_mask; +struct ata_taskfile *tf; +struct completion *waiting; +}; + +int ata_qc_complete_internal (struct ata_queued_cmd *qc) +{ +struct ata_exec_internal_arg *arg = qc->private_data; +struct completion *waiting = arg->waiting; + +if (!(qc->err_mask & ~AC_ERR_DEV )) + qc->ap->ops->tf_read (qc->ap, arg->tf); + arg->err_mask = qc->err_mask; + arg->waiting =NULL ; +complete (waiting); + +return 0 ; +} + +/** +* ata_exec_internal - execute libata internal command +* @ap: Port to which the command is sent +* @dev: Device to which the command is sent +* @tf: Taskfile registers for the command and the result +* @dma_dir: Data tranfer direction of the command +* @buf: Data buffer of the command +* @buflen: Length of data buffer +* +* Executes libata internal command with timeout. @tf contains +* command on entry and result on return. Timeout and error +* conditions are reported via return value. No recovery action +* is taken after a command times out. It's caller's duty to +* clean up after timeout. +* +* LOCKING: +* None. Should be called with kernel context, might sleep. +*/ + +static unsigned +ata_exec_internal (struct ata_port *ap,struct ata_device *dev, +struct ata_taskfile *tf, +int dma_dir,void *buf,unsigned int buflen) +{ +u8 command = tf->command; +struct ata_queued_cmd *qc; +DECLARE_COMPLETION (wait); +unsigned long flags; +struct ata_exec_internal_arg arg; + +spin_lock_irqsave (&ap->host_set->lock, flags); + + qc =ata_qc_new_init (ap, dev); +BUG_ON (qc ==NULL ); + + qc->tf = *tf; + qc->dma_dir = dma_dir; +if (dma_dir !=DMA_NONE ) { +ata_sg_init_one (qc, buf, buflen); + qc->nsect = buflen /ATA_SECT_SIZE ; +printk ("dma_dir=%d buflen=%u nsect=%d\n" , + dma_dir, buflen, qc->nsect); + } + + arg.waiting = &wait; + arg.tf = tf; + qc->private_data = &arg; + qc->complete_fn = ata_qc_complete_internal; + +if (ata_qc_issue (qc)) + goto issue_fail; + +spin_unlock_irqrestore (&ap->host_set->lock, flags); + +if (!wait_for_completion_timeout (&wait,ATA_TMOUT_INTERNAL )) { +spin_lock_irqsave (&ap->host_set->lock, flags); + +/* We're racing with irq here. If we lose, the +* following test prevents us from completing the qc +* again. If completion irq occurs after here but +* before the caller cleans up, it will result in a +* spurious interrupt. We can live with that. +*/ +if (arg.waiting) { + qc->err_mask =AC_ERR_OTHER ; +ata_qc_complete (qc); +printk (KERN_WARNING "ata%u: qc timeout (cmd 0x%x)\n" , + ap->id, command); + } + +spin_unlock_irqrestore (&ap->host_set->lock, flags); + } + +return arg.err_mask; + + issue_fail: +ata_qc_free (qc); +spin_unlock_irqrestore (&ap->host_set->lock, flags); +return AC_ERR_OTHER ; +} +static int ata_qc_wait_err (struct ata_queued_cmd *qc,struct completion *wait) {
include/linux/libata.h +2 -0
@@ -136,6 +136,8 @@ enum {ATA_TMOUT_BOOT_QUICK =7 *HZ ,/* hueristic */ ATA_TMOUT_DATAOUT =30 *HZ ,ATA_TMOUT_DATAOUT_QUICK =5 *HZ , +ATA_TMOUT_INTERNAL =30 *HZ , +ATA_TMOUT_INTERNAL_QUICK =5 *HZ ,/* ATA bus states */ BUS_UNKNOWN = 0,
Re: [PATCH 1/4] libata: implement ata_exec_internal()
Quoted 16 lines · Tejun Heo
This patch implements
ata_exec_internal()function which performs libata internal command execution. Previously, this was done by each user by manually initializing a qc, issueing it, waiting for its completion and handling errors. In addition to obvious code factoring, usingata_exec_internal()fixes the following bugs.* qc not freed on issue failure
*ap->qactiveclearing could race with the next internal command
* race between timeout handling and irq
* ignoring error condition not represented intf->statusAlso, qc & hardware are not accessed anymore once it's completed, making internal commands more conformant with general semantics.
ata_exec_internal()also makes it easy to issue internal commands from multiple threads if that becomes necessary.
Quoted 4 lines
This patch only implements
ata_exec_internal(). A following patch will convert all users.Signed-off-by Tejun Heo <[email protected]>
I ACK this version of the patchset, patches 1-4, but they don't seem to apply to 'upstream' branch:
[jgarzik@pretzel libata-dev]$ git-applymbox /g/tmp/mbox ~/info/signoff.txt 4 patch(es) to process.
Applying 'libata: implement ata_exec_internal()'
error: patch failed: include/linux/libata.h:136
error: include/linux/libata.h: patch does not apply
Re: [PATCH 1/4] libata: implement ata_exec_internal()
Quoted 26 lines · Jeff Garzik
Quoted 16 lines · Tejun Heo
This patch implements
ata_exec_internal()function which performs libata internal command execution. Previously, this was done by each user by manually initializing a qc, issueing it, waiting for its completion and handling errors. In addition to obvious code factoring, usingata_exec_internal()fixes the following bugs.* qc not freed on issue failure
*ap->qactiveclearing could race with the next internal command
* race between timeout handling and irq
* ignoring error condition not represented intf->statusAlso, qc & hardware are not accessed anymore once it's completed, making internal commands more conformant with general semantics.
ata_exec_internal()also makes it easy to issue internal commands from multiple threads if that becomes necessary.Quoted 4 lines
This patch only implements
ata_exec_internal(). A following patch will convert all users.Signed-off-by Tejun Heo <[email protected]>I ACK this version of the patchset, patches 1-4, but they don't seem to apply to 'upstream' branch:
Quoted 8 lines
[jgarzik@pretzel libata-dev]$ git-applymbox /g/tmp/mbox
~/info/signoff.txt4 patch(es) to process.Applying 'libata: implement
ata_exec_internal()'error: patch failed:
include/linux/libata.h:136
error:include/linux/libata.h: patch does not apply
Oh... Sorry, all patches are against the ALL branch. Do you want me to regenerate them against upstream?
Also, I'm currently working on a series of EH improvement patches against the ALL branch. Would generating patches against upstream be better?
Thanks.
Signature · 1 line
tejun
Re: [PATCH 1/4] libata: implement ata_exec_internal()
Quoted 1 line · Tejun Heo
Oh... Sorry, all patches are against the ALL branch. Do you want me to regenerate them against upstream?
Yes, please.
Quoted 1 line
Also, I'm currently working on a series of EH improvement patches against the ALL branch. Would generating patches against upstream be better?
Unfortunately, generating patches against the ALL branch makes a lot of sense -- but it's also the worst choice you could make.
Generating patches against the ALL branch means that your patch becomes dependent on -all- branches in libata-dev.git, even the branches that aren't going upstream for quite a while... which is generally not what you want at all.
ALL is largely for testing and automated push to -mm, but not really for developers.
Sometimes you will run into cases where I slacked off on the merge of upstream+irq-pio+etc and created a bug, but most of the time the merge is easy. If there are ALL-specific patches, you are welcome to submit them separately, with the recogition that any such ALL-specific patch would likely be dropped and recreated each time ALL gets blown away, and recreated.
Jeff
PATCH 1/4 libata: implement ata_exec_internal()
This patch implements ata_exec_internal() function which performs libata internal command execution. Previously, this was done by each user by manually initializing a qc, issueing it, waiting for its completion and handling errors. In addition to obvious code factoring, using ata_exec_internal() fixes the following bugs.
* qc not freed on issue failure
* ap->qactive clearing could race with the next internal command
* race between timeout handling and irq
* ignoring error condition not represented in tf->status
Also, qc & hardware are not accessed anymore once it's completed, making internal commands more conformant with general semantics. ata_exec_internal() also makes it easy to issue internal commands from multiple threads if that becomes necessary.
This patch only implements ata_exec_internal(). A following patch will convert all users.
--
Jeff, all patches have been regenerated against upstream branch as of today. (575ab52a218e4ff0667a6cbd972c3af443ee8713)
Also, I took out a debug printk from ata_exec_internal (don't know how that one got left there). Other than that, all patches are identical to the previous posting.
Thanks. :-)
drivers/scsi/libata-core.c +99 -0
@@ -1046,6 +1046,105 @@ static unsigned int ata_pio_modes(constreturn modes; } +struct ata_exec_internal_arg { +unsigned int err_mask; +struct ata_taskfile *tf; +struct completion *waiting; +}; + +int ata_qc_complete_internal (struct ata_queued_cmd *qc) +{ +struct ata_exec_internal_arg *arg = qc->private_data; +struct completion *waiting = arg->waiting; + +if (!(qc->err_mask & ~AC_ERR_DEV )) + qc->ap->ops->tf_read (qc->ap, arg->tf); + arg->err_mask = qc->err_mask; + arg->waiting =NULL ; +complete (waiting); + +return 0 ; +} + +/** +* ata_exec_internal - execute libata internal command +* @ap: Port to which the command is sent +* @dev: Device to which the command is sent +* @tf: Taskfile registers for the command and the result +* @dma_dir: Data tranfer direction of the command +* @buf: Data buffer of the command +* @buflen: Length of data buffer +* +* Executes libata internal command with timeout. @tf contains +* command on entry and result on return. Timeout and error +* conditions are reported via return value. No recovery action +* is taken after a command times out. It's caller's duty to +* clean up after timeout. +* +* LOCKING: +* None. Should be called with kernel context, might sleep. +*/ + +static unsigned +ata_exec_internal (struct ata_port *ap,struct ata_device *dev, +struct ata_taskfile *tf, +int dma_dir,void *buf,unsigned int buflen) +{ +u8 command = tf->command; +struct ata_queued_cmd *qc; +DECLARE_COMPLETION (wait); +unsigned long flags; +struct ata_exec_internal_arg arg; + +spin_lock_irqsave (&ap->host_set->lock, flags); + + qc =ata_qc_new_init (ap, dev); +BUG_ON (qc ==NULL ); + + qc->tf = *tf; + qc->dma_dir = dma_dir; +if (dma_dir !=DMA_NONE ) { +ata_sg_init_one (qc, buf, buflen); + qc->nsect = buflen /ATA_SECT_SIZE ; + } + + arg.waiting = &wait; + arg.tf = tf; + qc->private_data = &arg; + qc->complete_fn = ata_qc_complete_internal; + +if (ata_qc_issue (qc)) + goto issue_fail; + +spin_unlock_irqrestore (&ap->host_set->lock, flags); + +if (!wait_for_completion_timeout (&wait,ATA_TMOUT_INTERNAL )) { +spin_lock_irqsave (&ap->host_set->lock, flags); + +/* We're racing with irq here. If we lose, the +* following test prevents us from completing the qc +* again. If completion irq occurs after here but +* before the caller cleans up, it will result in a +* spurious interrupt. We can live with that. +*/ +if (arg.waiting) { + qc->err_mask =AC_ERR_OTHER ; +ata_qc_complete (qc); +printk (KERN_WARNING "ata%u: qc timeout (cmd 0x%x)\n" , + ap->id, command); + } + +spin_unlock_irqrestore (&ap->host_set->lock, flags); + } + +return arg.err_mask; + + issue_fail: +ata_qc_free (qc); +spin_unlock_irqrestore (&ap->host_set->lock, flags); +return AC_ERR_OTHER ; +} +static int ata_qc_wait_err (struct ata_queued_cmd *qc,struct completion *wait) {
include/linux/libata.h +2 -0
@@ -135,6 +135,8 @@ enum {ATA_TMOUT_BOOT_QUICK =7 *HZ ,/* hueristic */ ATA_TMOUT_CDB =30 *HZ ,ATA_TMOUT_CDB_QUICK =5 *HZ , +ATA_TMOUT_INTERNAL =30 *HZ , +ATA_TMOUT_INTERNAL_QUICK =5 *HZ ,/* ATA bus states */ BUS_UNKNOWN = 0,
PATCH 2/4 libata: use ata_exec_internal()
This patch converts all users of libata internal commands to use ata_exec_internal().
drivers/scsi/libata-core.c +44 -101
@@ -1198,9 +1198,8 @@ static void ata_dev_identify(struct ata_u16 tmp;unsigned long xfer_modes;unsigned int using_edd; -DECLARE_COMPLETION (wait); -struct ata_queued_cmd *qc; -unsigned long flags; +struct ata_taskfile tf; +unsigned int err_mask;int rc;if (!ata_dev_present (dev)) {
@@ -1221,40 +1220,26 @@ static void ata_dev_identify(struct ata_ata_dev_select (ap, device,1 ,1 );/* select device 0/1 */ - qc =ata_qc_new_init (ap, dev); -BUG_ON (qc ==NULL ); - -ata_sg_init_one (qc, dev->id,sizeof (dev->id)); - qc->dma_dir =DMA_FROM_DEVICE ; - qc->tf.protocol =ATA_PROT_PIO ; - qc->nsect =1 ; - retry: +ata_tf_init (ap, &tf, device); +if (dev->class ==ATA_DEV_ATA ) { - qc->tf.command =ATA_CMD_ID_ATA ; + tf.command =ATA_CMD_ID_ATA ;DPRINTK ("do ATA identify\n" ); }else { - qc->tf.command =ATA_CMD_ID_ATAPI ; + tf.command =ATA_CMD_ID_ATAPI ;DPRINTK ("do ATAPI identify\n" ); } - qc->waiting = &wait; - qc->complete_fn = ata_qc_complete_noop; - -spin_lock_irqsave (&ap->host_set->lock, flags); - rc =ata_qc_issue (qc); -spin_unlock_irqrestore (&ap->host_set->lock, flags); + tf.protocol =ATA_PROT_PIO ; -if (rc) - goto err_out; -else -ata_qc_wait_err (qc, &wait); + err_mask =ata_exec_internal (ap, dev, &tf,DMA_FROM_DEVICE , + dev->id,sizeof (dev->id)); -spin_lock_irqsave (&ap->host_set->lock, flags); - ap->ops->tf_read (ap, &qc->tf); -spin_unlock_irqrestore (&ap->host_set->lock, flags); +if (err_mask) { +if (err_mask & ~AC_ERR_DEV ) + goto err_out; -if (qc->tf.command &ATA_ERR ) {/* * arg! EDD works for all test cases, but seems to return * the ATA signature for some ATAPI devices. Until the
@@ -1267,14 +1252,9 @@ retry:* to have this problem. */ if ((using_edd) && (dev->class ==ATA_DEV_ATA )) { -u8 err = qc->tf.feature; +u8 err = tf.feature;if (err &ATA_ABORTED ) { dev->class =ATA_DEV_ATAPI ; - qc->cursg =0 ; - qc->cursg_ofs =0 ; - qc->cursect =0 ; - qc->nsect =1 ; - qc->err_mask =0 ; goto retry; } }
@@ -2378,34 +2358,23 @@ static int ata_choose_xfer_mode(const ststatic void ata_dev_set_xfermode (struct ata_port *ap,struct ata_device *dev) { -DECLARE_COMPLETION (wait); -struct ata_queued_cmd *qc; -int rc; -unsigned long flags; +struct ata_taskfile tf;/* set up set-features taskfile */ DPRINTK ("set features - xfer mode\n" ); - qc =ata_qc_new_init (ap, dev); -BUG_ON (qc ==NULL ); - - qc->tf.command =ATA_CMD_SET_FEATURES ; - qc->tf.feature =SETFEATURES_XFER ; - qc->tf.flags |=ATA_TFLAG_ISADDR |ATA_TFLAG_DEVICE ; - qc->tf.protocol =ATA_PROT_NODATA ; - qc->tf.nsect = dev->xfer_mode; - - qc->waiting = &wait; - qc->complete_fn = ata_qc_complete_noop; - -spin_lock_irqsave (&ap->host_set->lock, flags); - rc =ata_qc_issue (qc); -spin_unlock_irqrestore (&ap->host_set->lock, flags); +ata_tf_init (ap, &tf, dev->devno); + tf.command =ATA_CMD_SET_FEATURES ; + tf.feature =SETFEATURES_XFER ; + tf.flags |=ATA_TFLAG_ISADDR |ATA_TFLAG_DEVICE ; + tf.protocol =ATA_PROT_NODATA ; + tf.nsect = dev->xfer_mode; -if (rc) +if (ata_exec_internal (ap, dev, &tf,DMA_NONE ,NULL ,0 )) { +printk (KERN_ERR "ata%u: failed to set xfermode, disabled\n" , + ap->id);ata_port_disable (ap); -else -ata_qc_wait_err (qc, &wait); + }DPRINTK ("EXIT\n" ); }
@@ -2420,41 +2389,25 @@ static void ata_dev_set_xfermode(structstatic void ata_dev_reread_id (struct ata_port *ap,struct ata_device *dev) { -DECLARE_COMPLETION (wait); -struct ata_queued_cmd *qc; -unsigned long flags; -int rc; - - qc =ata_qc_new_init (ap, dev); -BUG_ON (qc ==NULL ); +struct ata_taskfile tf; -ata_sg_init_one (qc, dev->id,sizeof (dev->id)); - qc->dma_dir =DMA_FROM_DEVICE ; +ata_tf_init (ap, &tf, dev->devno);if (dev->class ==ATA_DEV_ATA ) { - qc->tf.command =ATA_CMD_ID_ATA ; + tf.command =ATA_CMD_ID_ATA ;DPRINTK ("do ATA identify\n" ); }else { - qc->tf.command =ATA_CMD_ID_ATAPI ; + tf.command =ATA_CMD_ID_ATAPI ;DPRINTK ("do ATAPI identify\n" ); } - qc->tf.flags |=ATA_TFLAG_DEVICE ; - qc->tf.protocol =ATA_PROT_PIO ; - qc->nsect =1 ; + tf.flags |=ATA_TFLAG_DEVICE ; + tf.protocol =ATA_PROT_PIO ; - qc->waiting = &wait; - qc->complete_fn = ata_qc_complete_noop; - -spin_lock_irqsave (&ap->host_set->lock, flags); - rc =ata_qc_issue (qc); -spin_unlock_irqrestore (&ap->host_set->lock, flags); - -if (rc) +if (ata_exec_internal (ap, dev, &tf,DMA_FROM_DEVICE , + dev->id,sizeof (dev->id))) goto err_out; -ata_qc_wait_err (qc, &wait); -swap_buf_le16 (dev->id,ATA_ID_WORDS );ata_dump_id (dev);
@@ -2463,6 +2416,7 @@ static void ata_dev_reread_id(struct atareturn ; err_out: +printk (KERN_ERR "ata%u: failed to reread ID, disabled\n" , ap->id);ata_port_disable (ap); }
@@ -2476,10 +2430,7 @@ err_out:static void ata_dev_init_params (struct ata_port *ap,struct ata_device *dev) { -DECLARE_COMPLETION (wait); -struct ata_queued_cmd *qc; -int rc; -unsigned long flags; +struct ata_taskfile tf;u16 sectors = dev->id[6 ];u16 heads = dev->id[3 ];
@@ -2490,26 +2441,18 @@ static void ata_dev_init_params(struct a/* set up init dev params taskfile */ DPRINTK ("init dev params \n" ); - qc =ata_qc_new_init (ap, dev); -BUG_ON (qc ==NULL ); - - qc->tf.command =ATA_CMD_INIT_DEV_PARAMS ; - qc->tf.flags |=ATA_TFLAG_ISADDR |ATA_TFLAG_DEVICE ; - qc->tf.protocol =ATA_PROT_NODATA ; - qc->tf.nsect = sectors; - qc->tf.device |= (heads -1 ) &0x0f ;/* max head = num. of heads - 1 */ - - qc->waiting = &wait; - qc->complete_fn = ata_qc_complete_noop; - -spin_lock_irqsave (&ap->host_set->lock, flags); - rc =ata_qc_issue (qc); -spin_unlock_irqrestore (&ap->host_set->lock, flags); +ata_tf_init (ap, &tf, dev->devno); + tf.command =ATA_CMD_INIT_DEV_PARAMS ; + tf.flags |=ATA_TFLAG_ISADDR |ATA_TFLAG_DEVICE ; + tf.protocol =ATA_PROT_NODATA ; + tf.nsect = sectors; + tf.device |= (heads -1 ) &0x0f ;/* max head = num. of heads - 1 */ -if (rc) +if (ata_exec_internal (ap, dev, &tf,DMA_NONE ,NULL ,0 )) { +printk (KERN_ERR "ata%u: failed to init parameters, disabled\n" , + ap->id);ata_port_disable (ap); -else -ata_qc_wait_err (qc, &wait); + }DPRINTK ("EXIT\n" ); }
PATCH 3/4 libata: remove unused functions
In reply to Tejun HeoThere is no user of ata_qc_wait_err() and ata_qc_complete_noop() after ata_exec_internal() changes. Remove unused functions.
drivers/scsi/libata-core.c +0 -29
@@ -1145,30 +1145,6 @@ ata_exec_internal(struct ata_port *ap, sreturn AC_ERR_OTHER ; } -static int ata_qc_wait_err (struct ata_queued_cmd *qc, -struct completion *wait) -{ -int rc =0 ; - -if (wait_for_completion_timeout (wait,30 *HZ ) <1 ) { -/* timeout handling */ - qc->err_mask |=ac_err_mask (ata_chk_status (qc->ap)); - -if (!qc->err_mask) { -printk (KERN_WARNING "ata%u: slow completion (cmd %x)\n" , - qc->ap->id, qc->tf.command); - }else { -printk (KERN_WARNING "ata%u: qc timeout (cmd %x)\n" , - qc->ap->id, qc->tf.command); - rc = -EIO ; - } - -ata_qc_complete (qc); - } - -return rc; -} -/** * ata_dev_identify - obtain IDENTIFY x DEVICE page * @ap: port on which device we wish to probe resides
@@ -3524,11 +3500,6 @@ struct ata_queued_cmd *ata_qc_new_init(sreturn qc; } -int ata_qc_complete_noop (struct ata_queued_cmd *qc) -{ -return 0 ; -} -static void __ata_qc_complete (struct ata_queued_cmd *qc) {struct ata_port *ap = qc->ap;
drivers/scsi/libata.h +0 -1
@@ -39,7 +39,6 @@ struct ata_scsi_args {/* libata-core.c */ extern int atapi_enabled; -extern int ata_qc_complete_noop (struct ata_queued_cmd *qc);extern struct ata_queued_cmd *ata_qc_new_init (struct ata_port *ap,struct ata_device *dev);extern void ata_rwcmd_protocol (struct ata_queued_cmd *qc);
PATCH 4/4 libata: remove unused qc->waiting
In reply to Tejun HeoThere is no user of qc->waiting left after ata_exec_internal() changes. Kill the field.
drivers/scsi/libata-core.c +2 -12
@@ -3503,7 +3503,7 @@ struct ata_queued_cmd *ata_qc_new_init(sstatic void __ata_qc_complete (struct ata_queued_cmd *qc) {struct ata_port *ap = qc->ap; -unsigned int tag, do_clear =0 ; +unsigned int tag; qc->flags =0 ; tag = qc->tag;
@@ -3511,17 +3511,8 @@ static void __ata_qc_complete(struct ataif (tag == ap->active_tag) ap->active_tag =ATA_TAG_POISON ; qc->tag =ATA_TAG_POISON ; - do_clear =1 ; - } - -if (qc->waiting) { -struct completion *waiting = qc->waiting; - qc->waiting =NULL ; -complete (waiting); - } - -if (likely (do_clear))clear_bit (tag, &ap->qactive); + } }/**
@@ -3537,7 +3528,6 @@ static void __ata_qc_complete(struct atavoid ata_qc_free (struct ata_queued_cmd *qc) {assert (qc !=NULL );/* ata_qc_from_tag _might_ return NULL */ -assert (qc->waiting ==NULL );/* nothing should be waiting */ __ata_qc_complete (qc); }
include/linux/libata.h +0 -2
@@ -285,8 +285,6 @@ struct ata_queued_cmd {ata_qc_cb_t complete_fn; -struct completion *waiting; -void *private_data; };
Re: [PATCH 1/4] libata: implement ata_exec_internal()
In reply to Tejun Heoapplied 1-4
Re: [PATCH 1/4] libata: implement ata_exec_internal()
In reply to Tejun HeoQuoted 10 lines · Tejun Heo
This patch implements
ata_exec_internal()function which performs libata internal command execution. Previously, this was done by each user by manually initializing a qc, issueing it, waiting for its completion and handling errors. In addition to obvious code factoring, usingata_exec_internal()fixes the following bugs.* qc not freed on issue failure
*ap->qactiveclearing could race with the next internal command
* race between timeout handling and irq
* ignoring error condition not represented intf->status
Thanks, this fixes the "phantom slave device" problem on my machine. :)
Quoted 11 lines
Also, qc & hardware are not accessed anymore once it's completed, making internal commands more conformant with general semantics.
ata_exec_internal()also makes it easy to issue internal commands from multiple threads if that becomes necessary.This patch only implements
ata_exec_internal(). A following patch will convert all users.Signed-off-by Tejun Heo <[email protected]>
Re: [PATCH 1/4] libata: implement ata_exec_internal()
In reply to Tejun HeoQuoted 1 line · Tejun Heo
Oh... Sorry, all patches are against the ALL branch. Do you want me to regenerate them against upstream?
Yes, please.
Quoted 1 line
Also, I'm currently working on a series of EH improvement patches against the ALL branch. Would generating patches against upstream be better?
Diffing against ALL means that your patches become dependent on all libata-dev.git branches, even ones that aren't going upstream for a long time. Plus, ALL is routinely blown away and recreated, when the merge gets too messy.
Jeff
PATCH 2/4 libata: use ata_exec_internal()
In reply to Tejun HeoThis patch converts all users of libata internal commands to use ata_exec_internal().
drivers/scsi/libata-core.c +44 -101
@@ -1201,9 +1201,8 @@ static void ata_dev_identify(struct ata_u16 tmp;unsigned long xfer_modes;unsigned int using_edd; -DECLARE_COMPLETION (wait); -struct ata_queued_cmd *qc; -unsigned long flags; +struct ata_taskfile tf; +unsigned int err_mask;int rc;if (!ata_dev_present (dev)) {
@@ -1224,40 +1223,26 @@ static void ata_dev_identify(struct ata_ata_dev_select (ap, device,1 ,1 );/* select device 0/1 */ - qc =ata_qc_new_init (ap, dev); -BUG_ON (qc ==NULL ); - -ata_sg_init_one (qc, dev->id,sizeof (dev->id)); - qc->dma_dir =DMA_FROM_DEVICE ; - qc->tf.protocol =ATA_PROT_PIO ; - qc->nsect =1 ; - retry: +ata_tf_init (ap, &tf, device); +if (dev->class ==ATA_DEV_ATA ) { - qc->tf.command =ATA_CMD_ID_ATA ; + tf.command =ATA_CMD_ID_ATA ;DPRINTK ("do ATA identify\n" ); }else { - qc->tf.command =ATA_CMD_ID_ATAPI ; + tf.command =ATA_CMD_ID_ATAPI ;DPRINTK ("do ATAPI identify\n" ); } - qc->waiting = &wait; - qc->complete_fn = ata_qc_complete_noop; - -spin_lock_irqsave (&ap->host_set->lock, flags); - rc =ata_qc_issue (qc); -spin_unlock_irqrestore (&ap->host_set->lock, flags); + tf.protocol =ATA_PROT_PIO ; -if (rc) - goto err_out; -else -ata_qc_wait_err (qc, &wait); + err_mask =ata_exec_internal (ap, dev, &tf,DMA_FROM_DEVICE , + dev->id,sizeof (dev->id)); -spin_lock_irqsave (&ap->host_set->lock, flags); - ap->ops->tf_read (ap, &qc->tf); -spin_unlock_irqrestore (&ap->host_set->lock, flags); +if (err_mask) { +if (err_mask & ~AC_ERR_DEV ) + goto err_out; -if (qc->tf.command &ATA_ERR ) {/* * arg! EDD works for all test cases, but seems to return * the ATA signature for some ATAPI devices. Until the
@@ -1270,14 +1255,9 @@ retry:* to have this problem. */ if ((using_edd) && (dev->class ==ATA_DEV_ATA )) { -u8 err = qc->tf.feature; +u8 err = tf.feature;if (err &ATA_ABORTED ) { dev->class =ATA_DEV_ATAPI ; - qc->cursg =0 ; - qc->cursg_ofs =0 ; - qc->cursect =0 ; - qc->nsect =1 ; - qc->err_mask =0 ; goto retry; } }
@@ -2390,34 +2370,23 @@ static int ata_choose_xfer_mode(const ststatic void ata_dev_set_xfermode (struct ata_port *ap,struct ata_device *dev) { -DECLARE_COMPLETION (wait); -struct ata_queued_cmd *qc; -int rc; -unsigned long flags; +struct ata_taskfile tf;/* set up set-features taskfile */ DPRINTK ("set features - xfer mode\n" ); - qc =ata_qc_new_init (ap, dev); -BUG_ON (qc ==NULL ); - - qc->tf.command =ATA_CMD_SET_FEATURES ; - qc->tf.feature =SETFEATURES_XFER ; - qc->tf.flags |=ATA_TFLAG_ISADDR |ATA_TFLAG_DEVICE ; - qc->tf.protocol =ATA_PROT_NODATA ; - qc->tf.nsect = dev->xfer_mode; - - qc->waiting = &wait; - qc->complete_fn = ata_qc_complete_noop; - -spin_lock_irqsave (&ap->host_set->lock, flags); - rc =ata_qc_issue (qc); -spin_unlock_irqrestore (&ap->host_set->lock, flags); +ata_tf_init (ap, &tf, dev->devno); + tf.command =ATA_CMD_SET_FEATURES ; + tf.feature =SETFEATURES_XFER ; + tf.flags |=ATA_TFLAG_ISADDR |ATA_TFLAG_DEVICE ; + tf.protocol =ATA_PROT_NODATA ; + tf.nsect = dev->xfer_mode; -if (rc) +if (ata_exec_internal (ap, dev, &tf,DMA_NONE ,NULL ,0 )) { +printk (KERN_ERR "ata%u: failed to set xfermode, disabled\n" , + ap->id);ata_port_disable (ap); -else -ata_qc_wait_err (qc, &wait); + }DPRINTK ("EXIT\n" ); }
@@ -2432,41 +2401,25 @@ static void ata_dev_set_xfermode(structstatic void ata_dev_reread_id (struct ata_port *ap,struct ata_device *dev) { -DECLARE_COMPLETION (wait); -struct ata_queued_cmd *qc; -unsigned long flags; -int rc; - - qc =ata_qc_new_init (ap, dev); -BUG_ON (qc ==NULL ); +struct ata_taskfile tf; -ata_sg_init_one (qc, dev->id,sizeof (dev->id)); - qc->dma_dir =DMA_FROM_DEVICE ; +ata_tf_init (ap, &tf, dev->devno);if (dev->class ==ATA_DEV_ATA ) { - qc->tf.command =ATA_CMD_ID_ATA ; + tf.command =ATA_CMD_ID_ATA ;DPRINTK ("do ATA identify\n" ); }else { - qc->tf.command =ATA_CMD_ID_ATAPI ; + tf.command =ATA_CMD_ID_ATAPI ;DPRINTK ("do ATAPI identify\n" ); } - qc->tf.flags |=ATA_TFLAG_DEVICE ; - qc->tf.protocol =ATA_PROT_PIO ; - qc->nsect =1 ; + tf.flags |=ATA_TFLAG_DEVICE ; + tf.protocol =ATA_PROT_PIO ; - qc->waiting = &wait; - qc->complete_fn = ata_qc_complete_noop; - -spin_lock_irqsave (&ap->host_set->lock, flags); - rc =ata_qc_issue (qc); -spin_unlock_irqrestore (&ap->host_set->lock, flags); - -if (rc) +if (ata_exec_internal (ap, dev, &tf,DMA_FROM_DEVICE , + dev->id,sizeof (dev->id))) goto err_out; -ata_qc_wait_err (qc, &wait); -swap_buf_le16 (dev->id,ATA_ID_WORDS );ata_dump_id (dev);
@@ -2475,6 +2428,7 @@ static void ata_dev_reread_id(struct atareturn ; err_out: +printk (KERN_ERR "ata%u: failed to reread ID, disabled\n" , ap->id);ata_port_disable (ap); }
@@ -2488,10 +2442,7 @@ err_out:static void ata_dev_init_params (struct ata_port *ap,struct ata_device *dev) { -DECLARE_COMPLETION (wait); -struct ata_queued_cmd *qc; -int rc; -unsigned long flags; +struct ata_taskfile tf;u16 sectors = dev->id[6 ];u16 heads = dev->id[3 ];
@@ -2502,26 +2453,18 @@ static void ata_dev_init_params(struct a/* set up init dev params taskfile */ DPRINTK ("init dev params \n" ); - qc =ata_qc_new_init (ap, dev); -BUG_ON (qc ==NULL ); - - qc->tf.command =ATA_CMD_INIT_DEV_PARAMS ; - qc->tf.flags |=ATA_TFLAG_ISADDR |ATA_TFLAG_DEVICE ; - qc->tf.protocol =ATA_PROT_NODATA ; - qc->tf.nsect = sectors; - qc->tf.device |= (heads -1 ) &0x0f ;/* max head = num. of heads - 1 */ - - qc->waiting = &wait; - qc->complete_fn = ata_qc_complete_noop; - -spin_lock_irqsave (&ap->host_set->lock, flags); - rc =ata_qc_issue (qc); -spin_unlock_irqrestore (&ap->host_set->lock, flags); +ata_tf_init (ap, &tf, dev->devno); + tf.command =ATA_CMD_INIT_DEV_PARAMS ; + tf.flags |=ATA_TFLAG_ISADDR |ATA_TFLAG_DEVICE ; + tf.protocol =ATA_PROT_NODATA ; + tf.nsect = sectors; + tf.device |= (heads -1 ) &0x0f ;/* max head = num. of heads - 1 */ -if (rc) +if (ata_exec_internal (ap, dev, &tf,DMA_NONE ,NULL ,0 )) { +printk (KERN_ERR "ata%u: failed to init parameters, disabled\n" , + ap->id);ata_port_disable (ap); -else -ata_qc_wait_err (qc, &wait); + }DPRINTK ("EXIT\n" ); }
PATCH 3/4 libata: removed unused functions
In reply to Tejun HeoThere is no user of ata_qc_wait_err() and ata_qc_complete_noop() after ata_exec_internal() changes. Remove unused functions.
drivers/scsi/libata-core.c +0 -29
@@ -1148,30 +1148,6 @@ ata_exec_internal(struct ata_port *ap, sreturn AC_ERR_OTHER ; } -static int ata_qc_wait_err (struct ata_queued_cmd *qc, -struct completion *wait) -{ -int rc =0 ; - -if (wait_for_completion_timeout (wait,30 *HZ ) <1 ) { -/* timeout handling */ - qc->err_mask |=ac_err_mask (ata_chk_status (qc->ap)); - -if (!qc->err_mask) { -printk (KERN_WARNING "ata%u: slow completion (cmd %x)\n" , - qc->ap->id, qc->tf.command); - }else { -printk (KERN_WARNING "ata%u: qc timeout (cmd %x)\n" , - qc->ap->id, qc->tf.command); - rc = -EIO ; - } - -ata_qc_complete (qc); - } - -return rc; -} -/** * ata_dev_identify - obtain IDENTIFY x DEVICE page * @ap: port on which device we wish to probe resides
@@ -3709,11 +3685,6 @@ struct ata_queued_cmd *ata_qc_new_init(sreturn qc; } -int ata_qc_complete_noop (struct ata_queued_cmd *qc) -{ -return 0 ; -} -static void __ata_qc_complete (struct ata_queued_cmd *qc) {struct ata_port *ap = qc->ap;
drivers/scsi/libata.h +0 -1
@@ -39,7 +39,6 @@ struct ata_scsi_args {/* libata-core.c */ extern int atapi_enabled; -extern int ata_qc_complete_noop (struct ata_queued_cmd *qc);extern struct ata_queued_cmd *ata_qc_new_init (struct ata_port *ap,struct ata_device *dev);extern void ata_rwcmd_protocol (struct ata_queued_cmd *qc);
PATCH 4/4 libata: remove unused qc->waiting
In reply to Tejun HeoThere is no user of qc->waiting left after ata_exec_internal() changes. Kill the field.
drivers/scsi/libata-core.c +2 -12
@@ -3688,7 +3688,7 @@ struct ata_queued_cmd *ata_qc_new_init(sstatic void __ata_qc_complete (struct ata_queued_cmd *qc) {struct ata_port *ap = qc->ap; -unsigned int tag, do_clear =0 ; +unsigned int tag; qc->flags =0 ; tag = qc->tag;
@@ -3696,17 +3696,8 @@ static void __ata_qc_complete(struct ataif (tag == ap->active_tag) ap->active_tag =ATA_TAG_POISON ; qc->tag =ATA_TAG_POISON ; - do_clear =1 ; - } - -if (qc->waiting) { -struct completion *waiting = qc->waiting; - qc->waiting =NULL ; -complete (waiting); - } - -if (likely (do_clear))clear_bit (tag, &ap->qactive); + } }/**
@@ -3722,7 +3713,6 @@ static void __ata_qc_complete(struct atavoid ata_qc_free (struct ata_queued_cmd *qc) {assert (qc !=NULL );/* ata_qc_from_tag _might_ return NULL */ -assert (qc->waiting ==NULL );/* nothing should be waiting */ __ata_qc_complete (qc); }
include/linux/libata.h +0 -2
@@ -289,8 +289,6 @@ struct ata_queued_cmd {ata_qc_cb_t complete_fn; -struct completion *waiting; -void *private_data; };
PATCH 2/2 libata: remove qc->waiting
In reply to Albert LeeWith internal commands converted to use ata_exec_internal(), there is no user of qc->waiting left. Kill it.
drivers/scsi/libata-core.c +2 -12
@@ -3679,7 +3679,7 @@ struct ata_queued_cmd *ata_qc_new_init(sstatic void __ata_qc_complete (struct ata_queued_cmd *qc) {struct ata_port *ap = qc->ap; -unsigned int tag, do_clear =0 ; +unsigned int tag; qc->flags =0 ; tag = qc->tag;
@@ -3687,17 +3687,8 @@ static void __ata_qc_complete(struct ataif (tag == ap->active_tag) ap->active_tag =ATA_TAG_POISON ; qc->tag =ATA_TAG_POISON ; - do_clear =1 ; - } - -if (qc->waiting) { -struct completion *waiting = qc->waiting; - qc->waiting =NULL ; -complete (waiting); - } - -if (likely (do_clear))clear_bit (tag, &ap->qactive); + } }/**
@@ -3713,7 +3704,6 @@ static void __ata_qc_complete(struct atavoid ata_qc_free (struct ata_queued_cmd *qc) {assert (qc !=NULL );/* ata_qc_from_tag _might_ return NULL */ -assert (qc->waiting ==NULL );/* nothing should be waiting */ __ata_qc_complete (qc); }
include/linux/libata.h +0 -2
@@ -288,8 +288,6 @@ struct ata_queued_cmd {ata_qc_cb_t complete_fn;void *complete_data; -struct completion *waiting; -void *private_data; };