Commit 7fb6ec28 authored by Jeff Garzik's avatar Jeff Garzik

[libata] fix PIO completion race

Make sure we that completion is the final action we take; prior to this
change, another CPU may have changed ap->pio_task_state before we tested
it a final time.

Spotted by, and original patch by Albert Lee @ IBM.

Also includes a minor optimization:  eliminate a ton of unnecessary
queue_work() calls, simply by jumping to the beginning of the FSM
function ata_pio_task().
parent 065d9cac
...@@ -2465,9 +2465,12 @@ static unsigned long ata_pio_poll(struct ata_port *ap) ...@@ -2465,9 +2465,12 @@ static unsigned long ata_pio_poll(struct ata_port *ap)
* *
* LOCKING: * LOCKING:
* None. (executing in kernel thread context) * None. (executing in kernel thread context)
*
* RETURNS:
* Non-zero if qc completed, zero otherwise.
*/ */
static void ata_pio_complete (struct ata_port *ap) static int ata_pio_complete (struct ata_port *ap)
{ {
struct ata_queued_cmd *qc; struct ata_queued_cmd *qc;
u8 drv_stat; u8 drv_stat;
...@@ -2486,14 +2489,14 @@ static void ata_pio_complete (struct ata_port *ap) ...@@ -2486,14 +2489,14 @@ static void ata_pio_complete (struct ata_port *ap)
if (drv_stat & (ATA_BUSY | ATA_DRQ)) { if (drv_stat & (ATA_BUSY | ATA_DRQ)) {
ap->pio_task_state = PIO_ST_LAST_POLL; ap->pio_task_state = PIO_ST_LAST_POLL;
ap->pio_task_timeout = jiffies + ATA_TMOUT_PIO; ap->pio_task_timeout = jiffies + ATA_TMOUT_PIO;
return; return 0;
} }
} }
drv_stat = ata_wait_idle(ap); drv_stat = ata_wait_idle(ap);
if (!ata_ok(drv_stat)) { if (!ata_ok(drv_stat)) {
ap->pio_task_state = PIO_ST_ERR; ap->pio_task_state = PIO_ST_ERR;
return; return 0;
} }
qc = ata_qc_from_tag(ap, ap->active_tag); qc = ata_qc_from_tag(ap, ap->active_tag);
...@@ -2502,6 +2505,10 @@ static void ata_pio_complete (struct ata_port *ap) ...@@ -2502,6 +2505,10 @@ static void ata_pio_complete (struct ata_port *ap)
ap->pio_task_state = PIO_ST_IDLE; ap->pio_task_state = PIO_ST_IDLE;
ata_poll_qc_complete(qc, drv_stat); ata_poll_qc_complete(qc, drv_stat);
/* another command may start at this point */
return 1;
} }
...@@ -2709,7 +2716,7 @@ static void __atapi_pio_bytes(struct ata_queued_cmd *qc, unsigned int bytes) ...@@ -2709,7 +2716,7 @@ static void __atapi_pio_bytes(struct ata_queued_cmd *qc, unsigned int bytes)
next_sg: next_sg:
if (unlikely(qc->cursg >= qc->n_elem)) { if (unlikely(qc->cursg >= qc->n_elem)) {
/* /*
* The end of qc->sg is reached and the device expects * The end of qc->sg is reached and the device expects
* more data to transfer. In order not to overrun qc->sg * more data to transfer. In order not to overrun qc->sg
* and fulfill length specified in the byte count register, * and fulfill length specified in the byte count register,
...@@ -2721,7 +2728,7 @@ static void __atapi_pio_bytes(struct ata_queued_cmd *qc, unsigned int bytes) ...@@ -2721,7 +2728,7 @@ static void __atapi_pio_bytes(struct ata_queued_cmd *qc, unsigned int bytes)
unsigned int i; unsigned int i;
if (words) /* warning if bytes > 1 */ if (words) /* warning if bytes > 1 */
printk(KERN_WARNING "ata%u: %u bytes trailing data\n", printk(KERN_WARNING "ata%u: %u bytes trailing data\n",
ap->id, bytes); ap->id, bytes);
for (i = 0; i < words; i++) for (i = 0; i < words; i++)
...@@ -2849,9 +2856,7 @@ static void ata_pio_block(struct ata_port *ap) ...@@ -2849,9 +2856,7 @@ static void ata_pio_block(struct ata_port *ap)
if (is_atapi_taskfile(&qc->tf)) { if (is_atapi_taskfile(&qc->tf)) {
/* no more data to transfer or unsupported ATAPI command */ /* no more data to transfer or unsupported ATAPI command */
if ((status & ATA_DRQ) == 0) { if ((status & ATA_DRQ) == 0) {
ap->pio_task_state = PIO_ST_IDLE; ap->pio_task_state = PIO_ST_LAST;
ata_poll_qc_complete(qc, status);
return; return;
} }
...@@ -2887,7 +2892,12 @@ static void ata_pio_error(struct ata_port *ap) ...@@ -2887,7 +2892,12 @@ static void ata_pio_error(struct ata_port *ap)
static void ata_pio_task(void *_data) static void ata_pio_task(void *_data)
{ {
struct ata_port *ap = _data; struct ata_port *ap = _data;
unsigned long timeout = 0; unsigned long timeout;
int qc_completed;
fsm_start:
timeout = 0;
qc_completed = 0;
switch (ap->pio_task_state) { switch (ap->pio_task_state) {
case PIO_ST_IDLE: case PIO_ST_IDLE:
...@@ -2898,7 +2908,7 @@ static void ata_pio_task(void *_data) ...@@ -2898,7 +2908,7 @@ static void ata_pio_task(void *_data)
break; break;
case PIO_ST_LAST: case PIO_ST_LAST:
ata_pio_complete(ap); qc_completed = ata_pio_complete(ap);
break; break;
case PIO_ST_POLL: case PIO_ST_POLL:
...@@ -2913,10 +2923,9 @@ static void ata_pio_task(void *_data) ...@@ -2913,10 +2923,9 @@ static void ata_pio_task(void *_data)
} }
if (timeout) if (timeout)
queue_delayed_work(ata_wq, &ap->pio_task, queue_delayed_work(ata_wq, &ap->pio_task, timeout);
timeout); else if (!qc_completed)
else goto fsm_start;
queue_work(ata_wq, &ap->pio_task);
} }
static void atapi_request_sense(struct ata_port *ap, struct ata_device *dev, static void atapi_request_sense(struct ata_port *ap, struct ata_device *dev,
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment