From 3aa6a088780ea014a0b4ca50a928fbe5b640b352 Mon Sep 17 00:00:00 2001 From: Raghavendra Rao Ananta Date: Mon, 16 Mar 2020 20:53:50 -0700 Subject: [PATCH 1/2] haven: msgq: Make the send/recv wait if the cap-id is not ready It's a highly likely situation that the clients call hh_msgq_recv() from a thread in a polling fashion, and these threads are spawned during client driver's initialization. Hence, it's possible that the cap-ids for the message queues will not be ready so early and the clients would end up continuously spinning on hh_msgq_recv(), receiving the error -EAGAIN. To avoid this situation, put the client process onto a wait-queue until the cap-ids are ready. Although a little unlikely, to satisfy the symmetry, add the same provision for the callers of hh_msgq_send() as well. Change-Id: I4e042215c5e43271d566340ce1ef9c52ecafeb5c Signed-off-by: Raghavendra Rao Ananta Signed-off-by: Murali Nalajala --- drivers/virt/haven/hh_msgq.c | 48 +++++++++++++++++++++++++++++++----- 1 file changed, 42 insertions(+), 6 deletions(-) diff --git a/drivers/virt/haven/hh_msgq.c b/drivers/virt/haven/hh_msgq.c index 0dd22d7c0b0f..c2aa58ab843e 100644 --- a/drivers/virt/haven/hh_msgq.c +++ b/drivers/virt/haven/hh_msgq.c @@ -9,6 +9,7 @@ #include #include #include +#include #include #include @@ -40,6 +41,7 @@ struct hh_msgq_cap_table { wait_queue_head_t rx_wq; }; +static bool hh_msgq_initialized; static struct hh_msgq_cap_table hh_msgq_cap_table[HH_MSGQ_LABEL_MAX]; static irqreturn_t hh_msgq_rx_isr(int irq, void *dev) @@ -118,6 +120,9 @@ static int __hh_msgq_recv(struct hh_msgq_cap_table *cap_table_entry, * if the message queue is not yet ready to communicate, and -EPERM if the * caller doesn't have permissions to receive the data. 0 is the data is * successfully received. + * + * Note: this function may sleep and should not be called from interrupt + * context */ int hh_msgq_recv(void *msgq_client_desc, void **buff, size_t *size, unsigned long flags) @@ -139,15 +144,25 @@ int hh_msgq_recv(void *msgq_client_desc, goto err; } - if (cap_table_entry->rx_cap_id == HH_CAPID_INVAL) { - pr_err("%s: Recv info for label %d not yet initialized\n", + if ((cap_table_entry->rx_cap_id == HH_CAPID_INVAL) && + (flags & HH_MSGQ_NONBLOCK)) { + pr_err_ratelimited( + "%s: Recv info for label %d not yet initialized\n", __func__, client_desc->label); ret = -EAGAIN; goto err; } + spin_unlock(&cap_table_entry->cap_entry_lock); + + if (wait_event_interruptible(cap_table_entry->rx_wq, + cap_table_entry->rx_cap_id != HH_CAPID_INVAL)) + return -ERESTARTSYS; + + spin_lock(&cap_table_entry->cap_entry_lock); + if (!cap_table_entry->rx_irq) { - pr_err("%s: Rx IRQ for label %d not yet setup\n", + pr_err_ratelimited("%s: Rx IRQ for label %d not yet setup\n", __func__, client_desc->label); ret = -EAGAIN; goto err; @@ -260,15 +275,25 @@ int hh_msgq_send(void *msgq_client_desc, goto err; } - if (cap_table_entry->tx_cap_id == HH_CAPID_INVAL) { - pr_err("%s: Send info for label %d not yet initialized\n", + if ((cap_table_entry->tx_cap_id == HH_CAPID_INVAL) && + (flags & HH_MSGQ_NONBLOCK)) { + pr_err_ratelimited( + "%s: Send info for label %d not yet initialized\n", __func__, client_desc->label); ret = -EAGAIN; goto err; } + spin_unlock(&cap_table_entry->cap_entry_lock); + + if (wait_event_interruptible(cap_table_entry->tx_wq, + cap_table_entry->tx_cap_id != HH_CAPID_INVAL)) + return -ERESTARTSYS; + + spin_lock(&cap_table_entry->cap_entry_lock); + if (!cap_table_entry->tx_irq) { - pr_err("%s: Tx IRQ for label %d not yet setup\n", + pr_err_ratelimited("%s: Tx IRQ for label %d not yet setup\n", __func__, client_desc->label); ret = -EAGAIN; goto err; @@ -316,6 +341,9 @@ void *hh_msgq_register(enum hh_msgq_label label) struct hh_msgq_cap_table *cap_table_entry; struct hh_msgq_desc *client_desc; + if (!hh_msgq_initialized) + return ERR_PTR(-EPROBE_DEFER); + if (label < 0 || label >= HH_MSGQ_LABEL_MAX) return ERR_PTR(-EINVAL); @@ -393,6 +421,9 @@ int hh_msgq_populate_cap_info(enum hh_msgq_label label, u64 cap_id, struct hh_msgq_cap_table *cap_table_entry; int ret; + if (!hh_msgq_initialized) + return -EPROBE_DEFER; + if (label < 0 || label >= HH_MSGQ_LABEL_MAX) { pr_err("%s: Invalid label passed\n", __func__); return -EINVAL; @@ -415,6 +446,8 @@ int hh_msgq_populate_cap_info(enum hh_msgq_label label, u64 cap_id, cap_table_entry->tx_irq = irq; cap_table_entry->tx_cap_id = cap_id; + + wake_up_interruptible(&cap_table_entry->tx_wq); } else if (direction == HH_MSGQ_DIRECTION_RX) { ret = request_irq(irq, hh_msgq_rx_isr, 0, cap_table_entry->rx_irq_name, cap_table_entry); @@ -423,6 +456,8 @@ int hh_msgq_populate_cap_info(enum hh_msgq_label label, u64 cap_id, cap_table_entry->rx_cap_id = cap_id; cap_table_entry->rx_irq = irq; + + wake_up_interruptible(&cap_table_entry->rx_wq); } else { pr_err("%s: Invalid direction passed\n", __func__); ret = -EINVAL; @@ -493,6 +528,7 @@ static int __init hh_msgq_init(void) } } + hh_msgq_initialized = true; return 0; err: From 01acbed66c661ce1125ebe6342b7674d3e11cdcd Mon Sep 17 00:00:00 2001 From: Raghavendra Rao Ananta Date: Tue, 24 Mar 2020 06:48:51 -0700 Subject: [PATCH 2/2] haven: hh_msgq: Let clients manage the buffers for hh_msgq_send Currently, the clients of the hh_msgq driver would allocate buffers, fill with data and call hh_msgq_send(). Upon success, hh_msgq_send() would be freeing the buffer, while in the case of failure, the clients are responsible for it. However, this approach of buffer management is not symmetric and could be confusing for the clients. Hence, let the clients take full ownership of the buffers- allocating and freeing. Also make changes to the affected client drivers. Change-Id: I73101d823f9d3de16414e444fde494222f584e53 Signed-off-by: Raghavendra Rao Ananta --- drivers/soc/qcom/mem-buf.c | 30 +++++++++++++++++++++++------- drivers/virt/haven/hh_msgq.c | 13 ++----------- drivers/virt/haven/hh_rm_core.c | 13 ++++++++++--- 3 files changed, 35 insertions(+), 21 deletions(-) diff --git a/drivers/soc/qcom/mem-buf.c b/drivers/soc/qcom/mem-buf.c index 78e0cfaf21d2..d3c3689ad684 100644 --- a/drivers/soc/qcom/mem-buf.c +++ b/drivers/soc/qcom/mem-buf.c @@ -662,10 +662,16 @@ static void mem_buf_alloc_req_work(struct work_struct *work) resp_msg->ret = ret; ret = hh_msgq_send(mem_buf_hh_msgq_hdl, resp_msg, sizeof(*resp_msg), 0); + + /* + * Free the buffer regardless of the return value as the hypervisor + * would have consumed the data in the case of a success. + */ + kfree(resp_msg); + if (ret < 0) { pr_err("%s: failed to send memory allocation response rc: %d\n", __func__, ret); - kfree(resp_msg); mutex_lock(&mem_buf_xfer_mem_list_lock); list_del(&xfer_mem->entry); mutex_unlock(&mem_buf_xfer_mem_list_lock); @@ -868,10 +874,15 @@ static int mem_buf_request_mem(struct mem_buf_desc *membuf) } ret = mem_buf_msg_send(alloc_req_msg, msg_size); - if (ret < 0) { - kfree(alloc_req_msg); + + /* + * Free the buffer regardless of the return value as the hypervisor + * would have consumed the data in the case of a success. + */ + kfree(alloc_req_msg); + + if (ret < 0) goto out; - } ret = mem_buf_txn_wait(&txn); if (ret < 0) @@ -897,11 +908,16 @@ static void mem_buf_relinquish_mem(struct mem_buf_desc *membuf) msg->hdl = membuf->memparcel_hdl; ret = hh_msgq_send(mem_buf_hh_msgq_hdl, msg, sizeof(*msg), 0); - if (ret < 0) { + + /* + * Free the buffer regardless of the return value as the hypervisor + * would have consumed the data in the case of a success. + */ + kfree(msg); + + if (ret < 0) pr_err("%s failed to send memory relinquish message rc: %d\n", __func__, ret); - kfree(msg); - } } static int mem_buf_map_mem_s2(struct mem_buf_desc *membuf) diff --git a/drivers/virt/haven/hh_msgq.c b/drivers/virt/haven/hh_msgq.c index c2aa58ab843e..575f8211bc81 100644 --- a/drivers/virt/haven/hh_msgq.c +++ b/drivers/virt/haven/hh_msgq.c @@ -239,15 +239,12 @@ static int __hh_msgq_send(struct hh_msgq_cap_table *cap_table_entry, /** * hh_msgq_send: Send a message to the client on a different VM * @client_desc: The client descriptor that was obtained via hh_msgq_register() - * @buff: Pointer to the buffer that needs to be sent. The buffer should be - * dynamically allocated via kmalloc/kzalloc. + * @buff: Pointer to the buffer that needs to be sent * @size: The size of the buffer * @flags: Optional flags to pass to send the data. For the list of flags, * see linux/haven/hh_msgq.h * - * The function would free the buffer upon success, and returns 0. The caller - * should not be referencing the buffer anymore. - * On the other hand, it returns -EINVAL if the caller passes invalid arguments, + * The function returns -EINVAL if the caller passes invalid arguments, * -EAGAIN if the message queue is not yet ready to communicate, and -EPERM if * the caller doesn't have permissions to send the data. * @@ -312,12 +309,6 @@ int hh_msgq_send(void *msgq_client_desc, ret = __hh_msgq_send(cap_table_entry, buff, size, flags); } while (ret == -EAGAIN); - /* If the send is success, hypervisor should not be holding any - * references to 'buff', and hence, can be released. - */ - if (!ret) - kfree(buff); - return ret; err: spin_unlock(&cap_table_entry->cap_entry_lock); diff --git a/drivers/virt/haven/hh_rm_core.c b/drivers/virt/haven/hh_rm_core.c index 25d0ff88dde5..d0c7db026dff 100644 --- a/drivers/virt/haven/hh_rm_core.c +++ b/drivers/virt/haven/hh_rm_core.c @@ -518,10 +518,17 @@ static int hh_rm_send_request(u32 message_id, ret = hh_msgq_send(hh_rm_msgq_desc, send_buff, sizeof(*hdr) + payload_size, tx_flags); - if (ret) { - kfree(send_buff); + + /* + * In the case of a success, the hypervisor would have consumed + * the buffer. While in the case of a failure, we are going to + * quit anyways. Hence, free the buffer regardless of the + * return value. + */ + kfree(send_buff); + + if (ret) return ret; - } } return 0;