From: Robie B. <rb-...@ju...> - 2007-11-13 18:33:50
|
Hi, I have fixes for various issues I had when downloading data from a device that uses the mcp2150 chip (RS232->IrDA transceiver). Patches to follow, but I'll first explain everything here. My IrDA adapter is a Belkin Smartbeam, part number F5U235qea (USB 066f:4200) which uses the stir4200 driver. 1 IrCOMM negotiation failure This is happening because the mcp2150 appears to only be able to deal with one IrLMP connection for IAS queries, presumably because it has a minimal IrDA stack implementation. Previously the Linux stack assumed one IAS query per IrLMP connection, and IrCOMM used two (Parameters and IrDA:TinyTP:LsapSel). To fix this, I have completed more of the state machine in iriap.c so it doesn't end up in an unimplemented state after an IAS query, and modified ircomm_tty_attach.c to hold the object from iriap.c open between queries. Both IAS queries are now sent using the same IrLMP connection. Opening an IrCOMM connection to the mcp2150 using /dev/ircomm0 now works. I have also tested this with another device in case it broke something - it didn't. 2 stir4200 missing incoming frames Sometimes the stir4200 misses an incoming frame as it picks up 0xe0 BOF instead of a 0xc0 BOF. This caused me to notice problems 3 and 4 below. Using the stir4200 module parameter rx_sensitivity=6 fixed this. I tried various combinations and this is the only one that consistently gives me good performance. Is the default in stir4200.c correct? I've looked at the datasheet and found no indication of what would be a good default. I've looked at the archives and others have had problems with this too. In Windows, the device seems to work without timeouts and so is either being set up differently or is getting the equivalent of rx_sensitivity=6. Would changing the default be a good idea? 3 IrCOMM missing input validation for clen Once an incoming frame is dropped in 2 above, the next frame to come from the mcp2150 appears to be corrupt. Sometimes clen is bigger than the entire frame. This causes the control block to be ignored and the remaining (invalid) bytes of the frame to be passed as data. 4 Timeout during NRM_P This is the most baffling one. When an incoming frame is lost, we timeout on receiving a response from the secondary. According to the code and the spec, the correct response is to send an rr:cmd. This would be fine, but eventually the connection seems to stall while receiving lots of data from the secondary. I believe that the reason is that we never receive acknowledgement for TinyTP credits that we have sent (at a higher level of the stack), so the secondary gets starved of credits and stops sending. Why is it that after sending an i:cmd we send an rr:cmd on timeout instead of resending the i:cmd that we haven't received the acknowledgement for? What I'm seeing is an i:cmd sent, no response, an rr:cmd sent, an rr:rsp coming back, an rr:cmd sent, rr:rsp coming back and looping like this forever instead of getting more data from the secondary (i:rsp). The i:cmd that we sent containing TinyTP credits never gets acknowledged and we never resend it! I have patched this, but I'm not sure it should be applied without review. Patches to follow. Thanks, Robie |
From: Robie B. <rb-...@ju...> - 2007-11-13 18:42:04
|
iriap.c, iriap_event.c: support multiple queries by updating state after a query, rather than assuming that only one query will be made ircomm_tty_attach.c: use a single iriap object to make both queries when setting up a new connection. This is more efficient by saving two round trips, and simplifies IrCOMM setup from the point of view of the remote stack. The mcp2150, for example, cannot cope with multiple lsaps open at once, so fails otherwise. Signed-off-by: Robie Basak <rb-...@ju...> --- net/irda/ircomm/ircomm_tty_attach.c | 41 ++++++++++++++++++++++------------ net/irda/iriap.c | 7 +++-- net/irda/iriap_event.c | 11 ++++++++- 3 files changed, 40 insertions(+), 19 deletions(-) diff --git a/net/irda/ircomm/ircomm_tty_attach.c b/net/irda/ircomm/ircomm_tty_attach.c index b5a1388..880af75 100644 --- a/net/irda/ircomm/ircomm_tty_attach.c +++ b/net/irda/ircomm/ircomm_tty_attach.c @@ -433,10 +433,6 @@ static void ircomm_tty_getvalue_confirm(int result, __u16 obj_id, IRDA_ASSERT(self != NULL, return;); IRDA_ASSERT(self->magic == IRCOMM_TTY_MAGIC, return;); - /* We probably don't need to make any more queries */ - iriap_close(self->iriap); - self->iriap = NULL; - /* Check if request succeeded */ if (result != IAS_SUCCESS) { IRDA_DEBUG(4, "%s(), got NULL value!\n", __FUNCTION__ ); @@ -680,9 +676,8 @@ static int ircomm_tty_state_idle(struct ircomm_tty_cb *self, self->saddr = info->saddr; if (self->iriap) { - IRDA_WARNING("%s(), busy with a previous query\n", - __FUNCTION__); - return -EBUSY; + iriap_close(self->iriap); + self->iriap = NULL; } self->iriap = iriap_open(LSAP_ANY, IAS_CLIENT, self, @@ -703,6 +698,10 @@ static int ircomm_tty_state_idle(struct ircomm_tty_cb *self, ircomm_tty_next_state(self, IRCOMM_TTY_READY); break; case IRCOMM_TTY_WD_TIMER_EXPIRED: + if (self->iriap) { + iriap_close(self->iriap); + self->iriap = NULL; + } /* Just stay idle */ break; case IRCOMM_TTY_DETACH_CABLE: @@ -738,9 +737,8 @@ static int ircomm_tty_state_search(struct ircomm_tty_cb *self, self->saddr = info->saddr; if (self->iriap) { - IRDA_WARNING("%s(), busy with a previous query\n", - __FUNCTION__); - return -EBUSY; + iriap_close(self->iriap); + self->iriap = NULL; } self->iriap = iriap_open(LSAP_ANY, IAS_CLIENT, self, @@ -777,6 +775,10 @@ static int ircomm_tty_state_search(struct ircomm_tty_cb *self, ircomm_tty_start_watchdog_timer(self, 3*HZ); irlmp_discovery_request(DISCOVERY_DEFAULT_SLOTS); #endif + if (self->iriap) { + iriap_close(self->iriap); + self->iriap = NULL; + } break; case IRCOMM_TTY_DETACH_CABLE: ircomm_tty_next_state(self, IRCOMM_TTY_IDLE); @@ -807,15 +809,12 @@ static int ircomm_tty_state_query_parameters(struct ircomm_tty_cb *self, switch (event) { case IRCOMM_TTY_GOT_PARAMETERS: - if (self->iriap) { - IRDA_WARNING("%s(), busy with a previous query\n", + if (!self->iriap) { + IRDA_WARNING("%s(), no iriap object available\n", __FUNCTION__); return -EBUSY; } - self->iriap = iriap_open(LSAP_ANY, IAS_CLIENT, self, - ircomm_tty_getvalue_confirm); - iriap_getvaluebyclass_request(self->iriap, self->saddr, self->daddr, "IrDA:IrCOMM", "IrDA:TinyTP:LsapSel"); @@ -824,6 +823,10 @@ static int ircomm_tty_state_query_parameters(struct ircomm_tty_cb *self, ircomm_tty_next_state(self, IRCOMM_TTY_QUERY_LSAP_SEL); break; case IRCOMM_TTY_WD_TIMER_EXPIRED: + if (self->iriap) { + iriap_close(self->iriap); + self->iriap = NULL; + } /* Go back to search mode */ ircomm_tty_next_state(self, IRCOMM_TTY_SEARCH); ircomm_tty_start_watchdog_timer(self, 3*HZ); @@ -865,6 +868,10 @@ static int ircomm_tty_state_query_lsap_sel(struct ircomm_tty_cb *self, switch (event) { case IRCOMM_TTY_GOT_LSAPSEL: + if (self->iriap) { + iriap_close(self->iriap); + self->iriap = NULL; + } /* Connect to remote device */ ret = ircomm_connect_request(self->ircomm, self->dlsap_sel, self->saddr, self->daddr, @@ -873,6 +880,10 @@ static int ircomm_tty_state_query_lsap_sel(struct ircomm_tty_cb *self, ircomm_tty_next_state(self, IRCOMM_TTY_SETUP); break; case IRCOMM_TTY_WD_TIMER_EXPIRED: + if (self->iriap) { + iriap_close(self->iriap); + self->iriap = NULL; + } /* Go back to search mode */ ircomm_tty_next_state(self, IRCOMM_TTY_SEARCH); ircomm_tty_start_watchdog_timer(self, 3*HZ); diff --git a/net/irda/iriap.c b/net/irda/iriap.c index dc5e34a..12469ed 100644 --- a/net/irda/iriap.c +++ b/net/irda/iriap.c @@ -78,6 +78,7 @@ static int iriap_data_indication(void *instance, void *sap, struct sk_buff *skb); static void iriap_watchdog_timer_expired(void *data); +static void iriap_disconnect_request(struct iriap_cb *self); static inline void iriap_start_watchdog_timer(struct iriap_cb *self, int timeout) @@ -246,6 +247,9 @@ void iriap_close(struct iriap_cb *self) IRDA_ASSERT(self != NULL, return;); IRDA_ASSERT(self->magic == IAS_MAGIC, return;); + if (self->client_state == S_CALL) + iriap_disconnect_request(self); + if (self->lsap) { irlmp_close_lsap(self->lsap); self->lsap = NULL; @@ -517,9 +521,6 @@ static void iriap_getvaluebyclass_confirm(struct iriap_cb *self, break; } - /* Finished, close connection! */ - iriap_disconnect_request(self); - /* Warning, the client might close us, so remember no to use self * anymore after calling confirm */ diff --git a/net/irda/iriap_event.c b/net/irda/iriap_event.c index 8fb9d72..39e4263 100644 --- a/net/irda/iriap_event.c +++ b/net/irda/iriap_event.c @@ -235,6 +235,14 @@ static void state_s_call(struct iriap_cb *self, IRIAP_EVENT event, IRDA_ASSERT(self != NULL, return;); switch (event) { + case IAP_CALL_REQUEST_GVBC: + IRDA_ASSERT(self->request_skb == NULL, return;); + /* Don't forget to refcount it - + * see iriap_getvaluebyclass_request(). */ + skb_get(skb); + self->request_skb = skb; + iriap_do_call_event(self, IAP_CALL_REQUEST, skb); + break; case IAP_LM_DISCONNECT_INDICATION: /* Abort calls */ iriap_next_call_state(self, S_MAKE_CALL); @@ -302,7 +310,8 @@ static void state_s_outstanding(struct iriap_cb *self, IRIAP_EVENT event, /*iriap_send_ack(self);*/ /*LM_Idle_request(idle); */ - iriap_next_call_state(self, S_WAIT_FOR_CALL); + /* Back to start */ + iriap_next_call_state(self, S_MAKE_CALL); break; default: IRDA_DEBUG(0, "%s(), Unknown event %d\n", __FUNCTION__, event); -- 1.5.2.5 |
From: Robie B. <rb-...@ju...> - 2007-11-13 18:44:53
|
When using a stir4200-based USB adaptor to talk to a device that uses an mcp2150, the stir4200 sometimes drops an incoming frame causing the mcp2150 to try and retransmit the lost frame. In this combination, the next frame received from the mcp2150 is often invalid - either an empty i:rsp or an IrCOMM i:rsp with an invalid clen. These corner cases are now checked. Signed-off-by: Robie Basak <rb-...@ju...> --- net/irda/ircomm/ircomm_core.c | 10 ++++++++++ net/irda/irlap_event.c | 11 +++++++++++ 2 files changed, 21 insertions(+), 0 deletions(-) diff --git a/net/irda/ircomm/ircomm_core.c b/net/irda/ircomm/ircomm_core.c index 2d63fa8..08db47a 100644 --- a/net/irda/ircomm/ircomm_core.c +++ b/net/irda/ircomm/ircomm_core.c @@ -362,6 +362,16 @@ void ircomm_process_data(struct ircomm_cb *self, struct sk_buff *skb) clen = skb->data[0]; + /* Input validation check: a stir4200/mcp2150 combinations sometimes + * results in frames with clen > remaining packet size. These are + * illegal; if we throw away just this frame then it seems to carry on + * fine */ + if (skb->len < (clen+1)) { + IRDA_DEBUG(2, "%s() throwing away illegal frame\n", + __FUNCTION__ ); + return; + } + /* * If there are any data hiding in the control channel, we must * deliver it first. The side effect is that the control channel diff --git a/net/irda/irlap_event.c b/net/irda/irlap_event.c index 745f696..3516b1b 100644 --- a/net/irda/irlap_event.c +++ b/net/irda/irlap_event.c @@ -1199,6 +1199,17 @@ static int irlap_state_nrm_p(struct irlap_cb *self, IRLAP_EVENT event, switch (event) { case RECV_I_RSP: /* Optimize for the common case */ + if (skb->len <= LAP_ADDR_HEADER+LAP_CTRL_HEADER) { + /* Input validation check: a stir4200/mcp2150 + * combination sometimes results in an empty i:rsp. + * This makes no sense; we can just ignore the frame + * and send an rr:cmd immediately. This happens before + * changing nr or ns so triggers a retransmit */ + irlap_wait_min_turn_around(self, &self->qos_tx); + irlap_send_rr_frame(self, CMD_FRAME); + /* Keep state */ + break; + } /* FIXME: must check for remote_busy below */ #ifdef CONFIG_IRDA_FAST_RR /* -- 1.5.2.5 |
From: Robie B. <rb-...@ju...> - 2007-11-13 18:45:58
|
When receiving data using IrCOMM, the occasional i:cmd providing TTP credits was getting lost, eventually starving the sender of credits and stalling the connection. Signed-off-by: Robie Basak <rb-...@ju...> --- net/irda/irlap_event.c | 11 ++++++++--- 1 files changed, 8 insertions(+), 3 deletions(-) diff --git a/net/irda/irlap_event.c b/net/irda/irlap_event.c index 4c33bf5..745f696 100644 --- a/net/irda/irlap_event.c +++ b/net/irda/irlap_event.c @@ -1514,9 +1514,14 @@ static int irlap_state_nrm_p(struct irlap_cb *self, IRLAP_EVENT event, /* N2 is the disconnect timer. Until we reach it, we retry */ if (self->retry_count < self->N2) { - /* Retry sending the pf bit to the secondary */ - irlap_wait_min_turn_around(self, &self->qos_tx); - irlap_send_rr_frame(self, CMD_FRAME); + if (skb_peek(&self->wx_list) == NULL) { + IRDA_DEBUG(4, "ilap_state_nrm_p: resending rr"); + irlap_wait_min_turn_around(self, &self->qos_tx); + irlap_send_rr_frame(self, CMD_FRAME); + } else { + IRDA_DEBUG(4, "ilap_state_nrm_p: resend frames"); + irlap_resend_rejected_frames(self, CMD_FRAME); + } irlap_start_final_timer(self, self->final_timeout); self->retry_count++; -- 1.5.2.5 |
From: Samuel O. <sa...@so...> - 2007-11-26 00:55:25
|
Hi Robie, On Tue, Nov 13, 2007 at 06:45:57PM +0000, Robie Basak wrote: > When receiving data using IrCOMM, the occasional i:cmd providing TTP > credits was getting lost, eventually starving the sender of credits and > stalling the connection. Although out of specs, this looks reasonable to me. I'll push it for 2.6.25. Thanks a lot. Cheers, Samuel. > Signed-off-by: Robie Basak <rb-...@ju...> > --- > net/irda/irlap_event.c | 11 ++++++++--- > 1 files changed, 8 insertions(+), 3 deletions(-) > > diff --git a/net/irda/irlap_event.c b/net/irda/irlap_event.c > index 4c33bf5..745f696 100644 > --- a/net/irda/irlap_event.c > +++ b/net/irda/irlap_event.c > @@ -1514,9 +1514,14 @@ static int irlap_state_nrm_p(struct irlap_cb *self, IRLAP_EVENT event, > > /* N2 is the disconnect timer. Until we reach it, we retry */ > if (self->retry_count < self->N2) { > - /* Retry sending the pf bit to the secondary */ > - irlap_wait_min_turn_around(self, &self->qos_tx); > - irlap_send_rr_frame(self, CMD_FRAME); > + if (skb_peek(&self->wx_list) == NULL) { > + IRDA_DEBUG(4, "ilap_state_nrm_p: resending rr"); > + irlap_wait_min_turn_around(self, &self->qos_tx); > + irlap_send_rr_frame(self, CMD_FRAME); > + } else { > + IRDA_DEBUG(4, "ilap_state_nrm_p: resend frames"); > + irlap_resend_rejected_frames(self, CMD_FRAME); > + } > > irlap_start_final_timer(self, self->final_timeout); > self->retry_count++; > -- 1.5.2.5 > > ------------------------------------------------------------------------- > This SF.net email is sponsored by: Splunk Inc. > Still grepping through log files to find problems? Stop. > Now Search log events and configuration files using AJAX and a browser. > Download your FREE copy of Splunk now >> http://get.splunk.com/ > _______________________________________________ > irda-users mailing list > ird...@li... > http://lists.sourceforge.net/lists/listinfo/irda-users |