From 0387836deb1d721b3428c132a54bcab1a4a95115 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B8ren=20Schmidt?= Date: Wed, 14 Sep 2005 12:45:06 +0000 Subject: [PATCH] Harden the hotplug support for SATA devices. This also fixes a few races that was present in the timeout/detach code. Sponsored by: pair.com --- sys/dev/ata/ata-all.c | 49 ++++++++++++++++++++------ sys/dev/ata/ata-chipset.c | 6 ++-- sys/dev/ata/ata-lowlevel.c | 8 +++-- sys/dev/ata/ata-queue.c | 71 +++++++++++++++++--------------------- 4 files changed, 80 insertions(+), 54 deletions(-) diff --git a/sys/dev/ata/ata-all.c b/sys/dev/ata/ata-all.c index a779409ee84e..40916d965039 100644 --- a/sys/dev/ata/ata-all.c +++ b/sys/dev/ata/ata-all.c @@ -158,7 +158,7 @@ ata_detach(device_t dev) device_t *children; int nchildren, i; - /* check that we have a vaild channel to detach */ + /* check that we have a valid channel to detach */ if (!ch->r_irq) return ENXIO; @@ -183,10 +183,11 @@ int ata_reinit(device_t dev) { struct ata_channel *ch = device_get_softc(dev); + struct ata_request *request; device_t *children; int nchildren, i; - /* check that we have a vaild channel to reinit */ + /* check that we have a valid channel to reinit */ if (!ch || !ch->r_irq) return ENXIO; @@ -216,12 +217,25 @@ ata_reinit(device_t dev) * this child we need to inform the request that the * device is gone and remove it from ch->running */ + mtx_lock(&ch->state_mtx); if (ch->running && ch->running->dev == children[i]) { - device_printf(ch->running->dev, - "FAILURE - device detached\n"); - ch->running->dev = NULL; - ch->running = NULL; + callout_stop(&ch->running->callout); + request = ch->running; + ch->running = NULL; } + else + request = NULL; + mtx_unlock(&ch->state_mtx); + + if (request) { + request->result = ENXIO; + device_printf(request->dev, + "FAILURE - device detached\n"); + + /* if not timeout finish request here */ + if (!(request->flags & ATA_R_TIMEOUT)) + ata_finish(request); + } device_delete_child(dev, children[i]); } } @@ -230,7 +244,23 @@ ata_reinit(device_t dev) } /* catch request in ch->running if we havn't already */ - ata_catch_inflight(dev); + mtx_lock(&ch->state_mtx); + if ((request = ch->running)) + callout_stop(&request->callout); + ch->running = NULL; + mtx_unlock(&ch->state_mtx); + + /* if we got one put it on the queue again */ + if (request) { + device_printf(request->dev, + "WARNING - %s requeued due to channel reset", + ata_cmd2str(request)); + if (!(request->flags & (ATA_R_ATAPI | ATA_R_CONTROL))) + printf(" LBA=%llu", (unsigned long long)request->u.ata.lba); + printf("\n"); + request->flags |= ATA_R_REQUEUE; + ata_queue_request(request); + } /* we're done release the channel for new work */ mtx_lock(&ch->state_mtx); @@ -297,7 +327,7 @@ ata_interrupt(void *data) mtx_lock(&ch->state_mtx); do { /* do we have a running request */ - if (!(request = ch->running) || (request->flags & ATA_R_TIMEOUT)) + if (!(request = ch->running)) break; ATA_DEBUG_RQ(request, "interrupt"); @@ -311,8 +341,7 @@ ata_interrupt(void *data) /* check for the right state */ if (ch->state != ATA_ACTIVE && ch->state != ATA_STALL_QUEUE) { - device_printf(request->dev, - "interrupt state=%d unexpected\n", ch->state); + device_printf(request->dev, "interrupt on idle channel ignored\n"); break; } diff --git a/sys/dev/ata/ata-chipset.c b/sys/dev/ata/ata-chipset.c index b00229dea00d..0bba2a6f70e3 100644 --- a/sys/dev/ata/ata-chipset.c +++ b/sys/dev/ata/ata-chipset.c @@ -313,13 +313,12 @@ static void ata_sata_phy_event(void *context, int dummy) { struct ata_connect_task *tp = (struct ata_connect_task *)context; + struct ata_channel *ch = device_get_softc(tp->dev); device_t *children; int nchildren, i; mtx_lock(&Giant); /* newbus suckage it needs Giant */ if (tp->action == ATA_C_ATTACH) { - struct ata_channel *ch = device_get_softc(tp->dev); - device_printf(tp->dev, "CONNECTED\n"); ata_sata_connect(ch); ata_identify(tp->dev); @@ -331,6 +330,9 @@ ata_sata_phy_event(void *context, int dummy) device_delete_child(tp->dev, children[i]); free(children, M_TEMP); } + mtx_lock(&ch->state_mtx); + ch->state = ATA_IDLE; + mtx_unlock(&ch->state_mtx); device_printf(tp->dev, "DISCONNECTED\n"); } mtx_unlock(&Giant); /* suckage code dealt with, release Giant */ diff --git a/sys/dev/ata/ata-lowlevel.c b/sys/dev/ata/ata-lowlevel.c index 2751e877c3b4..29135b72cece 100644 --- a/sys/dev/ata/ata-lowlevel.c +++ b/sys/dev/ata/ata-lowlevel.c @@ -117,7 +117,8 @@ ata_begin_transaction(struct ata_request *request) /* if write command output the data */ if (write) { if (ata_wait(ch, atadev, (ATA_S_READY | ATA_S_DRQ)) < 0) { - device_printf(request->dev,"timeout waiting for write DRQ"); + device_printf(request->dev, + "timeout waiting for write DRQ\n"); request->result = EIO; goto begin_finished; } @@ -278,7 +279,8 @@ ata_end_transaction(struct ata_request *request) /* if read data get it */ if (request->flags & ATA_R_READ) { if (ata_wait(ch, atadev, (ATA_S_READY | ATA_S_DRQ)) < 0) { - device_printf(request->dev, "timeout waiting for read DRQ"); + device_printf(request->dev, + "timeout waiting for read DRQ\n"); request->result = EIO; goto end_finished; } @@ -302,7 +304,7 @@ ata_end_transaction(struct ata_request *request) /* if we get an error here we are done with the HW */ if (ata_wait(ch, atadev, (ATA_S_READY | ATA_S_DRQ)) < 0) { device_printf(request->dev, - "timeout waiting for write DRQ"); + "timeout waiting for write DRQ\n"); request->status = ATA_IDX_INB(ch, ATA_STATUS); goto end_finished; } diff --git a/sys/dev/ata/ata-queue.c b/sys/dev/ata/ata-queue.c index c1a7e6174373..da58c403b4ec 100644 --- a/sys/dev/ata/ata-queue.c +++ b/sys/dev/ata/ata-queue.c @@ -252,7 +252,8 @@ ata_completed(void *context, int dummy) * if reinit succeeds and the device doesn't get detached and * there are retries left we reinject this request */ - if (!ata_reinit(ch->dev) && request->dev && (request->retries-- > 0)) { + if (!ata_reinit(ch->dev) && !request->result && + (request->retries-- > 0)) { if (!(request->flags & ATA_R_QUIET)) { device_printf(request->dev, "TIMEOUT - %s retrying (%d retr%s left)", @@ -270,18 +271,19 @@ ata_completed(void *context, int dummy) } /* ran out of good intentions so finish with error */ - if (!(request->flags & ATA_R_QUIET)) { - if (request->dev) { - device_printf(request->dev, - "FAILURE - %s timed out", - ata_cmd2str(request)); - if (!(request->flags & (ATA_R_ATAPI | ATA_R_CONTROL))) - printf(" LBA=%llu", (unsigned long long)request->u.ata.lba); - printf("\n"); + if (!request->result) { + if (!(request->flags & ATA_R_QUIET)) { + if (request->dev) { + device_printf(request->dev, "FAILURE - %s timed out", + ata_cmd2str(request)); + if (!(request->flags & (ATA_R_ATAPI | ATA_R_CONTROL))) + printf(" LBA=%llu", + (unsigned long long)request->u.ata.lba); + printf("\n"); + } } - } - if (!request->result) request->result = EIO; + } } else { /* if this is a soft ECC error warn about it */ @@ -451,11 +453,12 @@ ata_timeout(struct ata_request *request) ATA_DEBUG_RQ(request, "timeout"); /* - * flag the request ATA_R_TIMEOUT and NULL out the running request - * so we wont loose the race with an eventual interrupt arriving late - * and dont reissue the command in ata_catch_inflight() + * if we have an ATA_ACTIVE request running, we flag the request + * ATA_R_TIMEOUT so ata_finish will handle it correctly + * also NULL out the running request so we wont loose + * the race with an eventual interrupt arriving late */ - if (ch->state == ATA_ACTIVE || ch->state == ATA_STALL_QUEUE) { + if (ch->state == ATA_ACTIVE) { request->flags |= ATA_R_TIMEOUT; ch->running = NULL; mtx_unlock(&ch->state_mtx); @@ -464,30 +467,6 @@ ata_timeout(struct ata_request *request) } else { mtx_unlock(&ch->state_mtx); - device_printf(request->dev, "timeout state=%d unexpected\n", ch->state); - } -} - -void -ata_catch_inflight(device_t dev) -{ - struct ata_channel *ch = device_get_softc(dev); - struct ata_request *request; - - mtx_lock(&ch->state_mtx); - if ((request = ch->running)) - callout_stop(&request->callout); - ch->running = NULL; - mtx_unlock(&ch->state_mtx); - if (request) { - device_printf(request->dev, - "WARNING - %s requeued due to channel reset", - ata_cmd2str(request)); - if (!(request->flags & (ATA_R_ATAPI | ATA_R_CONTROL))) - printf(" LBA=%llu", (unsigned long long)request->u.ata.lba); - printf("\n"); - request->flags |= ATA_R_REQUEUE; - ata_queue_request(request); } } @@ -497,6 +476,20 @@ ata_fail_requests(device_t dev) struct ata_channel *ch = device_get_softc(device_get_parent(dev)); struct ata_request *request; + /* do we have any outstanding request to care about ?*/ + mtx_lock(&ch->state_mtx); + if ((request = ch->running) && (!dev || request->dev == dev)) { + callout_stop(&request->callout); + ch->running = NULL; + } + else + request = NULL; + mtx_unlock(&ch->state_mtx); + if (request) { + request->result = ENXIO; + ata_finish(request); + } + /* fail all requests queued on this channel for device dev if !NULL */ mtx_lock(&ch->queue_mtx); while ((request = TAILQ_FIRST(&ch->ata_queue))) {