libata: Better timeout recovery
PATCH 1/1 libata: Better timeout recovery
Check for completed commands on a timeout, also implement data draining as Mark Lord suggested. The former should help a lot on various promise controllers which show random IRQ loss now and then, the latter at least for me fixes the hanging DRQ cases I can test.
To get the lost IRQ recovery working better we really need to short circuit a lot fo the recovery paths we trigger needlessly when EH finds that actually all was well.
---
drivers/ata/libata-eh.c | 19 +++++++++- drivers/ata/libata-sff.c | 88 ++++++++++++++++++++++++++++++++++++++++++++- drivers/ata/pata_isapnp.c | 12 +++++- drivers/ata/pata_pcmcia.c | 35 +++++++++++++++++- include/linux/libata.h | 5 +++ 5 files changed, 153 insertions(+), 6 deletions(-)
drivers/ata/libata-eh.c +17 -2
@@ -515,7 +515,7 @@ void ata_scsi_error(struct Scsi_Host *host)/* For new EH, all qcs are finished in one of three ways - * normal completion, error completion, and SCSI timeout. -* Both cmpletions can race against SCSI timeout. When normal +* Both completions can race against SCSI timeout. When normal * completion wins, the qc never reaches EH. When error * completion wins, the qc has ATA_QCFLAG_FAILED set. *
@@ -530,7 +530,19 @@ void ata_scsi_error(struct Scsi_Host *host)int nr_timedout =0 ;spin_lock_irqsave (ap->lock, flags); - + +/* This must occur under the ap->lock as we don't want +a polled recovery to race the real interrupt handler ++ The lost_interrupt handler checks for any completed but +non-notified command and completes much like an IRQ handler. ++ We then fall into the error recovery code which will treat +this as if normal completion won the race */ + +if (ap->ops->lost_interrupt) + ap->ops->lost_interrupt (ap); +list_for_each_entry_safe (scmd, tmp, &host->eh_cmd_q, eh_entry) {struct ata_queued_cmd *qc;
@@ -574,6 +586,9 @@ void ata_scsi_error(struct Scsi_Host *host) ap->eh_tries =ATA_EH_MAX_TRIES ; }else spin_unlock_wait (ap->lock); + +/* If we timed raced normal completion and there is nothing to +recover nr_timedout == 0 why exactly are we doing error recovery ? */ repeat:/* invoke error handler */
drivers/ata/libata-sff.c +87 -1
142 lines not on this page · raw message
drivers/ata/pata_isapnp.c +10 -2
@@ -17,7 +17,7 @@#include <linux/libata.h> #define DRV_NAME "pata_isapnp" -#define DRV_VERSION "0.2.2" +#define DRV_VERSION "0.2.5"static struct scsi_host_template isapnp_sht = {ATA_PIO_SHT (DRV_NAME ),
@@ -28,6 +28,13 @@ static struct ata_port_operations isapnp_port_ops = { .cable_detect = ata_cable_40wire, }; +static struct ata_port_operations isapnp_noalt_port_ops = { + .inherits = &ata_sff_port_ops, + .cable_detect = ata_cable_40wire, +/* No altstatus so we don't want to use the lost interrupt poll */ + .lost_interrupt =ATA_OP_NULL , +}; +/** * isapnp_init_one - attach an isapnp interface * @idev: PnP device
@@ -65,7 +72,7 @@ static int isapnp_init_one(struct pnp_dev *idev, const struct pnp_device_id *dev ap = host->ports[0 ]; - ap->ops = &isapnp_port_ops; + ap->ops = &isapnp_noalt_port_ops; ap->pio_mask =1 ; ap->flags |=ATA_FLAG_SLAVE_POSS ;
@@ -76,6 +83,7 @@ static int isapnp_init_one(struct pnp_dev *idev, const struct pnp_device_id *dev pnp_port_start(idev, 1), 1); ap->ioaddr.altstatus_addr = ctl_addr; ap->ioaddr.ctl_addr = ctl_addr; + ap->ops = &isapnp_port_ops; }ata_sff_std_ports (&ap->ioaddr);
drivers/ata/pata_pcmcia.c +34 -1
@@ -42,7 +42,7 @@#define DRV_NAME "pata_pcmcia" -#define DRV_VERSION "0.3.3" +#define DRV_VERSION "0.3.5"/* * Private data structure to glue stuff together
@@ -126,6 +126,38 @@ static unsigned int ata_data_xfer_8bit(struct ata_device *dev,return buflen; } +/** +* pcmcia_8bit_drain_fifo - Stock FIFO drain logic for SFF controllers +* @ap: port to drain +* @qc: command +* +* Drain the FIFO and device of any stuck data following a command +* failing to complete. In some cases this is neccessary before a +* reset will recover the device. +* +*/ + +void pcmcia_8bit_drain_fifo (struct ata_queued_cmd *qc) +{ +int count; +struct ata_port *ap; + +/* We only need to flush incoming data when a command was running */ +if (qc ==NULL || qc->dma_dir ==DMA_TO_DEVICE ) +return ; + + ap = qc->ap; + +/* Drain up to 64K of data before we give up this recovery method */ +for (count =0 ; (ap->ops->sff_check_status (ap) &ATA_DRQ ) + && count <65536 ;) +ioread8 (ap->ioaddr.data_addr); + +/* Can become DEBUG later */ +ata_port_printk (ap,KERN_WARNING ,"drained %d bytes to clear DRQ.\n" , + count); + +}static struct scsi_host_template pcmcia_sht = {ATA_PIO_SHT (DRV_NAME ),
@@ -143,6 +175,7 @@ static struct ata_port_operations pcmcia_8bit_port_ops = { .sff_data_xfer = ata_data_xfer_8bit, .cable_detect = ata_cable_40wire, .set_mode = pcmcia_set_mode_8bit, + .drain_fifo = pcmcia_8bit_drain_fifo, };#define CS_CHECK (fn, ret) \
include/linux/libata.h +5 -0
@@ -767,6 +767,7 @@ struct ata_port_operations {ata_reset_fn_t pmp_hardreset;ata_postreset_fn_t pmp_postreset;void (*error_handler)(struct ata_port *ap); +void (*lost_interrupt)(struct ata_port *ap);void (*post_internal_cmd)(struct ata_queued_cmd *qc);/*
@@ -808,6 +809,8 @@ struct ata_port_operations {void (*bmdma_start)(struct ata_queued_cmd *qc);void (*bmdma_stop)(struct ata_queued_cmd *qc);u8 (*bmdma_status)(struct ata_port *ap); + +void (*drain_fifo)(struct ata_queued_cmd *qc);#endif /* CONFIG_ATA_SFF */ ssize_t (*em_show)(struct ata_port *ap,char *buf);
@@ -1516,6 +1519,7 @@ extern bool ata_sff_qc_fill_rtf(struct ata_queued_cmd *qc);extern unsigned int ata_sff_host_intr (struct ata_port *ap,struct ata_queued_cmd *qc);extern irqreturn_t ata_sff_interrupt (int irq,void *dev_instance); +extern void ata_sff_lost_interrupt (struct ata_port *ap);extern void ata_sff_freeze (struct ata_port *ap);extern void ata_sff_thaw (struct ata_port *ap);extern int ata_sff_prereset (struct ata_link *link,unsigned long deadline);
@@ -1528,6 +1532,7 @@ extern int ata_sff_softreset(struct ata_link *link, unsigned int *classes,extern int sata_sff_hardreset (struct ata_link *link,unsigned int *class,unsigned long deadline);extern void ata_sff_postreset (struct ata_link *link,unsigned int *classes); +extern void ata_sff_drain_fifo (struct ata_queued_cmd *qc);extern void ata_sff_error_handler (struct ata_port *ap);extern void ata_sff_post_internal_cmd (struct ata_queued_cmd *qc);extern int ata_sff_port_start (struct ata_port *ap);
Quoted 11 lines · Alan Cox
Check for completed commands on a timeout, also implement data draining as Mark Lord suggested. The former should help a lot on various promise controllers which show random IRQ loss now and then, the latter at least for me fixes the hanging DRQ cases I can test.
To get the lost IRQ recovery working better we really need to short circuit a lot fo the recovery paths we trigger needlessly when EH finds that actually all was well.
Signed-off-by Alan Cox <[email protected]>---
This patch has a lot of style issues. Most of them are caught by checkpatch. A few more are indicated below:
Quoted 4 lines
drivers/ata/libata-eh.c +0
-0
[...]
Quoted 4 lines
drivers/ata/libata-eh.c +0
-0@@ -530,7 +530,19 @@ void ata_scsi_error(struct Scsi_Host *host)int nr_timedout =0 ;spin_lock_irqsave (ap->lock, flags);
drivers/ata/libata-eh.c void ata_scsi_error(struct Scsi_Host *host) · line 541
Quoted 10 lines
drivers/ata/libata-eh.c +9
-1- + +/* This must occur under the ap->lock as we don't want +a polled recovery to race the real interrupt handler ++ The lost_interrupt handler checks for any completed but +non-notified command and completes much like an IRQ handler. ++ We then fall into the error recovery code which will treat +this as if normal completion won the race */
I'd very much prefer comments to be formatted like this:
/* This must occur under the ap->lock as we don't want
* a polled recovery to race the real interrupt handler *
* The lost_interrupt handler checks for any completed but
* non-notified command and completes much like an IRQ handler. *
* We then fall into the error recovery code which will treat
* this as if normal completion won the race
*/
There are more of those which I won't bore you with.
[...]
Quoted 4 lines
drivers/ata/libata-sff.c +0
-0
[...]
drivers/ata/libata-sff.c bool ata_sff_qc_fill_rtf(struct ata_queued_cmd *qc) · line 1540
Quoted 7 lines
drivers/ata/libata-sff.c +1
-1@@ -1533,7 +1536,7 @@ bool ata_sff_qc_fill_rtf(struct ata_queued_cmd *qc)* RETURNS: * One if interrupt was handled, zero if not (shared irq). */ -inline unsigned int ata_sff_host_intr (struct ata_port *ap, +unsigned int ata_sff_host_intr (struct ata_port *ap,struct ata_queued_cmd *qc)
Indentation should be adjusted here.
[...]
Quoted 25 lines
drivers/ata/libata-sff.c +21
-0@@ -2073,6 +2117,39 @@ void ata_sff_postreset(struct ata_link *link, unsigned int *classes) }/** +* ata_sff_drain_fifo - Stock FIFO drain logic for SFF controllers +* @ap: port to drain +* @qc: command +* +* Drain the FIFO and device of any stuck data following a command +* failing to complete. In some cases this is neccessary before a +* reset will recover the device. +* +*/ + +void ata_sff_drain_fifo (struct ata_queued_cmd *qc) +{ +int count; +struct ata_port *ap; + +/* We only need to flush incoming data when a command was running */ +if (qc ==NULL || qc->dma_dir ==DMA_TO_DEVICE ) +return ; + + ap = qc->ap; +/* Drain up to 64K of data before we give up this recovery method */
drivers/ata/libata-sff.c void ata_sff_postreset(struct ata_link *link, unsigned int *classes) · line 2150
Quoted 10 lines
drivers/ata/libata-sff.c +10
-0+for (count =0 ; (ap->ops->sff_check_status (ap) &ATA_DRQ ) + && count <32768 ; count++) +ioread16 (ap->ioaddr.data_addr); + +/* Can become DEBUG later */ +if (count) +ata_port_printk (ap,KERN_WARNING , +"drained %d bytes to clear DRQ.\n" , count); + +}
Presumably, you didn't intentionally leave a blank line before the closing brace.
Sorry if you were aware of all that and just sent the patch as a first draft in order to get comments on the actual code.
Regards,
Elias
Quoted 1 line
I'd very much prefer comments to be formatted like this:
Would you... ;)
Quoted 9 lines
drivers/ata/libata-sff.c void ata_sff_postreset(struct ata_link *link, unsigned int *classes) · line 2150
Quoted 6 lines
drivers/ata/libata-sff.c +6
-0+/* Can become DEBUG later */ +if (count) +ata_port_printk (ap,KERN_WARNING , +"drained %d bytes to clear DRQ.\n" , count); + +}Presumably, you didn't intentionally leave a blank line before the closing brace.
Yes I did, it looked cleaner.
Quoted 2 lines
Sorry if you were aware of all that and just sent the patch as a first draft in order to get comments on the actual code.
At the moment the main concern is the code quality - but you are right I've not run the result through the style checker and probably should do so.
Quoted 3 lines · Alan Cox
Quoted 1 line
I'd very much prefer comments to be formatted like this:
Would you... ;)
I suppose I was "bold" here ;).
Regards,
Elias