[PATCH 0/4] isdn/gigaset: restructure modem response parser

[PATCH 0/4] isdn/gigaset: restructure modem response parser

[Tilman Schmidt]

This series of patches restructures the Gigaset ISDN driver's
modem response parser to improve code readability and conform
better to the device's specification and actual behaviour.
Could you please merge these through net-next?

Thanks,
Tilman

Tilman Schmidt (4):
  isdn/gigaset: restructure modem response parser (1)
  isdn/gigaset: restructure modem response parser (2)
  isdn/gigaset: restructure modem response parser (3)
  isdn/gigaset: restructure modem response parser (4)

 drivers/isdn/gigaset/ev-layer.c | 365 +++++++++++++++++++++-------------------
 1 file changed, 195 insertions(+), 170 deletions(-)

-- 
1.9.2.459.g68773ac

[PATCH 4/4] isdn/gigaset: restructure modem response parser (4)

[Tilman Schmidt]

Restructure the control structure of the modem response parser
to improve readability and error handling.

Signed-off-by: Tilman Schmidt <tilman@imap.cc>
---
 drivers/isdn/gigaset/ev-layer.c | 258 +++++++++++++++++++++++-----------------
 1 file changed, 151 insertions(+), 107 deletions(-)

diff --git a/drivers/isdn/gigaset/ev-layer.c b/drivers/isdn/gigaset/ev-layer.c
index dc6e6a3..1cfcea6 100644
--- a/drivers/isdn/gigaset/ev-layer.c
+++ b/drivers/isdn/gigaset/ev-layer.c
@@ -389,6 +389,20 @@ zsau_resp[] =
 	{NULL,				ZSAU_UNKNOWN}
 };
 
+/* check for and remove fixed string prefix
+ * If s starts with prefix terminated by a non-alphanumeric character,
+ * return pointer to the first character after that, otherwise return NULL.
+ */
+static char *skip_prefix(char *s, const char *prefix)
+{
+	while (*prefix)
+		if (*s++ != *prefix++)
+			return NULL;
+	if (isalnum(*s))
+		return NULL;
+	return s;
+}
+
 /* queue event with CID */
 static void add_cid_event(struct cardstate *cs, int cid, int type,
 			  void *ptr, int parameter)
