greybus: have greybus allocate its own buffers

Rather than having the host driver allocate the buffers that the
Greybus core uses to hold its data for sending or receiving, have
the host driver define what it requires those buffers to look like.

Two constraints define what the host driver requires: the maximum
number of bytes that the host device can send in a single request;
and a statement of the "headroom" that needs to be present for
use by the host device.

The direct description of the headroom is that it's the extra byte
the host device needs at the beginning of the "data" portion of
the buffer so the ES1 driver can insert the destination CPort id.
But more generally, the host driver could put other data in there
as well.

By stating these two parameters, Greybus can allocate the buffers it
uses by itself.  The host driver still allocates the buffers it uses
for receiving data--the content of those are copied as needed into
Greybus buffers when data arrives.
Signed-off-by: Alex Elder <elder@linaro.org>
Signed-off-by: Greg Kroah-Hartman <greg@kroah.com>

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->buffer_free) ||
-	    (!driver->buffer_send) || (!driver->buffer_cancel) ||
+	if ((!driver->buffer_send) || (!driver->buffer_cancel) ||
 	    (!driver->submit_svc)) {
 		pr_err("Must implement all greybus_host_driver callbacks!\n");
 		return NULL;

drivers/staging/greybus/es1-ap-usb.c

@@ -25,7 +25,7 @@
 
 /* Memory sizes for the buffers sent to/from the ES1 controller */
 #define ES1_SVC_MSG_SIZE	(sizeof(struct svc_msg) + SZ_64K)
-#define ES1_GBUF_MSG_SIZE	PAGE_SIZE
+#define ES1_GBUF_MSG_SIZE_MAX	PAGE_SIZE
 
 static const struct usb_device_id id_table[] = {
 	/* Made up numbers for the SVC USB Bridge in ES1 */
@@ -92,47 +92,41 @@ static inline struct es1_ap_dev *hd_to_es1(struct greybus_host_device *hd)
 static void cport_out_callback(struct urb *urb);
 
 /*
- * Allocate a buffer to be sent via UniPro.
- */
-static void *buffer_alloc(unsigned int size, gfp_t gfp_mask)
-{
-	u8 *buffer;
-
-	if (size > ES1_GBUF_MSG_SIZE) {
-		pr_err("guf was asked to be bigger than %ld!\n",
-		       ES1_GBUF_MSG_SIZE);
-	}
-
-	/*
-	 * 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.
+ * Buffer constraints for the host driver.
  *
-	 * This is only needed for outbound data, but we handle
-	 * buffers for inbound data the same way for consistency.
+ * A "buffer" is used to hold data to be transferred for Greybus by
+ * the host driver.  A buffer is represented by a "buffer pointer",
+ * which defines a region of memory used by the host driver for
+ * transferring the data.  When Greybus allocates a buffer, it must
+ * do so subject to the constraints associated with the host driver.
+ * These constraints are specified by two parameters: the
+ * headroom; and the maximum buffer size.
  *
-	 * XXX Do we need to indicate the destination device id too?
+ *			+------------------+
+ *			|    Host driver   | \
+ *			|   reserved area  |  }- headroom
+ *			|      . . .       | /
+ *  buffer pointer ---> +------------------+
+ *			| Buffer space for | \
+ *			| transferred data |  }- buffer size
+ *			|      . . .       | /   (limited to size_max)
+ *			+------------------+
+ *
+ *  headroom:	Every buffer must have at least this much space
+ *		*before* the buffer pointer, reserved for use by the
+ *		host driver.  I.e., ((char *)buffer - headroom) must
+ *		point to valid memory, usable only by the host driver.
+ *  size_max:	The maximum size of a buffer (not including the
+ *  		headroom) must not exceed this.
  */
-	buffer = kzalloc(GB_BUFFER_ALIGN + size, gfp_mask);
-	if (buffer)
-		buffer += GB_BUFFER_ALIGN;
-
-	return buffer;
-}
-
-/* Free a previously-allocated buffer */
-static void buffer_free(void *buffer)
+static void hd_buffer_constraints(struct greybus_host_device *hd)
 {
-	u8 *allocated = 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 */
-	allocated -= GB_BUFFER_ALIGN;
-	kfree(allocated);
+	/*
+	 * Only one byte is required, but this produces a result
+	 * that's better aligned for the user.
+	 */
+	hd->buffer_headroom = sizeof(u32);	/* For cport id */
+	hd->buffer_size_max = ES1_GBUF_MSG_SIZE_MAX;
 }
 
 #define ES1_TIMEOUT	500	/* 500 ms for the SVC to do something */
@@ -265,8 +259,6 @@ static void buffer_cancel(void *cookie)
 
 static struct greybus_host_driver es1_driver = {
 	.hd_priv_size		= sizeof(struct es1_ap_dev),
-	.buffer_alloc		= buffer_alloc,
-	.buffer_free		= buffer_free,
 	.buffer_send		= buffer_send,
 	.buffer_cancel		= buffer_cancel,
 	.submit_svc		= submit_svc,
@@ -493,6 +485,9 @@ static int ap_probe(struct usb_interface *interface,
 		return -ENOMEM;
 	}
 
+	/* Fill in the buffer allocation constraints */
+	hd_buffer_constraints(hd);
+
 	es1 = hd_to_es1(hd);
 	es1->hd = hd;
 	es1->usb_intf = interface;
@@ -557,14 +552,14 @@ static int ap_probe(struct usb_interface *interface,
 		urb = usb_alloc_urb(0, GFP_KERNEL);
 		if (!urb)
 			goto error;
-		buffer = kmalloc(ES1_GBUF_MSG_SIZE, GFP_KERNEL);
+		buffer = kmalloc(ES1_GBUF_MSG_SIZE_MAX, GFP_KERNEL);
 		if (!buffer)
 			goto error;
 
 		usb_fill_bulk_urb(urb, udev,
 				  usb_rcvbulkpipe(udev, es1->cport_in_endpoint),
-				  buffer, ES1_GBUF_MSG_SIZE, cport_in_callback,
-				  hd);
+				  buffer, ES1_GBUF_MSG_SIZE_MAX,
+				  cport_in_callback, hd);
 		es1->cport_in_urb[i] = urb;
 		es1->cport_in_buffer[i] = buffer;
 		retval = usb_submit_urb(urb, GFP_KERNEL);

drivers/staging/greybus/greybus.h

@@ -78,8 +78,6 @@ struct svc_msg;
 struct greybus_host_driver {
 	size_t	hd_priv_size;
 
-	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);
@@ -97,6 +95,10 @@ struct greybus_host_device {
 	struct ida cport_id_map;
 	u8 device_id;
 
+	/* Host device buffer constraints */
+	size_t buffer_headroom;
+	size_t buffer_size_max;
+
 	/* Private data for the host driver */
 	unsigned long hd_priv[0] __aligned(sizeof(s64));
 };

drivers/staging/greybus/operation.c

@@ -229,6 +229,27 @@ static void operation_timeout(struct work_struct *work)
 	gb_operation_complete(operation);
 }
 
+static void *
+gb_buffer_alloc(struct greybus_host_device *hd, size_t size, gfp_t gfp_flags)
+{
+	u8 *buffer;
+
+	buffer = kzalloc(hd->buffer_headroom + size, gfp_flags);
+	if (buffer)
+		buffer += hd->buffer_headroom;
+
+	return buffer;
+}
+
+static void
+gb_buffer_free(struct greybus_host_device *hd, void *buffer)
+{
+	u8 *allocated = buffer;
+
+	if (allocated)
+		kfree(allocated - hd->buffer_headroom);
+}
+
 /*
  * Allocate a buffer to be used for an operation request or response
  * message.  For outgoing messages, both types of message contain a
@@ -246,9 +267,9 @@ static int gb_operation_message_init(struct gb_operation *operation,
 	struct gb_message *message;
 	struct gb_operation_msg_hdr *header;
 
-	if (size > GB_OPERATION_MESSAGE_SIZE_MAX)
-		return -E2BIG;
 	size += sizeof(*header);
+	if (size > hd->buffer_size_max)
+		return -E2BIG;
 
 	if (request) {
 		message = &operation->request;
@@ -257,7 +278,7 @@ static int gb_operation_message_init(struct gb_operation *operation,
 		type |= GB_OPERATION_TYPE_RESPONSE;
 	}
 
-	message->buffer = hd->driver->buffer_alloc(size, gfp_flags);
+	message->buffer = gb_buffer_alloc(hd, size, gfp_flags);
 	if (!message->buffer)
 		return -ENOMEM;
 	message->buffer_size = size;
@@ -279,7 +300,7 @@ 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);
+	gb_buffer_free(hd, message->buffer);
 
 	message->operation = NULL;
 	message->payload = NULL;