[PATCH v2 00/16] RATP logic fixes and improvements

[PATCH v2 00/16] RATP logic fixes and improvements

[Aleksander Morgado]

Hey Sascha,

This v2 series of patches includes just changes in the commit messages, trying to explain which of the patches apply to barebox and/or the barebox<->bbremote interactions. I did test all these patches also with RATP FS, and I didn't see any regression.

I did see issues in RATP FS happening with and without my patches, though. E.g. "cat"-ing a long file (>1024 bytes) that is being exported via RATFS results in each byte read being sent through the ratp output console messages one by one (the actual read operation over RATFS goes in chunks of 255, but the cat stdout messages via RATP go byte by byte...). After that, the additional read operations above offset 1024 end up timing out. I believe I'm going to have to work with RATFS in the near future, so I'll likely spend some time looking at those issues soon anyway.

Aleksander Morgado (16):
  ratp: add missing transition to SYN-RECEIVED in behavior B
  ratp: avoid unnecessary variable initializations
  ratp: send missing RST in behavior C2
  ratp: add missing RST flag in behavior G
  ratp: completely ignore RST flagged packets in behavior G
  ratp: fix data presence check
  ratp: fix single byte sending flagged with SO
  ratp: remove bogus data checks in behavior C2
  ratp: remove FIXME comment: FIN always requires ACK
  ratp: fix sending ACKs without data
  ratp: consolidate ratp_sn_expected() and ratp_an_expected()
  ratp: prefer using ratp_send_ack() in behaviour I1
  ratp: send initial data in behaviour B if any pending
  ratp: don't ignore data that may arrive in behaviour H1
  ratp: consolidate setting the next AN or SN flags
  ratp: user close may happen in SYN-RECEIVED state

 lib/ratp.c             | 116 +++++++++++++++++++++++++++----------------------
 scripts/remote/ratp.py |  38 +++++++---------
 2 files changed, 78 insertions(+), 76 deletions(-)

--
2.13.1

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 01/16] ratp: add missing transition to SYN-RECEIVED in behavior B

[Aleksander Morgado]

The reference says:

    If the SYN flag was set but the ACK was not set then the other
    end of the connection has executed an active open also.
    Acknowledge the SYN, choose your MDL, and send:
        <SN=0><AN=received SN+1 modulo 2><CTL=SYN, ACK><LENGTH=MDL>
    Go to the SYN-RECEIVED state without any further processing.

Add this missing step.

This fix isn't related to any barebox<->bbremote interaction issue,
because the case where both ends start an active connection doesn't
apply in this case.

Signed-off-by: Aleksander Morgado <aleksander@aleksander.es>
---
 lib/ratp.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/lib/ratp.c b/lib/ratp.c
index 22e83636f..0d384aa4e 100644
--- a/lib/ratp.c
+++ b/lib/ratp.c
@@ -619,21 +619,25 @@ static void ratp_behaviour_b(struct ratp_internal *ri, void *pkt)
 	if (hdr->control & RATP_CONTROL_SYN) {
 		uint8_t control;
 
+		ri->sn_received = ratp_sn(hdr);
+
 		if (hdr->control & RATP_CONTROL_ACK) {
 			control = ratp_set_sn(ratp_an(hdr)) |
 				ratp_set_an(!ratp_sn(hdr)) |
 				RATP_CONTROL_ACK;
+			ratp_send_hdr(ri, control);
+			ratp_state_change(ri, RATP_STATE_ESTABLISHED);
 		} else {
+			struct ratp_header synack = {};
+
 			control = ratp_set_an(!ratp_sn(hdr)) |
 				RATP_CONTROL_SYN |
 				RATP_CONTROL_ACK;
 
+			ratp_create_packet(ri, &synack, control, 255);
+			ratp_send_pkt(ri, &synack, sizeof(synack));
+			ratp_state_change(ri, RATP_STATE_SYN_RECEIVED);
 		}
-
-		ri->sn_received = ratp_sn(hdr);
-
-		ratp_send_hdr(ri, control);
-		ratp_state_change(ri, RATP_STATE_ESTABLISHED);
 	}
 }
 
-- 
2.13.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 02/16] ratp: avoid unnecessary variable initializations

[Aleksander Morgado]

This is just a cleanup; the variables are completely initialized later
on so the initial values are totally discarded anyway.

Signed-off-by: Aleksander Morgado <aleksander@aleksander.es>
---
 lib/ratp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/ratp.c b/lib/ratp.c
