No subject

suffix but I miss the actual device or I have devices which firmware
does not have a dfu suffix written to it. :)

Neverless I will test this cases by using your code to check the
firmware files without a device flashing them into and I will also use
the reference suffix implementation to add a suffix to the freerunner
firmware files and then try to flash it to the device. That should
cover most cases for now and if later testing on other devices reveal
some bugs we just need to fix them.

Some small comments about the implementation further below.

> diff --git a/src/dfu_file.c b/src/dfu_file.c
> new file mode 100644
> index 0000000..a2bbe83
> --- /dev/null
> +++ b/src/dfu_file.c
> @@ -0,0 +1,97 @@
> +/*
> + * Checks for and parses a DFU suffix
> + *
> + * (C) 2011 Tormod Volden <debian.tormod at gmail.com>
> + */

Please add the GPL header here. We are using it in all c files but
normally don't care in really small header files.

> +#include <stdio.h>
> +#include <unistd.h>
> +#include <string.h>
> +#include <fcntl.h>
> +#include <sys/stat.h>
> +#include <errno.h>
> +
> +#include "dfu_file.h"
> +
> +/* reads the fd and name member, fills in all others */
> +/* returns 0 if no DFU suffix */
> +/* returns positive if valid DFU suffix */
> +/* returns negative on file read error */

I normally prefer multiline comments just start with /* on the first
line and terminate with */ on the last line. But thats just
nitpicking.

> +int parse_dfu_suffix(struct dfu_file *file)
> +{
> +    int ret;
> +    struct stat st;
> +    /* supported suffices are at least 16 bytes */
> +    unsigned char dfusuffix[16];
> +
> +    file->size = 0;
> +    /* default values, if no valid suffix is found */
> +    file->dwCRC = 0;
> +    file->suffixlen = 0;
> +    file->bcdDFU = 0;
> +    file->idVendor = 0xffff; /* wildcard value */
> +    file->idProduct = 0xffff; /* wildcard value */
> +    file->bcdDevice = 0xffff; /* wildcard value */
> +
> +    ret = fstat(file->fd, &st);
> +    if (ret < 0) {
> +	perror(file->name);
> +	return ret;

Looks to me like the indent is broken here. This happens again and
again below.

> +    }
> +
> +    file->size = st.st_size;
> +    if (file->size < sizeof(dfusuffix)) {
> +	fprintf(stderr, "File too short for DFU suffix\n");
> +	return 0;
> +    }
> +
> +    ret = lseek(file->fd, -sizeof(dfusuffix), SEEK_END);
> +    if (ret < 0) {
> +	fprintf(stderr, "Could not seek to DFU suffix\n");
> +	perror(file->name);
> +	goto rewind;
> +    }
> +
> +    ret = read(file->fd, dfusuffix, sizeof(dfusuffix));
> +    if (ret < 0) {
> +	fprintf(stderr, "Could not read DFU suffix\n");
> +	perror(file->name);
> +	goto rewind;
> +    } else if (ret < sizeof(dfusuffix)) {
> +	fprintf(stderr, "Could not read whole DFU suffix\n");
> +	ret = -EIO;
> +	goto rewind;
> +    }
> +
> +    if (dfusuffix[10] != 'D' ||
> +        dfusuffix[9]  != 'F' ||
> +        dfusuffix[8]  != 'U') {
> +	fprintf(stderr, "No valid DFU suffix signature\n");
> +	ret = 0;
> +	goto rewind;
> +    }
> +
> +    file->dwCRC = (dfusuffix[15] << 24) +
> +		  (dfusuffix[14] << 16) +
> +		  (dfusuffix[13] << 8) +
> +		   dfusuffix[12];
> +
> +    file->bcdDFU = (dfusuffix[7] << 8) + dfusuffix[6];
> +    printf("Dfu suffix version %x\n", file->bcdDFU);
> +
> +    file->suffixlen = dfusuffix[11];
> +    if (file->suffixlen < sizeof(dfusuffix)) {
> +	fprintf(stderr, "Unsupported DFU suffix length %i\n", file->suffixlen);
> +	ret = 0;
> +	goto rewind;
> +    }

I'm wondering if it is worth it to read out the suffix length from the
suffix itself as it has a fixed size in both 1.0 and 1.1. On the other
hand it is provided in the suffix we should read it from there for the
sake of completeness? :)

> +    file->idVendor = (dfusuffix[5] << 8) + dfusuffix[4];
> +    file->idProduct = (dfusuffix[3] << 8) + dfusuffix[2];
> +    file->bcdDevice = (dfusuffix[1] << 8) + dfusuffix[0];
> +
> +rewind:
> +    lseek(file->fd, 0, SEEK_SET);
> +    return ret;
> +}

The major part that is missing here is the check of the CRC sum over
the file. As we discussed earlier I can handle this after this
patchset got merged.

