diff --git a/drivers/staging/greybus/connection.c b/drivers/staging/greybus/connection.c index 258d96cdba67..584f49164261 100644 --- a/drivers/staging/greybus/connection.c +++ b/drivers/staging/greybus/connection.c @@ -40,7 +40,7 @@ void greybus_cport_in(struct greybus_host_device *hd, u16 cport_id, "nonexistent connection (%zu bytes dropped)\n", length); return; } - gb_connection_operation_recv(connection, data, length); + gb_connection_recv(connection, data, length); } EXPORT_SYMBOL_GPL(greybus_cport_in); diff --git a/drivers/staging/greybus/core.c b/drivers/staging/greybus/core.c index 588e62412fd3..04fc5412c351 100644 --- a/drivers/staging/greybus/core.c +++ b/drivers/staging/greybus/core.c @@ -169,11 +169,9 @@ struct greybus_host_device *greybus_create_hd(struct greybus_host_driver *driver * Validate that the driver implements all of the callbacks * so that we don't have to every time we make them. */ - if ((!driver->alloc_gbuf_data) || - (!driver->free_gbuf_data) || - (!driver->submit_svc) || - (!driver->submit_gbuf) || - (!driver->kill_gbuf)) { + if ((!driver->buffer_alloc) || (!driver->buffer_free) || + (!driver->buffer_send) || (!driver->buffer_cancel) || + (!driver->submit_svc)) { pr_err("Must implement all greybus_host_driver callbacks!\n"); return NULL; } diff --git a/drivers/staging/greybus/es1-ap-usb.c b/drivers/staging/greybus/es1-ap-usb.c index e276f0c4e3cd..3404dc59a151 100644 --- a/drivers/staging/greybus/es1-ap-usb.c +++ b/drivers/staging/greybus/es1-ap-usb.c @@ -20,7 +20,6 @@ #define ES1_SVC_MSG_SIZE (sizeof(struct svc_msg) + SZ_64K) #define ES1_GBUF_MSG_SIZE PAGE_SIZE - static const struct usb_device_id id_table[] = { /* Made up numbers for the SVC USB Bridge in ES1 */ { USB_DEVICE(0xffff, 0x0001) }, @@ -86,70 +85,47 @@ static inline struct es1_ap_dev *hd_to_es1(struct greybus_host_device *hd) static void cport_out_callback(struct urb *urb); /* - * Allocate the actual buffer for this gbuf and device and cport - * - * We are responsible for setting the following fields in a struct gbuf: - * void *hcpriv; - * void *transfer_buffer; - * u32 transfer_buffer_length; + * Allocate a buffer to be sent via UniPro. */ -static int alloc_gbuf_data(struct gbuf *gbuf, unsigned int size, - gfp_t gfp_mask) +static void *buffer_alloc(unsigned int size, gfp_t gfp_mask) { - u32 cport_reserve = gbuf->dest_cport_id == CPORT_ID_BAD ? 0 : 1; u8 *buffer; - if (gbuf->transfer_buffer) - return -EALREADY; - if (size > ES1_GBUF_MSG_SIZE) { pr_err("guf was asked to be bigger than %ld!\n", ES1_GBUF_MSG_SIZE); } - /* For ES2 we need to figure out what cport is going to what endpoint, - * but for ES1, it's so dirt simple, we don't have a choice... + /* + * For ES1 we need to insert a byte at the front of the data + * to indicate the destination CPort id. We only need one + * extra byte, but we allocate four extra bytes to allow the + * buffer returned to be aligned on a four-byte boundary. * - * Also, do a "slow" allocation now, if we need speed, use a cache + * This is only needed for outbound data, but we handle + * buffers for inbound data the same way for consistency. * - * For ES1 outbound buffers need to insert their target - * CPort Id before the data; set aside an extra byte for - * that purpose in that case. + * XXX Do we need to indicate the destination device id too? */ - buffer = kzalloc(cport_reserve + size, gfp_mask); - if (!buffer) - return -ENOMEM; + buffer = kzalloc(GB_BUFFER_ALIGN + size, gfp_mask); + if (buffer) + buffer += GB_BUFFER_ALIGN; - /* Insert the cport id for outbound buffers */ - if (cport_reserve) { - if (gbuf->dest_cport_id > (u16)U8_MAX) { - pr_err("gbuf->dest_cport_id (%hd) is out of range!\n", - gbuf->dest_cport_id); - kfree(buffer); - return -EINVAL; - } - *buffer++ = gbuf->dest_cport_id; - } - gbuf->transfer_buffer = buffer; - gbuf->transfer_buffer_length = size; - - return 0; + return buffer; } -/* Free the memory we allocated with a gbuf */ -static void free_gbuf_data(struct gbuf *gbuf) +/* Free a previously-allocated buffer */ +static void buffer_free(void *buffer) { - u8 *transfer_buffer = gbuf->transfer_buffer; + u8 *allocated = buffer; - /* Can be called with a NULL transfer_buffer on some error paths */ - if (!transfer_buffer) + /* Can be called with a NULL buffer on some error paths */ + if (!allocated) return; - /* Account for the cport id in outbound buffers */ - if (gbuf->dest_cport_id != CPORT_ID_BAD) - transfer_buffer--; /* Back up to cport id */ - kfree(transfer_buffer); - gbuf->transfer_buffer = NULL; + /* Account for the space set aside for the prepended cport id */ + allocated -= GB_BUFFER_ALIGN; + kfree(allocated); } #define ES1_TIMEOUT 500 /* 500 ms for the SVC to do something */ @@ -207,53 +183,86 @@ static struct urb *next_free_urb(struct es1_ap_dev *es1, gfp_t gfp_mask) return urb; } -static int submit_gbuf(struct gbuf *gbuf, gfp_t gfp_mask) +/* + * Returns an opaque cookie value if successful, or a pointer coded + * error otherwise. If the caller wishes to cancel the in-flight + * buffer, it must supply the returned cookie to the cancel routine. + */ +static void *buffer_send(struct greybus_host_device *hd, u16 dest_cport_id, + void *buffer, size_t buffer_size, gfp_t gfp_mask) { - struct greybus_host_device *hd = gbuf->hd; struct es1_ap_dev *es1 = hd_to_es1(hd); struct usb_device *udev = es1->usb_dev; + u8 *transfer_buffer = buffer; + int transfer_buffer_size; int retval; - u8 *transfer_buffer; - u8 *buffer; struct urb *urb; - transfer_buffer = gbuf->transfer_buffer; - if (!transfer_buffer) - return -EINVAL; - buffer = &transfer_buffer[-1]; /* yes, we mean -1 */ + if (!buffer) { + pr_err("null buffer supplied to send\n"); + return ERR_PTR(-EINVAL); + } + if (buffer_size > (size_t)INT_MAX) { + pr_err("bad buffer size (%zu) supplied to send\n", buffer_size); + return ERR_PTR(-EINVAL); + } + transfer_buffer--; + transfer_buffer_size = buffer_size + 1; + + /* + * The data actually transferred will include an indication + * of where the data should be sent. Do one last check of + * the target CPort id before filling it in. + */ + if (dest_cport_id == CPORT_ID_BAD) { + pr_err("request to send inbound data buffer\n"); + return ERR_PTR(-EINVAL); + } + if (dest_cport_id > (u16)U8_MAX) { + pr_err("dest_cport_id (%hd) is out of range for ES1\n", + dest_cport_id); + return ERR_PTR(-EINVAL); + } + /* OK, the destination is fine; record it in the transfer buffer */ + *transfer_buffer = dest_cport_id; /* Find a free urb */ urb = next_free_urb(es1, gfp_mask); if (!urb) - return -ENOMEM; - - gbuf->hcd_data = urb; + return ERR_PTR(-ENOMEM); usb_fill_bulk_urb(urb, udev, usb_sndbulkpipe(udev, es1->cport_out_endpoint), - buffer, gbuf->transfer_buffer_length + 1, - cport_out_callback, gbuf); + transfer_buffer, transfer_buffer_size, + cport_out_callback, hd); retval = usb_submit_urb(urb, gfp_mask); - return retval; + if (retval) { + pr_err("error %d submitting URB\n", retval); + return ERR_PTR(retval); + } + + return urb; } -static void kill_gbuf(struct gbuf *gbuf) +static void buffer_cancel(void *cookie) { - struct urb *urb = gbuf->hcd_data; - - if (!urb) - return; + struct urb *urb = cookie; + /* + * We really should be defensive and track all outstanding + * (sent) buffers rather than trusting the cookie provided + * is valid. For the time being, this will do. + */ usb_kill_urb(urb); } static struct greybus_host_driver es1_driver = { .hd_priv_size = sizeof(struct es1_ap_dev), - .alloc_gbuf_data = alloc_gbuf_data, - .free_gbuf_data = free_gbuf_data, + .buffer_alloc = buffer_alloc, + .buffer_free = buffer_free, + .buffer_send = buffer_send, + .buffer_cancel = buffer_cancel, .submit_svc = submit_svc, - .submit_gbuf = submit_gbuf, - .kill_gbuf = kill_gbuf, }; /* Common function to report consistent warnings based on URB status */ @@ -329,7 +338,8 @@ static void ap_disconnect(struct usb_interface *interface) /* Callback for when we get a SVC message */ static void svc_in_callback(struct urb *urb) { - struct es1_ap_dev *es1 = urb->context; + struct greybus_host_device *hd = urb->context; + struct es1_ap_dev *es1 = hd_to_es1(hd); struct device *dev = &urb->dev->dev; int status = check_urb_status(urb); int retval; @@ -355,8 +365,9 @@ static void svc_in_callback(struct urb *urb) static void cport_in_callback(struct urb *urb) { + struct greybus_host_device *hd = urb->context; + struct es1_ap_dev *es1 = hd_to_es1(hd); struct device *dev = &urb->dev->dev; - struct es1_ap_dev *es1 = urb->context; int status = check_urb_status(urb); int retval; u8 cport; @@ -396,15 +407,12 @@ static void cport_in_callback(struct urb *urb) static void cport_out_callback(struct urb *urb) { - struct gbuf *gbuf = urb->context; - struct es1_ap_dev *es1 = hd_to_es1(gbuf->hd); + struct greybus_host_device *hd = urb->context; + struct es1_ap_dev *es1 = hd_to_es1(hd); unsigned long flags; + /* int status = check_urb_status(urb); */ int i; - /* Record whether the transfer was successful */ - gbuf->status = check_urb_status(urb); - gbuf->hcd_data = NULL; - /* * See if this was an urb in our pool, if so mark it "free", otherwise * we need to free it ourselves. @@ -423,6 +431,8 @@ static void cport_out_callback(struct urb *urb) usb_free_urb(urb); /* + * Rest assured Greg, this craziness is getting fixed. + * * Yes, you are right, we aren't telling anyone that the urb finished. * "That's crazy! How does this all even work?" you might be saying. * The "magic" is the idea that greybus works on the "operation" level, @@ -529,7 +539,7 @@ static int ap_probe(struct usb_interface *interface, usb_fill_int_urb(es1->svc_urb, udev, usb_rcvintpipe(udev, es1->svc_endpoint), es1->svc_buffer, ES1_SVC_MSG_SIZE, svc_in_callback, - es1, svc_interval); + hd, svc_interval); retval = usb_submit_urb(es1->svc_urb, GFP_KERNEL); if (retval) goto error; @@ -549,7 +559,7 @@ static int ap_probe(struct usb_interface *interface, usb_fill_bulk_urb(urb, udev, usb_rcvbulkpipe(udev, es1->cport_in_endpoint), buffer, ES1_GBUF_MSG_SIZE, cport_in_callback, - es1); + hd); es1->cport_in_urb[i] = urb; es1->cport_in_buffer[i] = buffer; retval = usb_submit_urb(urb, GFP_KERNEL); diff --git a/drivers/staging/greybus/greybus.h b/drivers/staging/greybus/greybus.h index 301bd4598c11..3df2b5a60d80 100644 --- a/drivers/staging/greybus/greybus.h +++ b/drivers/staging/greybus/greybus.h @@ -68,7 +68,9 @@ struct greybus_host_device; struct svc_msg; -struct gbuf; + +/* Buffers allocated from the host driver will be aligned to this multiple */ +#define GB_BUFFER_ALIGN sizeof(u32) /* Greybus "Host driver" structure, needed by a host controller driver to be * able to handle both SVC control as well as "real" greybus messages @@ -76,13 +78,13 @@ struct gbuf; struct greybus_host_driver { size_t hd_priv_size; - int (*alloc_gbuf_data)(struct gbuf *gbuf, unsigned int size, - gfp_t gfp_mask); - void (*free_gbuf_data)(struct gbuf *gbuf); + void *(*buffer_alloc)(unsigned int size, gfp_t gfp_mask); + void (*buffer_free)(void *buffer); + void *(*buffer_send)(struct greybus_host_device *hd, u16 dest_cport_id, + void *buffer, size_t buffer_size, gfp_t gfp_mask); + void (*buffer_cancel)(void *cookie); int (*submit_svc)(struct svc_msg *svc_msg, struct greybus_host_device *hd); - int (*submit_gbuf)(struct gbuf *gbuf, gfp_t gfp_mask); - void (*kill_gbuf)(struct gbuf *gbuf); }; struct greybus_host_device { @@ -153,8 +155,6 @@ int gb_ap_init(void); void gb_ap_exit(void); int gb_debugfs_init(void); void gb_debugfs_cleanup(void); -int gb_gbuf_init(void); -void gb_gbuf_exit(void); extern struct bus_type greybus_bus_type; extern const struct attribute_group *greybus_module_groups[]; diff --git a/drivers/staging/greybus/operation.c b/drivers/staging/greybus/operation.c index 223988327795..96f4c689e998 100644 --- a/drivers/staging/greybus/operation.c +++ b/drivers/staging/greybus/operation.c @@ -72,7 +72,7 @@ static void gb_pending_operation_insert(struct gb_operation *operation) spin_unlock_irq(&gb_operations_lock); /* Store the operation id in the request header */ - header = operation->request.gbuf.transfer_buffer; + header = operation->request.buffer; header->id = cpu_to_le16(operation->id); } @@ -103,20 +103,37 @@ gb_pending_operation_find(struct gb_connection *connection, u16 id) return found ? operation : NULL; } -static int greybus_submit_gbuf(struct gbuf *gbuf, gfp_t gfp_mask) +static int gb_message_send(struct gb_message *message, gfp_t gfp_mask) { - gbuf->status = -EINPROGRESS; + struct gb_connection *connection = message->operation->connection; + u16 dest_cport_id = connection->interface_cport_id; - return gbuf->hd->driver->submit_gbuf(gbuf, gfp_mask); + message->status = -EINPROGRESS; + message->cookie = connection->hd->driver->buffer_send(connection->hd, + dest_cport_id, + message->buffer, + message->buffer_size, + gfp_mask); + if (IS_ERR(message->cookie)) { + message->status = PTR_ERR(message->cookie); + message->cookie = NULL; + + return message->status; + } + return 0; } -static void greybus_kill_gbuf(struct gbuf *gbuf) +static void gb_message_cancel(struct gb_message *message) { - if (gbuf->status != -EINPROGRESS) + struct greybus_host_device *hd; + + if (message->status != -EINPROGRESS) return; - gbuf->hd->driver->kill_gbuf(gbuf); + hd = message->operation->connection->hd; + hd->driver->buffer_cancel(message->cookie); } + /* * An operations's response message has arrived. If no callback was * supplied it was submitted for asynchronous completion, so we notify @@ -139,7 +156,7 @@ int gb_operation_wait(struct gb_operation *operation) ret = wait_for_completion_interruptible(&operation->completion); /* If interrupted, cancel the in-flight buffer */ if (ret < 0) - greybus_kill_gbuf(&operation->request.gbuf); + gb_message_cancel(&operation->request); return ret; } @@ -149,7 +166,7 @@ static void gb_operation_request_handle(struct gb_operation *operation) struct gb_protocol *protocol = operation->connection->protocol; struct gb_operation_msg_hdr *header; - header = operation->request.gbuf.transfer_buffer; + header = operation->request.buffer; /* * If the protocol has no incoming request handler, report @@ -179,7 +196,7 @@ static void gb_operation_recv_work(struct work_struct *recv_work) bool incoming_request; operation = container_of(recv_work, struct gb_operation, recv_work); - incoming_request = operation->response.gbuf.transfer_buffer == NULL; + incoming_request = operation->response.buffer == NULL; if (incoming_request) gb_operation_request_handle(operation); gb_operation_complete(operation); @@ -213,16 +230,12 @@ static void operation_timeout(struct work_struct *work) */ static int gb_operation_message_init(struct gb_operation *operation, u8 type, size_t size, - bool request, bool data_out) + bool request, gfp_t gfp_flags) { struct gb_connection *connection = operation->connection; struct greybus_host_device *hd = connection->hd; struct gb_message *message; struct gb_operation_msg_hdr *header; - struct gbuf *gbuf; - gfp_t gfp_flags = data_out ? GFP_KERNEL : GFP_ATOMIC; - u16 dest_cport_id; - int ret; if (size > GB_OPERATION_MESSAGE_SIZE_MAX) return -E2BIG; @@ -234,22 +247,15 @@ static int gb_operation_message_init(struct gb_operation *operation, message = &operation->response; type |= GB_OPERATION_TYPE_RESPONSE; } - gbuf = &message->gbuf; - if (data_out) - dest_cport_id = connection->interface_cport_id; - else - dest_cport_id = CPORT_ID_BAD; - - gbuf->hd = hd; - gbuf->dest_cport_id = dest_cport_id; - gbuf->status = -EBADR; /* Initial value--means "never set" */ - ret = hd->driver->alloc_gbuf_data(gbuf, size, gfp_flags); - if (ret) - return ret; + message->buffer = hd->driver->buffer_alloc(size, gfp_flags); + if (!message->buffer) + return -ENOMEM; + message->buffer_size = size; + message->status = -EBADR; /* Initial value--means "never set" */ /* Fill in the header structure */ - header = (struct gb_operation_msg_hdr *)gbuf->transfer_buffer; + header = message->buffer; header->size = cpu_to_le16(size); header->id = 0; /* Filled in when submitted */ header->type = type; @@ -262,9 +268,15 @@ static int gb_operation_message_init(struct gb_operation *operation, static void gb_operation_message_exit(struct gb_message *message) { + struct greybus_host_device *hd; + + hd = message->operation->connection->hd; + hd->driver->buffer_free(message->buffer); + message->operation = NULL; message->payload = NULL; - message->gbuf.hd->driver->free_gbuf_data(&message->gbuf); + message->buffer = NULL; + message->buffer_size = 0; } /* @@ -298,13 +310,13 @@ struct gb_operation *gb_operation_create(struct gb_connection *connection, operation->connection = connection; ret = gb_operation_message_init(operation, type, request_size, - true, outgoing); + true, gfp_flags); if (ret) goto err_cache; if (outgoing) { ret = gb_operation_message_init(operation, type, response_size, - false, false); + false, GFP_KERNEL); if (ret) goto err_request; } @@ -377,11 +389,11 @@ int gb_operation_request_send(struct gb_operation *operation, * XXX * I think the order of operations is going to be * significant, and if so, we may need a mutex to surround - * setting the operation id and submitting the gbuf. + * setting the operation id and submitting the buffer. */ operation->callback = callback; gb_pending_operation_insert(operation); - ret = greybus_submit_gbuf(&operation->request.gbuf, GFP_KERNEL); + ret = gb_message_send(&operation->request, GFP_KERNEL); if (ret) return ret; @@ -418,12 +430,12 @@ int gb_operation_response_send(struct gb_operation *operation) * data into the buffer and do remaining handling via a work queue. * */ -void gb_connection_operation_recv(struct gb_connection *connection, +void gb_connection_recv(struct gb_connection *connection, void *data, size_t size) { struct gb_operation_msg_hdr *header; struct gb_operation *operation; - struct gbuf *gbuf; + struct gb_message *message; u16 msg_size; if (connection->state != GB_CONNECTION_STATE_ENABLED) @@ -446,8 +458,8 @@ void gb_connection_operation_recv(struct gb_connection *connection, } cancel_delayed_work(&operation->timeout_work); gb_pending_operation_remove(operation); - gbuf = &operation->response.gbuf; - if (size > gbuf->transfer_buffer_length) { + message = &operation->response; + if (size > message->buffer_size) { operation->result = GB_OP_OVERFLOW; gb_connection_err(connection, "recv buffer too small"); return; @@ -461,10 +473,10 @@ void gb_connection_operation_recv(struct gb_connection *connection, gb_connection_err(connection, "can't create operation"); return; } - gbuf = &operation->request.gbuf; + message = &operation->request; } - memcpy(gbuf->transfer_buffer, data, msg_size); + memcpy(message->buffer, data, msg_size); /* The rest will be handled in work queue context */ queue_work(gb_operation_recv_workqueue, &operation->recv_work); @@ -476,9 +488,9 @@ void gb_connection_operation_recv(struct gb_connection *connection, void gb_operation_cancel(struct gb_operation *operation) { operation->canceled = true; - greybus_kill_gbuf(&operation->request.gbuf); - if (operation->response.gbuf.transfer_buffer) - greybus_kill_gbuf(&operation->response.gbuf); + gb_message_cancel(&operation->request); + if (operation->response.buffer) + gb_message_cancel(&operation->response); } int gb_operation_init(void) diff --git a/drivers/staging/greybus/operation.h b/drivers/staging/greybus/operation.h index f43531dbcf33..81fd7f70b8ba 100644 --- a/drivers/staging/greybus/operation.h +++ b/drivers/staging/greybus/operation.h @@ -24,21 +24,16 @@ enum gb_operation_status { GB_OP_TIMEOUT = 0xff, }; -struct gbuf { - struct greybus_host_device *hd; - u16 dest_cport_id; /* Destination CPort id */ - int status; - - void *transfer_buffer; - u32 transfer_buffer_length; - - void *hcd_data; /* for the HCD to track the gbuf */ -}; - struct gb_message { void *payload; + struct gb_operation *operation; - struct gbuf gbuf; + int status; + + void *buffer; + size_t buffer_size; + + void *cookie; }; /* @@ -87,7 +82,7 @@ struct gb_operation { struct list_head links; /* connection->{operations,pending} */ }; -void gb_connection_operation_recv(struct gb_connection *connection, +void gb_connection_recv(struct gb_connection *connection, void *data, size_t size); struct gb_operation *gb_operation_create(struct gb_connection *connection,