index 0d384aa4e..e9536499e 100644
--- a/lib/ratp.c
+++ b/lib/ratp.c
@@ -382,7 +382,7 @@ static int ratp_send_ack(struct ratp_internal *ri, struct ratp_header *hdr)
 static int ratp_send_next_data(struct ratp_internal *ri)
 {
 	uint16_t crc;
-	uint8_t control = RATP_CONTROL_ACK;
+	uint8_t control;
 	struct ratp_header *hdr;
 	int pktlen;
 	struct ratp_message *msg;
@@ -594,7 +594,7 @@ static void ratp_behaviour_b(struct ratp_internal *ri, void *pkt)
 
 	if ((hdr->control & RATP_CONTROL_ACK) && !ratp_an_expected(ri, hdr)) {
 		if (!(hdr->control & RATP_CONTROL_RST)) {
-			uint8_t control = RATP_CONTROL_RST;
+			uint8_t control;
 
 			control = RATP_CONTROL_RST |
 				ratp_set_sn(ratp_an(hdr));
-- 
2.13.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 03/16] ratp: send missing RST in behavior C2

[Aleksander Morgado]

The reference says:

    If SYN was set we assume that the other end crashed and has
    attempted to open a new connection.  We respond by sending a
    legal reset:
        <SN=received AN><AN=received SN+1 modulo 2><CTL=RST, ACK>

Add this missing step.

This issue affects the barebox<->bbremote interaction if bbremote
crashes without closing the channel and restarts opening a complete
new connection.

Signed-off-by: Aleksander Morgado <aleksander@aleksander.es>
---
 lib/ratp.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/lib/ratp.c b/lib/ratp.c
index e9536499e..a8ac52c75 100644
--- a/lib/ratp.c
+++ b/lib/ratp.c
@@ -731,8 +731,15 @@ static int ratp_behaviour_c2(struct ratp_internal *ri, void *pkt)
 		return 1;
 
 	if (hdr->control & RATP_CONTROL_SYN) {
+		uint8_t control;
+
 		ri->status = -ECONNRESET;
 		pr_debug("Error: Connection reset\n");
+
+		control = RATP_CONTROL_RST | RATP_CONTROL_ACK |
+			ratp_set_sn(ratp_an(hdr)) | ratp_set_an(!ratp_sn(hdr));
+		ratp_send_hdr(ri, control);
+
 		ratp_state_change(ri, RATP_STATE_CLOSED);
 		return 1;
 	}
-- 
2.13.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 04/16] ratp: add missing RST flag in behavior G

[Aleksander Morgado]

The reference says:

   If the ACK flag was set then send:
       <SN=received AN><CTL=RST>
   If the ACK flag was not set then send:
       <SN=0><AN=received SN+1 modulo 2><CTL=RST, ACK>
   (i.e. in both cases RST is needed)

The RST flag is set initially in the 'control' variable; so instead of
completely overwriting it, make sure the additional flags are OR-ed
to the current value.

This fix isn't related to any failed barebox<->bbremote interaction
(it really is a very rare corner case).

Signed-off-by: Aleksander Morgado <aleksander@aleksander.es>
---
 lib/ratp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/ratp.c b/lib/ratp.c
index a8ac52c75..43b8b04dc 100644
--- a/lib/ratp.c
+++ b/lib/ratp.c
@@ -1038,7 +1038,7 @@ static int ratp_behaviour_g(struct ratp_internal *ri, void *pkt)
 	if (hdr->control & RATP_CONTROL_ACK)
 		control |= ratp_set_sn(ratp_an(hdr));
 	else
-		control = ratp_set_an(ratp_sn(hdr) + 1) | RATP_CONTROL_ACK;
+		control |= ratp_set_an(ratp_sn(hdr) + 1) | RATP_CONTROL_ACK;
 
 	ratp_send_hdr(ri, control);
 
-- 
2.13.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 05/16] ratp: completely ignore RST flagged packets in behavior G

[Aleksander Morgado]

The reference says:

    This procedure represents the behavior of the CLOSED state of a
    connection. All incoming packets are discarded. If the packet
    had the RST flag set take no action. Otherwise it is necessary
    to build a RST packet.

So, skip building the RST packet if the incoming one had RST set.

This commit fixes an infinite loop of messages sent and received
between both ends during the connection close procedure, found when
testing barebox against a third party ratp implementation.

Signed-off-by: Aleksander Morgado <aleksander@aleksander.es>
---
 lib/ratp.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/lib/ratp.c b/lib/ratp.c
index 43b8b04dc..d3c252047 100644
--- a/lib/ratp.c
+++ b/lib/ratp.c
@@ -1033,6 +1033,9 @@ static int ratp_behaviour_g(struct ratp_internal *ri, void *pkt)
 
 	pr_debug("%s\n", __func__);
 
