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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
