Date: Wed, 07 Jan 2004 09:57:23 +0000 From: Ian Dowse <iedowse@maths.tcd.ie> To: current@freebsd.org Cc: Soren Schmidt <sos@spider.deepcore.dk> Subject: Possible improvement for ATA resume problems Message-ID: <200401070957.aa66581@salmon.maths.tcd.ie>
next in thread | raw e-mail | index | archive | help
I've had some success with the following patch to avoid ATA-related hangs at resume time and also resume-time errors such as: ad0: FAILURE - READ_MUL status=59<READY,DSC,DRQ,ERROR> error=4<ABORTED> ad0: FAILURE - WRITE_MUL status=51<READY,DSC,ERROR> error=4<ABORTED> The patch does two things: o Re-issue the ATA_SET_MULTI command to ATA disks when reinitialising the channel. This appears to be a simple bug, and explains the ABORTED errors above. It doesn't explain why things used to recover for me after one or two of these errors though. Also reissue the ATA_SETFEATURES commands relating to caching. Non-disk devices might need to do something similar too. o Ensure that no normal requests can get queued while a channel is being reinitialised. This stops regular I/O operations from happening before a channel has been fully set up after a resume. It works by putting any such requests on a "deferred" list, and moving them back when the reinit is complete. Note that the root cause here is probably the fact that the ATA resume method blocks while waiting for some operations to complete, giving normal processes a chance to run before the resume has fully completed. The deferred queue approach is fairly gross - I guess a much better way would be a mechanism to allow a single request to be executed synchronously while the ATA channel remains locked (optionally using DELAY() calls and polling when sleeping is not permitted). I've no idea if this will help much in general, but it has certainly made suspend/resume work more reliably for me. Ian Index: ata-all.c =================================================================== RCS file: /dump/FreeBSD-CVS/src/sys/dev/ata/ata-all.c,v retrieving revision 1.198 diff -u -r1.198 ata-all.c --- ata-all.c 30 Nov 2003 16:27:58 -0000 1.198 +++ ata-all.c 7 Jan 2004 08:55:20 -0000 @@ -122,6 +122,7 @@ bzero(&ch->queue_mtx, sizeof(struct mtx)); mtx_init(&ch->queue_mtx, "ATA queue lock", MTX_DEF, 0); TAILQ_INIT(&ch->ata_queue); + TAILQ_INIT(&ch->ata_deferred_queue); /* initialise device(s) on this channel */ ch->locking(ch, ATA_LF_LOCK); @@ -231,7 +232,7 @@ ata_reinit(struct ata_channel *ch) { struct ata_request *request = ch->running; - int devices, misdev, newdev; + int devices, keptdev, misdev, newdev; if (!ch->r_irq) return ENXIO; @@ -239,6 +240,8 @@ /* reset the HW */ ata_printf(ch, -1, "resetting devices ..\n"); ATA_FORCELOCK_CH(ch, ATA_CONTROL); + ch->flags |= ATA_REINIT_ACTIVE; + KASSERT(TAILQ_EMPTY(&ch->ata_queue), ("ata_reinit: queue not empty!")); ch->running = NULL; devices = ch->devices; ch->hw.reset(ch); @@ -275,6 +278,16 @@ /* identify whats present on this channel now */ ata_identify_devices(ch); + /* reset/reconfigure devices that are still there */ + if ((keptdev = devices & ch->devices)) { + if ((keptdev & (ATA_ATA_MASTER | ATA_ATAPI_MASTER)) && + ch->device[MASTER].reset) + ch->device[MASTER].reset(&ch->device[MASTER]); + if ((keptdev & (ATA_ATA_SLAVE | ATA_ATAPI_SLAVE)) && + ch->device[SLAVE].reset) + ch->device[SLAVE].reset(&ch->device[SLAVE]); + } + /* attach new devices that appeared during reset */ if ((newdev = ~devices & ch->devices)) { if ((newdev & (ATA_ATA_MASTER | ATA_ATAPI_MASTER)) && @@ -295,6 +308,15 @@ atapi_cam_reinit_bus(ch); #endif + /* Now it's safe to execute any normal requests that were deferred. */ + mtx_lock(&ch->queue_mtx); + while (!TAILQ_EMPTY(&ch->ata_deferred_queue)) { + request = TAILQ_FIRST(&ch->ata_deferred_queue); + TAILQ_REMOVE(&ch->ata_deferred_queue, request, chain); + TAILQ_INSERT_TAIL(&ch->ata_queue, request, chain); + } + mtx_unlock(&ch->queue_mtx); + ch->flags &= ~ATA_REINIT_ACTIVE; printf("done\n"); return 0; } @@ -556,7 +578,7 @@ if (request) { request->device = atadev; request->u.ata.command = command; - request->flags = (ATA_R_READ | ATA_R_QUIET); + request->flags = (ATA_R_READ | ATA_R_QUIET | ATA_R_DONTDEFER); request->data = (caddr_t)atacap; request->timeout = 2; request->retries = 3; Index: ata-all.h =================================================================== RCS file: /dump/FreeBSD-CVS/src/sys/dev/ata/ata-all.h,v retrieving revision 1.68 diff -u -r1.68 ata-all.h --- ata-all.h 28 Nov 2003 19:01:28 -0000 1.68 +++ ata-all.h 7 Jan 2004 02:46:06 -0000 @@ -195,6 +195,7 @@ #define ATA_R_AT_HEAD 0x0200 #define ATA_R_REQUEUE 0x0400 #define ATA_R_SKIPSTART 0x0800 +#define ATA_R_DONTDEFER 0x1000 void (*callback)(struct ata_request *request); int retries; /* retry count */ @@ -218,6 +219,7 @@ void *softc; /* ptr to softc for device */ void (*attach)(struct ata_device *atadev); void (*detach)(struct ata_device *atadev); + void (*reset)(struct ata_device *atadev); void (*start)(struct ata_device *atadev); int flags; #define ATA_D_USE_CHS 0x0001 @@ -274,6 +276,8 @@ int offset; }; +TAILQ_HEAD(ata_reqhead, ata_request); + /* structure describing an ATA channel */ struct ata_channel { struct device *dev; /* device handle */ @@ -289,6 +293,7 @@ #define ATA_USE_PC98GEOM 0x04 #define ATA_ATAPI_DMA_RO 0x08 #define ATA_48BIT_ACTIVE 0x10 +#define ATA_REINIT_ACTIVE 0x20 struct ata_device device[2]; /* devices on this channel */ #define MASTER 0x00 @@ -310,7 +315,8 @@ #define ATA_LF_UNLOCK 0x0002 struct mtx queue_mtx; - TAILQ_HEAD(, ata_request) ata_queue; /* head of ATA queue */ + struct ata_reqhead ata_queue; /* head of ATA queue */ + struct ata_reqhead ata_deferred_queue; void *running; /* currently running request */ }; Index: ata-disk.c =================================================================== RCS file: /dump/FreeBSD-CVS/src/sys/dev/ata/ata-disk.c,v retrieving revision 1.164 diff -u -r1.164 ata-disk.c --- ata-disk.c 11 Nov 2003 14:55:35 -0000 1.164 +++ ata-disk.c 7 Jan 2004 02:05:59 -0000 @@ -55,6 +55,7 @@ /* prototypes */ static void ad_detach(struct ata_device *); +static void ad_reset(struct ata_device *); static void ad_start(struct ata_device *); static void ad_done(struct ata_request *); static disk_open_t adopen; @@ -119,28 +120,13 @@ lbasize48 > 268435455) adp->total_secs = lbasize48; - /* enable read caching */ - ata_controlcmd(atadev, ATA_SETFEATURES, ATA_SF_ENAB_RCACHE, 0, 0); - - /* enable write caching if enabled */ - if (ata_wc) - ata_controlcmd(atadev, ATA_SETFEATURES, ATA_SF_ENAB_WCACHE, 0, 0); - else - ata_controlcmd(atadev, ATA_SETFEATURES, ATA_SF_DIS_WCACHE, 0, 0); - - /* use multiple sectors/interrupt if device supports it */ - adp->max_iosize = DEV_BSIZE; - if (ad_version(atadev->param->version_major)) { - int secsperint = max(1, min(atadev->param->sectors_intr, 16)); - - if (!ata_controlcmd(atadev, ATA_SET_MULTI, 0, 0, secsperint)) - adp->max_iosize = secsperint * DEV_BSIZE; - } + atadev->softc = adp; + ad_reset(atadev); /* setup the function ptrs */ atadev->detach = ad_detach; + atadev->reset = ad_reset; atadev->start = ad_start; - atadev->softc = adp; /* lets create the disk device */ adp->disk.d_open = adopen; @@ -167,6 +153,30 @@ } static void +ad_reset(struct ata_device *atadev) +{ + struct ad_softc *adp = atadev->softc; + + /* enable read caching */ + ata_controlcmd(atadev, ATA_SETFEATURES, ATA_SF_ENAB_RCACHE, 0, 0); + + /* enable write caching if enabled */ + if (ata_wc) + ata_controlcmd(atadev, ATA_SETFEATURES, ATA_SF_ENAB_WCACHE, 0, 0); + else + ata_controlcmd(atadev, ATA_SETFEATURES, ATA_SF_DIS_WCACHE, 0, 0); + + /* use multiple sectors/interrupt if device supports it */ + adp->max_iosize = DEV_BSIZE; + if (ad_version(atadev->param->version_major)) { + int secsperint = max(1, min(atadev->param->sectors_intr, 16)); + + if (!ata_controlcmd(atadev, ATA_SET_MULTI, 0, 0, secsperint)) + adp->max_iosize = secsperint * DEV_BSIZE; + } +} + +static void ad_detach(struct ata_device *atadev) { struct ad_softc *adp = atadev->softc; @@ -185,6 +195,7 @@ ata_free_lun(&adp_lun_map, adp->lun); atadev->attach = NULL; atadev->detach = NULL; + atadev->reset = NULL; atadev->start = NULL; atadev->softc = NULL; atadev->flags = 0; Index: ata-queue.c =================================================================== RCS file: /dump/FreeBSD-CVS/src/sys/dev/ata/ata-queue.c,v retrieving revision 1.13 diff -u -r1.13 ata-queue.c --- ata-queue.c 16 Dec 2003 19:41:38 -0000 1.13 +++ ata-queue.c 7 Jan 2004 08:55:21 -0000 @@ -76,16 +76,25 @@ void ata_queue_request(struct ata_request *request) { + struct ata_reqhead *qhead; + /* mark request as virgin (it might be a reused one) */ request->result = request->status = request->error = 0; request->flags &= ~ATA_R_DONE; + qhead = &request->device->channel->ata_queue; + /* Don't let normal requests interfere with reinitialisation. */ + if ((request->device->channel->flags & ATA_REINIT_ACTIVE) && + (request->flags & ATA_R_DONTDEFER) == 0) { + qhead = &request->device->channel->ata_deferred_queue; + } + /* put request on the locked queue at the specified location */ mtx_lock(&request->device->channel->queue_mtx); if (request->flags & ATA_R_AT_HEAD) - TAILQ_INSERT_HEAD(&request->device->channel->ata_queue, request, chain); + TAILQ_INSERT_HEAD(qhead, request, chain); else - TAILQ_INSERT_TAIL(&request->device->channel->ata_queue, request, chain); + TAILQ_INSERT_TAIL(qhead, request, chain); mtx_unlock(&request->device->channel->queue_mtx); /* should we skip start ? */ @@ -116,7 +125,7 @@ request->u.ata.lba = lba; request->u.ata.count = count; request->u.ata.feature = feature; - request->flags = ATA_R_CONTROL; + request->flags = ATA_R_CONTROL | ATA_R_DONTDEFER; request->timeout = 5; ata_queue_request(request); error = request->result; Index: atapi-cd.c =================================================================== RCS file: /dump/FreeBSD-CVS/src/sys/dev/ata/atapi-cd.c,v retrieving revision 1.157 diff -u -r1.157 atapi-cd.c --- atapi-cd.c 7 Dec 2003 23:15:22 -0000 1.157 +++ atapi-cd.c 7 Jan 2004 08:55:21 -0000 @@ -208,6 +208,7 @@ ata_free_lun(&acd_lun_map, cdp->lun); atadev->attach = NULL; atadev->detach = NULL; + atadev->reset = NULL; atadev->start = NULL; atadev->softc = NULL; atadev->flags = 0; Index: atapi-fd.c =================================================================== RCS file: /dump/FreeBSD-CVS/src/sys/dev/ata/atapi-fd.c,v retrieving revision 1.89 diff -u -r1.89 atapi-fd.c --- atapi-fd.c 11 Nov 2003 14:55:35 -0000 1.89 +++ atapi-fd.c 7 Jan 2004 01:46:37 -0000 @@ -126,6 +126,7 @@ ata_free_lun(&afd_lun_map, fdp->lun); atadev->attach = NULL; atadev->detach = NULL; + atadev->reset = NULL; atadev->start = NULL; atadev->softc = NULL; atadev->flags = 0; Index: atapi-tape.c =================================================================== RCS file: /dump/FreeBSD-CVS/src/sys/dev/ata/atapi-tape.c,v retrieving revision 1.84 diff -u -r1.84 atapi-tape.c --- atapi-tape.c 11 Nov 2003 14:55:36 -0000 1.84 +++ atapi-tape.c 7 Jan 2004 01:46:55 -0000 @@ -174,6 +174,7 @@ ata_free_lun(&ast_lun_map, stp->lun); atadev->attach = NULL; atadev->detach = NULL; + atadev->reset = NULL; atadev->start = NULL; atadev->softc = NULL; atadev->flags = 0;
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200401070957.aa66581>