On Tue, Jan 28 2014, Robert Baldyga wrote:
> diff --git a/tools/usb/aio_multibuff/device_app/aio_multibuff.c b/tools/usb/aio_multibuff/device_app/aio_multibuff.c
> new file mode 100644
> index 0000000..df1db84
> --- /dev/null
> +++ b/tools/usb/aio_multibuff/device_app/aio_multibuff.c
> +#define USBFFS_DIR "usbffs/"
Uh?
> +int ready = 0;
static bool ready;
> +
> +static void handle_ep0(int ep0) {
> + struct usb_functionfs_event event;
> + int ret;
> +
> + struct pollfd pfds[1];
> + pfds[0].fd = ep0;
> + pfds[0].events = POLLIN;
> +
> + poll(pfds, 1, 0);
You should check return code of the function.
> +
> + if((pfds[0].revents & POLLIN)) {
> + ret = read(ep0, &event, sizeof(struct usb_functionfs_event));
> + if(!ret)
> + return;
> + display_event(&event);
> + if (event.type == FUNCTIONFS_SETUP) {
> + if (event.u.setup.bRequestType & USB_DIR_IN)
> + write(ep0, NULL, 0);
> + else
> + read(ep0, NULL, 0);
> + }
> + if (event.type == FUNCTIONFS_ENABLE)
+ } else if (event.type == FUNCTIONFS_ENABLE) {
It's more obvious what's happening here with else-if.
> + ready = 1;
+ }
Furthermore, you should also handle FUNCTIONFS_DISABLE.
> + }
> +}
> +
> +#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[AIO_MAX];
> +unsigned char *buf2[AIO_MAX];
Only half of those buffers are initialised in init_buffs.
> +
> +void init_bufs() {
> + int i, j;
> + for (i=0; i<BUFS_MAX; ++i) {
for (i = 0; i < sizeof(buf1) / sizeof(*buf1); ++i) {
Applies for all other for loops.
> + buf1[i] = malloc(BUF_LEN);
> + buf2[i] = malloc(BUF_LEN);
> + for (j=0; j<BUF_LEN; ++j) {
> + buf1[i][j] = j%64;
> + buf2[i][j] = j%64;
Is “% 64” really needed here for anything? Why not let it wrap around
256 automatically?
> + }
> + }
> + for (i=0; i<AIO_MAX; ++i) {
> + iocb1[i] = malloc(sizeof(struct iocb));
> + iocb2[i] = malloc(sizeof(struct iocb));
+ iocb1[i] = malloc(sizeof(**iocb1));
+ iocb2[i] = malloc(sizeof(**iocb2));
> + }
> +}
> +int main() {
> + int i, ret;
> + char ep_path[64];
Magic number. “char ep_path[sizeof(USBFFS_DIR) + 3 /* ep# */];” would
be much better.
> +
> + int ep0;
> + int ep[2];
> +
> + io_context_t ctx;
> +
> + int requested1 = 0;
> + int requested2 = 0;
> +
> + /* open endpoint files */
> + if((ep0 = open(USBFFS_DIR"ep0", O_RDWR)) < 0) printf("unable to open ep0\n");
USBFFS_DIR should be taken as argument or assumed to be “./”. Why
“usbffs/”? What if someone wished to mount two FFS gadgets and run this
program twice.
> + if(write(ep0, (void*)&descriptors, sizeof descriptors) < 0) printf("unable do write descriptors\n");
> + if(write(ep0, (void*)&strings, sizeof strings) < 0) printf("unable to write strings\n");
This is C, there's no need to cast to (void *).
Also, you should probably “return 1” on error instead of continuing, not
to mention that it would be nice to actually see the returned code.
perror may be your friend here. Applies to other places as well.
> + for(i=0; i<2; ++i) {
> + sprintf(ep_path, USBFFS_DIR"ep%d", i+1);
snprintf!
> + if((ep[i] = open(ep_path, O_RDWR | O_NONBLOCK, 0)) < 0)
> + printf("unable to open ep%d\n", i+1);
> + }
> +
> + memset(&ctx, 0, sizeof(ctx));
> + io_setup(AIO_MAX, &ctx); /* setup aio context to handle up to AIO_MAX requests */
> +
> + init_bufs();
> +
> + while(1) {
> + handle_ep0(ep0);
> + /* we are waiting for function ENABLE */
> + if(!ready) continue;
It would be much better if handle_ep0 took pointer to ready as an
argument. This would avoid unnecessary global variable.
> + /*
> + * when we're preparing new data to submit,
> + * second buffer being transmitted
> + */
> + if (!requested1) { /* if all req's from iocb1 completed */
> + for(i=0; i<BUFS_MAX; ++i) /* prepare requests */
> + io_prep_pwrite(iocb1[i], ep[0], 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 */
> + for(i=0; i<BUFS_MAX; ++i) /* prepare requests */
> + io_prep_pwrite(iocb2[i], ep[0], 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);
> + if(ret > 0) { /* if we got event */
> + if (e.obj->aio_fildes == ep[0])
> + requested1--;
> + if (e.obj->aio_fildes == ep[1])
> + requested2--;
> + }
> + }
> + }
> +
> + /* free resources */
> +
> + delete_bufs();
> + io_destroy(ctx);
> +
> + for(i=0; i<2; ++i) close(ep[i]);
> + close(ep0);
> +
> + return 0;
> +}
Haven't looked at any 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
