Change in osmo-ccid-firmware[master]: 7816 fsm: move to static msgb

This is merely a historical archive of years 2008-2021, before the migration to mailman3.

A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.

Hoernchen gerrit-no-reply at lists.osmocom.org
Sat Nov 7 18:42:25 UTC 2020


Hoernchen has uploaded this change for review. ( https://gerrit.osmocom.org/c/osmo-ccid-firmware/+/21071 )


Change subject: 7816 fsm: move to static msgb
......................................................................

7816 fsm: move to static msgb

Error handling was difficult due to the need to pass msgb pointers
attached to the sub fsms back to the cb that is polled from the main
loop to be able free them, while ensuring they never get lost, because
memory leakage is deadly.

This is now fixed by using static pseudo-msgbs for the fsms that are
never deallocated. This only adds one tpdu tx copy, the ccid response
msgbs were already being copied anyway, so memory usage has not changed
except for the "unused slots" case that is not really important since
the octsim was designed around concurrent slot operation anway.

All of this allows convenient error handling in the allstate function
instead of having to spread it all over the sub fsms - in practice
handling errors mostly consists of card deactivation + returning a
proper failure message that matches the ccid command anyway.

Change-Id: I65e77c376aca9ed50e234a0b58a7450a8bbd4fe0
---
M ccid_common/ccid_slot_fsm.c
M ccid_common/iso7816_fsm.c
M ccid_common/iso7816_fsm.h
3 files changed, 216 insertions(+), 151 deletions(-)



  git pull ssh://gerrit.osmocom.org:29418/osmo-ccid-firmware refs/changes/71/21071/1

diff --git a/ccid_common/ccid_slot_fsm.c b/ccid_common/ccid_slot_fsm.c
index 043fa8f..5fc74e0 100644
--- a/ccid_common/ccid_slot_fsm.c
+++ b/ccid_common/ccid_slot_fsm.c
@@ -159,6 +159,7 @@
 	case ISO7816_E_TPDU_FAILED_IND:
 	case ISO7816_E_PPS_DONE_IND:
 	case ISO7816_E_PPS_FAILED_IND:
+	case ISO7816_E_PPS_UNSUPPORTED_IND:
 	case ISO7816_E_WTIME_EXP:
 		cs->event_data = data;
 #ifdef OCTSIMFWBUILD
@@ -173,6 +174,7 @@
 	}
 }
 
+/* do not free msgbs passed from the fsms, they are statically allocated! */
 static int iso_handle_fsm_events(struct ccid_slot *cs, bool enable){
 	struct iso_fsm_slot *ss = ccid_slot2iso_fsm_slot(cs);
 	struct msgb *tpdu, *resp;
@@ -207,7 +209,6 @@
 		resp = ccid_gen_data_block(cs, ss->seq, CCID_CMD_STATUS_OK, 0,
 					   msgb_data(tpdu), msgb_length(tpdu));
 		ccid_slot_send_unbusy(cs, resp);
-		/* Don't free "TPDU" here, as the ATR should survive */
 		cs->event = 0;
 		break;
 	case ISO7816_E_ATR_ERR_IND:
@@ -217,7 +218,6 @@
 		resp = ccid_gen_data_block(cs, ss->seq, CCID_CMD_STATUS_FAILED, CCID_ERR_ICC_MUTE,
 					   msgb_data(tpdu), msgb_length(tpdu));
 		ccid_slot_send_unbusy(cs, resp);
-		/* Don't free "TPDU" here, as the ATR should survive */
 		cs->event = 0;
 		break;
 		break;
@@ -227,7 +227,6 @@
 			msgb_hexdump(tpdu));
 		resp = ccid_gen_data_block(cs, ss->seq, CCID_CMD_STATUS_OK, 0, msgb_l4(tpdu), msgb_l4len(tpdu));
 		ccid_slot_send_unbusy(cs, resp);
-		msgb_free(tpdu);
 		cs->event = 0;
 		break;
 	case ISO7816_E_TPDU_FAILED_IND:
@@ -237,7 +236,6 @@
 		/* FIXME: other error causes than card removal?*/
 		resp = ccid_gen_data_block(cs, ss->seq, CCID_CMD_STATUS_FAILED, CCID_ERR_ICC_MUTE, msgb_l2(tpdu), 0);
 		ccid_slot_send_unbusy(cs, resp);
-		msgb_free(tpdu);
 		cs->event = 0;
 		break;
 	case ISO7816_E_PPS_DONE_IND:
@@ -264,17 +262,34 @@
 
 		ccid_slot_send_unbusy(cs, resp);
 
-		/* this frees the pps req from the host, pps resp buffer stays with the pps fsm */
-		msgb_free(tpdu);
+		cs->event = 0;
+		break;
+	case ISO7816_E_PPS_UNSUPPORTED_IND:
+		tpdu = data;
+
+		/* perform deactivation */
+		card_uart_ctrl(ss->cuart, CUART_CTL_RST, true);
+		card_uart_ctrl(ss->cuart, CUART_CTL_POWER_5V0, false);
+		cs->icc_powered = false;
+
+		/* failed comand */
+		resp = ccid_gen_parameters_t0(cs, ss->seq, CCID_CMD_STATUS_FAILED, 0);
+		ccid_slot_send_unbusy(cs, resp);
+
 		cs->event = 0;
 		break;
 	case ISO7816_E_PPS_FAILED_IND:
 		tpdu = data;