@@ -430,15 +444,11 @@ static void add_cid_event(struct cardstate *cs, int cid, int type,
  */
 void gigaset_handle_modem_response(struct cardstate *cs)
 {
-	unsigned char *argv[MAX_REC_PARAMS + 1];
-	int params;
-	int i, j;
-	char *psep, *ptr;
+	char *eoc, *psep, *ptr;
 	const struct resp_type_t *rt;
 	const struct zsau_resp_t *zr;
-	int curarg;
-	int abort;
 	int cid, parameter;
+	u8 type, value;
 
 	if (!cs->cbytes) {
 		/* ignore additional LFs/CRs (M10x config mode or cx100) */
@@ -456,6 +466,19 @@ void gigaset_handle_modem_response(struct cardstate *cs)
 		return;
 	}
 
+	/* look up response type */
+	for (rt = resp_type; rt->response; ++rt) {
+		eoc = skip_prefix(cs->respdata, rt->response);
+		if (eoc)
+			break;
+	}
+	if (!rt->response) {
+		add_cid_event(cs, 0, RSP_NONE, NULL, 0);
+		gig_dbg(DEBUG_EVENT, "unknown modem response: '%s'\n",
+			cs->respdata);
+		return;
+	}
+
 	/* check for CID */
 	psep = strrchr(cs->respdata, ';');
 	if (psep &&
@@ -468,120 +491,141 @@ void gigaset_handle_modem_response(struct cardstate *cs)
 		cid = 0;
 	}
 
-	/* parse line */
-	argv[0] = cs->respdata;
-	params = 1;
-	for (i = 0; i < cs->cbytes; i++)
-		switch (cs->respdata[i]) {
-		case ';':
-		case ',':
-		case '=':
-			cs->respdata[i] = 0;
-			if (params > MAX_REC_PARAMS)
-				dev_warn(cs->dev,
-					 "too many parameters in response\n");
-			else
-				argv[params++] = cs->respdata + i + 1;
-		}
-
-	gig_dbg(DEBUG_EVENT, "CMD received: %s", argv[0]);
+	gig_dbg(DEBUG_EVENT, "CMD received: %s", cs->respdata);
 	if (cid)
 		gig_dbg(DEBUG_EVENT, "CID: %d", cid);
-	gig_dbg(DEBUG_EVENT, "available params: %d", params - 1);
-	for (j = 1; j < params; j++)
-		gig_dbg(DEBUG_EVENT, "param %d: %s", j, argv[j]);
-
-	abort = 1;
-	curarg = 0;
-	while (curarg < params) {
-		for (rt = resp_type; rt->response; ++rt)
-			if (!strcmp(argv[curarg], rt->response))
-				break;
 
-		if (!rt->response) {
-			add_cid_event(cs, 0, RSP_NONE, NULL, 0);
-			gig_dbg(DEBUG_EVENT, "unknown modem response: '%s'\n",
-				argv[curarg]);
-			break;
-		}
+	switch (rt->type) {
+	case RT_NOTHING:
+		/* check parameter separator */
+		if (*eoc)
+			goto bad_param;	/* extra parameter */
 
-		++curarg;
+		add_cid_event(cs, cid, rt->resp_code, NULL, 0);
+		break;
 
-		switch (rt->type) {
-		case RT_NOTHING:
-			add_cid_event(cs, cid, rt->resp_code, NULL, 0);
-			break;
-		case RT_RING:
-			if (!cid) {
-				dev_err(cs->dev,
-					"received RING without CID!\n");
-				add_cid_event(cs, 0, RSP_INVAL, NULL, 0);
-				abort = 1;
-			} else {
-				add_cid_event(cs, 0, rt->resp_code, NULL, cid);
-				abort = 0;
-			}
-			break;
-		case RT_ZSAU:
-			if (curarg >= params) {
-				add_cid_event(cs, cid, rt->resp_code, NULL,
-					      ZSAU_NONE);
-				break;
-			}
-			for (zr = zsau_resp; zr->str; ++zr)
-				if (!strcmp(argv[curarg], zr->str))
+	case RT_RING:
+		/* check parameter separator */
+		if (!*eoc)
+			eoc = NULL;	/* no parameter */
+		else if (*eoc++ != ',')
+			goto bad_param;
+
+		add_cid_event(cs, 0, rt->resp_code, NULL, cid);
+
+		/* process parameters as individual responses */
+		while (eoc) {
+			/* look up parameter type */
+			psep = NULL;
+			for (rt = resp_type; rt->response; ++rt) {
+				psep = skip_prefix(eoc, rt->response);
+				if (psep)
 					break;
-			if (!zr->str)
+			}
+
+			/* all legal parameters are of type RT_STRING */
+			if (!psep || rt->type != RT_STRING) {
 				dev_warn(cs->dev,
-					 "%s: unknown parameter %s after ZSAU\n",
-					 __func__, argv[curarg]);
-			add_cid_event(cs, cid, rt->resp_code, NULL, zr->code);
-			++curarg;
-			break;
-		case RT_STRING:
-			if (curarg < params) {
-				ptr = kstrdup(argv[curarg], GFP_ATOMIC);
-				if (!ptr)
-					dev_err(cs->dev, "out of memory\n");
-				++curarg;
-			} else {
-				ptr = NULL;
+					 "illegal RING parameter: '%s'\n",
+					 eoc);
+				return;
 			}
-			gig_dbg(DEBUG_EVENT, "string==%s", ptr ? ptr : "NULL");
+
+			/* skip parameter value separator */
+			if (*psep++ != '=')
+				goto bad_param;
+
+			/* look up end of parameter */
+			eoc = strchr(psep, ',');
+			if (eoc)
+				*eoc++ = 0;
+
+			/* retrieve parameter value */
+			ptr = kstrdup(psep, GFP_ATOMIC);
+
+			/* queue event */
 			add_cid_event(cs, cid, rt->resp_code, ptr, 0);
-			break;
-		case RT_ZCAU:
-			parameter = -1;
-			if (curarg + 1 < params) {
-				u8 type, value;
-
-				i = kstrtou8(argv[curarg++], 16, &type);
-				j = kstrtou8(argv[curarg++], 16, &value);
-				if (i == 0 && j == 0)
-					parameter = (type << 8) | value;
-			} else
-				curarg = params - 1;
-			add_cid_event(cs, cid, rt->resp_code, NULL, parameter);
-			break;
-		case RT_NUMBER:
-			if (curarg >= params ||
-			    kstrtoint(argv[curarg++], 10, &parameter))
-				parameter = -1;
-			gig_dbg(DEBUG_EVENT, "parameter==%d", parameter);
-			add_cid_event(cs, cid, rt->resp_code, NULL, parameter);
-			if (rt->resp_code == RSP_ZDLE)
-				cs->dle = parameter;
-			break;
 		}
+		break;
 
-		if (abort)
+	case RT_ZSAU:
+		/* check parameter separator */
+		if (!*eoc) {
+			/* no parameter */
+			add_cid_event(cs, cid, rt->resp_code, NULL, ZSAU_NONE);
 			break;
-	}
+		}
+		if (*eoc++ != '=')
+			goto bad_param;
 
-	if (curarg != params)
-		gig_dbg(DEBUG_EVENT,
-			"invalid number of processed parameters: %d/%d",
-			curarg, params);
+		/* look up parameter value */
+		for (zr = zsau_resp; zr->str; ++zr)
+			if (!strcmp(eoc, zr->str))
+				break;
+		if (!zr->str)
+			goto bad_param;
+
+		add_cid_event(cs, cid, rt->resp_code, NULL, zr->code);
+		break;
+
+	case RT_STRING:
+		/* check parameter separator */
+		if (*eoc++ != '=')
+			goto bad_param;
+
+		/* retrieve parameter value */
+		ptr = kstrdup(eoc, GFP_ATOMIC);
+
+		/* queue event */
+		add_cid_event(cs, cid, rt->resp_code, ptr, 0);
+		break;
+
+	case RT_ZCAU:
+		/* check parameter separators */
+		if (*eoc++ != '=')
+			goto bad_param;
+		psep = strchr(eoc, ',');
+		if (!psep)
+			goto bad_param;
+		*psep++ = 0;
+
+		/* decode parameter values */
+		if (kstrtou8(eoc, 16, &type) || kstrtou8(psep, 16, &value)) {
+			*--psep = ',';
+			goto bad_param;
+		}
+		parameter = (type << 8) | value;
+
+		add_cid_event(cs, cid, rt->resp_code, NULL, parameter);
+		break;
+
+	case RT_NUMBER:
+		/* check parameter separator */
+		if (*eoc++ != '=')
+			goto bad_param;
+
+		/* decode parameter value */
+		if (kstrtoint(eoc, 10, &parameter))
+			goto bad_param;
+
+		/* special case ZDLE: set flag before queueing event */
+		if (rt->resp_code == RSP_ZDLE)
+			cs->dle = parameter;
+
+		add_cid_event(cs, cid, rt->resp_code, NULL, parameter);
+		break;
+
+bad_param:
+		/* parameter unexpected, incomplete or malformed */
+		dev_warn(cs->dev, "bad parameter in response '%s'\n",
+			 cs->respdata);
+		add_cid_event(cs, cid, rt->resp_code, NULL, -1);
+		break;
+
+	default:
+		dev_err(cs->dev, "%s: internal error on '%s'\n",
+			__func__, cs->respdata);
+	}
 }
 EXPORT_SYMBOL_GPL(gigaset_handle_modem_response);
 
-- 
1.9.2.459.g68773ac

[PATCH 2/4] isdn/gigaset: restructure modem response parser (2)

[Tilman Schmidt]

Separate literal string handling from main parser loop.

Signed-off-by: Tilman Schmidt <tilman@imap.cc>
---
 drivers/isdn/gigaset/ev-layer.c | 126 ++++++++++++++++++----------------------
 1 file changed, 57 insertions(+), 69 deletions(-)

diff --git a/drivers/isdn/gigaset/ev-layer.c b/drivers/isdn/gigaset/ev-layer.c
index d1b84f4..b2a1fb1 100644
--- a/drivers/isdn/gigaset/ev-layer.c
+++ b/drivers/isdn/gigaset/ev-layer.c
@@ -455,91 +455,79 @@ void gigaset_handle_modem_response(struct cardstate *cs)
 	const struct resp_type_t *rt;
 	const struct zsau_resp_t *zr;
 	int curarg;
-	int resp_code;
-	int param_type;
 	int abort;
-	size_t len;
 	int cid, parameter;
-	int rawstring;
 
-	len = cs->cbytes;
-	if (!len) {
+	if (!cs->cbytes) {
 		/* ignore additional LFs/CRs (M10x config mode or cx100) */
 		gig_dbg(DEBUG_MCMD, "skipped EOL [%02X]", cs->respdata[0]);
 		return;
 	}
-	cs->respdata[len] = 0;
-	argv[0] = cs->respdata;
-	params = 1;
+	cs->respdata[cs->cbytes] = 0;
+
 	if (cs->at_state.getstring) {
-		/* getstring only allowed without cid at the moment */
+		/* state machine wants next line verbatim */
 		cs->at_state.getstring = 0;
-		rawstring = 1;
-		cid = 0;
-	} else {
-		/* parse line */
-		for (i = 0; i < len; i++)
-			switch (cs->respdata[i]) {
-			case ';':
-			case ',':
-			case '=':
-				if (params > MAX_REC_PARAMS) {
-					dev_warn(cs->dev,
-						 "too many parameters in response\n");
-					/* need last parameter (might be CID) */
-					params--;
-				}
-				argv[params++] = cs->respdata + i + 1;
-			}
+		ptr = kstrdup(cs->respdata, GFP_ATOMIC);
+		gig_dbg(DEBUG_EVENT, "string==%s", ptr ? ptr : "NULL");
+		add_cid_event(cs, 0, RSP_STRING, ptr, 0);
+		return;
+	}
 
-		rawstring = 0;
-		cid = params > 1 ? cid_of_response(argv[params - 1]) : 0;
-		if (cid < 0) {
-			gigaset_add_event(cs, &cs->at_state, RSP_INVAL,
-					  NULL, 0, NULL);
-			return;
+	/* parse line */
+	argv[0] = cs->respdata;
+	params = 1;
+	for (i = 0; i < cs->cbytes; i++)
+		switch (cs->respdata[i]) {
+		case ';':
+		case ',':
+		case '=':
+			if (params > MAX_REC_PARAMS) {
+				dev_warn(cs->dev,
+					 "too many parameters in response\n");
+				/* need last parameter (might be CID) */
+				params--;
+			}
+			argv[params++] = cs->respdata + i + 1;
 		}
 
-		for (j = 1; j < params; ++j)
-			argv[j][-1] = 0;
+	cid = params > 1 ? cid_of_response(argv[params - 1]) : 0;
+	if (cid < 0) {
+		gigaset_add_event(cs, &cs->at_state, RSP_INVAL, NULL, 0, NULL);
+		return;
+	}
+
+	for (j = 1; j < params; ++j)
+		argv[j][-1] = 0;
 
-		gig_dbg(DEBUG_EVENT, "CMD received: %s", argv[0]);
-		if (cid) {
-			--params;
-			gig_dbg(DEBUG_EVENT, "CID: %s", argv[params]);
-		}
-		gig_dbg(DEBUG_EVENT, "available params: %d", params - 1);
-		for (j = 1; j < params; j++)
-			gig_dbg(DEBUG_EVENT, "param %d: %s", j, argv[j]);
+	gig_dbg(DEBUG_EVENT, "CMD received: %s", argv[0]);
+	if (cid) {
+		--params;
+		gig_dbg(DEBUG_EVENT, "CID: %s", argv[params]);
 	}
+	gig_dbg(DEBUG_EVENT, "available params: %d", params - 1);
+	for (j = 1; j < params; j++)
+		gig_dbg(DEBUG_EVENT, "param %d: %s", j, argv[j]);
 
 	abort = 1;
 	curarg = 0;
 	while (curarg < params) {
-		if (rawstring) {
-			resp_code = RSP_STRING;
-			param_type = RT_STRING;
-		} else {
-			for (rt = resp_type; rt->response; ++rt)
-				if (!strcmp(argv[curarg], rt->response))
-					break;
-
-			if (!rt->response) {
-				add_cid_event(cs, 0, RSP_NONE, NULL, 0);
-				gig_dbg(DEBUG_EVENT,
-					"unknown modem response: '%s'\n",
-					argv[curarg]);
+		for (rt = resp_type; rt->response; ++rt)
+			if (!strcmp(argv[curarg], rt->response))
 				break;
-			}
 
-			resp_code = rt->resp_code;
-			param_type = rt->type;
-			++curarg;
+		if (!rt->response) {
+			add_cid_event(cs, 0, RSP_NONE, NULL, 0);
+			gig_dbg(DEBUG_EVENT, "unknown modem response: '%s'\n",
+				argv[curarg]);
+			break;
 		}
 
-		switch (param_type) {
+		++curarg;
+
+		switch (rt->type) {
 		case RT_NOTHING:
-			add_cid_event(cs, cid, resp_code, NULL, 0);
+			add_cid_event(cs, cid, rt->resp_code, NULL, 0);
 			break;
 		case RT_RING:
 			if (!cid) {
@@ -548,13 +536,13 @@ void gigaset_handle_modem_response(struct cardstate *cs)
 				add_cid_event(cs, 0, RSP_INVAL, NULL, 0);
 				abort = 1;
 			} else {
-				add_cid_event(cs, 0, resp_code, NULL, cid);
+				add_cid_event(cs, 0, rt->resp_code, NULL, cid);
 				abort = 0;
 			}
 			break;
 		case RT_ZSAU:
 			if (curarg >= params) {
-				add_cid_event(cs, cid, resp_code, NULL,
+				add_cid_event(cs, cid, rt->resp_code, NULL,
 					      ZSAU_NONE);
 				break;
 			}
@@ -565,7 +553,7 @@ void gigaset_handle_modem_response(struct cardstate *cs)
 				dev_warn(cs->dev,
 					 "%s: unknown parameter %s after ZSAU\n",
 					 __func__, argv[curarg]);
-			add_cid_event(cs, cid, resp_code, NULL, zr->code);
+			add_cid_event(cs, cid, rt->resp_code, NULL, zr->code);
 			++curarg;
 			break;
 		case RT_STRING:
@@ -578,7 +566,7 @@ void gigaset_handle_modem_response(struct cardstate *cs)
 				ptr = NULL;
 			}
 			gig_dbg(DEBUG_EVENT, "string==%s", ptr ? ptr : "NULL");
-			add_cid_event(cs, cid, resp_code, ptr, 0);
+			add_cid_event(cs, cid, rt->resp_code, ptr, 0);
 			break;
 		case RT_ZCAU:
 			parameter = -1;
@@ -591,15 +579,15 @@ void gigaset_handle_modem_response(struct cardstate *cs)
 					parameter = (type << 8) | value;
 			} else
 				curarg = params - 1;
-			add_cid_event(cs, cid, resp_code, NULL, parameter);
+			add_cid_event(cs, cid, rt->resp_code, NULL, parameter);
 			break;
 		case RT_NUMBER:
 			if (curarg >= params ||
 			    kstrtoint(argv[curarg++], 10, &parameter))
 				parameter = -1;
 			gig_dbg(DEBUG_EVENT, "parameter==%d", parameter);
-			add_cid_event(cs, cid, resp_code, NULL, parameter);
-			if (resp_code == RSP_ZDLE)
+			add_cid_event(cs, cid, rt->resp_code, NULL, parameter);
+			if (rt->resp_code == RSP_ZDLE)
 				cs->dle = parameter;
 			break;
 		}
-- 
1.9.2.459.g68773ac

[PATCH 1/4] isdn/gigaset: restructure modem response parser (1)

[Tilman Schmidt]

Factor out queueing of modem response events into helper function
add_cid_event().

Signed-off-by: Tilman Schmidt <tilman@imap.cc>
---
 drivers/isdn/gigaset/ev-layer.c | 99 +++++++++++++++++++++++------------------
 1 file changed, 55 insertions(+), 44 deletions(-)

diff --git a/drivers/isdn/gigaset/ev-layer.c b/drivers/isdn/gigaset/ev-layer.c
index c8ced12..d1b84f4 100644
--- a/drivers/isdn/gigaset/ev-layer.c
+++ b/drivers/isdn/gigaset/ev-layer.c
@@ -407,6 +407,37 @@ static int cid_of_response(char *s)
 	return cid;
 }
 
+/* queue event with CID */
+static void add_cid_event(struct cardstate *cs, int cid, int type,
+			  void *ptr, int parameter)
+{
+	unsigned long flags;
+	unsigned next, tail;
+	struct event_t *event;
+
+	gig_dbg(DEBUG_EVENT, "queueing event %d for cid %d", type, cid);
+
+	spin_lock_irqsave(&cs->ev_lock, flags);
+
+	tail = cs->ev_tail;
+	next = (tail + 1) % MAX_EVENTS;
+	if (unlikely(next == cs->ev_head)) {
+		dev_err(cs->dev, "event queue full\n");
+		kfree(ptr);
+	} else {
+		event = cs->events + tail;
+		event->type = type;
+		event->cid = cid;
+		event->ptr = ptr;
+		event->arg = NULL;
+		event->parameter = parameter;
+		event->at_state = NULL;
+		cs->ev_tail = next;
+	}
+
+	spin_unlock_irqrestore(&cs->ev_lock, flags);
+}
+
 /**
  * gigaset_handle_modem_response() - process received modem response
  * @cs:		device descriptor structure.
@@ -420,17 +451,15 @@ void gigaset_handle_modem_response(struct cardstate *cs)
 	unsigned char *argv[MAX_REC_PARAMS + 1];
 	int params;
 	int i, j;
+	char *ptr;
 	const struct resp_type_t *rt;
 	const struct zsau_resp_t *zr;
 	int curarg;
-	unsigned long flags;
-	unsigned next, tail, head;
-	struct event_t *event;
 	int resp_code;
 	int param_type;
 	int abort;
 	size_t len;
-	int cid;
+	int cid, parameter;
 	int rawstring;
 
 	len = cs->cbytes;
@@ -484,26 +513,9 @@ void gigaset_handle_modem_response(struct cardstate *cs)
 			gig_dbg(DEBUG_EVENT, "param %d: %s", j, argv[j]);
 	}
 
-	spin_lock_irqsave(&cs->ev_lock, flags);
-	head = cs->ev_head;
-	tail = cs->ev_tail;
-
 	abort = 1;
 	curarg = 0;
 	while (curarg < params) {
-		next = (tail + 1) % MAX_EVENTS;
-		if (unlikely(next == head)) {
-			dev_err(cs->dev, "event queue full\n");
-			break;
-		}
-
-		event = cs->events + tail;
-		event->at_state = NULL;
-		event->cid = cid;
-		event->ptr = NULL;
-		event->arg = NULL;
-		tail = next;
-
 		if (rawstring) {
 			resp_code = RSP_STRING;
 			param_type = RT_STRING;
@@ -513,7 +525,7 @@ void gigaset_handle_modem_response(struct cardstate *cs)
 					break;
 
 			if (!rt->response) {
-				event->type = RSP_NONE;
+				add_cid_event(cs, 0, RSP_NONE, NULL, 0);
 				gig_dbg(DEBUG_EVENT,
 					"unknown modem response: '%s'\n",
 					argv[curarg]);
@@ -525,78 +537,77 @@ void gigaset_handle_modem_response(struct cardstate *cs)
 			++curarg;
 		}
 
-		event->type = resp_code;
-
 		switch (param_type) {
 		case RT_NOTHING:
+			add_cid_event(cs, cid, resp_code, NULL, 0);
 			break;
 		case RT_RING:
 			if (!cid) {
 				dev_err(cs->dev,
 					"received RING without CID!\n");
-				event->type = RSP_INVAL;
+				add_cid_event(cs, 0, RSP_INVAL, NULL, 0);
 				abort = 1;
 			} else {
-				event->cid = 0;
-				event->parameter = cid;
+				add_cid_event(cs, 0, resp_code, NULL, cid);
 				abort = 0;
 			}
 			break;
 		case RT_ZSAU:
 			if (curarg >= params) {
-				event->parameter = ZSAU_NONE;
+				add_cid_event(cs, cid, resp_code, NULL,
+					      ZSAU_NONE);
 				break;
 			}
 			for (zr = zsau_resp; zr->str; ++zr)
 				if (!strcmp(argv[curarg], zr->str))
 					break;
-			event->parameter = zr->code;
 			if (!zr->str)
 				dev_warn(cs->dev,
 					 "%s: unknown parameter %s after ZSAU\n",
 					 __func__, argv[curarg]);
+			add_cid_event(cs, cid, resp_code, NULL, zr->code);
 			++curarg;
 			break;
 		case RT_STRING:
 			if (curarg < params) {
-				event->ptr = kstrdup(argv[curarg], GFP_ATOMIC);
-				if (!event->ptr)
+				ptr = kstrdup(argv[curarg], GFP_ATOMIC);
+				if (!ptr)
 					dev_err(cs->dev, "out of memory\n");
 				++curarg;
+			} else {
+				ptr = NULL;
 			}
-			gig_dbg(DEBUG_EVENT, "string==%s",
-				event->ptr ? (char *) event->ptr : "NULL");
+			gig_dbg(DEBUG_EVENT, "string==%s", ptr ? ptr : "NULL");
+			add_cid_event(cs, cid, resp_code, ptr, 0);
 			break;
 		case RT_ZCAU:
-			event->parameter = -1;
+			parameter = -1;
 			if (curarg + 1 < params) {
 				u8 type, value;
 
 				i = kstrtou8(argv[curarg++], 16, &type);
 				j = kstrtou8(argv[curarg++], 16, &value);
 				if (i == 0 && j == 0)
-					event->parameter = (type << 8) | value;
+					parameter = (type << 8) | value;
 			} else
 				curarg = params - 1;
+			add_cid_event(cs, cid, resp_code, NULL, parameter);
 			break;
 		case RT_NUMBER:
 			if (curarg >= params ||
-			    kstrtoint(argv[curarg++], 10, &event->parameter))
-				event->parameter = -1;
-			gig_dbg(DEBUG_EVENT, "parameter==%d", event->parameter);
+			    kstrtoint(argv[curarg++], 10, &parameter))
+				parameter = -1;
+			gig_dbg(DEBUG_EVENT, "parameter==%d", parameter);
+			add_cid_event(cs, cid, resp_code, NULL, parameter);
+			if (resp_code == RSP_ZDLE)
+				cs->dle = parameter;
 			break;
 		}
 
-		if (resp_code == RSP_ZDLE)
-			cs->dle = event->parameter;
-
 		if (abort)
 			break;
 	}
 
-	cs->ev_tail = tail;
-	spin_unlock_irqrestore(&cs->ev_lock, flags);
-
 	if (curarg != params)
 		gig_dbg(DEBUG_EVENT,
 			"invalid number of processed parameters: %d/%d",
-- 
1.9.2.459.g68773ac

[PATCH 3/4] isdn/gigaset: restructure modem response parser (3)

[Tilman Schmidt]

Separate CID detection from main parser loop.

Signed-off-by: Tilman Schmidt <tilman@imap.cc>
---
 drivers/isdn/gigaset/ev-layer.c | 56 ++++++++++++++---------------------------
 1 file changed, 19 insertions(+), 37 deletions(-)

diff --git a/drivers/isdn/gigaset/ev-layer.c b/drivers/isdn/gigaset/ev-layer.c
index b2a1fb1..dc6e6a3 100644
--- a/drivers/isdn/gigaset/ev-layer.c
+++ b/drivers/isdn/gigaset/ev-layer.c
@@ -389,24 +389,6 @@ zsau_resp[] =
 	{NULL,				ZSAU_UNKNOWN}
 };
 
-/* retrieve CID from parsed response
- * returns 0 if no CID, -1 if invalid CID, or CID value 1..65535
- */
-static int cid_of_response(char *s)
-{
-	int cid;
-	int rc;
-
-	if (s[-1] != ';')
-		return 0;	/* no CID separator */
-	rc = kstrtoint(s, 10, &cid);
-	if (rc)
-		return 0;	/* CID not numeric */
-	if (cid < 1 || cid > 65535)
-		return -1;	/* CID out of range */
-	return cid;
-}
-
 /* queue event with CID */
 static void add_cid_event(struct cardstate *cs, int cid, int type,
 			  void *ptr, int parameter)
@@ -451,7 +433,7 @@ void gigaset_handle_modem_response(struct cardstate *cs)
 	unsigned char *argv[MAX_REC_PARAMS + 1];
 	int params;
 	int i, j;
-	char *ptr;
+	char *psep, *ptr;
 	const struct resp_type_t *rt;
 	const struct zsau_resp_t *zr;
 	int curarg;
@@ -474,6 +456,18 @@ void gigaset_handle_modem_response(struct cardstate *cs)
 		return;
 	}
 
+	/* check for CID */
+	psep = strrchr(cs->respdata, ';');
+	if (psep &&
+	    !kstrtoint(psep + 1, 10, &cid) &&
+	    cid >= 1 && cid <= 65535) {
+		/* valid CID: chop it off */
+		*psep = 0;
+	} else {
+		/* no valid CID: leave unchanged */
+		cid = 0;
+	}
+
 	/* parse line */
 	argv[0] = cs->respdata;
 	params = 1;
@@ -482,29 +476,17 @@ void gigaset_handle_modem_response(struct cardstate *cs)
 		case ';':
 		case ',':
 		case '=':
-			if (params > MAX_REC_PARAMS) {
+			cs->respdata[i] = 0;
+			if (params > MAX_REC_PARAMS)
 				dev_warn(cs->dev,
 					 "too many parameters in response\n");
-				/* need last parameter (might be CID) */
-				params--;
-			}
-			argv[params++] = cs->respdata + i + 1;
+			else
+				argv[params++] = cs->respdata + i + 1;
 		}
 
-	cid = params > 1 ? cid_of_response(argv[params - 1]) : 0;
-	if (cid < 0) {
-		gigaset_add_event(cs, &cs->at_state, RSP_INVAL, NULL, 0, NULL);
-		return;
-	}
-
-	for (j = 1; j < params; ++j)
-		argv[j][-1] = 0;
-
 	gig_dbg(DEBUG_EVENT, "CMD received: %s", argv[0]);
-	if (cid) {
-		--params;
-		gig_dbg(DEBUG_EVENT, "CID: %s", argv[params]);
-	}
+	if (cid)
+		gig_dbg(DEBUG_EVENT, "CID: %d", cid);
 	gig_dbg(DEBUG_EVENT, "available params: %d", params - 1);
 	for (j = 1; j < params; j++)
 		gig_dbg(DEBUG_EVENT, "param %d: %s", j, argv[j]);
-- 
1.9.2.459.g68773ac

Re: [PATCH 0/4] isdn/gigaset: restructure modem response parser

[David Miller]

From: Tilman Schmidt <tilman@imap.cc>
Date: Sat, 21 Mar 2015 20:15:32 +0100 (CET)

> This series of patches restructures the Gigaset ISDN driver's
> modem response parser to improve code readability and conform
> better to the device's specification and actual behaviour.
> Could you please merge these through net-next?

Series applied to net-next, thanks.