* [PATCH 05/12] libata: return AC_ERR_* from issue functions
2006-01-22 7:58 [PATCHSET] libata: various fixes related to EH Tejun Heo
@ 2006-01-22 7:58 ` Tejun Heo
2006-01-22 9:36 ` Jeff Garzik
2006-01-22 7:58 ` [PATCH 04/12] libata: add detailed AC_ERR_* flags Tejun Heo
` (11 subsequent siblings)
12 siblings, 1 reply; 28+ messages in thread
From: Tejun Heo @ 2006-01-22 7:58 UTC (permalink / raw)
To: jgarzik, linux-ide, albertcc; +Cc: Tejun Heo
Return AC_ERR_* mask from issue fuctions instead of 0/-1. This
enables things like failing a qc with AC_ERR_HSM when the device
doesn't set DRDY when the qc is about to be issued.
Signed-off-by: Tejun Heo <htejun@gmail.com>
---
drivers/scsi/ahci.c | 4 ++--
drivers/scsi/libata-core.c | 17 ++++++++---------
drivers/scsi/libata-scsi.c | 10 ++++------
drivers/scsi/libata.h | 2 +-
drivers/scsi/pdc_adma.c | 4 ++--
drivers/scsi/sata_mv.c | 4 ++--
drivers/scsi/sata_promise.c | 4 ++--
drivers/scsi/sata_qstor.c | 4 ++--
drivers/scsi/sata_sil24.c | 4 ++--
drivers/scsi/sata_sx4.c | 4 ++--
include/linux/libata.h | 4 ++--
11 files changed, 29 insertions(+), 32 deletions(-)
027f06c43b4e575cc58d2dcb6af8723ef3cd3f2d
diff --git a/drivers/scsi/ahci.c b/drivers/scsi/ahci.c
index e7b937b..75b1fb5 100644
--- a/drivers/scsi/ahci.c
+++ b/drivers/scsi/ahci.c
@@ -184,7 +184,7 @@ struct ahci_port_priv {
static u32 ahci_scr_read (struct ata_port *ap, unsigned int sc_reg);
static void ahci_scr_write (struct ata_port *ap, unsigned int sc_reg, u32 val);
static int ahci_init_one (struct pci_dev *pdev, const struct pci_device_id *ent);
-static int ahci_qc_issue(struct ata_queued_cmd *qc);
+static unsigned int ahci_qc_issue(struct ata_queued_cmd *qc);
static irqreturn_t ahci_interrupt (int irq, void *dev_instance, struct pt_regs *regs);
static void ahci_phy_reset(struct ata_port *ap);
static void ahci_irq_clear(struct ata_port *ap);
@@ -798,7 +798,7 @@ static irqreturn_t ahci_interrupt (int i
return IRQ_RETVAL(handled);
}
-static int ahci_qc_issue(struct ata_queued_cmd *qc)
+static unsigned int ahci_qc_issue(struct ata_queued_cmd *qc)
{
struct ata_port *ap = qc->ap;
void __iomem *port_mmio = (void __iomem *) ap->ioaddr.cmd_addr;
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index f5519f0..b29bf0d 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -1125,10 +1125,9 @@ ata_exec_internal(struct ata_port *ap, s
qc->private_data = &wait;
qc->complete_fn = ata_qc_complete_internal;
- if (ata_qc_issue(qc)) {
- qc->err_mask = AC_ERR_OTHER;
+ qc->err_mask = ata_qc_issue(qc);
+ if (qc->err_mask)
ata_qc_complete(qc);
- }
spin_unlock_irqrestore(&ap->host_set->lock, flags);
@@ -3674,10 +3673,10 @@ static inline int ata_should_dma_map(str
* spin_lock_irqsave(host_set lock)
*
* RETURNS:
- * Zero on success, negative on error.
+ * Zero on success, AC_ERR_* mask on failure
*/
-int ata_qc_issue(struct ata_queued_cmd *qc)
+unsigned int ata_qc_issue(struct ata_queued_cmd *qc)
{
struct ata_port *ap = qc->ap;
@@ -3702,7 +3701,7 @@ int ata_qc_issue(struct ata_queued_cmd *
sg_err:
qc->flags &= ~ATA_QCFLAG_DMAMAP;
- return -1;
+ return AC_ERR_SYSTEM;
}
@@ -3721,10 +3720,10 @@ sg_err:
* spin_lock_irqsave(host_set lock)
*
* RETURNS:
- * Zero on success, negative on error.
+ * Zero on success, AC_ERR_* mask on failure
*/
-int ata_qc_issue_prot(struct ata_queued_cmd *qc)
+unsigned int ata_qc_issue_prot(struct ata_queued_cmd *qc)
{
struct ata_port *ap = qc->ap;
@@ -3769,7 +3768,7 @@ int ata_qc_issue_prot(struct ata_queued_
default:
WARN_ON(1);
- return -1;
+ return AC_ERR_SYSTEM;
}
return 0;
diff --git a/drivers/scsi/libata-scsi.c b/drivers/scsi/libata-scsi.c
index c496309..95e3c27 100644
--- a/drivers/scsi/libata-scsi.c
+++ b/drivers/scsi/libata-scsi.c
@@ -1322,10 +1322,9 @@ static void ata_scsi_translate(struct at
goto early_finish;
/* select device, send command to hardware */
- if (ata_qc_issue(qc)) {
- qc->err_mask |= AC_ERR_OTHER;
+ qc->err_mask = ata_qc_issue(qc);
+ if (qc->err_mask)
ata_qc_complete(qc);
- }
VPRINTK("EXIT\n");
return;
@@ -2044,10 +2043,9 @@ static void atapi_request_sense(struct a
qc->complete_fn = atapi_sense_complete;
- if (ata_qc_issue(qc)) {
- qc->err_mask |= AC_ERR_OTHER;
+ qc->err_mask = ata_qc_issue(qc);
+ if (qc->err_mask)
ata_qc_complete(qc);
- }
DPRINTK("EXIT\n");
}
diff --git a/drivers/scsi/libata.h b/drivers/scsi/libata.h
index e03ce48..9d76923 100644
--- a/drivers/scsi/libata.h
+++ b/drivers/scsi/libata.h
@@ -45,7 +45,7 @@ extern struct ata_queued_cmd *ata_qc_new
struct ata_device *dev);
extern int ata_rwcmd_protocol(struct ata_queued_cmd *qc);
extern void ata_qc_free(struct ata_queued_cmd *qc);
-extern int ata_qc_issue(struct ata_queued_cmd *qc);
+extern unsigned int ata_qc_issue(struct ata_queued_cmd *qc);
extern int ata_check_atapi_dma(struct ata_queued_cmd *qc);
extern void ata_dev_select(struct ata_port *ap, unsigned int device,
unsigned int wait, unsigned int can_sleep);
diff --git a/drivers/scsi/pdc_adma.c b/drivers/scsi/pdc_adma.c
index e8df0c9..3a6bf58 100644
--- a/drivers/scsi/pdc_adma.c
+++ b/drivers/scsi/pdc_adma.c
@@ -131,7 +131,7 @@ static void adma_host_stop(struct ata_ho
static void adma_port_stop(struct ata_port *ap);
static void adma_phy_reset(struct ata_port *ap);
static void adma_qc_prep(struct ata_queued_cmd *qc);
-static int adma_qc_issue(struct ata_queued_cmd *qc);
+static unsigned int adma_qc_issue(struct ata_queued_cmd *qc);
static int adma_check_atapi_dma(struct ata_queued_cmd *qc);
static void adma_bmdma_stop(struct ata_queued_cmd *qc);
static u8 adma_bmdma_status(struct ata_port *ap);
@@ -419,7 +419,7 @@ static inline void adma_packet_start(str
writew(aPIOMD4 | aGO, chan + ADMA_CONTROL);
}
-static int adma_qc_issue(struct ata_queued_cmd *qc)
+static unsigned int adma_qc_issue(struct ata_queued_cmd *qc)
{
struct adma_port_priv *pp = qc->ap->private_data;
diff --git a/drivers/scsi/sata_mv.c b/drivers/scsi/sata_mv.c
index 89bcd85..281223a 100644
--- a/drivers/scsi/sata_mv.c
+++ b/drivers/scsi/sata_mv.c
@@ -328,7 +328,7 @@ static void mv_host_stop(struct ata_host
static int mv_port_start(struct ata_port *ap);
static void mv_port_stop(struct ata_port *ap);
static void mv_qc_prep(struct ata_queued_cmd *qc);
-static int mv_qc_issue(struct ata_queued_cmd *qc);
+static unsigned int mv_qc_issue(struct ata_queued_cmd *qc);
static irqreturn_t mv_interrupt(int irq, void *dev_instance,
struct pt_regs *regs);
static void mv_eng_timeout(struct ata_port *ap);
@@ -1040,7 +1040,7 @@ static void mv_qc_prep(struct ata_queued
* LOCKING:
* Inherited from caller.
*/
-static int mv_qc_issue(struct ata_queued_cmd *qc)
+static unsigned int mv_qc_issue(struct ata_queued_cmd *qc)
{
void __iomem *port_mmio = mv_ap_base(qc->ap);
struct mv_port_priv *pp = qc->ap->private_data;
diff --git a/drivers/scsi/sata_promise.c b/drivers/scsi/sata_promise.c
index b0b0a69..bac36d5 100644
--- a/drivers/scsi/sata_promise.c
+++ b/drivers/scsi/sata_promise.c
@@ -95,7 +95,7 @@ static void pdc_qc_prep(struct ata_queue
static void pdc_tf_load_mmio(struct ata_port *ap, const struct ata_taskfile *tf);
static void pdc_exec_command_mmio(struct ata_port *ap, const struct ata_taskfile *tf);
static void pdc_irq_clear(struct ata_port *ap);
-static int pdc_qc_issue_prot(struct ata_queued_cmd *qc);
+static unsigned int pdc_qc_issue_prot(struct ata_queued_cmd *qc);
static struct scsi_host_template pdc_ata_sht = {
@@ -544,7 +544,7 @@ static inline void pdc_packet_start(stru
readl((void __iomem *) ap->ioaddr.cmd_addr + PDC_PKT_SUBMIT); /* flush */
}
-static int pdc_qc_issue_prot(struct ata_queued_cmd *qc)
+static unsigned int pdc_qc_issue_prot(struct ata_queued_cmd *qc)
{
switch (qc->tf.protocol) {
case ATA_PROT_DMA:
diff --git a/drivers/scsi/sata_qstor.c b/drivers/scsi/sata_qstor.c
index de05e28..2afbeb7 100644
--- a/drivers/scsi/sata_qstor.c
+++ b/drivers/scsi/sata_qstor.c
@@ -120,7 +120,7 @@ static void qs_host_stop(struct ata_host
static void qs_port_stop(struct ata_port *ap);
static void qs_phy_reset(struct ata_port *ap);
static void qs_qc_prep(struct ata_queued_cmd *qc);
-static int qs_qc_issue(struct ata_queued_cmd *qc);
+static unsigned int qs_qc_issue(struct ata_queued_cmd *qc);
static int qs_check_atapi_dma(struct ata_queued_cmd *qc);
static void qs_bmdma_stop(struct ata_queued_cmd *qc);
static u8 qs_bmdma_status(struct ata_port *ap);
@@ -352,7 +352,7 @@ static inline void qs_packet_start(struc
readl(chan + QS_CCT_CFF); /* flush */
}
-static int qs_qc_issue(struct ata_queued_cmd *qc)
+static unsigned int qs_qc_issue(struct ata_queued_cmd *qc)
{
struct qs_port_priv *pp = qc->ap->private_data;
diff --git a/drivers/scsi/sata_sil24.c b/drivers/scsi/sata_sil24.c
index fb59012..5a7a7b1 100644
--- a/drivers/scsi/sata_sil24.c
+++ b/drivers/scsi/sata_sil24.c
@@ -251,7 +251,7 @@ static void sil24_scr_write(struct ata_p
static void sil24_tf_read(struct ata_port *ap, struct ata_taskfile *tf);
static void sil24_phy_reset(struct ata_port *ap);
static void sil24_qc_prep(struct ata_queued_cmd *qc);
-static int sil24_qc_issue(struct ata_queued_cmd *qc);
+static unsigned int sil24_qc_issue(struct ata_queued_cmd *qc);
static void sil24_irq_clear(struct ata_port *ap);
static void sil24_eng_timeout(struct ata_port *ap);
static irqreturn_t sil24_interrupt(int irq, void *dev_instance, struct pt_regs *regs);
@@ -557,7 +557,7 @@ static void sil24_qc_prep(struct ata_que
sil24_fill_sg(qc, sge);
}
-static int sil24_qc_issue(struct ata_queued_cmd *qc)
+static unsigned int sil24_qc_issue(struct ata_queued_cmd *qc)
{
struct ata_port *ap = qc->ap;
void __iomem *port = (void __iomem *)ap->ioaddr.cmd_addr;
diff --git a/drivers/scsi/sata_sx4.c b/drivers/scsi/sata_sx4.c
index bc87c16..3175c6b 100644
--- a/drivers/scsi/sata_sx4.c
+++ b/drivers/scsi/sata_sx4.c
@@ -174,7 +174,7 @@ static void pdc20621_get_from_dimm(struc
static void pdc20621_put_to_dimm(struct ata_probe_ent *pe,
void *psource, u32 offset, u32 size);
static void pdc20621_irq_clear(struct ata_port *ap);
-static int pdc20621_qc_issue_prot(struct ata_queued_cmd *qc);
+static unsigned int pdc20621_qc_issue_prot(struct ata_queued_cmd *qc);
static struct scsi_host_template pdc_sata_sht = {
@@ -678,7 +678,7 @@ static void pdc20621_packet_start(struct
}
}
-static int pdc20621_qc_issue_prot(struct ata_queued_cmd *qc)
+static unsigned int pdc20621_qc_issue_prot(struct ata_queued_cmd *qc)
{
switch (qc->tf.protocol) {
case ATA_PROT_DMA:
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 8ff3a7f..b1ea2f9 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -427,7 +427,7 @@ struct ata_port_operations {
void (*bmdma_start) (struct ata_queued_cmd *qc);
void (*qc_prep) (struct ata_queued_cmd *qc);
- int (*qc_issue) (struct ata_queued_cmd *qc);
+ unsigned int (*qc_issue) (struct ata_queued_cmd *qc);
void (*eng_timeout) (struct ata_port *ap);
@@ -515,7 +515,7 @@ extern void ata_port_stop (struct ata_po
extern void ata_host_stop (struct ata_host_set *host_set);
extern irqreturn_t ata_interrupt (int irq, void *dev_instance, struct pt_regs *regs);
extern void ata_qc_prep(struct ata_queued_cmd *qc);
-extern int ata_qc_issue_prot(struct ata_queued_cmd *qc);
+extern unsigned int ata_qc_issue_prot(struct ata_queued_cmd *qc);
extern void ata_sg_init_one(struct ata_queued_cmd *qc, void *buf,
unsigned int buflen);
extern void ata_sg_init(struct ata_queued_cmd *qc, struct scatterlist *sg,
--
1.0.8
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH 05/12] libata: return AC_ERR_* from issue functions
2006-01-22 7:58 ` [PATCH 05/12] libata: return AC_ERR_* from issue functions Tejun Heo
@ 2006-01-22 9:36 ` Jeff Garzik
0 siblings, 0 replies; 28+ messages in thread
From: Jeff Garzik @ 2006-01-22 9:36 UTC (permalink / raw)
To: Tejun Heo; +Cc: linux-ide, albertcc
Tejun Heo wrote:
> Return AC_ERR_* mask from issue fuctions instead of 0/-1. This
> enables things like failing a qc with AC_ERR_HSM when the device
> doesn't set DRDY when the qc is about to be issued.
>
> Signed-off-by: Tejun Heo <htejun@gmail.com>
>
> ---
>
> drivers/scsi/ahci.c | 4 ++--
> drivers/scsi/libata-core.c | 17 ++++++++---------
> drivers/scsi/libata-scsi.c | 10 ++++------
> drivers/scsi/libata.h | 2 +-
> drivers/scsi/pdc_adma.c | 4 ++--
> drivers/scsi/sata_mv.c | 4 ++--
> drivers/scsi/sata_promise.c | 4 ++--
> drivers/scsi/sata_qstor.c | 4 ++--
> drivers/scsi/sata_sil24.c | 4 ++--
> drivers/scsi/sata_sx4.c | 4 ++--
> include/linux/libata.h | 4 ++--
> 11 files changed, 29 insertions(+), 32 deletions(-)
OK, except, this makes me wonder if an ata_err_t type is needed.
Jeff
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 04/12] libata: add detailed AC_ERR_* flags
2006-01-22 7:58 [PATCHSET] libata: various fixes related to EH Tejun Heo
2006-01-22 7:58 ` [PATCH 05/12] libata: return AC_ERR_* from issue functions Tejun Heo
@ 2006-01-22 7:58 ` Tejun Heo
2006-01-22 9:30 ` Jeff Garzik
2006-01-22 7:58 ` [PATCH 06/12] SCSI: export scsi_eh_finish_cmd() and scsi_eh_flush_done_q() Tejun Heo
` (10 subsequent siblings)
12 siblings, 1 reply; 28+ messages in thread
From: Tejun Heo @ 2006-01-22 7:58 UTC (permalink / raw)
To: jgarzik, linux-ide, albertcc; +Cc: Tejun Heo
Add detailed AC_ERR_* flags and use them. Long-term goal is to
describe all errors with err_mask and tf combination (tf for failed
sector information, etc...). After proper error diagnosis is
implemented, sense data should also be generated from err_mask instead
of directly from hardware tf registers as it is currently.
Signed-off-by: Tejun Heo <htejun@gmail.com>
---
drivers/scsi/ahci.c | 6 ++----
drivers/scsi/libata-core.c | 12 ++++++------
drivers/scsi/sata_mv.c | 2 +-
drivers/scsi/sata_sil24.c | 2 +-
include/linux/libata.h | 15 ++++++++++-----
5 files changed, 20 insertions(+), 17 deletions(-)
1939435a8f0e7b2b1578b84cedd49f8adcab1ea9
diff --git a/drivers/scsi/ahci.c b/drivers/scsi/ahci.c
index 30676b0..e7b937b 100644
--- a/drivers/scsi/ahci.c
+++ b/drivers/scsi/ahci.c
@@ -680,7 +680,7 @@ static void ahci_eng_timeout(struct ata_
* not being called from the SCSI EH.
*/
qc->scsidone = scsi_finish_command;
- qc->err_mask |= AC_ERR_OTHER;
+ qc->err_mask |= AC_ERR_TIMEOUT;
ata_qc_complete(qc);
}
@@ -720,10 +720,8 @@ static inline int ahci_host_intr(struct
/* command processing has stopped due to error; restart */
ahci_restart_port(ap, status);
- if (qc) {
- qc->err_mask |= AC_ERR_OTHER;
+ if (qc)
ata_qc_complete(qc);
- }
}
return 1;
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index 43a2328..f5519f0 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -1142,7 +1142,7 @@ ata_exec_internal(struct ata_port *ap, s
* spurious interrupt. We can live with that.
*/
if (qc->flags & ATA_QCFLAG_ACTIVE) {
- qc->err_mask = AC_ERR_OTHER;
+ qc->err_mask = AC_ERR_TIMEOUT;
ata_qc_complete(qc);
printk(KERN_WARNING "ata%u: qc timeout (cmd 0x%x)\n",
ap->id, command);
@@ -2917,7 +2917,7 @@ static unsigned long ata_pio_poll(struct
status = ata_chk_status(ap);
if (status & ATA_BUSY) {
if (time_after(jiffies, ap->pio_task_timeout)) {
- qc->err_mask |= AC_ERR_ATA_BUS;
+ qc->err_mask |= AC_ERR_TIMEOUT;
ap->hsm_task_state = HSM_ST_TMOUT;
return 0;
}
@@ -3295,7 +3295,7 @@ static void atapi_pio_bytes(struct ata_q
err_out:
printk(KERN_INFO "ata%u: dev %u: ATAPI check failed\n",
ap->id, dev->devno);
- qc->err_mask |= AC_ERR_ATA_BUS;
+ qc->err_mask |= AC_ERR_HSM;
ap->hsm_task_state = HSM_ST_ERR;
}
@@ -3353,7 +3353,7 @@ static void ata_pio_block(struct ata_por
} else {
/* handle BSY=0, DRQ=0 as error */
if ((status & ATA_DRQ) == 0) {
- qc->err_mask |= AC_ERR_ATA_BUS;
+ qc->err_mask |= AC_ERR_HSM;
ap->hsm_task_state = HSM_ST_ERR;
return;
}
@@ -4159,14 +4159,14 @@ static void atapi_packet_task(void *_dat
/* sleep-wait for BSY to clear */
DPRINTK("busy wait\n");
if (ata_busy_sleep(ap, ATA_TMOUT_CDB_QUICK, ATA_TMOUT_CDB)) {
- qc->err_mask |= AC_ERR_ATA_BUS;
+ qc->err_mask |= AC_ERR_TIMEOUT;
goto err_out;
}
/* make sure DRQ is set */
status = ata_chk_status(ap);
if ((status & (ATA_BUSY | ATA_DRQ)) != ATA_DRQ) {
- qc->err_mask |= AC_ERR_ATA_BUS;
+ qc->err_mask |= AC_ERR_HSM;
goto err_out;
}
diff --git a/drivers/scsi/sata_mv.c b/drivers/scsi/sata_mv.c
index cd54244..89bcd85 100644
--- a/drivers/scsi/sata_mv.c
+++ b/drivers/scsi/sata_mv.c
@@ -1866,7 +1866,7 @@ static void mv_eng_timeout(struct ata_po
*/
spin_lock_irqsave(&ap->host_set->lock, flags);
qc->scsidone = scsi_finish_command;
- qc->err_mask |= AC_ERR_OTHER;
+ qc->err_mask |= AC_ERR_TIMEOUT;
ata_qc_complete(qc);
spin_unlock_irqrestore(&ap->host_set->lock, flags);
}
diff --git a/drivers/scsi/sata_sil24.c b/drivers/scsi/sata_sil24.c
index 9231301..fb59012 100644
--- a/drivers/scsi/sata_sil24.c
+++ b/drivers/scsi/sata_sil24.c
@@ -653,7 +653,7 @@ static void sil24_eng_timeout(struct ata
*/
printk(KERN_ERR "ata%u: command timeout\n", ap->id);
qc->scsidone = scsi_finish_command;
- qc->err_mask |= AC_ERR_OTHER;
+ qc->err_mask |= AC_ERR_TIMEOUT;
ata_qc_complete(qc);
sil24_reset_controller(ap);
diff --git a/include/linux/libata.h b/include/linux/libata.h
index d58b659..8ff3a7f 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -222,10 +222,15 @@ enum hsm_task_states {
};
enum ata_completion_errors {
- AC_ERR_OTHER = (1 << 0),
- AC_ERR_DEV = (1 << 1),
- AC_ERR_ATA_BUS = (1 << 2),
- AC_ERR_HOST_BUS = (1 << 3),
+ AC_ERR_DEV = (1 << 0), /* device reported error */
+ AC_ERR_HSM = (1 << 1), /* host state machine violation */
+ AC_ERR_TIMEOUT = (1 << 2), /* timeout */
+ AC_ERR_MEDIA = (1 << 3), /* media error */
+ AC_ERR_ATA_BUS = (1 << 4), /* ATA bus error */
+ AC_ERR_HOST_BUS = (1 << 5), /* host bus error */
+ AC_ERR_SYSTEM = (1 << 6), /* system error */
+ AC_ERR_INVALID = (1 << 7), /* invalid argument */
+ AC_ERR_OTHER = (1 << 8), /* unknown */
};
/* forward declarations */
@@ -833,7 +838,7 @@ static inline int ata_try_flush_cache(co
static inline unsigned int ac_err_mask(u8 status)
{
if (status & ATA_BUSY)
- return AC_ERR_ATA_BUS;
+ return AC_ERR_HSM;
if (status & (ATA_ERR | ATA_DF))
return AC_ERR_DEV;
return 0;
--
1.0.8
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH 04/12] libata: add detailed AC_ERR_* flags
2006-01-22 7:58 ` [PATCH 04/12] libata: add detailed AC_ERR_* flags Tejun Heo
@ 2006-01-22 9:30 ` Jeff Garzik
2006-01-22 9:46 ` Tejun Heo
0 siblings, 1 reply; 28+ messages in thread
From: Jeff Garzik @ 2006-01-22 9:30 UTC (permalink / raw)
To: Tejun Heo; +Cc: linux-ide, albertcc
Tejun Heo wrote:
> Add detailed AC_ERR_* flags and use them. Long-term goal is to
> describe all errors with err_mask and tf combination (tf for failed
> sector information, etc...). After proper error diagnosis is
> implemented, sense data should also be generated from err_mask instead
> of directly from hardware tf registers as it is currently.
>
> Signed-off-by: Tejun Heo <htejun@gmail.com>
>
> ---
>
> drivers/scsi/ahci.c | 6 ++----
> drivers/scsi/libata-core.c | 12 ++++++------
> drivers/scsi/sata_mv.c | 2 +-
> drivers/scsi/sata_sil24.c | 2 +-
> include/linux/libata.h | 15 ++++++++++-----
> 5 files changed, 20 insertions(+), 17 deletions(-)
>
> 1939435a8f0e7b2b1578b84cedd49f8adcab1ea9
> diff --git a/drivers/scsi/ahci.c b/drivers/scsi/ahci.c
> index 30676b0..e7b937b 100644
> --- a/drivers/scsi/ahci.c
> +++ b/drivers/scsi/ahci.c
> @@ -680,7 +680,7 @@ static void ahci_eng_timeout(struct ata_
> * not being called from the SCSI EH.
> */
> qc->scsidone = scsi_finish_command;
> - qc->err_mask |= AC_ERR_OTHER;
> + qc->err_mask |= AC_ERR_TIMEOUT;
> ata_qc_complete(qc);
> }
>
> @@ -720,10 +720,8 @@ static inline int ahci_host_intr(struct
> /* command processing has stopped due to error; restart */
> ahci_restart_port(ap, status);
>
> - if (qc) {
> - qc->err_mask |= AC_ERR_OTHER;
> + if (qc)
> ata_qc_complete(qc);
> - }
why are you erasing the error mask here?
> diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
> index 43a2328..f5519f0 100644
> --- a/drivers/scsi/libata-core.c
> +++ b/drivers/scsi/libata-core.c
> @@ -1142,7 +1142,7 @@ ata_exec_internal(struct ata_port *ap, s
> * spurious interrupt. We can live with that.
> */
> if (qc->flags & ATA_QCFLAG_ACTIVE) {
> - qc->err_mask = AC_ERR_OTHER;
> + qc->err_mask = AC_ERR_TIMEOUT;
> ata_qc_complete(qc);
> printk(KERN_WARNING "ata%u: qc timeout (cmd 0x%x)\n",
> ap->id, command);
> @@ -2917,7 +2917,7 @@ static unsigned long ata_pio_poll(struct
> status = ata_chk_status(ap);
> if (status & ATA_BUSY) {
> if (time_after(jiffies, ap->pio_task_timeout)) {
> - qc->err_mask |= AC_ERR_ATA_BUS;
> + qc->err_mask |= AC_ERR_TIMEOUT;
> ap->hsm_task_state = HSM_ST_TMOUT;
> return 0;
> }
perhaps you want to set both flags? If BSY is stuck on, that's a
problem with the ATA bus.
> @@ -4159,14 +4159,14 @@ static void atapi_packet_task(void *_dat
> /* sleep-wait for BSY to clear */
> DPRINTK("busy wait\n");
> if (ata_busy_sleep(ap, ATA_TMOUT_CDB_QUICK, ATA_TMOUT_CDB)) {
> - qc->err_mask |= AC_ERR_ATA_BUS;
> + qc->err_mask |= AC_ERR_TIMEOUT;
> goto err_out;
> }
ditto
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH 04/12] libata: add detailed AC_ERR_* flags
2006-01-22 9:30 ` Jeff Garzik
@ 2006-01-22 9:46 ` Tejun Heo
2006-01-22 9:50 ` Tejun Heo
0 siblings, 1 reply; 28+ messages in thread
From: Tejun Heo @ 2006-01-22 9:46 UTC (permalink / raw)
To: Jeff Garzik; +Cc: linux-ide, albertcc
Jeff Garzik wrote:
> Tejun Heo wrote:
>
>> Add detailed AC_ERR_* flags and use them. Long-term goal is to
>> describe all errors with err_mask and tf combination (tf for failed
>> sector information, etc...). After proper error diagnosis is
>> implemented, sense data should also be generated from err_mask instead
>> of directly from hardware tf registers as it is currently.
>>
>> Signed-off-by: Tejun Heo <htejun@gmail.com>
>>
>> ---
>>
>> drivers/scsi/ahci.c | 6 ++----
>> drivers/scsi/libata-core.c | 12 ++++++------
>> drivers/scsi/sata_mv.c | 2 +-
>> drivers/scsi/sata_sil24.c | 2 +-
>> include/linux/libata.h | 15 ++++++++++-----
>> 5 files changed, 20 insertions(+), 17 deletions(-)
>>
>> 1939435a8f0e7b2b1578b84cedd49f8adcab1ea9
>> diff --git a/drivers/scsi/ahci.c b/drivers/scsi/ahci.c
>> index 30676b0..e7b937b 100644
>> --- a/drivers/scsi/ahci.c
>> +++ b/drivers/scsi/ahci.c
>> @@ -680,7 +680,7 @@ static void ahci_eng_timeout(struct ata_
>> * not being called from the SCSI EH.
>> */
>> qc->scsidone = scsi_finish_command;
>> - qc->err_mask |= AC_ERR_OTHER;
>> + qc->err_mask |= AC_ERR_TIMEOUT;
>> ata_qc_complete(qc);
>> }
>>
>> @@ -720,10 +720,8 @@ static inline int ahci_host_intr(struct
>> /* command processing has stopped due to error; restart */
>> ahci_restart_port(ap, status);
>>
>> - if (qc) {
>> - qc->err_mask |= AC_ERR_OTHER;
>> + if (qc)
>> ata_qc_complete(qc);
>> - }
>
>
> why are you erasing the error mask here?
>
Oh.. I forgot to write about it. About 10 lines above, detailed
err_mask is determined from irq status but qc->err_mask always used to
be set to AC_ERR_OTHER regardless of the determined value.
>
>> diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
>> index 43a2328..f5519f0 100644
>> --- a/drivers/scsi/libata-core.c
>> +++ b/drivers/scsi/libata-core.c
>> @@ -1142,7 +1142,7 @@ ata_exec_internal(struct ata_port *ap, s
>> * spurious interrupt. We can live with that.
>> */
>> if (qc->flags & ATA_QCFLAG_ACTIVE) {
>> - qc->err_mask = AC_ERR_OTHER;
>> + qc->err_mask = AC_ERR_TIMEOUT;
>> ata_qc_complete(qc);
>> printk(KERN_WARNING "ata%u: qc timeout (cmd 0x%x)\n",
>> ap->id, command);
>> @@ -2917,7 +2917,7 @@ static unsigned long ata_pio_poll(struct
>> status = ata_chk_status(ap);
>> if (status & ATA_BUSY) {
>> if (time_after(jiffies, ap->pio_task_timeout)) {
>> - qc->err_mask |= AC_ERR_ATA_BUS;
>> + qc->err_mask |= AC_ERR_TIMEOUT;
>> ap->hsm_task_state = HSM_ST_TMOUT;
>> return 0;
>> }
>
>
> perhaps you want to set both flags? If BSY is stuck on, that's a
> problem with the ATA bus.
>
Two things.
1. My intention was to use AC_ERR_ATA_BUS solely for recognized
transport errors (ICRC, transport error bits in SError...) such that
it's more useful for determining whether lowering transport speed is
necessary or not. For example, in my working tree, three ATA_BUS errors
or ten HSM/DEV errors for known supported commands (SCSI rw commands)
during last 15 mins trigger speed down.
2. For SATA, if it's actually an ATA_BUS error, later SError analysis
should turn on ATA_BUS error. I'm not sure about PATA. Maybe always
turning on ATA_BUS with HSM is a good idea for PATA. Anyways, I think
this deeper analysis is better done during EH.
--
tejun
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH 04/12] libata: add detailed AC_ERR_* flags
2006-01-22 9:46 ` Tejun Heo
@ 2006-01-22 9:50 ` Tejun Heo
0 siblings, 0 replies; 28+ messages in thread
From: Tejun Heo @ 2006-01-22 9:50 UTC (permalink / raw)
To: Tejun Heo; +Cc: Jeff Garzik, linux-ide, albertcc
Tejun Heo wrote:
> Jeff Garzik wrote:
>
>> Tejun Heo wrote:
>>
>>> Add detailed AC_ERR_* flags and use them. Long-term goal is to
>>> describe all errors with err_mask and tf combination (tf for failed
>>> sector information, etc...). After proper error diagnosis is
>>> implemented, sense data should also be generated from err_mask instead
>>> of directly from hardware tf registers as it is currently.
>>>
>>> Signed-off-by: Tejun Heo <htejun@gmail.com>
>>>
>>> ---
>>>
>>> drivers/scsi/ahci.c | 6 ++----
>>> drivers/scsi/libata-core.c | 12 ++++++------
>>> drivers/scsi/sata_mv.c | 2 +-
>>> drivers/scsi/sata_sil24.c | 2 +-
>>> include/linux/libata.h | 15 ++++++++++-----
>>> 5 files changed, 20 insertions(+), 17 deletions(-)
>>>
>>> 1939435a8f0e7b2b1578b84cedd49f8adcab1ea9
>>> diff --git a/drivers/scsi/ahci.c b/drivers/scsi/ahci.c
>>> index 30676b0..e7b937b 100644
>>> --- a/drivers/scsi/ahci.c
>>> +++ b/drivers/scsi/ahci.c
>>> @@ -680,7 +680,7 @@ static void ahci_eng_timeout(struct ata_
>>> * not being called from the SCSI EH.
>>> */
>>> qc->scsidone = scsi_finish_command;
>>> - qc->err_mask |= AC_ERR_OTHER;
>>> + qc->err_mask |= AC_ERR_TIMEOUT;
>>> ata_qc_complete(qc);
>>> }
>>>
>>> @@ -720,10 +720,8 @@ static inline int ahci_host_intr(struct
>>> /* command processing has stopped due to error; restart */
>>> ahci_restart_port(ap, status);
>>>
>>> - if (qc) {
>>> - qc->err_mask |= AC_ERR_OTHER;
>>> + if (qc)
>>> ata_qc_complete(qc);
>>> - }
>>
>>
>>
>> why are you erasing the error mask here?
>>
>
> Oh.. I forgot to write about it. About 10 lines above, detailed
> err_mask is determined from irq status but qc->err_mask always used to
> be set to AC_ERR_OTHER regardless of the determined value.
>
And, yeah, I forgot to do qc->err_mask |= err_mask. :-P
--
tejun
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 06/12] SCSI: export scsi_eh_finish_cmd() and scsi_eh_flush_done_q()
2006-01-22 7:58 [PATCHSET] libata: various fixes related to EH Tejun Heo
2006-01-22 7:58 ` [PATCH 05/12] libata: return AC_ERR_* from issue functions Tejun Heo
2006-01-22 7:58 ` [PATCH 04/12] libata: add detailed AC_ERR_* flags Tejun Heo
@ 2006-01-22 7:58 ` Tejun Heo
2006-01-22 9:36 ` Jeff Garzik
2006-01-22 7:58 ` [PATCH 03/12] libata: fix ata_qc_issue() error handling Tejun Heo
` (9 subsequent siblings)
12 siblings, 1 reply; 28+ messages in thread
From: Tejun Heo @ 2006-01-22 7:58 UTC (permalink / raw)
To: jgarzik, linux-ide, albertcc; +Cc: Tejun Heo
Export two SCSI EH command handling functions. To be used by libata EH.
Signed-off-by: Tejun Heo <htejun@gmail.com>
---
drivers/scsi/scsi_error.c | 7 ++++---
include/scsi/scsi_eh.h | 3 +++
2 files changed, 7 insertions(+), 3 deletions(-)
089544e4124c0a027694e7234f3b3cd1a5103d55
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index a2333d2..6bac3d2 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -584,8 +584,7 @@ static int scsi_request_sense(struct scs
* keep a list of pending commands for final completion, and once we
* are ready to leave error handling we handle completion for real.
**/
-static void scsi_eh_finish_cmd(struct scsi_cmnd *scmd,
- struct list_head *done_q)
+void scsi_eh_finish_cmd(struct scsi_cmnd *scmd, struct list_head *done_q)
{
scmd->device->host->host_failed--;
scmd->eh_eflags = 0;
@@ -597,6 +596,7 @@ static void scsi_eh_finish_cmd(struct sc
scsi_setup_cmd_retry(scmd);
list_move_tail(&scmd->eh_entry, done_q);
}
+EXPORT_SYMBOL(scsi_eh_finish_cmd);
/**
* scsi_eh_get_sense - Get device sense data.
@@ -1425,7 +1425,7 @@ static void scsi_eh_ready_devs(struct Sc
* @done_q: list_head of processed commands.
*
**/
-static void scsi_eh_flush_done_q(struct list_head *done_q)
+void scsi_eh_flush_done_q(struct list_head *done_q)
{
struct scsi_cmnd *scmd, *next;
@@ -1454,6 +1454,7 @@ static void scsi_eh_flush_done_q(struct
}
}
}
+EXPORT_SYMBOL(scsi_eh_flush_done_q);
/**
* scsi_unjam_host - Attempt to fix a host which has a cmd that failed.
diff --git a/include/scsi/scsi_eh.h b/include/scsi/scsi_eh.h
index fabd879..d160880 100644
--- a/include/scsi/scsi_eh.h
+++ b/include/scsi/scsi_eh.h
@@ -35,6 +35,9 @@ static inline int scsi_sense_valid(struc
}
+extern void scsi_eh_finish_cmd(struct scsi_cmnd *scmd,
+ struct list_head *done_q);
+extern void scsi_eh_flush_done_q(struct list_head *done_q);
extern void scsi_report_bus_reset(struct Scsi_Host *, int);
extern void scsi_report_device_reset(struct Scsi_Host *, int, int);
extern int scsi_block_when_processing_errors(struct scsi_device *);
--
1.0.8
^ permalink raw reply related [flat|nested] 28+ messages in thread* [PATCH 03/12] libata: fix ata_qc_issue() error handling
2006-01-22 7:58 [PATCHSET] libata: various fixes related to EH Tejun Heo
` (2 preceding siblings ...)
2006-01-22 7:58 ` [PATCH 06/12] SCSI: export scsi_eh_finish_cmd() and scsi_eh_flush_done_q() Tejun Heo
@ 2006-01-22 7:58 ` Tejun Heo
2006-01-22 9:25 ` Jeff Garzik
2006-01-22 7:58 ` [PATCH 02/12] libata: make the owner of a qc responsible for freeing it Tejun Heo
` (8 subsequent siblings)
12 siblings, 1 reply; 28+ messages in thread
From: Tejun Heo @ 2006-01-22 7:58 UTC (permalink / raw)
To: jgarzik, linux-ide, albertcc; +Cc: Tejun Heo
When ata_qc_issue() fails, the qc might have been dma mapped or not.
So, performing only ata_qc_free() results in dma map leak. This patch
makes ata_qc_issue() mark dma map flags correctly on failure and calls
ata_qc_complete() after ata_qc_issue() fails.
Signed-off-by: Tejun Heo <htejun@gmail.com>
---
drivers/scsi/libata-core.c | 18 ++++++++----------
drivers/scsi/libata-scsi.c | 6 ++++--
2 files changed, 12 insertions(+), 12 deletions(-)
1a74b7390a28190a7031e7fc444be5792c82d256
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index 15df633..43a2328 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -1125,8 +1125,10 @@ ata_exec_internal(struct ata_port *ap, s
qc->private_data = &wait;
qc->complete_fn = ata_qc_complete_internal;
- if (ata_qc_issue(qc))
- goto issue_fail;
+ if (ata_qc_issue(qc)) {
+ qc->err_mask = AC_ERR_OTHER;
+ ata_qc_complete(qc);
+ }
spin_unlock_irqrestore(&ap->host_set->lock, flags);
@@ -1155,11 +1157,6 @@ ata_exec_internal(struct ata_port *ap, s
ata_qc_free(qc);
return err_mask;
-
- issue_fail:
- ata_qc_free(qc);
- spin_unlock_irqrestore(&ap->host_set->lock, flags);
- return AC_ERR_OTHER;
}
/**
@@ -3687,10 +3684,10 @@ int ata_qc_issue(struct ata_queued_cmd *
if (ata_should_dma_map(qc)) {
if (qc->flags & ATA_QCFLAG_SG) {
if (ata_sg_setup(qc))
- goto err_out;
+ goto sg_err;
} else if (qc->flags & ATA_QCFLAG_SINGLE) {
if (ata_sg_setup_one(qc))
- goto err_out;
+ goto sg_err;
}
} else {
qc->flags &= ~ATA_QCFLAG_DMAMAP;
@@ -3703,7 +3700,8 @@ int ata_qc_issue(struct ata_queued_cmd *
return ap->ops->qc_issue(qc);
-err_out:
+sg_err:
+ qc->flags &= ~ATA_QCFLAG_DMAMAP;
return -1;
}
diff --git a/drivers/scsi/libata-scsi.c b/drivers/scsi/libata-scsi.c
index ce3fe92..c496309 100644
--- a/drivers/scsi/libata-scsi.c
+++ b/drivers/scsi/libata-scsi.c
@@ -1322,8 +1322,10 @@ static void ata_scsi_translate(struct at
goto early_finish;
/* select device, send command to hardware */
- if (ata_qc_issue(qc))
- goto err_did;
+ if (ata_qc_issue(qc)) {
+ qc->err_mask |= AC_ERR_OTHER;
+ ata_qc_complete(qc);
+ }
VPRINTK("EXIT\n");
return;
--
1.0.8
^ permalink raw reply related [flat|nested] 28+ messages in thread* [PATCH 02/12] libata: make the owner of a qc responsible for freeing it
2006-01-22 7:58 [PATCHSET] libata: various fixes related to EH Tejun Heo
` (3 preceding siblings ...)
2006-01-22 7:58 ` [PATCH 03/12] libata: fix ata_qc_issue() error handling Tejun Heo
@ 2006-01-22 7:58 ` Tejun Heo
2006-01-22 9:37 ` Jeff Garzik
2006-01-22 7:58 ` [PATCH 01/12] libata: fold __ata_qc_complete() into ata_qc_free() Tejun Heo
` (7 subsequent siblings)
12 siblings, 1 reply; 28+ messages in thread
From: Tejun Heo @ 2006-01-22 7:58 UTC (permalink / raw)
To: jgarzik, linux-ide, albertcc; +Cc: Tejun Heo
qc used to be freed automatically on command completion. However, as
a qc can carry information about its completion status, it can be
useful to its owner/issuer after command completion. This patch makes
freeing qc responsibility of its owner. This simplifies
ata_exec_internal() and makes command turn-around for atapi request
sensing less hackish.
This change was originally suggested by Jeff Garzik.
Signed-off-by: Tejun Heo <htejun@gmail.com>
---
drivers/scsi/libata-core.c | 47 ++++++++++++--------------------------------
drivers/scsi/libata-scsi.c | 14 +++++++------
include/linux/libata.h | 2 +-
3 files changed, 21 insertions(+), 42 deletions(-)
576c63b9e5ffef8fac2a379b639fbe198fd150ac
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index 0e49323..15df633 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -1072,24 +1072,12 @@ static unsigned int ata_pio_modes(const
timing API will get this right anyway */
}
-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)
+void ata_qc_complete_internal(struct ata_queued_cmd *qc)
{
- struct ata_exec_internal_arg *arg = qc->private_data;
- struct completion *waiting = arg->waiting;
+ struct completion *waiting = qc->private_data;
- 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;
+ qc->ap->ops->tf_read(qc->ap, &qc->tf);
complete(waiting);
-
- return 0;
}
/**
@@ -1120,7 +1108,7 @@ ata_exec_internal(struct ata_port *ap, s
struct ata_queued_cmd *qc;
DECLARE_COMPLETION(wait);
unsigned long flags;
- struct ata_exec_internal_arg arg;
+ unsigned int err_mask;
spin_lock_irqsave(&ap->host_set->lock, flags);
@@ -1134,9 +1122,7 @@ ata_exec_internal(struct ata_port *ap, s
qc->nsect = buflen / ATA_SECT_SIZE;
}
- arg.waiting = &wait;
- arg.tf = tf;
- qc->private_data = &arg;
+ qc->private_data = &wait;
qc->complete_fn = ata_qc_complete_internal;
if (ata_qc_issue(qc))
@@ -1153,7 +1139,7 @@ ata_exec_internal(struct ata_port *ap, s
* before the caller cleans up, it will result in a
* spurious interrupt. We can live with that.
*/
- if (arg.waiting) {
+ if (qc->flags & ATA_QCFLAG_ACTIVE) {
qc->err_mask = AC_ERR_OTHER;
ata_qc_complete(qc);
printk(KERN_WARNING "ata%u: qc timeout (cmd 0x%x)\n",
@@ -1163,7 +1149,12 @@ ata_exec_internal(struct ata_port *ap, s
spin_unlock_irqrestore(&ap->host_set->lock, flags);
}
- return arg.err_mask;
+ *tf = qc->tf;
+ err_mask = qc->err_mask;
+
+ ata_qc_free(qc);
+
+ return err_mask;
issue_fail:
ata_qc_free(qc);
@@ -3633,8 +3624,6 @@ void ata_qc_free(struct ata_queued_cmd *
void ata_qc_complete(struct ata_queued_cmd *qc)
{
- int rc;
-
assert(qc != NULL); /* ata_qc_from_tag _might_ return NULL */
assert(qc->flags & ATA_QCFLAG_ACTIVE);
@@ -3648,17 +3637,7 @@ void ata_qc_complete(struct ata_queued_c
qc->flags &= ~ATA_QCFLAG_ACTIVE;
/* call completion callback */
- rc = qc->complete_fn(qc);
-
- /* if callback indicates not to complete command (non-zero),
- * return immediately
- */
- if (rc != 0)
- return;
-
- ata_qc_free(qc);
-
- VPRINTK("EXIT\n");
+ qc->complete_fn(qc);
}
static inline int ata_should_dma_map(struct ata_queued_cmd *qc)
diff --git a/drivers/scsi/libata-scsi.c b/drivers/scsi/libata-scsi.c
index 0e65bfe..ce3fe92 100644
--- a/drivers/scsi/libata-scsi.c
+++ b/drivers/scsi/libata-scsi.c
@@ -1219,7 +1219,7 @@ nothing_to_do:
return 1;
}
-static int ata_scsi_qc_complete(struct ata_queued_cmd *qc)
+static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
{
struct scsi_cmnd *cmd = qc->scsicmd;
u8 *cdb = cmd->cmnd;
@@ -1256,7 +1256,7 @@ static int ata_scsi_qc_complete(struct a
qc->scsidone(cmd);
- return 0;
+ ata_qc_free(qc);
}
/**
@@ -1982,7 +1982,7 @@ void ata_scsi_badcmd(struct scsi_cmnd *c
done(cmd);
}
-static int atapi_sense_complete(struct ata_queued_cmd *qc)
+static void atapi_sense_complete(struct ata_queued_cmd *qc)
{
if (qc->err_mask && ((qc->err_mask & AC_ERR_DEV) == 0))
/* FIXME: not quite right; we don't want the
@@ -1993,7 +1993,7 @@ static int atapi_sense_complete(struct a
ata_gen_ata_desc_sense(qc);
qc->scsidone(qc->scsicmd);
- return 0;
+ ata_qc_free(qc);
}
/* is it pointless to prefer PIO for "safety reasons"? */
@@ -2050,7 +2050,7 @@ static void atapi_request_sense(struct a
DPRINTK("EXIT\n");
}
-static int atapi_qc_complete(struct ata_queued_cmd *qc)
+static void atapi_qc_complete(struct ata_queued_cmd *qc)
{
struct scsi_cmnd *cmd = qc->scsicmd;
unsigned int err_mask = qc->err_mask;
@@ -2060,7 +2060,7 @@ static int atapi_qc_complete(struct ata_
if (unlikely(err_mask & AC_ERR_DEV)) {
cmd->result = SAM_STAT_CHECK_CONDITION;
atapi_request_sense(qc);
- return 1;
+ return;
}
else if (unlikely(err_mask))
@@ -2100,7 +2100,7 @@ static int atapi_qc_complete(struct ata_
}
qc->scsidone(cmd);
- return 0;
+ ata_qc_free(qc);
}
/**
* atapi_xlat - Initialize PACKET taskfile
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 46ccea2..d58b659 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -235,7 +235,7 @@ struct ata_port;
struct ata_queued_cmd;
/* typedefs */
-typedef int (*ata_qc_cb_t) (struct ata_queued_cmd *qc);
+typedef void (*ata_qc_cb_t) (struct ata_queued_cmd *qc);
struct ata_ioports {
unsigned long cmd_addr;
--
1.0.8
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH 02/12] libata: make the owner of a qc responsible for freeing it
2006-01-22 7:58 ` [PATCH 02/12] libata: make the owner of a qc responsible for freeing it Tejun Heo
@ 2006-01-22 9:37 ` Jeff Garzik
2006-01-22 10:16 ` Tejun Heo
0 siblings, 1 reply; 28+ messages in thread
From: Jeff Garzik @ 2006-01-22 9:37 UTC (permalink / raw)
To: Tejun Heo; +Cc: linux-ide, albertcc
Tejun Heo wrote:
> --- a/drivers/scsi/libata-scsi.c
> +++ b/drivers/scsi/libata-scsi.c
> @@ -1219,7 +1219,7 @@ nothing_to_do:
> return 1;
> }
>
> -static int ata_scsi_qc_complete(struct ata_queued_cmd *qc)
> +static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
> {
> struct scsi_cmnd *cmd = qc->scsicmd;
> u8 *cdb = cmd->cmnd;
> @@ -1256,7 +1256,7 @@ static int ata_scsi_qc_complete(struct a
>
> qc->scsidone(cmd);
>
> - return 0;
> + ata_qc_free(qc);
> }
>
> /**
> @@ -1982,7 +1982,7 @@ void ata_scsi_badcmd(struct scsi_cmnd *c
> done(cmd);
> }
>
> -static int atapi_sense_complete(struct ata_queued_cmd *qc)
> +static void atapi_sense_complete(struct ata_queued_cmd *qc)
> {
> if (qc->err_mask && ((qc->err_mask & AC_ERR_DEV) == 0))
> /* FIXME: not quite right; we don't want the
> @@ -1993,7 +1993,7 @@ static int atapi_sense_complete(struct a
> ata_gen_ata_desc_sense(qc);
>
> qc->scsidone(qc->scsicmd);
> - return 0;
> + ata_qc_free(qc);
> }
>
> /* is it pointless to prefer PIO for "safety reasons"? */
> @@ -2050,7 +2050,7 @@ static void atapi_request_sense(struct a
> DPRINTK("EXIT\n");
> }
>
> -static int atapi_qc_complete(struct ata_queued_cmd *qc)
> +static void atapi_qc_complete(struct ata_queued_cmd *qc)
> {
> struct scsi_cmnd *cmd = qc->scsicmd;
> unsigned int err_mask = qc->err_mask;
> @@ -2060,7 +2060,7 @@ static int atapi_qc_complete(struct ata_
> if (unlikely(err_mask & AC_ERR_DEV)) {
> cmd->result = SAM_STAT_CHECK_CONDITION;
> atapi_request_sense(qc);
> - return 1;
> + return;
> }
>
> else if (unlikely(err_mask))
> @@ -2100,7 +2100,7 @@ static int atapi_qc_complete(struct ata_
> }
>
> qc->scsidone(cmd);
> - return 0;
> + ata_qc_free(qc);
Can you describe the ATAPI error handling flow, after applying these two
changes (patch #1 and this one, patch #2)?
Jeff
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH 02/12] libata: make the owner of a qc responsible for freeing it
2006-01-22 9:37 ` Jeff Garzik
@ 2006-01-22 10:16 ` Tejun Heo
0 siblings, 0 replies; 28+ messages in thread
From: Tejun Heo @ 2006-01-22 10:16 UTC (permalink / raw)
To: Jeff Garzik; +Cc: linux-ide, albertcc
Jeff Garzik wrote:
> Tejun Heo wrote:
>
>> --- a/drivers/scsi/libata-scsi.c
>> +++ b/drivers/scsi/libata-scsi.c
>> @@ -1219,7 +1219,7 @@ nothing_to_do:
>> return 1;
>> }
>>
>> -static int ata_scsi_qc_complete(struct ata_queued_cmd *qc)
>> +static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
>> {
>> struct scsi_cmnd *cmd = qc->scsicmd;
>> u8 *cdb = cmd->cmnd;
>> @@ -1256,7 +1256,7 @@ static int ata_scsi_qc_complete(struct a
>>
>> qc->scsidone(cmd);
>>
>> - return 0;
>> + ata_qc_free(qc);
>> }
>>
>> /**
>> @@ -1982,7 +1982,7 @@ void ata_scsi_badcmd(struct scsi_cmnd *c
>> done(cmd);
>> }
>>
>> -static int atapi_sense_complete(struct ata_queued_cmd *qc)
>> +static void atapi_sense_complete(struct ata_queued_cmd *qc)
>> {
>> if (qc->err_mask && ((qc->err_mask & AC_ERR_DEV) == 0))
>> /* FIXME: not quite right; we don't want the
>> @@ -1993,7 +1993,7 @@ static int atapi_sense_complete(struct a
>> ata_gen_ata_desc_sense(qc);
>>
>> qc->scsidone(qc->scsicmd);
>> - return 0;
>> + ata_qc_free(qc);
>> }
>>
>> /* is it pointless to prefer PIO for "safety reasons"? */
>> @@ -2050,7 +2050,7 @@ static void atapi_request_sense(struct a
>> DPRINTK("EXIT\n");
>> }
>>
>> -static int atapi_qc_complete(struct ata_queued_cmd *qc)
>> +static void atapi_qc_complete(struct ata_queued_cmd *qc)
>> {
>> struct scsi_cmnd *cmd = qc->scsicmd;
>> unsigned int err_mask = qc->err_mask;
>> @@ -2060,7 +2060,7 @@ static int atapi_qc_complete(struct ata_
>> if (unlikely(err_mask & AC_ERR_DEV)) {
>> cmd->result = SAM_STAT_CHECK_CONDITION;
>> atapi_request_sense(qc);
>> - return 1;
>> + return;
>> }
>>
>> else if (unlikely(err_mask))
>> @@ -2100,7 +2100,7 @@ static int atapi_qc_complete(struct ata_
>> }
>>
>> qc->scsidone(cmd);
>> - return 0;
>> + ata_qc_free(qc);
>
>
>
> Can you describe the ATAPI error handling flow, after applying these two
> changes (patch #1 and this one, patch #2)?
>
Sure. Except for where ata_qc_free() is called, it isn't different from
the original.
A. Before change.
1. ATAPI qc completes with ERR_DEV.
2. ata_qc_complete calls atapi_qc_complete which in turn sees AC_ERR_DEV
and calls atapi_request_sense.
3. atapi_request_sense re-init qc and issues it again for REQUEST SENSE
4. atapi_qc_complete returns 1 to instruct ata_qc_complete to not call
ata_qc_free().
5. REQUEST_SENSE completes. ata_qc_complete is called and it calls
atapi_sense_complete() which notifies SCSI layer and returns 0.
6. ata_qc_complete sees return value 0 and calls ata_qc_free().
B. After change.
1. ATAPI qc completes with ERR_DEV.
2. ata_qc_complete calls atapi_qc_complete which in turn sees AC_ERR_DEV
and calls atapi_request_sense.
3. atapi_request_sense re-init qc and issues it again for REQUEST SENSE.
4. atapi_qc_complete returns without calling ata_qc_free.
ata_qc_complete finishes. qc isn't freed and still in-flight.
5. REQUEST_SENSE completes. ata_qc_complete is called and it calls
atapi_sense_complete() which notifies SCSI layer, free's the qc and returns.
6. ata_qc_complete returns.
--
tejun
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 01/12] libata: fold __ata_qc_complete() into ata_qc_free()
2006-01-22 7:58 [PATCHSET] libata: various fixes related to EH Tejun Heo
` (4 preceding siblings ...)
2006-01-22 7:58 ` [PATCH 02/12] libata: make the owner of a qc responsible for freeing it Tejun Heo
@ 2006-01-22 7:58 ` Tejun Heo
2006-01-22 7:58 ` [PATCH 10/12] libata: implement ATA_FLAG_IN_EH port flag Tejun Heo
` (6 subsequent siblings)
12 siblings, 0 replies; 28+ messages in thread
From: Tejun Heo @ 2006-01-22 7:58 UTC (permalink / raw)
To: jgarzik, linux-ide, albertcc; +Cc: Tejun Heo
All ata_qc_free() does is calling __ata_qc_complete() which isn't used
anywhere else. Fold __ata_qc_complete() into ata_qc_free().
Signed-off-by: Tejun Heo <htejun@gmail.com>
---
drivers/scsi/libata-core.c | 30 ++++++++++++------------------
1 files changed, 12 insertions(+), 18 deletions(-)
7474f1f37d6bcfa451b24e9a6905b56a1bd45a29
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index a07bd35..0e49323 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -73,7 +73,6 @@ static int fgb(u32 bitmap);
static int ata_choose_xfer_mode(const struct ata_port *ap,
u8 *xfer_mode_out,
unsigned int *xfer_shift_out);
-static void __ata_qc_complete(struct ata_queued_cmd *qc);
static unsigned int ata_unique_id = 1;
static struct workqueue_struct *ata_wq;
@@ -3593,21 +3592,6 @@ struct ata_queued_cmd *ata_qc_new_init(s
return qc;
}
-static void __ata_qc_complete(struct ata_queued_cmd *qc)
-{
- struct ata_port *ap = qc->ap;
- unsigned int tag;
-
- qc->flags = 0;
- tag = qc->tag;
- if (likely(ata_tag_valid(tag))) {
- if (tag == ap->active_tag)
- ap->active_tag = ATA_TAG_POISON;
- qc->tag = ATA_TAG_POISON;
- clear_bit(tag, &ap->qactive);
- }
-}
-
/**
* ata_qc_free - free unused ata_queued_cmd
* @qc: Command to complete
@@ -3620,9 +3604,19 @@ static void __ata_qc_complete(struct ata
*/
void ata_qc_free(struct ata_queued_cmd *qc)
{
+ struct ata_port *ap = qc->ap;
+ unsigned int tag;
+
assert(qc != NULL); /* ata_qc_from_tag _might_ return NULL */
- __ata_qc_complete(qc);
+ qc->flags = 0;
+ tag = qc->tag;
+ if (likely(ata_tag_valid(tag))) {
+ if (tag == ap->active_tag)
+ ap->active_tag = ATA_TAG_POISON;
+ qc->tag = ATA_TAG_POISON;
+ clear_bit(tag, &ap->qactive);
+ }
}
/**
@@ -3662,7 +3656,7 @@ void ata_qc_complete(struct ata_queued_c
if (rc != 0)
return;
- __ata_qc_complete(qc);
+ ata_qc_free(qc);
VPRINTK("EXIT\n");
}
--
1.0.8
^ permalink raw reply related [flat|nested] 28+ messages in thread* [PATCH 10/12] libata: implement ATA_FLAG_IN_EH port flag
2006-01-22 7:58 [PATCHSET] libata: various fixes related to EH Tejun Heo
` (5 preceding siblings ...)
2006-01-22 7:58 ` [PATCH 01/12] libata: fold __ata_qc_complete() into ata_qc_free() Tejun Heo
@ 2006-01-22 7:58 ` Tejun Heo
2006-01-22 9:49 ` Jeff Garzik
2006-01-22 7:58 ` [PATCH 11/12] libata: ignore normal qc completion during EH Tejun Heo
` (5 subsequent siblings)
12 siblings, 1 reply; 28+ messages in thread
From: Tejun Heo @ 2006-01-22 7:58 UTC (permalink / raw)
To: jgarzik, linux-ide, albertcc; +Cc: Tejun Heo
ATA_FLAG_IN_EH flag is set on entry to EH and cleared on completion.
This patch just sets and clears the flag. Following patches will
build normal qc execution / EH synchronization aroung this flag.
Signed-off-by: Tejun Heo <htejun@gmail.com>
---
drivers/scsi/libata-scsi.c | 6 ++++++
include/linux/libata.h | 2 ++
2 files changed, 8 insertions(+), 0 deletions(-)
cdc39a8dc6c08611e6317f4dcad2abaa5e027525
diff --git a/drivers/scsi/libata-scsi.c b/drivers/scsi/libata-scsi.c
index accb63a..cf54536 100644
--- a/drivers/scsi/libata-scsi.c
+++ b/drivers/scsi/libata-scsi.c
@@ -738,6 +738,8 @@ int ata_scsi_error(struct Scsi_Host *hos
DPRINTK("ENTER\n");
spin_lock_irqsave(&ap->host_set->lock, flags);
+ assert(!(ap->flags & ATA_FLAG_IN_EH));
+ ap->flags |= ATA_FLAG_IN_EH;
qc = ata_qc_from_tag(ap, ap->active_tag);
spin_unlock_irqrestore(&ap->host_set->lock, flags);
@@ -778,6 +780,10 @@ int ata_scsi_error(struct Scsi_Host *hos
scsi_eh_flush_done_q(&ap->eh_done_q);
+ spin_lock_irqsave(&ap->host_set->lock, flags);
+ ap->flags &= ~ATA_FLAG_IN_EH;
+ spin_unlock_irqrestore(&ap->host_set->lock, flags);
+
DPRINTK("EXIT\n");
return 0;
}
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 576788d..063221d 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -162,6 +162,8 @@ enum {
ATA_FLAG_PIO_LBA48 = (1 << 13), /* Host DMA engine is LBA28 only */
ATA_FLAG_IRQ_MASK = (1 << 14), /* Mask IRQ in PIO xfers */
+ ATA_FLAG_IN_EH = (1 << 15), /* EH in progress */
+
ATA_QCFLAG_ACTIVE = (1 << 1), /* cmd not yet ack'd to scsi lyer */
ATA_QCFLAG_SG = (1 << 3), /* have s/g table? */
ATA_QCFLAG_SINGLE = (1 << 4), /* no s/g, just a single buffer */
--
1.0.8
^ permalink raw reply related [flat|nested] 28+ messages in thread* [PATCH 11/12] libata: ignore normal qc completion during EH
2006-01-22 7:58 [PATCHSET] libata: various fixes related to EH Tejun Heo
` (6 preceding siblings ...)
2006-01-22 7:58 ` [PATCH 10/12] libata: implement ATA_FLAG_IN_EH port flag Tejun Heo
@ 2006-01-22 7:58 ` Tejun Heo
2006-01-22 9:53 ` Jeff Garzik
2006-01-22 7:58 ` [PATCH 12/12] libata: EH / pio tasks synchronization Tejun Heo
` (4 subsequent siblings)
12 siblings, 1 reply; 28+ messages in thread
From: Tejun Heo @ 2006-01-22 7:58 UTC (permalink / raw)
To: jgarzik, linux-ide, albertcc; +Cc: Tejun Heo
Don't let interrupt handler and pio task complete qc's while EH is in
progress. This makes sure that qc's don't go away underneath EH.
Signed-off-by: Tejun Heo <htejun@gmail.com>
---
drivers/scsi/libata-core.c | 32 +++++++++++++++++++-------------
drivers/scsi/libata-scsi.c | 2 +-
drivers/scsi/libata.h | 1 +
3 files changed, 21 insertions(+), 14 deletions(-)
c72a043f83d46dd4b90a9f689beb5ed9b135fc3a
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index b2f51bb..9556100 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -3589,19 +3589,7 @@ void ata_qc_free(struct ata_queued_cmd *
}
}
-/**
- * ata_qc_complete - Complete an active ATA command
- * @qc: Command to complete
- * @err_mask: ATA Status register contents
- *
- * Indicate to the mid and upper layers that an ATA
- * command has completed, with either an ok or not-ok status.
- *
- * LOCKING:
- * spin_lock_irqsave(host_set lock)
- */
-
-void ata_qc_complete(struct ata_queued_cmd *qc)
+void __ata_qc_complete(struct ata_queued_cmd *qc)
{
assert(qc != NULL); /* ata_qc_from_tag _might_ return NULL */
assert(qc->flags & ATA_QCFLAG_ACTIVE);
@@ -3619,6 +3607,24 @@ void ata_qc_complete(struct ata_queued_c
qc->complete_fn(qc);
}
+/**
+ * ata_qc_complete - Complete an active ATA command
+ * @qc: Command to complete
+ * @err_mask: ATA Status register contents
+ *
+ * Indicate to the mid and upper layers that an ATA
+ * command has completed, with either an ok or not-ok status.
+ *
+ * LOCKING:
+ * spin_lock_irqsave(host_set lock)
+ */
+
+void ata_qc_complete(struct ata_queued_cmd *qc)
+{
+ if (!(qc->ap->flags & ATA_FLAG_IN_EH))
+ __ata_qc_complete(qc);
+}
+
static inline int ata_should_dma_map(struct ata_queued_cmd *qc)
{
struct ata_port *ap = qc->ap;
diff --git a/drivers/scsi/libata-scsi.c b/drivers/scsi/libata-scsi.c
index cf54536..9e4bc3c 100644
--- a/drivers/scsi/libata-scsi.c
+++ b/drivers/scsi/libata-scsi.c
@@ -801,7 +801,7 @@ static void __ata_eh_qc_complete(struct
spin_lock_irqsave(&ap->host_set->lock, flags);
qc->scsidone = ata_eh_scsidone;
- ata_qc_complete(qc);
+ __ata_qc_complete(qc);
assert(!ata_tag_valid(qc->tag));
spin_unlock_irqrestore(&ap->host_set->lock, flags);
diff --git a/drivers/scsi/libata.h b/drivers/scsi/libata.h
index 9d76923..1cd071a 100644
--- a/drivers/scsi/libata.h
+++ b/drivers/scsi/libata.h
@@ -46,6 +46,7 @@ extern struct ata_queued_cmd *ata_qc_new
extern int ata_rwcmd_protocol(struct ata_queued_cmd *qc);
extern void ata_qc_free(struct ata_queued_cmd *qc);
extern unsigned int ata_qc_issue(struct ata_queued_cmd *qc);
+extern void __ata_qc_complete(struct ata_queued_cmd *qc);
extern int ata_check_atapi_dma(struct ata_queued_cmd *qc);
extern void ata_dev_select(struct ata_port *ap, unsigned int device,
unsigned int wait, unsigned int can_sleep);
--
1.0.8
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH 11/12] libata: ignore normal qc completion during EH
2006-01-22 7:58 ` [PATCH 11/12] libata: ignore normal qc completion during EH Tejun Heo
@ 2006-01-22 9:53 ` Jeff Garzik
2006-01-22 11:09 ` Tejun Heo
0 siblings, 1 reply; 28+ messages in thread
From: Jeff Garzik @ 2006-01-22 9:53 UTC (permalink / raw)
To: Tejun Heo; +Cc: linux-ide, albertcc
Tejun Heo wrote:
> Don't let interrupt handler and pio task complete qc's while EH is in
> progress. This makes sure that qc's don't go away underneath EH.
>
> Signed-off-by: Tejun Heo <htejun@gmail.com>
I haven't done an in-depth analysis, but this feels like the wrong
approach: you've still got drivers writing to qc->err_mask, and
potentially other struct members.
The better solution is most likely to examine the interrupt handling
paths of each driver...
Jeff
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 11/12] libata: ignore normal qc completion during EH
2006-01-22 9:53 ` Jeff Garzik
@ 2006-01-22 11:09 ` Tejun Heo
0 siblings, 0 replies; 28+ messages in thread
From: Tejun Heo @ 2006-01-22 11:09 UTC (permalink / raw)
To: Jeff Garzik; +Cc: linux-ide, albertcc
Jeff Garzik wrote:
> Tejun Heo wrote:
>
>> Don't let interrupt handler and pio task complete qc's while EH is in
>> progress. This makes sure that qc's don't go away underneath EH.
>>
>> Signed-off-by: Tejun Heo <htejun@gmail.com>
>
>
> I haven't done an in-depth analysis, but this feels like the wrong
> approach: you've still got drivers writing to qc->err_mask, and
> potentially other struct members.
>
> The better solution is most likely to examine the interrupt handling
> paths of each driver...
>
No matter what we do to the interrupt handlers, once a qc enters EH, the
qc is owned by EH and other entities must not be allowed to
complete/free it. This patch does not eliminate interferences between
EH and others but it makes sure that oops doesn't occur due to
completing qc underneath EH.
This patch + flushing solve the whole problem for PIO tasks.
This patch + proper IRQ masking during EH solve it for interrupt driven
commands.
--
tejun
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 12/12] libata: EH / pio tasks synchronization
2006-01-22 7:58 [PATCHSET] libata: various fixes related to EH Tejun Heo
` (7 preceding siblings ...)
2006-01-22 7:58 ` [PATCH 11/12] libata: ignore normal qc completion during EH Tejun Heo
@ 2006-01-22 7:58 ` Tejun Heo
2006-01-22 9:58 ` Jeff Garzik
2006-01-22 7:58 ` [PATCH 09/12] libata: kill NULL qc handling from ->eng_timeout callbacks Tejun Heo
` (3 subsequent siblings)
12 siblings, 1 reply; 28+ messages in thread
From: Tejun Heo @ 2006-01-22 7:58 UTC (permalink / raw)
To: jgarzik, linux-ide, albertcc; +Cc: Tejun Heo
This patch makes sure that pio tasks are flushed before proceeding
with EH.
Signed-off-by: Tejun Heo <htejun@gmail.com>
---
drivers/scsi/libata-core.c | 80 +++++++++++++++++++++++++++++++++++++++++---
include/linux/libata.h | 3 +-
2 files changed, 76 insertions(+), 7 deletions(-)
ea84319b67f28b01f8b09d0151755b890dfff7dc
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index 9556100..183bc26 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -1072,6 +1072,72 @@ static unsigned int ata_pio_modes(const
timing API will get this right anyway */
}
+static inline void
+ata_queue_packet_task(struct ata_port *ap)
+{
+ if (!(ap->flags & ATA_FLAG_FLUSH_PIO_TASK))
+ queue_work(ata_wq, &ap->packet_task);
+}
+
+static inline void
+ata_queue_pio_task(struct ata_port *ap)
+{
+ if (!(ap->flags & ATA_FLAG_FLUSH_PIO_TASK))
+ queue_work(ata_wq, &ap->pio_task);
+}
+
+static inline void
+ata_queue_delayed_pio_task(struct ata_port *ap, unsigned long delay)
+{
+ if (!(ap->flags & ATA_FLAG_FLUSH_PIO_TASK))
+ queue_delayed_work(ata_wq, &ap->pio_task, delay);
+}
+
+/**
+ * ata_flush_pio_tasks - Flush pio_task and packet_task
+ * @ap: the target ata_port
+ *
+ * After this function completes, pio_task and packet_task are
+ * guranteed not to be running or scheduled.
+ *
+ * LOCKING:
+ * Kernel thread context (may sleep)
+ */
+
+static void ata_flush_pio_tasks(struct ata_port *ap)
+{
+ int tmp = 0;
+ unsigned long flags;
+
+ DPRINTK("ENTER\n");
+
+ spin_lock_irqsave(&ap->host_set->lock, flags);
+ ap->flags |= ATA_FLAG_FLUSH_PIO_TASK;
+ spin_unlock_irqrestore(&ap->host_set->lock, flags);
+
+ DPRINTK("flush #1\n");
+ flush_workqueue(ata_wq);
+
+ /*
+ * At this point, if a task is running, it's guaranteed to see
+ * the FLUSH flag; thus, it will never queue pio tasks again.
+ * Cancel and flush.
+ */
+ tmp |= cancel_delayed_work(&ap->pio_task);
+ tmp |= cancel_delayed_work(&ap->packet_task);
+ if (!tmp) {
+ DPRINTK("flush #2\n");
+ flush_workqueue(ata_wq);
+ }
+
+ spin_lock_irqsave(&ap->host_set->lock, flags);
+ ap->flags &= ~ATA_FLAG_FLUSH_PIO_TASK;
+ ap->hsm_task_state = HSM_ST_IDLE;
+ spin_unlock_irqrestore(&ap->host_set->lock, flags);
+
+ DPRINTK("EXIT\n");
+}
+
void ata_qc_complete_internal(struct ata_queued_cmd *qc)
{
struct completion *waiting = qc->private_data;
@@ -3414,7 +3480,7 @@ fsm_start:
}
if (timeout)
- queue_delayed_work(ata_wq, &ap->pio_task, timeout);
+ ata_queue_delayed_pio_task(ap, timeout);
else if (!qc_completed)
goto fsm_start;
}
@@ -3447,6 +3513,8 @@ static void ata_qc_timeout(struct ata_qu
DPRINTK("ENTER\n");
+ ata_flush_pio_tasks(ap);
+
spin_lock_irqsave(&host_set->lock, flags);
switch (qc->tf.protocol) {
@@ -3733,26 +3801,26 @@ unsigned int ata_qc_issue_prot(struct at
ata_qc_set_polling(qc);
ata_tf_to_host(ap, &qc->tf);
ap->hsm_task_state = HSM_ST;
- queue_work(ata_wq, &ap->pio_task);
+ ata_queue_pio_task(ap);
break;
case ATA_PROT_ATAPI:
ata_qc_set_polling(qc);
ata_tf_to_host(ap, &qc->tf);
- queue_work(ata_wq, &ap->packet_task);
+ ata_queue_packet_task(ap);
break;
case ATA_PROT_ATAPI_NODATA:
ap->flags |= ATA_FLAG_NOINTR;
ata_tf_to_host(ap, &qc->tf);
- queue_work(ata_wq, &ap->packet_task);
+ ata_queue_packet_task(ap);
break;
case ATA_PROT_ATAPI_DMA:
ap->flags |= ATA_FLAG_NOINTR;
ap->ops->tf_load(ap, &qc->tf); /* load tf registers */
ap->ops->bmdma_setup(qc); /* set up bmdma */
- queue_work(ata_wq, &ap->packet_task);
+ ata_queue_packet_task(ap);
break;
default:
@@ -4183,7 +4251,7 @@ static void atapi_packet_task(void *_dat
/* PIO commands are handled by polling */
ap->hsm_task_state = HSM_ST;
- queue_work(ata_wq, &ap->pio_task);
+ ata_queue_pio_task(ap);
}
return;
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 063221d..7837ba3 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -162,7 +162,8 @@ enum {
ATA_FLAG_PIO_LBA48 = (1 << 13), /* Host DMA engine is LBA28 only */
ATA_FLAG_IRQ_MASK = (1 << 14), /* Mask IRQ in PIO xfers */
- ATA_FLAG_IN_EH = (1 << 15), /* EH in progress */
+ ATA_FLAG_FLUSH_PIO_TASK = (1 << 15), /* Flush PIO task */
+ ATA_FLAG_IN_EH = (1 << 16), /* EH in progress */
ATA_QCFLAG_ACTIVE = (1 << 1), /* cmd not yet ack'd to scsi lyer */
ATA_QCFLAG_SG = (1 << 3), /* have s/g table? */
--
1.0.8
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH 12/12] libata: EH / pio tasks synchronization
2006-01-22 7:58 ` [PATCH 12/12] libata: EH / pio tasks synchronization Tejun Heo
@ 2006-01-22 9:58 ` Jeff Garzik
2006-01-22 10:27 ` Tejun Heo
0 siblings, 1 reply; 28+ messages in thread
From: Jeff Garzik @ 2006-01-22 9:58 UTC (permalink / raw)
To: Tejun Heo; +Cc: linux-ide, albertcc
Tejun Heo wrote:
> This patch makes sure that pio tasks are flushed before proceeding
> with EH.
>
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> +static void ata_flush_pio_tasks(struct ata_port *ap)
> +{
> + int tmp = 0;
> + unsigned long flags;
> +
> + DPRINTK("ENTER\n");
> +
> + spin_lock_irqsave(&ap->host_set->lock, flags);
> + ap->flags |= ATA_FLAG_FLUSH_PIO_TASK;
> + spin_unlock_irqrestore(&ap->host_set->lock, flags);
> +
> + DPRINTK("flush #1\n");
> + flush_workqueue(ata_wq);
> +
> + /*
> + * At this point, if a task is running, it's guaranteed to see
> + * the FLUSH flag; thus, it will never queue pio tasks again.
> + * Cancel and flush.
> + */
> + tmp |= cancel_delayed_work(&ap->pio_task);
> + tmp |= cancel_delayed_work(&ap->packet_task);
> + if (!tmp) {
> + DPRINTK("flush #2\n");
> + flush_workqueue(ata_wq);
> + }
> +
> + spin_lock_irqsave(&ap->host_set->lock, flags);
> + ap->flags &= ~ATA_FLAG_FLUSH_PIO_TASK;
> + ap->hsm_task_state = HSM_ST_IDLE;
> + spin_unlock_irqrestore(&ap->host_set->lock, flags);
Mostly ok, except
1) forcing hsm_task_state to HSM_ST_IDLE seems unlikely to be correct.
The error handling code should already be doing that.
2) should break this patch up into two pieces: add and use new helpers,
and add flush code.
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH 12/12] libata: EH / pio tasks synchronization
2006-01-22 9:58 ` Jeff Garzik
@ 2006-01-22 10:27 ` Tejun Heo
0 siblings, 0 replies; 28+ messages in thread
From: Tejun Heo @ 2006-01-22 10:27 UTC (permalink / raw)
To: Jeff Garzik; +Cc: linux-ide, albertcc
Jeff Garzik wrote:
> Tejun Heo wrote:
>
>> This patch makes sure that pio tasks are flushed before proceeding
>> with EH.
>>
>> Signed-off-by: Tejun Heo <htejun@gmail.com>
>
>
>> +static void ata_flush_pio_tasks(struct ata_port *ap)
>> +{
>> + int tmp = 0;
>> + unsigned long flags;
>> +
>> + DPRINTK("ENTER\n");
>> +
>> + spin_lock_irqsave(&ap->host_set->lock, flags);
>> + ap->flags |= ATA_FLAG_FLUSH_PIO_TASK;
>> + spin_unlock_irqrestore(&ap->host_set->lock, flags);
>> +
>> + DPRINTK("flush #1\n");
>> + flush_workqueue(ata_wq);
>> +
>> + /*
>> + * At this point, if a task is running, it's guaranteed to see
>> + * the FLUSH flag; thus, it will never queue pio tasks again.
>> + * Cancel and flush.
>> + */
>> + tmp |= cancel_delayed_work(&ap->pio_task);
>> + tmp |= cancel_delayed_work(&ap->packet_task);
>> + if (!tmp) {
>> + DPRINTK("flush #2\n");
>> + flush_workqueue(ata_wq);
>> + }
>> +
>> + spin_lock_irqsave(&ap->host_set->lock, flags);
>> + ap->flags &= ~ATA_FLAG_FLUSH_PIO_TASK;
>> + ap->hsm_task_state = HSM_ST_IDLE;
>> + spin_unlock_irqrestore(&ap->host_set->lock, flags);
>
>
> Mostly ok, except
>
> 1) forcing hsm_task_state to HSM_ST_IDLE seems unlikely to be correct.
> The error handling code should already be doing that.
Currently no ->eng_timeout does that. Unless hsm_task_state is useful
for error analysis, EH handlers would be doing...
ata_flush_pio_tasks(ap);
ap->hsm_task_state = HSM_ST_IDLE;
Currently, it's only ata_qc_timeout and I'm very okay with both. Do you
think resetting hsm_task_state should be done in eng_timeout?
> 2) should break this patch up into two pieces: add and use new helpers,
> and add flush code.
Sure.
--
tejun
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 09/12] libata: kill NULL qc handling from ->eng_timeout callbacks
2006-01-22 7:58 [PATCHSET] libata: various fixes related to EH Tejun Heo
` (8 preceding siblings ...)
2006-01-22 7:58 ` [PATCH 12/12] libata: EH / pio tasks synchronization Tejun Heo
@ 2006-01-22 7:58 ` Tejun Heo
2006-01-22 7:58 ` [PATCH 08/12] libata: fix handling of race between timeout and completion Tejun Heo
` (2 subsequent siblings)
12 siblings, 0 replies; 28+ messages in thread
From: Tejun Heo @ 2006-01-22 7:58 UTC (permalink / raw)
To: jgarzik, linux-ide, albertcc; +Cc: Tejun Heo
->eng_timeout is not invoked with NULL qc anymore. Kill NULL qc
handling from all ->eng_timeout callbacks.
Signed-off-by: Tejun Heo <htejun@gmail.com>
---
drivers/scsi/ahci.c | 12 +++---------
drivers/scsi/libata-core.c | 12 +-----------
drivers/scsi/sata_mv.c | 9 ++-------
drivers/scsi/sata_promise.c | 9 +--------
drivers/scsi/sata_sil24.c | 5 -----
drivers/scsi/sata_sx4.c | 9 +--------
6 files changed, 8 insertions(+), 48 deletions(-)
ceaea59820d442a5b57e1dd2bf185173dfd2c109
diff --git a/drivers/scsi/ahci.c b/drivers/scsi/ahci.c
index 89ba2bd..fdd11ee 100644
--- a/drivers/scsi/ahci.c
+++ b/drivers/scsi/ahci.c
@@ -666,19 +666,13 @@ static void ahci_eng_timeout(struct ata_
spin_lock_irqsave(&host_set->lock, flags);
+ ahci_restart_port(ap, readl(port_mmio + PORT_IRQ_STAT));
qc = ata_qc_from_tag(ap, ap->active_tag);
- if (!qc) {
- printk(KERN_ERR "ata%u: BUG: timeout without command\n",
- ap->id);
- } else {
- ahci_restart_port(ap, readl(port_mmio + PORT_IRQ_STAT));
- qc->err_mask |= AC_ERR_TIMEOUT;
- }
+ qc->err_mask |= AC_ERR_TIMEOUT;
spin_unlock_irqrestore(&host_set->lock, flags);
- if (qc)
- ata_eh_qc_complete(qc);
+ ata_eh_qc_complete(qc);
}
static inline int ahci_host_intr(struct ata_port *ap, struct ata_queued_cmd *qc)
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index 93ae2dc..b2f51bb 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -3503,20 +3503,10 @@ static void ata_qc_timeout(struct ata_qu
void ata_eng_timeout(struct ata_port *ap)
{
- struct ata_queued_cmd *qc;
-
DPRINTK("ENTER\n");
- qc = ata_qc_from_tag(ap, ap->active_tag);
- if (qc)
- ata_qc_timeout(qc);
- else {
- printk(KERN_ERR "ata%u: BUG: timeout without command\n",
- ap->id);
- goto out;
- }
+ ata_qc_timeout(ata_qc_from_tag(ap, ap->active_tag));
-out:
DPRINTK("EXIT\n");
}
diff --git a/drivers/scsi/sata_mv.c b/drivers/scsi/sata_mv.c
index d9a554c..9b56f9b 100644
--- a/drivers/scsi/sata_mv.c
+++ b/drivers/scsi/sata_mv.c
@@ -1853,13 +1853,8 @@ static void mv_eng_timeout(struct ata_po
mv_err_intr(ap);
mv_stop_and_reset(ap);
- if (!qc) {
- printk(KERN_ERR "ata%u: BUG: timeout without command\n",
- ap->id);
- } else {
- qc->err_mask |= AC_ERR_TIMEOUT;
- ata_eh_qc_complete(qc);
- }
+ qc->err_mask |= AC_ERR_TIMEOUT;
+ ata_eh_qc_complete(qc);
}
/**
diff --git a/drivers/scsi/sata_promise.c b/drivers/scsi/sata_promise.c
index 77ef646..9fc9cbf 100644
--- a/drivers/scsi/sata_promise.c
+++ b/drivers/scsi/sata_promise.c
@@ -394,11 +394,6 @@ static void pdc_eng_timeout(struct ata_p
spin_lock_irqsave(&host_set->lock, flags);
qc = ata_qc_from_tag(ap, ap->active_tag);
- if (!qc) {
- printk(KERN_ERR "ata%u: BUG: timeout without command\n",
- ap->id);
- goto out;
- }
switch (qc->tf.protocol) {
case ATA_PROT_DMA:
@@ -418,10 +413,8 @@ static void pdc_eng_timeout(struct ata_p
break;
}
-out:
spin_unlock_irqrestore(&host_set->lock, flags);
- if (qc)
- ata_eh_qc_complete(qc);
+ ata_eh_qc_complete(qc);
DPRINTK("EXIT\n");
}
diff --git a/drivers/scsi/sata_sil24.c b/drivers/scsi/sata_sil24.c
index 7222fc7..1160fda 100644
--- a/drivers/scsi/sata_sil24.c
+++ b/drivers/scsi/sata_sil24.c
@@ -638,11 +638,6 @@ static void sil24_eng_timeout(struct ata
struct ata_queued_cmd *qc;
qc = ata_qc_from_tag(ap, ap->active_tag);
- if (!qc) {
- printk(KERN_ERR "ata%u: BUG: timeout without command\n",
- ap->id);
- return;
- }
printk(KERN_ERR "ata%u: command timeout\n", ap->id);
qc->err_mask |= AC_ERR_TIMEOUT;
diff --git a/drivers/scsi/sata_sx4.c b/drivers/scsi/sata_sx4.c
index 9f992fb..b4ffa3f 100644
--- a/drivers/scsi/sata_sx4.c
+++ b/drivers/scsi/sata_sx4.c
@@ -866,11 +866,6 @@ static void pdc_eng_timeout(struct ata_p
spin_lock_irqsave(&host_set->lock, flags);
qc = ata_qc_from_tag(ap, ap->active_tag);
- if (!qc) {
- printk(KERN_ERR "ata%u: BUG: timeout without command\n",
- ap->id);
- goto out;
- }
switch (qc->tf.protocol) {
case ATA_PROT_DMA:
@@ -889,10 +884,8 @@ static void pdc_eng_timeout(struct ata_p
break;
}
-out:
spin_unlock_irqrestore(&host_set->lock, flags);
- if (qc)
- ata_eh_qc_complete(qc);
+ ata_eh_qc_complete(qc);
DPRINTK("EXIT\n");
}
--
1.0.8
^ permalink raw reply related [flat|nested] 28+ messages in thread* [PATCH 08/12] libata: fix handling of race between timeout and completion
2006-01-22 7:58 [PATCHSET] libata: various fixes related to EH Tejun Heo
` (9 preceding siblings ...)
2006-01-22 7:58 ` [PATCH 09/12] libata: kill NULL qc handling from ->eng_timeout callbacks Tejun Heo
@ 2006-01-22 7:58 ` Tejun Heo
2006-01-22 9:41 ` Jeff Garzik
2006-01-22 7:58 ` [PATCH 07/12] libata: implement and apply ata_eh_qc_complete/retry() Tejun Heo
2006-01-22 9:10 ` [PATCHSET] libata: various fixes related to EH Jeff Garzik
12 siblings, 1 reply; 28+ messages in thread
From: Tejun Heo @ 2006-01-22 7:58 UTC (permalink / raw)
To: jgarzik, linux-ide, albertcc; +Cc: Tejun Heo
If a qc completes after SCSI timer expires but before libata EH kicks
in, the qc gets completed but the scsicmd still gets passed to libata
EH resulting in ->eng_timeout invocation with NULL qc. Currently none
of ->eng_timeout callbacks handles this properly. This patch makes
ata_scsi_error() bypass ->eng_timeout and handle this rare case.
Signed-off-by: Tejun Heo <htejun@gmail.com>
---
drivers/scsi/libata-scsi.c | 42 +++++++++++++++++++++++++++++++++++++++---
1 files changed, 39 insertions(+), 3 deletions(-)
ba51311c2cc3177f9c5d33aee11be1488d783c7c
diff --git a/drivers/scsi/libata-scsi.c b/drivers/scsi/libata-scsi.c
index ab6b533..accb63a 100644
--- a/drivers/scsi/libata-scsi.c
+++ b/drivers/scsi/libata-scsi.c
@@ -731,12 +731,48 @@ int ata_scsi_slave_config(struct scsi_de
int ata_scsi_error(struct Scsi_Host *host)
{
- struct ata_port *ap;
+ struct ata_port *ap = (struct ata_port *) &host->hostdata[0];
+ struct ata_queued_cmd *qc;
+ unsigned long flags;
DPRINTK("ENTER\n");
- ap = (struct ata_port *) &host->hostdata[0];
- ap->ops->eng_timeout(ap);
+ spin_lock_irqsave(&ap->host_set->lock, flags);
+ qc = ata_qc_from_tag(ap, ap->active_tag);
+ spin_unlock_irqrestore(&ap->host_set->lock, flags);
+
+ if (qc) {
+ ap->ops->eng_timeout(ap);
+ } else {
+ struct scsi_cmnd *scmd;
+ unsigned char *sb;
+
+ /* The scmd had timed out but the corresponding qc
+ * completed successfully inbetween timer expiration
+ * and here. Retry if possible.
+ *
+ * It is better to enter eng_timeout and perform EH
+ * before retrying the command, but this case should
+ * be _very_ rare and eng_timeout isn't ready for
+ * NULL-qc case.
+ */
+ scmd = list_entry(host->eh_cmd_q.next,
+ struct scsi_cmnd, eh_entry);
+ sb = scmd->sense_buffer;
+
+ /* Timeout, fake parity for now */
+ scmd->result = (DRIVER_SENSE << 24) | SAM_STAT_CHECK_CONDITION;
+ sb[0] = 0x70;
+ sb[7] = 0x0a;
+ sb[2] = ABORTED_COMMAND;
+ sb[12] = 0x47;
+ sb[13] = 0x00;
+
+ printk(KERN_WARNING "ata%u: interrupt and timer raced for "
+ "scsicmd %p\n", ap->id, scmd);
+
+ scsi_eh_finish_cmd(scmd, &ap->eh_done_q);
+ }
assert(host->host_failed == 0 && list_empty(&host->eh_cmd_q));
--
1.0.8
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH 08/12] libata: fix handling of race between timeout and completion
2006-01-22 7:58 ` [PATCH 08/12] libata: fix handling of race between timeout and completion Tejun Heo
@ 2006-01-22 9:41 ` Jeff Garzik
0 siblings, 0 replies; 28+ messages in thread
From: Jeff Garzik @ 2006-01-22 9:41 UTC (permalink / raw)
To: Tejun Heo; +Cc: linux-ide, albertcc
Tejun Heo wrote:
> If a qc completes after SCSI timer expires but before libata EH kicks
> in, the qc gets completed but the scsicmd still gets passed to libata
> EH resulting in ->eng_timeout invocation with NULL qc. Currently none
> of ->eng_timeout callbacks handles this properly. This patch makes
> ata_scsi_error() bypass ->eng_timeout and handle this rare case.
>
> Signed-off-by: Tejun Heo <htejun@gmail.com>
>
> ---
>
> drivers/scsi/libata-scsi.c | 42 +++++++++++++++++++++++++++++++++++++++---
> 1 files changed, 39 insertions(+), 3 deletions(-)
>
> ba51311c2cc3177f9c5d33aee11be1488d783c7c
> diff --git a/drivers/scsi/libata-scsi.c b/drivers/scsi/libata-scsi.c
> index ab6b533..accb63a 100644
> --- a/drivers/scsi/libata-scsi.c
> +++ b/drivers/scsi/libata-scsi.c
> @@ -731,12 +731,48 @@ int ata_scsi_slave_config(struct scsi_de
>
> int ata_scsi_error(struct Scsi_Host *host)
> {
> - struct ata_port *ap;
> + struct ata_port *ap = (struct ata_port *) &host->hostdata[0];
> + struct ata_queued_cmd *qc;
> + unsigned long flags;
>
> DPRINTK("ENTER\n");
>
> - ap = (struct ata_port *) &host->hostdata[0];
> - ap->ops->eng_timeout(ap);
> + spin_lock_irqsave(&ap->host_set->lock, flags);
> + qc = ata_qc_from_tag(ap, ap->active_tag);
> + spin_unlock_irqrestore(&ap->host_set->lock, flags);
> +
> + if (qc) {
> + ap->ops->eng_timeout(ap);
> + } else {
> + struct scsi_cmnd *scmd;
> + unsigned char *sb;
> +
> + /* The scmd had timed out but the corresponding qc
> + * completed successfully inbetween timer expiration
> + * and here. Retry if possible.
> + *
> + * It is better to enter eng_timeout and perform EH
> + * before retrying the command, but this case should
> + * be _very_ rare and eng_timeout isn't ready for
> + * NULL-qc case.
> + */
> + scmd = list_entry(host->eh_cmd_q.next,
> + struct scsi_cmnd, eh_entry);
> + sb = scmd->sense_buffer;
> +
> + /* Timeout, fake parity for now */
> + scmd->result = (DRIVER_SENSE << 24) | SAM_STAT_CHECK_CONDITION;
> + sb[0] = 0x70;
> + sb[7] = 0x0a;
> + sb[2] = ABORTED_COMMAND;
> + sb[12] = 0x47;
> + sb[13] = 0x00;
> +
> + printk(KERN_WARNING "ata%u: interrupt and timer raced for "
> + "scsicmd %p\n", ap->id, scmd);
> +
> + scsi_eh_finish_cmd(scmd, &ap->eh_done_q);
Honestly, I'm not sure how to best solve this. I suppose this is ok for
now.
Jeff
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 07/12] libata: implement and apply ata_eh_qc_complete/retry()
2006-01-22 7:58 [PATCHSET] libata: various fixes related to EH Tejun Heo
` (10 preceding siblings ...)
2006-01-22 7:58 ` [PATCH 08/12] libata: fix handling of race between timeout and completion Tejun Heo
@ 2006-01-22 7:58 ` Tejun Heo
2006-01-22 9:10 ` [PATCHSET] libata: various fixes related to EH Jeff Garzik
12 siblings, 0 replies; 28+ messages in thread
From: Tejun Heo @ 2006-01-22 7:58 UTC (permalink / raw)
To: jgarzik, linux-ide, albertcc; +Cc: Tejun Heo
Implement ata_eh_qc_complete/retry() using scsi_eh_finish_cmd() and
scsi_eh_flush_done_q(). This removes all eh scsicmd finish hacks from
low level drivers.
This change was first suggested by Jeff Garzik.
Signed-off-by: Tejun Heo <htejun@gmail.com>
---
drivers/scsi/ahci.c | 12 ++-------
drivers/scsi/libata-core.c | 14 ++++------
drivers/scsi/libata-scsi.c | 59 +++++++++++++++++++++++++++++++++++++++----
drivers/scsi/sata_mv.c | 12 +--------
drivers/scsi/sata_promise.c | 12 +--------
drivers/scsi/sata_sil24.c | 10 +------
drivers/scsi/sata_sx4.c | 12 +--------
include/linux/libata.h | 3 ++
8 files changed, 70 insertions(+), 64 deletions(-)
fa3ba673fc07197ab8a882d68e9641f3e8dc041f
diff --git a/drivers/scsi/ahci.c b/drivers/scsi/ahci.c
index 75b1fb5..89ba2bd 100644
--- a/drivers/scsi/ahci.c
+++ b/drivers/scsi/ahci.c
@@ -672,19 +672,13 @@ static void ahci_eng_timeout(struct ata_
ap->id);
} else {
ahci_restart_port(ap, readl(port_mmio + PORT_IRQ_STAT));
-
- /* hack alert! We cannot use the supplied completion
- * function from inside the ->eh_strategy_handler() thread.
- * libata is the only user of ->eh_strategy_handler() in
- * any kernel, so the default scsi_done() assumes it is
- * not being called from the SCSI EH.
- */
- qc->scsidone = scsi_finish_command;
qc->err_mask |= AC_ERR_TIMEOUT;
- ata_qc_complete(qc);
}
spin_unlock_irqrestore(&host_set->lock, flags);
+
+ if (qc)
+ ata_eh_qc_complete(qc);
}
static inline int ahci_host_intr(struct ata_port *ap, struct ata_queued_cmd *qc)
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index b29bf0d..93ae2dc 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -3449,14 +3449,6 @@ static void ata_qc_timeout(struct ata_qu
spin_lock_irqsave(&host_set->lock, flags);
- /* hack alert! We cannot use the supplied completion
- * function from inside the ->eh_strategy_handler() thread.
- * libata is the only user of ->eh_strategy_handler() in
- * any kernel, so the default scsi_done() assumes it is
- * not being called from the SCSI EH.
- */
- qc->scsidone = scsi_finish_command;
-
switch (qc->tf.protocol) {
case ATA_PROT_DMA:
@@ -3480,12 +3472,13 @@ static void ata_qc_timeout(struct ata_qu
/* complete taskfile transaction */
qc->err_mask |= ac_err_mask(drv_stat);
- ata_qc_complete(qc);
break;
}
spin_unlock_irqrestore(&host_set->lock, flags);
+ ata_eh_qc_complete(qc);
+
DPRINTK("EXIT\n");
}
@@ -4422,6 +4415,7 @@ static void ata_host_init(struct ata_por
INIT_WORK(&ap->packet_task, atapi_packet_task, ap);
INIT_WORK(&ap->pio_task, ata_pio_task, ap);
+ INIT_LIST_HEAD(&ap->eh_done_q);
for (i = 0; i < ATA_MAX_DEVICES; i++)
ap->device[i].devno = i;
@@ -5165,6 +5159,8 @@ EXPORT_SYMBOL_GPL(ata_dev_classify);
EXPORT_SYMBOL_GPL(ata_dev_id_string);
EXPORT_SYMBOL_GPL(ata_dev_config);
EXPORT_SYMBOL_GPL(ata_scsi_simulate);
+EXPORT_SYMBOL_GPL(ata_eh_qc_complete);
+EXPORT_SYMBOL_GPL(ata_eh_qc_retry);
EXPORT_SYMBOL_GPL(ata_pio_need_iordy);
EXPORT_SYMBOL_GPL(ata_timing_compute);
diff --git a/drivers/scsi/libata-scsi.c b/drivers/scsi/libata-scsi.c
index 95e3c27..ab6b533 100644
--- a/drivers/scsi/libata-scsi.c
+++ b/drivers/scsi/libata-scsi.c
@@ -738,17 +738,64 @@ int ata_scsi_error(struct Scsi_Host *hos
ap = (struct ata_port *) &host->hostdata[0];
ap->ops->eng_timeout(ap);
- /* TODO: this is per-command; when queueing is supported
- * this code will either change or move to a more
- * appropriate place
- */
- host->host_failed--;
- INIT_LIST_HEAD(&host->eh_cmd_q);
+ assert(host->host_failed == 0 && list_empty(&host->eh_cmd_q));
+
+ scsi_eh_flush_done_q(&ap->eh_done_q);
DPRINTK("EXIT\n");
return 0;
}
+static void ata_eh_scsidone(struct scsi_cmnd *scmd)
+{
+ /* nada */
+}
+
+static void __ata_eh_qc_complete(struct ata_queued_cmd *qc)
+{
+ struct ata_port *ap = qc->ap;
+ struct scsi_cmnd *scmd = qc->scsicmd;
+ unsigned long flags;
+
+ spin_lock_irqsave(&ap->host_set->lock, flags);
+ qc->scsidone = ata_eh_scsidone;
+ ata_qc_complete(qc);
+ assert(!ata_tag_valid(qc->tag));
+ spin_unlock_irqrestore(&ap->host_set->lock, flags);
+
+ scsi_eh_finish_cmd(scmd, &ap->eh_done_q);
+}
+
+/**
+ * ata_eh_qc_complete - Complete an active ATA command from EH
+ * @qc: Command to complete
+ *
+ * Indicate to the mid and upper layers that an ATA command has
+ * completed. To be used from EH.
+ */
+void ata_eh_qc_complete(struct ata_queued_cmd *qc)
+{
+ struct scsi_cmnd *scmd = qc->scsicmd;
+ scmd->retries = scmd->allowed;
+ __ata_eh_qc_complete(qc);
+}
+
+/**
+ * ata_eh_qc_retry - Tell midlayer to retry an ATA command after EH
+ * @qc: Command to retry
+ *
+ * Indicate to the mid and upper layers that an ATA command
+ * should be retried. To be used from EH.
+ *
+ * SCSI midlayer limits the number of retries to scmd->allowed.
+ * This function might need to adjust scmd->retries for commands
+ * which get retried due to unrelated NCQ failures.
+ */
+void ata_eh_qc_retry(struct ata_queued_cmd *qc)
+{
+ __ata_eh_qc_complete(qc);
+}
+
/**
* ata_scsi_start_stop_xlat - Translate SCSI START STOP UNIT command
* @qc: Storage for translated ATA taskfile
diff --git a/drivers/scsi/sata_mv.c b/drivers/scsi/sata_mv.c
index 281223a..d9a554c 100644
--- a/drivers/scsi/sata_mv.c
+++ b/drivers/scsi/sata_mv.c
@@ -1839,7 +1839,6 @@ static void mv_phy_reset(struct ata_port
static void mv_eng_timeout(struct ata_port *ap)
{
struct ata_queued_cmd *qc;
- unsigned long flags;
printk(KERN_ERR "ata%u: Entering mv_eng_timeout\n",ap->id);
DPRINTK("All regs @ start of eng_timeout\n");
@@ -1858,17 +1857,8 @@ static void mv_eng_timeout(struct ata_po
printk(KERN_ERR "ata%u: BUG: timeout without command\n",
ap->id);
} else {
- /* hack alert! We cannot use the supplied completion
- * function from inside the ->eh_strategy_handler() thread.
- * libata is the only user of ->eh_strategy_handler() in
- * any kernel, so the default scsi_done() assumes it is
- * not being called from the SCSI EH.
- */
- spin_lock_irqsave(&ap->host_set->lock, flags);
- qc->scsidone = scsi_finish_command;
qc->err_mask |= AC_ERR_TIMEOUT;
- ata_qc_complete(qc);
- spin_unlock_irqrestore(&ap->host_set->lock, flags);
+ ata_eh_qc_complete(qc);
}
}
diff --git a/drivers/scsi/sata_promise.c b/drivers/scsi/sata_promise.c
index bac36d5..77ef646 100644
--- a/drivers/scsi/sata_promise.c
+++ b/drivers/scsi/sata_promise.c
@@ -400,21 +400,12 @@ static void pdc_eng_timeout(struct ata_p
goto out;
}
- /* hack alert! We cannot use the supplied completion
- * function from inside the ->eh_strategy_handler() thread.
- * libata is the only user of ->eh_strategy_handler() in
- * any kernel, so the default scsi_done() assumes it is
- * not being called from the SCSI EH.
- */
- qc->scsidone = scsi_finish_command;
-
switch (qc->tf.protocol) {
case ATA_PROT_DMA:
case ATA_PROT_NODATA:
printk(KERN_ERR "ata%u: command timeout\n", ap->id);
drv_stat = ata_wait_idle(ap);
qc->err_mask |= __ac_err_mask(drv_stat);
- ata_qc_complete(qc);
break;
default:
@@ -424,12 +415,13 @@ static void pdc_eng_timeout(struct ata_p
ap->id, qc->tf.command, drv_stat);
qc->err_mask |= ac_err_mask(drv_stat);
- ata_qc_complete(qc);
break;
}
out:
spin_unlock_irqrestore(&host_set->lock, flags);
+ if (qc)
+ ata_eh_qc_complete(qc);
DPRINTK("EXIT\n");
}
diff --git a/drivers/scsi/sata_sil24.c b/drivers/scsi/sata_sil24.c
index 5a7a7b1..7222fc7 100644
--- a/drivers/scsi/sata_sil24.c
+++ b/drivers/scsi/sata_sil24.c
@@ -644,17 +644,9 @@ static void sil24_eng_timeout(struct ata
return;
}
- /*
- * hack alert! We cannot use the supplied completion
- * function from inside the ->eh_strategy_handler() thread.
- * libata is the only user of ->eh_strategy_handler() in
- * any kernel, so the default scsi_done() assumes it is
- * not being called from the SCSI EH.
- */
printk(KERN_ERR "ata%u: command timeout\n", ap->id);
- qc->scsidone = scsi_finish_command;
qc->err_mask |= AC_ERR_TIMEOUT;
- ata_qc_complete(qc);
+ ata_eh_qc_complete(qc);
sil24_reset_controller(ap);
}
diff --git a/drivers/scsi/sata_sx4.c b/drivers/scsi/sata_sx4.c
index 3175c6b..9f992fb 100644
--- a/drivers/scsi/sata_sx4.c
+++ b/drivers/scsi/sata_sx4.c
@@ -872,20 +872,11 @@ static void pdc_eng_timeout(struct ata_p
goto out;
}
- /* hack alert! We cannot use the supplied completion
- * function from inside the ->eh_strategy_handler() thread.
- * libata is the only user of ->eh_strategy_handler() in
- * any kernel, so the default scsi_done() assumes it is
- * not being called from the SCSI EH.
- */
- qc->scsidone = scsi_finish_command;
-
switch (qc->tf.protocol) {
case ATA_PROT_DMA:
case ATA_PROT_NODATA:
printk(KERN_ERR "ata%u: command timeout\n", ap->id);
qc->err_mask |= __ac_err_mask(ata_wait_idle(ap));
- ata_qc_complete(qc);
break;
default:
@@ -895,12 +886,13 @@ static void pdc_eng_timeout(struct ata_p
ap->id, qc->tf.command, drv_stat);
qc->err_mask |= ac_err_mask(drv_stat);
- ata_qc_complete(qc);
break;
}
out:
spin_unlock_irqrestore(&host_set->lock, flags);
+ if (qc)
+ ata_eh_qc_complete(qc);
DPRINTK("EXIT\n");
}
diff --git a/include/linux/libata.h b/include/linux/libata.h
index b1ea2f9..576788d 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -398,6 +398,7 @@ struct ata_port {
unsigned long pio_task_timeout;
u32 msg_enable;
+ struct list_head eh_done_q;
void *private_data;
};
@@ -490,6 +491,8 @@ extern int ata_scsi_detect(struct scsi_h
extern int ata_scsi_ioctl(struct scsi_device *dev, int cmd, void __user *arg);
extern int ata_scsi_queuecmd(struct scsi_cmnd *cmd, void (*done)(struct scsi_cmnd *));
extern int ata_scsi_error(struct Scsi_Host *host);
+extern void ata_eh_qc_complete(struct ata_queued_cmd *qc);
+extern void ata_eh_qc_retry(struct ata_queued_cmd *qc);
extern int ata_scsi_release(struct Scsi_Host *host);
extern unsigned int ata_host_intr(struct ata_port *ap, struct ata_queued_cmd *qc);
extern int ata_scsi_device_resume(struct scsi_device *);
--
1.0.8
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCHSET] libata: various fixes related to EH
2006-01-22 7:58 [PATCHSET] libata: various fixes related to EH Tejun Heo
` (11 preceding siblings ...)
2006-01-22 7:58 ` [PATCH 07/12] libata: implement and apply ata_eh_qc_complete/retry() Tejun Heo
@ 2006-01-22 9:10 ` Jeff Garzik
12 siblings, 0 replies; 28+ messages in thread
From: Jeff Garzik @ 2006-01-22 9:10 UTC (permalink / raw)
To: Tejun Heo; +Cc: linux-ide, albertcc
Tejun Heo wrote:
> Hello, Jeff, Albert.
>
> This patchset is composed of 12 patches fixing/updating various EH
> related stuff in libata. Although not all of the patches are
> logically related, they need to be ordered because they modify
> similar parts of code.
>
> #01 : cosmetic
> #02 : ata_qc_new/free model
> #03 : ata_qc_issue() error handling fix
> #04 : detailed AC_ERR_* flags
> #05 : return AC_ERR_* from issue functions
> #06-07 : ata_eh_qc_retry/complete
> #08-12 : eh synchronization (#12 is the pio/eh sync patch I talked
> about in the thread "update timer for PIO polling - revised")
>
> Jeff, these are hopefully more acceptable patches from my recent EH
> work. I'll soon follow up with more pervasive patches. My working
> version of new EH now does most things described in ATA EH doc
> including reset, revalidation and gearing down. I've also ported
> Jen's NCQ support over it, and, although it has a few issues, it's
> generally working okay.
At first glance, all 12 patches look pretty good. Definitely moving in
the right direction, though I noticed a few things. Comments will follow...
Jeff
^ permalink raw reply [flat|nested] 28+ messages in thread