+
+		/* perform deactivation */
+		card_uart_ctrl(ss->cuart, CUART_CTL_RST, true);
+		card_uart_ctrl(ss->cuart, CUART_CTL_POWER_5V0, false);
+		cs->icc_powered = false;
+
 		/* failed fi/di */
 		resp = ccid_gen_parameters_t0(cs, ss->seq, CCID_CMD_STATUS_FAILED, 10);
 		ccid_slot_send_unbusy(cs, resp);
-		/* this frees the pps req from the host, pps resp buffer stays with the pps fsm */
-		msgb_free(tpdu);
+
 		cs->event = 0;
 		break;
 	case 0:
@@ -310,6 +325,7 @@
 
 	LOGPCS(cs, LOGL_DEBUG, "scheduling TPDU transfer: %s\n", msgb_hexdump(msg));
 	osmo_fsm_inst_dispatch(ss->fi, ISO7816_E_XCEIVE_TPDU_CMD, msg);
+	msgb_free(msg);
 	/* continues in iso_fsm_clot_user_cb once response/error/timeout is received */
 	return 1;
 }
@@ -348,7 +364,7 @@
 				const struct ccid_pars_decoded *pars_dec)
 {
 	struct iso_fsm_slot *ss = ccid_slot2iso_fsm_slot(cs);
-	struct msgb *tpdu;
+	uint8_t PPS1 = (pars_dec->fi << 4 | pars_dec->di);
 
 	/* see 6.1.7 for error offsets */
 	if(proto != CCID_PROTOCOL_NUM_T0)
@@ -370,17 +386,10 @@
 	    -> we can't really do 4 stop bits?!
 	*/
 
-	/* Hardware does not support SPU, so no PPS2, and PPS3 is reserved anyway */
-	tpdu = msgb_alloc(6, "PPSRQ");
-	OSMO_ASSERT(tpdu);
-	msgb_put_u8(tpdu, 0xff);
-	msgb_put_u8(tpdu, (1 << 4)); /* only PPS1, T=0 */
-	msgb_put_u8(tpdu, (pars_dec->fi << 4 | pars_dec->di));
-	msgb_put_u8(tpdu, 0xff ^ (1 << 4) ^ (pars_dec->fi << 4 | pars_dec->di));
+	LOGPCS(cs, LOGL_DEBUG, "scheduling PPS transfer, PPS1: %2x\n", PPS1);
 
-
-	LOGPCS(cs, LOGL_DEBUG, "scheduling PPS transfer: %s\n", msgb_hexdump(tpdu));
-	osmo_fsm_inst_dispatch(ss->fi, ISO7816_E_XCEIVE_PPS_CMD, tpdu);
+	/* pass PPS1 instead of msgb */
+	osmo_fsm_inst_dispatch(ss->fi, ISO7816_E_XCEIVE_PPS_CMD, PPS1);
 	/* continues in iso_fsm_clot_user_cb once response/error/timeout is received */
 	return 0;
 }
diff --git a/ccid_common/iso7816_fsm.c b/ccid_common/iso7816_fsm.c
index bf19539..66250d5 100644
--- a/ccid_common/iso7816_fsm.c
+++ b/ccid_common/iso7816_fsm.c
@@ -33,6 +33,47 @@
 #include "cuart.h"
 #include "iso7816_fsm.h"
 
+/* unionize to ensure at least properly aligned msgb struct */
+#define DECLARE_STATIC_MSGB(name, size) \
+	struct msgb* name; \
+	union { \
+	struct msgb name ## dummy; \
+	unsigned char name ## _msgbuf[sizeof(struct msgb) + size]; \
+	};
+
+#define INIT_STATIC_MSGB(name) { \
+	name = (struct msgb*)name ## _msgbuf; \
+	memset(name, 0x00, sizeof(name ## _msgbuf)); \
+	name->data_len = sizeof(name ## _msgbuf) - sizeof(struct msgb); \
+	name->len = 0; \
+	name->data = name->_data; \
+	name->head = name->_data; \
+	name->tail = name->_data; \
+}
+
+#define COPY_TO_STATIC_MSGB(src, dst) { \
+	struct msgb *new_msg = dst; \
+	struct msgb *msg = src; \
+	 \
+	/* copy data */ \
+	memcpy(new_msg->_data, msg->_data, new_msg->data_len); \
+	 \
+	/* copy header */ \
+	new_msg->len = msg->len; \
+	new_msg->data += msg->data - msg->_data; \
+	new_msg->head += msg->head - msg->_data; \
+	new_msg->tail += msg->tail - msg->_data; \
+	 \
+	if (msg->l1h) \
+		new_msg->l1h = new_msg->_data + (msg->l1h - msg->_data); \
+	if (msg->l2h) \
+		new_msg->l2h = new_msg->_data + (msg->l2h - msg->_data); \
+	if (msg->l3h) \
+		new_msg->l3h = new_msg->_data + (msg->l3h - msg->_data); \
+	if (msg->l4h) \
+		new_msg->l4h = new_msg->_data + (msg->l4h - msg->_data); \
+	}
+
 /* Section 8.2: the Answer-to-Reset (... a string of at most 32 bytes) */
 #define MAX_ATR_SIZE 32
 
@@ -132,12 +173,25 @@
 	uint8_t hist_len;	/*!< store the number of expected historical bytes */
 	uint8_t y;		/*!< last mask of the upcoming TA, TB, TC, TD interface bytes */
 	uint8_t i;		/*!< interface byte subgroup number */
-	struct msgb *atr;	/*!< ATR data */
+	DECLARE_STATIC_MSGB(atr, 33) /*!< ATR data */
 	uint8_t computed_checksum;
 	uint16_t protocol_support;
 };
 