> diff --git a/src/dfu_file.h b/src/dfu_file.h
> new file mode 100644
> index 0000000..8c33665
> --- /dev/null
> +++ b/src/dfu_file.h
> @@ -0,0 +1,20 @@
> +
> +#ifndef _DFU_FILE_H
> +#define _DFU_FILE_H
> +
> +struct dfu_file {
> +    const char *name;
> +    int fd;
> +    off_t size;
> +    /* From DFU suffix fields */
> +    u_int32_t dwCRC;
> +    unsigned char suffixlen;
> +    u_int16_t bcdDFU;
> +    u_int16_t idVendor;
> +    u_int16_t idProduct;
> +    u_int16_t bcdDevice;
> +};
> +
> +int parse_dfu_suffix(struct dfu_file *file);
> +
> +#endif
> diff --git a/src/main.c b/src/main.c
> index e72222e..5e27fc7 100644
> --- a/src/main.c
> +++ b/src/main.c
> @@ -32,6 +32,7 @@
>  
>  #include "dfu.h"
>  #include "usb_dfu.h"
> +#include "dfu_file.h"
>  #include "dfu_load.h"
>  #include "quirks.h"
>  #ifdef HAVE_CONFIG_H
> @@ -464,8 +465,7 @@ int main(int argc, char **argv)
>  	struct dfu_status status;
>  	struct usb_dfu_func_descriptor func_dfu;
>  	libusb_context *ctx;
> -	char *filename = NULL;
> -	int fd;
> +	struct dfu_file file;
>  	char *alt_name = NULL; /* query alt name if non-NULL */
>  	char *end;
>  	int final_reset = 0;
> @@ -478,6 +478,7 @@ int main(int argc, char **argv)
>  
>  	host_page_size = getpagesize();
>  	memset(dif, 0, sizeof(*dif));
> +	file.name = NULL;
>  
>  	ret = libusb_init(&ctx);
>  	if (ret) {
> @@ -557,11 +558,11 @@ int main(int argc, char **argv)
>  			break;
>  		case 'U':
>  			mode = MODE_UPLOAD;
> -			filename = optarg;
> +			file.name = optarg;
>  			break;
>  		case 'D':
>  			mode = MODE_DOWNLOAD;
> -			filename = optarg;
> +			file.name = optarg;
>  			break;
>  		case 'R':
>  			final_reset = 1;
> @@ -578,7 +579,7 @@ int main(int argc, char **argv)
>  		exit(2);
>  	}
>  
> -	if (!filename) {
> +	if (!file.name) {
>  		fprintf(stderr, "You need to specify a filename to -D or -U\n");
>  		help();
>  		exit(2);
> @@ -869,24 +870,44 @@ status_again:
>  
>  	switch (mode) {
>  	case MODE_UPLOAD:
> -		fd = open(filename, O_WRONLY|O_CREAT|O_EXCL, 0644);
> -		if (fd < 0) {
> -			perror(filename);
> +		file.fd = open(file.name, O_WRONLY|O_CREAT|O_EXCL, 0644);
> +		if (file.fd < 0) {
> +			perror(file.name);
>  			exit(1);
>  		}
> -		if (dfuload_do_upload(dif, transfer_size, fd) < 0)
> +		if (dfuload_do_upload(dif, transfer_size, file.fd) < 0)
>  			exit(1);
> -		close(fd);
> +		close(file.fd);
>  		break;
>  	case MODE_DOWNLOAD:
> -		fd = open(filename, O_RDONLY|O_BINARY);
> -		if (fd < 0) {
> -			perror(filename);
> +		file.fd = open(file.name, O_RDONLY|O_BINARY);
> +		if (file.fd < 0) {
> +			perror(file.name);
>  			exit(1);
>  		}
> -		if (dfuload_do_dnload(dif, transfer_size, fd) < 0)
> +		ret = parse_dfu_suffix(&file);
> +		if (ret < 0) {
> +			exit(1);
> +		} else if (ret == 0) {
> +			fprintf(stderr, "Warning: File has no DFU suffix\n");
> +		} else if (file.bcdDFU != 0x0100) {
> +			fprintf(stderr, "Unsupported DFU file revision "
> +				"%04x\n", file.bcdDFU);
> +			exit(1);
> +		}
> +		if (file.idVendor != 0xffff &&
> +		    dif->vendor != file.idVendor) {
> +			fprintf(stderr, "Warning: File vendor ID %04x does "
> +				"not match device\n", file.idVendor);
> +		}
> +		if (file.idProduct != 0xffff &&
> +		    dif->product != file.idProduct) {
> +			fprintf(stderr, "Warning: File product ID %04x does "
> +				"not match device\n", file.idProduct);
> +		}
> +		if (dfuload_do_dnload(dif, transfer_size, file.fd) < 0)
>  			exit(1);
> -		close(fd);
> +		close(file.fd);
>  		break;
>  	default:
>  		fprintf(stderr, "Unsupported mode: %u\n", mode);

The rest looks good.

regards
Stefan Schmidt