+	if (hdr->control & RATP_CONTROL_RST)
+		return 0;
+
 	control = RATP_CONTROL_RST;
 
 	if (hdr->control & RATP_CONTROL_ACK)
-- 
2.13.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 06/16] ratp: fix data presence check

[Aleksander Morgado]

Looking at the "data length" and SO flag isn't enough to declare a
packet with or without data, because SYN flagged packets will also use
the "data length" field to define MDL.

So, improve the check to match against SYN|RST|FIN flagged packets,
which can never have data.

This commit fixed a segfault in barebox when an unexpected SYN packet
was sent in the middle of a connection; barebox thought the packet had
data because the "data length" in the SYN packet was different than 0.

Signed-off-by: Aleksander Morgado <aleksander@aleksander.es>
---
 lib/ratp.c             | 4 ++--
 scripts/remote/ratp.py | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/lib/ratp.c b/lib/ratp.c
index d3c252047..c946bea1a 100644
--- a/lib/ratp.c
+++ b/lib/ratp.c
@@ -165,7 +165,7 @@ static bool ratp_has_data(struct ratp_header *hdr)
 {
 	if (hdr->control & RATP_CONTROL_SO)
 		return 1;
-	if (hdr->data_length)
+	if (!(hdr->control & (RATP_CONTROL_SYN | RATP_CONTROL_RST | RATP_CONTROL_FIN)) && hdr->data_length)
 		return 1;
 	return 0;
 }
@@ -1338,7 +1338,7 @@ static int ratp_behaviour_i1(struct ratp_internal *ri, void *pkt)
 	struct ratp_header *hdr = pkt;
 	uint8_t control = 0;
 
-	if (!hdr->data_length && !(hdr->control & RATP_CONTROL_SO))
+	if (!ratp_has_data (hdr))
 		return 1;
 
 	pr_vdebug("%s **received** %d\n", __func__, hdr->data_length);
diff --git a/scripts/remote/ratp.py b/scripts/remote/ratp.py
index 079fb871a..a41d2e8a3 100644
--- a/scripts/remote/ratp.py
+++ b/scripts/remote/ratp.py
@@ -525,7 +525,7 @@ class RatpConnection(object):
             # Our fin was lost, rely on retransmission
             return False
 
-        if r.length or r.c_so:
+        if (r.length and not r.c_syn and not r.c_rst and not r.c_fin) or r.c_so:
             self._retrans = None
             s = RatpPacket(flags='RA')
             s.c_sn = r.c_an
@@ -596,7 +596,7 @@ class RatpConnection(object):
         if r.c_so:
             self._r_sn = r.c_sn
             self._rx_buf.append(chr(r.length))
-        elif r.length:
+        elif r.length and not r.c_syn and not r.c_rst and not r.c_fin:
             self._r_sn = r.c_sn
             self._rx_buf.append(r.payload)
         else:
-- 
2.13.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 07/16] ratp: fix single byte sending flagged with SO

[Aleksander Morgado]

When a single data byte is sent, it is flagged as SO and the actual
data goes in the length byte within the header.

Worth noting that this issue wasn't found in any barebox<->bbremote
interaction because all data packets sent between them always have
more than one byte (due to the barebox command specific header).

Signed-off-by: Aleksander Morgado <aleksander@aleksander.es>
---
 lib/ratp.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/lib/ratp.c b/lib/ratp.c
index c946bea1a..846f8130c 100644
--- a/lib/ratp.c
+++ b/lib/ratp.c
@@ -384,6 +384,7 @@ static int ratp_send_next_data(struct ratp_internal *ri)
 	uint16_t crc;
 	uint8_t control;
 	struct ratp_header *hdr;
+	uint8_t *data;
 	int pktlen;
 	struct ratp_message *msg;
 	int len;
@@ -409,19 +410,19 @@ static int ratp_send_next_data(struct ratp_internal *ri)
 		RATP_CONTROL_ACK;
 
 	hdr = msg->buf;
+	data = (uint8_t *)(hdr + 1);
 
 	if (msg->eor)
 		control |= RATP_CONTROL_EOR;
 
+	pktlen = sizeof(struct ratp_header);
 	if (len > 1) {
-		void *data = hdr + 1;
-		pktlen = sizeof(*hdr) + len + 2;
+		pktlen += len + 2;
 		crc = cyg_crc16(data, len);
 		put_unaligned_be16(crc, data + len);
-	} else {
-		pktlen = sizeof(struct ratp_header);
+	} else if (len == 1) {
 		control |= RATP_CONTROL_SO;
-		len = 0;
+		len = *data;
 	}
 
 	ratp_create_packet(ri, hdr, control, len);