+struct pps_fsm_priv {
+	DECLARE_STATIC_MSGB(tx_cmd, 6);
+	DECLARE_STATIC_MSGB(rx_cmd, 6);
+	uint8_t pps0_recv; /*!< contains flags so we know how many pps bytes follow */
+};
+
+struct tpdu_fsm_priv {
+	DECLARE_STATIC_MSGB(tpdu, 300);
+	bool is_command; /* is this a command TPDU (true) or a response (false) */
+};
+
 static struct atr_fsm_priv *get_atr_fsm_priv(struct osmo_fsm_inst *fi);
+static struct pps_fsm_priv *get_pps_fsm_priv(struct osmo_fsm_inst *fi);
+static struct tpdu_fsm_priv *get_tpdu_fsm_priv(struct osmo_fsm_inst *fi);
 
 /***********************************************************************
  * ISO7816-3 Main FSM
@@ -257,6 +311,7 @@
 		break;
 	case ISO7816_E_POWER_UP_IND:
 		break;
+	case ISO7816_E_PPS_UNSUPPORTED_IND:
 	case ISO7816_E_PPS_FAILED_IND:
 		msg = data;
 		/* notify user about PPS result */
@@ -285,10 +340,6 @@
 		osmo_fsm_inst_state_chg(fi, ISO7816_S_IN_ATR, 0, 0);
 		osmo_fsm_inst_dispatch(ip->atr_fi, event, data);
 		break;
-	case ISO7816_E_WTIME_EXP:
-		ip->user_cb(fi, event, 0, NULL);
-		osmo_fsm_inst_state_chg(fi, ISO7816_S_RESET, 0, 0);
-		break;
 	default:
 		OSMO_ASSERT(0);
 	}
