[PATCH] make new y-modem (PATCH v2) work on big-endian CPU

[PATCH] make new y-modem (PATCH v2) work on big-endian CPU

[Antony Pavlov]

apply this commit after that one:

    Author: Robert Jarzmik <robert.jarzmik@free.fr>
    Date:   Sun Nov 4 18:55:23 2012 +0100

        commands: change Y-Modem implementation
---
 lib/xymodem.c |   13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/lib/xymodem.c b/lib/xymodem.c
index 1469a9a..0e82ce2 100644
--- a/lib/xymodem.c
+++ b/lib/xymodem.c
@@ -257,8 +257,8 @@ static ssize_t xy_read_block(struct xyz_ctxt *proto, struct xy_block *blk,
 	uint64_t timeout)
 {
 	ssize_t rc, data_len = 0;
-	unsigned char hdr, seqs[2], crcs[2];
-	int crc = 0, hdr_found = 0;
+	unsigned char hdr, seqs[2];
+	uint16_t crc = 0, hdr_found = 0;
 	uint64_t start = get_time_ns();
 
 	while (!hdr_found) {
@@ -308,12 +308,13 @@ static ssize_t xy_read_block(struct xyz_ctxt *proto, struct xy_block *blk,
 
 	switch (proto->crc_mode) {
 	case CRC_ADD8:
-		rc = xy_gets(proto->cdev, proto->fifo, crcs, 1, timeout);
-		crc = crcs[0];
+		rc = xy_gets(proto->cdev, proto->fifo,
+				(unsigned char *)&crc, 1, timeout);
 		break;
 	case CRC_CRC16:
-		rc = xy_gets(proto->cdev, proto->fifo, crcs, 2, timeout);
-		crc = be16_to_cpu(*(uint16_t *)crcs);
+		rc = xy_gets(proto->cdev, proto->fifo,
+				(unsigned char *)&crc, 2, timeout);
+		crc = be16_to_cpu(crc);
 		break;
 	case CRC_NONE:
 		rc = 0;
-- 
1.7.10.4


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

Re: [PATCH] make new y-modem (PATCH v2) work on big-endian CPU

[Robert Jarzmik]

Antony Pavlov <antonynpavlov@gmail.com> writes:

> apply this commit after that one:
>
>     Author: Robert Jarzmik <robert.jarzmik@free.fr>
>     Date:   Sun Nov 4 18:55:23 2012 +0100
>
>         commands: change Y-Modem implementation
> ---
>  lib/xymodem.c |   13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/lib/xymodem.c b/lib/xymodem.c
> index 1469a9a..0e82ce2 100644
> --- a/lib/xymodem.c
> +++ b/lib/xymodem.c
> @@ -257,8 +257,8 @@ static ssize_t xy_read_block(struct xyz_ctxt *proto, struct xy_block *blk,
>  	uint64_t timeout)
>  {
>  	ssize_t rc, data_len = 0;
> -	unsigned char hdr, seqs[2], crcs[2];
> -	int crc = 0, hdr_found = 0;
> +	unsigned char hdr, seqs[2];
> +	uint16_t crc = 0, hdr_found = 0;
>  	uint64_t start = get_time_ns();
>  
>  	while (!hdr_found) {
> @@ -308,12 +308,13 @@ static ssize_t xy_read_block(struct xyz_ctxt *proto, struct xy_block *blk,
>  
>  	switch (proto->crc_mode) {
>  	case CRC_ADD8:
> -		rc = xy_gets(proto->cdev, proto->fifo, crcs, 1, timeout);
> -		crc = crcs[0];
> +		rc = xy_gets(proto->cdev, proto->fifo,
> +				(unsigned char *)&crc, 1, timeout);
This doesn't look good to me.
In big-endian arch, suppose you read 0xab as the crc.
In that case, won't crc be equal to 0xab00 instead of 0x00ab in this code ? If
that's the case, the previous code was right ...

>  		break;
>  	case CRC_CRC16:
> -		rc = xy_gets(proto->cdev, proto->fifo, crcs, 2, timeout);
> -		crc = be16_to_cpu(*(uint16_t *)crcs);
> +		rc = xy_gets(proto->cdev, proto->fifo,
> +				(unsigned char *)&crc, 2, timeout);
> +		crc = be16_to_cpu(crc);

Does that mean that the code up there doesn't work on big endian ? I'm really
puzzled, as the sender sends in big endian, the xy_gets() receives the bytes
into crcs[2], and the be16_to_cpu() should be a no-op ...

I think that we should do this simpler :
  crc = crcs[0] << 8 + crcs[1]

I'll put that into my github branch for testing.

Cheers.

-- 
Robert

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

Re: [PATCH] make new y-modem (PATCH v2) work on big-endian CPU

[Antony Pavlov]

On 8 November 2012 03:41, Robert Jarzmik <robert.jarzmik@free.fr> wrote:
> Antony Pavlov <antonynpavlov@gmail.com> writes:
>
>> apply this commit after that one:
>>
>>     Author: Robert Jarzmik <robert.jarzmik@free.fr>
>>     Date:   Sun Nov 4 18:55:23 2012 +0100
>>
>>         commands: change Y-Modem implementation
>> ---
>>  lib/xymodem.c |   13 +++++++------
>>  1 file changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/lib/xymodem.c b/lib/xymodem.c
>> index 1469a9a..0e82ce2 100644
>> --- a/lib/xymodem.c
>> +++ b/lib/xymodem.c
>> @@ -257,8 +257,8 @@ static ssize_t xy_read_block(struct xyz_ctxt *proto, struct xy_block *blk,
>>       uint64_t timeout)
>>  {
>>       ssize_t rc, data_len = 0;
>> -     unsigned char hdr, seqs[2], crcs[2];
>> -     int crc = 0, hdr_found = 0;
>> +     unsigned char hdr, seqs[2];
>> +     uint16_t crc = 0, hdr_found = 0;
>>       uint64_t start = get_time_ns();
>>
>>       while (!hdr_found) {
>> @@ -308,12 +308,13 @@ static ssize_t xy_read_block(struct xyz_ctxt *proto, struct xy_block *blk,
>>
>>       switch (proto->crc_mode) {
>>       case CRC_ADD8:
>> -             rc = xy_gets(proto->cdev, proto->fifo, crcs, 1, timeout);
>> -             crc = crcs[0];
>> +             rc = xy_gets(proto->cdev, proto->fifo,
>> +                             (unsigned char *)&crc, 1, timeout);
> This doesn't look good to me.
> In big-endian arch, suppose you read 0xab as the crc.
> In that case, won't crc be equal to 0xab00 instead of 0x00ab in this code ? If
> that's the case, the previous code was right ...

To make the patch I got your old working version and your PATCH v2
branches. I got 'git diff'. I found the key difference and tested the
patch. That is all.

May be the 'proto->crc_mode==CRC_ADD8' condition is always false
during my tests?

>>               break;
>>       case CRC_CRC16:
>> -             rc = xy_gets(proto->cdev, proto->fifo, crcs, 2, timeout);
>> -             crc = be16_to_cpu(*(uint16_t *)crcs);
>> +             rc = xy_gets(proto->cdev, proto->fifo,
>> +                             (unsigned char *)&crc, 2, timeout);
>> +             crc = be16_to_cpu(crc);
>
> Does that mean that the code up there doesn't work on big endian ? I'm really
> puzzled, as the sender sends in big endian, the xy_gets() receives the bytes
> into crcs[2], and the be16_to_cpu() should be a no-op ...

I have no idea just now. I'll try to disassemble generated code.

> I think that we should do this simpler :
>   crc = crcs[0] << 8 + crcs[1]
>
> I'll put that into my github branch for testing.

-- 
Best regards,
  Antony Pavlov

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

Re: [PATCH] make new y-modem (PATCH v2) work on big-endian CPU

[Robert Jarzmik]

Antony Pavlov <antonynpavlov@gmail.com> writes:

> May be the 'proto->crc_mode==CRC_ADD8' condition is always false
> during my tests?
Yes, surely. CRC8 is a fallback when the sender is unable to cope with CRC16. As
you use linux sb, it does cope with CRC16 :)

Cheers.

-- 
Robert

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

Re: [PATCH] make new y-modem (PATCH v2) work on big-endian CPU

[Antony Pavlov]

On 8 November 2012 03:41, Robert Jarzmik <robert.jarzmik@free.fr> wrote:
> Antony Pavlov <antonynpavlov@gmail.com> writes:
>
>> apply this commit after that one:
>>
>>     Author: Robert Jarzmik <robert.jarzmik@free.fr>
>>     Date:   Sun Nov 4 18:55:23 2012 +0100
>>
>>         commands: change Y-Modem implementation
>> ---
>>  lib/xymodem.c |   13 +++++++------
>>  1 file changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/lib/xymodem.c b/lib/xymodem.c
>> index 1469a9a..0e82ce2 100644
>> --- a/lib/xymodem.c
>> +++ b/lib/xymodem.c
>> @@ -257,8 +257,8 @@ static ssize_t xy_read_block(struct xyz_ctxt *proto, struct xy_block *blk,
>>       uint64_t timeout)
>>  {
>>       ssize_t rc, data_len = 0;
>> -     unsigned char hdr, seqs[2], crcs[2];
>> -     int crc = 0, hdr_found = 0;
>> +     unsigned char hdr, seqs[2];
>> +     uint16_t crc = 0, hdr_found = 0;
>>       uint64_t start = get_time_ns();
>>
>>       while (!hdr_found) {
>> @@ -308,12 +308,13 @@ static ssize_t xy_read_block(struct xyz_ctxt *proto, struct xy_block *blk,
>>
>>       switch (proto->crc_mode) {
>>       case CRC_ADD8:
>> -             rc = xy_gets(proto->cdev, proto->fifo, crcs, 1, timeout);
>> -             crc = crcs[0];
>> +             rc = xy_gets(proto->cdev, proto->fifo,
>> +                             (unsigned char *)&crc, 1, timeout);
> This doesn't look good to me.
> In big-endian arch, suppose you read 0xab as the crc.
> In that case, won't crc be equal to 0xab00 instead of 0x00ab in this code ? If
> that's the case, the previous code was right ...
>
>>               break;
>>       case CRC_CRC16:
>> -             rc = xy_gets(proto->cdev, proto->fifo, crcs, 2, timeout);
>> -             crc = be16_to_cpu(*(uint16_t *)crcs);
>> +             rc = xy_gets(proto->cdev, proto->fifo,
>> +                             (unsigned char *)&crc, 2, timeout);
>> +             crc = be16_to_cpu(crc);
>
> Does that mean that the code up there doesn't work on big endian ? I'm really
> puzzled, as the sender sends in big endian, the xy_gets() receives the bytes
> into crcs[2], and the be16_to_cpu() should be a no-op ...
>
> I think that we should do this simpler :
>   crc = crcs[0] << 8 + crcs[1]
>
> I'll put that into my github branch for testing.

I have just tested the commits from your github 'xymodem' branch on my
big-endian and little-endian MIPS boards.

It works perfect!

Tested-by: Antony Pavlov <antonynpavlov@gmail.com>

-- 
Best regards,
  Antony Pavlov

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