-- 
2.13.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 08/16] ratp: remove bogus data checks in behavior C2

[Aleksander Morgado]

The SN validation was being completely ignored if the packet had no
data (e.g. for RST, FIN or SYN or plain ACKs). This condition is now
removed so that the SN check is done.

The second check removed was actually never being used, as it was
already being tested for not having data in the first one.

These two fixes are a cleanup to follow the protocol correctly.

Signed-off-by: Aleksander Morgado <aleksander@aleksander.es>
---
 lib/ratp.c             |  8 +-------
 scripts/remote/ratp.py | 16 +++++-----------
 2 files changed, 6 insertions(+), 18 deletions(-)

diff --git a/lib/ratp.c b/lib/ratp.c
index 846f8130c..321721ab7 100644
--- a/lib/ratp.c
+++ b/lib/ratp.c
@@ -721,9 +721,6 @@ static int ratp_behaviour_c2(struct ratp_internal *ri, void *pkt)
 
 	pr_debug("%s\n", __func__);
 
-	if (!ratp_has_data(hdr))
-		return 0;
-
 	if (ratp_sn_expected(ri, hdr))
 		return 0;
 
@@ -745,9 +742,6 @@ static int ratp_behaviour_c2(struct ratp_internal *ri, void *pkt)
 		return 1;
 	}
 
-	if (!ratp_has_data(hdr))
-		return 1;
-
 	pr_debug("Sending ack for duplicate message\n");
 	ret = ratp_send_ack(ri, hdr);
 	if (ret)
@@ -1846,4 +1840,4 @@ eor:
 	}
 
 	return 0;
-}
\ No newline at end of file
+}
diff --git a/scripts/remote/ratp.py b/scripts/remote/ratp.py
index a41d2e8a3..0dfc8420c 100644
--- a/scripts/remote/ratp.py
+++ b/scripts/remote/ratp.py
@@ -339,9 +339,6 @@ class RatpConnection(object):
     def _c2(self, r):
         logging.info("C2")
 
-        if r.length == 0 and r.c_so == 0:
-            return True
-
         if r.c_sn != self._r_sn:
             return True
 
@@ -358,14 +355,11 @@ class RatpConnection(object):
             self._state = RatpState.closed
             return False
 
-        # FIXME: only ack duplicate data packages?
-        # This is not documented in RFC 916
-        if r.length or r.c_so:
-            logging.info("C2: duplicate data packet, dropping")
-            s = RatpPacket(flags='A')
-            s.c_sn = r.c_an
-            s.c_an = (r.c_sn + 1) % 2
-            self._write(s)
+        logging.info("C2: duplicate packet")
+        s = RatpPacket(flags='A')
+        s.c_sn = r.c_an
+        s.c_an = (r.c_sn + 1) % 2
+        self._write(s)
 
         return False
 
-- 
2.13.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 09/16] ratp: remove FIXME comment: FIN always requires ACK

[Aleksander Morgado]

Section 3.4 in the RFC916 shows a packet flow for the connection close
where the initial packet sent by the endpoint starting the close has
just the FIN flag set, without an ACK:

          -->     <SN=0><AN=1><CTL=FIN>
                <SN=1><AN=1><CTL=FIN,ACK>  <--
          -->     <SN=1><AN=0><CTL=ACK>

This may lead to think that it is actually allowed to send the initial
packet with just FIN set, without ACK-ing any other packet from the
peer.

But, this is actually not possible, the packet MUST be ACK-ing a
previous packet from the peer, even if this is just a duplicated ACK,
because otherwise the packet with the FIN wouldn't get processed in
the H2 behavior (FIN processing) of the peer, as the F2 behavior (ACK
processing) would filter it out.

This is actually the same reasoning why data packets always have ACK
set, even if the same ACK has already been sent previously (e.g. with
a simple ACK packet without data); if they didn't have it, they would
be filtered out in the F2 behavior, never arriving the I1 behavior,
which is where the received data is processed.

Signed-off-by: Aleksander Morgado <aleksander@aleksander.es>
---
 scripts/remote/ratp.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/remote/ratp.py b/scripts/remote/ratp.py
index 0dfc8420c..e6b3e19b6 100644
--- a/scripts/remote/ratp.py
+++ b/scripts/remote/ratp.py
@@ -724,7 +724,7 @@ class RatpConnection(object):
         deadline = monotonic() + timeout
         logging.info("CLOSE")
         if self._state == RatpState.established:
