From 821c620afa1ad29be6a85a9a3f691e32e973a317 Mon Sep 17 00:00:00 2001 From: Alex Elder Date: Sat, 13 Jun 2015 11:02:07 -0500 Subject: [PATCH 1/5] greybus: introduce cport_id_valid() Define a public predicate that defines whether a CPort ID is valid. Use it in the message_send() routine, and make the message reported more accurately reflect the error. Also use it to check whether the CPort ID in a received message is valid; if it is not, just drop the message. Get rid of local variable "buffer" in message_send(); it adds no value. Signed-off-by: Alex Elder Signed-off-by: Greg Kroah-Hartman --- drivers/staging/greybus/es1.c | 18 ++++++++++-------- drivers/staging/greybus/es2.c | 18 ++++++++++-------- drivers/staging/greybus/greybus.h | 5 +++++ 3 files changed, 25 insertions(+), 16 deletions(-) diff --git a/drivers/staging/greybus/es1.c b/drivers/staging/greybus/es1.c index 56c80c6ddb3a..e68ee48e8189 100644 --- a/drivers/staging/greybus/es1.c +++ b/drivers/staging/greybus/es1.c @@ -176,21 +176,17 @@ static void *message_send(struct greybus_host_device *hd, u16 cport_id, { struct es1_ap_dev *es1 = hd_to_es1(hd); struct usb_device *udev = es1->usb_dev; - void *buffer; size_t buffer_size; int retval; struct urb *urb; - buffer = message->buffer; - buffer_size = sizeof(*message->header) + message->payload_size; - /* * 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 (cport_id == CPORT_ID_BAD) { - pr_err("request to send inbound data buffer\n"); + if (!cport_id_valid(cport_id)) { + pr_err("invalid destination cport 0x%02x\n", cport_id); return ERR_PTR(-EINVAL); } @@ -205,9 +201,11 @@ static void *message_send(struct greybus_host_device *hd, u16 cport_id, */ put_unaligned_le16(cport_id, message->header->pad); + buffer_size = sizeof(*message->header) + message->payload_size; + usb_fill_bulk_urb(urb, udev, usb_sndbulkpipe(udev, es1->cport_out_endpoint), - buffer, buffer_size, + message->buffer, buffer_size, cport_out_callback, message); retval = usb_submit_urb(urb, gfp_mask); if (retval) { @@ -371,8 +369,12 @@ static void cport_in_callback(struct urb *urb) cport_id = get_unaligned_le16(header->pad); put_unaligned_le16(0, header->pad); - greybus_data_rcvd(hd, cport_id, urb->transfer_buffer, + if (cport_id_valid(cport_id)) + greybus_data_rcvd(hd, cport_id, urb->transfer_buffer, urb->actual_length); + else + dev_err(dev, "%s: invalid cport id 0x%02x received\n", + __func__, cport_id); exit: /* put our urb back in the request pool */ retval = usb_submit_urb(urb, GFP_ATOMIC); diff --git a/drivers/staging/greybus/es2.c b/drivers/staging/greybus/es2.c index 5257779f7aed..daa2e5176045 100644 --- a/drivers/staging/greybus/es2.c +++ b/drivers/staging/greybus/es2.c @@ -176,21 +176,17 @@ static void *message_send(struct greybus_host_device *hd, u16 cport_id, { struct es1_ap_dev *es1 = hd_to_es1(hd); struct usb_device *udev = es1->usb_dev; - void *buffer; size_t buffer_size; int retval; struct urb *urb; - buffer = message->buffer; - buffer_size = sizeof(*message->header) + message->payload_size; - /* * 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 (cport_id == CPORT_ID_BAD) { - pr_err("request to send inbound data buffer\n"); + if (!cport_id_valid(cport_id)) { + pr_err("invalid destination cport 0x%02x\n", cport_id); return ERR_PTR(-EINVAL); } @@ -205,9 +201,11 @@ static void *message_send(struct greybus_host_device *hd, u16 cport_id, */ put_unaligned_le16(cport_id, message->header->pad); + buffer_size = sizeof(*message->header) + message->payload_size; + usb_fill_bulk_urb(urb, udev, usb_sndbulkpipe(udev, es1->cport_out_endpoint), - buffer, buffer_size, + message->buffer, buffer_size, cport_out_callback, message); retval = usb_submit_urb(urb, gfp_mask); if (retval) { @@ -371,8 +369,12 @@ static void cport_in_callback(struct urb *urb) cport_id = get_unaligned_le16(header->pad); put_unaligned_le16(0, header->pad); - greybus_data_rcvd(hd, cport_id, urb->transfer_buffer, + if (cport_id_valid(cport_id)) + greybus_data_rcvd(hd, cport_id, urb->transfer_buffer, urb->actual_length); + else + dev_err(dev, "%s: invalid cport id 0x%02x received\n", + __func__, cport_id); exit: /* put our urb back in the request pool */ retval = usb_submit_urb(urb, GFP_ATOMIC); diff --git a/drivers/staging/greybus/greybus.h b/drivers/staging/greybus/greybus.h index 05eab4c5f9a0..518f142b6b5f 100644 --- a/drivers/staging/greybus/greybus.h +++ b/drivers/staging/greybus/greybus.h @@ -196,5 +196,10 @@ static inline int is_gb_connection(const struct device *dev) return dev->type == &greybus_connection_type; } +static inline bool cport_id_valid(u16 cport_id) +{ + return cport_id != CPORT_ID_BAD; +} + #endif /* __KERNEL__ */ #endif /* __LINUX_GREYBUS_H */ From d29b3d631e572400b45b5f9e48e432493663b0fc Mon Sep 17 00:00:00 2001 From: Alex Elder Date: Sat, 13 Jun 2015 11:02:08 -0500 Subject: [PATCH 2/5] greybus: esX: encapsulate packing cport id into header For the ES1 and ES2 host interfaces we encode the CPort ID over which the message should be sent within the message itself. The CPort ID is recorded in unused pad bytes found in the operation message header in order to avoid introducing misaligned messages. This patch defines some helper routines to abstract this activity. Signed-off-by: Alex Elder Signed-off-by: Greg Kroah-Hartman --- drivers/staging/greybus/es1.c | 42 ++++++++++++++++++++++++++--------- drivers/staging/greybus/es2.c | 42 ++++++++++++++++++++++++++--------- 2 files changed, 64 insertions(+), 20 deletions(-) diff --git a/drivers/staging/greybus/es1.c b/drivers/staging/greybus/es1.c index e68ee48e8189..945dbe658ed1 100644 --- a/drivers/staging/greybus/es1.c +++ b/drivers/staging/greybus/es1.c @@ -166,6 +166,32 @@ static void free_urb(struct es1_ap_dev *es1, struct urb *urb) usb_free_urb(urb); } +/* + * We (ab)use the operation-message header pad bytes to transfer the + * cport id in order to minimise overhead. + */ +static void +gb_message_cport_pack(struct gb_operation_msg_hdr *header, u16 cport_id) +{ + put_unaligned_le16(cport_id, header->pad); +} + +/* Clear the pad bytes used for the CPort id */ +static void gb_message_cport_clear(struct gb_operation_msg_hdr *header) +{ + put_unaligned_le16(0, header->pad); +} + +/* Extract the CPort id packed into the header, and clear it */ +static u16 gb_message_cport_unpack(struct gb_operation_msg_hdr *header) +{ + u16 cport_id = get_unaligned_le16(header->pad); + + gb_message_cport_clear(header); + + return cport_id; +} + /* * Returns an opaque cookie value if successful, or a pointer coded * error otherwise. If the caller wishes to cancel the in-flight @@ -195,11 +221,8 @@ static void *message_send(struct greybus_host_device *hd, u16 cport_id, if (!urb) return ERR_PTR(-ENOMEM); - /* - * We (ab)use the operation-message header pad bytes to transfer the - * cport id in order to minimise overhead. - */ - put_unaligned_le16(cport_id, message->header->pad); + /* Pack the cport id into the message header */ + gb_message_cport_pack(message->header, cport_id); buffer_size = sizeof(*message->header) + message->payload_size; @@ -211,7 +234,7 @@ static void *message_send(struct greybus_host_device *hd, u16 cport_id, if (retval) { pr_err("error %d submitting URB\n", retval); free_urb(es1, urb); - put_unaligned_le16(0, message->header->pad); + gb_message_cport_clear(message->header); return ERR_PTR(retval); } @@ -365,9 +388,9 @@ static void cport_in_callback(struct urb *urb) goto exit; } + /* Extract the CPort id, which is packed in the message header */ header = urb->transfer_buffer; - cport_id = get_unaligned_le16(header->pad); - put_unaligned_le16(0, header->pad); + cport_id = gb_message_cport_unpack(header); if (cport_id_valid(cport_id)) greybus_data_rcvd(hd, cport_id, urb->transfer_buffer, @@ -390,8 +413,7 @@ static void cport_out_callback(struct urb *urb) struct es1_ap_dev *es1 = hd_to_es1(hd); int status = check_urb_status(urb); - /* Clear the pad bytes used for the cport id */ - put_unaligned_le16(0, message->header->pad); + gb_message_cport_clear(message->header); /* * Tell the submitter that the message send (attempt) is diff --git a/drivers/staging/greybus/es2.c b/drivers/staging/greybus/es2.c index daa2e5176045..d6966bf0a262 100644 --- a/drivers/staging/greybus/es2.c +++ b/drivers/staging/greybus/es2.c @@ -166,6 +166,32 @@ static void free_urb(struct es1_ap_dev *es1, struct urb *urb) usb_free_urb(urb); } +/* + * We (ab)use the operation-message header pad bytes to transfer the + * cport id in order to minimise overhead. + */ +static void +gb_message_cport_pack(struct gb_operation_msg_hdr *header, u16 cport_id) +{ + put_unaligned_le16(cport_id, header->pad); +} + +/* Clear the pad bytes used for the CPort id */ +static void gb_message_cport_clear(struct gb_operation_msg_hdr *header) +{ + put_unaligned_le16(0, header->pad); +} + +/* Extract the CPort id packed into the header, and clear it */ +static u16 gb_message_cport_unpack(struct gb_operation_msg_hdr *header) +{ + u16 cport_id = get_unaligned_le16(header->pad); + + gb_message_cport_clear(header); + + return cport_id; +} + /* * Returns an opaque cookie value if successful, or a pointer coded * error otherwise. If the caller wishes to cancel the in-flight @@ -195,11 +221,8 @@ static void *message_send(struct greybus_host_device *hd, u16 cport_id, if (!urb) return ERR_PTR(-ENOMEM); - /* - * We (ab)use the operation-message header pad bytes to transfer the - * cport id in order to minimise overhead. - */ - put_unaligned_le16(cport_id, message->header->pad); + /* Pack the cport id into the message header */ + gb_message_cport_pack(message->header, cport_id); buffer_size = sizeof(*message->header) + message->payload_size; @@ -211,7 +234,7 @@ static void *message_send(struct greybus_host_device *hd, u16 cport_id, if (retval) { pr_err("error %d submitting URB\n", retval); free_urb(es1, urb); - put_unaligned_le16(0, message->header->pad); + gb_message_cport_clear(message->header); return ERR_PTR(retval); } @@ -365,9 +388,9 @@ static void cport_in_callback(struct urb *urb) goto exit; } + /* Extract the CPort id, which is packed in the message header */ header = urb->transfer_buffer; - cport_id = get_unaligned_le16(header->pad); - put_unaligned_le16(0, header->pad); + cport_id = gb_message_cport_unpack(header); if (cport_id_valid(cport_id)) greybus_data_rcvd(hd, cport_id, urb->transfer_buffer, @@ -390,8 +413,7 @@ static void cport_out_callback(struct urb *urb) struct es1_ap_dev *es1 = hd_to_es1(hd); int status = check_urb_status(urb); - /* Clear the pad bytes used for the cport id */ - put_unaligned_le16(0, message->header->pad); + gb_message_cport_clear(message->header); /* * Tell the submitter that the message send (attempt) is From fb690ca96f9a7aff8bc8135ce23606e243f4cbae Mon Sep 17 00:00:00 2001 From: Alex Elder Date: Sat, 13 Jun 2015 11:02:09 -0500 Subject: [PATCH 3/5] greybus: rename HOST_DEV_CPORT_ID_MAX We limit the number of host-side CPorts to a fixed maximum (which is less than the 4096 that UniPro allows). This patch imposes a similar limit on the CPort IDs defined by modules (signaling an error if one too large is found in a manifest). It seems reasonable to use the same value for both limits. Change the name of the constant that defines the host limit and use it for both. Update cport_id_valid() to enforce the maximum. (Ultimately we should impose a limit like this; this change is being made in preparation for supporting multiple connections over a single CPort.) Signed-off-by: Alex Elder Signed-off-by: Greg Kroah-Hartman --- drivers/staging/greybus/connection.c | 2 +- drivers/staging/greybus/core.c | 2 +- drivers/staging/greybus/greybus.h | 12 ++++++------ drivers/staging/greybus/manifest.c | 5 ++++- 4 files changed, 12 insertions(+), 9 deletions(-) diff --git a/drivers/staging/greybus/connection.c b/drivers/staging/greybus/connection.c index 7236e47ea9c8..9467aaba2b32 100644 --- a/drivers/staging/greybus/connection.c +++ b/drivers/staging/greybus/connection.c @@ -158,7 +158,7 @@ struct gb_connection *gb_connection_create(struct gb_bundle *bundle, if (!connection) return NULL; - retval = ida_simple_get(id_map, 0, HOST_DEV_CPORT_ID_MAX, GFP_KERNEL); + retval = ida_simple_get(id_map, 0, CPORT_ID_MAX, GFP_KERNEL); if (retval < 0) { kfree(connection); return NULL; diff --git a/drivers/staging/greybus/core.c b/drivers/staging/greybus/core.c index 18b5d5f48398..d4fffecb1abc 100644 --- a/drivers/staging/greybus/core.c +++ b/drivers/staging/greybus/core.c @@ -246,7 +246,7 @@ static int __init gb_init(void) if (greybus_disabled()) return -ENODEV; - BUILD_BUG_ON(HOST_DEV_CPORT_ID_MAX >= (long)CPORT_ID_BAD); + BUILD_BUG_ON(CPORT_ID_MAX >= (long)CPORT_ID_BAD); gb_debugfs_init(); diff --git a/drivers/staging/greybus/greybus.h b/drivers/staging/greybus/greybus.h index 518f142b6b5f..5c6f9607cf73 100644 --- a/drivers/staging/greybus/greybus.h +++ b/drivers/staging/greybus/greybus.h @@ -52,14 +52,14 @@ .serial_number = (s), /* XXX I couldn't get my Kconfig file to be noticed for out-of-tree build */ -#ifndef CONFIG_HOST_DEV_CPORT_ID_MAX -#define CONFIG_HOST_DEV_CPORT_ID_MAX 128 -#endif /* !CONFIG_HOST_DEV_CPORT_ID_MAX */ +#ifndef CONFIG_CPORT_ID_MAX +#define CONFIG_CPORT_ID_MAX 128 +#endif /* !CONFIG_CPORT_ID_MAX */ /* Maximum number of CPorts usable by a host device */ /* XXX This should really be determined by the AP module manifest */ -#define HOST_DEV_CPORT_ID_MAX CONFIG_HOST_DEV_CPORT_ID_MAX -#define CPORT_ID_BAD U16_MAX /* UniPro max id is 4095 */ +#define CPORT_ID_MAX CONFIG_CPORT_ID_MAX +#define CPORT_ID_BAD U16_MAX /* UniPro max id is 4095 */ /* For SP1 hardware, we are going to "hardcode" each device to have all logical * blocks in order to be able to address them as one unified "unit". Then @@ -198,7 +198,7 @@ static inline int is_gb_connection(const struct device *dev) static inline bool cport_id_valid(u16 cport_id) { - return cport_id != CPORT_ID_BAD; + return cport_id != CPORT_ID_BAD && cport_id <= CPORT_ID_MAX; } #endif /* __KERNEL__ */ diff --git a/drivers/staging/greybus/manifest.c b/drivers/staging/greybus/manifest.c index e329f3763176..ad55a73b737b 100644 --- a/drivers/staging/greybus/manifest.c +++ b/drivers/staging/greybus/manifest.c @@ -224,9 +224,12 @@ static u32 gb_manifest_parse_cports(struct gb_bundle *bundle) if (desc_cport->bundle != bundle_id) continue; + cport_id = le16_to_cpu(desc_cport->id); + if (cport_id > CPORT_ID_MAX) + goto cleanup; + /* Found one. Set up its function structure */ protocol_id = desc_cport->protocol_id; - cport_id = le16_to_cpu(desc_cport->id); if (!gb_connection_create(bundle, cport_id, protocol_id)) goto cleanup; From 88d18a975ddac4fa849909c2ae178b0c126f7f8e Mon Sep 17 00:00:00 2001 From: Alex Elder Date: Sat, 13 Jun 2015 11:02:10 -0500 Subject: [PATCH 4/5] greybus: reserve host cport id 0 For ES1 and ES2, we use pad bytes in an operation message header to encode the CPort ID used for transferring the message. The pad bytes should otherwise be zero, and we ensure this as the message is passed to or from the upper layer. If host-side CPort ID 0 is used, we have no way of knowing whether the CPort field has been "packed" into the header. To allow detection of this, reserve host CPort id 0. Update cport_id_valid() to treat 0 as invalid. (CPort ID 0 is reserved by one of the UniPro standards. We'll assume for now that we never use it for Greybus.) Signed-off-by: Alex Elder Signed-off-by: Greg Kroah-Hartman --- drivers/staging/greybus/core.c | 10 +++++++++- drivers/staging/greybus/greybus.h | 2 +- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/drivers/staging/greybus/core.c b/drivers/staging/greybus/core.c index d4fffecb1abc..bc9c1ebf7983 100644 --- a/drivers/staging/greybus/core.c +++ b/drivers/staging/greybus/core.c @@ -170,6 +170,7 @@ struct greybus_host_device *greybus_create_hd(struct greybus_host_driver *driver size_t buffer_size_max) { struct greybus_host_device *hd; + int ret; /* * Validate that the driver implements all of the callbacks @@ -200,12 +201,19 @@ struct greybus_host_device *greybus_create_hd(struct greybus_host_driver *driver if (!hd) return ERR_PTR(-ENOMEM); + ida_init(&hd->cport_id_map); + /* Reserve CPort id 0 */ + ret = ida_simple_get(&hd->cport_id_map, 0, 1, GFP_KERNEL); + if (ret < 0) { + kfree(hd); + return ERR_PTR(ret); + } + kref_init(&hd->kref); hd->parent = parent; hd->driver = driver; INIT_LIST_HEAD(&hd->interfaces); INIT_LIST_HEAD(&hd->connections); - ida_init(&hd->cport_id_map); hd->buffer_size_max = buffer_size_max; return hd; diff --git a/drivers/staging/greybus/greybus.h b/drivers/staging/greybus/greybus.h index 5c6f9607cf73..d727dea6847e 100644 --- a/drivers/staging/greybus/greybus.h +++ b/drivers/staging/greybus/greybus.h @@ -198,7 +198,7 @@ static inline int is_gb_connection(const struct device *dev) static inline bool cport_id_valid(u16 cport_id) { - return cport_id != CPORT_ID_BAD && cport_id <= CPORT_ID_MAX; + return cport_id && cport_id != CPORT_ID_BAD && cport_id <= CPORT_ID_MAX; } #endif /* __KERNEL__ */ From 4bc1389de9fcaaaf2f93e278f588858fb4fe2038 Mon Sep 17 00:00:00 2001 From: Alex Elder Date: Sat, 13 Jun 2015 11:02:11 -0500 Subject: [PATCH 5/5] greybus: esX: use one byte to encode cport ids in header We now limit the maximum value for both host and module CPort ids, and we know they can always be represented in a single byte. Make use of this by using only one of the two pad bytes for encoding the CPort id in a message header. (Note that we have never used a CPort higher than 255. Encoding such a small CPort id in little endian 2-byte format has the same result as what is done here.) Signed-off-by: Alex Elder Signed-off-by: Greg Kroah-Hartman --- drivers/staging/greybus/es1.c | 9 ++++++--- drivers/staging/greybus/es2.c | 9 ++++++--- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/drivers/staging/greybus/es1.c b/drivers/staging/greybus/es1.c index 945dbe658ed1..d6824b6a83b4 100644 --- a/drivers/staging/greybus/es1.c +++ b/drivers/staging/greybus/es1.c @@ -173,19 +173,19 @@ static void free_urb(struct es1_ap_dev *es1, struct urb *urb) static void gb_message_cport_pack(struct gb_operation_msg_hdr *header, u16 cport_id) { - put_unaligned_le16(cport_id, header->pad); + header->pad[0] = cport_id; } /* Clear the pad bytes used for the CPort id */ static void gb_message_cport_clear(struct gb_operation_msg_hdr *header) { - put_unaligned_le16(0, header->pad); + header->pad[0] = 0; } /* Extract the CPort id packed into the header, and clear it */ static u16 gb_message_cport_unpack(struct gb_operation_msg_hdr *header) { - u16 cport_id = get_unaligned_le16(header->pad); + u16 cport_id = header->pad[0]; gb_message_cport_clear(header); @@ -574,6 +574,9 @@ static int ap_probe(struct usb_interface *interface, u8 ap_intf_id = 0x01; // FIXME - get endo "ID" from the SVC u8 svc_interval = 0; + /* We need to fit a CPort ID in one byte of a message header */ + BUILD_BUG_ON(CPORT_ID_MAX > U8_MAX); + udev = usb_get_dev(interface_to_usbdev(interface)); hd = greybus_create_hd(&es1_driver, &udev->dev, ES1_GBUF_MSG_SIZE_MAX); diff --git a/drivers/staging/greybus/es2.c b/drivers/staging/greybus/es2.c index d6966bf0a262..450a0162e8ad 100644 --- a/drivers/staging/greybus/es2.c +++ b/drivers/staging/greybus/es2.c @@ -173,19 +173,19 @@ static void free_urb(struct es1_ap_dev *es1, struct urb *urb) static void gb_message_cport_pack(struct gb_operation_msg_hdr *header, u16 cport_id) { - put_unaligned_le16(cport_id, header->pad); + header->pad[0] = cport_id; } /* Clear the pad bytes used for the CPort id */ static void gb_message_cport_clear(struct gb_operation_msg_hdr *header) { - put_unaligned_le16(0, header->pad); + header->pad[0] = 0; } /* Extract the CPort id packed into the header, and clear it */ static u16 gb_message_cport_unpack(struct gb_operation_msg_hdr *header) { - u16 cport_id = get_unaligned_le16(header->pad); + u16 cport_id = header->pad[0]; gb_message_cport_clear(header); @@ -574,6 +574,9 @@ static int ap_probe(struct usb_interface *interface, u8 ap_intf_id = 0x01; // FIXME - get endo "ID" from the SVC u8 svc_interval = 0; + /* We need to fit a CPort ID in one byte of a message header */ + BUILD_BUG_ON(CPORT_ID_MAX > U8_MAX); + udev = usb_get_dev(interface_to_usbdev(interface)); hd = greybus_create_hd(&es1_driver, &udev->dev, ES1_GBUF_MSG_SIZE_MAX);