Re: [PATCH v3 2/5] soc: qcom: Introduce QMI helpers
On Fri 17 Nov 18:11 PST 2017, Chris Lew wrote: > On 11/15/2017 12:10 PM, Bjorn Andersson wrote: > [..]> +static void qmi_handle_message(struct qmi_handle *qmi, > > + struct sockaddr_qrtr *sq, > > + const void *buf, size_t len) > > +{ > > + const struct qmi_header *hdr; > > + struct qmi_txn tmp_txn; > > + struct qmi_txn *txn = NULL; > > + int ret; > > + > > + if (len < sizeof(*hdr)) { > > + pr_err("ignoring short QMI packet\n"); > > + return; > > + } > > + > > + hdr = buf; > > + > > + /* If this is a response, find the matching transaction handle */ > > + if (hdr->type == QMI_RESPONSE) { > > + mutex_lock(>txn_lock); > > + txn = idr_find(>txns, hdr->txn_id); > > + if (txn) > > + mutex_lock(>lock); > > + mutex_unlock(>txn_lock); > > + } > > + > > + if (txn && txn->dest && txn->ei) { > > + ret = qmi_decode_message(buf, len, txn->ei, txn->dest); > > + if (ret < 0) > > + pr_err("failed to decode incoming message\n"); > > + > > + txn->result = ret; > > + complete(>completion); > > + > > + mutex_unlock(>lock); > > + } else if (txn) { > > + qmi_invoke_handler(qmi, sq, txn, buf, len); > > + > > + mutex_unlock(>lock); > > + } else { > > + /* Create a txn based on the txn_id of the incoming message */ > > + memset(_txn, 0, sizeof(tmp_txn)); > > + tmp_txn.id = hdr->txn_id; > > + > > + qmi_invoke_handler(qmi, sq, _txn, buf, len); > > I'm seeing an opportunity for user error with timed out transactions. If > txn_wait gets timed out the txn is removed from the txns list. Later if we > get the response, it comes down to this else with the tmp_txn. Some handlers > may try to do a complete(>completion) like the qmi_sample_client > ping_pong_cb. This leads to a null pointer dereference since the temp txn > was never initialized. > You're right, that's no good. > Should clients not call complete and we can call the complete in the else > if(txn) condition? > I don't think the problem is limited to the completion. A typical design pattern for these things could be to wrap the txn in a struct and use container_of() in the response handler to access or fill in additional information. If the txn in this scenario happens to be the temporal one we've just messed up the stack. The else statement was added to deal with requests and indications. The case I can think of where this would make sense is for a client to send a request but not wait for the result and then have some logic to happen once the response arrives (but unrelated to the requesting context). The quick solution would be to not allow this to happen, i.e. only take the else block if it's a request or a indication. The alternative is to allow the response-handler to know if it was called from the else-statement, but that would mean that every response-handler would have to check this. Regards, Bjorn
Re: [PATCH v3 2/5] soc: qcom: Introduce QMI helpers
On Fri 17 Nov 18:11 PST 2017, Chris Lew wrote: > On 11/15/2017 12:10 PM, Bjorn Andersson wrote: > [..]> +static void qmi_handle_message(struct qmi_handle *qmi, > > + struct sockaddr_qrtr *sq, > > + const void *buf, size_t len) > > +{ > > + const struct qmi_header *hdr; > > + struct qmi_txn tmp_txn; > > + struct qmi_txn *txn = NULL; > > + int ret; > > + > > + if (len < sizeof(*hdr)) { > > + pr_err("ignoring short QMI packet\n"); > > + return; > > + } > > + > > + hdr = buf; > > + > > + /* If this is a response, find the matching transaction handle */ > > + if (hdr->type == QMI_RESPONSE) { > > + mutex_lock(>txn_lock); > > + txn = idr_find(>txns, hdr->txn_id); > > + if (txn) > > + mutex_lock(>lock); > > + mutex_unlock(>txn_lock); > > + } > > + > > + if (txn && txn->dest && txn->ei) { > > + ret = qmi_decode_message(buf, len, txn->ei, txn->dest); > > + if (ret < 0) > > + pr_err("failed to decode incoming message\n"); > > + > > + txn->result = ret; > > + complete(>completion); > > + > > + mutex_unlock(>lock); > > + } else if (txn) { > > + qmi_invoke_handler(qmi, sq, txn, buf, len); > > + > > + mutex_unlock(>lock); > > + } else { > > + /* Create a txn based on the txn_id of the incoming message */ > > + memset(_txn, 0, sizeof(tmp_txn)); > > + tmp_txn.id = hdr->txn_id; > > + > > + qmi_invoke_handler(qmi, sq, _txn, buf, len); > > I'm seeing an opportunity for user error with timed out transactions. If > txn_wait gets timed out the txn is removed from the txns list. Later if we > get the response, it comes down to this else with the tmp_txn. Some handlers > may try to do a complete(>completion) like the qmi_sample_client > ping_pong_cb. This leads to a null pointer dereference since the temp txn > was never initialized. > You're right, that's no good. > Should clients not call complete and we can call the complete in the else > if(txn) condition? > I don't think the problem is limited to the completion. A typical design pattern for these things could be to wrap the txn in a struct and use container_of() in the response handler to access or fill in additional information. If the txn in this scenario happens to be the temporal one we've just messed up the stack. The else statement was added to deal with requests and indications. The case I can think of where this would make sense is for a client to send a request but not wait for the result and then have some logic to happen once the response arrives (but unrelated to the requesting context). The quick solution would be to not allow this to happen, i.e. only take the else block if it's a request or a indication. The alternative is to allow the response-handler to know if it was called from the else-statement, but that would mean that every response-handler would have to check this. Regards, Bjorn
Re: [PATCH v3 2/5] soc: qcom: Introduce QMI helpers
On 11/15/2017 12:10 PM, Bjorn Andersson wrote: [..]> +static void qmi_handle_message(struct qmi_handle *qmi, + struct sockaddr_qrtr *sq, + const void *buf, size_t len) +{ + const struct qmi_header *hdr; + struct qmi_txn tmp_txn; + struct qmi_txn *txn = NULL; + int ret; + + if (len < sizeof(*hdr)) { + pr_err("ignoring short QMI packet\n"); + return; + } + + hdr = buf; + + /* If this is a response, find the matching transaction handle */ + if (hdr->type == QMI_RESPONSE) { + mutex_lock(>txn_lock); + txn = idr_find(>txns, hdr->txn_id); + if (txn) + mutex_lock(>lock); + mutex_unlock(>txn_lock); + } + + if (txn && txn->dest && txn->ei) { + ret = qmi_decode_message(buf, len, txn->ei, txn->dest); + if (ret < 0) + pr_err("failed to decode incoming message\n"); + + txn->result = ret; + complete(>completion); + + mutex_unlock(>lock); + } else if (txn) { + qmi_invoke_handler(qmi, sq, txn, buf, len); + + mutex_unlock(>lock); + } else { + /* Create a txn based on the txn_id of the incoming message */ + memset(_txn, 0, sizeof(tmp_txn)); + tmp_txn.id = hdr->txn_id; + + qmi_invoke_handler(qmi, sq, _txn, buf, len); I'm seeing an opportunity for user error with timed out transactions. If txn_wait gets timed out the txn is removed from the txns list. Later if we get the response, it comes down to this else with the tmp_txn. Some handlers may try to do a complete(>completion) like the qmi_sample_client ping_pong_cb. This leads to a null pointer dereference since the temp txn was never initialized. Should clients not call complete and we can call the complete in the else if(txn) condition? Thanks, Chris -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
Re: [PATCH v3 2/5] soc: qcom: Introduce QMI helpers
On 11/15/2017 12:10 PM, Bjorn Andersson wrote: [..]> +static void qmi_handle_message(struct qmi_handle *qmi, + struct sockaddr_qrtr *sq, + const void *buf, size_t len) +{ + const struct qmi_header *hdr; + struct qmi_txn tmp_txn; + struct qmi_txn *txn = NULL; + int ret; + + if (len < sizeof(*hdr)) { + pr_err("ignoring short QMI packet\n"); + return; + } + + hdr = buf; + + /* If this is a response, find the matching transaction handle */ + if (hdr->type == QMI_RESPONSE) { + mutex_lock(>txn_lock); + txn = idr_find(>txns, hdr->txn_id); + if (txn) + mutex_lock(>lock); + mutex_unlock(>txn_lock); + } + + if (txn && txn->dest && txn->ei) { + ret = qmi_decode_message(buf, len, txn->ei, txn->dest); + if (ret < 0) + pr_err("failed to decode incoming message\n"); + + txn->result = ret; + complete(>completion); + + mutex_unlock(>lock); + } else if (txn) { + qmi_invoke_handler(qmi, sq, txn, buf, len); + + mutex_unlock(>lock); + } else { + /* Create a txn based on the txn_id of the incoming message */ + memset(_txn, 0, sizeof(tmp_txn)); + tmp_txn.id = hdr->txn_id; + + qmi_invoke_handler(qmi, sq, _txn, buf, len); I'm seeing an opportunity for user error with timed out transactions. If txn_wait gets timed out the txn is removed from the txns list. Later if we get the response, it comes down to this else with the tmp_txn. Some handlers may try to do a complete(>completion) like the qmi_sample_client ping_pong_cb. This leads to a null pointer dereference since the temp txn was never initialized. Should clients not call complete and we can call the complete in the else if(txn) condition? Thanks, Chris -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
Re: [PATCH v3 2/5] soc: qcom: Introduce QMI helpers
On Thu 16 Nov 04:11 PST 2017, Srinivas Kandagatla wrote: > On 15/11/17 20:10, Bjorn Andersson wrote: [..] > > diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig > > index 91b70b170a82..9718f1c41e3d 100644 > > --- a/drivers/soc/qcom/Kconfig > > +++ b/drivers/soc/qcom/Kconfig > > @@ -37,6 +37,7 @@ config QCOM_PM > > config QCOM_QMI_HELPERS > > tristate > > + depends on ARCH_QCOM > > This bit is confusing!!, first patch added this symbol and this patch added > the depends. either we move this to first patch or move out the > QCOM_QMI_HELPERS from first patch and include in this patch > Ohh, that's not right. The depends should be in the same patch. And we don't really depends on ARCH_QCOM here, but without it Kconfig doesn't know that make won't enter drivers/soc/qcom so we will end up with a link error. I'm hoping we can fix this issue, to get some more compile testing of these things. > > help > > Helper library for handling QMI encoded messages. QMI encoded > > messages are used in communication between the majority of QRTR > > diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile [..] > > obj-$(CONFIG_QCOM_QMI_HELPERS)+= qmi_helpers.o > > qmi_helpers-y += qmi_encdec.o > > +qmi_helpers-y += qmi_interface.o > for better reading.. i would suggest.. > qmi_helpers-y += qmi_encdec.o qmi_interface.o > Sounds reasonable. > > > obj-$(CONFIG_QCOM_SMD_RPM)+= smd-rpm.o > > obj-$(CONFIG_QCOM_SMEM) +=smem.o > > obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o > > diff --git a/drivers/soc/qcom/qmi_interface.c > > b/drivers/soc/qcom/qmi_interface.c [..] > > +#include > > Do we need this? > I don't think so. [..] > > +/** > > + * qmi_recv_new_server() - handler of NEW_SERVER control message > > + * @qmi: qmi handle > > + * @node: node of the new server > > + * @port: port of the new server > > + * > service and instance is not documented here. > Thanks for noticing, will fix these. [..] > > +/** > > + * qmi_handle_init() - initialize a QMI client handle > > + * @qmi: QMI handle to initialize > > + * @max_msg_len: maximum size of incoming message > do we need this?? > We need a buffer into which we can receive incoming packets, so either we allocate a large enough buffer up front or we have to ask qrtr before each packet how much space we will need. I think largest possible buffer is 64kB, but typically we need much less. And the IDL compiler seems to output this constant, so it seems reasonable to pass it here. > > > + * @handlers: NULL-terminated list of QMI message handlers > > + * > recv_buf_size and ops not documented > Looks like I've renamed max_msg_len to recv_buf_size, and forgot to add ops. Will fix. [..] > > + > > +/** > > + * qmi_send_indication() - send an indication QMI message > > + * @qmi: QMI client handle > > + * @sq:destination sockaddr > > + * @txn: transaction object to use for the message > > txn is not required here? > No. Indications are fire-and-forget messages, but still need a transaction id, so it's confusing for the client to have to create a txn, use it and then cancel it (or to add another txn operation for this). Therefor I'm hiding the txn handling inside this function. Will fix the kerneldoc. [..] > > diff --git a/include/linux/soc/qcom/qmi.h b/include/linux/soc/qcom/qmi.h > > index 5df7edfc6bfd..9b4f4fa5bed6 100644 > > --- a/include/linux/soc/qcom/qmi.h > > +++ b/include/linux/soc/qcom/qmi.h > > Looks like this patch added few things like list, wq and so to this header > file, should we not add headers for those ?? > Will review these. Thanks for the review! Regards, Bjorn
Re: [PATCH v3 2/5] soc: qcom: Introduce QMI helpers
On Thu 16 Nov 04:11 PST 2017, Srinivas Kandagatla wrote: > On 15/11/17 20:10, Bjorn Andersson wrote: [..] > > diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig > > index 91b70b170a82..9718f1c41e3d 100644 > > --- a/drivers/soc/qcom/Kconfig > > +++ b/drivers/soc/qcom/Kconfig > > @@ -37,6 +37,7 @@ config QCOM_PM > > config QCOM_QMI_HELPERS > > tristate > > + depends on ARCH_QCOM > > This bit is confusing!!, first patch added this symbol and this patch added > the depends. either we move this to first patch or move out the > QCOM_QMI_HELPERS from first patch and include in this patch > Ohh, that's not right. The depends should be in the same patch. And we don't really depends on ARCH_QCOM here, but without it Kconfig doesn't know that make won't enter drivers/soc/qcom so we will end up with a link error. I'm hoping we can fix this issue, to get some more compile testing of these things. > > help > > Helper library for handling QMI encoded messages. QMI encoded > > messages are used in communication between the majority of QRTR > > diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile [..] > > obj-$(CONFIG_QCOM_QMI_HELPERS)+= qmi_helpers.o > > qmi_helpers-y += qmi_encdec.o > > +qmi_helpers-y += qmi_interface.o > for better reading.. i would suggest.. > qmi_helpers-y += qmi_encdec.o qmi_interface.o > Sounds reasonable. > > > obj-$(CONFIG_QCOM_SMD_RPM)+= smd-rpm.o > > obj-$(CONFIG_QCOM_SMEM) +=smem.o > > obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o > > diff --git a/drivers/soc/qcom/qmi_interface.c > > b/drivers/soc/qcom/qmi_interface.c [..] > > +#include > > Do we need this? > I don't think so. [..] > > +/** > > + * qmi_recv_new_server() - handler of NEW_SERVER control message > > + * @qmi: qmi handle > > + * @node: node of the new server > > + * @port: port of the new server > > + * > service and instance is not documented here. > Thanks for noticing, will fix these. [..] > > +/** > > + * qmi_handle_init() - initialize a QMI client handle > > + * @qmi: QMI handle to initialize > > + * @max_msg_len: maximum size of incoming message > do we need this?? > We need a buffer into which we can receive incoming packets, so either we allocate a large enough buffer up front or we have to ask qrtr before each packet how much space we will need. I think largest possible buffer is 64kB, but typically we need much less. And the IDL compiler seems to output this constant, so it seems reasonable to pass it here. > > > + * @handlers: NULL-terminated list of QMI message handlers > > + * > recv_buf_size and ops not documented > Looks like I've renamed max_msg_len to recv_buf_size, and forgot to add ops. Will fix. [..] > > + > > +/** > > + * qmi_send_indication() - send an indication QMI message > > + * @qmi: QMI client handle > > + * @sq:destination sockaddr > > + * @txn: transaction object to use for the message > > txn is not required here? > No. Indications are fire-and-forget messages, but still need a transaction id, so it's confusing for the client to have to create a txn, use it and then cancel it (or to add another txn operation for this). Therefor I'm hiding the txn handling inside this function. Will fix the kerneldoc. [..] > > diff --git a/include/linux/soc/qcom/qmi.h b/include/linux/soc/qcom/qmi.h > > index 5df7edfc6bfd..9b4f4fa5bed6 100644 > > --- a/include/linux/soc/qcom/qmi.h > > +++ b/include/linux/soc/qcom/qmi.h > > Looks like this patch added few things like list, wq and so to this header > file, should we not add headers for those ?? > Will review these. Thanks for the review! Regards, Bjorn
Re: [PATCH v3 2/5] soc: qcom: Introduce QMI helpers
On 15/11/17 20:10, Bjorn Andersson wrote: Drivers that needs to communicate with a remote QMI service all has to perform the operations of discovering the service, encoding and decoding the messages and operate the socket. This introduces an abstraction for these common operations, reducing most of the duplication in such cases. Signed-off-by: Bjorn Andersson--- Changes since v2: - Fix typos - Checkpatch fixes - Use non-GPL EXPORT_SYMBOL Changes since v1: - Squashed notion of qmi vs qrtr in interface, to simplify client code - Lookup and service registration is cached and handled by core on ENETRESET - Introduce callbacks for BYE and DEL_CLIENT control messages - Revisited locking for socket and transaction objects, squashed a few races and made it possible to send "indication" messages from message handlers. - kerneldoc updates - Moved handling of net_reset from clients to this code, greatly simplifies typical drivers and reduces duplication - Pass transaction id of QMI message to handlers even though it's not a response, allows sending a response with the txn id of an incoming request. - Split qmi_send_message() in three, instead of passing type as parameter - to clean up handling of "indication" messages. drivers/soc/qcom/Kconfig | 1 + drivers/soc/qcom/Makefile| 1 + drivers/soc/qcom/qmi_interface.c | 849 +++ include/linux/soc/qcom/qmi.h | 157 4 files changed, 1008 insertions(+) create mode 100644 drivers/soc/qcom/qmi_interface.c I have tested this patch along with slimbus ngd for wcd9335 analog audio playback. Tested-By: Srinivas Kandagatla Few minor comments though!! diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig index 91b70b170a82..9718f1c41e3d 100644 --- a/drivers/soc/qcom/Kconfig +++ b/drivers/soc/qcom/Kconfig @@ -37,6 +37,7 @@ config QCOM_PM config QCOM_QMI_HELPERS tristate + depends on ARCH_QCOM This bit is confusing!!, first patch added this symbol and this patch added the depends. either we move this to first patch or move out the QCOM_QMI_HELPERS from first patch and include in this patch help Helper library for handling QMI encoded messages. QMI encoded messages are used in communication between the majority of QRTR diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile index 625750acfeef..b04b5044775f 100644 --- a/drivers/soc/qcom/Makefile +++ b/drivers/soc/qcom/Makefile @@ -4,6 +4,7 @@ obj-$(CONFIG_QCOM_MDT_LOADER) += mdt_loader.o obj-$(CONFIG_QCOM_PM) += spm.o obj-$(CONFIG_QCOM_QMI_HELPERS)+= qmi_helpers.o qmi_helpers-y += qmi_encdec.o +qmi_helpers-y += qmi_interface.o for better reading.. i would suggest.. qmi_helpers-y += qmi_encdec.o qmi_interface.o obj-$(CONFIG_QCOM_SMD_RPM)+= smd-rpm.o obj-$(CONFIG_QCOM_SMEM) +=smem.o obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o diff --git a/drivers/soc/qcom/qmi_interface.c b/drivers/soc/qcom/qmi_interface.c new file mode 100644 index ..6651257f15a4 --- /dev/null +++ b/drivers/soc/qcom/qmi_interface.c @@ -0,0 +1,849 @@ +/* + * Copyright (C) 2017 Linaro Ltd. + * + * This software is licensed under the terms of the GNU General Public + * License version 2, as published by the Free Software Foundation, and + * may be copied, distributed, and modified under those terms. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include +#include +#include +#include Do we need this? +#include +#include +#include +#include +#include +#include +#include +#include + +static struct socket *qmi_sock_create(struct qmi_handle *qmi, + struct sockaddr_qrtr *sq); + +/** + * qmi_recv_new_server() - handler of NEW_SERVER control message + * @qmi: qmi handle + * @node: node of the new server + * @port: port of the new server + * service and instance is not documented here. + * Calls the new_server callback to inform the client about + msg.msg_name = + msg.msg_namelen = sizeof(sq); + + mutex_lock(>sock_lock); + if (qmi->sock) { + ret = kernel_sendmsg(qmi->sock, , , 1, sizeof(pkt)); + if (ret < 0) + pr_err("failed to send lookup registration: %d\n", ret); + } + mutex_unlock(>sock_lock); +} + +/** + * qmi_add_lookup() - register a new lookup with the name service + * @qmi: qmi handle + * @service: service id of the request + * @instance: instance id of the request + * version not documented. + * Returns 0 on success, negative errno on failure. + * + * Registering a lookup query with the name
Re: [PATCH v3 2/5] soc: qcom: Introduce QMI helpers
On 15/11/17 20:10, Bjorn Andersson wrote: Drivers that needs to communicate with a remote QMI service all has to perform the operations of discovering the service, encoding and decoding the messages and operate the socket. This introduces an abstraction for these common operations, reducing most of the duplication in such cases. Signed-off-by: Bjorn Andersson --- Changes since v2: - Fix typos - Checkpatch fixes - Use non-GPL EXPORT_SYMBOL Changes since v1: - Squashed notion of qmi vs qrtr in interface, to simplify client code - Lookup and service registration is cached and handled by core on ENETRESET - Introduce callbacks for BYE and DEL_CLIENT control messages - Revisited locking for socket and transaction objects, squashed a few races and made it possible to send "indication" messages from message handlers. - kerneldoc updates - Moved handling of net_reset from clients to this code, greatly simplifies typical drivers and reduces duplication - Pass transaction id of QMI message to handlers even though it's not a response, allows sending a response with the txn id of an incoming request. - Split qmi_send_message() in three, instead of passing type as parameter - to clean up handling of "indication" messages. drivers/soc/qcom/Kconfig | 1 + drivers/soc/qcom/Makefile| 1 + drivers/soc/qcom/qmi_interface.c | 849 +++ include/linux/soc/qcom/qmi.h | 157 4 files changed, 1008 insertions(+) create mode 100644 drivers/soc/qcom/qmi_interface.c I have tested this patch along with slimbus ngd for wcd9335 analog audio playback. Tested-By: Srinivas Kandagatla Few minor comments though!! diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig index 91b70b170a82..9718f1c41e3d 100644 --- a/drivers/soc/qcom/Kconfig +++ b/drivers/soc/qcom/Kconfig @@ -37,6 +37,7 @@ config QCOM_PM config QCOM_QMI_HELPERS tristate + depends on ARCH_QCOM This bit is confusing!!, first patch added this symbol and this patch added the depends. either we move this to first patch or move out the QCOM_QMI_HELPERS from first patch and include in this patch help Helper library for handling QMI encoded messages. QMI encoded messages are used in communication between the majority of QRTR diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile index 625750acfeef..b04b5044775f 100644 --- a/drivers/soc/qcom/Makefile +++ b/drivers/soc/qcom/Makefile @@ -4,6 +4,7 @@ obj-$(CONFIG_QCOM_MDT_LOADER) += mdt_loader.o obj-$(CONFIG_QCOM_PM) += spm.o obj-$(CONFIG_QCOM_QMI_HELPERS)+= qmi_helpers.o qmi_helpers-y += qmi_encdec.o +qmi_helpers-y += qmi_interface.o for better reading.. i would suggest.. qmi_helpers-y += qmi_encdec.o qmi_interface.o obj-$(CONFIG_QCOM_SMD_RPM)+= smd-rpm.o obj-$(CONFIG_QCOM_SMEM) +=smem.o obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o diff --git a/drivers/soc/qcom/qmi_interface.c b/drivers/soc/qcom/qmi_interface.c new file mode 100644 index ..6651257f15a4 --- /dev/null +++ b/drivers/soc/qcom/qmi_interface.c @@ -0,0 +1,849 @@ +/* + * Copyright (C) 2017 Linaro Ltd. + * + * This software is licensed under the terms of the GNU General Public + * License version 2, as published by the Free Software Foundation, and + * may be copied, distributed, and modified under those terms. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include +#include +#include +#include Do we need this? +#include +#include +#include +#include +#include +#include +#include +#include + +static struct socket *qmi_sock_create(struct qmi_handle *qmi, + struct sockaddr_qrtr *sq); + +/** + * qmi_recv_new_server() - handler of NEW_SERVER control message + * @qmi: qmi handle + * @node: node of the new server + * @port: port of the new server + * service and instance is not documented here. + * Calls the new_server callback to inform the client about + msg.msg_name = + msg.msg_namelen = sizeof(sq); + + mutex_lock(>sock_lock); + if (qmi->sock) { + ret = kernel_sendmsg(qmi->sock, , , 1, sizeof(pkt)); + if (ret < 0) + pr_err("failed to send lookup registration: %d\n", ret); + } + mutex_unlock(>sock_lock); +} + +/** + * qmi_add_lookup() - register a new lookup with the name service + * @qmi: qmi handle + * @service: service id of the request + * @instance: instance id of the request + * version not documented. + * Returns 0 on success, negative errno on failure. + * + * Registering a lookup query with the name server will cause the name server + * to send NEW_SERVER and
[PATCH v3 2/5] soc: qcom: Introduce QMI helpers
Drivers that needs to communicate with a remote QMI service all has to perform the operations of discovering the service, encoding and decoding the messages and operate the socket. This introduces an abstraction for these common operations, reducing most of the duplication in such cases. Signed-off-by: Bjorn Andersson--- Changes since v2: - Fix typos - Checkpatch fixes - Use non-GPL EXPORT_SYMBOL Changes since v1: - Squashed notion of qmi vs qrtr in interface, to simplify client code - Lookup and service registration is cached and handled by core on ENETRESET - Introduce callbacks for BYE and DEL_CLIENT control messages - Revisited locking for socket and transaction objects, squashed a few races and made it possible to send "indication" messages from message handlers. - kerneldoc updates - Moved handling of net_reset from clients to this code, greatly simplifies typical drivers and reduces duplication - Pass transaction id of QMI message to handlers even though it's not a response, allows sending a response with the txn id of an incoming request. - Split qmi_send_message() in three, instead of passing type as parameter - to clean up handling of "indication" messages. drivers/soc/qcom/Kconfig | 1 + drivers/soc/qcom/Makefile| 1 + drivers/soc/qcom/qmi_interface.c | 849 +++ include/linux/soc/qcom/qmi.h | 157 4 files changed, 1008 insertions(+) create mode 100644 drivers/soc/qcom/qmi_interface.c diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig index 91b70b170a82..9718f1c41e3d 100644 --- a/drivers/soc/qcom/Kconfig +++ b/drivers/soc/qcom/Kconfig @@ -37,6 +37,7 @@ config QCOM_PM config QCOM_QMI_HELPERS tristate + depends on ARCH_QCOM help Helper library for handling QMI encoded messages. QMI encoded messages are used in communication between the majority of QRTR diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile index 625750acfeef..b04b5044775f 100644 --- a/drivers/soc/qcom/Makefile +++ b/drivers/soc/qcom/Makefile @@ -4,6 +4,7 @@ obj-$(CONFIG_QCOM_MDT_LOADER) += mdt_loader.o obj-$(CONFIG_QCOM_PM) += spm.o obj-$(CONFIG_QCOM_QMI_HELPERS) += qmi_helpers.o qmi_helpers-y += qmi_encdec.o +qmi_helpers-y += qmi_interface.o obj-$(CONFIG_QCOM_SMD_RPM) += smd-rpm.o obj-$(CONFIG_QCOM_SMEM) += smem.o obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o diff --git a/drivers/soc/qcom/qmi_interface.c b/drivers/soc/qcom/qmi_interface.c new file mode 100644 index ..6651257f15a4 --- /dev/null +++ b/drivers/soc/qcom/qmi_interface.c @@ -0,0 +1,849 @@ +/* + * Copyright (C) 2017 Linaro Ltd. + * + * This software is licensed under the terms of the GNU General Public + * License version 2, as published by the Free Software Foundation, and + * may be copied, distributed, and modified under those terms. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +static struct socket *qmi_sock_create(struct qmi_handle *qmi, + struct sockaddr_qrtr *sq); + +/** + * qmi_recv_new_server() - handler of NEW_SERVER control message + * @qmi: qmi handle + * @node: node of the new server + * @port: port of the new server + * + * Calls the new_server callback to inform the client about a newly registered + * server matching the currently registered service lookup. + */ +static void qmi_recv_new_server(struct qmi_handle *qmi, + unsigned int service, unsigned int instance, + unsigned int node, unsigned int port) +{ + struct qmi_ops *ops = >ops; + struct qmi_service *svc; + int ret; + + if (!ops->new_server) + return; + + /* Ignore EOF marker */ + if (!node && !port) + return; + + svc = kzalloc(sizeof(*svc), GFP_KERNEL); + if (!svc) + return; + + svc->service = service; + svc->version = instance & 0xff; + svc->instance = instance >> 8; + svc->node = node; + svc->port = port; + + ret = ops->new_server(qmi, svc); + if (ret < 0) + kfree(svc); + else + list_add(>list_node, >lookup_results); +} + +/** + * qmi_recv_del_server() - handler of DEL_SERVER control message + * @qmi: qmi handle + * @node: node of the dying server, a value of -1 matches all nodes + * @port: port of the dying server, a value of -1 matches all ports + * + * Calls the del_server callback for each previously seen server, allowing the + * client to
[PATCH v3 2/5] soc: qcom: Introduce QMI helpers
Drivers that needs to communicate with a remote QMI service all has to perform the operations of discovering the service, encoding and decoding the messages and operate the socket. This introduces an abstraction for these common operations, reducing most of the duplication in such cases. Signed-off-by: Bjorn Andersson --- Changes since v2: - Fix typos - Checkpatch fixes - Use non-GPL EXPORT_SYMBOL Changes since v1: - Squashed notion of qmi vs qrtr in interface, to simplify client code - Lookup and service registration is cached and handled by core on ENETRESET - Introduce callbacks for BYE and DEL_CLIENT control messages - Revisited locking for socket and transaction objects, squashed a few races and made it possible to send "indication" messages from message handlers. - kerneldoc updates - Moved handling of net_reset from clients to this code, greatly simplifies typical drivers and reduces duplication - Pass transaction id of QMI message to handlers even though it's not a response, allows sending a response with the txn id of an incoming request. - Split qmi_send_message() in three, instead of passing type as parameter - to clean up handling of "indication" messages. drivers/soc/qcom/Kconfig | 1 + drivers/soc/qcom/Makefile| 1 + drivers/soc/qcom/qmi_interface.c | 849 +++ include/linux/soc/qcom/qmi.h | 157 4 files changed, 1008 insertions(+) create mode 100644 drivers/soc/qcom/qmi_interface.c diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig index 91b70b170a82..9718f1c41e3d 100644 --- a/drivers/soc/qcom/Kconfig +++ b/drivers/soc/qcom/Kconfig @@ -37,6 +37,7 @@ config QCOM_PM config QCOM_QMI_HELPERS tristate + depends on ARCH_QCOM help Helper library for handling QMI encoded messages. QMI encoded messages are used in communication between the majority of QRTR diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile index 625750acfeef..b04b5044775f 100644 --- a/drivers/soc/qcom/Makefile +++ b/drivers/soc/qcom/Makefile @@ -4,6 +4,7 @@ obj-$(CONFIG_QCOM_MDT_LOADER) += mdt_loader.o obj-$(CONFIG_QCOM_PM) += spm.o obj-$(CONFIG_QCOM_QMI_HELPERS) += qmi_helpers.o qmi_helpers-y += qmi_encdec.o +qmi_helpers-y += qmi_interface.o obj-$(CONFIG_QCOM_SMD_RPM) += smd-rpm.o obj-$(CONFIG_QCOM_SMEM) += smem.o obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o diff --git a/drivers/soc/qcom/qmi_interface.c b/drivers/soc/qcom/qmi_interface.c new file mode 100644 index ..6651257f15a4 --- /dev/null +++ b/drivers/soc/qcom/qmi_interface.c @@ -0,0 +1,849 @@ +/* + * Copyright (C) 2017 Linaro Ltd. + * + * This software is licensed under the terms of the GNU General Public + * License version 2, as published by the Free Software Foundation, and + * may be copied, distributed, and modified under those terms. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +static struct socket *qmi_sock_create(struct qmi_handle *qmi, + struct sockaddr_qrtr *sq); + +/** + * qmi_recv_new_server() - handler of NEW_SERVER control message + * @qmi: qmi handle + * @node: node of the new server + * @port: port of the new server + * + * Calls the new_server callback to inform the client about a newly registered + * server matching the currently registered service lookup. + */ +static void qmi_recv_new_server(struct qmi_handle *qmi, + unsigned int service, unsigned int instance, + unsigned int node, unsigned int port) +{ + struct qmi_ops *ops = >ops; + struct qmi_service *svc; + int ret; + + if (!ops->new_server) + return; + + /* Ignore EOF marker */ + if (!node && !port) + return; + + svc = kzalloc(sizeof(*svc), GFP_KERNEL); + if (!svc) + return; + + svc->service = service; + svc->version = instance & 0xff; + svc->instance = instance >> 8; + svc->node = node; + svc->port = port; + + ret = ops->new_server(qmi, svc); + if (ret < 0) + kfree(svc); + else + list_add(>list_node, >lookup_results); +} + +/** + * qmi_recv_del_server() - handler of DEL_SERVER control message + * @qmi: qmi handle + * @node: node of the dying server, a value of -1 matches all nodes + * @port: port of the dying server, a value of -1 matches all ports + * + * Calls the del_server callback for each previously seen server, allowing the + * client to react to the disappearing