@@ -297,26 +348,18 @@
 static void iso7816_3_in_atr_action(struct osmo_fsm_inst *fi, uint32_t event, void *data)
 {
 	struct iso7816_3_priv *ip = get_iso7816_3_priv(fi);
-	struct msgb *atr;
+	struct msgb *atr = data;
 	OSMO_ASSERT(fi->fsm == &iso7816_3_fsm);
 
 	switch (event) {
 	case ISO7816_E_RX_SINGLE:
 	case ISO7816_E_RX_ERR_IND:
-	case ISO7816_E_WTIME_EXP:
 		/* simply pass this through to the child FSM for the ATR */
 		osmo_fsm_inst_dispatch(ip->atr_fi, event, data);
 		break;
 	case ISO7816_E_ATR_DONE_IND:
-		atr = data;
 		/* FIXME: verify ATR result: success / failure */
 		osmo_fsm_inst_state_chg(fi, ISO7816_S_WAIT_TPDU, 0, 0);
-		/* notify user about ATR */
-		ip->user_cb(fi, event, 0, atr);
-		break;
-	case ISO7816_E_ATR_ERR_IND:
-		atr = data;
-		osmo_fsm_inst_state_chg(fi, ISO7816_S_RESET, 0, 0);
 		ip->user_cb(fi, event, 0, atr);
 		break;
 	default:
@@ -368,7 +411,7 @@
 	case ISO7816_E_RX_ERR_IND:
 	case ISO7816_E_TX_COMPL:
 	case ISO7816_E_TX_ERR_IND:
-		/* simply pass this through to the child FSM for the ATR */
+		/* simply pass this through to the child FSM for the TPDU */
 		osmo_fsm_inst_dispatch(ip->tpdu_fi, event, data);
 		break;
 	case ISO7816_E_TPDU_DONE_IND:
@@ -377,10 +420,6 @@
 		/* hand finished TPDU to user */
 		ip->user_cb(fi, event, 0, apdu);
 		break;
-	case ISO7816_E_WTIME_EXP:
-		/* FIXME: power off? */
-		osmo_fsm_inst_state_chg(fi, ISO7816_S_RESET, 0, 0);
-		break;
 	default:
 		OSMO_ASSERT(0);
 	}
@@ -391,6 +430,8 @@
 	OSMO_ASSERT(fi->fsm == &iso7816_3_fsm);
 	struct iso7816_3_priv *ip = get_iso7816_3_priv(fi);
 	struct atr_fsm_priv *atp = get_atr_fsm_priv(ip->atr_fi);
+	struct pps_fsm_priv *ppp = get_pps_fsm_priv(ip->pps_fi);
+	struct tpdu_fsm_priv *tpdup = get_tpdu_fsm_priv(ip->tpdu_fi);
 
 	switch (event) {
 	case ISO7816_E_HW_ERR_IND:
@@ -398,6 +439,13 @@
 		/* FIXME: power off? */
 		if(fi->state == ISO7816_S_WAIT_ATR || fi->state == ISO7816_S_IN_ATR)
 			ip->user_cb(fi, ISO7816_E_ATR_ERR_IND, 0, atp->atr);
+
+		if(fi->state == ISO7816_S_WAIT_PPS_RSP || fi->state == ISO7816_S_IN_PPS_RSP)
+			ip->user_cb(fi, ISO7816_E_PPS_FAILED_IND, 0, ppp->tx_cmd);
+
+		if(fi->state == ISO7816_S_WAIT_TPDU || fi->state == ISO7816_S_IN_TPDU)
+			ip->user_cb(fi, ISO7816_E_TPDU_FAILED_IND, 0, tpdup->tpdu);
+
 		osmo_fsm_inst_state_chg(fi, ISO7816_S_RESET, 0, 0);
 		break;
 	case ISO7816_E_POWER_DN_IND:
@@ -407,6 +455,24 @@
 	case ISO7816_E_ABORT_REQ:
 		/* FIXME */
 		break;
+	case ISO7816_E_WTIME_EXP:
+		if(fi->state == ISO7816_S_WAIT_ATR || fi->state == ISO7816_S_IN_ATR) {
+			/* atr timeout instead of tck might be fine */
+			osmo_fsm_inst_dispatch(ip->atr_fi, event, data);
+			break;
+		}
+		if(fi->state == ISO7816_S_WAIT_PPS_RSP || fi->state == ISO7816_S_IN_PPS_RSP)
+			ip->user_cb(fi, ISO7816_E_PPS_FAILED_IND, 0, ppp->tx_cmd);
+
+		if(fi->state == ISO7816_S_WAIT_TPDU || fi->state == ISO7816_S_IN_TPDU)
+			ip->user_cb(fi, ISO7816_E_TPDU_FAILED_IND, 0, tpdup->tpdu);
+
+		osmo_fsm_inst_state_chg(fi, ISO7816_S_RESET, 0, 0);
+		break;
+	case ISO7816_E_ATR_ERR_IND:
+		osmo_fsm_inst_state_chg(fi, ISO7816_S_RESET, 0, 0);
+		ip->user_cb(fi, event, 0, atp->atr);
+		break;
 	default:
 		OSMO_ASSERT(0);
 		break;
@@ -434,25 +500,28 @@
 static void iso7816_3_s_ins_pps_rsp_action(struct osmo_fsm_inst *fi, uint32_t event, void *data)
 {
 	struct iso7816_3_priv *ip = get_iso7816_3_priv(fi);
-	struct msgb *ppsrsp;
+	struct msgb *ppsrsp = data;
 	OSMO_ASSERT(fi->fsm == &iso7816_3_fsm);
 
 	switch (event) {
+	/* --v-- events from outside --v-- */
 	case ISO7816_E_RX_SINGLE:
-	case ISO7816_E_WTIME_EXP:
-		/* simply pass this through to the child FSM for the ATR */
+		/* simply pass this through to the child FSM for the PPS */
 		osmo_fsm_inst_dispatch(ip->pps_fi, event, data);
 		break;
+
+	/* --v-- events from childf fsm --v-- */
 	case ISO7816_E_PPS_DONE_IND:
-	case ISO7816_E_PPS_FAILED_IND:
-		ppsrsp = data;
 		osmo_fsm_inst_state_chg(fi, ISO7816_S_WAIT_TPDU, 0, 0);
 		/* notify user about PPS result */
 		ip->user_cb(fi, event, 0, ppsrsp);
 		break;
+	case ISO7816_E_PPS_UNSUPPORTED_IND:
+	case ISO7816_E_PPS_FAILED_IND:
 	case ISO7816_E_RX_ERR_IND:
-		ppsrsp = data;
+		/* error cases lead to slot reset */
 		osmo_fsm_inst_state_chg(fi, ISO7816_S_RESET, 0, 0);
+		/* notify user about PPS result */
 		ip->user_cb(fi, event, 0, ppsrsp);
 		break;
 	default:
@@ -466,6 +535,7 @@
 		.in_event_mask =	S(ISO7816_E_RESET_REL_IND) |
 							S(ISO7816_E_POWER_UP_IND) |
 							S(ISO7816_E_PPS_FAILED_IND)|
+							S(ISO7816_E_PPS_UNSUPPORTED_IND)|
 							S(ISO7816_E_TPDU_FAILED_IND),
 		.out_state_mask =	S(ISO7816_S_WAIT_ATR) |
 					S(ISO7816_S_RESET),
@@ -474,8 +544,7 @@
 	},
 	[ISO7816_S_WAIT_ATR] = {
 		.name = "WAIT_ATR",
-		.in_event_mask =	S(ISO7816_E_RX_SINGLE) |
-					S(ISO7816_E_WTIME_EXP),
+		.in_event_mask =	S(ISO7816_E_RX_SINGLE),
 		.out_state_mask =	S(ISO7816_S_RESET) |
 					S(ISO7816_S_IN_ATR),
 		.action = iso7816_3_wait_atr_action,
@@ -484,9 +553,7 @@
 		.name = "IN_ATR",
 		.in_event_mask =	S(ISO7816_E_RX_SINGLE) |
 					S(ISO7816_E_RX_ERR_IND) |
-					S(ISO7816_E_ATR_DONE_IND) |
-					S(ISO7816_E_ATR_ERR_IND) |
-					S(ISO7816_E_WTIME_EXP),
+					S(ISO7816_E_ATR_DONE_IND),
 		.out_state_mask =	S(ISO7816_S_RESET) |
 					S(ISO7816_S_IN_ATR) |
 					S(ISO7816_S_WAIT_TPDU),
@@ -510,8 +577,7 @@
 					S(ISO7816_E_TX_COMPL) |
 					S(ISO7816_E_RX_ERR_IND) |
 					S(ISO7816_E_TX_ERR_IND) |
-					S(ISO7816_E_TPDU_DONE_IND) |
-					S(ISO7816_E_WTIME_EXP),
+					S(ISO7816_E_TPDU_DONE_IND),
 		.out_state_mask =	S(ISO7816_S_RESET) |
 					S(ISO7816_S_WAIT_TPDU) |
 					S(ISO7816_S_IN_TPDU),
@@ -521,7 +587,6 @@
 		.name = "WAIT_PPS_RESP",
 		.in_event_mask =	S(ISO7816_E_TX_COMPL) |
 					S(ISO7816_E_TX_ERR_IND) |
-					S(ISO7816_E_WTIME_EXP) |
 					S(ISO7816_E_RX_SINGLE),
 		.out_state_mask =	S(ISO7816_S_RESET) |
 					S(ISO7816_S_WAIT_TPDU) |
@@ -536,7 +601,7 @@
 					S(ISO7816_E_RX_ERR_IND) |
 					S(ISO7816_E_PPS_DONE_IND) |
 					S(ISO7816_E_PPS_FAILED_IND) |
-					S(ISO7816_E_WTIME_EXP),
+					S(ISO7816_E_PPS_UNSUPPORTED_IND),
 		.out_state_mask =	S(ISO7816_S_RESET) |
 					S(ISO7816_S_WAIT_TPDU) |
 					S(ISO7816_S_IN_PPS_RSP),
@@ -554,7 +619,9 @@
 				S(ISO7816_E_POWER_DN_IND) |
 				S(ISO7816_E_RESET_ACT_IND) |
 				S(ISO7816_E_HW_ERR_IND) |
-				S(ISO7816_E_ABORT_REQ),
+				S(ISO7816_E_ABORT_REQ) |
+				S(ISO7816_E_WTIME_EXP) |
+				S(ISO7816_E_ATR_ERR_IND),
 };
 
 /***********************************************************************
@@ -617,10 +684,12 @@
 	atp->hist_len = 0;
 	atp->y = 0;
 	atp->i = 0;
-	if (!atp->atr)
-		atp->atr = msgb_alloc_c(fi, 33, "ATR"); /* TS + 32 chars */
-	else
-		msgb_reset(atp->atr);
+
+	/* might be used by cb later, but this is fine - error means broken, no one cares about a half-received atr
+	 * in the error repsonse anyway, next state after atr is wait tpdu, not reset, unless error, so we should
+	 * not end up here during normal operation.
+	 */
+	msgb_reset(atp->atr);
 	atp->computed_checksum = 0;
 	atp->protocol_support = 0;
 }
@@ -764,16 +833,16 @@
 		break;
 	case ISO7816_E_WTIME_EXP:
 		switch (fi->state) {
-		case ATR_S_WAIT_HIST:
-		case ATR_S_WAIT_TCK:
-			/* Some cards have an ATR with long indication of historical bytes */
-			/* FIXME: should we check the checksum? */
-			osmo_fsm_inst_state_chg(fi, ATR_S_DONE, 0, 0);
-			osmo_fsm_inst_dispatch(fi->proc.parent, ISO7816_E_ATR_DONE_IND, atp->atr);
-			break;
-		default:
-			osmo_fsm_inst_dispatch(fi->proc.parent, ISO7816_E_ATR_ERR_IND, NULL);
-			break;
+			case ATR_S_WAIT_HIST:
+			case ATR_S_WAIT_TCK:
+				/* Some cards have an ATR with long indication of historical bytes */
+				/* FIXME: should we check the checksum? */
+				osmo_fsm_inst_state_chg(fi, ATR_S_DONE, 0, 0);
+				osmo_fsm_inst_dispatch(fi->proc.parent, ISO7816_E_ATR_DONE_IND, atp->atr);
+				break;
+			default:
+				osmo_fsm_inst_dispatch(fi->proc.parent, ISO7816_E_ATR_ERR_IND, NULL);
+				break;
 		}
 		break;
 	default:
@@ -900,43 +969,41 @@
 /***********************************************************************
  * PPS FSM
  ***********************************************************************/
-struct pps_fsm_priv {
-	struct msgb* tx_cmd;
-	struct msgb* rx_cmd;
-	uint8_t pps0_recv;
-};
 
-static void pps_s_pps_req_init_onenter(struct osmo_fsm_inst *fi, uint32_t old_state)
+/* type-safe method to obtain pps_fsm_priv from fi */
+static struct pps_fsm_priv *get_pps_fsm_priv(struct osmo_fsm_inst *fi)
 {
-	struct pps_fsm_priv *atp = fi->priv;
-
-	if (!atp->rx_cmd)
-		atp->rx_cmd = msgb_alloc_c(fi, 6, "PPSRSP"); /* at most 6 */
-	else
-		msgb_reset(atp->rx_cmd);
-
-	/* notify in case card got pulled out */
-	if (atp->tx_cmd && old_state != PPS_S_DONE){
-		osmo_fsm_inst_dispatch(fi->proc.parent,
-				ISO7816_E_PPS_FAILED_IND, atp->tx_cmd);
-		atp->tx_cmd = 0;
-	}
+	OSMO_ASSERT(fi);
+	OSMO_ASSERT(fi->fsm == &pps_fsm);
+	return (struct pps_fsm_priv *) fi->priv;
 }
 
 static void pps_s_pps_req_init_action(struct osmo_fsm_inst *fi, uint32_t event, void *data)
 {
-	struct pps_fsm_priv *atp = fi->priv;
+	struct pps_fsm_priv *atp = get_pps_fsm_priv(fi);
 	struct osmo_fsm_inst *parent_fi = fi->proc.parent;
 	struct iso7816_3_priv *ip = get_iso7816_3_priv(parent_fi);
+	struct msgb* pps_to_transmit = atp->tx_cmd;
 
-	/* keep the buffer to compare it with the received response */
-	atp->tx_cmd = data;
+	/* data passed is PPS1, not msgb ptr! */
+	uint8_t PPS1 = (uint8_t)data;
 
 	switch (event) {
 	case ISO7816_E_XCEIVE_PPS_CMD:
+
+		/* buf might be required for cb until we end up here */
+		msgb_reset(atp->rx_cmd);
+		msgb_reset(atp->tx_cmd);
+
+		/* Hardware does not support SPU, so no PPS2, and PPS3 is reserved anyway */
+		msgb_put_u8(pps_to_transmit, 0xff);
+		msgb_put_u8(pps_to_transmit, (1 << 4)); /* only PPS1, T=0 */
+		msgb_put_u8(pps_to_transmit, PPS1);
+		msgb_put_u8(pps_to_transmit, 0xff ^ (1 << 4) ^ PPS1);
+
 		osmo_fsm_inst_state_chg(fi, PPS_S_TX_PPS_REQ, 0, 0);
 		card_uart_set_rx_threshold(ip->uart, 1);
-		card_uart_tx(ip->uart, msgb_data(data), msgb_length(data), true);
+		card_uart_tx(ip->uart, msgb_data(pps_to_transmit), msgb_length(pps_to_transmit), true);
 		break;
 	default:
 		OSMO_ASSERT(0);
@@ -958,9 +1025,6 @@
 	}
 }
 
-
-
-
 static void pps_wait_pX_action(struct osmo_fsm_inst *fi, uint32_t event, void *data)
 {
 	struct pps_fsm_priv *atp = fi->priv;
@@ -1020,30 +1084,19 @@
 
 				osmo_fsm_inst_state_chg(fi, PPS_S_DONE, 0, 0);
 
-				/* pps was successful if response equals request
-				 * rx buffer stays with the fsm, tx buffer gets handed back and freed
-				 * by the cb */
+				/* pps was successful if response equals request */
 				if (msgb_length(atp->rx_cmd) == msgb_length(atp->tx_cmd) &&
 					!memcmp(pps_received, pps_sent, msgb_length(atp->rx_cmd))) {
-					osmo_fsm_inst_dispatch(fi->proc.parent,
-							ISO7816_E_PPS_DONE_IND, atp->tx_cmd);
+					osmo_fsm_inst_dispatch(fi->proc.parent, ISO7816_E_PPS_DONE_IND, atp->tx_cmd);
 				} else {
-					osmo_fsm_inst_dispatch(fi->proc.parent,
-							ISO7816_E_PPS_FAILED_IND, atp->tx_cmd);
+					osmo_fsm_inst_dispatch(fi->proc.parent, ISO7816_E_PPS_FAILED_IND, atp->tx_cmd);
 				}
-				/* ownership transfer */
-				atp->tx_cmd = 0;
 			}
 			break;
 		default:
 			OSMO_ASSERT(0);
 		}
 		break;
-	case ISO7816_E_WTIME_EXP:
-		osmo_fsm_inst_state_chg(fi, PPS_S_DONE, 0, 0);
-		/* FIXME: timeout handling if no pps supported ? */
-		osmo_fsm_inst_dispatch(fi->proc.parent, ISO7816_E_RX_ERR_IND, NULL);
-		break;
 	default:
 		OSMO_ASSERT(0);
 	}