-            fin = RatpPacket(flags='FA')  # FIXME: only F?
+            fin = RatpPacket(flags='FA')
             fin.c_sn = (self._s_sn + 1) % 2
             fin.c_an = (self._r_sn + 1) % 2
             self._write(fin)
-- 
2.13.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 10/16] ratp: fix sending ACKs without data

[Aleksander Morgado]

All ACKs without data must be built in the same way from the input
message:
     <SN=received AN><AN=received SN+1 modulo 2><CTL=ACK>

The code was actually doing this instead:
     <SN=0><AN=SN><CTL=ACK>

This change fixes how the retransmissions are ACK-ed by the peer, and
would have affected the barebox<->bbremote interactions.

Signed-off-by: Aleksander Morgado <aleksander@aleksander.es>
---
 lib/ratp.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/lib/ratp.c b/lib/ratp.c
index 321721ab7..5c52d3b5f 100644
--- a/lib/ratp.c
+++ b/lib/ratp.c
@@ -364,13 +364,12 @@ static bool ratp_sn_expected(struct ratp_internal *ri, struct ratp_header *hdr)
 
 static int ratp_send_ack(struct ratp_internal *ri, struct ratp_header *hdr)
 {
-	uint8_t control = RATP_CONTROL_ACK;
+	uint8_t control;
 	int ret;
 
-	if (hdr->control & RATP_CONTROL_SN)
-		control |= RATP_CONTROL_AN;
-	else
-		control |= 0;
+	control = ratp_set_sn(ratp_an(hdr)) |
+		ratp_set_an(ratp_sn(hdr) + 1) |
+		RATP_CONTROL_ACK;
 
 	ret = ratp_send_hdr(ri, control);
 	if (ret)
-- 
2.13.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 11/16] ratp: consolidate ratp_sn_expected() and ratp_an_expected()

[Aleksander Morgado]

This is no code change in ratp_sn_expected(), it's just rewritten in
the same way as ratp_an_expected(), which follows the RFC916 approach.

Signed-off-by: Aleksander Morgado <aleksander@aleksander.es>
---
 lib/ratp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/ratp.c b/lib/ratp.c
index 5c52d3b5f..46a2b645c 100644
--- a/lib/ratp.c
+++ b/lib/ratp.c
@@ -359,7 +359,7 @@ static bool ratp_an_expected(struct ratp_internal *ri, struct ratp_header *hdr)
 
 static bool ratp_sn_expected(struct ratp_internal *ri, struct ratp_header *hdr)
 {
-	return ratp_sn(hdr) != ri->sn_received;
+	return ratp_sn(hdr) == (ri->sn_received + 1) % 2;
 }
 
 static int ratp_send_ack(struct ratp_internal *ri, struct ratp_header *hdr)
-- 
2.13.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 12/16] ratp: prefer using ratp_send_ack() in behaviour I1

[Aleksander Morgado]

Instead of manually constructing an ACK, use the ratp_send_ack()
method, which already does that properly.

Signed-off-by: Aleksander Morgado <aleksander@aleksander.es>
---
 lib/ratp.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/lib/ratp.c b/lib/ratp.c
index 46a2b645c..c7f3f4171 100644
--- a/lib/ratp.c
+++ b/lib/ratp.c
@@ -1330,7 +1330,6 @@ static int msg_recv(struct ratp_internal *ri, void *pkt)
 static int ratp_behaviour_i1(struct ratp_internal *ri, void *pkt)
 {
 	struct ratp_header *hdr = pkt;
-	uint8_t control = 0;
 
 	if (!ratp_has_data (hdr))
 		return 1;
@@ -1341,15 +1340,10 @@ static int ratp_behaviour_i1(struct ratp_internal *ri, void *pkt)
 
 	msg_recv(ri, pkt);
 
-	if (list_empty(&ri->sendmsg) || ri->sendmsg_current) {
-		control = ratp_set_sn(!ri->sn_sent) |
-			ratp_set_an(ri->sn_received + 1) |
-			RATP_CONTROL_ACK;
-
-		ratp_send_hdr(ri, control);
-	} else {
+	if (list_empty(&ri->sendmsg) || ri->sendmsg_current)
+		ratp_send_ack(ri, hdr);
+	else
 		ratp_send_next_data(ri);
-	}
 
 	return 0;
 }
-- 
2.13.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 13/16] ratp: send initial data in behaviour B if any pending

[Aleksander Morgado]

And also, use ratp_send_ack() instead of manually constructing an ACK
if no data is pending to be sent.

The current barebox implementation doesn't allow any queueing of data
until the connection is established, so this is probably not a case
that would get run anyway.

Signed-off-by: Aleksander Morgado <aleksander@aleksander.es>
---
 lib/ratp.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/lib/ratp.c b/lib/ratp.c
