Add sm_block_ack() to statemachine?

[Osma Suominen]

Hi all,

I've been tackling 3110/statemachine related problems lately.

At first I was going to suggest that the statemachine semantics are
changed so that sm_message_send() always waits for an ack and resends
the message if necessary. sm_block() would then only wait for the reply
message. This would be cleaner with respect to protocol layers (since
messages/replies are one layer above message frames and ack frames).
The 3110 needs both isolated message sends (with no reply) and isolated
message receives (that aren't directly a reply to a preceding request).

However, then I realized that this change would lead to nasty race
conditions. Once you call gn_sm_loop() there is no guarantee that only
one frame is received; thus, you have to set up the statemachine to
expect a reply message before you can start waiting for an ack, even
though the reply message comes after the ack. If the received bytes are
buffered (e.g. during high load on a slow machine), both can be read
from the buffer in one go without resuming execution in the driver code
in between the ack and the reply.

My new suggestion is to add a new function to the statemachine. You
could label this a convenience function, just like the sm_block*
functions are labeled, but in fact all present drivers seem to
exclusively rely on sm_block* - the few exceptions are probably small
bugs (more on this below).

The new function is called sm_block_ack() and simply waits for an ack.
It is meant to be called right after sm_message_send(), when no reply
message is expected. Waiting for the ack is useful, because not doing so
would create problems if there was a transmission error. (Well, the
opposite could be true as well, because of the two-army problem...) The
3110 needs this functionality in a few places; it is possible that other
drivers could use it as well. However, if you think this is entirely
3110 specific it could sit in nk3110.c as well.

A patch that adds this function to the statemachine is in the end of
this message. It also removes one hack from __sm_block_timeout() which
becomes unnecessary with my upcoming 3110 and fbus-3110 fixes. Applying
this patch cripples SMS functionality on the 3110, but I will fix that
soon after (I have working code already, just waiting for CVS commit
rights that Pawel promised me - thanks for the trust!) so don't hesitate
to apply this because of that.

During the hacking I inspected all phone drivers, looking for strange
use of sm_message_send() and sm_block*, namely situations where one is
called without the other. I found a few places that did this, and I
believe many of them are buglets, whose practical consequences are small
or nonexistent (bad handling of transmit errors). Below is a list. The
ones that are real bugs could be fixed by adding calls to sm_block* or
perhaps the new sm_block_ack if there really is no reply message from
the phone.


* Places where sm_message_send is called without sm_block* following

atgen.c:573 (waitfor bug? see desc below)

nk6100.c:1005 (auth code doesn't check return msg)
nk6100.c:2985,2992,2995,3001,3004,3009 (making calls, check return msg later)
nk6100.c:3028 (answer call; return msg for 1st of 2 msgs not checked)
nk6100.c:3639 (NBSupload does't check return msg)

nk6510.c:59 (waitfor bug?)
nk6510.c:489,490 (Identify code sends 2 msgs then waits for both responses)

nk7110.c:64 (waitfor bug?)
nk7110.c:399,400 (Identify code sends 2 msgs then waits for both responses)

waitfor bug: sm_message_send is followed by sm_wait_for but not sm_block.
This seems like a misunderstanding made by the coder, since sm_wait_for
doesn't itself do much - specifically, it doesn't wait! Changing it to
sm_block() would probably be appropriate here.

* Places where sm_block* is called not immediately after sm_message_send

nk6100.c:3018 (making calls; return msg only checked after several msgs)

nk6510.c:1394 (SMS send code calls sm_block possibly several times)


Once again a huge message...oh well. I write ten times more descriptions
to the list than I write actual code :)

-Osma

--- common/gsm-statemachine.c.orig      2003-06-01 21:21:15.000000000 +0300
+++ common/gsm-statemachine.c   2003-06-02 21:09:57.000000000 +0300
@@ -225,9 +225,6 @@
                err = sm_wait_for(waitfor, data, state);
                if (err != GN_ERR_NONE) return err;

-               /* if no packet has been sent, don't wait for ack */
-               if (s == GN_SM_Initialised) break;
-
                timeradd(&now, &timeout, &next);
                do {
                        s = gn_sm_loop(1, state);  /* Timeout=100ms */
@@ -287,6 +284,37 @@
        return sm_block_no_retry_timeout(waitfor, 100, data, state);
 }

+/* Block waiting for an ack, don't wait for a reply message of any sort */
+gn_error sm_block_ack(struct gn_statemachine *state)
+{
+       int retry;
+       gn_state s;
+       gn_error err;
+       struct timeval now, next, timeout;
+
+       s = state->current_state;
+       timeout.tv_sec = 3;
+       timeout.tv_usec = 0;
+       gettimeofday(&now, NULL);
+       for (retry = 0; retry < 2; retry++) {
+               timeradd(&now, &timeout, &next);
+               do {
+                       s = gn_sm_loop(1, state);  /* Timeout=100ms */
+                       gettimeofday(&now, NULL);
+               } while (timercmp(&next, &now, >) && (s == GN_SM_MessageSent));
+
+               if (s == GN_SM_WaitingForResponse || s == 
GN_SM_ResponseReceived)
+                       return GN_ERR_NONE;
+
+               dprintf("sm_block_ack Retry - %d\n", retry);
+               sm_reset(state);
+               err = sm_message_send(state->last_msg_size, 
state->last_msg_type, state->last_msg, state);
+               if (err != GN_ERR_NONE) return err;
+       }
+
+       return GN_ERR_TIMEOUT;
+}
+
 /* Just to do things neatly */
 API gn_error gn_sm_functions(gn_operation op, gn_data *data, struct 
gn_statemachine *sm)
 {
--- include/gnokii-internal.h.orig      2003-06-02 21:15:43.000000000 +0300
+++ include/gnokii-internal.h   2003-06-01 22:05:05.000000000 +0300
@@ -54,6 +54,7 @@
 gn_error sm_block(int waitfor, gn_data *data, struct gn_statemachine *state);
 gn_error sm_block_no_retry_timeout(int waitfor, int t, gn_data *data, struct 
gn_statemachine *state);
 gn_error sm_block_no_retry(int waitfor, gn_data *data, struct gn_statemachine 
*state);
+gn_error sm_block_ack(struct gn_statemachine *state);
 void sm_message_dump(int messagetype, unsigned char *message, int length);
 void sm_unhandled_frame_dump(int messagetype, unsigned char *message, int 
length, struct gn_statemachine *state);


-- 
*** Osma Suominen *** address@hidden *** http://www.iki.fi/ozone/ ***