Mercurial > illumos > illumos-gate

changeset 14099:25c58d7ccca6

Find changesets by keywords (author, files, the commit message), revision number or hash, or revset expression.
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