index c7f3f4171..e810a9e54 100644
--- a/lib/ratp.c
+++ b/lib/ratp.c
@@ -622,11 +622,11 @@ static void ratp_behaviour_b(struct ratp_internal *ri, void *pkt)
 		ri->sn_received = ratp_sn(hdr);
 
 		if (hdr->control & RATP_CONTROL_ACK) {
-			control = ratp_set_sn(ratp_an(hdr)) |
-				ratp_set_an(!ratp_sn(hdr)) |
-				RATP_CONTROL_ACK;
-			ratp_send_hdr(ri, control);
 			ratp_state_change(ri, RATP_STATE_ESTABLISHED);
+			if (list_empty(&ri->sendmsg) || ri->sendmsg_current)
+				ratp_send_ack(ri, hdr);
+			else
+				ratp_send_next_data(ri);
 		} else {
 			struct ratp_header synack = {};
 
-- 
2.13.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 14/16] ratp: don't ignore data that may arrive in behaviour H1

[Aleksander Morgado]

If an input packet arrives H1 that has data in it, we need to:
  * track sn_received
  * if we have data pending, send it
  * if we don't have data pending, send a plain ACK

This process, as noted in RFC916, is the same as the I1 procedure, so
go and run it:

     Go to the ESTABLISHED state and execute procedure I1 to process
     any data which might be in this packet.

This fix allows the peer to queue data in the last packet doing the
connection establishment. It doesn't apply to the barebox<->bbremote
interaction because bbremote won't queue data until the connection is
completely established, but it allows third party ratp implementations
to do that.

Signed-off-by: Aleksander Morgado <aleksander@aleksander.es>
---
 lib/ratp.c             |  8 +++++++-
 scripts/remote/ratp.py | 14 ++++++--------
 2 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/lib/ratp.c b/lib/ratp.c
index e810a9e54..2dee41009 100644
--- a/lib/ratp.c
+++ b/lib/ratp.c
@@ -1042,6 +1042,8 @@ static int ratp_behaviour_g(struct ratp_internal *ri, void *pkt)
 	return 0;
 }
 
