* [PATCH/RFC 0/3] libata: misc fixes
@ 2005-11-09 4:57 Albert Lee
2005-11-09 5:03 ` [PATCH 1/3] libata: if condition fix for ata_dev_identify() Albert Lee
` (2 more replies)
0 siblings, 3 replies; 19+ messages in thread
From: Albert Lee @ 2005-11-09 4:57 UTC (permalink / raw)
To: Jeff Garzik; +Cc: Linux IDE, Bartlomiej Zolnierkiewicz, Doug Maxey, Tejun Heo
Dear all,
Misc fixes for libata:
1/3: if condition fix for ata_dev_identify().
2/3: move err_mask to ata_queued_cmd
3/3: check qc->err_mask for the internal commands
Patch against the mainline tree
(0b154bb7d0cce80e9c0bcf11d4f9e71b59409d26).
For your review and advice, thanks.
Albert
^ permalink raw reply [flat|nested] 19+ messages in thread* [PATCH 1/3] libata: if condition fix for ata_dev_identify() 2005-11-09 4:57 [PATCH/RFC 0/3] libata: misc fixes Albert Lee @ 2005-11-09 5:03 ` Albert Lee 2005-11-09 5:07 ` [PATCH/RFC 2/3] libata: move err_mask to ata_queued_cmd Albert Lee 2005-11-09 5:12 ` [PATCH/RFC 3/3] libata: check qc->err_mask for the internal commands Albert Lee 2 siblings, 0 replies; 19+ messages in thread From: Albert Lee @ 2005-11-09 5:03 UTC (permalink / raw) To: Jeff Garzik; +Cc: Linux IDE, Bartlomiej Zolnierkiewicz, Doug Maxey, Tejun Heo Patch 1/3: - if condition fix for ata_dev_identify() - ata_pio_poll() minor cleanup. Changes: - Use (dev->class == ATA_DEV_ATA) for ata_dev_identify() since "qc->tf.command" has been overwritten by the device status - Use HSM_ST_TMOUT directly in ata_pio_poll() For your review, thanks. Albert Signed-off-by: Albert Lee <albertcc@tw.ibm.com> ============ --- linux/drivers/scsi/libata-core.c 2005-11-07 18:25:38.000000000 +0800 +++ identify/drivers/scsi/libata-core.c 2005-11-09 10:14:41.000000000 +0800 @@ -1144,7 +1144,7 @@ retry: * ATA software reset (SRST, the default) does not appear * to have this problem. */ - if ((using_edd) && (qc->tf.command == ATA_CMD_ID_ATA)) { + if ((using_edd) && (dev->class == ATA_DEV_ATA)) { u8 err = qc->tf.feature; if (err & ATA_ABORTED) { dev->class = ATA_DEV_ATAPI; @@ -2747,7 +2747,6 @@ static unsigned long ata_pio_poll(struct u8 status; unsigned int poll_state = HSM_ST_UNKNOWN; unsigned int reg_state = HSM_ST_UNKNOWN; - const unsigned int tmout_state = HSM_ST_TMOUT; switch (ap->hsm_task_state) { case HSM_ST: @@ -2768,7 +2767,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)) { - ap->hsm_task_state = tmout_state; + ap->hsm_task_state = HSM_ST_TMOUT; return 0; } ap->hsm_task_state = poll_state; ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH/RFC 2/3] libata: move err_mask to ata_queued_cmd 2005-11-09 4:57 [PATCH/RFC 0/3] libata: misc fixes Albert Lee 2005-11-09 5:03 ` [PATCH 1/3] libata: if condition fix for ata_dev_identify() Albert Lee @ 2005-11-09 5:07 ` Albert Lee 2005-11-09 6:25 ` Jeff Garzik 2005-11-09 5:12 ` [PATCH/RFC 3/3] libata: check qc->err_mask for the internal commands Albert Lee 2 siblings, 1 reply; 19+ messages in thread From: Albert Lee @ 2005-11-09 5:07 UTC (permalink / raw) To: Jeff Garzik; +Cc: Linux IDE, Bartlomiej Zolnierkiewicz, Doug Maxey, Tejun Heo Patch 2/3: move err_mask to ata_queued_cmd. Changes: - Move the err_mask parameter of the complete functions to ata_queued_cmd. This can make the err_mask more accessible outside the complete functions; also makes the HSM easier to pass err_mask between states. For your review and advice, thanks. Albert Signed-off-by: Albert Lee <albertcc@tw.ibm.com> ============ --- linux/include/linux/libata.h 2005-11-07 18:25:47.000000000 +0800 +++ err_mask/include/linux/libata.h 2005-11-08 16:43:25.000000000 +0800 @@ -190,7 +190,7 @@ struct ata_port; struct ata_queued_cmd; /* typedefs */ -typedef int (*ata_qc_cb_t) (struct ata_queued_cmd *qc, unsigned int err_mask); +typedef int (*ata_qc_cb_t) (struct ata_queued_cmd *qc); struct ata_ioports { unsigned long cmd_addr; @@ -275,6 +275,8 @@ struct ata_queued_cmd { /* DO NOT iterate over __sg manually, use ata_for_each_sg() */ struct scatterlist *__sg; + unsigned int err_mask; + ata_qc_cb_t complete_fn; struct completion *waiting; @@ -472,7 +474,7 @@ extern void ata_bmdma_start (struct ata_ extern void ata_bmdma_stop(struct ata_queued_cmd *qc); extern u8 ata_bmdma_status(struct ata_port *ap); extern void ata_bmdma_irq_clear(struct ata_port *ap); -extern void ata_qc_complete(struct ata_queued_cmd *qc, unsigned int err_mask); +extern void ata_qc_complete(struct ata_queued_cmd *qc); extern void ata_eng_timeout(struct ata_port *ap); extern void ata_scsi_simulate(u16 *id, struct scsi_cmnd *cmd, void (*done)(struct scsi_cmnd *)); --- linux/drivers/scsi/libata.h 2005-11-07 18:25:38.000000000 +0800 +++ err_mask/drivers/scsi/libata.h 2005-11-08 16:31:00.000000000 +0800 @@ -39,7 +39,7 @@ 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 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); --- identify/drivers/scsi/libata-core.c 2005-11-09 10:14:41.000000000 +0800 +++ err_mask/drivers/scsi/libata-core.c 2005-11-09 10:15:33.000000000 +0800 @@ -1152,6 +1152,7 @@ retry: qc->cursg_ofs = 0; qc->cursect = 0; qc->nsect = 1; + qc->err_mask = 0; goto retry; } } @@ -2719,7 +2720,7 @@ static int ata_sg_setup(struct ata_queue * None. (grabs host lock) */ -void ata_poll_qc_complete(struct ata_queued_cmd *qc, unsigned int err_mask) +void ata_poll_qc_complete(struct ata_queued_cmd *qc) { struct ata_port *ap = qc->ap; unsigned long flags; @@ -2727,7 +2728,7 @@ void ata_poll_qc_complete(struct ata_que spin_lock_irqsave(&ap->host_set->lock, flags); ap->flags &= ~ATA_FLAG_NOINTR; ata_irq_on(ap); - ata_qc_complete(qc, err_mask); + ata_qc_complete(qc); spin_unlock_irqrestore(&ap->host_set->lock, flags); } @@ -2744,10 +2745,14 @@ void ata_poll_qc_complete(struct ata_que static unsigned long ata_pio_poll(struct ata_port *ap) { + struct ata_queued_cmd *qc; u8 status; unsigned int poll_state = HSM_ST_UNKNOWN; unsigned int reg_state = HSM_ST_UNKNOWN; + qc = ata_qc_from_tag(ap, ap->active_tag); + assert(qc != NULL); + switch (ap->hsm_task_state) { case HSM_ST: case HSM_ST_POLL: @@ -2767,6 +2772,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; ap->hsm_task_state = HSM_ST_TMOUT; return 0; } @@ -2812,18 +2818,19 @@ static int ata_pio_complete (struct ata_ } } + qc = ata_qc_from_tag(ap, ap->active_tag); + assert(qc != NULL); + drv_stat = ata_wait_idle(ap); if (!ata_ok(drv_stat)) { + qc->err_mask |= AC_ERR_ATA_BUS; ap->hsm_task_state = HSM_ST_ERR; return 0; } - qc = ata_qc_from_tag(ap, ap->active_tag); - assert(qc != NULL); - ap->hsm_task_state = HSM_ST_IDLE; - ata_poll_qc_complete(qc, 0); + ata_poll_qc_complete(qc); /* another command may start at this point */ @@ -3131,6 +3138,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; ap->hsm_task_state = HSM_ST_ERR; } @@ -3180,6 +3188,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; ap->hsm_task_state = HSM_ST_ERR; return; } @@ -3199,7 +3208,7 @@ static void ata_pio_error(struct ata_por ap->hsm_task_state = HSM_ST_IDLE; - ata_poll_qc_complete(qc, AC_ERR_ATA_BUS); + ata_poll_qc_complete(qc); } static void ata_pio_task(void *_data) @@ -3322,7 +3331,8 @@ static void ata_qc_timeout(struct ata_qu ap->id, qc->tf.command, drv_stat, host_stat); /* complete taskfile transaction */ - ata_qc_complete(qc, ac_err_mask(drv_stat)); + qc->err_mask |= ac_err_mask(drv_stat); + ata_qc_complete(qc); break; } @@ -3420,6 +3430,7 @@ struct ata_queued_cmd *ata_qc_new_init(s qc->cursect = qc->cursg = qc->cursg_ofs = 0; qc->nsect = 0; qc->nbytes = qc->curbytes = 0; + qc->err_mask = 0; ata_tf_init(ap, &qc->tf, dev->devno); } @@ -3427,7 +3438,7 @@ struct ata_queued_cmd *ata_qc_new_init(s return qc; } -int ata_qc_complete_noop(struct ata_queued_cmd *qc, unsigned int err_mask) +int ata_qc_complete_noop(struct ata_queued_cmd *qc) { return 0; } @@ -3486,7 +3497,7 @@ void ata_qc_free(struct ata_queued_cmd * * spin_lock_irqsave(host_set lock) */ -void ata_qc_complete(struct ata_queued_cmd *qc, unsigned int err_mask) +void ata_qc_complete(struct ata_queued_cmd *qc) { int rc; @@ -3503,7 +3514,7 @@ void ata_qc_complete(struct ata_queued_c qc->flags &= ~ATA_QCFLAG_ACTIVE; /* call completion callback */ - rc = qc->complete_fn(qc, err_mask); + rc = qc->complete_fn(qc); /* if callback indicates not to complete command (non-zero), * return immediately @@ -3941,7 +3952,8 @@ inline unsigned int ata_host_intr (struc ap->ops->irq_clear(ap); /* complete taskfile transaction */ - ata_qc_complete(qc, ac_err_mask(status)); + qc->err_mask |= ac_err_mask(status); + ata_qc_complete(qc); break; default: @@ -4035,13 +4047,17 @@ 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)) - goto err_out_status; + if (ata_busy_sleep(ap, ATA_TMOUT_CDB_QUICK, ATA_TMOUT_CDB)) { + qc->err_mask |= AC_ERR_ATA_BUS; + goto err_out; + } /* make sure DRQ is set */ status = ata_chk_status(ap); - if ((status & (ATA_BUSY | ATA_DRQ)) != ATA_DRQ) + if ((status & (ATA_BUSY | ATA_DRQ)) != ATA_DRQ) { + qc->err_mask |= AC_ERR_ATA_BUS; goto err_out; + } /* send SCSI cdb */ DPRINTK("send cdb\n"); @@ -4073,10 +4089,8 @@ static void atapi_packet_task(void *_dat return; -err_out_status: - status = ata_chk_status(ap); err_out: - ata_poll_qc_complete(qc, __ac_err_mask(status)); + ata_poll_qc_complete(qc); } --- linux/drivers/scsi/libata-scsi.c 2005-11-07 18:25:38.000000000 +0800 +++ err_mask/drivers/scsi/libata-scsi.c 2005-11-08 16:25:51.000000000 +0800 @@ -1218,12 +1218,11 @@ nothing_to_do: return 1; } -static int ata_scsi_qc_complete(struct ata_queued_cmd *qc, - unsigned int err_mask) +static int ata_scsi_qc_complete(struct ata_queued_cmd *qc) { struct scsi_cmnd *cmd = qc->scsicmd; u8 *cdb = cmd->cmnd; - int need_sense = (err_mask != 0); + int need_sense = (qc->err_mask != 0); /* For ATA pass thru (SAT) commands, generate a sense block if * user mandated it or if there's an error. Note that if we @@ -2016,9 +2015,10 @@ void atapi_request_sense(struct ata_port DPRINTK("EXIT\n"); } -static int atapi_qc_complete(struct ata_queued_cmd *qc, unsigned int err_mask) +static int atapi_qc_complete(struct ata_queued_cmd *qc) { struct scsi_cmnd *cmd = qc->scsicmd; + unsigned int err_mask = qc->err_mask; VPRINTK("ENTER, err_mask 0x%X\n", err_mask); --- linux/drivers/scsi/ahci.c 2005-11-07 18:25:36.000000000 +0800 +++ err_mask/drivers/scsi/ahci.c 2005-11-08 16:48:47.000000000 +0800 @@ -610,7 +610,8 @@ static void ahci_eng_timeout(struct ata_ * not being called from the SCSI EH. */ qc->scsidone = scsi_finish_command; - ata_qc_complete(qc, AC_ERR_OTHER); + qc->err_mask |= AC_ERR_OTHER; + ata_qc_complete(qc); } spin_unlock_irqrestore(&host_set->lock, flags); @@ -631,15 +632,17 @@ static inline int ahci_host_intr(struct ci = readl(port_mmio + PORT_CMD_ISSUE); if (likely((ci & 0x1) == 0)) { if (qc) { - ata_qc_complete(qc, 0); + ata_qc_complete(qc); qc = NULL; } } if (status & PORT_IRQ_FATAL) { ahci_intr_error(ap, status); - if (qc) - ata_qc_complete(qc, AC_ERR_OTHER); + if (qc) { + qc->err_mask |= AC_ERR_OTHER; + ata_qc_complete(qc); + } } return 1; --- linux/drivers/scsi/pdc_adma.c 2005-11-07 18:25:38.000000000 +0800 +++ err_mask/drivers/scsi/pdc_adma.c 2005-11-08 16:52:12.000000000 +0800 @@ -465,14 +465,12 @@ static inline unsigned int adma_intr_pkt continue; qc = ata_qc_from_tag(ap, ap->active_tag); if (qc && (!(qc->tf.ctl & ATA_NIEN))) { - unsigned int err_mask = 0; - if ((status & (aPERR | aPSD | aUIRQ))) - err_mask = AC_ERR_OTHER; + qc->err_mask |= AC_ERR_OTHER; else if (pp->pkt[0] != cDONE) - err_mask = AC_ERR_OTHER; + qc->err_mask |= AC_ERR_OTHER; - ata_qc_complete(qc, err_mask); + ata_qc_complete(qc); } } return handled; @@ -502,7 +500,8 @@ static inline unsigned int adma_intr_mmi /* complete taskfile transaction */ pp->state = adma_state_idle; - ata_qc_complete(qc, ac_err_mask(status)); + qc->err_mask |= ac_err_mask(status); + ata_qc_complete(qc); handled = 1; } } --- linux/drivers/scsi/sata_mv.c 2005-11-07 18:25:38.000000000 +0800 +++ err_mask/drivers/scsi/sata_mv.c 2005-11-08 17:09:25.000000000 +0800 @@ -1136,7 +1136,8 @@ static void mv_host_intr(struct ata_host VPRINTK("port %u IRQ found for qc, " "ata_status 0x%x\n", port,ata_status); /* mark qc status appropriately */ - ata_qc_complete(qc, err_mask); + qc->err_mask |= err_mask; + ata_qc_complete(qc); } } } @@ -1312,7 +1313,8 @@ static void mv_eng_timeout(struct ata_po */ spin_lock_irqsave(&ap->host_set->lock, flags); qc->scsidone = scsi_finish_command; - ata_qc_complete(qc, AC_ERR_OTHER); + qc->err_mask |= AC_ERR_OTHER; + ata_qc_complete(qc); spin_unlock_irqrestore(&ap->host_set->lock, flags); } } --- linux/drivers/scsi/sata_promise.c 2005-11-07 18:25:38.000000000 +0800 +++ err_mask/drivers/scsi/sata_promise.c 2005-11-08 17:10:54.000000000 +0800 @@ -401,7 +401,8 @@ static void pdc_eng_timeout(struct ata_p case ATA_PROT_NODATA: printk(KERN_ERR "ata%u: command timeout\n", ap->id); drv_stat = ata_wait_idle(ap); - ata_qc_complete(qc, __ac_err_mask(drv_stat)); + qc->err_mask |= __ac_err_mask(drv_stat); + ata_qc_complete(qc); break; default: @@ -410,7 +411,8 @@ static void pdc_eng_timeout(struct ata_p printk(KERN_ERR "ata%u: unknown timeout, cmd 0x%x stat 0x%x\n", ap->id, qc->tf.command, drv_stat); - ata_qc_complete(qc, ac_err_mask(drv_stat)); + qc->err_mask |= ac_err_mask(drv_stat); + ata_qc_complete(qc); break; } @@ -422,21 +424,21 @@ out: static inline unsigned int pdc_host_intr( struct ata_port *ap, struct ata_queued_cmd *qc) { - unsigned int handled = 0, err_mask = 0; + unsigned int handled = 0; u32 tmp; void __iomem *mmio = (void __iomem *) ap->ioaddr.cmd_addr + PDC_GLOBAL_CTL; tmp = readl(mmio); if (tmp & PDC_ERR_MASK) { - err_mask = AC_ERR_DEV; + qc->err_mask |= AC_ERR_DEV; pdc_reset_port(ap); } switch (qc->tf.protocol) { case ATA_PROT_DMA: case ATA_PROT_NODATA: - err_mask |= ac_err_mask(ata_wait_idle(ap)); - ata_qc_complete(qc, err_mask); + qc->err_mask |= ac_err_mask(ata_wait_idle(ap)); + ata_qc_complete(qc); handled = 1; break; --- linux/drivers/scsi/sata_qstor.c 2005-11-07 18:25:38.000000000 +0800 +++ err_mask/drivers/scsi/sata_qstor.c 2005-11-08 17:37:19.000000000 +0800 @@ -407,8 +407,8 @@ static inline unsigned int qs_intr_pkt(s case 3: /* device error */ pp->state = qs_state_idle; qs_enter_reg_mode(qc->ap); - ata_qc_complete(qc, - ac_err_mask(sDST)); + qc->err_mask |= ac_err_mask(sDST); + ata_qc_complete(qc); break; default: break; @@ -445,7 +445,8 @@ static inline unsigned int qs_intr_mmio( /* complete taskfile transaction */ pp->state = qs_state_idle; - ata_qc_complete(qc, ac_err_mask(status)); + qc->err_mask |= ac_err_mask(status); + ata_qc_complete(qc); handled = 1; } } --- linux/drivers/scsi/sata_sil24.c 2005-11-07 18:25:38.000000000 +0800 +++ err_mask/drivers/scsi/sata_sil24.c 2005-11-09 10:33:32.000000000 +0800 @@ -518,7 +518,8 @@ static void sil24_eng_timeout(struct ata */ printk(KERN_ERR "ata%u: command timeout\n", ap->id); qc->scsidone = scsi_finish_command; - ata_qc_complete(qc, AC_ERR_OTHER); + qc->err_mask |= AC_ERR_OTHER; + ata_qc_complete(qc); sil24_reset_controller(ap); } @@ -567,8 +568,10 @@ static void sil24_error_intr(struct ata_ err_mask = AC_ERR_OTHER; } - if (qc) - ata_qc_complete(qc, err_mask); + if (qc) { + qc->err_mask |= err_mask; + ata_qc_complete(qc); + } sil24_reset_controller(ap); } @@ -592,8 +595,10 @@ static inline void sil24_host_intr(struc */ sil24_update_tf(ap); - if (qc) - ata_qc_complete(qc, ac_err_mask(pp->tf.command)); + if (qc) { + qc->err_mask |= ac_err_mask(pp->tf.command); + ata_qc_complete(qc); + } } else sil24_error_intr(ap, slot_stat); } --- linux/drivers/scsi/sata_sx4.c 2005-11-07 18:25:38.000000000 +0800 +++ err_mask/drivers/scsi/sata_sx4.c 2005-11-08 17:00:45.000000000 +0800 @@ -718,7 +718,8 @@ static inline unsigned int pdc20621_host VPRINTK("ata%u: read hdma, 0x%x 0x%x\n", ap->id, readl(mmio + 0x104), readl(mmio + PDC_HDMA_CTLSTAT)); /* get drive status; clear intr; complete txn */ - ata_qc_complete(qc, ac_err_mask(ata_wait_idle(ap))); + qc->err_mask |= ac_err_mask(ata_wait_idle(ap)); + ata_qc_complete(qc); pdc20621_pop_hdma(qc); } @@ -756,7 +757,8 @@ static inline unsigned int pdc20621_host VPRINTK("ata%u: write ata, 0x%x 0x%x\n", ap->id, readl(mmio + 0x104), readl(mmio + PDC_HDMA_CTLSTAT)); /* get drive status; clear intr; complete txn */ - ata_qc_complete(qc, ac_err_mask(ata_wait_idle(ap))); + qc->err_mask |= ac_err_mask(ata_wait_idle(ap)); + ata_qc_complete(qc); pdc20621_pop_hdma(qc); } handled = 1; @@ -766,7 +768,8 @@ static inline unsigned int pdc20621_host status = ata_busy_wait(ap, ATA_BUSY | ATA_DRQ, 1000); DPRINTK("BUS_NODATA (drv_stat 0x%X)\n", status); - ata_qc_complete(qc, ac_err_mask(status)); + qc->err_mask |= ac_err_mask(status); + ata_qc_complete(qc); handled = 1; } else { @@ -881,7 +884,8 @@ static void pdc_eng_timeout(struct ata_p case ATA_PROT_DMA: case ATA_PROT_NODATA: printk(KERN_ERR "ata%u: command timeout\n", ap->id); - ata_qc_complete(qc, __ac_err_mask(ata_wait_idle(ap))); + qc->err_mask |= __ac_err_mask(ata_wait_idle(ap)); + ata_qc_complete(qc); break; default: @@ -890,7 +894,8 @@ static void pdc_eng_timeout(struct ata_p printk(KERN_ERR "ata%u: unknown timeout, cmd 0x%x stat 0x%x\n", ap->id, qc->tf.command, drv_stat); - ata_qc_complete(qc, ac_err_mask(drv_stat)); + qc->err_mask |= ac_err_mask(drv_stat); + ata_qc_complete(qc); break; } ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH/RFC 2/3] libata: move err_mask to ata_queued_cmd 2005-11-09 5:07 ` [PATCH/RFC 2/3] libata: move err_mask to ata_queued_cmd Albert Lee @ 2005-11-09 6:25 ` Jeff Garzik 2005-11-10 7:56 ` [PATCH/RFC 0/4] libata: move err_mask to ata_queued_cmd (revised) Albert Lee 0 siblings, 1 reply; 19+ messages in thread From: Jeff Garzik @ 2005-11-09 6:25 UTC (permalink / raw) To: Albert Lee; +Cc: Linux IDE, Bartlomiej Zolnierkiewicz, Doug Maxey, Tejun Heo Albert Lee wrote: > Patch 2/3: > move err_mask to ata_queued_cmd. > > Changes: > - Move the err_mask parameter of the complete functions to ata_queued_cmd. > This can make the err_mask more accessible outside the complete functions; > also makes the HSM easier to pass err_mask between states. > > For your review and advice, thanks. > > Albert > Signed-off-by: Albert Lee <albertcc@tw.ibm.com> This patch is OK in principle, but I would like it split up some more. In particular, this patch seems to have mixed in some related changes along with the single "more err_mask to ata_queued_cmd" change itself. Jeff ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH/RFC 0/4] libata: move err_mask to ata_queued_cmd (revised) 2005-11-09 6:25 ` Jeff Garzik @ 2005-11-10 7:56 ` Albert Lee 2005-11-10 7:59 ` [PATCH/RFC 1/4] libata: minor patch before moving err_mask Albert Lee ` (4 more replies) 0 siblings, 5 replies; 19+ messages in thread From: Albert Lee @ 2005-11-10 7:56 UTC (permalink / raw) To: Jeff Garzik; +Cc: Linux IDE, Bartlomiej Zolnierkiewicz, Doug Maxey, Tejun Heo Dear all, Description: Move the err_mask parameter of the complete functions to ata_queued_cmd. This can make the err_mask more accessible outside the complete functions; also can make the HSM easier to determine and pass err_mask between states. patch 1/4: minor patch to pave the road for patch 2 and 3 patch 2/4: move err_mask to ata_queued_cmd patch 3/4: determine the err_mask when the error is found patch 4/4: determine the err_mask directly in atapi_packet_task() Revised patch against the mainline tree (e4d76e1c0b15590f2ad9bba89426c2520cd22ca6). For your review and advice, thanks. Albert ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH/RFC 1/4] libata: minor patch before moving err_mask 2005-11-10 7:56 ` [PATCH/RFC 0/4] libata: move err_mask to ata_queued_cmd (revised) Albert Lee @ 2005-11-10 7:59 ` Albert Lee 2005-11-10 8:01 ` [PATCH/RFC 2/4] libata: move err_mask to ata_queued_cmd Albert Lee ` (3 subsequent siblings) 4 siblings, 0 replies; 19+ messages in thread From: Albert Lee @ 2005-11-10 7:59 UTC (permalink / raw) To: Jeff Garzik; +Cc: Linux IDE, Bartlomiej Zolnierkiewicz, Doug Maxey, Tejun Heo Patch 1/4: minor patch before moving the err_mask Changes: - add qc to ata_pio_poll() - reorder the initialization of qc in ata_pio_complete() For your review and advice, thanks. Albert Signed-off-by: Albert Lee <albertcc@tw.ibm.com> ============ --- linux/drivers/scsi/libata-core.c 2005-11-10 11:03:03.000000000 +0800 +++ err_mask1/drivers/scsi/libata-core.c 2005-11-10 11:13:54.000000000 +0800 @@ -2744,10 +2744,14 @@ void ata_poll_qc_complete(struct ata_que static unsigned long ata_pio_poll(struct ata_port *ap) { + struct ata_queued_cmd *qc; u8 status; unsigned int poll_state = HSM_ST_UNKNOWN; unsigned int reg_state = HSM_ST_UNKNOWN; + qc = ata_qc_from_tag(ap, ap->active_tag); + assert(qc != NULL); + switch (ap->hsm_task_state) { case HSM_ST: case HSM_ST_POLL: @@ -2812,15 +2816,15 @@ static int ata_pio_complete (struct ata_ } } + qc = ata_qc_from_tag(ap, ap->active_tag); + assert(qc != NULL); + drv_stat = ata_wait_idle(ap); if (!ata_ok(drv_stat)) { ap->hsm_task_state = HSM_ST_ERR; return 0; } - qc = ata_qc_from_tag(ap, ap->active_tag); - assert(qc != NULL); - ap->hsm_task_state = HSM_ST_IDLE; ata_poll_qc_complete(qc, 0); ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH/RFC 2/4] libata: move err_mask to ata_queued_cmd 2005-11-10 7:56 ` [PATCH/RFC 0/4] libata: move err_mask to ata_queued_cmd (revised) Albert Lee 2005-11-10 7:59 ` [PATCH/RFC 1/4] libata: minor patch before moving err_mask Albert Lee @ 2005-11-10 8:01 ` Albert Lee 2005-11-10 8:03 ` [PATCH/RFC 3/4] libata: determine the err_mask when the error is found Albert Lee ` (2 subsequent siblings) 4 siblings, 0 replies; 19+ messages in thread From: Albert Lee @ 2005-11-10 8:01 UTC (permalink / raw) To: Jeff Garzik; +Cc: Linux IDE, Bartlomiej Zolnierkiewicz, Doug Maxey, Tejun Heo Patch 2/4: move err_mask to ata_queued_cmd Changes: - remove err_mask from the parameter list of the complete functions - move err_mask to ata_queued_cmd - initialize qc->err_mask when needed - for each function call to ata_qc_complete(), replace the err_mask parameter with qc->err_mask. For your review and advice, thanks. Albert Signed-off-by: Albert Lee <albertcc@tw.ibm.com> ============ --- linux/include/linux/libata.h 2005-11-10 11:03:23.000000000 +0800 +++ err_mask2/include/linux/libata.h 2005-11-10 11:08:42.000000000 +0800 @@ -190,7 +190,7 @@ struct ata_port; struct ata_queued_cmd; /* typedefs */ -typedef int (*ata_qc_cb_t) (struct ata_queued_cmd *qc, unsigned int err_mask); +typedef int (*ata_qc_cb_t) (struct ata_queued_cmd *qc); struct ata_ioports { unsigned long cmd_addr; @@ -275,6 +275,8 @@ struct ata_queued_cmd { /* DO NOT iterate over __sg manually, use ata_for_each_sg() */ struct scatterlist *__sg; + unsigned int err_mask; + ata_qc_cb_t complete_fn; struct completion *waiting; @@ -472,7 +474,7 @@ extern void ata_bmdma_start (struct ata_ extern void ata_bmdma_stop(struct ata_queued_cmd *qc); extern u8 ata_bmdma_status(struct ata_port *ap); extern void ata_bmdma_irq_clear(struct ata_port *ap); -extern void ata_qc_complete(struct ata_queued_cmd *qc, unsigned int err_mask); +extern void ata_qc_complete(struct ata_queued_cmd *qc); extern void ata_eng_timeout(struct ata_port *ap); extern void ata_scsi_simulate(u16 *id, struct scsi_cmnd *cmd, void (*done)(struct scsi_cmnd *)); --- linux/drivers/scsi/libata.h 2005-11-10 11:03:03.000000000 +0800 +++ err_mask2/drivers/scsi/libata.h 2005-11-10 11:09:01.000000000 +0800 @@ -39,7 +39,7 @@ 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 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); --- err_mask1/drivers/scsi/libata-core.c 2005-11-10 11:13:54.000000000 +0800 +++ err_mask2/drivers/scsi/libata-core.c 2005-11-10 11:28:51.000000000 +0800 @@ -1152,6 +1152,7 @@ retry: qc->cursg_ofs = 0; qc->cursect = 0; qc->nsect = 1; + qc->err_mask = 0; goto retry; } } @@ -2719,7 +2720,7 @@ static int ata_sg_setup(struct ata_queue * None. (grabs host lock) */ -void ata_poll_qc_complete(struct ata_queued_cmd *qc, unsigned int err_mask) +void ata_poll_qc_complete(struct ata_queued_cmd *qc) { struct ata_port *ap = qc->ap; unsigned long flags; @@ -2727,7 +2728,7 @@ void ata_poll_qc_complete(struct ata_que spin_lock_irqsave(&ap->host_set->lock, flags); ap->flags &= ~ATA_FLAG_NOINTR; ata_irq_on(ap); - ata_qc_complete(qc, err_mask); + ata_qc_complete(qc); spin_unlock_irqrestore(&ap->host_set->lock, flags); } @@ -2827,7 +2828,8 @@ static int ata_pio_complete (struct ata_ ap->hsm_task_state = HSM_ST_IDLE; - ata_poll_qc_complete(qc, 0); + assert(qc->err_mask == 0); + ata_poll_qc_complete(qc); /* another command may start at this point */ @@ -3203,7 +3205,8 @@ static void ata_pio_error(struct ata_por ap->hsm_task_state = HSM_ST_IDLE; - ata_poll_qc_complete(qc, AC_ERR_ATA_BUS); + qc->err_mask |= AC_ERR_ATA_BUS; + ata_poll_qc_complete(qc); } static void ata_pio_task(void *_data) @@ -3326,7 +3329,8 @@ static void ata_qc_timeout(struct ata_qu ap->id, qc->tf.command, drv_stat, host_stat); /* complete taskfile transaction */ - ata_qc_complete(qc, ac_err_mask(drv_stat)); + qc->err_mask |= ac_err_mask(drv_stat); + ata_qc_complete(qc); break; } @@ -3424,6 +3428,7 @@ struct ata_queued_cmd *ata_qc_new_init(s qc->cursect = qc->cursg = qc->cursg_ofs = 0; qc->nsect = 0; qc->nbytes = qc->curbytes = 0; + qc->err_mask = 0; ata_tf_init(ap, &qc->tf, dev->devno); } @@ -3431,7 +3436,7 @@ struct ata_queued_cmd *ata_qc_new_init(s return qc; } -int ata_qc_complete_noop(struct ata_queued_cmd *qc, unsigned int err_mask) +int ata_qc_complete_noop(struct ata_queued_cmd *qc) { return 0; } @@ -3490,7 +3495,7 @@ void ata_qc_free(struct ata_queued_cmd * * spin_lock_irqsave(host_set lock) */ -void ata_qc_complete(struct ata_queued_cmd *qc, unsigned int err_mask) +void ata_qc_complete(struct ata_queued_cmd *qc) { int rc; @@ -3507,7 +3512,7 @@ void ata_qc_complete(struct ata_queued_c qc->flags &= ~ATA_QCFLAG_ACTIVE; /* call completion callback */ - rc = qc->complete_fn(qc, err_mask); + rc = qc->complete_fn(qc); /* if callback indicates not to complete command (non-zero), * return immediately @@ -3945,7 +3950,8 @@ inline unsigned int ata_host_intr (struc ap->ops->irq_clear(ap); /* complete taskfile transaction */ - ata_qc_complete(qc, ac_err_mask(status)); + qc->err_mask |= ac_err_mask(status); + ata_qc_complete(qc); break; default: @@ -4080,7 +4086,8 @@ static void atapi_packet_task(void *_dat err_out_status: status = ata_chk_status(ap); err_out: - ata_poll_qc_complete(qc, __ac_err_mask(status)); + qc->err_mask |= __ac_err_mask(status); + ata_poll_qc_complete(qc); } --- linux/drivers/scsi/libata-scsi.c 2005-11-10 11:03:03.000000000 +0800 +++ err_mask2/drivers/scsi/libata-scsi.c 2005-11-10 11:25:13.000000000 +0800 @@ -1219,12 +1219,11 @@ nothing_to_do: return 1; } -static int ata_scsi_qc_complete(struct ata_queued_cmd *qc, - unsigned int err_mask) +static int ata_scsi_qc_complete(struct ata_queued_cmd *qc) { struct scsi_cmnd *cmd = qc->scsicmd; u8 *cdb = cmd->cmnd; - int need_sense = (err_mask != 0); + int need_sense = (qc->err_mask != 0); /* For ATA pass thru (SAT) commands, generate a sense block if * user mandated it or if there's an error. Note that if we @@ -2017,9 +2016,10 @@ void atapi_request_sense(struct ata_port DPRINTK("EXIT\n"); } -static int atapi_qc_complete(struct ata_queued_cmd *qc, unsigned int err_mask) +static int atapi_qc_complete(struct ata_queued_cmd *qc) { struct scsi_cmnd *cmd = qc->scsicmd; + unsigned int err_mask = qc->err_mask; VPRINTK("ENTER, err_mask 0x%X\n", err_mask); --- linux/drivers/scsi/ahci.c 2005-11-10 11:03:03.000000000 +0800 +++ err_mask2/drivers/scsi/ahci.c 2005-11-10 11:32:02.000000000 +0800 @@ -610,7 +610,8 @@ static void ahci_eng_timeout(struct ata_ * not being called from the SCSI EH. */ qc->scsidone = scsi_finish_command; - ata_qc_complete(qc, AC_ERR_OTHER); + qc->err_mask |= AC_ERR_OTHER; + ata_qc_complete(qc); } spin_unlock_irqrestore(&host_set->lock, flags); @@ -631,15 +632,18 @@ static inline int ahci_host_intr(struct ci = readl(port_mmio + PORT_CMD_ISSUE); if (likely((ci & 0x1) == 0)) { if (qc) { - ata_qc_complete(qc, 0); + assert(qc->err_mask == 0); + ata_qc_complete(qc); qc = NULL; } } if (status & PORT_IRQ_FATAL) { ahci_intr_error(ap, status); - if (qc) - ata_qc_complete(qc, AC_ERR_OTHER); + if (qc) { + qc->err_mask |= AC_ERR_OTHER; + ata_qc_complete(qc); + } } return 1; --- linux/drivers/scsi/pdc_adma.c 2005-11-10 11:03:04.000000000 +0800 +++ err_mask2/drivers/scsi/pdc_adma.c 2005-11-10 12:30:16.000000000 +0800 @@ -464,14 +464,12 @@ static inline unsigned int adma_intr_pkt continue; qc = ata_qc_from_tag(ap, ap->active_tag); if (qc && (!(qc->tf.ctl & ATA_NIEN))) { - unsigned int err_mask = 0; - if ((status & (aPERR | aPSD | aUIRQ))) - err_mask = AC_ERR_OTHER; + qc->err_mask |= AC_ERR_OTHER; else if (pp->pkt[0] != cDONE) - err_mask = AC_ERR_OTHER; + qc->err_mask |= AC_ERR_OTHER; - ata_qc_complete(qc, err_mask); + ata_qc_complete(qc); } } return handled; @@ -501,7 +499,8 @@ static inline unsigned int adma_intr_mmi /* complete taskfile transaction */ pp->state = adma_state_idle; - ata_qc_complete(qc, ac_err_mask(status)); + qc->err_mask |= ac_err_mask(status); + ata_qc_complete(qc); handled = 1; } } --- linux/drivers/scsi/sata_mv.c 2005-11-10 11:03:04.000000000 +0800 +++ err_mask2/drivers/scsi/sata_mv.c 2005-11-10 11:35:19.000000000 +0800 @@ -1136,7 +1136,8 @@ static void mv_host_intr(struct ata_host VPRINTK("port %u IRQ found for qc, " "ata_status 0x%x\n", port,ata_status); /* mark qc status appropriately */ - ata_qc_complete(qc, err_mask); + qc->err_mask |= err_mask; + ata_qc_complete(qc); } } } @@ -1312,7 +1313,8 @@ static void mv_eng_timeout(struct ata_po */ spin_lock_irqsave(&ap->host_set->lock, flags); qc->scsidone = scsi_finish_command; - ata_qc_complete(qc, AC_ERR_OTHER); + qc->err_mask |= AC_ERR_OTHER; + ata_qc_complete(qc); spin_unlock_irqrestore(&ap->host_set->lock, flags); } } --- linux/drivers/scsi/sata_promise.c 2005-11-10 11:03:04.000000000 +0800 +++ err_mask2/drivers/scsi/sata_promise.c 2005-11-10 12:43:52.000000000 +0800 @@ -401,7 +401,8 @@ static void pdc_eng_timeout(struct ata_p case ATA_PROT_NODATA: printk(KERN_ERR "ata%u: command timeout\n", ap->id); drv_stat = ata_wait_idle(ap); - ata_qc_complete(qc, __ac_err_mask(drv_stat)); + qc->err_mask |= __ac_err_mask(drv_stat); + ata_qc_complete(qc); break; default: @@ -410,7 +411,8 @@ static void pdc_eng_timeout(struct ata_p printk(KERN_ERR "ata%u: unknown timeout, cmd 0x%x stat 0x%x\n", ap->id, qc->tf.command, drv_stat); - ata_qc_complete(qc, ac_err_mask(drv_stat)); + qc->err_mask |= ac_err_mask(drv_stat); + ata_qc_complete(qc); break; } @@ -422,21 +424,21 @@ out: static inline unsigned int pdc_host_intr( struct ata_port *ap, struct ata_queued_cmd *qc) { - unsigned int handled = 0, err_mask = 0; + unsigned int handled = 0; u32 tmp; void __iomem *mmio = (void __iomem *) ap->ioaddr.cmd_addr + PDC_GLOBAL_CTL; tmp = readl(mmio); if (tmp & PDC_ERR_MASK) { - err_mask = AC_ERR_DEV; + qc->err_mask |= AC_ERR_DEV; pdc_reset_port(ap); } switch (qc->tf.protocol) { case ATA_PROT_DMA: case ATA_PROT_NODATA: - err_mask |= ac_err_mask(ata_wait_idle(ap)); - ata_qc_complete(qc, err_mask); + qc->err_mask |= ac_err_mask(ata_wait_idle(ap)); + ata_qc_complete(qc); handled = 1; break; --- linux/drivers/scsi/sata_qstor.c 2005-11-10 11:03:04.000000000 +0800 +++ err_mask2/drivers/scsi/sata_qstor.c 2005-11-10 11:38:29.000000000 +0800 @@ -406,8 +406,8 @@ static inline unsigned int qs_intr_pkt(s case 3: /* device error */ pp->state = qs_state_idle; qs_enter_reg_mode(qc->ap); - ata_qc_complete(qc, - ac_err_mask(sDST)); + qc->err_mask |= ac_err_mask(sDST); + ata_qc_complete(qc); break; default: break; @@ -444,7 +444,8 @@ static inline unsigned int qs_intr_mmio( /* complete taskfile transaction */ pp->state = qs_state_idle; - ata_qc_complete(qc, ac_err_mask(status)); + qc->err_mask |= ac_err_mask(status); + ata_qc_complete(qc); handled = 1; } } --- linux/drivers/scsi/sata_sil24.c 2005-11-10 11:03:04.000000000 +0800 +++ err_mask2/drivers/scsi/sata_sil24.c 2005-11-10 11:41:38.000000000 +0800 @@ -518,7 +518,8 @@ static void sil24_eng_timeout(struct ata */ printk(KERN_ERR "ata%u: command timeout\n", ap->id); qc->scsidone = scsi_finish_command; - ata_qc_complete(qc, AC_ERR_OTHER); + qc->err_mask |= AC_ERR_OTHER; + ata_qc_complete(qc); sil24_reset_controller(ap); } @@ -529,7 +530,6 @@ static void sil24_error_intr(struct ata_ struct sil24_port_priv *pp = ap->private_data; void __iomem *port = (void __iomem *)ap->ioaddr.cmd_addr; u32 irq_stat, cmd_err, sstatus, serror; - unsigned int err_mask; irq_stat = readl(port + PORT_IRQ_STAT); writel(irq_stat, port + PORT_IRQ_STAT); /* clear irq */ @@ -557,18 +557,18 @@ static void sil24_error_intr(struct ata_ * Device is reporting error, tf registers are valid. */ sil24_update_tf(ap); - err_mask = ac_err_mask(pp->tf.command); + qc->err_mask |= ac_err_mask(pp->tf.command); } else { /* * Other errors. libata currently doesn't have any * mechanism to report these errors. Just turn on * ATA_ERR. */ - err_mask = AC_ERR_OTHER; + qc->err_mask |= AC_ERR_OTHER; } if (qc) - ata_qc_complete(qc, err_mask); + ata_qc_complete(qc); sil24_reset_controller(ap); } @@ -592,8 +592,10 @@ static inline void sil24_host_intr(struc */ sil24_update_tf(ap); - if (qc) - ata_qc_complete(qc, ac_err_mask(pp->tf.command)); + if (qc) { + qc->err_mask |= ac_err_mask(pp->tf.command); + ata_qc_complete(qc); + } } else sil24_error_intr(ap, slot_stat); } --- linux/drivers/scsi/sata_sx4.c 2005-11-10 11:03:04.000000000 +0800 +++ err_mask2/drivers/scsi/sata_sx4.c 2005-11-10 11:43:38.000000000 +0800 @@ -718,7 +718,8 @@ static inline unsigned int pdc20621_host VPRINTK("ata%u: read hdma, 0x%x 0x%x\n", ap->id, readl(mmio + 0x104), readl(mmio + PDC_HDMA_CTLSTAT)); /* get drive status; clear intr; complete txn */ - ata_qc_complete(qc, ac_err_mask(ata_wait_idle(ap))); + qc->err_mask |= ac_err_mask(ata_wait_idle(ap)); + ata_qc_complete(qc); pdc20621_pop_hdma(qc); } @@ -756,7 +757,8 @@ static inline unsigned int pdc20621_host VPRINTK("ata%u: write ata, 0x%x 0x%x\n", ap->id, readl(mmio + 0x104), readl(mmio + PDC_HDMA_CTLSTAT)); /* get drive status; clear intr; complete txn */ - ata_qc_complete(qc, ac_err_mask(ata_wait_idle(ap))); + qc->err_mask |= ac_err_mask(ata_wait_idle(ap)); + ata_qc_complete(qc); pdc20621_pop_hdma(qc); } handled = 1; @@ -766,7 +768,8 @@ static inline unsigned int pdc20621_host status = ata_busy_wait(ap, ATA_BUSY | ATA_DRQ, 1000); DPRINTK("BUS_NODATA (drv_stat 0x%X)\n", status); - ata_qc_complete(qc, ac_err_mask(status)); + qc->err_mask |= ac_err_mask(status); + ata_qc_complete(qc); handled = 1; } else { @@ -881,7 +884,8 @@ static void pdc_eng_timeout(struct ata_p case ATA_PROT_DMA: case ATA_PROT_NODATA: printk(KERN_ERR "ata%u: command timeout\n", ap->id); - ata_qc_complete(qc, __ac_err_mask(ata_wait_idle(ap))); + qc->err_mask |= __ac_err_mask(ata_wait_idle(ap)); + ata_qc_complete(qc); break; default: @@ -890,7 +894,8 @@ static void pdc_eng_timeout(struct ata_p printk(KERN_ERR "ata%u: unknown timeout, cmd 0x%x stat 0x%x\n", ap->id, qc->tf.command, drv_stat); - ata_qc_complete(qc, ac_err_mask(drv_stat)); + qc->err_mask |= ac_err_mask(drv_stat); + ata_qc_complete(qc); break; } ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH/RFC 3/4] libata: determine the err_mask when the error is found 2005-11-10 7:56 ` [PATCH/RFC 0/4] libata: move err_mask to ata_queued_cmd (revised) Albert Lee 2005-11-10 7:59 ` [PATCH/RFC 1/4] libata: minor patch before moving err_mask Albert Lee 2005-11-10 8:01 ` [PATCH/RFC 2/4] libata: move err_mask to ata_queued_cmd Albert Lee @ 2005-11-10 8:03 ` Albert Lee 2005-11-10 8:05 ` [PATCH/RFC 4/4] libata: determine the err_mask directly in atapi_packet_task() Albert Lee 2005-12-04 2:01 ` [PATCH/RFC 0/4] libata: move err_mask to ata_queued_cmd (revised) Jeff Garzik 4 siblings, 0 replies; 19+ messages in thread From: Albert Lee @ 2005-11-10 8:03 UTC (permalink / raw) To: Jeff Garzik; +Cc: Linux IDE, Bartlomiej Zolnierkiewicz, Doug Maxey, Tejun Heo Patch 3/4: determine the err_mask when the error is found Changes: - move "qc->err_mask |= AC_ERR_ATA_BUS" to where the error is found - add "assert(qc->err_mask)" to ata_pio_error() to make sure qc->err_mask was available when we enter the error state For your review and advice, thanks. Albert Signed-off-by: Albert Lee <albertcc@tw.ibm.com> ============ --- err_mask2/drivers/scsi/libata-core.c 2005-11-10 11:28:51.000000000 +0800 +++ err_mask3/drivers/scsi/libata-core.c 2005-11-10 15:09:52.000000000 +0800 @@ -2772,6 +2772,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; ap->hsm_task_state = HSM_ST_TMOUT; return 0; } @@ -2822,6 +2823,7 @@ static int ata_pio_complete (struct ata_ drv_stat = ata_wait_idle(ap); if (!ata_ok(drv_stat)) { + qc->err_mask |= AC_ERR_ATA_BUS; ap->hsm_task_state = HSM_ST_ERR; return 0; } @@ -3137,6 +3139,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; ap->hsm_task_state = HSM_ST_ERR; } @@ -3186,6 +3189,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; ap->hsm_task_state = HSM_ST_ERR; return; } @@ -3203,9 +3207,13 @@ static void ata_pio_error(struct ata_por qc = ata_qc_from_tag(ap, ap->active_tag); assert(qc != NULL); + /* make sure qc->err_mask was available to + * know about what's going wrong and recover + */ + assert(qc->err_mask); + ap->hsm_task_state = HSM_ST_IDLE; - qc->err_mask |= AC_ERR_ATA_BUS; ata_poll_qc_complete(qc); } ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH/RFC 4/4] libata: determine the err_mask directly in atapi_packet_task() 2005-11-10 7:56 ` [PATCH/RFC 0/4] libata: move err_mask to ata_queued_cmd (revised) Albert Lee ` (2 preceding siblings ...) 2005-11-10 8:03 ` [PATCH/RFC 3/4] libata: determine the err_mask when the error is found Albert Lee @ 2005-11-10 8:05 ` Albert Lee 2005-12-04 2:01 ` [PATCH/RFC 0/4] libata: move err_mask to ata_queued_cmd (revised) Jeff Garzik 4 siblings, 0 replies; 19+ messages in thread From: Albert Lee @ 2005-11-10 8:05 UTC (permalink / raw) To: Jeff Garzik; +Cc: Linux IDE, Bartlomiej Zolnierkiewicz, Doug Maxey, Tejun Heo Patch 4/4: determine the err_mask directly in atapi_packet_task() Changes: - set qc->err_mask directly when we found the error - remove the code to determine err_mask from the device status For your review and advice, thanks. Albert Signed-off-by: Albert Lee <albertcc@tw.ibm.com> ============ --- err_mask3/drivers/scsi/libata-core.c 2005-11-10 15:09:52.000000000 +0800 +++ err_mask4/drivers/scsi/libata-core.c 2005-11-10 15:10:13.000000000 +0800 @@ -4053,13 +4053,17 @@ 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)) - goto err_out_status; + if (ata_busy_sleep(ap, ATA_TMOUT_CDB_QUICK, ATA_TMOUT_CDB)) { + qc->err_mask |= AC_ERR_ATA_BUS; + goto err_out; + } /* make sure DRQ is set */ status = ata_chk_status(ap); - if ((status & (ATA_BUSY | ATA_DRQ)) != ATA_DRQ) + if ((status & (ATA_BUSY | ATA_DRQ)) != ATA_DRQ) { + qc->err_mask |= AC_ERR_ATA_BUS; goto err_out; + } /* send SCSI cdb */ DPRINTK("send cdb\n"); @@ -4091,10 +4095,7 @@ static void atapi_packet_task(void *_dat return; -err_out_status: - status = ata_chk_status(ap); err_out: - qc->err_mask |= __ac_err_mask(status); ata_poll_qc_complete(qc); } ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH/RFC 0/4] libata: move err_mask to ata_queued_cmd (revised) 2005-11-10 7:56 ` [PATCH/RFC 0/4] libata: move err_mask to ata_queued_cmd (revised) Albert Lee ` (3 preceding siblings ...) 2005-11-10 8:05 ` [PATCH/RFC 4/4] libata: determine the err_mask directly in atapi_packet_task() Albert Lee @ 2005-12-04 2:01 ` Jeff Garzik 2005-12-05 6:58 ` [PATCH 0/4] libata: move err_mask to ata_queued_cmd (revised #2) Albert Lee 4 siblings, 1 reply; 19+ messages in thread From: Jeff Garzik @ 2005-12-04 2:01 UTC (permalink / raw) To: Albert Lee; +Cc: Linux IDE, Bartlomiej Zolnierkiewicz, Doug Maxey, Tejun Heo Albert Lee wrote: > Dear all, > > Description: > Move the err_mask parameter of the complete functions to ata_queued_cmd. > This can make the err_mask more accessible outside the complete functions; > also can make the HSM easier to determine and pass err_mask between states. > > patch 1/4: minor patch to pave the road for patch 2 and 3 > patch 2/4: move err_mask to ata_queued_cmd > patch 3/4: determine the err_mask when the error is found > patch 4/4: determine the err_mask directly in atapi_packet_task() > > Revised patch against the mainline tree > (e4d76e1c0b15590f2ad9bba89426c2520cd22ca6). These patches look good, and I'm finally ready to apply them to the 'upstream' branch. Would you be kind enough to rediff and resend? Since I waited a while (my fault!), the second patch doesn't seem to apply anymore: [jgarzik@pretzel libata-dev]$ git-applymbox /g/tmp/mbox ~/info/signoff.txt 4 patch(es) to process. Applying 'libata: minor patch before moving err_mask' Wrote tree 6db1429964448401102718bb7ebcb2a7b2efae14 Committed: be7766250023702e65fab822ecf6a8b805028036 Applying 'libata: move err_mask to ata_queued_cmd' error: patch failed: drivers/scsi/libata-core.c:3424 error: drivers/scsi/libata-core.c: patch does not apply error: patch failed: drivers/scsi/ahci.c:631 error: drivers/scsi/ahci.c: patch does not apply error: patch failed: drivers/scsi/sata_mv.c:1136 error: drivers/scsi/sata_mv.c: patch does not apply error: patch failed: drivers/scsi/sata_sil24.c:557 error: drivers/scsi/sata_sil24.c: patch does not apply ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 0/4] libata: move err_mask to ata_queued_cmd (revised #2) 2005-12-04 2:01 ` [PATCH/RFC 0/4] libata: move err_mask to ata_queued_cmd (revised) Jeff Garzik @ 2005-12-05 6:58 ` Albert Lee 2005-12-05 7:36 ` [PATCH 1/4] libata: minor patch before moving err_mask Albert Lee ` (4 more replies) 0 siblings, 5 replies; 19+ messages in thread From: Albert Lee @ 2005-12-05 6:58 UTC (permalink / raw) To: Jeff Garzik; +Cc: Linux IDE, Bartlomiej Zolnierkiewicz, Doug Maxey, Tejun Heo Dear all, Description: Move the err_mask parameter of the complete functions to ata_queued_cmd. This can make the err_mask more accessible outside the complete functions; also can make the HSM easier to determine and pass err_mask between states. patch 1/4: minor patch to pave the road for patch 2 and 3 patch 2/4: move err_mask to ata_queued_cmd patch 3/4: determine the err_mask when the error is found patch 4/4: determine the err_mask directly in atapi_packet_task() Revised patch against the upstream branch of libata-dev tree (4ef679e6caf1261b6380a610a705a90d7e2738c6). For your review, thanks. Albert ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 1/4] libata: minor patch before moving err_mask 2005-12-05 6:58 ` [PATCH 0/4] libata: move err_mask to ata_queued_cmd (revised #2) Albert Lee @ 2005-12-05 7:36 ` Albert Lee 2005-12-05 7:38 ` [PATCH 2/4] libata: move err_mask to ata_queued_cmd Albert Lee ` (3 subsequent siblings) 4 siblings, 0 replies; 19+ messages in thread From: Albert Lee @ 2005-12-05 7:36 UTC (permalink / raw) To: Jeff Garzik; +Cc: Linux IDE, Bartlomiej Zolnierkiewicz, Doug Maxey, Tejun Heo Patch 1/4: minor patch before moving the err_mask Changes: - add qc to ata_pio_poll() - reorder the initialization of qc in ata_pio_complete() For your review, thanks. Albert Signed-off-by: Albert Lee <albertcc@tw.ibm.com> =================== --- upstream/drivers/scsi/libata-core.c 2005-12-05 10:13:48.000000000 +0800 +++ err_mask1/drivers/scsi/libata-core.c 2005-12-05 12:32:33.000000000 +0800 @@ -2802,10 +2802,14 @@ void ata_poll_qc_complete(struct ata_que static unsigned long ata_pio_poll(struct ata_port *ap) { + struct ata_queued_cmd *qc; u8 status; unsigned int poll_state = HSM_ST_UNKNOWN; unsigned int reg_state = HSM_ST_UNKNOWN; + qc = ata_qc_from_tag(ap, ap->active_tag); + assert(qc != NULL); + switch (ap->hsm_task_state) { case HSM_ST: case HSM_ST_POLL: @@ -2870,15 +2874,15 @@ static int ata_pio_complete (struct ata_ } } + qc = ata_qc_from_tag(ap, ap->active_tag); + assert(qc != NULL); + drv_stat = ata_wait_idle(ap); if (!ata_ok(drv_stat)) { ap->hsm_task_state = HSM_ST_ERR; return 0; } - qc = ata_qc_from_tag(ap, ap->active_tag); - assert(qc != NULL); - ap->hsm_task_state = HSM_ST_IDLE; ata_poll_qc_complete(qc, 0); ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 2/4] libata: move err_mask to ata_queued_cmd 2005-12-05 6:58 ` [PATCH 0/4] libata: move err_mask to ata_queued_cmd (revised #2) Albert Lee 2005-12-05 7:36 ` [PATCH 1/4] libata: minor patch before moving err_mask Albert Lee @ 2005-12-05 7:38 ` Albert Lee 2005-12-05 7:40 ` [PATCH 3/4] libata: determine the err_mask when the error is found Albert Lee ` (2 subsequent siblings) 4 siblings, 0 replies; 19+ messages in thread From: Albert Lee @ 2005-12-05 7:38 UTC (permalink / raw) To: Jeff Garzik; +Cc: Linux IDE, Bartlomiej Zolnierkiewicz, Doug Maxey, Tejun Heo Patch 2/4: move err_mask to ata_queued_cmd Changes: - remove err_mask from the parameter list of the complete functions - move err_mask to ata_queued_cmd - initialize qc->err_mask when needed - for each function call to ata_qc_complete(), replace the err_mask parameter with qc->err_mask. For your review, thanks. Albert Signed-off-by: Albert Lee <albertcc@tw.ibm.com> =============== --- upstream/include/linux/libata.h 2005-12-05 10:13:59.000000000 +0800 +++ err_mask2/include/linux/libata.h 2005-12-05 10:30:34.000000000 +0800 @@ -194,7 +194,7 @@ struct ata_port; struct ata_queued_cmd; /* typedefs */ -typedef int (*ata_qc_cb_t) (struct ata_queued_cmd *qc, unsigned int err_mask); +typedef int (*ata_qc_cb_t) (struct ata_queued_cmd *qc); struct ata_ioports { unsigned long cmd_addr; @@ -279,6 +279,8 @@ struct ata_queued_cmd { /* DO NOT iterate over __sg manually, use ata_for_each_sg() */ struct scatterlist *__sg; + unsigned int err_mask; + ata_qc_cb_t complete_fn; struct completion *waiting; @@ -475,7 +477,7 @@ extern void ata_bmdma_start (struct ata_ extern void ata_bmdma_stop(struct ata_queued_cmd *qc); extern u8 ata_bmdma_status(struct ata_port *ap); extern void ata_bmdma_irq_clear(struct ata_port *ap); -extern void ata_qc_complete(struct ata_queued_cmd *qc, unsigned int err_mask); +extern void ata_qc_complete(struct ata_queued_cmd *qc); extern void ata_eng_timeout(struct ata_port *ap); extern void ata_scsi_simulate(u16 *id, struct scsi_cmnd *cmd, void (*done)(struct scsi_cmnd *)); @@ -667,6 +669,7 @@ static inline void ata_qc_reinit(struct qc->cursect = qc->cursg = qc->cursg_ofs = 0; qc->nsect = 0; qc->nbytes = qc->curbytes = 0; + qc->err_mask = 0; ata_tf_init(qc->ap, &qc->tf, qc->dev->devno); } --- upstream/drivers/scsi/libata.h 2005-12-05 10:13:48.000000000 +0800 +++ err_mask2/drivers/scsi/libata.h 2005-12-05 10:26:14.000000000 +0800 @@ -39,7 +39,7 @@ 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 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); --- err_mask1/drivers/scsi/libata-core.c 2005-12-05 12:32:33.000000000 +0800 +++ err_mask2/drivers/scsi/libata-core.c 2005-12-05 12:37:14.000000000 +0800 @@ -1053,9 +1053,9 @@ static int ata_qc_wait_err(struct ata_qu if (wait_for_completion_timeout(wait, 30 * HZ) < 1) { /* timeout handling */ - unsigned int err_mask = ac_err_mask(ata_chk_status(qc->ap)); + qc->err_mask |= ac_err_mask(ata_chk_status(qc->ap)); - if (!err_mask) { + if (!qc->err_mask) { printk(KERN_WARNING "ata%u: slow completion (cmd %x)\n", qc->ap->id, qc->tf.command); } else { @@ -1064,7 +1064,7 @@ static int ata_qc_wait_err(struct ata_qu rc = -EIO; } - ata_qc_complete(qc, err_mask); + ata_qc_complete(qc); } return rc; @@ -1175,6 +1175,7 @@ retry: qc->cursg_ofs = 0; qc->cursect = 0; qc->nsect = 1; + qc->err_mask = 0; goto retry; } } @@ -2777,7 +2778,7 @@ skip_map: * None. (grabs host lock) */ -void ata_poll_qc_complete(struct ata_queued_cmd *qc, unsigned int err_mask) +void ata_poll_qc_complete(struct ata_queued_cmd *qc) { struct ata_port *ap = qc->ap; unsigned long flags; @@ -2785,7 +2786,7 @@ void ata_poll_qc_complete(struct ata_que spin_lock_irqsave(&ap->host_set->lock, flags); ap->flags &= ~ATA_FLAG_NOINTR; ata_irq_on(ap); - ata_qc_complete(qc, err_mask); + ata_qc_complete(qc); spin_unlock_irqrestore(&ap->host_set->lock, flags); } @@ -2885,7 +2886,8 @@ static int ata_pio_complete (struct ata_ ap->hsm_task_state = HSM_ST_IDLE; - ata_poll_qc_complete(qc, 0); + assert(qc->err_mask == 0); + ata_poll_qc_complete(qc); /* another command may start at this point */ @@ -3261,7 +3263,8 @@ static void ata_pio_error(struct ata_por ap->hsm_task_state = HSM_ST_IDLE; - ata_poll_qc_complete(qc, AC_ERR_ATA_BUS); + qc->err_mask |= AC_ERR_ATA_BUS; + ata_poll_qc_complete(qc); } static void ata_pio_task(void *_data) @@ -3363,7 +3366,8 @@ static void ata_qc_timeout(struct ata_qu ap->id, qc->tf.command, drv_stat, host_stat); /* complete taskfile transaction */ - ata_qc_complete(qc, ac_err_mask(drv_stat)); + qc->err_mask |= ac_err_mask(drv_stat); + ata_qc_complete(qc); break; } @@ -3462,7 +3466,7 @@ struct ata_queued_cmd *ata_qc_new_init(s return qc; } -int ata_qc_complete_noop(struct ata_queued_cmd *qc, unsigned int err_mask) +int ata_qc_complete_noop(struct ata_queued_cmd *qc) { return 0; } @@ -3521,7 +3525,7 @@ void ata_qc_free(struct ata_queued_cmd * * spin_lock_irqsave(host_set lock) */ -void ata_qc_complete(struct ata_queued_cmd *qc, unsigned int err_mask) +void ata_qc_complete(struct ata_queued_cmd *qc) { int rc; @@ -3538,7 +3542,7 @@ void ata_qc_complete(struct ata_queued_c qc->flags &= ~ATA_QCFLAG_ACTIVE; /* call completion callback */ - rc = qc->complete_fn(qc, err_mask); + rc = qc->complete_fn(qc); /* if callback indicates not to complete command (non-zero), * return immediately @@ -3976,7 +3980,8 @@ inline unsigned int ata_host_intr (struc ap->ops->irq_clear(ap); /* complete taskfile transaction */ - ata_qc_complete(qc, ac_err_mask(status)); + qc->err_mask |= ac_err_mask(status); + ata_qc_complete(qc); break; default: @@ -4111,7 +4116,8 @@ static void atapi_packet_task(void *_dat err_out_status: status = ata_chk_status(ap); err_out: - ata_poll_qc_complete(qc, __ac_err_mask(status)); + qc->err_mask |= __ac_err_mask(status); + ata_poll_qc_complete(qc); } --- upstream/drivers/scsi/libata-scsi.c 2005-12-05 10:13:48.000000000 +0800 +++ err_mask2/drivers/scsi/libata-scsi.c 2005-12-05 12:51:30.000000000 +0800 @@ -1203,12 +1203,11 @@ nothing_to_do: return 1; } -static int ata_scsi_qc_complete(struct ata_queued_cmd *qc, - unsigned int err_mask) +static int ata_scsi_qc_complete(struct ata_queued_cmd *qc) { struct scsi_cmnd *cmd = qc->scsicmd; u8 *cdb = cmd->cmnd; - int need_sense = (err_mask != 0); + int need_sense = (qc->err_mask != 0); /* For ATA pass thru (SAT) commands, generate a sense block if * user mandated it or if there's an error. Note that if we @@ -1955,9 +1954,9 @@ void ata_scsi_badcmd(struct scsi_cmnd *c done(cmd); } -static int atapi_sense_complete(struct ata_queued_cmd *qc,unsigned int err_mask) +static int atapi_sense_complete(struct ata_queued_cmd *qc) { - if (err_mask && ((err_mask & AC_ERR_DEV) == 0)) + if (qc->err_mask && ((qc->err_mask & AC_ERR_DEV) == 0)) /* FIXME: not quite right; we don't want the * translation of taskfile registers into * a sense descriptors, since that's only @@ -2015,15 +2014,18 @@ static void atapi_request_sense(struct a qc->complete_fn = atapi_sense_complete; - if (ata_qc_issue(qc)) - ata_qc_complete(qc, AC_ERR_OTHER); + if (ata_qc_issue(qc)) { + qc->err_mask |= AC_ERR_OTHER; + ata_qc_complete(qc); + } DPRINTK("EXIT\n"); } -static int atapi_qc_complete(struct ata_queued_cmd *qc, unsigned int err_mask) +static int atapi_qc_complete(struct ata_queued_cmd *qc) { struct scsi_cmnd *cmd = qc->scsicmd; + unsigned int err_mask = qc->err_mask; VPRINTK("ENTER, err_mask 0x%X\n", err_mask); --- upstream/drivers/scsi/ahci.c 2005-12-05 10:13:48.000000000 +0800 +++ err_mask2/drivers/scsi/ahci.c 2005-12-05 10:36:41.000000000 +0800 @@ -643,7 +643,8 @@ static void ahci_eng_timeout(struct ata_ * not being called from the SCSI EH. */ qc->scsidone = scsi_finish_command; - ata_qc_complete(qc, AC_ERR_OTHER); + qc->err_mask |= AC_ERR_OTHER; + ata_qc_complete(qc); } spin_unlock_irqrestore(&host_set->lock, flags); @@ -664,7 +665,8 @@ static inline int ahci_host_intr(struct ci = readl(port_mmio + PORT_CMD_ISSUE); if (likely((ci & 0x1) == 0)) { if (qc) { - ata_qc_complete(qc, 0); + assert(qc->err_mask == 0); + ata_qc_complete(qc); qc = NULL; } } @@ -681,8 +683,10 @@ static inline int ahci_host_intr(struct /* command processing has stopped due to error; restart */ ahci_restart_port(ap, status); - if (qc) - ata_qc_complete(qc, err_mask); + if (qc) { + qc->err_mask |= AC_ERR_OTHER; + ata_qc_complete(qc); + } } return 1; --- upstream/drivers/scsi/pdc_adma.c 2005-12-05 10:13:48.000000000 +0800 +++ err_mask2/drivers/scsi/pdc_adma.c 2005-12-05 13:27:44.000000000 +0800 @@ -464,14 +464,12 @@ static inline unsigned int adma_intr_pkt continue; qc = ata_qc_from_tag(ap, ap->active_tag); if (qc && (!(qc->tf.ctl & ATA_NIEN))) { - unsigned int err_mask = 0; - if ((status & (aPERR | aPSD | aUIRQ))) - err_mask = AC_ERR_OTHER; + qc->err_mask |= AC_ERR_OTHER; else if (pp->pkt[0] != cDONE) - err_mask = AC_ERR_OTHER; + qc->err_mask |= AC_ERR_OTHER; - ata_qc_complete(qc, err_mask); + ata_qc_complete(qc); } } return handled; @@ -501,7 +499,8 @@ static inline unsigned int adma_intr_mmi /* complete taskfile transaction */ pp->state = adma_state_idle; - ata_qc_complete(qc, ac_err_mask(status)); + qc->err_mask |= ac_err_mask(status); + ata_qc_complete(qc); handled = 1; } } --- upstream/drivers/scsi/sata_mv.c 2005-12-05 10:13:49.000000000 +0800 +++ err_mask2/drivers/scsi/sata_mv.c 2005-12-05 13:45:41.000000000 +0800 @@ -1242,8 +1242,10 @@ static void mv_host_intr(struct ata_host VPRINTK("port %u IRQ found for qc, " "ata_status 0x%x\n", port,ata_status); /* mark qc status appropriately */ - if (!(qc->tf.ctl & ATA_NIEN)) - ata_qc_complete(qc, err_mask); + if (!(qc->tf.ctl & ATA_NIEN)) { + qc->err_mask |= err_mask; + ata_qc_complete(qc); + } } } } @@ -1864,7 +1866,8 @@ static void mv_eng_timeout(struct ata_po */ spin_lock_irqsave(&ap->host_set->lock, flags); qc->scsidone = scsi_finish_command; - ata_qc_complete(qc, AC_ERR_OTHER); + qc->err_mask |= AC_ERR_OTHER; + ata_qc_complete(qc); spin_unlock_irqrestore(&ap->host_set->lock, flags); } } --- upstream/drivers/scsi/sata_promise.c 2005-12-05 10:13:49.000000000 +0800 +++ err_mask2/drivers/scsi/sata_promise.c 2005-12-05 10:43:12.000000000 +0800 @@ -401,7 +401,8 @@ static void pdc_eng_timeout(struct ata_p case ATA_PROT_NODATA: printk(KERN_ERR "ata%u: command timeout\n", ap->id); drv_stat = ata_wait_idle(ap); - ata_qc_complete(qc, __ac_err_mask(drv_stat)); + qc->err_mask |= __ac_err_mask(drv_stat); + ata_qc_complete(qc); break; default: @@ -410,7 +411,8 @@ static void pdc_eng_timeout(struct ata_p printk(KERN_ERR "ata%u: unknown timeout, cmd 0x%x stat 0x%x\n", ap->id, qc->tf.command, drv_stat); - ata_qc_complete(qc, ac_err_mask(drv_stat)); + qc->err_mask |= ac_err_mask(drv_stat); + ata_qc_complete(qc); break; } @@ -422,21 +424,21 @@ out: static inline unsigned int pdc_host_intr( struct ata_port *ap, struct ata_queued_cmd *qc) { - unsigned int handled = 0, err_mask = 0; + unsigned int handled = 0; u32 tmp; void __iomem *mmio = (void __iomem *) ap->ioaddr.cmd_addr + PDC_GLOBAL_CTL; tmp = readl(mmio); if (tmp & PDC_ERR_MASK) { - err_mask = AC_ERR_DEV; + qc->err_mask |= AC_ERR_DEV; pdc_reset_port(ap); } switch (qc->tf.protocol) { case ATA_PROT_DMA: case ATA_PROT_NODATA: - err_mask |= ac_err_mask(ata_wait_idle(ap)); - ata_qc_complete(qc, err_mask); + qc->err_mask |= ac_err_mask(ata_wait_idle(ap)); + ata_qc_complete(qc); handled = 1; break; --- upstream/drivers/scsi/sata_qstor.c 2005-12-05 10:13:49.000000000 +0800 +++ err_mask2/drivers/scsi/sata_qstor.c 2005-12-05 10:45:09.000000000 +0800 @@ -409,8 +409,8 @@ static inline unsigned int qs_intr_pkt(s case 3: /* device error */ pp->state = qs_state_idle; qs_enter_reg_mode(qc->ap); - ata_qc_complete(qc, - ac_err_mask(sDST)); + qc->err_mask |= ac_err_mask(sDST); + ata_qc_complete(qc); break; default: break; @@ -447,7 +447,8 @@ static inline unsigned int qs_intr_mmio( /* complete taskfile transaction */ pp->state = qs_state_idle; - ata_qc_complete(qc, ac_err_mask(status)); + qc->err_mask |= ac_err_mask(status); + ata_qc_complete(qc); handled = 1; } } --- upstream/drivers/scsi/sata_sil24.c 2005-12-05 10:13:49.000000000 +0800 +++ err_mask2/drivers/scsi/sata_sil24.c 2005-12-05 13:10:36.000000000 +0800 @@ -654,7 +654,8 @@ static void sil24_eng_timeout(struct ata */ printk(KERN_ERR "ata%u: command timeout\n", ap->id); qc->scsidone = scsi_finish_command; - ata_qc_complete(qc, AC_ERR_OTHER); + qc->err_mask |= AC_ERR_OTHER; + ata_qc_complete(qc); sil24_reset_controller(ap); } @@ -711,8 +712,10 @@ static void sil24_error_intr(struct ata_ sil24_reset_controller(ap); } - if (qc) - ata_qc_complete(qc, err_mask); + if (qc) { + qc->err_mask |= err_mask; + ata_qc_complete(qc); + } } static inline void sil24_host_intr(struct ata_port *ap) @@ -734,8 +737,10 @@ static inline void sil24_host_intr(struc */ sil24_update_tf(ap); - if (qc) - ata_qc_complete(qc, ac_err_mask(pp->tf.command)); + if (qc) { + qc->err_mask |= ac_err_mask(pp->tf.command); + ata_qc_complete(qc); + } } else sil24_error_intr(ap, slot_stat); } --- upstream/drivers/scsi/sata_sx4.c 2005-12-05 10:13:49.000000000 +0800 +++ err_mask2/drivers/scsi/sata_sx4.c 2005-12-05 10:51:16.000000000 +0800 @@ -718,7 +718,8 @@ static inline unsigned int pdc20621_host VPRINTK("ata%u: read hdma, 0x%x 0x%x\n", ap->id, readl(mmio + 0x104), readl(mmio + PDC_HDMA_CTLSTAT)); /* get drive status; clear intr; complete txn */ - ata_qc_complete(qc, ac_err_mask(ata_wait_idle(ap))); + qc->err_mask |= ac_err_mask(ata_wait_idle(ap)); + ata_qc_complete(qc); pdc20621_pop_hdma(qc); } @@ -756,7 +757,8 @@ static inline unsigned int pdc20621_host VPRINTK("ata%u: write ata, 0x%x 0x%x\n", ap->id, readl(mmio + 0x104), readl(mmio + PDC_HDMA_CTLSTAT)); /* get drive status; clear intr; complete txn */ - ata_qc_complete(qc, ac_err_mask(ata_wait_idle(ap))); + qc->err_mask |= ac_err_mask(ata_wait_idle(ap)); + ata_qc_complete(qc); pdc20621_pop_hdma(qc); } handled = 1; @@ -766,7 +768,8 @@ static inline unsigned int pdc20621_host status = ata_busy_wait(ap, ATA_BUSY | ATA_DRQ, 1000); DPRINTK("BUS_NODATA (drv_stat 0x%X)\n", status); - ata_qc_complete(qc, ac_err_mask(status)); + qc->err_mask |= ac_err_mask(status); + ata_qc_complete(qc); handled = 1; } else { @@ -881,7 +884,8 @@ static void pdc_eng_timeout(struct ata_p case ATA_PROT_DMA: case ATA_PROT_NODATA: printk(KERN_ERR "ata%u: command timeout\n", ap->id); - ata_qc_complete(qc, __ac_err_mask(ata_wait_idle(ap))); + qc->err_mask |= __ac_err_mask(ata_wait_idle(ap)); + ata_qc_complete(qc); break; default: @@ -890,7 +894,8 @@ static void pdc_eng_timeout(struct ata_p printk(KERN_ERR "ata%u: unknown timeout, cmd 0x%x stat 0x%x\n", ap->id, qc->tf.command, drv_stat); - ata_qc_complete(qc, ac_err_mask(drv_stat)); + qc->err_mask |= ac_err_mask(drv_stat); + ata_qc_complete(qc); break; } ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 3/4] libata: determine the err_mask when the error is found 2005-12-05 6:58 ` [PATCH 0/4] libata: move err_mask to ata_queued_cmd (revised #2) Albert Lee 2005-12-05 7:36 ` [PATCH 1/4] libata: minor patch before moving err_mask Albert Lee 2005-12-05 7:38 ` [PATCH 2/4] libata: move err_mask to ata_queued_cmd Albert Lee @ 2005-12-05 7:40 ` Albert Lee 2005-12-05 7:42 ` [PATCH 4/4] libata: determine the err_mask directly in atapi_packet_task() Albert Lee 2005-12-06 3:34 ` [PATCH/RFC 1/1] libata: err_mask misc fix Albert Lee 4 siblings, 0 replies; 19+ messages in thread From: Albert Lee @ 2005-12-05 7:40 UTC (permalink / raw) To: Jeff Garzik; +Cc: Linux IDE, Bartlomiej Zolnierkiewicz, Doug Maxey, Tejun Heo Patch 3/4: determine the err_mask when the error is found Changes: - move "qc->err_mask |= AC_ERR_ATA_BUS" to where the error is found - add "assert(qc->err_mask)" to ata_pio_error() to make sure qc->err_mask was available when we enter the error state For your review, thanks. Albert Signed-off-by: Albert Lee <albertcc@tw.ibm.com> ============ --- err_mask2/drivers/scsi/libata-core.c 2005-12-05 12:37:14.000000000 +0800 +++ err_mask3/drivers/scsi/libata-core.c 2005-12-05 12:42:06.000000000 +0800 @@ -2830,6 +2830,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; ap->hsm_task_state = HSM_ST_TMOUT; return 0; } @@ -2880,6 +2881,7 @@ static int ata_pio_complete (struct ata_ drv_stat = ata_wait_idle(ap); if (!ata_ok(drv_stat)) { + qc->err_mask |= __ac_err_mask(drv_stat); ap->hsm_task_state = HSM_ST_ERR; return 0; } @@ -3195,6 +3197,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; ap->hsm_task_state = HSM_ST_ERR; } @@ -3244,6 +3247,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; ap->hsm_task_state = HSM_ST_ERR; return; } @@ -3261,9 +3265,13 @@ static void ata_pio_error(struct ata_por qc = ata_qc_from_tag(ap, ap->active_tag); assert(qc != NULL); + /* make sure qc->err_mask is available to + * know what's wrong and recover + */ + assert(qc->err_mask); + ap->hsm_task_state = HSM_ST_IDLE; - qc->err_mask |= AC_ERR_ATA_BUS; ata_poll_qc_complete(qc); } ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 4/4] libata: determine the err_mask directly in atapi_packet_task() 2005-12-05 6:58 ` [PATCH 0/4] libata: move err_mask to ata_queued_cmd (revised #2) Albert Lee ` (2 preceding siblings ...) 2005-12-05 7:40 ` [PATCH 3/4] libata: determine the err_mask when the error is found Albert Lee @ 2005-12-05 7:42 ` Albert Lee 2005-12-06 3:34 ` [PATCH/RFC 1/1] libata: err_mask misc fix Albert Lee 4 siblings, 0 replies; 19+ messages in thread From: Albert Lee @ 2005-12-05 7:42 UTC (permalink / raw) To: Jeff Garzik; +Cc: Linux IDE, Bartlomiej Zolnierkiewicz, Doug Maxey, Tejun Heo Patch 4/4: determine the err_mask directly in atapi_packet_task() Changes: - set qc->err_mask directly when we found the error - remove the code to determine err_mask from device status For your review, thanks. Albert Signed-off-by: Albert Lee <albertcc@tw.ibm.com> ============ --- err_mask3/drivers/scsi/libata-core.c 2005-12-05 12:42:06.000000000 +0800 +++ err_mask4/drivers/scsi/libata-core.c 2005-12-05 12:44:28.000000000 +0800 @@ -4083,13 +4083,17 @@ 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)) - goto err_out_status; + if (ata_busy_sleep(ap, ATA_TMOUT_CDB_QUICK, ATA_TMOUT_CDB)) { + qc->err_mask |= AC_ERR_ATA_BUS; + goto err_out; + } /* make sure DRQ is set */ status = ata_chk_status(ap); - if ((status & (ATA_BUSY | ATA_DRQ)) != ATA_DRQ) + if ((status & (ATA_BUSY | ATA_DRQ)) != ATA_DRQ) { + qc->err_mask |= AC_ERR_ATA_BUS; goto err_out; + } /* send SCSI cdb */ DPRINTK("send cdb\n"); @@ -4121,10 +4125,7 @@ static void atapi_packet_task(void *_dat return; -err_out_status: - status = ata_chk_status(ap); err_out: - qc->err_mask |= __ac_err_mask(status); ata_poll_qc_complete(qc); } ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH/RFC 1/1] libata: err_mask misc fix 2005-12-05 6:58 ` [PATCH 0/4] libata: move err_mask to ata_queued_cmd (revised #2) Albert Lee ` (3 preceding siblings ...) 2005-12-05 7:42 ` [PATCH 4/4] libata: determine the err_mask directly in atapi_packet_task() Albert Lee @ 2005-12-06 3:34 ` Albert Lee 2005-12-12 20:26 ` Jeff Garzik 4 siblings, 1 reply; 19+ messages in thread From: Albert Lee @ 2005-12-06 3:34 UTC (permalink / raw) To: Jeff Garzik; +Cc: Linux IDE, Bartlomiej Zolnierkiewicz, Doug Maxey, Tejun Heo Desc: Misc follow-up patch for the err_mask patch 1-4. Changes: 1. ata_pio_complete(): It seems unnecessary to wait for the clearing of the DRQ bit. (Waiting for BSY=0 should be enough. ata_ok() also checks the correctness of the status bits later.) 2. ata_pio_block(): - added error checking, before transfering data. - minor comments fix For your review, thanks. Albert Signed-off-by: Albert Lee <albertcc@tw.ibm.com> ============ --- err_mask4/drivers/scsi/libata-core.c 2005-12-05 12:44:28.000000000 +0800 +++ pio_state5/drivers/scsi/libata-core.c 2005-12-06 11:17:14.000000000 +0800 @@ -2865,11 +2865,11 @@ static int ata_pio_complete (struct ata_ * msecs, then chk-status again. If still busy, fall back to * HSM_ST_POLL state. */ - drv_stat = ata_busy_wait(ap, ATA_BUSY | ATA_DRQ, 10); - if (drv_stat & (ATA_BUSY | ATA_DRQ)) { + drv_stat = ata_busy_wait(ap, ATA_BUSY, 10); + if (drv_stat & ATA_BUSY) { msleep(2); - drv_stat = ata_busy_wait(ap, ATA_BUSY | ATA_DRQ, 10); - if (drv_stat & (ATA_BUSY | ATA_DRQ)) { + drv_stat = ata_busy_wait(ap, ATA_BUSY, 10); + if (drv_stat & ATA_BUSY) { ap->hsm_task_state = HSM_ST_LAST_POLL; ap->pio_task_timeout = jiffies + ATA_TMOUT_PIO; return 0; @@ -3236,8 +3236,16 @@ static void ata_pio_block(struct ata_por qc = ata_qc_from_tag(ap, ap->active_tag); assert(qc != NULL); + /* check error */ + if (status & (ATA_ERR | ATA_DF)) { + qc->err_mask |= AC_ERR_DEV; + ap->hsm_task_state = HSM_ST_ERR; + return; + } + + /* transfer data if any */ if (is_atapi_taskfile(&qc->tf)) { - /* no more data to transfer or unsupported ATAPI command */ + /* DRQ=0 means no more data to transfer */ if ((status & ATA_DRQ) == 0) { ap->hsm_task_state = HSM_ST_LAST; return; ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH/RFC 1/1] libata: err_mask misc fix 2005-12-06 3:34 ` [PATCH/RFC 1/1] libata: err_mask misc fix Albert Lee @ 2005-12-12 20:26 ` Jeff Garzik 0 siblings, 0 replies; 19+ messages in thread From: Jeff Garzik @ 2005-12-12 20:26 UTC (permalink / raw) To: Albert Lee; +Cc: Linux IDE, Bartlomiej Zolnierkiewicz, Doug Maxey, Tejun Heo applied patches 1-4, and this fix, to 'upstream' ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH/RFC 3/3] libata: check qc->err_mask for the internal commands 2005-11-09 4:57 [PATCH/RFC 0/3] libata: misc fixes Albert Lee 2005-11-09 5:03 ` [PATCH 1/3] libata: if condition fix for ata_dev_identify() Albert Lee 2005-11-09 5:07 ` [PATCH/RFC 2/3] libata: move err_mask to ata_queued_cmd Albert Lee @ 2005-11-09 5:12 ` Albert Lee 2005-11-09 6:31 ` Jeff Garzik 2 siblings, 1 reply; 19+ messages in thread From: Albert Lee @ 2005-11-09 5:12 UTC (permalink / raw) To: Jeff Garzik; +Cc: Linux IDE, Bartlomiej Zolnierkiewicz, Doug Maxey, Tejun Heo Patch 3/3: check qc->err_mask for the internal commands. Problem: The results of internal commands issued by libata itself were not checked. For example, ata_dev_identify() only checks the device status (qc->tf.command). However, checking the device status is not enough to detect all errors. Changes: - check qc->err_mask for the internal commands: - ata_dev_identify() - ata_dev_set_xfermode() - ata_dev_reread_id() - ata_dev_init_params() - atapi_request_sense() The patch accesses qc (and ap) after the qc is completed. This is bad from the object life cycle point of view and might cause race. However, since libata has complete control over the hardware when libata issues the internal commands and it seems noboby else may issue command at the same time, accessing qc after ata_qc_complete() looks to be ok here. p.s. The patch above can also make ata_dev_identify() detect the "phantom slave device" problem. The "phantom slave device" was seen in specific PATA master-only configuration. The master device responds to some commands issued to the slave device, and make a illusion that the slave device exists. The device status register of the "phantom slave device" always read as zero. So, using (qc->tf.command & ATA_ERR) cannot detect the error. Checking qc->err_mask can make ata_dev_identify() detect such "phantom slave device" and exclude the phantom device. For your review and advice, thanks. Albert Signed-off-by: Albert Lee <albertcc@tw.ibm.com> ============ --- err_mask/drivers/scsi/libata-core.c 2005-11-09 10:15:33.000000000 +0800 +++ phantom/drivers/scsi/libata-core.c 2005-11-09 10:15:54.000000000 +0800 @@ -1132,7 +1132,7 @@ retry: ap->ops->tf_read(ap, &qc->tf); spin_unlock_irqrestore(&ap->host_set->lock, flags); - if (qc->tf.command & ATA_ERR) { + if (qc->err_mask) { /* * arg! EDD works for all test cases, but seems to return * the ATA signature for some ATAPI devices. Until the @@ -1144,8 +1144,10 @@ retry: * ATA software reset (SRST, the default) does not appear * to have this problem. */ - if ((using_edd) && (dev->class == ATA_DEV_ATA)) { + if ((using_edd) && (dev->class == ATA_DEV_ATA) && + (qc->tf.command & ATA_ERR)) { u8 err = qc->tf.feature; + if (err & ATA_ABORTED) { dev->class = ATA_DEV_ATAPI; qc->cursg = 0; @@ -2267,11 +2269,19 @@ static void ata_dev_set_xfermode(struct spin_unlock_irqrestore(&ap->host_set->lock, flags); if (rc) - ata_port_disable(ap); - else - wait_for_completion(&wait); + goto err_out; + + wait_for_completion(&wait); + + if (qc->err_mask) + goto err_out; DPRINTK("EXIT\n"); + + return; +err_out: + printk(KERN_ERR "ata%u: set XFER MODE failed\n", ap->id); + ata_port_disable(ap); } /** @@ -2319,6 +2329,9 @@ static void ata_dev_reread_id(struct ata wait_for_completion(&wait); + if (qc->err_mask) + goto err_out; + swap_buf_le16(dev->id, ATA_ID_WORDS); ata_dump_id(dev); @@ -2327,6 +2340,7 @@ static void ata_dev_reread_id(struct ata return; err_out: + printk(KERN_ERR "ata%u: reread IDENTIFY device data failed\n", ap->id); ata_port_disable(ap); } @@ -2371,11 +2385,19 @@ static void ata_dev_init_params(struct a spin_unlock_irqrestore(&ap->host_set->lock, flags); if (rc) - ata_port_disable(ap); - else - wait_for_completion(&wait); + goto err_out; + + wait_for_completion(&wait); + + if (qc->err_mask) + goto err_out; DPRINTK("EXIT\n"); + + return; +err_out: + printk(KERN_ERR "ata%u: init CHS dev params failed\n", ap->id); + ata_port_disable(ap); } /** --- err_mask/drivers/scsi/libata-scsi.c 2005-11-08 16:25:51.000000000 +0800 +++ phantom/drivers/scsi/libata-scsi.c 2005-11-09 10:37:39.000000000 +0800 @@ -2008,11 +2008,19 @@ void atapi_request_sense(struct ata_port spin_unlock_irqrestore(&ap->host_set->lock, flags); if (rc) - ata_port_disable(ap); - else - wait_for_completion(&wait); + goto err_out; + + wait_for_completion(&wait); + + if (qc->err_mask) + goto err_out; DPRINTK("EXIT\n"); + + return; +err_out: + printk(KERN_ERR "ata%u: atapi request sense failed\n", ap->id); + ata_port_disable(ap); } static int atapi_qc_complete(struct ata_queued_cmd *qc) ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH/RFC 3/3] libata: check qc->err_mask for the internal commands 2005-11-09 5:12 ` [PATCH/RFC 3/3] libata: check qc->err_mask for the internal commands Albert Lee @ 2005-11-09 6:31 ` Jeff Garzik 0 siblings, 0 replies; 19+ messages in thread From: Jeff Garzik @ 2005-11-09 6:31 UTC (permalink / raw) To: Albert Lee; +Cc: Linux IDE, Bartlomiej Zolnierkiewicz, Doug Maxey, Tejun Heo Albert Lee wrote: > Patch 3/3: > check qc->err_mask for the internal commands. > > Problem: > The results of internal commands issued by libata itself were not checked. > > For example, ata_dev_identify() only checks the device status (qc->tf.command). > However, checking the device status is not enough to detect all errors. > > Changes: > - check qc->err_mask for the internal commands: > - ata_dev_identify() > - ata_dev_set_xfermode() > - ata_dev_reread_id() > - ata_dev_init_params() > - atapi_request_sense() > > The patch accesses qc (and ap) after the qc is completed. > This is bad from the object life cycle point of view and might cause race. > However, since libata has complete control over the hardware when libata issues the internal commands > and it seems noboby else may issue command at the same time, > accessing qc after ata_qc_complete() looks to be ok here. > > p.s. > The patch above can also make ata_dev_identify() detect the > "phantom slave device" problem. > > The "phantom slave device" was seen in specific PATA master-only configuration. > The master device responds to some commands issued to the slave device, and > make a illusion that the slave device exists. > The device status register of the "phantom slave device" always read as zero. > So, using (qc->tf.command & ATA_ERR) cannot detect the error. > Checking qc->err_mask can make ata_dev_identify() detect such > "phantom slave device" and exclude the phantom device. > > For your review and advice, thanks. > > Albert > Signed-off-by: Albert Lee <albertcc@tw.ibm.com> In general this seems like an OK direction to take, but I am a bit worried about the object lifetime issues that you mention. It is IMO worth considering a better lifetime strategy at this point, before applying this [worthwhile] change. Jeff ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2005-12-12 20:27 UTC | newest] Thread overview: 19+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2005-11-09 4:57 [PATCH/RFC 0/3] libata: misc fixes Albert Lee 2005-11-09 5:03 ` [PATCH 1/3] libata: if condition fix for ata_dev_identify() Albert Lee 2005-11-09 5:07 ` [PATCH/RFC 2/3] libata: move err_mask to ata_queued_cmd Albert Lee 2005-11-09 6:25 ` Jeff Garzik 2005-11-10 7:56 ` [PATCH/RFC 0/4] libata: move err_mask to ata_queued_cmd (revised) Albert Lee 2005-11-10 7:59 ` [PATCH/RFC 1/4] libata: minor patch before moving err_mask Albert Lee 2005-11-10 8:01 ` [PATCH/RFC 2/4] libata: move err_mask to ata_queued_cmd Albert Lee 2005-11-10 8:03 ` [PATCH/RFC 3/4] libata: determine the err_mask when the error is found Albert Lee 2005-11-10 8:05 ` [PATCH/RFC 4/4] libata: determine the err_mask directly in atapi_packet_task() Albert Lee 2005-12-04 2:01 ` [PATCH/RFC 0/4] libata: move err_mask to ata_queued_cmd (revised) Jeff Garzik 2005-12-05 6:58 ` [PATCH 0/4] libata: move err_mask to ata_queued_cmd (revised #2) Albert Lee 2005-12-05 7:36 ` [PATCH 1/4] libata: minor patch before moving err_mask Albert Lee 2005-12-05 7:38 ` [PATCH 2/4] libata: move err_mask to ata_queued_cmd Albert Lee 2005-12-05 7:40 ` [PATCH 3/4] libata: determine the err_mask when the error is found Albert Lee 2005-12-05 7:42 ` [PATCH 4/4] libata: determine the err_mask directly in atapi_packet_task() Albert Lee 2005-12-06 3:34 ` [PATCH/RFC 1/1] libata: err_mask misc fix Albert Lee 2005-12-12 20:26 ` Jeff Garzik 2005-11-09 5:12 ` [PATCH/RFC 3/3] libata: check qc->err_mask for the internal commands Albert Lee 2005-11-09 6:31 ` Jeff Garzik
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).