[PATCH sunxi-tools] fel: support selection of specific USB bus and device number

[Bernhard Nortmann]

See https://github.com/linux-sunxi/sunxi-tools/issues/37

The patch introduces a "--dev" (-d) option to specify the
desired FEL device. This is useful if multiple target devices
are connected to the same host.
---
fel.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----------
1 file changed, 68 insertions(+), 13 deletions(-)

diff --git a/fel.c b/fel.c
index 59f0f72..0c69d80 100644
--- a/fel.c
+++ b/fel.c
@@ -1270,12 +1270,56 @@ static unsigned int file_upload(libusb_device_handle *handle, size_t count,
return i; // return number of files that were processed
}

+/* open libusb handle to desired FEL device */
+static void request_libusb_handle(libusb_device_handle **handle,
+ int busnum, int devnum, uint16_t vendor_id, uint16_t product_id)
+{
+ if (busnum < 0 || devnum < 0)
+ // With the default values (busnum -1, devnum -1) we don't care
+ // for a specific USB device; so let libusb open the first
+ // device that matches VID/PID.
+ *handle = libusb_open_device_with_vid_pid(NULL, vendor_id, product_id);
+ else {
+ // look for specific bus and device number
+ pr_info("Selecting USB Bus %03d Device %03d\n", busnum, devnum);
+ libusb_device **list;
+ size_t ndevs, i;
+ bool found = false;
+
+ ndevs = libusb_get_device_list(NULL, &list);
+ for (i = 0; i < ndevs; i++)
+ if (libusb_get_bus_number(list[i]) == busnum
+ && libusb_get_device_address(list[i]) == devnum) {
+ found = true; // bus:devnum matched
+ struct libusb_device_descriptor desc;
+ libusb_get_device_descriptor(list[i], &desc);
+ if (desc.idVendor != vendor_id
+ || desc.idProduct != product_id) {
+ fprintf(stderr, "ERROR: Bus %03d Device %03d not a FEL device (expected %04x:%04x, got %04x:%04x)\n",
+ busnum, devnum, vendor_id, product_id, desc.idVendor, desc.idProduct);
+ exit(1);
+ }
+ // open handle to this specific device (incrementing its refcount)
+ libusb_open(list[i], handle);
+ break;
+ }
+ libusb_free_device_list(list, true);
+
+ if (!found) {
+ fprintf(stderr, "ERROR: Bus %03d Device %03d not found in libusb device list\n",
+ busnum, devnum);
+ exit(1);
+ }
+ }
+}
+
int main(int argc, char **argv)
{
bool uboot_autostart = false; /* flag for "uboot" command = U-Boot autostart */
bool pflag_active = false; /* -p switch, causing "write" to output progress */
int rc;
libusb_device_handle *handle = NULL;
+ int busnum = -1, devnum = -1;
int iface_detached = -1;
rc = libusb_init(NULL);
assert(rc == 0);
@@ -1284,6 +1328,7 @@ int main(int argc, char **argv)
printf("Usage: %s [options] command arguments... [command...]\n"
" -v, --verbose Verbose logging\n"
" -p, --progress \"write\" transfers show a progress bar\n"
+ " -d, --dev bus:devnum Use specific USB bus and device number\n"
"\n"
" spl file Load and execute U-Boot SPL\n"
" If file additionally contains a main U-Boot binary\n"
@@ -1316,7 +1361,29 @@ int main(int argc, char **argv)
);
}