+static int ratp_behaviour_i1(struct ratp_internal *ri, void *pkt);
+
 /*
  * Our SYN has been acknowledged.  At this point we are
  * technically in the ESTABLISHED state.  Send any initial data
@@ -1062,7 +1064,11 @@ static int ratp_behaviour_h1(struct ratp_internal *ri, void *pkt)
 
 	ratp_state_change(ri, RATP_STATE_ESTABLISHED);
 
-	return 0;
+	/* If the input message has data (i.e. it is not just an ACK
+	 * without data) then we need to send back an ACK ourselves,
+	 * or even data if we have it pending. This is the same
+	 * procedure done in i1, so just run it. */
+	return ratp_behaviour_i1 (ri, pkt);
 }
 
 /*
diff --git a/scripts/remote/ratp.py b/scripts/remote/ratp.py
index e6b3e19b6..7972d31f2 100644
--- a/scripts/remote/ratp.py
+++ b/scripts/remote/ratp.py
@@ -489,12 +489,8 @@ class RatpConnection(object):
 
     def _h1(self, r):
         logging.info("H1")
-
-        # FIXME: initial data?
         self._state = RatpState.established
-        self._r_sn = r.c_sn
-
-        return False
+        return self._common_i1(r)
 
     def _h2(self, r):
         logging.info("H2")
@@ -584,9 +580,7 @@ class RatpConnection(object):
         self._time_wait_deadline = monotonic() + self._get_rto()
         return False
 
-    def _i1(self, r):
-        logging.info("I1")
-
+    def _common_i1(self, r):
         if r.c_so:
             self._r_sn = r.c_sn
             self._rx_buf.append(chr(r.length))
@@ -608,6 +602,10 @@ class RatpConnection(object):
         self._write(s)
         return False
 
+    def _i1(self, r):
+        logging.info("I1")
+        return self._common_i1(r)
+
     def _machine(self, pkt):
         logging.info("State: %r", self._state)
         if self._state == RatpState.listen:
-- 
2.13.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 15/16] ratp: consolidate setting the next AN or SN flags

[Aleksander Morgado]

Setting the next AN or SN flag was being done in two different ways
throughout the code; e.g. here for AN:

    ratp_set_an(ratp_sn(hdr) + 1);
or:
    ratp_set_an(!ratp_sn(hdr));

We define a pair of new ratp_set_next_sn() and ratp_set_next_an()
macros that make it clear that the next value is desired, and also
hide the computation of the actual flag value within the macro, so the
previous example would now look like:

    ratp_set_next_an(ratp_sn(hdr));

Signed-off-by: Aleksander Morgado <aleksander@aleksander.es>
---
 lib/ratp.c | 30 ++++++++++++++++--------------
 1 file changed, 16 insertions(+), 14 deletions(-)

diff --git a/lib/ratp.c b/lib/ratp.c
index 2dee41009..46a82c69a 100644
--- a/lib/ratp.c
+++ b/lib/ratp.c
@@ -139,8 +139,10 @@ static bool ratp_an(struct ratp_header *hdr)
 	return hdr->control & RATP_CONTROL_AN ? 1 : 0;
 }
 
-#define ratp_set_sn(sn) (((sn) % 2) ? RATP_CONTROL_SN : 0)
-#define ratp_set_an(an) (((an) % 2) ? RATP_CONTROL_AN : 0)
+#define ratp_set_sn(sn) (sn ? RATP_CONTROL_SN : 0)
+#define ratp_set_an(an) (an ? RATP_CONTROL_AN : 0)
+#define ratp_set_next_sn(sn) (((sn + 1) % 2) ? RATP_CONTROL_SN : 0)
+#define ratp_set_next_an(an) (((an + 1) % 2) ? RATP_CONTROL_AN : 0)
 
 static inline int ratp_header_ok(struct ratp_internal *ri, struct ratp_header *h)
 {
@@ -368,7 +370,7 @@ static int ratp_send_ack(struct ratp_internal *ri, struct ratp_header *hdr)
 	int ret;
 
 	control = ratp_set_sn(ratp_an(hdr)) |
-		ratp_set_an(ratp_sn(hdr) + 1) |
+		ratp_set_next_an(ratp_sn(hdr)) |
 		RATP_CONTROL_ACK;
 
 	ret = ratp_send_hdr(ri, control);
@@ -404,8 +406,8 @@ static int ratp_send_next_data(struct ratp_internal *ri)
 
 	len = msg->len;
 
-	control = ratp_set_sn(ri->sn_sent + 1) |
-		ratp_set_an(ri->sn_received + 1) |
+	control = ratp_set_next_sn(ri->sn_sent) |
+		ratp_set_next_an(ri->sn_received) |
 		RATP_CONTROL_ACK;
 
 	hdr = msg->buf;
@@ -630,7 +632,7 @@ static void ratp_behaviour_b(struct ratp_internal *ri, void *pkt)
 		} else {
 			struct ratp_header synack = {};
 
-			control = ratp_set_an(!ratp_sn(hdr)) |
+			control = ratp_set_next_an(ratp_sn(hdr)) |
 				RATP_CONTROL_SYN |
 				RATP_CONTROL_ACK;
 
@@ -734,7 +736,7 @@ static int ratp_behaviour_c2(struct ratp_internal *ri, void *pkt)
 		pr_debug("Error: Connection reset\n");
 
 		control = RATP_CONTROL_RST | RATP_CONTROL_ACK |
-			ratp_set_sn(ratp_an(hdr)) | ratp_set_an(!ratp_sn(hdr));
+			ratp_set_sn(ratp_an(hdr)) | ratp_set_next_an(ratp_sn(hdr));
 		ratp_send_hdr(ri, control);
 
 		ratp_state_change(ri, RATP_STATE_CLOSED);
@@ -1035,7 +1037,7 @@ static int ratp_behaviour_g(struct ratp_internal *ri, void *pkt)
 	if (hdr->control & RATP_CONTROL_ACK)
 		control |= ratp_set_sn(ratp_an(hdr));
 	else
-		control |= ratp_set_an(ratp_sn(hdr) + 1) | RATP_CONTROL_ACK;
+		control |= ratp_set_next_an(ratp_sn(hdr)) | RATP_CONTROL_ACK;
 
 	ratp_send_hdr(ri, control);
 
@@ -1099,7 +1101,7 @@ static int ratp_behaviour_h2(struct ratp_internal *ri, void *pkt)
 	ri->status = -ENETDOWN;
 
 	control = ratp_set_sn(ratp_an(hdr)) |
-		ratp_set_an(ratp_sn(hdr) + 1) |
+		ratp_set_next_an(ratp_sn(hdr)) |
 		RATP_CONTROL_FIN |
 		RATP_CONTROL_ACK;
 
@@ -1165,7 +1167,7 @@ static int ratp_behaviour_h3(struct ratp_internal *ri, void *pkt)
 
 	if (ratp_has_data(hdr)) {
 		control = ratp_set_sn(ratp_an(hdr)) |
-			ratp_set_an(ratp_sn(hdr) + 1) |
+			ratp_set_next_an(ratp_sn(hdr)) |
 			RATP_CONTROL_RST |
 			RATP_CONTROL_ACK;
 		ratp_send_hdr(ri, control);
@@ -1176,7 +1178,7 @@ static int ratp_behaviour_h3(struct ratp_internal *ri, void *pkt)
 	}
 
 	control = ratp_set_sn(ratp_an(hdr)) |
-		ratp_set_an(ratp_sn(hdr) + 1) |
+		ratp_set_next_an(ratp_sn(hdr)) |
 		RATP_CONTROL_ACK;
 
 	expected = ratp_an_expected(ri, hdr);
@@ -1278,7 +1280,7 @@ static int ratp_behaviour_h6(struct ratp_internal *ri, void *pkt)
 	if (!(hdr->control & RATP_CONTROL_FIN))
 		return 1;
 
-	control = ratp_set_sn(ratp_an(hdr) + 1) | RATP_CONTROL_ACK;
+	control = ratp_set_next_sn(ratp_an(hdr)) | RATP_CONTROL_ACK;
 
 	ratp_send_hdr(ri, control);
 
@@ -1695,8 +1697,8 @@ void ratp_close(struct ratp *ratp)
 
 		ratp_state_change(ri, RATP_STATE_FIN_WAIT);
 
-		control = ratp_set_sn(!ri->sn_sent) |
-			ratp_set_an(ri->sn_received + 1) |
+		control = ratp_set_next_sn(ri->sn_sent) |
+			ratp_set_next_an(ri->sn_received) |
 			RATP_CONTROL_FIN | RATP_CONTROL_ACK;
 
 		ratp_create_packet(ri, &fin, control, 0);
-- 
2.13.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 16/16] ratp: user close may happen in SYN-RECEIVED state

[Aleksander Morgado]

The reference says:

     5.2.3. SYN-RECEIVED
     ...
     Departures
       - A CLOSE request is made by the user.  Create a packet with
         FIN set.  Send it and go to the FIN-WAIT state.

Add this missing step.

Probably not a real usecase for barebox anyway as there is no user
triggered close.

Signed-off-by: Aleksander Morgado <aleksander@aleksander.es>
---
 lib/ratp.c             | 2 +-
 scripts/remote/ratp.py | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/ratp.c b/lib/ratp.c
index 46a82c69a..e7fbf640a 100644
--- a/lib/ratp.c
+++ b/lib/ratp.c
@@ -1689,7 +1689,7 @@ void ratp_close(struct ratp *ratp)
 	if (!ri)
 		return;
 
-	if (ri->state == RATP_STATE_ESTABLISHED) {
+	if (ri->state == RATP_STATE_ESTABLISHED || ri->state == RATP_STATE_SYN_RECEIVED) {
 		uint64_t start;
 		u8 control;
 
diff --git a/scripts/remote/ratp.py b/scripts/remote/ratp.py
index 7972d31f2..44f3e2f40 100644
--- a/scripts/remote/ratp.py
+++ b/scripts/remote/ratp.py
@@ -721,7 +721,7 @@ class RatpConnection(object):
     def close(self, timeout=1.0):
         deadline = monotonic() + timeout
         logging.info("CLOSE")
-        if self._state == RatpState.established:
+        if self._state == RatpState.established or self._state == RatpState.syn_received:
             fin = RatpPacket(flags='FA')
             fin.c_sn = (self._s_sn + 1) % 2
             fin.c_an = (self._r_sn + 1) % 2
-- 
2.13.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2 00/16] RATP logic fixes and improvements

[Sascha Hauer]

On Wed, Jun 21, 2017 at 09:13:07PM +0200, Aleksander Morgado wrote:
> Hey Sascha,
> 
> This v2 series of patches includes just changes in the commit
> messages, trying to explain which of the patches apply to barebox
> and/or the barebox<->bbremote interactions. I did test all these
> patches also with RATP FS, and I didn't see any regression.
> 
> I did see issues in RATP FS happening with and without my patches,
> though. E.g. "cat"-ing a long file (>1024 bytes) that is being
> exported via RATFS results in each byte read being sent through the
> ratp output console messages one by one (the actual read operation
> over RATFS goes in chunks of 255, but the cat stdout messages via RATP
> go byte by byte...). After that, the additional read operations above
> offset 1024 end up timing out. I believe I'm going to have to work
> with RATFS in the near future, so I'll likely spend some time looking
> at those issues soon anyway.

Ok, you see RATP FS issues, but they are unrelated to your series. Fine
then, let's leave this for a future task. Applied, thanks

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox