Mercurial > illumos > illumos-gate
changeset 14099:25c58d7ccca6
3902 Race between ipmi_submit_driver_request() and kcs_loop()
Reviewed by: Alek Pinchuk <alek.pinchuk@nexenta.com>
Reviewed by: Garrett D'Amore <garrett@damore.org>
Reviewed by: Joshua M. Clulow <josh@sysmgr.org>
Approved by: Robert Mustacchi <rm@joyent.com>
author | Marcel Telka <marcel.telka@nexenta.com> |
---|---|
date | Wed, 24 Jul 2013 01:02:13 +0200 |
parents | 6df2f688829c |
children | 2c12d65cf215 |
files | usr/src/uts/intel/io/ipmi/ipmi.c usr/src/uts/intel/io/ipmi/ipmi_kcs.c usr/src/uts/intel/io/ipmi/ipmi_main.c usr/src/uts/intel/io/ipmi/ipmivars.h |
diffstat | 4 files changed, 90 insertions(+), 44 deletions(-) [+] |
line wrap: on
line diff
--- a/usr/src/uts/intel/io/ipmi/ipmi.c Wed Jul 31 03:43:37 2013 +0100 +++ b/usr/src/uts/intel/io/ipmi/ipmi.c Wed Jul 24 01:02:13 2013 +0200 @@ -28,7 +28,7 @@ /* * Copyright 2012, Joyent, Inc. All rights reserved. - * Copyright 2013, Nexenta Systems, Inc. All rights reserved. + * Copyright 2013 Nexenta Systems, Inc. All rights reserved. */ #include <sys/devops.h> @@ -49,9 +49,6 @@ #include <sys/ipmi.h> #include "ipmivars.h" -static kmutex_t slpmutex; -static kcondvar_t slplock; - /* * Request management. */ @@ -78,6 +75,10 @@ req->ir_reply = (uchar_t *)&req[1] + requestlen; req->ir_replybuflen = replylen; } + + cv_init(&req->ir_cv, NULL, CV_DEFAULT, NULL); + req->ir_status = IRS_ALLOCATED; + return (req); } @@ -85,6 +86,11 @@ void ipmi_free_request(struct ipmi_request *req) { + if (req == NULL) + return; + + cv_destroy(&req->ir_cv); + kmem_free(req, req->ir_sz); } @@ -101,11 +107,15 @@ * Anonymous requests (from inside the driver) always have a * waiter that we awaken. */ - if (req->ir_owner == NULL) { - mutex_enter(&slpmutex); - cv_signal(&slplock); - mutex_exit(&slpmutex); - } else { + if (req->ir_status == IRS_CANCELED) { + ASSERT(req->ir_owner == NULL); + ipmi_free_request(req); + return; + } + req->ir_status = IRS_COMPLETED; + cv_signal(&req->ir_cv); + + if (req->ir_owner != NULL) { dev = req->ir_owner; TAILQ_INSERT_TAIL(&dev->ipmi_completed_requests, req, ir_link); pollwakeup(dev->ipmi_pollhead, POLLIN | POLLRDNORM); @@ -116,28 +126,46 @@ * Enqueue an internal driver request and wait until it is completed. */ static int -ipmi_submit_driver_request(struct ipmi_softc *sc, struct ipmi_request *req, +ipmi_submit_driver_request(struct ipmi_softc *sc, struct ipmi_request **preq, int timo) { int error; + struct ipmi_request *req = *preq; + + ASSERT(req->ir_owner == NULL); IPMI_LOCK(sc); error = sc->ipmi_enqueue_request(sc, req); - if (error == 0) { - /* Wait for result - see ipmi_complete_request */ + + if (error != 0) { IPMI_UNLOCK(sc); - mutex_enter(&slpmutex); + return (error); + } + + while (req->ir_status != IRS_COMPLETED && error >= 0) if (timo == 0) - cv_wait(&slplock, &slpmutex); + cv_wait(&req->ir_cv, &sc->ipmi_lock); else - error = cv_timedwait(&slplock, &slpmutex, + error = cv_timedwait(&req->ir_cv, &sc->ipmi_lock, ddi_get_lbolt() + timo); - mutex_exit(&slpmutex); - IPMI_LOCK(sc); - if (error == -1) + + switch (req->ir_status) { + case IRS_QUEUED: + TAILQ_REMOVE(&sc->ipmi_pending_requests, req, ir_link); + req->ir_status = IRS_CANCELED; error = EWOULDBLOCK; - else + break; + case IRS_PROCESSED: + req->ir_status = IRS_CANCELED; + error = EWOULDBLOCK; + *preq = NULL; + break; + case IRS_COMPLETED: error = req->ir_error; + break; + default: + panic("IPMI: Invalid request status"); + break; } IPMI_UNLOCK(sc); @@ -164,6 +192,7 @@ req = TAILQ_FIRST(&sc->ipmi_pending_requests); TAILQ_REMOVE(&sc->ipmi_pending_requests, req, ir_link); + req->ir_status = IRS_PROCESSED; return (req); } @@ -174,6 +203,7 @@ IPMI_LOCK_ASSERT(sc); TAILQ_INSERT_TAIL(&sc->ipmi_pending_requests, req, ir_link); + req->ir_status = IRS_QUEUED; cv_signal(&sc->ipmi_request_added); return (0); } @@ -185,9 +215,6 @@ cv_destroy(&sc->ipmi_request_added); mutex_destroy(&sc->ipmi_lock); - - cv_destroy(&slplock); - mutex_destroy(&slpmutex); } boolean_t @@ -196,9 +223,6 @@ struct ipmi_request *req; int error, i; - mutex_init(&slpmutex, NULL, MUTEX_DEFAULT, NULL); - cv_init(&slplock, NULL, CV_DEFAULT, NULL); - /* Initialize interface-independent state. */ mutex_init(&sc->ipmi_lock, NULL, MUTEX_DEFAULT, NULL); cv_init(&sc->ipmi_request_added, NULL, CV_DEFAULT, NULL); @@ -215,7 +239,7 @@ req = ipmi_alloc_driver_request(IPMI_ADDR(IPMI_APP_REQUEST, 0), IPMI_GET_DEVICE_ID, 0, 15); - error = ipmi_submit_driver_request(sc, req, MAX_TIMEOUT); + error = ipmi_submit_driver_request(sc, &req, MAX_TIMEOUT); if (error == EWOULDBLOCK) { cmn_err(CE_WARN, "Timed out waiting for GET_DEVICE_ID"); ipmi_free_request(req); @@ -248,7 +272,7 @@ req = ipmi_alloc_driver_request(IPMI_ADDR(IPMI_APP_REQUEST, 0), IPMI_CLEAR_FLAGS, 1, 0); - if ((error = ipmi_submit_driver_request(sc, req, 0)) != 0) { + if ((error = ipmi_submit_driver_request(sc, &req, 0)) != 0) { cmn_err(CE_WARN, "Failed to clear IPMI flags: %d\n", error); ipmi_free_request(req); return (B_FALSE); @@ -268,7 +292,7 @@ IPMI_GET_CHANNEL_INFO, 1, 0); req->ir_request[0] = (uchar_t)i; - if (ipmi_submit_driver_request(sc, req, 0) != 0) { + if (ipmi_submit_driver_request(sc, &req, 0) != 0) { ipmi_free_request(req); break; } @@ -285,7 +309,7 @@ req = ipmi_alloc_driver_request(IPMI_ADDR(IPMI_APP_REQUEST, 0), IPMI_GET_WDOG, 0, 0); - if ((error = ipmi_submit_driver_request(sc, req, 0)) != 0) { + if ((error = ipmi_submit_driver_request(sc, &req, 0)) != 0) { cmn_err(CE_WARN, "Failed to check IPMI watchdog: %d\n", error); ipmi_free_request(req); return (B_FALSE);
--- a/usr/src/uts/intel/io/ipmi/ipmi_kcs.c Wed Jul 31 03:43:37 2013 +0100 +++ b/usr/src/uts/intel/io/ipmi/ipmi_kcs.c Wed Jul 24 01:02:13 2013 +0200 @@ -28,6 +28,7 @@ /* * Copyright 2012, Joyent, Inc. All rights reserved. + * Copyright 2013 Nexenta Systems, Inc. All rights reserved. */ #include <sys/param.h> @@ -452,6 +453,7 @@ IPMI_LOCK(sc); while ((req = ipmi_dequeue_request(sc)) != NULL) { + IPMI_UNLOCK(sc); ok = 0; for (i = 0; i < 3 && !ok; i++) ok = kcs_polled_request(sc, req); @@ -459,6 +461,7 @@ req->ir_error = 0; else req->ir_error = EIO; + IPMI_LOCK(sc); ipmi_complete_request(sc, req); } IPMI_UNLOCK(sc);
--- a/usr/src/uts/intel/io/ipmi/ipmi_main.c Wed Jul 31 03:43:37 2013 +0100 +++ b/usr/src/uts/intel/io/ipmi/ipmi_main.c Wed Jul 24 01:02:13 2013 +0200 @@ -21,7 +21,7 @@ /* * Copyright 2012, Joyent, Inc. All rights reserved. - * Copyright 2013, Nexenta Systems, Inc. All rights reserved. + * Copyright 2013 Nexenta Systems, Inc. All rights reserved. */ /* @@ -348,7 +348,6 @@ len = kreq->ir_replylen + 1; if (recv.msg.data_len < len && cmd == IPMICTL_RECEIVE_MSG) { IPMI_UNLOCK(sc); - ipmi_free_request(kreq); return (EMSGSIZE); } TAILQ_REMOVE(&dev->ipmi_completed_requests, kreq, ir_link); @@ -482,6 +481,25 @@ return (DDI_FAILURE); } +static void +ipmi_cleanup(dev_info_t *dip) +{ + /* poke the taskq so that it can terminate */ + IPMI_LOCK(sc); + sc->ipmi_detaching = 1; + cv_signal(&sc->ipmi_request_added); + IPMI_UNLOCK(sc); + + ipmi_shutdown(sc); + ddi_remove_minor_node(dip, NULL); + ipmi_dip = NULL; + + list_destroy(&dev_list); + id_space_destroy(minor_ids); + + sc->ipmi_detaching = 0; +} + static int ipmi_attach(dev_info_t *dip, ddi_attach_cmd_t cmd) { @@ -528,7 +546,7 @@ minor_ids = id_space_create("ipmi_id_space", 1, 128); if (ipmi_startup(sc) != B_TRUE) { - ipmi_shutdown(sc); + ipmi_cleanup(dip); return (DDI_FAILURE); } @@ -549,18 +567,8 @@ if (!list_is_empty(&dev_list)) return (DDI_FAILURE); - /* poke the taskq so that it can terminate */ - sc->ipmi_detaching = 1; - cv_signal(&sc->ipmi_request_added); + ipmi_cleanup(dip); - ipmi_shutdown(sc); - ddi_remove_minor_node(dip, NULL); - ipmi_dip = NULL; - - list_destroy(&dev_list); - id_space_destroy(minor_ids); - - sc->ipmi_detaching = 0; ipmi_attached = B_FALSE; return (DDI_SUCCESS); }
--- a/usr/src/uts/intel/io/ipmi/ipmivars.h Wed Jul 31 03:43:37 2013 +0100 +++ b/usr/src/uts/intel/io/ipmi/ipmivars.h Wed Jul 24 01:02:13 2013 +0200 @@ -28,7 +28,7 @@ /* * Copyright 2012, Joyent, Inc. All rights reserved. - * Copyright 2013, Nexenta Systems, Inc. All rights reserved. + * Copyright 2013 Nexenta Systems, Inc. All rights reserved. */ #ifndef _IPMIVARS_H_ @@ -44,6 +44,14 @@ struct ipmi_device; struct ipmi_request; +typedef enum { + IRS_ALLOCATED, + IRS_QUEUED, + IRS_PROCESSED, + IRS_COMPLETED, + IRS_CANCELED +} ir_status_t; + struct ipmi_request { TAILQ_ENTRY(ipmi_request) ir_link; struct ipmi_device *ir_owner; /* Driver uses NULL. */ @@ -58,6 +66,9 @@ uint8_t ir_command; uint8_t ir_compcode; int ir_sz; /* size of request */ + + kcondvar_t ir_cv; + ir_status_t ir_status; }; #define MAX_RES 3