On Wed, Jan 29 2014, Robert Baldyga wrote:
> +++ b/tools/usb/aio_multibuff/device_app/aio_multibuff.c
> +#define BUF_LEN 8192
> +#define BUFS_MAX 128
> +#define AIO_MAX (BUFS_MAX*2)
> +
> +struct iocb *iocb1[AIO_MAX];
> +struct iocb *iocb2[AIO_MAX];
> +
> +unsigned char *buf1[BUFS_MAX];
> +unsigned char *buf2[BUFS_MAX];
Why are there twice as many iocb structures as there are buffers?
> +static void display_event(struct usb_functionfs_event *event)
> +{
> + static const char *const names[] = {
> + [FUNCTIONFS_BIND] = "BIND",
> + [FUNCTIONFS_UNBIND] = "UNBIND",
> + [FUNCTIONFS_ENABLE] = "ENABLE",
> + [FUNCTIONFS_DISABLE] = "DISABLE",
> + [FUNCTIONFS_SETUP] = "SETUP",
> + [FUNCTIONFS_SUSPEND] = "SUSPEND",
> + [FUNCTIONFS_RESUME] = "RESUME",
> + };
> + switch (event->type) {
> + case FUNCTIONFS_BIND:
> + case FUNCTIONFS_UNBIND:
> + case FUNCTIONFS_ENABLE:
> + case FUNCTIONFS_DISABLE:
> + case FUNCTIONFS_SETUP:
> + case FUNCTIONFS_SUSPEND:
> + case FUNCTIONFS_RESUME:
> + printf("Event %s\n", names[event->type]);
> + default:
> + break;
> + }
Weird indent level throughout the function.
> +}
> +
> +static void handle_ep0(int ep0, bool *ready)
> +{
> + struct usb_functionfs_event event;
> + int ret;
> +
> + struct pollfd pfds[1];
> + pfds[0].fd = ep0;
> + pfds[0].events = POLLIN;
> +
> + ret = poll(pfds, 1, 0);
> +
> + if (ret && (pfds[0].revents & POLLIN)) {
> + ret = read(ep0, &event, sizeof(struct usb_functionfs_event));
ret = read(ep0, &event, sizeof(event));
> + if (!ret)
> + return;
At the very least call perror.
> + display_event(&event);
> + switch (event.type) {
> + case FUNCTIONFS_SETUP:
> + if (event.u.setup.bRequestType & USB_DIR_IN)
> + write(ep0, NULL, 0);
> + else
> + read(ep0, NULL, 0);
> + break;
> +
> + case FUNCTIONFS_ENABLE:
> + *ready = true;
> + break;
> +
> + case FUNCTIONFS_DISABLE:
> + *ready = false;
> + break;
> +
> + default:
> + break;
> + }
> + }
> +}
> +int main(int argc, char *argv[])
> +{
> + int i, ret;
> + char ep_path[64];
Better yet, allocate this dynamically. Just before the first snprintf.
> +
> + int ep0, ep1;
> +
> + io_context_t ctx;
> +
> + int requested1 = 0, requested2 = 0;
> + int actual;
> + bool ready;
> +
> + if (argc != 2) {
> + printf("ffs directory not specified!\n");
> + return 1;
> + }
> +
> + /* open endpoint files */
ep_path = malloc(strlen(argv[1]) + 4 /* "/ep#" */ + 1 /* '\0' */);
if (!ep_path) {
perror("malloc");
return 1;
}
At this point you could get away with sprintf.
> + snprintf(ep_path, sizeof(ep_path), "%s/ep0", argv[1]);
> + ep0 = open(ep_path, O_RDWR);
> + if (ep0 < 0) {
> + perror("unable to open ep0");
> + return 1;
> + }
> + if (write(ep0, &descriptors, sizeof(descriptors)) < 0) {
> + perror("unable do write descriptors");
> + return 1;
> + }
> + if (write(ep0, &strings, sizeof(strings)) < 0) {
> + perror("unable to write strings");
> + return 1;
> + }
> + snprintf(ep_path, sizeof(ep_path), "%s/ep1", argv[1]);
> + ep1 = open(ep_path, O_RDWR);
> + if (ep1 < 0) {
> + perror("unable to open ep1");
> + return 1;
> + }
> +
> + memset(&ctx, 0, sizeof(ctx));
> + /* setup aio context to handle up to AIO_MAX requests */
> + io_setup(AIO_MAX, &ctx);
> +
> + init_bufs();
> +
> + while (1) {
> + handle_ep0(ep0, &ready);
> + /* we are waiting for function ENABLE */
> + if (!ready)
> + continue;
> + /*
> + * when we're preparing new data to submit,
> + * second buffer being transmitted
> + */
> + if (!requested1) { /* if all req's from iocb1 completed */
> + actual = 2;
> + for (i = 0; i < BUFS_MAX; ++i) /* prepare requests */
> + io_prep_pwrite(iocb1[i], ep1, buf1[i],
> + BUF_LEN, 0);
> + /* submit table of requests */
> + ret = io_submit(ctx, BUFS_MAX, iocb1);
> + requested1 = ret;
> + printf("submit: %d requests from buf 1\n", ret);
> + }
> + if (!requested2) { /* if all req's from iocb2 completed */
> + actual = 1;
> + for (i = 0; i < BUFS_MAX; ++i) /* prepare requests */
> + io_prep_pwrite(iocb2[i], ep1, buf2[i],
> + BUF_LEN, 0);
> + /* submit table of requests */
> + ret = io_submit(ctx, BUFS_MAX, iocb2);
> + requested2 = ret;
> + printf("submit: %d requests from buf 2\n", ret);
> + }
> + /* if something was submitted we wait for event */
> + if (requested1 || requested2) {
> + struct io_event e;
> + struct timespec timeout = {0, 1000};
> + /* we wait for one event */
> + ret = io_getevents(ctx, 1, 1, &e, &timeout);
What's the purpose of the timeout?
> + if (ret > 0) { /* if we got event */
> + if (actual == 1)
> + requested1--;
> + else
> + requested2--;
> + }
This whole loop would look cleaner if buf, iocb and requested variables
were two-element arrays. Or better yet, if you had a structure with
a buffer, iocb and requested count. With a structure, you could easily
go away without a global variables, if init_bufs and delete_bufs took
the structure as an argument.
> + }
> + }
> +
> + /* free resources */
> +
> + delete_bufs();
> + io_destroy(ctx);
> +
> + close(ep1);
> + close(ep0);
> +
> + return 0;
> +}
Haven't looked at the other files.
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michał “mina86” Nazarewicz (o o)
ooo +--<mpn@xxxxxxxxxx>--<xmpp:mina86@xxxxxxxxxx>--ooO--(_)--Ooo--
Attachment:
signature.asc
Description: PGP signature
