From f470ab4283d249c404c94c01cc4a58af6db0703e Mon Sep 17 00:00:00 2001 From: scottl Date: Sun, 16 Jan 2005 07:34:26 +0000 Subject: Lock the AMR driver: - Introduce the amr_io_lock to control access to command queues, bio queues, and the hardware. - Eliminate the taskqueue and do all completion processing in the ithread. - Assign a static slot number to each command instead of doing a linear search for free slots each time a command is needed. - Modify the interrupt handler to more closely match what Linux does, for safety. --- sys/dev/amr/amr.c | 193 ++++++++++++++++++++++++------------------------- sys/dev/amr/amr_cam.c | 16 ++++ sys/dev/amr/amr_disk.c | 2 +- sys/dev/amr/amr_pci.c | 7 +- sys/dev/amr/amrvar.h | 8 +- 5 files changed, 120 insertions(+), 106 deletions(-) (limited to 'sys') diff --git a/sys/dev/amr/amr.c b/sys/dev/amr/amr.c index 62ce551..473152e 100644 --- a/sys/dev/amr/amr.c +++ b/sys/dev/amr/amr.c @@ -196,13 +196,6 @@ amr_attach(struct amr_softc *sc) TAILQ_INIT(&sc->amr_ready); bioq_init(&sc->amr_bioq); -#if __FreeBSD_version >= 500005 - /* - * Initialise command-completion task. - */ - TASK_INIT(&sc->amr_task_complete, 0, amr_complete, sc); -#endif - debug(2, "queue init done"); /* @@ -350,6 +343,9 @@ amr_free(struct amr_softc *sc) /* destroy control device */ if( sc->amr_dev_t != (struct cdev *)NULL) destroy_dev(sc->amr_dev_t); + + if (mtx_initialized(&sc->amr_io_lock)) + mtx_destroy(&sc->amr_io_lock); } /******************************************************************************* @@ -361,8 +357,10 @@ amr_submit_bio(struct amr_softc *sc, struct bio *bio) { debug_called(2); + mtx_lock(&sc->amr_io_lock); amr_enqueue_bio(sc, bio); amr_startio(sc); + mtx_unlock(&sc->amr_io_lock); return(0); } @@ -413,7 +411,6 @@ amr_ioctl(struct cdev *dev, u_long cmd, caddr_t addr, int32_t flag, d_thread_t * } arg; struct amr_command *ac; struct amr_mailbox_ioctl *mbi; - struct amr_passthrough *ap; void *dp, *au_buffer; unsigned long au_length; unsigned char *au_cmd; @@ -463,7 +460,6 @@ amr_ioctl(struct cdev *dev, u_long cmd, caddr_t addr, int32_t flag, d_thread_t * error = 0; dp = NULL; - ap = NULL; ac = NULL; /* handle inbound data buffer */ @@ -471,11 +467,14 @@ amr_ioctl(struct cdev *dev, u_long cmd, caddr_t addr, int32_t flag, d_thread_t * if ((dp = malloc(au_length, M_DEVBUF, M_WAITOK)) == NULL) return(ENOMEM); - if ((error = copyin(au_buffer, dp, au_length)) != 0) - goto out; + if ((error = copyin(au_buffer, dp, au_length)) != 0) { + free(dp, M_DEVBUF); + return (error); + } debug(2, "copyin %ld bytes from %p -> %p", au_length, au_buffer, dp); } + mtx_lock(&sc->amr_io_lock); if ((ac = amr_alloccmd(sc)) == NULL) { error = ENOMEM; goto out; @@ -483,29 +482,28 @@ amr_ioctl(struct cdev *dev, u_long cmd, caddr_t addr, int32_t flag, d_thread_t * /* handle SCSI passthrough command */ if (au_cmd[0] == AMR_CMD_PASS) { - if ((ap = malloc(sizeof(*ap), M_DEVBUF, M_WAITOK | M_ZERO)) == NULL) { - error = ENOMEM; - goto out; - } + struct amr_passthrough ap; /* 60 bytes */ + int len; /* copy cdb */ - ap->ap_cdb_length = au_cmd[2]; - bcopy(au_cmd + 3, ap->ap_cdb, ap->ap_cdb_length); + len = au_cmd[2]; + ap.ap_cdb_length = len; + bcopy(au_cmd + 3, ap.ap_cdb, len); /* build passthrough */ - ap->ap_timeout = au_cmd[ap->ap_cdb_length + 3] & 0x07; - ap->ap_ars = (au_cmd[ap->ap_cdb_length + 3] & 0x08) ? 1 : 0; - ap->ap_islogical = (au_cmd[ap->ap_cdb_length + 3] & 0x80) ? 1 : 0; - ap->ap_logical_drive_no = au_cmd[ap->ap_cdb_length + 4]; - ap->ap_channel = au_cmd[ap->ap_cdb_length + 5]; - ap->ap_scsi_id = au_cmd[ap->ap_cdb_length + 6]; - ap->ap_request_sense_length = 14; - ap->ap_data_transfer_length = au_length; + ap.ap_timeout = au_cmd[len + 3] & 0x07; + ap.ap_ars = (au_cmd[len + 3] & 0x08) ? 1 : 0; + ap.ap_islogical = (au_cmd[len + 3] & 0x80) ? 1 : 0; + ap.ap_logical_drive_no = au_cmd[len + 4]; + ap.ap_channel = au_cmd[len + 5]; + ap.ap_scsi_id = au_cmd[len + 6]; + ap.ap_request_sense_length = 14; + ap.ap_data_transfer_length = au_length; /* XXX what about the request-sense area? does the caller want it? */ /* build command */ - ac->ac_data = ap; - ac->ac_length = sizeof(*ap); + ac->ac_data = ≈ + ac->ac_length = sizeof(struct amr_passthrough); ac->ac_flags |= AMR_CMD_DATAOUT; ac->ac_ccb_data = dp; ac->ac_ccb_length = au_length; @@ -541,20 +539,24 @@ amr_ioctl(struct cdev *dev, u_long cmd, caddr_t addr, int32_t flag, d_thread_t * goto out; /* copy out data and set status */ - if (au_length != 0) + if (au_length != 0) { + mtx_unlock(&sc->amr_io_lock); error = copyout(dp, au_buffer, au_length); + mtx_lock(&sc->amr_io_lock); + } debug(2, "copyout %ld bytes from %p -> %p", au_length, dp, au_buffer); if (dp != NULL) debug(2, "%16d", (int)dp); *au_statusp = ac->ac_status; out: - if (dp != NULL) - free(dp, M_DEVBUF); - if (ap != NULL) - free(ap, M_DEVBUF); - if (ac != NULL) - amr_releasecmd(ac); + /* + * At this point, we know that there is a lock held and that these + * objects have been allocated. + */ + free(dp, M_DEVBUF); + amr_releasecmd(ac); + mtx_unlock(&sc->amr_io_lock); return(error); } @@ -600,6 +602,8 @@ amr_query_controller(struct amr_softc *sc) struct amr_enquiry *ae; int ldrv; + mtx_lock(&sc->amr_io_lock); + /* * If we haven't found the real limit yet, let us have a couple of commands in * order to be able to probe. @@ -639,6 +643,7 @@ amr_query_controller(struct amr_softc *sc) */ if ((ap = amr_enquiry(sc, 2048, AMR_CMD_CONFIG, AMR_CONFIG_PRODUCT_INFO, 0)) == NULL) { device_printf(sc->amr_dev, "can't obtain product data from controller\n"); + mtx_unlock(&sc->amr_io_lock); return(1); } sc->amr_maxdrives = 40; @@ -653,6 +658,7 @@ amr_query_controller(struct amr_softc *sc) if ((ae = (struct amr_enquiry *)amr_enquiry(sc, 2048, AMR_CMD_EXT_ENQUIRY2, 0, 0)) == NULL) { if ((ae = (struct amr_enquiry *)amr_enquiry(sc, 2048, AMR_CMD_ENQUIRY, 0, 0)) == NULL) { device_printf(sc->amr_dev, "can't obtain configuration data from controller\n"); + mtx_unlock(&sc->amr_io_lock); return(1); } ae->ae_signature = 0; @@ -687,6 +693,7 @@ amr_query_controller(struct amr_softc *sc) */ sc->amr_maxio = imin(sc->amr_maxio, AMR_LIMITCMD); + mtx_unlock(&sc->amr_io_lock); return(0); } @@ -752,6 +759,7 @@ amr_flush(struct amr_softc *sc) /* get ourselves a command buffer */ error = 1; + mtx_lock(&sc->amr_io_lock); if ((ac = amr_alloccmd(sc)) == NULL) goto out; /* set command flags */ @@ -768,6 +776,7 @@ amr_flush(struct amr_softc *sc) out: if (ac != NULL) amr_releasecmd(ac); + mtx_unlock(&sc->amr_io_lock); return(error); } @@ -859,7 +868,7 @@ static void amr_completeio(struct amr_command *ac) { struct amr_softc *sc = ac->ac_sc; - + if (ac->ac_status != AMR_STATUS_SUCCESS) { /* could be more verbose here? */ ac->ac_bio->bio_error = EIO; ac->ac_bio->bio_flags |= BIO_ERROR; @@ -894,16 +903,16 @@ amr_bio_command(struct amr_softc *sc, struct amr_command **acp) ac = NULL; error = 0; + /* get a command */ + if ((ac = amr_alloccmd(sc)) == NULL) + return (ENOMEM); + /* get a bio to work on */ - if ((bio = amr_dequeue_bio(sc)) == NULL) - goto out; + if ((bio = amr_dequeue_bio(sc)) == NULL) { + amr_releasecmd(ac); + return (0); + } - /* get a command */ - if ((ac = amr_alloccmd(sc)) == NULL) { - error = ENOMEM; - goto out; - } - /* connect the bio to the command */ ac->ac_complete = amr_completeio; ac->ac_bio = bio; @@ -931,13 +940,6 @@ amr_bio_command(struct amr_softc *sc, struct amr_command **acp) (long long)bio->bio_pblkno, blkcount, (u_long)sc->amr_drive[driveno].al_size); -out: - if (error != 0) { - if (ac != NULL) - amr_releasecmd(ac); - if (bio != NULL) /* this breaks ordering... */ - amr_enqueue_bio(sc, bio); - } *acp = ac; return(error); } @@ -961,7 +963,7 @@ amr_wait_command(struct amr_command *ac) count = 0; /* XXX better timeout? */ while ((ac->ac_flags & AMR_CMD_BUSY) && (count < 30)) { - tsleep(ac, PRIBIO | PCATCH, "amrwcmd", hz); + msleep(ac, &ac->ac_sc->amr_io_lock, PRIBIO | PCATCH, "amrwcmd", hz); } return(0); } @@ -1049,7 +1051,7 @@ amr_quartz_poll_command1(struct amr_softc *sc, struct amr_command *ac) if ((sc->amr_state & AMR_STATE_CRASHDUMP) == 0) { count=0; while (sc->amr_busyslots) { - tsleep(sc, PRIBIO | PCATCH, "amrpoll", hz); + msleep(sc, &sc->amr_io_lock, PRIBIO | PCATCH, "amrpoll", hz); if(count++>10) { break; } @@ -1104,36 +1106,18 @@ static int amr_getslot(struct amr_command *ac) { struct amr_softc *sc = ac->ac_sc; - int s, slot, limit, error; + int slot; debug_called(3); - /* if the command already has a slot, don't try to give it another one */ - if (ac->ac_slot != 0) - return(0); + slot = ac->ac_slot; + if (sc->amr_busycmd[slot] != NULL) + panic("amr: slot %d busy?\n", slot); - /* enforce slot usage limit */ - limit = (ac->ac_flags & AMR_CMD_PRIORITY) ? sc->amr_maxio : sc->amr_maxio - 4; - if (sc->amr_busyslots > limit) - return(EBUSY); - - /* - * Allocate a slot. XXX linear scan is slow - */ - error = EBUSY; - s = splbio(); - for (slot = 0; slot < sc->amr_maxio; slot++) { - if (sc->amr_busycmd[slot] == NULL) { - sc->amr_busycmd[slot] = ac; - sc->amr_busyslots++; - ac->ac_slot = slot; - error = 0; - break; - } - } - splx(s); + sc->amr_busycmd[slot] = ac; + sc->amr_busyslots++; - return(error); + return (0); } /******************************************************************************** @@ -1362,8 +1346,9 @@ amr_start(struct amr_command *ac) ac->ac_flags |= AMR_CMD_BUSY; /* get a command slot (freed in amr_done) */ - if (amr_getslot(ac)) + if (amr_getslot(ac)) { return(EBUSY); + } /* Now we have a slot, we can map the command (unmapped in amr_complete). */ if ((error = amr_mapcmd(ac)) == ENOMEM) { @@ -1449,6 +1434,7 @@ amr_start1(struct amr_softc *sc, struct amr_command *ac) * * Returns nonzero if any commands on the work queue were marked as completed. */ + int amr_done(struct amr_softc *sc) { @@ -1493,18 +1479,8 @@ amr_done(struct amr_softc *sc) } } - /* if we've completed any commands, try posting some more */ - sc->amr_state &= ~AMR_STATE_QUEUE_FRZN; - if (result) - amr_startio(sc); - /* handle completion and timeouts */ -#if __FreeBSD_version >= 500005 - if (sc->amr_state & AMR_STATE_INTEN) - taskqueue_enqueue(taskqueue_swi_giant, &sc->amr_task_complete); - else -#endif - amr_complete(sc, 0); + amr_complete(sc, 0); return(result); } @@ -1512,6 +1488,7 @@ amr_done(struct amr_softc *sc) /******************************************************************************** * Do completion processing on done commands on (sc) */ + static void amr_complete(void *context, int pending) { @@ -1549,7 +1526,9 @@ amr_complete(void *context, int pending) wakeup(sc); } } - amr_startio(sc); + + sc->amr_state &= ~AMR_STATE_QUEUE_FRZN; + amr_startio(sc); } /******************************************************************************** @@ -1577,11 +1556,12 @@ amr_alloccmd(struct amr_softc *sc) amr_alloccmd_cluster(sc); ac = amr_dequeue_free(sc); } - if (ac == NULL) + if (ac == NULL) { + sc->amr_state |= AMR_STATE_QUEUE_FRZN; return(NULL); + } /* clear out significant fields */ - ac->ac_slot = 0; ac->ac_status = 0; bzero(&ac->ac_mailbox, sizeof(struct amr_mailbox)); ac->ac_flags = 0; @@ -1611,21 +1591,27 @@ amr_alloccmd_cluster(struct amr_softc *sc) { struct amr_command_cluster *acc; struct amr_command *ac; - int s, i; + int s, i, nextslot; - acc = malloc(AMR_CMD_CLUSTERSIZE, M_DEVBUF, M_NOWAIT); + if (sc->amr_nextslot > sc->amr_maxio) + return; + acc = malloc(AMR_CMD_CLUSTERSIZE, M_DEVBUF, M_NOWAIT | M_ZERO); if (acc != NULL) { s = splbio(); + nextslot = sc->amr_nextslot; TAILQ_INSERT_TAIL(&sc->amr_cmd_clusters, acc, acc_link); splx(s); for (i = 0; i < AMR_CMD_CLUSTERCOUNT; i++) { ac = &acc->acc_command[i]; - bzero(ac, sizeof(*ac)); ac->ac_sc = sc; + ac->ac_slot = nextslot; if (!bus_dmamap_create(sc->amr_buffer_dmat, 0, &ac->ac_dmamap) && !bus_dmamap_create(sc->amr_buffer_dmat, 0, &ac->ac_ccb_dmamap)) amr_releasecmd(ac); + if (++nextslot > sc->amr_maxio) + break; } + sc->amr_nextslot = nextslot; } } @@ -1683,6 +1669,7 @@ amr_quartz_get_work(struct amr_softc *sc, struct amr_mailbox *mbsave) { int s, worked; u_int32_t outd; + u_int8_t nstatus; debug_called(3); @@ -1692,14 +1679,19 @@ amr_quartz_get_work(struct amr_softc *sc, struct amr_mailbox *mbsave) /* work waiting for us? */ if ((outd = AMR_QGET_ODB(sc)) == AMR_QODB_READY) { - /* save mailbox, which contains a list of completed commands */ - bcopy((void *)(uintptr_t)(volatile void *)sc->amr_mailbox, mbsave, sizeof(*mbsave)); - /* acknowledge interrupt */ AMR_QPUT_ODB(sc, AMR_QODB_READY); + while ((nstatus = sc->amr_mailbox->mb_nstatus) == 0xff) + ; + sc->amr_mailbox->mb_nstatus = 0xff; + + /* save mailbox, which contains a list of completed commands */ + bcopy((void *)(uintptr_t)(volatile void *)sc->amr_mailbox, mbsave, sizeof(*mbsave)); + mbsave->mb_nstatus = nstatus; + /* acknowledge that we have the commands */ - AMR_QPUT_IDB(sc, sc->amr_mailboxphys | AMR_QIDB_ACK); + AMR_QPUT_IDB(sc, AMR_QIDB_ACK); #ifndef AMR_QUARTZ_GOFASTER /* @@ -1843,6 +1835,7 @@ amr_describe_controller(struct amr_softc *sc) struct amr_enquiry *ae; char *prod; + mtx_lock(&sc->amr_io_lock); /* * Try to get 40LD product info, which tells us what the card is labelled as. */ @@ -1852,6 +1845,7 @@ amr_describe_controller(struct amr_softc *sc) ap->ap_memsize); free(ap, M_DEVBUF); + mtx_unlock(&sc->amr_io_lock); return; } @@ -1918,6 +1912,7 @@ amr_describe_controller(struct amr_softc *sc) ae->ae_adapter.aa_memorysize); } free(ae, M_DEVBUF); + mtx_unlock(&sc->amr_io_lock); } int diff --git a/sys/dev/amr/amr_cam.c b/sys/dev/amr/amr_cam.c index 3c7aaf8..a342699 100644 --- a/sys/dev/amr/amr_cam.c +++ b/sys/dev/amr/amr_cam.c @@ -262,8 +262,10 @@ amr_cam_action(struct cam_sim *sim, union ccb *ccb) /* save the channel number in the ccb */ csio->ccb_h.sim_priv.entries[0].field = cam_sim_bus(sim); + mtx_lock(&sc->amr_io_lock); amr_enqueue_ccb(sc, ccb); amr_startio(sc); + mtx_unlock(&sc->amr_io_lock); return; } break; @@ -491,7 +493,11 @@ out: static void amr_cam_poll(struct cam_sim *sim) { + struct amr_softc *sc = cam_sim_softc(sim); + + mtx_lock(&sc->amr_io_lock); amr_done(cam_sim_softc(sim)); + mtx_unlock(&sc->amr_io_lock); } /******************************************************************************** @@ -500,6 +506,7 @@ amr_cam_poll(struct cam_sim *sim) static void amr_cam_complete(struct amr_command *ac) { + struct amr_softc *sc = ac->ac_sc; struct amr_passthrough *ap = (struct amr_passthrough *)ac->ac_data; struct ccb_scsiio *csio = (struct ccb_scsiio *)ac->ac_private; struct scsi_inquiry_data *inq = (struct scsi_inquiry_data *)csio->data_ptr; @@ -552,7 +559,11 @@ amr_cam_complete(struct amr_command *ac) free(ap, M_DEVBUF); if ((csio->ccb_h.flags & CAM_DIR_MASK) != CAM_DIR_NONE) debug(2, "%*D\n", imin(csio->dxfer_len, 16), csio->data_ptr, " "); + mtx_unlock(&sc->amr_io_lock); + mtx_lock(&Giant); xpt_done((union ccb *)csio); + mtx_unlock(&Giant); + mtx_lock(&sc->amr_io_lock); amr_releasecmd(ac); } @@ -563,6 +574,7 @@ amr_cam_complete(struct amr_command *ac) static void amr_cam_complete_extcdb(struct amr_command *ac) { + struct amr_softc *sc = ac->ac_sc; struct amr_ext_passthrough *aep = (struct amr_ext_passthrough *)ac->ac_data; struct ccb_scsiio *csio = (struct ccb_scsiio *)ac->ac_private; struct scsi_inquiry_data *inq = (struct scsi_inquiry_data *)csio->data_ptr; @@ -615,6 +627,10 @@ amr_cam_complete_extcdb(struct amr_command *ac) free(aep, M_DEVBUF); if ((csio->ccb_h.flags & CAM_DIR_MASK) != CAM_DIR_NONE) debug(2, "%*D\n", imin(csio->dxfer_len, 16), csio->data_ptr, " "); + mtx_unlock(&sc->amr_io_lock); + mtx_lock(&Giant); xpt_done((union ccb *)csio); + mtx_unlock(&Giant); + mtx_lock(&sc->amr_io_lock); amr_releasecmd(ac); } diff --git a/sys/dev/amr/amr_disk.c b/sys/dev/amr/amr_disk.c index da7e94e..047ada0 100644 --- a/sys/dev/amr/amr_disk.c +++ b/sys/dev/amr/amr_disk.c @@ -256,7 +256,7 @@ amrd_attach(device_t dev) sc->amrd_disk->d_name = "amrd"; sc->amrd_disk->d_dump = (dumper_t *)amrd_dump; sc->amrd_disk->d_unit = sc->amrd_unit; - sc->amrd_disk->d_flags = DISKFLAG_NEEDSGIANT; + sc->amrd_disk->d_flags = 0; disk_create(sc->amrd_disk, DISK_VERSION); #ifdef FREEBSD_4 disks_registered++; diff --git a/sys/dev/amr/amr_pci.c b/sys/dev/amr/amr_pci.c index 8f3ef11..d408eef 100644 --- a/sys/dev/amr/amr_pci.c +++ b/sys/dev/amr/amr_pci.c @@ -173,6 +173,7 @@ amr_pci_attach(device_t dev) sc = device_get_softc(dev); bzero(sc, sizeof(*sc)); sc->amr_dev = dev; + mtx_init(&sc->amr_io_lock, "AMR IO Lock", NULL, MTX_DEF); /* assume failure is 'not configured' */ error = ENXIO; @@ -233,7 +234,7 @@ amr_pci_attach(device_t dev) device_printf(sc->amr_dev, "can't allocate interrupt\n"); goto out; } - if (bus_setup_intr(sc->amr_dev, sc->amr_irq, INTR_TYPE_BIO | INTR_ENTROPY, amr_pci_intr, sc, &sc->amr_intr)) { + if (bus_setup_intr(sc->amr_dev, sc->amr_irq, INTR_TYPE_BIO | INTR_ENTROPY | INTR_MPSAFE, amr_pci_intr, sc, &sc->amr_intr)) { device_printf(sc->amr_dev, "can't set up interrupt\n"); goto out; } @@ -271,7 +272,7 @@ amr_pci_attach(device_t dev) MAXBSIZE, AMR_NSEG, /* maxsize, nsegments */ MAXBSIZE, /* maxsegsize */ BUS_DMA_ALLOCNOW, /* flags */ - busdma_lock_mutex, &Giant, /* lockfunc, lockarg */ + busdma_lock_mutex, &sc->amr_io_lock, /* lockfunc, lockarg */ &sc->amr_buffer_dmat)) { device_printf(sc->amr_dev, "can't allocate buffer DMA tag\n"); goto out; @@ -423,7 +424,9 @@ amr_pci_intr(void *arg) debug_called(2); /* collect finished commands, queue anything waiting */ + mtx_lock(&sc->amr_io_lock); amr_done(sc); + mtx_unlock(&sc->amr_io_lock); } /******************************************************************************** diff --git a/sys/dev/amr/amrvar.h b/sys/dev/amr/amrvar.h index 6dc6838..a36b684 100644 --- a/sys/dev/amr/amrvar.h +++ b/sys/dev/amr/amrvar.h @@ -57,8 +57,9 @@ */ #if __FreeBSD_version >= 500005 -# include # include +# include +# include #endif #ifdef AMR_DEBUG @@ -174,6 +175,7 @@ struct amr_softc bus_dmamap_t amr_sg_dmamap; /* map for s/g buffers */ /* controller limits and features */ + int amr_nextslot; /* Next slot to use for newly allocated commands */ int amr_maxio; /* maximum number of I/O transactions */ int amr_maxdrives; /* max number of logical drives */ int amr_maxchan; /* count of SCSI channels */ @@ -222,9 +224,7 @@ struct amr_softc /* misc glue */ struct intr_config_hook amr_ich; /* wait-for-interrupts probe hook */ struct callout_handle amr_timeout; /* periodic status check */ -#if __FreeBSD_version >= 500005 - struct task amr_task_complete; /* deferred-completion task */ -#endif + struct mtx amr_io_lock; }; /* -- cgit v1.1