@@ -1062,12 +1115,10 @@
 static const struct osmo_fsm_state pps_states[] = {
 	[PPS_S_PPS_REQ_INIT] = {
 		.name = "INIT",
-		.in_event_mask =	S(ISO7816_E_XCEIVE_PPS_CMD) |
-							S(ISO7816_E_WTIME_EXP),
+		.in_event_mask =	S(ISO7816_E_XCEIVE_PPS_CMD),
 		.out_state_mask =	S(PPS_S_PPS_REQ_INIT) |
 							S(PPS_S_TX_PPS_REQ),
 		.action = pps_s_pps_req_init_action,
-		.onenter = pps_s_pps_req_init_onenter,
 	},
 	[PPS_S_TX_PPS_REQ] = {
 		.name = "TX_PPS_REQ",
@@ -1077,51 +1128,57 @@
 	},
 	[PPS_S_WAIT_PPSX] = {
 		.name = "WAIT_PPSS",
-		.in_event_mask =	S(ISO7816_E_RX_SINGLE) |
-							S(ISO7816_E_WTIME_EXP),
+		.in_event_mask =	S(ISO7816_E_RX_SINGLE),
 		.out_state_mask =	S(PPS_S_WAIT_PPS0) |
-							S(PPS_S_WAIT_PPSX),
+							S(PPS_S_WAIT_PPSX) |
+							S(PPS_S_PPS_REQ_INIT) |
+							S(PPS_S_DONE),
 		.action = pps_wait_pX_action,
 	},
 	[PPS_S_WAIT_PPS0] = {
 		.name = "WAIT_PPS0",
-		.in_event_mask =	S(ISO7816_E_RX_SINGLE) |
-							S(ISO7816_E_WTIME_EXP),
+		.in_event_mask =	S(ISO7816_E_RX_SINGLE),
 		.out_state_mask =	S(PPS_S_WAIT_PPS1) |
 							S(PPS_S_WAIT_PPS2) |
 							S(PPS_S_WAIT_PPS3) |
-							S(PPS_S_WAIT_PCK),
+							S(PPS_S_WAIT_PCK) |
+							S(PPS_S_PPS_REQ_INIT) |
+							S(PPS_S_DONE),
 		.action = pps_wait_pX_action,
 	},
 	[PPS_S_WAIT_PPS1] = {
 		.name = "WAIT_PPS1",
-		.in_event_mask =	S(ISO7816_E_RX_SINGLE) |
-							S(ISO7816_E_WTIME_EXP),
+		.in_event_mask =	S(ISO7816_E_RX_SINGLE),
 		.out_state_mask =	S(PPS_S_WAIT_PPS2) |
 							S(PPS_S_WAIT_PPS3) |
-							S(PPS_S_WAIT_PCK),
+							S(PPS_S_WAIT_PCK) |
+							S(PPS_S_PPS_REQ_INIT) |
+							S(PPS_S_DONE),
 		.action = pps_wait_pX_action,
 	},
 	[PPS_S_WAIT_PPS2] = {
 		.name = "WAIT_PPS2",
-		.in_event_mask =	S(ISO7816_E_RX_SINGLE) |
-							S(ISO7816_E_WTIME_EXP),
+		.in_event_mask =	S(ISO7816_E_RX_SINGLE),
 		.out_state_mask =	S(PPS_S_WAIT_PPS3) |
-							S(PPS_S_WAIT_PCK),
+							S(PPS_S_WAIT_PCK) |
+							S(PPS_S_PPS_REQ_INIT) |
+							S(PPS_S_DONE),
 		.action = pps_wait_pX_action,
 	},
 	[PPS_S_WAIT_PPS3] = {
 		.name = "WAIT_PPS3",
-		.in_event_mask =	S(ISO7816_E_RX_SINGLE) |
-							S(ISO7816_E_WTIME_EXP),
-		.out_state_mask =	S(PPS_S_WAIT_PCK),
+		.in_event_mask =	S(ISO7816_E_RX_SINGLE),
+		.out_state_mask =	S(PPS_S_WAIT_PCK) |
+							S(PPS_S_PPS_REQ_INIT) |
+							S(PPS_S_DONE),
 		.action = pps_wait_pX_action,
 	},
 	[PPS_S_WAIT_PCK] = {
 		.name = "WAIT_PCK",
-		.in_event_mask =	S(ISO7816_E_RX_SINGLE) |
-							S(ISO7816_E_WTIME_EXP),
-		.out_state_mask =	S(PPS_S_DONE),
+		.in_event_mask =	S(ISO7816_E_RX_SINGLE),
+		.out_state_mask =	S(PPS_S_DONE) |
+							S(PPS_S_PPS_REQ_INIT) |
+							S(PPS_S_DONE),
 		.action = pps_wait_pX_action,
 	},
 	[PPS_S_DONE] = {
@@ -1158,11 +1215,6 @@
 	return (struct osim_apdu_cmd_hdr *) msgb_data(msg);
 }
 
-struct tpdu_fsm_priv {
-	struct msgb *tpdu;
-	bool is_command; /* is this a command TPDU (true) or a response (false) */
-};
-
 /* type-safe method to obtain iso7816_3_priv from fi */
 static struct tpdu_fsm_priv *get_tpdu_fsm_priv(struct osmo_fsm_inst *fi)
 {
@@ -1171,17 +1223,6 @@
 	return (struct tpdu_fsm_priv *) fi->priv;
 }
 
-static void tpdu_s_init_onenter(struct osmo_fsm_inst *fi, uint32_t old_state)
-{
-	struct tpdu_fsm_priv *tfp = get_tpdu_fsm_priv(fi);
-
-	/* notify in case card got pulled out */
-	if (tfp->tpdu && old_state != TPDU_S_DONE){
-		osmo_fsm_inst_dispatch(fi->proc.parent, ISO7816_E_TPDU_FAILED_IND, tfp->tpdu);
-		tfp->tpdu = 0;
-	}
-}
-
 static void tpdu_s_init_action(struct osmo_fsm_inst *fi, uint32_t event, void *data)
 {
 	struct tpdu_fsm_priv *tfp = get_tpdu_fsm_priv(fi);
@@ -1191,8 +1232,12 @@
 
 	switch (event) {
 	case ISO7816_E_XCEIVE_TPDU_CMD:
+
+		/* buf might be required for cb until we end up here */
+		msgb_reset(tfp->tpdu);
+		COPY_TO_STATIC_MSGB(data, tfp->tpdu);
+
 		/* start transmission of a TPDU by sending the 5-byte header */
-		tfp->tpdu = (struct msgb *)data;
 		OSMO_ASSERT(msgb_length(tfp->tpdu) >= sizeof(*tpduh));
 		/* l2h = after the 5byte header */
 		tfp->tpdu->l2h = msgb_data(tfp->tpdu) + sizeof(*tpduh);
@@ -1455,8 +1500,6 @@
 		/* Notify parent FSM */
 		osmo_fsm_inst_dispatch(fi->proc.parent, ISO7816_E_TPDU_DONE_IND, tfp->tpdu);
 
-		/* ownership transfer */
-		tfp->tpdu = 0;
 		break;
 	default:
 		OSMO_ASSERT(0);
@@ -1497,7 +1540,6 @@
 		.out_state_mask = S(TPDU_S_INIT) |
 				  S(TPDU_S_TX_HDR),
 		.action = tpdu_s_init_action,
-		.onenter = tpdu_s_init_onenter,
 	},
 	[TPDU_S_TX_HDR] = {
 		.name = "TX_HDR",
@@ -1590,6 +1632,10 @@
 {
 	struct iso7816_3_priv *ip;
 	struct osmo_fsm_inst *fi;
+	struct atr_fsm_priv *atp;
+	struct pps_fsm_priv *ppsp;
+	struct tpdu_fsm_priv *tpdup;
+
 
 	fi = osmo_fsm_inst_alloc(&iso7816_3_fsm, ctx, NULL, log_level, id);
 	ip = talloc_zero(fi, struct iso7816_3_priv);
@@ -1611,6 +1657,9 @@
 	if (!ip->atr_fi->priv)
 		goto out_atr;
 
+	atp = get_atr_fsm_priv(ip->atr_fi);
+	INIT_STATIC_MSGB(atp->atr);
+
 	ip->tpdu_fi = osmo_fsm_inst_alloc_child(&tpdu_fsm, fi, ISO7816_E_SW_ERR_IND);
 	if (!ip->tpdu_fi)
 		goto out_atr;
@@ -1618,14 +1667,20 @@
 	if (!ip->tpdu_fi->priv)
 		goto out_tpdu;
 
-#if 1
+	tpdup = get_tpdu_fsm_priv(ip->tpdu_fi);
+	INIT_STATIC_MSGB(tpdup->tpdu);
+
 	ip->pps_fi = osmo_fsm_inst_alloc_child(&pps_fsm, fi, ISO7816_E_SW_ERR_IND);
 	if (!ip->pps_fi)
 		goto out_tpdu;
 	ip->pps_fi->priv = talloc_zero(ip->pps_fi, struct pps_fsm_priv);
 	if (!ip->pps_fi->priv)
 		goto out_pps;
-#endif
+
+	ppsp = get_pps_fsm_priv(ip->pps_fi);
+	INIT_STATIC_MSGB(ppsp->rx_cmd);
+	INIT_STATIC_MSGB(ppsp->tx_cmd);
+
 
 	/* This ensures the 'onenter' function of the initial state is called */
 	osmo_fsm_inst_state_chg(fi, ISO7816_S_RESET, 0, 0);
diff --git a/ccid_common/iso7816_fsm.h b/ccid_common/iso7816_fsm.h
index c0ef620..fcba87b 100644
--- a/ccid_common/iso7816_fsm.h
+++ b/ccid_common/iso7816_fsm.h
@@ -42,6 +42,7 @@
 	ISO7816_E_XCEIVE_PPS_CMD,
 	ISO7816_E_PPS_DONE_IND,
 	ISO7816_E_PPS_FAILED_IND,
+	ISO7816_E_PPS_UNSUPPORTED_IND,
 	/* TODO: Clock stop request */
 	/* TODO: Rx FIFO overrun */
 	/* TODO: Rx buffer overrun */

-- 
To view, visit https://gerrit.osmocom.org/c/osmo-ccid-firmware/+/21071
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-ccid-firmware
Gerrit-Branch: master
Gerrit-Change-Id: I65e77c376aca9ed50e234a0b58a7450a8bbd4fe0
Gerrit-Change-Number: 21071
Gerrit-PatchSet: 1
Gerrit-Owner: Hoernchen <ewild at sysmocom.de>
Gerrit-MessageType: newchange
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20201107/af0ceac8/attachment.htm>


More information about the gerrit-log mailing list