Hi, On 01/29/2014 01:39 PM, Michal Nazarewicz wrote: > 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? It's mistake. AIO_MAX should be used only for io_setup. > > >> +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? io_getevents is blocking with timeout==NULL, but we want to do something in meantime (at least handle ep0 events). > >> + 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. > > > Thanks for advices, I will fix it. Best regards Robert Baldyga Samsung R&D Institute Poland -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html