- handle = libusb_open_device_with_vid_pid(NULL, 0x1f3a, 0xefe8);
+ /* process all "prefix"-type arguments first */
+ while (argc > 1) {
+ if (strcmp(argv[1], "--verbose") == 0 || strcmp(argv[1], "-v") == 0)
+ verbose = true;
+ else if (strcmp(argv[1], "--progress") == 0 || strcmp(argv[1], "-p") == 0)
+ pflag_active = true;
+ else if (strncmp(argv[1], "--dev", 5) == 0 || strncmp(argv[1], "-d", 2) == 0) {
+ char *dev = argv[1];
+ dev += strspn(argv[1], "-dev="); // skip option chars, ignore '='
+ if (*dev == 0) { // at end of argument, use the next one instead
+ dev = argv[2];
+ argc -= 1;
+ argv += 1;
+ }
+ busnum = strtoul(dev, &dev, 10);
+ devnum = strtoul(dev + 1, NULL, 10);
+ } else
+ break; /* no valid (prefix) option detected, exit loop */
+ argc -= 1;
+ argv += 1;
+ }
+
+ request_libusb_handle(&handle, busnum, devnum, 0x1f3a, 0xefe8);
if (!handle) {
switch (errno) {
case EACCES:
@@ -1343,18 +1410,6 @@ int main(int argc, char **argv)
exit(1);
}

- /* process all "prefix"-type arguments first */
- while (argc > 1) {
- if (strcmp(argv[1], "--verbose") == 0 || strcmp(argv[1], "-v") == 0)
- verbose = true;
- else if (strcmp(argv[1], "--progress") == 0 || strcmp(argv[1], "-p") == 0)
- pflag_active = true;
- else
- break; /* no valid (prefix) option detected, exit loop */
- argc -= 1;
- argv += 1;
- }
-
while (argc > 1 ) {
int skip = 1;

--
2.4.10

[Bernhard Nortmann]

See https://github.com/linux-sunxi/sunxi-tools/issues/37

The patch introduces a "--dev" (-d) option to specify the
desired FEL device. This is useful if multiple target devices
are connected to the same host.
---
fel.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----------
1 file changed, 68 insertions(+), 13 deletions(-)

diff --git a/fel.c b/fel.c

index 59f0f72..3de9926 100644

+ char *dev_arg = argv[1];
+ dev_arg += strspn(dev_arg, "-dev="); // skip option chars, ignore '='
+ if (*dev_arg == 0) { // at end of argument, use the next one instead
+ dev_arg = argv[2];

+ argc -= 1;
+ argv += 1;
+ }

+ busnum = strtoul(dev_arg, &dev_arg, 10);
+ devnum = strtoul(dev_arg + 1, NULL, 10);

[Bernhard Nortmann]

Changes in v2:
- Renamed "char *dev" to "char *dev_arg", to make it clearer that we're working
on the device argument. Use strspn(dev_arg,...) instead of strspn(argv[1],...)

Bernhard Nortmann (1):
fel: support selection of specific USB bus and device number

fel.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----------
1 file changed, 68 insertions(+), 13 deletions(-)

--
2.4.10

[Bernhard Nortmann]

Changes in v2:
- Renamed "char *dev" to "char *dev_arg", to make it clearer that we're working
on the device argument. Use strspn(dev_arg,...) instead of strspn(argv[1],...)

Changes in v3:
- Protect against segfault when trying to access non-existant argv[2]
- Moved "handle == NULL" error handler from main() to request_libusb_handle()
- Add missing "Signed-off-by:"

Bernhard Nortmann (1):
fel: support selection of specific USB bus and device number

fel.c | 101 +++++++++++++++++++++++++++++++++++++++++++++++++++---------------
1 file changed, 78 insertions(+), 23 deletions(-)

--
2.4.10

[Bernhard Nortmann]

See https://github.com/linux-sunxi/sunxi-tools/issues/37

The patch introduces a "--dev" (-d) option to specify the
desired FEL device. This is useful if multiple target devices
are connected to the same host.

Signed-off-by: Bernhard Nortmann <bernhard...@web.de>
---

fel.c | 101 +++++++++++++++++++++++++++++++++++++++++++++++++++---------------
1 file changed, 78 insertions(+), 23 deletions(-)

diff --git a/fel.c b/fel.c
index 59f0f72..3e6c6ae 100644
--- a/fel.c
+++ b/fel.c
@@ -1270,12 +1270,67 @@ static unsigned int file_upload(libusb_device_handle *handle, size_t count,

+ if (!*handle) {
+ switch (errno) {
+ case EACCES:
+ fprintf(stderr, "ERROR: You don't have permission to access Allwinner USB FEL device\n");
+ break;
+ default:
+ fprintf(stderr, "ERROR: Allwinner USB FEL device not found!\n");
+ break;
+ }

+ exit(1);
+ }
+}
+

int main(int argc, char **argv)
{
bool uboot_autostart = false; /* flag for "uboot" command = U-Boot autostart */
bool pflag_active = false; /* -p switch, causing "write" to output progress */
int rc;
libusb_device_handle *handle = NULL;
+ int busnum = -1, devnum = -1;
int iface_detached = -1;
rc = libusb_init(NULL);
assert(rc == 0);

@@ -1284,6 +1339,7 @@ int main(int argc, char **argv)

printf("Usage: %s [options] command arguments... [command...]\n"
" -v, --verbose Verbose logging\n"
" -p, --progress \"write\" transfers show a progress bar\n"
+ " -d, --dev bus:devnum Use specific USB bus and device number\n"
"\n"
" spl file Load and execute U-Boot SPL\n"
" If file additionally contains a main U-Boot binary\n"

@@ -1316,18 +1372,29 @@ int main(int argc, char **argv)

);
}

- handle = libusb_open_device_with_vid_pid(NULL, 0x1f3a, 0xefe8);

- if (!handle) {
- switch (errno) {
- case EACCES:
- fprintf(stderr, "ERROR: You don't have permission to access Allwinner USB FEL device\n");
- break;
- default:
- fprintf(stderr, "ERROR: Allwinner USB FEL device not found!\n");
- break;
- }
- exit(1);

+ /* process all "prefix"-type arguments first */
+ while (argc > 1) {
+ if (strcmp(argv[1], "--verbose") == 0 || strcmp(argv[1], "-v") == 0)
+ verbose = true;
+ else if (strcmp(argv[1], "--progress") == 0 || strcmp(argv[1], "-p") == 0)
+ pflag_active = true;
+ else if (strncmp(argv[1], "--dev", 5) == 0 || strncmp(argv[1], "-d", 2) == 0) {

+ char *dev_arg = argv[1];

+ dev_arg += strspn(dev_arg, "-dev="); // skip option chars, ignore '='
+ if (*dev_arg == 0 && argc > 2) { // at end of argument, use the next one instead
+ dev_arg = argv[2];

+ argc -= 1;
+ argv += 1;
+ }

+ busnum = strtoul(dev_arg, &dev_arg, 10);

+ devnum = strtoul(dev_arg + 1, NULL, 10);

+ } else
+ break; /* no valid (prefix) option detected, exit loop */
+ argc -= 1;
+ argv += 1;
}
+
+ request_libusb_handle(&handle, busnum, devnum, 0x1f3a, 0xefe8);

rc = libusb_claim_interface(handle, 0);
#if defined(__linux__)
if (rc != LIBUSB_SUCCESS) {

[Siarhei Siamashka]

Hello Bernhard,

Thanks, this is clearly a useful feature.

On Wed, 16 Mar 2016 16:43:18 +0100
Bernhard Nortmann <bernhard...@web.de> wrote:

> See https://github.com/linux-sunxi/sunxi-tools/issues/37
>
> The patch introduces a "--dev" (-d) option to specify the
> desired FEL device. This is useful if multiple target devices
> are connected to the same host.
>
> Signed-off-by: Bernhard Nortmann <bernhard...@web.de>

Should we also give more explicit credit in the commit message
to NextThingCo people for their work on the initial implementation
of this patch? Yes, I know that the github tracker link has all
the necessary details, but still we could be a bit nicer.

I have added Wynter Woods to CC.

> fel.c | 101 +++++++++++++++++++++++++++++++++++++++++++++++++++---------------
> 1 file changed, 78 insertions(+), 23 deletions(-)
>
> diff --git a/fel.c b/fel.c
> index 59f0f72..3e6c6ae 100644
> --- a/fel.c
> +++ b/fel.c
> @@ -1270,12 +1270,67 @@ static unsigned int file_upload(libusb_device_handle *handle, size_t count,
> return i; // return number of files that were processed
> }
>
> +/* open libusb handle to desired FEL device */
> +static void request_libusb_handle(libusb_device_handle **handle,
> + int busnum, int devnum, uint16_t vendor_id, uint16_t product_id)
> +{
> + if (busnum < 0 || devnum < 0)
> + // With the default values (busnum -1, devnum -1) we don't care
> + // for a specific USB device; so let libusb open the first
> + // device that matches VID/PID.
> + *handle = libusb_open_device_with_vid_pid(NULL, vendor_id, product_id);

Some style nitpicks here:

1) Mixing the C++ '//' comments with the C '/* */' comments does not
look very consistent. Probably it's best to stick to a single style
across the whole source file.

2) https://www.kernel.org/doc/Documentation/CodingStyle

Do not unnecessarily use braces where a single statement will do.

if (condition)
action();

and

if (condition)
do_this();
else
do_that();

This does not apply if only one branch of a conditional statement is a single
statement; in the latter case use braces in both branches:

if (condition) {
do_this();
do_that();
} else {
otherwise();

}

> + else {
> + // look for specific bus and device number
> + pr_info("Selecting USB Bus %03d Device %03d\n", busnum, devnum);
> + libusb_device **list;
> + size_t ndevs, i;
> + bool found = false;
> +
> + ndevs = libusb_get_device_list(NULL, &list);
> + for (i = 0; i < ndevs; i++)

A style nitpick again. It would be probably better to add {} around
the loop body here because the loop body is not just a single line
statement.

> + if (libusb_get_bus_number(list[i]) == busnum
> + && libusb_get_device_address(list[i]) == devnum) {
> + found = true; // bus:devnum matched
> + struct libusb_device_descriptor desc;
> + libusb_get_device_descriptor(list[i], &desc);
> + if (desc.idVendor != vendor_id
> + || desc.idProduct != product_id) {
> + fprintf(stderr, "ERROR: Bus %03d Device %03d not a FEL device (expected %04x:%04x, got %04x:%04x)\n",

I'm not too keen on strictly enforcing the 80 characters line
limit everywhere, but this particular line can be easily made
shorter:

fprintf(stderr, "ERROR: Bus %03d Device %03d not a FEL device "
"(expected %04x:%04x, got %04x:%04x)\n",

...);

> + busnum, devnum, vendor_id, product_id, desc.idVendor, desc.idProduct);
> + exit(1);
> + }
> + // open handle to this specific device (incrementing its refcount)
> + libusb_open(list[i], handle);

Are we sure that the *handle variable is set to NULL if libusb_open()
call fails? The documentation at
http://libusb.sourceforge.net/api-1.0/group__dev.html#ga8163100afdf933fabed0db7fa81c89d1
says
"handle - output location for the returned device handle
pointer. Only populated when the return code is 0"

> + break;
> + }
> + libusb_free_device_list(list, true);
> +
> + if (!found) {
> + fprintf(stderr, "ERROR: Bus %03d Device %03d not found in libusb device list\n",
> + busnum, devnum);
> + exit(1);
> + }
> + }
> + if (!*handle) {
> + switch (errno) {

Are we sure that the 'errno' has not been overwritten by something
else by now? The http://man7.org/linux/man-pages/man3/errno.3.html
says:

A common mistake is to do

if (somecall() == -1) {
printf("somecall() failed\n");
if (errno == ...) { ... }
}

where errno no longer needs to have the value it had upon return from
somecall() (i.e., it may have been changed by the printf(3)). If the
value of errno should be preserved across a library call, it must be
saved:

if (somecall() == -1) {
int errsv = errno;
printf("somecall() failed\n");
if (errsv == ...) { ... }

What if the 'dev_arg' string is malformed and contains only a single
decimal number without the ':' separator? It is probably best to
rewrite this as

if (sscanf(dev_arg, "%u:%u", &busnum, &devnum) != 2) {
fprintf(stderr, "ERROR: Expected 'bus:devnum', got '%s'.\n", dev_arg);
exit(1);

Best regards,
Siarhei Siamashka

[Bernhard Nortmann]

Hi Siarhei!

Am 16.03.2016 um 17:58 schrieb Siarhei Siamashka:
> Hello Bernhard,
>
> Thanks, this is clearly a useful feature.
>

> [...]

>
> Should we also give more explicit credit in the commit message
> to NextThingCo people for their work on the initial implementation
> of this patch? Yes, I know that the github tracker link has all
> the necessary details, but still we could be a bit nicer.
>
> I have added Wynter Woods to CC.

Agreed. While the functionality has been refactored extensively, the
patch was originally inspired by their work. But we also should
encourage people to contribute additions like this back to the
sunxi-tools repo or the mailing list...

> Are we sure that the *handle variable is set to NULL if libusb_open()
> call fails? The documentation at
> http://libusb.sourceforge.net/api-1.0/group__dev.html#ga8163100afdf933fabed0db7fa81c89d1
> says
> "handle - output location for the returned device handle
> pointer. Only populated when the return code is 0"

> Are we sure that the 'errno' has not been overwritten by something
> else by now? The http://man7.org/linux/man-pages/man3/errno.3.html
> says:
>
> A common mistake is to do
>
> if (somecall() == -1) {
> printf("somecall() failed\n");
> if (errno == ...) { ... }
> }
>
> where errno no longer needs to have the value it had upon return from
> somecall() (i.e., it may have been changed by the printf(3)). If the
> value of errno should be preserved across a library call, it must be
> saved:
>
> if (somecall() == -1) {
> int errsv = errno;
> printf("somecall() failed\n");
> if (errsv == ...) { ... }
> }

You're right. I have relocated the "result == NULL" case back right
after the libusb_open_device_with_vid_pid() call where it originated
from, and now handle libusb_get_device_list() and libusb_open() failures
separately.

> What if the 'dev_arg' string is malformed and contains only a single
> decimal number without the ':' separator? It is probably best to
> rewrite this as
>
> if (sscanf(dev_arg, "%u:%u", &busnum, &devnum) != 2) {
> fprintf(stderr, "ERROR: Expected 'bus:devnum', got '%s'.\n", dev_arg);
> exit(1);
> }

With the stroul() I'd expect one or both variables to end up as 0, which
would never match a valid USB bus:devnum combination (so we'd bail out
with a "ERROR: Bus %03d Device %03d not found in libusb device list"
message).

However, your solution is probably cleaner. Would it be safe to use "%u"
on the signed variables? For now I have preferred "%d" instead...

I'll be submitting a v4 shortly, which also addresses various style
issues you pointed out.

Regards, B. Nortmann

[Bernhard Nortmann]

See https://github.com/linux-sunxi/sunxi-tools/issues/37

The patch was originally inspired by
https://github.com/NextThingCo/sunxi-tools/commit/16386a7
and
https://github.com/NextThingCo/sunxi-tools/commit/47bafaf

It introduces a "--dev" (-d) option to specify the desired FEL

device. This is useful if multiple target devices are connected
to the same host.

Signed-off-by: Bernhard Nortmann <bernhard...@web.de>
---

fel.c | 122 +++++++++++++++++++++++++++++++++++++++++++++++++++++-------------
1 file changed, 98 insertions(+), 24 deletions(-)

diff --git a/fel.c b/fel.c
index 59f0f72..807c008 100644
--- a/fel.c
+++ b/fel.c
@@ -1270,12 +1270,83 @@ static unsigned int file_upload(libusb_device_handle *handle, size_t count,

return i; // return number of files that were processed
}

+/* open libusb handle to desired FEL device */

+libusb_device_handle *libusb_open_device(int busnum, int devnum,
+ uint16_t vendor_id, uint16_t product_id)
+{
+ libusb_device_handle *result = NULL;

+
+ if (busnum < 0 || devnum < 0) {

+ /* With the default values (busnum -1, devnum -1) we don't care
+ * for a specific USB device; so let libusb open the first
+ * device that matches VID/PID.
+ */
+ result = libusb_open_device_with_vid_pid(NULL, vendor_id, product_id);
+ if (!result) {
+ switch (errno) {

+ case EACCES:
+ fprintf(stderr, "ERROR: You don't have permission to access Allwinner USB FEL device\n");
+ break;
+ default:
+ fprintf(stderr, "ERROR: Allwinner USB FEL device not found!\n");
+ break;
+ }
+ exit(1);
+ }

+ return result;
+ }
+
+ /* look for specific bus and device number */

+ pr_info("Selecting USB Bus %03d Device %03d\n", busnum, devnum);

+ bool found = false;

+ ssize_t rc, i;
+ libusb_device **list;
+
+ rc = libusb_get_device_list(NULL, &list);
+ if (rc < 0) {
+ fprintf(stderr, "libusb_get_device_list() ERROR: %s\n",
+ libusb_strerror(rc));
+ exit(1);
+ }
+ for (i = 0; i < rc; i++) {

+ if (libusb_get_bus_number(list[i]) == busnum
+ && libusb_get_device_address(list[i]) == devnum) {

+ found = true; /* bus:devnum matched */

+ struct libusb_device_descriptor desc;
+ libusb_get_device_descriptor(list[i], &desc);
+ if (desc.idVendor != vendor_id
+ || desc.idProduct != product_id) {
+ fprintf(stderr, "ERROR: Bus %03d Device %03d not a FEL device "

+ "(expected %04x:%04x, got %04x:%04x)\n", busnum, devnum,
+ vendor_id, product_id, desc.idVendor, desc.idProduct);
+ exit(1);
+ }
+ /* open handle to this specific device (incrementing its refcount) */
+ rc = libusb_open(list[i], &result);
+ if (rc != 0) {
+ fprintf(stderr, "libusb_open() ERROR: %s\n",
+ libusb_strerror(rc));
+ exit(1);
+ }
+ break;
+ }

+ }
+ libusb_free_device_list(list, true);
+
+ if (!found) {
+ fprintf(stderr, "ERROR: Bus %03d Device %03d not found in libusb device list\n",
+ busnum, devnum);
+ exit(1);
+ }

+ return result;

+}
+
int main(int argc, char **argv)
{
bool uboot_autostart = false; /* flag for "uboot" command = U-Boot autostart */
bool pflag_active = false; /* -p switch, causing "write" to output progress */
int rc;

- libusb_device_handle *handle = NULL;

+ int busnum = -1, devnum = -1;
int iface_detached = -1;
rc = libusb_init(NULL);
assert(rc == 0);

@@ -1284,6 +1355,7 @@ int main(int argc, char **argv)

printf("Usage: %s [options] command arguments... [command...]\n"
" -v, --verbose Verbose logging\n"
" -p, --progress \"write\" transfers show a progress bar\n"
+ " -d, --dev bus:devnum Use specific USB bus and device number\n"
"\n"
" spl file Load and execute U-Boot SPL\n"
" If file additionally contains a main U-Boot binary\n"

@@ -1316,18 +1388,32 @@ int main(int argc, char **argv)

);
}

- handle = libusb_open_device_with_vid_pid(NULL, 0x1f3a, 0xefe8);

- if (!handle) {
- switch (errno) {
- case EACCES:
- fprintf(stderr, "ERROR: You don't have permission to access Allwinner USB FEL device\n");
- break;
- default:
- fprintf(stderr, "ERROR: Allwinner USB FEL device not found!\n");
- break;
- }
- exit(1);

+ /* process all "prefix"-type arguments first */
+ while (argc > 1) {
+ if (strcmp(argv[1], "--verbose") == 0 || strcmp(argv[1], "-v") == 0)
+ verbose = true;
+ else if (strcmp(argv[1], "--progress") == 0 || strcmp(argv[1], "-p") == 0)
+ pflag_active = true;
+ else if (strncmp(argv[1], "--dev", 5) == 0 || strncmp(argv[1], "-d", 2) == 0) {

+ char *dev_arg = argv[1];

+ dev_arg += strspn(dev_arg, "-dev="); /* skip option chars, ignore '=' */
+ if (*dev_arg == 0 && argc > 2) { /* at end of argument, use the next one instead */
+ dev_arg = argv[2];

+ argc -= 1;
+ argv += 1;
+ }

+ if (sscanf(dev_arg, "%d:%d", &busnum, &devnum) != 2) {
+ fprintf(stderr, "ERROR: Expected 'bus:devnum', got '%s'.\n", dev_arg);
+ exit(1);
+ }

+ } else
+ break; /* no valid (prefix) option detected, exit loop */
+ argc -= 1;
+ argv += 1;
}
+

+ libusb_device_handle *handle = libusb_open_device(busnum, devnum, 0x1f3a, 0xefe8);
+ assert(handle != NULL);

rc = libusb_claim_interface(handle, 0);
#if defined(__linux__)
if (rc != LIBUSB_SUCCESS) {

@@ -1343,18 +1429,6 @@ int main(int argc, char **argv)

[Bernhard Nortmann]

Changes in v2:
- Renamed "char *dev" to "char *dev_arg", to make it clearer that we're working
on the device argument. Use strspn(dev_arg,...) instead of strspn(argv[1],...)

Changes in v3:
- Protect against segfault when trying to access non-existant argv[2]
- Moved "handle == NULL" error handler from main() to request_libusb_handle()
- Add missing "Signed-off-by:"

Changes in v4:
- Reworked commit message
- Handle possible libusb errors more consistently
- Be more strict on parsing "bus:devnum" device specifier
- Fixed various style issues

Bernhard Nortmann (1):
fel: support selection of specific USB bus and device number

fel.c | 122 +++++++++++++++++++++++++++++++++++++++++++++++++++++-------------
1 file changed, 98 insertions(+), 24 deletions(-)

--
2.4.10

[Bernhard Nortmann]

See https://github.com/linux-sunxi/sunxi-tools/issues/37

The patch was originally inspired by
https://github.com/NextThingCo/sunxi-tools/commit/16386a7
and
https://github.com/NextThingCo/sunxi-tools/commit/47bafaf

It introduces a "--dev" (-d) option to specify the desired FEL

device. This is useful if multiple target devices are connected
to the same host.

Signed-off-by: Bernhard Nortmann <bernhard...@web.de>
---

fel.c | 129 ++++++++++++++++++++++++++++++++++++++++++++++++++++--------------
1 file changed, 103 insertions(+), 26 deletions(-)

diff --git a/fel.c b/fel.c
index 59f0f72..dc27ecc 100644
--- a/fel.c
+++ b/fel.c
@@ -37,6 +37,9 @@
#include "endian_compat.h"
#include "progress.h"

+static const uint16_t AW_USB_VENDOR_ID = 0x1F3A;
+static const uint16_t AW_USB_PRODUCT_ID = 0XEFE8;
+
struct aw_usb_request {
char signature[8];
uint32_t length;
@@ -1270,20 +1273,92 @@ static unsigned int file_upload(libusb_device_handle *handle, size_t count,

return i; // return number of files that were processed
}

+/* open libusb handle to desired FEL device */

+static libusb_device_handle *open_fel_device(int busnum, int devnum,
+ uint16_t vendor_id, uint16_t product_id)
+{
+ libusb_device_handle *result = NULL;

+
+ if (busnum < 0 || devnum < 0) {

+ /* With the default values (busnum -1, devnum -1) we don't care
+ * for a specific USB device; so let libusb open the first
+ * device that matches VID/PID.
+ */
+ result = libusb_open_device_with_vid_pid(NULL, vendor_id, product_id);
+ if (!result) {
+ switch (errno) {
+ case EACCES:
+ fprintf(stderr, "ERROR: You don't have permission to access Allwinner USB FEL device\n");
+ break;
+ default:
+ fprintf(stderr, "ERROR: Allwinner USB FEL device not found!\n");
+ break;
+ }
+ exit(1);
+ }
+ return result;
+ }
+

+ /* look for specific bus and device number */

+ pr_info("Selecting USB Bus %03d Device %03d\n", busnum, devnum);

+ bool found = false;

+ ssize_t rc, i;
+ libusb_device **list;
+
+ rc = libusb_get_device_list(NULL, &list);
+ if (rc < 0) {
+ fprintf(stderr, "libusb_get_device_list() ERROR: %s\n",
+ libusb_strerror(rc));
+ exit(1);
+ }

+ for (i = 0; i < rc; i++) {

+ if (libusb_get_bus_number(list[i]) == busnum
+ && libusb_get_device_address(list[i]) == devnum) {

+ found = true; /* bus:devnum matched */

+ struct libusb_device_descriptor desc;
+ libusb_get_device_descriptor(list[i], &desc);
+ if (desc.idVendor != vendor_id
+ || desc.idProduct != product_id) {
+ fprintf(stderr, "ERROR: Bus %03d Device %03d not a FEL device "

+ "(expected %04x:%04x, got %04x:%04x)\n", busnum, devnum,
+ vendor_id, product_id, desc.idVendor, desc.idProduct);
+ exit(1);
+ }

+ /* open handle to this specific device (incrementing its refcount) */
+ rc = libusb_open(list[i], &result);
+ if (rc != 0) {
+ fprintf(stderr, "libusb_open() ERROR: %s\n",
+ libusb_strerror(rc));
+ exit(1);
+ }
+ break;
+ }

+ }
+ libusb_free_device_list(list, true);
+
+ if (!found) {
+ fprintf(stderr, "ERROR: Bus %03d Device %03d not found in libusb device list\n",
+ busnum, devnum);
+ exit(1);
+ }

+ return result;

+}
+
int main(int argc, char **argv)
{
bool uboot_autostart = false; /* flag for "uboot" command = U-Boot autostart */
bool pflag_active = false; /* -p switch, causing "write" to output progress */

- int rc;
- libusb_device_handle *handle = NULL;
+ libusb_device_handle *handle;

+ int busnum = -1, devnum = -1;
int iface_detached = -1;

- rc = libusb_init(NULL);
+ int rc = libusb_init(NULL);
assert(rc == 0);

if (argc <= 1) {

printf("Usage: %s [options] command arguments... [command...]\n"
" -v, --verbose Verbose logging\n"
" -p, --progress \"write\" transfers show a progress bar\n"
+ " -d, --dev bus:devnum Use specific USB bus and device number\n"
"\n"
" spl file Load and execute U-Boot SPL\n"
" If file additionally contains a main U-Boot binary\n"

@@ -1316,18 +1391,32 @@ int main(int argc, char **argv)

);
}

- handle = libusb_open_device_with_vid_pid(NULL, 0x1f3a, 0xefe8);

- if (!handle) {
- switch (errno) {
- case EACCES:
- fprintf(stderr, "ERROR: You don't have permission to access Allwinner USB FEL device\n");
- break;
- default:
- fprintf(stderr, "ERROR: Allwinner USB FEL device not found!\n");
- break;
- }
- exit(1);

+ /* process all "prefix"-type arguments first */
+ while (argc > 1) {
+ if (strcmp(argv[1], "--verbose") == 0 || strcmp(argv[1], "-v") == 0)
+ verbose = true;
+ else if (strcmp(argv[1], "--progress") == 0 || strcmp(argv[1], "-p") == 0)
+ pflag_active = true;
+ else if (strncmp(argv[1], "--dev", 5) == 0 || strncmp(argv[1], "-d", 2) == 0) {

+ char *dev_arg = argv[1];
+ dev_arg += strspn(dev_arg, "-dev="); /* skip option chars, ignore '=' */

+ if (*dev_arg == 0 && argc > 2) { /* at end of argument, use the next one instead */
+ dev_arg = argv[2];

+ argc -= 1;
+ argv += 1;
+ }

+ if (sscanf(dev_arg, "%d:%d", &busnum, &devnum) != 2) {
+ fprintf(stderr, "ERROR: Expected 'bus:devnum', got '%s'.\n", dev_arg);
+ exit(1);
+ }

+ } else
+ break; /* no valid (prefix) option detected, exit loop */
+ argc -= 1;
+ argv += 1;
}
+

+ handle = open_fel_device(busnum, devnum, AW_USB_VENDOR_ID, AW_USB_PRODUCT_ID);

+ assert(handle != NULL);
rc = libusb_claim_interface(handle, 0);
#if defined(__linux__)
if (rc != LIBUSB_SUCCESS) {

@@ -1343,18 +1432,6 @@ int main(int argc, char **argv)

[Bernhard Nortmann]

Changes in v2:
- Renamed "char *dev" to "char *dev_arg", to make it clearer that we're working
on the device argument. Use strspn(dev_arg,...) instead of strspn(argv[1],...)

Changes in v3:
- Protect against segfault when trying to access non-existant argv[2]
- Moved "handle == NULL" error handler from main() to request_libusb_handle()
- Add missing "Signed-off-by:"

Changes in v4:
- Reworked commit message
- Handle possible libusb errors more consistently
- Be more strict on parsing "bus:devnum" device specifier
- Fixed various style issues

Changes in v5:
- Renamed libusb_open_device() to open_fel_device(), otherwise unsuspecting
readers might mistake it for a libusb library routine.
- Declare the 'magic' Allwinner FEL VID/PID constants near the top of file.

Bernhard Nortmann (1):
fel: support selection of specific USB bus and device number

fel.c | 129 ++++++++++++++++++++++++++++++++++++++++++++++++++++--------------
1 file changed, 103 insertions(+), 26 deletions(-)

--
2.4.10

[Siarhei Siamashka]

Thanks, also adding an extra

|| busnum < 0 || devnum < 0

check here would make it perfect.

If the user tries to provide negative numbers in the command line, then
it is reasonable to fail with an error instead of silently reverting to
the default "pick any FEL device" behaviour.

Reviewed-by: Siarhei Siamashka <siarhei....@gmail.com>

[Bernhard Nortmann]

Am 20.03.2016 um 13:53 schrieb Siarhei Siamashka:
> Thanks, also adding an extra
>
> || busnum < 0 || devnum < 0
>
> check here would make it perfect.
>
> If the user tries to provide negative numbers in the command line, then
> it is reasonable to fail with an error instead of silently reverting to
> the default "pick any FEL device" behaviour.

Should we extend that to be "|| busnum <= 0 || devnum <= 0"?

Afaik, a zero bus or device number would be impossible to match anyway.

Regards, B. Nortmann

[Siarhei Siamashka]

Yes, this makes sense. Feel free to do this adjustment when pushing the
patch to git.

[Bernhard Nortmann]

Thanks! Committed to sunxi-tools/master.