From 4f83b8d34964ef343afe5e8f731a0e37e311a42d Mon Sep 17 00:00:00 2001 From: Alex Elder Date: Tue, 18 Nov 2014 13:26:38 -0600 Subject: [PATCH 01/17] greybus: fix an allocation flag bug We allocate message buffers with GFP_KERNEL allocation flags if possible. However when an incoming request message is received we can be in interrupt context, so we must use GFP_ATOMIC in that case. The computation of gfp_flags in gb_operation_message_init() is wrong. It is needlessly using GFP_ATOMIC when allocating outbound response buffers. Fix the flawed logic. Change the name of "data_out" to be "outbound" to be consistent with usage elsewhere. (Data/messages are "inbound" or "outbound"; requests are "incoming" or "outgoing".) Signed-off-by: Alex Elder Signed-off-by: Greg Kroah-Hartman --- drivers/staging/greybus/operation.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/staging/greybus/operation.c b/drivers/staging/greybus/operation.c index 223988327795..103fc9746796 100644 --- a/drivers/staging/greybus/operation.c +++ b/drivers/staging/greybus/operation.c @@ -213,14 +213,14 @@ 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, bool outbound) { 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; + gfp_t gfp_flags = request && !outbound ? GFP_ATOMIC : GFP_KERNEL; u16 dest_cport_id; int ret; @@ -236,7 +236,7 @@ static int gb_operation_message_init(struct gb_operation *operation, } gbuf = &message->gbuf; - if (data_out) + if (outbound) dest_cport_id = connection->interface_cport_id; else dest_cport_id = CPORT_ID_BAD; From 5259ef138cbc78c537ca9f375eb0d18f21320c01 Mon Sep 17 00:00:00 2001 From: Alex Elder Date: Tue, 18 Nov 2014 13:26:39 -0600 Subject: [PATCH 02/17] greybus: prepend cport byte for all gbufs Treat communication buffers for both inbound and outbound data the same way, prepending a "destination cport id" byte before the data in the buffer. Currently this is done only for outbound data buffers. This isn't needed for inbound data, but handling it this way allows the free routine to work without knowing whether the buffer was used for sending or receiving. Signed-off-by: Alex Elder Signed-off-by: Greg Kroah-Hartman --- drivers/staging/greybus/es1-ap-usb.c | 43 ++++++++++++++++------------ 1 file changed, 24 insertions(+), 19 deletions(-) diff --git a/drivers/staging/greybus/es1-ap-usb.c b/drivers/staging/greybus/es1-ap-usb.c index e276f0c4e3cd..062fb1a818ba 100644 --- a/drivers/staging/greybus/es1-ap-usb.c +++ b/drivers/staging/greybus/es1-ap-usb.c @@ -96,7 +96,7 @@ static void cport_out_callback(struct urb *urb); static int alloc_gbuf_data(struct gbuf *gbuf, unsigned int size, gfp_t gfp_mask) { - u32 cport_reserve = gbuf->dest_cport_id == CPORT_ID_BAD ? 0 : 1; + u8 dest_cport_id = gbuf->dest_cport_id; u8 *buffer; if (gbuf->transfer_buffer) @@ -107,29 +107,29 @@ static int alloc_gbuf_data(struct gbuf *gbuf, unsigned int size, 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. So we allocate one + * extra byte to allow for that. * - * 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); + buffer = kzalloc(1 + size, gfp_mask); if (!buffer) return -ENOMEM; /* 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; + if (dest_cport_id != CPORT_ID_BAD && dest_cport_id > (u16)U8_MAX) { + pr_err("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; @@ -145,9 +145,8 @@ static void free_gbuf_data(struct gbuf *gbuf) if (!transfer_buffer) return; - /* Account for the cport id in outbound buffers */ - if (gbuf->dest_cport_id != CPORT_ID_BAD) - transfer_buffer--; /* Back up to cport id */ + /* Account for the prepended cport id */ + transfer_buffer--; kfree(transfer_buffer); gbuf->transfer_buffer = NULL; } @@ -222,6 +221,12 @@ static int submit_gbuf(struct gbuf *gbuf, gfp_t gfp_mask) return -EINVAL; buffer = &transfer_buffer[-1]; /* yes, we mean -1 */ + /* Do one last check of the target CPort id */ + if (*buffer == CPORT_ID_BAD) { + pr_err("request to submit inbound buffer\n"); + return -EINVAL; + } + /* Find a free urb */ urb = next_free_urb(es1, gfp_mask); if (!urb) From 06a4a061f1917ab6dfdddfbf4a13c0a87f207602 Mon Sep 17 00:00:00 2001 From: Alex Elder Date: Tue, 18 Nov 2014 13:26:40 -0600 Subject: [PATCH 03/17] greybus: improve data buffer alignment For ES1 we need to insert the destination CPort id in whatever we supply for sending over UniPro. Currently we allocate one extra byte supply the caller with an address that's offset by one from the beginning of the allocated space. As a result we always return a poorly-aligned buffer pointer. Instead, allocate enough space so that we can return a better aligned buffer to the caller. Notes: - It may be that it's more important to supply an aligned address to the hardware. - We probably need to be more careful about writing into these buffers at unaligned offsets anyway. (E.g., writing a 2-byte value at an odd offset can't be assumed to work.) Signed-off-by: Alex Elder Signed-off-by: Greg Kroah-Hartman --- drivers/staging/greybus/es1-ap-usb.c | 15 ++++++++------- drivers/staging/greybus/greybus.h | 3 +++ 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/drivers/staging/greybus/es1-ap-usb.c b/drivers/staging/greybus/es1-ap-usb.c index 062fb1a818ba..a98a2cb67211 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) }, @@ -109,17 +108,19 @@ static int alloc_gbuf_data(struct gbuf *gbuf, unsigned int size, /* * For ES1 we need to insert a byte at the front of the data - * to indicate the destination CPort id. So we allocate one - * extra byte to allow for that. + * 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. * * This is only needed for outbound data, but we handle * buffers for inbound data the same way for consistency. * * XXX Do we need to indicate the destination device id too? */ - buffer = kzalloc(1 + size, gfp_mask); + buffer = kzalloc(GB_BUFFER_ALIGN + size, gfp_mask); if (!buffer) return -ENOMEM; + buffer += GB_BUFFER_ALIGN; /* Insert the cport id for outbound buffers */ if (dest_cport_id != CPORT_ID_BAD && dest_cport_id > (u16)U8_MAX) { @@ -128,7 +129,7 @@ static int alloc_gbuf_data(struct gbuf *gbuf, unsigned int size, kfree(buffer); return -EINVAL; } - *buffer++ = gbuf->dest_cport_id; + *(buffer - 1) = gbuf->dest_cport_id; gbuf->transfer_buffer = buffer; gbuf->transfer_buffer_length = size; @@ -145,8 +146,8 @@ static void free_gbuf_data(struct gbuf *gbuf) if (!transfer_buffer) return; - /* Account for the prepended cport id */ - transfer_buffer--; + /* Account for the space set aside for the prepended cport id */ + transfer_buffer -= GB_BUFFER_ALIGN; kfree(transfer_buffer); gbuf->transfer_buffer = NULL; } diff --git a/drivers/staging/greybus/greybus.h b/drivers/staging/greybus/greybus.h index 301bd4598c11..fa8065156192 100644 --- a/drivers/staging/greybus/greybus.h +++ b/drivers/staging/greybus/greybus.h @@ -70,6 +70,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 */ From 0f4c808a7ea2ee3d81f5c3047bd14d7057cbfe37 Mon Sep 17 00:00:00 2001 From: Alex Elder Date: Tue, 18 Nov 2014 13:26:41 -0600 Subject: [PATCH 04/17] greybus: fill in destination data at send time For ES1 we need to insert the destination CPort id before the data to be sent over UniPro. Currently this is done at the time the buffer is created, but there's no need to do so until we're actually going to send the content of the buffer. Move the setting of that destination information into submit_gbuf(). Note that this allows us to defer initializing a few other gbuf fields until after we know the buffer allocation has succeeded. Signed-off-by: Alex Elder Signed-off-by: Greg Kroah-Hartman --- drivers/staging/greybus/es1-ap-usb.c | 23 ++++++++++------------- drivers/staging/greybus/operation.c | 6 +++--- 2 files changed, 13 insertions(+), 16 deletions(-) diff --git a/drivers/staging/greybus/es1-ap-usb.c b/drivers/staging/greybus/es1-ap-usb.c index a98a2cb67211..a92f8934928a 100644 --- a/drivers/staging/greybus/es1-ap-usb.c +++ b/drivers/staging/greybus/es1-ap-usb.c @@ -95,7 +95,6 @@ static void cport_out_callback(struct urb *urb); static int alloc_gbuf_data(struct gbuf *gbuf, unsigned int size, gfp_t gfp_mask) { - u8 dest_cport_id = gbuf->dest_cport_id; u8 *buffer; if (gbuf->transfer_buffer) @@ -122,15 +121,6 @@ static int alloc_gbuf_data(struct gbuf *gbuf, unsigned int size, return -ENOMEM; buffer += GB_BUFFER_ALIGN; - /* Insert the cport id for outbound buffers */ - if (dest_cport_id != CPORT_ID_BAD && dest_cport_id > (u16)U8_MAX) { - pr_err("dest_cport_id (%hd) is out of range!\n", - gbuf->dest_cport_id); - kfree(buffer); - return -EINVAL; - } - *(buffer - 1) = gbuf->dest_cport_id; - gbuf->transfer_buffer = buffer; gbuf->transfer_buffer_length = size; @@ -212,6 +202,7 @@ static int submit_gbuf(struct gbuf *gbuf, 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; + u16 dest_cport_id = gbuf->dest_cport_id; int retval; u8 *transfer_buffer; u8 *buffer; @@ -222,11 +213,17 @@ static int submit_gbuf(struct gbuf *gbuf, gfp_t gfp_mask) return -EINVAL; buffer = &transfer_buffer[-1]; /* yes, we mean -1 */ - /* Do one last check of the target CPort id */ - if (*buffer == CPORT_ID_BAD) { - pr_err("request to submit inbound buffer\n"); + /* 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 -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 -EINVAL; + } + *buffer = dest_cport_id; /* Find a free urb */ urb = next_free_urb(es1, gfp_mask); diff --git a/drivers/staging/greybus/operation.c b/drivers/staging/greybus/operation.c index 103fc9746796..b5cd9a234fb6 100644 --- a/drivers/staging/greybus/operation.c +++ b/drivers/staging/greybus/operation.c @@ -241,12 +241,12 @@ static int gb_operation_message_init(struct gb_operation *operation, 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; + gbuf->hd = hd; + gbuf->dest_cport_id = dest_cport_id; + gbuf->status = -EBADR; /* Initial value--means "never set" */ /* Fill in the header structure */ header = (struct gb_operation_msg_hdr *)gbuf->transfer_buffer; From d2a259f213c925f404eb7491fae8fa03a56b3467 Mon Sep 17 00:00:00 2001 From: Alex Elder Date: Tue, 18 Nov 2014 13:26:42 -0600 Subject: [PATCH 05/17] greybus: allocate space without gbufs This begins the transition to buffer allocation that does not rely on the gbuf construct. The host driver allocation routine will return a pointer to the buffer to be used, and the caller will be responsible for keeping track of that pointer, as well as the requested buffer size. Rename the allocation method to reflect it's no longer tied to a gbuf. Signed-off-by: Alex Elder Signed-off-by: Greg Kroah-Hartman --- drivers/staging/greybus/core.c | 2 +- drivers/staging/greybus/es1-ap-usb.c | 25 ++++++------------------- drivers/staging/greybus/greybus.h | 3 +-- drivers/staging/greybus/operation.c | 8 ++++---- 4 files changed, 12 insertions(+), 26 deletions(-) diff --git a/drivers/staging/greybus/core.c b/drivers/staging/greybus/core.c index 588e62412fd3..ab50e2d6f817 100644 --- a/drivers/staging/greybus/core.c +++ b/drivers/staging/greybus/core.c @@ -169,7 +169,7 @@ 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) || + if ((!driver->buffer_alloc) || (!driver->free_gbuf_data) || (!driver->submit_svc) || (!driver->submit_gbuf) || diff --git a/drivers/staging/greybus/es1-ap-usb.c b/drivers/staging/greybus/es1-ap-usb.c index a92f8934928a..98ab05d9cead 100644 --- a/drivers/staging/greybus/es1-ap-usb.c +++ b/drivers/staging/greybus/es1-ap-usb.c @@ -85,21 +85,12 @@ 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) { 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); @@ -117,14 +108,10 @@ static int alloc_gbuf_data(struct gbuf *gbuf, unsigned int size, * XXX Do we need to indicate the destination device id too? */ buffer = kzalloc(GB_BUFFER_ALIGN + size, gfp_mask); - if (!buffer) - return -ENOMEM; - buffer += GB_BUFFER_ALIGN; + if (buffer) + buffer += GB_BUFFER_ALIGN; - gbuf->transfer_buffer = buffer; - gbuf->transfer_buffer_length = size; - - return 0; + return buffer; } /* Free the memory we allocated with a gbuf */ @@ -252,7 +239,7 @@ static void kill_gbuf(struct gbuf *gbuf) static struct greybus_host_driver es1_driver = { .hd_priv_size = sizeof(struct es1_ap_dev), - .alloc_gbuf_data = alloc_gbuf_data, + .buffer_alloc = buffer_alloc, .free_gbuf_data = free_gbuf_data, .submit_svc = submit_svc, .submit_gbuf = submit_gbuf, diff --git a/drivers/staging/greybus/greybus.h b/drivers/staging/greybus/greybus.h index fa8065156192..82ab1e6973d2 100644 --- a/drivers/staging/greybus/greybus.h +++ b/drivers/staging/greybus/greybus.h @@ -79,8 +79,7 @@ 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 *(*buffer_alloc)(unsigned int size, gfp_t gfp_mask); void (*free_gbuf_data)(struct gbuf *gbuf); int (*submit_svc)(struct svc_msg *svc_msg, struct greybus_host_device *hd); diff --git a/drivers/staging/greybus/operation.c b/drivers/staging/greybus/operation.c index b5cd9a234fb6..e3669a7a7901 100644 --- a/drivers/staging/greybus/operation.c +++ b/drivers/staging/greybus/operation.c @@ -222,7 +222,6 @@ static int gb_operation_message_init(struct gb_operation *operation, struct gbuf *gbuf; gfp_t gfp_flags = request && !outbound ? GFP_ATOMIC : GFP_KERNEL; u16 dest_cport_id; - int ret; if (size > GB_OPERATION_MESSAGE_SIZE_MAX) return -E2BIG; @@ -241,9 +240,10 @@ static int gb_operation_message_init(struct gb_operation *operation, else dest_cport_id = CPORT_ID_BAD; - ret = hd->driver->alloc_gbuf_data(gbuf, size, gfp_flags); - if (ret) - return ret; + gbuf->transfer_buffer = hd->driver->buffer_alloc(size, gfp_flags); + if (!gbuf->transfer_buffer) + return -ENOMEM; + gbuf->transfer_buffer_length = size; gbuf->hd = hd; gbuf->dest_cport_id = dest_cport_id; gbuf->status = -EBADR; /* Initial value--means "never set" */ From 9ec5411adf7cd872424f579701a91fffd508270b Mon Sep 17 00:00:00 2001 From: Alex Elder Date: Tue, 18 Nov 2014 13:26:43 -0600 Subject: [PATCH 06/17] greybus: free space without gbufs Switch the host driver free routine to take a pointer to the previously-allocated buffer that should be freed. Rename the method to reflect it's no longer tied to a gbuf. Signed-off-by: Alex Elder Signed-off-by: Greg Kroah-Hartman --- drivers/staging/greybus/core.c | 3 +-- drivers/staging/greybus/es1-ap-usb.c | 17 ++++++++--------- drivers/staging/greybus/greybus.h | 2 +- drivers/staging/greybus/operation.c | 4 +++- 4 files changed, 13 insertions(+), 13 deletions(-) diff --git a/drivers/staging/greybus/core.c b/drivers/staging/greybus/core.c index ab50e2d6f817..0f03521c53f9 100644 --- a/drivers/staging/greybus/core.c +++ b/drivers/staging/greybus/core.c @@ -169,8 +169,7 @@ 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->buffer_alloc) || - (!driver->free_gbuf_data) || + if ((!driver->buffer_alloc) || (!driver->buffer_free) || (!driver->submit_svc) || (!driver->submit_gbuf) || (!driver->kill_gbuf)) { diff --git a/drivers/staging/greybus/es1-ap-usb.c b/drivers/staging/greybus/es1-ap-usb.c index 98ab05d9cead..660c36367cbd 100644 --- a/drivers/staging/greybus/es1-ap-usb.c +++ b/drivers/staging/greybus/es1-ap-usb.c @@ -114,19 +114,18 @@ static void *buffer_alloc(unsigned int size, gfp_t gfp_mask) 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 space set aside for the prepended cport id */ - transfer_buffer -= GB_BUFFER_ALIGN; - kfree(transfer_buffer); - gbuf->transfer_buffer = NULL; + allocated -= GB_BUFFER_ALIGN; + kfree(allocated); } #define ES1_TIMEOUT 500 /* 500 ms for the SVC to do something */ @@ -240,7 +239,7 @@ static void kill_gbuf(struct gbuf *gbuf) static struct greybus_host_driver es1_driver = { .hd_priv_size = sizeof(struct es1_ap_dev), .buffer_alloc = buffer_alloc, - .free_gbuf_data = free_gbuf_data, + .buffer_free = buffer_free, .submit_svc = submit_svc, .submit_gbuf = submit_gbuf, .kill_gbuf = kill_gbuf, diff --git a/drivers/staging/greybus/greybus.h b/drivers/staging/greybus/greybus.h index 82ab1e6973d2..f27dcaf067ca 100644 --- a/drivers/staging/greybus/greybus.h +++ b/drivers/staging/greybus/greybus.h @@ -80,7 +80,7 @@ struct greybus_host_driver { size_t hd_priv_size; void *(*buffer_alloc)(unsigned int size, gfp_t gfp_mask); - void (*free_gbuf_data)(struct gbuf *gbuf); + void (*buffer_free)(void *buffer); int (*submit_svc)(struct svc_msg *svc_msg, struct greybus_host_device *hd); int (*submit_gbuf)(struct gbuf *gbuf, gfp_t gfp_mask); diff --git a/drivers/staging/greybus/operation.c b/drivers/staging/greybus/operation.c index e3669a7a7901..c04aced63204 100644 --- a/drivers/staging/greybus/operation.c +++ b/drivers/staging/greybus/operation.c @@ -264,7 +264,9 @@ static void gb_operation_message_exit(struct gb_message *message) { message->operation = NULL; message->payload = NULL; - message->gbuf.hd->driver->free_gbuf_data(&message->gbuf); + message->gbuf.hd->driver->buffer_free(message->gbuf.transfer_buffer); + message->gbuf.transfer_buffer = NULL; + message->gbuf.transfer_buffer_length = 0; } /* From a9163b2c30c9e110530ed5f56bc5296bb152aa98 Mon Sep 17 00:00:00 2001 From: Alex Elder Date: Tue, 18 Nov 2014 13:26:44 -0600 Subject: [PATCH 07/17] greybus: cancel buffers via magic cookie Change the interface for canceling in-flight buffers to take a magic cookie value as argument rather than a gbuf. Right now we pass the gbuf->hcd_data pointer that's assumed to have been set by the submit routine. But the next patch will change the submit routine to return the cookie to be used, and the caller will be responsible for keeping track of it. Signed-off-by: Alex Elder Signed-off-by: Greg Kroah-Hartman --- drivers/staging/greybus/core.c | 4 ++-- drivers/staging/greybus/es1-ap-usb.c | 16 +++++++++------- drivers/staging/greybus/greybus.h | 4 ++-- drivers/staging/greybus/operation.c | 3 ++- 4 files changed, 15 insertions(+), 12 deletions(-) diff --git a/drivers/staging/greybus/core.c b/drivers/staging/greybus/core.c index 0f03521c53f9..39f8c4a5c2d2 100644 --- a/drivers/staging/greybus/core.c +++ b/drivers/staging/greybus/core.c @@ -170,9 +170,9 @@ struct greybus_host_device *greybus_create_hd(struct greybus_host_driver *driver * so that we don't have to every time we make them. */ if ((!driver->buffer_alloc) || (!driver->buffer_free) || - (!driver->submit_svc) || (!driver->submit_gbuf) || - (!driver->kill_gbuf)) { + (!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 660c36367cbd..c4a7def1dda7 100644 --- a/drivers/staging/greybus/es1-ap-usb.c +++ b/drivers/staging/greybus/es1-ap-usb.c @@ -226,13 +226,15 @@ static int submit_gbuf(struct gbuf *gbuf, gfp_t gfp_mask) return retval; } -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); } @@ -240,9 +242,9 @@ static struct greybus_host_driver es1_driver = { .hd_priv_size = sizeof(struct es1_ap_dev), .buffer_alloc = buffer_alloc, .buffer_free = buffer_free, - .submit_svc = submit_svc, .submit_gbuf = submit_gbuf, - .kill_gbuf = kill_gbuf, + .buffer_cancel = buffer_cancel, + .submit_svc = submit_svc, }; /* Common function to report consistent warnings based on URB status */ diff --git a/drivers/staging/greybus/greybus.h b/drivers/staging/greybus/greybus.h index f27dcaf067ca..a9b2b459d7ad 100644 --- a/drivers/staging/greybus/greybus.h +++ b/drivers/staging/greybus/greybus.h @@ -81,10 +81,10 @@ struct greybus_host_driver { void *(*buffer_alloc)(unsigned int size, gfp_t gfp_mask); void (*buffer_free)(void *buffer); + int (*submit_gbuf)(struct gbuf *gbuf, 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 { diff --git a/drivers/staging/greybus/operation.c b/drivers/staging/greybus/operation.c index c04aced63204..26c9dd688cc3 100644 --- a/drivers/staging/greybus/operation.c +++ b/drivers/staging/greybus/operation.c @@ -115,8 +115,9 @@ static void greybus_kill_gbuf(struct gbuf *gbuf) if (gbuf->status != -EINPROGRESS) return; - gbuf->hd->driver->kill_gbuf(gbuf); + gbuf->hd->driver->buffer_cancel(gbuf->hcd_data); } + /* * An operations's response message has arrived. If no callback was * supplied it was submitted for asynchronous completion, so we notify From fa23ffeee6949ab5962fe2727ffb107574123aaf Mon Sep 17 00:00:00 2001 From: Alex Elder Date: Tue, 18 Nov 2014 13:26:45 -0600 Subject: [PATCH 08/17] greybus: stash hd as context for all URBs This changes the context value stashed with each USB URB so that it is always the host device pointer. In cport_out_callback() this allows us to get away with *not* requiring the gbuf for handling completions any more. We are (currently) ignoring the gbuf status value returned anyway, so we'll skip setting it altogether. Greg's comments in cport_out_callback() point out that ignoring this was misguided, and handling send errors will be put in place in an upcoming patch. The context is set to the host device pointer for SVC receive and CPort receive URBs for consistency--because we can. Signed-off-by: Alex Elder Signed-off-by: Greg Kroah-Hartman --- drivers/staging/greybus/es1-ap-usb.c | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/drivers/staging/greybus/es1-ap-usb.c b/drivers/staging/greybus/es1-ap-usb.c index c4a7def1dda7..9801d08fbc08 100644 --- a/drivers/staging/greybus/es1-ap-usb.c +++ b/drivers/staging/greybus/es1-ap-usb.c @@ -221,7 +221,7 @@ static int submit_gbuf(struct gbuf *gbuf, gfp_t gfp_mask) usb_fill_bulk_urb(urb, udev, usb_sndbulkpipe(udev, es1->cport_out_endpoint), buffer, gbuf->transfer_buffer_length + 1, - cport_out_callback, gbuf); + cport_out_callback, hd); retval = usb_submit_urb(urb, gfp_mask); return retval; } @@ -320,7 +320,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; @@ -346,8 +347,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; @@ -387,15 +389,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. @@ -414,6 +413,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, @@ -520,7 +521,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; @@ -540,7 +541,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); From 58a5bdc7358ae87d2f7b8c85319f624651b7555b Mon Sep 17 00:00:00 2001 From: Alex Elder Date: Tue, 18 Nov 2014 13:26:46 -0600 Subject: [PATCH 09/17] greybus: send buffers without gbufs Change the method that sends messages so that it sends "raw" buffers rather than gbufs. To do this, we supply the host device and destination CPort when sending. As with other recent patches, change the name of the method to reflect that we're no longer dealing with gbufs. The interface has changed as well. Now this routine will return a "cookie" value. The cookie is used to represent the outgoing request, and is supplied by the caller if necessary to cancel a previously-sent buffer. We'll store the result in gbuf->hcd_data for now (which produces the same result as before...). Signed-off-by: Alex Elder Signed-off-by: Greg Kroah-Hartman --- drivers/staging/greybus/core.c | 3 +- drivers/staging/greybus/es1-ap-usb.c | 56 ++++++++++++++++++---------- drivers/staging/greybus/greybus.h | 3 +- drivers/staging/greybus/operation.c | 10 ++++- 4 files changed, 49 insertions(+), 23 deletions(-) diff --git a/drivers/staging/greybus/core.c b/drivers/staging/greybus/core.c index 39f8c4a5c2d2..04fc5412c351 100644 --- a/drivers/staging/greybus/core.c +++ b/drivers/staging/greybus/core.c @@ -170,8 +170,7 @@ struct greybus_host_device *greybus_create_hd(struct greybus_host_driver *driver * so that we don't have to every time we make them. */ if ((!driver->buffer_alloc) || (!driver->buffer_free) || - (!driver->submit_gbuf) || - (!driver->buffer_cancel) || + (!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 9801d08fbc08..3404dc59a151 100644 --- a/drivers/staging/greybus/es1-ap-usb.c +++ b/drivers/staging/greybus/es1-ap-usb.c @@ -183,47 +183,65 @@ 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; - u16 dest_cport_id = gbuf->dest_cport_id; + 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; - /* Do one last check of the target CPort id before filling it in */ + /* + * 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 -EINVAL; + 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 -EINVAL; + return ERR_PTR(-EINVAL); } - *buffer = dest_cport_id; + /* 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, + 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 buffer_cancel(void *cookie) @@ -242,7 +260,7 @@ static struct greybus_host_driver es1_driver = { .hd_priv_size = sizeof(struct es1_ap_dev), .buffer_alloc = buffer_alloc, .buffer_free = buffer_free, - .submit_gbuf = submit_gbuf, + .buffer_send = buffer_send, .buffer_cancel = buffer_cancel, .submit_svc = submit_svc, }; diff --git a/drivers/staging/greybus/greybus.h b/drivers/staging/greybus/greybus.h index a9b2b459d7ad..4ac7376fe815 100644 --- a/drivers/staging/greybus/greybus.h +++ b/drivers/staging/greybus/greybus.h @@ -81,7 +81,8 @@ struct greybus_host_driver { void *(*buffer_alloc)(unsigned int size, gfp_t gfp_mask); void (*buffer_free)(void *buffer); - int (*submit_gbuf)(struct gbuf *gbuf, gfp_t gfp_mask); + 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); diff --git a/drivers/staging/greybus/operation.c b/drivers/staging/greybus/operation.c index 26c9dd688cc3..33cc4145db3c 100644 --- a/drivers/staging/greybus/operation.c +++ b/drivers/staging/greybus/operation.c @@ -106,8 +106,16 @@ gb_pending_operation_find(struct gb_connection *connection, u16 id) static int greybus_submit_gbuf(struct gbuf *gbuf, gfp_t gfp_mask) { gbuf->status = -EINPROGRESS; + gbuf->hcd_data = gbuf->hd->driver->buffer_send(gbuf->hd, + gbuf->dest_cport_id, gbuf->transfer_buffer, + gbuf->transfer_buffer_length, gfp_mask); + if (IS_ERR(gbuf->hcd_data)) { + gbuf->status = PTR_ERR(gbuf->hcd_data); + gbuf->hcd_data = NULL; - return gbuf->hd->driver->submit_gbuf(gbuf, gfp_mask); + return gbuf->status; + } + return 0; } static void greybus_kill_gbuf(struct gbuf *gbuf) From 6a70736aca05d4c8acd80f30bf485dd785ae1a2b Mon Sep 17 00:00:00 2001 From: Alex Elder Date: Tue, 18 Nov 2014 13:26:47 -0600 Subject: [PATCH 10/17] greybus: rework message initialization Rework gb_opreation_message_init() so it doesn't use a struct gbuf local variable. Signed-off-by: Alex Elder Signed-off-by: Greg Kroah-Hartman --- drivers/staging/greybus/operation.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/drivers/staging/greybus/operation.c b/drivers/staging/greybus/operation.c index 33cc4145db3c..57694e03c187 100644 --- a/drivers/staging/greybus/operation.c +++ b/drivers/staging/greybus/operation.c @@ -228,7 +228,6 @@ static int gb_operation_message_init(struct gb_operation *operation, struct greybus_host_device *hd = connection->hd; struct gb_message *message; struct gb_operation_msg_hdr *header; - struct gbuf *gbuf; gfp_t gfp_flags = request && !outbound ? GFP_ATOMIC : GFP_KERNEL; u16 dest_cport_id; @@ -242,23 +241,22 @@ static int gb_operation_message_init(struct gb_operation *operation, message = &operation->response; type |= GB_OPERATION_TYPE_RESPONSE; } - gbuf = &message->gbuf; if (outbound) dest_cport_id = connection->interface_cport_id; else dest_cport_id = CPORT_ID_BAD; - gbuf->transfer_buffer = hd->driver->buffer_alloc(size, gfp_flags); - if (!gbuf->transfer_buffer) + message->gbuf.transfer_buffer = hd->driver->buffer_alloc(size, gfp_flags); + if (!message->gbuf.transfer_buffer) return -ENOMEM; - gbuf->transfer_buffer_length = size; - gbuf->hd = hd; - gbuf->dest_cport_id = dest_cport_id; - gbuf->status = -EBADR; /* Initial value--means "never set" */ + message->gbuf.transfer_buffer_length = size; + message->gbuf.hd = hd; + message->gbuf.dest_cport_id = dest_cport_id; + message->gbuf.status = -EBADR; /* Initial value--means "never set" */ /* Fill in the header structure */ - header = (struct gb_operation_msg_hdr *)gbuf->transfer_buffer; + header = message->gbuf.transfer_buffer; header->size = cpu_to_le16(size); header->id = 0; /* Filled in when submitted */ header->type = type; From 002fe66a7d8bdbea058025a5804f5e0a375226da Mon Sep 17 00:00:00 2001 From: Alex Elder Date: Tue, 18 Nov 2014 13:26:48 -0600 Subject: [PATCH 11/17] greybus: send messages, not gbufs Rework greybus_submit_gbuf() to be oriented toward an operation message rather than a gbuf, and rename it accordingly. Signed-off-by: Alex Elder Signed-off-by: Greg Kroah-Hartman --- drivers/staging/greybus/operation.c | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/drivers/staging/greybus/operation.c b/drivers/staging/greybus/operation.c index 57694e03c187..d22b9275ba96 100644 --- a/drivers/staging/greybus/operation.c +++ b/drivers/staging/greybus/operation.c @@ -103,17 +103,21 @@ 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; - gbuf->hcd_data = gbuf->hd->driver->buffer_send(gbuf->hd, - gbuf->dest_cport_id, gbuf->transfer_buffer, - gbuf->transfer_buffer_length, gfp_mask); - if (IS_ERR(gbuf->hcd_data)) { - gbuf->status = PTR_ERR(gbuf->hcd_data); - gbuf->hcd_data = NULL; + struct greybus_host_device *hd = message->gbuf.hd; - return gbuf->status; + message->gbuf.status = -EINPROGRESS; + message->gbuf.hcd_data = hd->driver->buffer_send(hd, + message->gbuf.dest_cport_id, + message->gbuf.transfer_buffer, + message->gbuf.transfer_buffer_length, + gfp_mask); + if (IS_ERR(message->gbuf.hcd_data)) { + message->gbuf.status = PTR_ERR(message->gbuf.hcd_data); + message->gbuf.hcd_data = NULL; + + return message->gbuf.status; } return 0; } @@ -390,7 +394,7 @@ int gb_operation_request_send(struct gb_operation *operation, */ 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; From 35b1342bb040a1e12d82b46ae296f660684a2d23 Mon Sep 17 00:00:00 2001 From: Alex Elder Date: Tue, 18 Nov 2014 13:26:49 -0600 Subject: [PATCH 12/17] greybus: cancel messages, not gbufs Rework greybus_kill_gbuf() to be oriented toward an operation message rather than a gbuf, and rename it. Signed-off-by: Alex Elder Signed-off-by: Greg Kroah-Hartman --- drivers/staging/greybus/operation.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/drivers/staging/greybus/operation.c b/drivers/staging/greybus/operation.c index d22b9275ba96..5d5cce68680e 100644 --- a/drivers/staging/greybus/operation.c +++ b/drivers/staging/greybus/operation.c @@ -122,12 +122,12 @@ static int gb_message_send(struct gb_message *message, gfp_t gfp_mask) return 0; } -static void greybus_kill_gbuf(struct gbuf *gbuf) +static void gb_message_cancel(struct gb_message *message) { - if (gbuf->status != -EINPROGRESS) + if (message->gbuf.status != -EINPROGRESS) return; - gbuf->hd->driver->buffer_cancel(gbuf->hcd_data); + message->gbuf.hd->driver->buffer_cancel(message->gbuf.hcd_data); } /* @@ -152,7 +152,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; } @@ -489,9 +489,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); + gb_message_cancel(&operation->request); if (operation->response.gbuf.transfer_buffer) - greybus_kill_gbuf(&operation->response.gbuf); + gb_message_cancel(&operation->response); } int gb_operation_init(void) From 61089e89e50ba10592670518c0f5611c33d64f39 Mon Sep 17 00:00:00 2001 From: Alex Elder Date: Tue, 18 Nov 2014 13:26:50 -0600 Subject: [PATCH 13/17] greybus: rework receve handling Rework gb_connection_operation_recv() to be more oriented toward an operation message, and to no longer use a struct gbuf local variable. Rename it to be a little more wieldy. Signed-off-by: Alex Elder Signed-off-by: Greg Kroah-Hartman --- drivers/staging/greybus/connection.c | 2 +- drivers/staging/greybus/operation.c | 12 ++++++------ drivers/staging/greybus/operation.h | 2 +- 3 files changed, 8 insertions(+), 8 deletions(-) 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/operation.c b/drivers/staging/greybus/operation.c index 5d5cce68680e..254864effe27 100644 --- a/drivers/staging/greybus/operation.c +++ b/drivers/staging/greybus/operation.c @@ -431,12 +431,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) @@ -459,8 +459,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->gbuf.transfer_buffer_length) { operation->result = GB_OP_OVERFLOW; gb_connection_err(connection, "recv buffer too small"); return; @@ -474,10 +474,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->gbuf.transfer_buffer, data, msg_size); /* The rest will be handled in work queue context */ queue_work(gb_operation_recv_workqueue, &operation->recv_work); diff --git a/drivers/staging/greybus/operation.h b/drivers/staging/greybus/operation.h index f43531dbcf33..a9d4b8a1adc3 100644 --- a/drivers/staging/greybus/operation.h +++ b/drivers/staging/greybus/operation.h @@ -87,7 +87,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, From e238e641ee79db947f1f1222204ae12258061d94 Mon Sep 17 00:00:00 2001 From: Alex Elder Date: Tue, 18 Nov 2014 13:26:51 -0600 Subject: [PATCH 14/17] greybus: kill the last gbuf remnants All the code has now been adjusted such that we can do away with the old gbuf structure. Three unused references remained in "greybus.h", so those are deleted. Other than that most of the changes were done by simple global substitution. The gb_message structure incorporates the fields that were previously found its embedded gbuf structure. A few names have been changed in the process: gbuf->transfer_buffer message->buffer gbuf->transfer_buffer_size message->buffer_size gbuf->hcd_data; message->cookie Signed-off-by: Alex Elder Signed-off-by: Greg Kroah-Hartman --- drivers/staging/greybus/greybus.h | 3 -- drivers/staging/greybus/operation.c | 58 ++++++++++++++--------------- drivers/staging/greybus/operation.h | 20 ++++------ 3 files changed, 37 insertions(+), 44 deletions(-) diff --git a/drivers/staging/greybus/greybus.h b/drivers/staging/greybus/greybus.h index 4ac7376fe815..3df2b5a60d80 100644 --- a/drivers/staging/greybus/greybus.h +++ b/drivers/staging/greybus/greybus.h @@ -68,7 +68,6 @@ 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) @@ -156,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 254864effe27..6c082cc16457 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); } @@ -105,29 +105,29 @@ gb_pending_operation_find(struct gb_connection *connection, u16 id) static int gb_message_send(struct gb_message *message, gfp_t gfp_mask) { - struct greybus_host_device *hd = message->gbuf.hd; + struct greybus_host_device *hd = message->hd; - message->gbuf.status = -EINPROGRESS; - message->gbuf.hcd_data = hd->driver->buffer_send(hd, - message->gbuf.dest_cport_id, - message->gbuf.transfer_buffer, - message->gbuf.transfer_buffer_length, + message->status = -EINPROGRESS; + message->cookie = hd->driver->buffer_send(hd, + message->dest_cport_id, + message->buffer, + message->buffer_size, gfp_mask); - if (IS_ERR(message->gbuf.hcd_data)) { - message->gbuf.status = PTR_ERR(message->gbuf.hcd_data); - message->gbuf.hcd_data = NULL; + if (IS_ERR(message->cookie)) { + message->status = PTR_ERR(message->cookie); + message->cookie = NULL; - return message->gbuf.status; + return message->status; } return 0; } static void gb_message_cancel(struct gb_message *message) { - if (message->gbuf.status != -EINPROGRESS) + if (message->status != -EINPROGRESS) return; - message->gbuf.hd->driver->buffer_cancel(message->gbuf.hcd_data); + message->hd->driver->buffer_cancel(message->cookie); } /* @@ -162,7 +162,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 @@ -192,7 +192,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); @@ -251,16 +251,16 @@ static int gb_operation_message_init(struct gb_operation *operation, else dest_cport_id = CPORT_ID_BAD; - message->gbuf.transfer_buffer = hd->driver->buffer_alloc(size, gfp_flags); - if (!message->gbuf.transfer_buffer) + message->buffer = hd->driver->buffer_alloc(size, gfp_flags); + if (!message->buffer) return -ENOMEM; - message->gbuf.transfer_buffer_length = size; - message->gbuf.hd = hd; - message->gbuf.dest_cport_id = dest_cport_id; - message->gbuf.status = -EBADR; /* Initial value--means "never set" */ + message->buffer_size = size; + message->hd = hd; + message->dest_cport_id = dest_cport_id; + message->status = -EBADR; /* Initial value--means "never set" */ /* Fill in the header structure */ - header = message->gbuf.transfer_buffer; + header = message->buffer; header->size = cpu_to_le16(size); header->id = 0; /* Filled in when submitted */ header->type = type; @@ -275,9 +275,9 @@ static void gb_operation_message_exit(struct gb_message *message) { message->operation = NULL; message->payload = NULL; - message->gbuf.hd->driver->buffer_free(message->gbuf.transfer_buffer); - message->gbuf.transfer_buffer = NULL; - message->gbuf.transfer_buffer_length = 0; + message->hd->driver->buffer_free(message->buffer); + message->buffer = NULL; + message->buffer_size = 0; } /* @@ -390,7 +390,7 @@ 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); @@ -460,7 +460,7 @@ void gb_connection_recv(struct gb_connection *connection, cancel_delayed_work(&operation->timeout_work); gb_pending_operation_remove(operation); message = &operation->response; - if (size > message->gbuf.transfer_buffer_length) { + if (size > message->buffer_size) { operation->result = GB_OP_OVERFLOW; gb_connection_err(connection, "recv buffer too small"); return; @@ -477,7 +477,7 @@ void gb_connection_recv(struct gb_connection *connection, message = &operation->request; } - memcpy(message->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); @@ -490,7 +490,7 @@ void gb_operation_cancel(struct gb_operation *operation) { operation->canceled = true; gb_message_cancel(&operation->request); - if (operation->response.gbuf.transfer_buffer) + if (operation->response.buffer) gb_message_cancel(&operation->response); } diff --git a/drivers/staging/greybus/operation.h b/drivers/staging/greybus/operation.h index a9d4b8a1adc3..2fcb181749a9 100644 --- a/drivers/staging/greybus/operation.h +++ b/drivers/staging/greybus/operation.h @@ -24,21 +24,17 @@ 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; + struct greybus_host_device *hd; + u16 dest_cport_id; /* Destination CPort id */ + int status; + + void *buffer; + size_t buffer_size; + + void *cookie; }; /* From 3ed67aba9f3b2af83b9b9cf7cd6f7ab25de5acc2 Mon Sep 17 00:00:00 2001 From: Alex Elder Date: Tue, 18 Nov 2014 13:26:52 -0600 Subject: [PATCH 15/17] greybus: stop storing hd in message The host device pointer doesn't have to be stored in every message. It can be derived by following up the chain of pointers back to the operation's connection. Signed-off-by: Alex Elder Signed-off-by: Greg Kroah-Hartman --- drivers/staging/greybus/operation.c | 16 +++++++++++----- drivers/staging/greybus/operation.h | 2 +- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/drivers/staging/greybus/operation.c b/drivers/staging/greybus/operation.c index 6c082cc16457..23cf745a337a 100644 --- a/drivers/staging/greybus/operation.c +++ b/drivers/staging/greybus/operation.c @@ -105,10 +105,10 @@ gb_pending_operation_find(struct gb_connection *connection, u16 id) static int gb_message_send(struct gb_message *message, gfp_t gfp_mask) { - struct greybus_host_device *hd = message->hd; + struct gb_connection *connection = message->operation->connection; message->status = -EINPROGRESS; - message->cookie = hd->driver->buffer_send(hd, + message->cookie = connection->hd->driver->buffer_send(connection->hd, message->dest_cport_id, message->buffer, message->buffer_size, @@ -124,10 +124,13 @@ static int gb_message_send(struct gb_message *message, gfp_t gfp_mask) static void gb_message_cancel(struct gb_message *message) { + struct greybus_host_device *hd; + if (message->status != -EINPROGRESS) return; - message->hd->driver->buffer_cancel(message->cookie); + hd = message->operation->connection->hd; + hd->driver->buffer_cancel(message->cookie); } /* @@ -255,7 +258,6 @@ static int gb_operation_message_init(struct gb_operation *operation, if (!message->buffer) return -ENOMEM; message->buffer_size = size; - message->hd = hd; message->dest_cport_id = dest_cport_id; message->status = -EBADR; /* Initial value--means "never set" */ @@ -273,9 +275,13 @@ 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->hd->driver->buffer_free(message->buffer); message->buffer = NULL; message->buffer_size = 0; } diff --git a/drivers/staging/greybus/operation.h b/drivers/staging/greybus/operation.h index 2fcb181749a9..5e068ff9f546 100644 --- a/drivers/staging/greybus/operation.h +++ b/drivers/staging/greybus/operation.h @@ -26,8 +26,8 @@ enum gb_operation_status { struct gb_message { void *payload; + struct gb_operation *operation; - struct greybus_host_device *hd; u16 dest_cport_id; /* Destination CPort id */ int status; From 1f764af77c6adb3b4035b8f41b48198f251dc7f8 Mon Sep 17 00:00:00 2001 From: Alex Elder Date: Tue, 18 Nov 2014 13:26:53 -0600 Subject: [PATCH 16/17] greybus: stop storing dest_cport_id in message We can derive the destination CPort id of any (outbound) message from the connection it's operation is associated with. So we don't need to store that information in every message. As a result, we no longer need to record it at message initialization time. Signed-off-by: Alex Elder Signed-off-by: Greg Kroah-Hartman --- drivers/staging/greybus/operation.c | 10 ++-------- drivers/staging/greybus/operation.h | 1 - 2 files changed, 2 insertions(+), 9 deletions(-) diff --git a/drivers/staging/greybus/operation.c b/drivers/staging/greybus/operation.c index 23cf745a337a..705b195dfe01 100644 --- a/drivers/staging/greybus/operation.c +++ b/drivers/staging/greybus/operation.c @@ -106,10 +106,11 @@ gb_pending_operation_find(struct gb_connection *connection, u16 id) static int gb_message_send(struct gb_message *message, gfp_t gfp_mask) { struct gb_connection *connection = message->operation->connection; + u16 dest_cport_id = connection->interface_cport_id; message->status = -EINPROGRESS; message->cookie = connection->hd->driver->buffer_send(connection->hd, - message->dest_cport_id, + dest_cport_id, message->buffer, message->buffer_size, gfp_mask); @@ -236,7 +237,6 @@ static int gb_operation_message_init(struct gb_operation *operation, struct gb_message *message; struct gb_operation_msg_hdr *header; gfp_t gfp_flags = request && !outbound ? GFP_ATOMIC : GFP_KERNEL; - u16 dest_cport_id; if (size > GB_OPERATION_MESSAGE_SIZE_MAX) return -E2BIG; @@ -249,16 +249,10 @@ static int gb_operation_message_init(struct gb_operation *operation, type |= GB_OPERATION_TYPE_RESPONSE; } - if (outbound) - dest_cport_id = connection->interface_cport_id; - else - dest_cport_id = CPORT_ID_BAD; - message->buffer = hd->driver->buffer_alloc(size, gfp_flags); if (!message->buffer) return -ENOMEM; message->buffer_size = size; - message->dest_cport_id = dest_cport_id; message->status = -EBADR; /* Initial value--means "never set" */ /* Fill in the header structure */ diff --git a/drivers/staging/greybus/operation.h b/drivers/staging/greybus/operation.h index 5e068ff9f546..81fd7f70b8ba 100644 --- a/drivers/staging/greybus/operation.h +++ b/drivers/staging/greybus/operation.h @@ -28,7 +28,6 @@ struct gb_message { void *payload; struct gb_operation *operation; - u16 dest_cport_id; /* Destination CPort id */ int status; void *buffer; From de80073a1768b0fb01df0e597225047fd66e8044 Mon Sep 17 00:00:00 2001 From: Alex Elder Date: Tue, 18 Nov 2014 13:26:54 -0600 Subject: [PATCH 17/17] greybus: pass gfp_flags for message allocation The only reason gb_operation_message_init() gets its "outbound" argument is so we can determine what allocation flags to use. Just pass the flags in directly instead. Signed-off-by: Alex Elder Signed-off-by: Greg Kroah-Hartman --- drivers/staging/greybus/operation.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/drivers/staging/greybus/operation.c b/drivers/staging/greybus/operation.c index 705b195dfe01..96f4c689e998 100644 --- a/drivers/staging/greybus/operation.c +++ b/drivers/staging/greybus/operation.c @@ -230,13 +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 outbound) + 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; - gfp_t gfp_flags = request && !outbound ? GFP_ATOMIC : GFP_KERNEL; if (size > GB_OPERATION_MESSAGE_SIZE_MAX) return -E2BIG; @@ -311,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; }