diff mbox series

[v3,2/5] soc: qcom: Introduce QMI helpers

Message ID 20171115201012.25892-3-bjorn.andersson@linaro.org
State Superseded
Headers show
Series In-kernel QMI helpers and sysmon | expand

Commit Message

Bjorn Andersson Nov. 15, 2017, 8:10 p.m. UTC
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 <bjorn.andersson@linaro.org>

---

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

-- 
2.15.0

--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Srinivas Kandagatla Nov. 16, 2017, 12:11 p.m. UTC | #1
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 <bjorn.andersson@linaro.org>

> ---

> 

> 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 <srinivas.kandagatla@linaro.org>


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 000000000000..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 <linux/kernel.h>

> +#include <linux/module.h>

> +#include <linux/device.h>

> +#include <linux/platform_device.h>


Do we need this?

> +#include <linux/qrtr.h>

> +#include <linux/net.h>

> +#include <linux/completion.h>

> +#include <linux/idr.h>

> +#include <linux/string.h>

> +#include <net/sock.h>

> +#include <linux/workqueue.h>

> +#include <linux/soc/qcom/qmi.h>

> +

> +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 = &sq;

> +	msg.msg_namelen = sizeof(sq);

> +

> +	mutex_lock(&qmi->sock_lock);

> +	if (qmi->sock) {

> +		ret = kernel_sendmsg(qmi->sock, &msg, &iv, 1, sizeof(pkt));

> +		if (ret < 0)

> +			pr_err("failed to send lookup registration: %d\n", ret);

> +	}

> +	mutex_unlock(&qmi->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 DEL_SERVER control messages to this socket as

> + * matching services are registered.

> + */

> +int qmi_add_lookup(struct qmi_handle *qmi, unsigned int service,

> +		   unsigned int version, unsigned int instance)

> +{

> +	struct qmi_service *svc;

> +

> +	svc = kzalloc(sizeof(*svc), GFP_KERNEL);

> +	if (!svc)

> +		return -ENOMEM;

> +

> +	svc->service = service;

> +	svc->version = version;

> +	svc->instance = instance;

> +

> +	list_add(&svc->list_node, &qmi->lookups);

> +

> +	qmi_send_new_lookup(qmi, svc);

> +

> +	return 0;

> +}

> +EXPORT_SYMBOL(qmi_add_lookup);

> +**

> + * qmi_add_server() - register a service with the name service

> + * @qmi:	qmi handle

> + * @service:	type of the service

> + * @instance:	instance of the service

> + *


version not documented.


> + * Returns 0 on success, negative errno on failure.

> + *

> + * Register a new service with the name service. This allows clients to find

> + * and start sending messages to the client associated with @qmi.

> + */

> +int qmi_add_server(struct qmi_handle *qmi, unsigned int service,

> +		   unsigned int version, unsigned int instance)

> +{

> +	struct qmi_service *svc;

> +

> +	svc = kzalloc(sizeof(*svc), GFP_KERNEL);

> +	if (!svc)

> +		return -ENOMEM;

> +

> +static struct socket *qmi_sock_create(struct qmi_handle *qmi,

> +				      struct sockaddr_qrtr *sq)

> +{

> +	struct socket *sock;

> +	int sl = sizeof(*sq);

> +	int ret;

> +

> +	ret = sock_create_kern(&init_net, AF_QIPCRTR, SOCK_DGRAM,

> +			       PF_QIPCRTR, &sock);

> +	if (ret < 0)

> +		return ERR_PTR(ret);

> +

> +	ret = kernel_getsockname(sock, (struct sockaddr *)sq, &sl);

> +	if (ret < 0) {

> +		sock_release(sock);

> +		return ERR_PTR(ret);

> +	}

> +

> +	sock->sk->sk_user_data = qmi;

> +	sock->sk->sk_data_ready = qmi_data_ready;

> +	sock->sk->sk_error_report = qmi_data_ready;

> +

> +	return sock;

> +}

> +

> +/**

> + * 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??


> + * @handlers:	NULL-terminated list of QMI message handlers

> + *

recv_buf_size and ops not documented

> + * Returns 0 on success, negative errno on failure.

> + *

> + * This initializes the QMI client handle to allow sending and receiving QMI

> + * messages. As messages are received the appropriate handler will be invoked.

> + */

> +int qmi_handle_init(struct qmi_handle *qmi, size_t recv_buf_size,

> +		    struct qmi_ops *ops, struct qmi_msg_handler *handlers)

> +{

> +	int ret;

> +

>

.....

> + */

> +ssize_t qmi_send_response(struct qmi_handle *qmi, struct sockaddr_qrtr *sq,

> +			  struct qmi_txn *txn, int msg_id, size_t len,

> +			  struct qmi_elem_info *ei, const void *c_struct)

> +{

> +	return qmi_send_message(qmi, sq, txn, QMI_RESPONSE, msg_id, len, ei,

> +				c_struct);

> +}

> +EXPORT_SYMBOL(qmi_send_response);

> +

> +/**

> + * 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?

> + * @msg_id:	message id

> + * @len:	max length of the QMI message

> + * @ei:		QMI message description

> + * @c_struct:	object to be encoded

> + *

> + * Returns 0 on success, negative errno on failure.

> + */

> +ssize_t qmi_send_indication(struct qmi_handle *qmi, struct sockaddr_qrtr *sq,

> +			    int msg_id, size_t len, struct qmi_elem_info *ei,

> +			    const void *c_struct)

> +{

> +	struct qmi_txn txn;

> +	ssize_t rval;

...

> 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 ??

> @@ -105,6 +105,158 @@ struct qmi_response_type_v01 {

>   

>   extern struct qmi_elem_info qmi_response_type_v01_ei[];

>   

..
> +struct qmi_handle {

> +	struct socket *sock;

> +	struct mutex sock_lock;

> +

> +	struct sockaddr_qrtr sq;

> +

> +	struct work_struct work;

> +	struct workqueue_struct *wq;

> +

> +	void *recv_buf;

> +	size_t recv_buf_size;

> +

> +	struct list_head lookups;

> +	struct list_head lookup_results;

> +	struct list_head services;

> +

> +	struct qmi_ops ops;

> +

> +	struct idr txns;

> +	struct mutex txn_lock;

> +

> +	struct qmi_msg_handler *handlers;

> +};

> +

> +int qmi_add_lookup(struct qmi_handle *qmi, unsigned int service,

> +		   unsigned int version, unsigned int instance);

> +int qmi_add_server(struct qmi_handle *qmi, unsigned int service,

> +		   unsigned int version, unsigned int instance);

> +

> +int qmi_handle_init(struct qmi_handle *qmi, size_t max_msg_len,

> +		    struct qmi_ops *ops, struct qmi_msg_handler *handlers);

> +void qmi_handle_release(struct qmi_handle *qmi);

> +

> +ssize_t qmi_send_request(struct qmi_handle *qmi, struct sockaddr_qrtr *sq,

> +			 struct qmi_txn *txn, int msg_id, size_t len,

> +			 struct qmi_elem_info *ei, const void *c_struct);

> +ssize_t qmi_send_response(struct qmi_handle *qmi, struct sockaddr_qrtr *sq,

> +			  struct qmi_txn *txn, int msg_id, size_t len,

> +			  struct qmi_elem_info *ei, const void *c_struct);

> +ssize_t qmi_send_indication(struct qmi_handle *qmi, struct sockaddr_qrtr *sq,

> +			    int msg_id, size_t len, struct qmi_elem_info *ei,

> +			    const void *c_struct);

> +

>   void *qmi_encode_message(int type, unsigned int msg_id, size_t *len,

>   			 unsigned int txn_id, struct qmi_elem_info *ei,

>   			 const void *c_struct);

> @@ -112,4 +264,9 @@ void *qmi_encode_message(int type, unsigned int msg_id, size_t *len,

>   int qmi_decode_message(const void *buf, size_t len,

>   		       struct qmi_elem_info *ei, void *c_struct);

>   

> +int qmi_txn_init(struct qmi_handle *qmi, struct qmi_txn *txn,

> +		 struct qmi_elem_info *ei, void *c_struct);

> +int qmi_txn_wait(struct qmi_txn *txn, unsigned long timeout);

> +void qmi_txn_cancel(struct qmi_txn *txn);


Same comment as first patch, should we consider adding dummy functions 
to facilitate COMPILE_TEST for drivers using this apis?

> +

>   #endif

> 

--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Andersson Nov. 17, 2017, 6:30 a.m. UTC | #2
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 <linux/platform_device.h>

> 

> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Chris Lew Nov. 18, 2017, 2:11 a.m. UTC | #3
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(&qmi->txn_lock);

> +		txn = idr_find(&qmi->txns, hdr->txn_id);

> +		if (txn)

> +			mutex_lock(&txn->lock);

> +		mutex_unlock(&qmi->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(&txn->completion);

> +

> +		mutex_unlock(&txn->lock);

> +	} else if (txn) {

> +		qmi_invoke_handler(qmi, sq, txn, buf, len);

> +

> +		mutex_unlock(&txn->lock);

> +	} else {

> +		/* Create a txn based on the txn_id of the incoming message */

> +		memset(&tmp_txn, 0, sizeof(tmp_txn));

> +		tmp_txn.id = hdr->txn_id;

> +

> +		qmi_invoke_handler(qmi, sq, &tmp_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(&txn->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
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Andersson Nov. 21, 2017, 10:42 p.m. UTC | #4
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(&qmi->txn_lock);

> > +		txn = idr_find(&qmi->txns, hdr->txn_id);

> > +		if (txn)

> > +			mutex_lock(&txn->lock);

> > +		mutex_unlock(&qmi->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(&txn->completion);

> > +

> > +		mutex_unlock(&txn->lock);

> > +	} else if (txn) {

> > +		qmi_invoke_handler(qmi, sq, txn, buf, len);

> > +

> > +		mutex_unlock(&txn->lock);

> > +	} else {

> > +		/* Create a txn based on the txn_id of the incoming message */

> > +		memset(&tmp_txn, 0, sizeof(tmp_txn));

> > +		tmp_txn.id = hdr->txn_id;

> > +

> > +		qmi_invoke_handler(qmi, sq, &tmp_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(&txn->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
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox series

Patch

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 000000000000..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 <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/platform_device.h>
+#include <linux/qrtr.h>
+#include <linux/net.h>
+#include <linux/completion.h>
+#include <linux/idr.h>
+#include <linux/string.h>
+#include <net/sock.h>
+#include <linux/workqueue.h>
+#include <linux/soc/qcom/qmi.h>
+
+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 = &qmi->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(&svc->list_node, &qmi->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 server.
+ */
+static void qmi_recv_del_server(struct qmi_handle *qmi,
+				unsigned int node, unsigned int port)
+{
+	struct qmi_ops *ops = &qmi->ops;
+	struct qmi_service *svc;
+	struct qmi_service *tmp;
+
+	list_for_each_entry_safe(svc, tmp, &qmi->lookup_results, list_node) {
+		if (node != -1 && svc->node != node)
+			continue;
+		if (port != -1 && svc->port != port)
+			continue;
+
+		if (ops->del_server)
+			ops->del_server(qmi, svc);
+
+		list_del(&svc->list_node);
+		kfree(svc);
+	}
+}
+
+/**
+ * qmi_recv_bye() - handler of BYE control message
+ * @qmi:	qmi handle
+ * @node:	id of the dying node
+ *
+ * Signals the client that all previously registered services on this node are
+ * now gone and then calls the bye callback to allow the client client further
+ * cleaning up resources associated with this remote.
+ */
+static void qmi_recv_bye(struct qmi_handle *qmi,
+			 unsigned int node)
+{
+	struct qmi_ops *ops = &qmi->ops;
+
+	qmi_recv_del_server(qmi, node, -1);
+
+	if (ops->bye)
+		ops->bye(qmi, node);
+}
+
+/**
+ * qmi_recv_del_client() - handler of DEL_CLIENT control message
+ * @qmi:	qmi handle
+ * @node:	node of the dying client
+ * @port:	port of the dying client
+ *
+ * Signals the client about a dying client, by calling the del_client callback.
+ */
+static void qmi_recv_del_client(struct qmi_handle *qmi,
+				unsigned int node, unsigned int port)
+{
+	struct qmi_ops *ops = &qmi->ops;
+
+	if (ops->del_client)
+		ops->del_client(qmi, node, port);
+}
+
+static void qmi_recv_ctrl_pkt(struct qmi_handle *qmi,
+			      const void *buf, size_t len)
+{
+	const struct qrtr_ctrl_pkt *pkt = buf;
+
+	if (len < sizeof(struct qrtr_ctrl_pkt)) {
+		pr_debug("ignoring short control packet\n");
+		return;
+	}
+
+	switch (le32_to_cpu(pkt->cmd)) {
+	case QRTR_TYPE_BYE:
+		qmi_recv_bye(qmi, le32_to_cpu(pkt->client.node));
+		break;
+	case QRTR_TYPE_NEW_SERVER:
+		qmi_recv_new_server(qmi,
+				    le32_to_cpu(pkt->server.service),
+				    le32_to_cpu(pkt->server.instance),
+				    le32_to_cpu(pkt->server.node),
+				    le32_to_cpu(pkt->server.port));
+		break;
+	case QRTR_TYPE_DEL_SERVER:
+		qmi_recv_del_server(qmi,
+				    le32_to_cpu(pkt->server.node),
+				    le32_to_cpu(pkt->server.port));
+		break;
+	case QRTR_TYPE_DEL_CLIENT:
+		qmi_recv_del_client(qmi,
+				    le32_to_cpu(pkt->client.node),
+				    le32_to_cpu(pkt->client.port));
+		break;
+	}
+}
+
+static void qmi_send_new_lookup(struct qmi_handle *qmi, struct qmi_service *svc)
+{
+	struct qrtr_ctrl_pkt pkt;
+	struct sockaddr_qrtr sq;
+	struct msghdr msg = {0};
+	struct kvec iv = { &pkt, sizeof(pkt) };
+	int ret;
+
+	memset(&pkt, 0, sizeof(pkt));
+	pkt.cmd = cpu_to_le32(QRTR_TYPE_NEW_LOOKUP);
+	pkt.server.service = cpu_to_le32(svc->service);
+	pkt.server.instance = cpu_to_le32(svc->version | svc->instance << 8);
+
+	sq.sq_family = qmi->sq.sq_family;
+	sq.sq_node = qmi->sq.sq_node;
+	sq.sq_port = QRTR_PORT_CTRL;
+
+	msg.msg_name = &sq;
+	msg.msg_namelen = sizeof(sq);
+
+	mutex_lock(&qmi->sock_lock);
+	if (qmi->sock) {
+		ret = kernel_sendmsg(qmi->sock, &msg, &iv, 1, sizeof(pkt));
+		if (ret < 0)
+			pr_err("failed to send lookup registration: %d\n", ret);
+	}
+	mutex_unlock(&qmi->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
+ *
+ * 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 DEL_SERVER control messages to this socket as
+ * matching services are registered.
+ */
+int qmi_add_lookup(struct qmi_handle *qmi, unsigned int service,
+		   unsigned int version, unsigned int instance)
+{
+	struct qmi_service *svc;
+
+	svc = kzalloc(sizeof(*svc), GFP_KERNEL);
+	if (!svc)
+		return -ENOMEM;
+
+	svc->service = service;
+	svc->version = version;
+	svc->instance = instance;
+
+	list_add(&svc->list_node, &qmi->lookups);
+
+	qmi_send_new_lookup(qmi, svc);
+
+	return 0;
+}
+EXPORT_SYMBOL(qmi_add_lookup);
+
+static void qmi_send_new_server(struct qmi_handle *qmi, struct qmi_service *svc)
+{
+	struct qrtr_ctrl_pkt pkt;
+	struct sockaddr_qrtr sq;
+	struct msghdr msg = {0};
+	struct kvec iv = { &pkt, sizeof(pkt) };
+	int ret;
+
+	memset(&pkt, 0, sizeof(pkt));
+	pkt.cmd = cpu_to_le32(QRTR_TYPE_NEW_SERVER);
+	pkt.server.service = cpu_to_le32(svc->service);
+	pkt.server.instance = cpu_to_le32(svc->version | svc->instance << 8);
+	pkt.server.node = qmi->sq.sq_node;
+	pkt.server.port = qmi->sq.sq_port;
+
+	sq.sq_family = qmi->sq.sq_family;
+	sq.sq_node = qmi->sq.sq_node;
+	sq.sq_port = QRTR_PORT_CTRL;
+
+	msg.msg_name = &sq;
+	msg.msg_namelen = sizeof(sq);
+
+	mutex_lock(&qmi->sock_lock);
+	if (qmi->sock) {
+		ret = kernel_sendmsg(qmi->sock, &msg, &iv, 1, sizeof(pkt));
+		if (ret < 0)
+			pr_err("send service registration failed: %d\n", ret);
+	}
+	mutex_unlock(&qmi->sock_lock);
+}
+
+/**
+ * qmi_add_server() - register a service with the name service
+ * @qmi:	qmi handle
+ * @service:	type of the service
+ * @instance:	instance of the service
+ *
+ * Returns 0 on success, negative errno on failure.
+ *
+ * Register a new service with the name service. This allows clients to find
+ * and start sending messages to the client associated with @qmi.
+ */
+int qmi_add_server(struct qmi_handle *qmi, unsigned int service,
+		   unsigned int version, unsigned int instance)
+{
+	struct qmi_service *svc;
+
+	svc = kzalloc(sizeof(*svc), GFP_KERNEL);
+	if (!svc)
+		return -ENOMEM;
+
+	svc->service = service;
+	svc->version = version;
+	svc->instance = instance;
+
+	list_add(&svc->list_node, &qmi->services);
+
+	qmi_send_new_server(qmi, svc);
+
+	return 0;
+}
+EXPORT_SYMBOL(qmi_add_server);
+
+/**
+ * qmi_txn_init() - allocate transaction id within the given QMI handle
+ * @qmi:	QMI handle
+ * @txn:	transaction context
+ * @ei:		description of how to decode a matching response (optional)
+ * @c_struct:	pointer to the object to decode the response into (optional)
+ *
+ * Returns transaction id on success, negative errno on failure.
+ *
+ * This allocates a transaction id within the QMI handle. If @ei and @c_struct
+ * are specified any responses to this transaction will be decoded as described
+ * by @ei into @c_struct.
+ *
+ * A client calling qmi_txn_init() must call either qmi_txn_wait() or
+ * qmi_txn_cancel() to free up the allocated resources.
+ */
+int qmi_txn_init(struct qmi_handle *qmi, struct qmi_txn *txn,
+		 struct qmi_elem_info *ei, void *c_struct)
+{
+	int ret;
+
+	memset(txn, 0, sizeof(*txn));
+
+	mutex_init(&txn->lock);
+	init_completion(&txn->completion);
+	txn->qmi = qmi;
+	txn->ei = ei;
+	txn->dest = c_struct;
+
+	mutex_lock(&qmi->txn_lock);
+	ret = idr_alloc_cyclic(&qmi->txns, txn, 0, INT_MAX, GFP_KERNEL);
+	if (ret < 0)
+		pr_err("failed to allocate transaction id\n");
+
+	txn->id = ret;
+	mutex_unlock(&qmi->txn_lock);
+
+	return ret;
+}
+EXPORT_SYMBOL(qmi_txn_init);
+
+/**
+ * qmi_txn_wait() - wait for a response on a transaction
+ * @txn:	transaction handle
+ * @timeout:	timeout, in jiffies
+ *
+ * Returns the transaction response on success, negative errno on failure.
+ *
+ * If the transaction is decoded by the means of @ei and @c_struct the return
+ * value will be the returned value of qmi_decode_message(), otherwise it's up
+ * to the specified message handler to fill out the result.
+ */
+int qmi_txn_wait(struct qmi_txn *txn, unsigned long timeout)
+{
+	struct qmi_handle *qmi = txn->qmi;
+	int ret;
+
+	ret = wait_for_completion_interruptible_timeout(&txn->completion,
+							timeout);
+
+	mutex_lock(&qmi->txn_lock);
+	mutex_lock(&txn->lock);
+	idr_remove(&qmi->txns, txn->id);
+	mutex_unlock(&txn->lock);
+	mutex_unlock(&qmi->txn_lock);
+
+	if (ret < 0)
+		return ret;
+	else if (ret == 0)
+		return -ETIMEDOUT;
+	else
+		return txn->result;
+}
+EXPORT_SYMBOL(qmi_txn_wait);
+
+/**
+ * qmi_txn_cancel() - cancel an ongoing transaction
+ * @txn:	transaction id
+ */
+void qmi_txn_cancel(struct qmi_txn *txn)
+{
+	struct qmi_handle *qmi = txn->qmi;
+
+	mutex_lock(&qmi->txn_lock);
+	mutex_lock(&txn->lock);
+	idr_remove(&qmi->txns, txn->id);
+	mutex_unlock(&txn->lock);
+	mutex_unlock(&qmi->txn_lock);
+}
+EXPORT_SYMBOL(qmi_txn_cancel);
+
+/**
+ * qmi_invoke_handler() - find and invoke a handler for a message
+ * @qmi:	qmi handle
+ * @sq:		sockaddr of the sender
+ * @txn:	transaction object for the message
+ * @buf:	buffer containing the message
+ * @len:	length of @buf
+ *
+ * Find handler and invoke handler for the incoming message.
+ */
+static void qmi_invoke_handler(struct qmi_handle *qmi, struct sockaddr_qrtr *sq,
+			       struct qmi_txn *txn, const void *buf, size_t len)
+{
+	struct qmi_msg_handler *handler;
+	const struct qmi_header *hdr = buf;
+	void *dest;
+	int ret;
+
+	if (!qmi->handlers)
+		return;
+
+	for (handler = qmi->handlers; handler->fn; handler++) {
+		if (handler->type == hdr->type &&
+		    handler->msg_id == hdr->msg_id)
+			break;
+	}
+
+	if (!handler->fn)
+		return;
+
+	dest = kzalloc(handler->decoded_size, GFP_KERNEL);
+	if (!dest)
+		return;
+
+	ret = qmi_decode_message(buf, len, handler->ei, dest);
+	if (ret < 0)
+		pr_err("failed to decode incoming message\n");
+	else
+		handler->fn(qmi, sq, txn, dest);
+
+	kfree(dest);
+}
+
+/**
+ * qmi_handle_net_reset() - invoked to handle ENETRESET on a QMI handle
+ * @qmi:	the QMI context
+ *
+ * As a result of registering a name service with the QRTR all open sockets are
+ * flagged with ENETRESET and this function will be called. The typical case is
+ * the initial boot, where this signals that the local node id has been
+ * configured and as such any bound sockets needs to be rebound. So close the
+ * socket, inform the client and re-initialize the socket.
+ *
+ * For clients it's generally sufficient to react to the del_server callbacks,
+ * but server code is expected to treat the net_reset callback as a "bye" from
+ * all nodes.
+ *
+ * Finally the QMI handle will send out registration requests for any lookups
+ * and services.
+ */
+static void qmi_handle_net_reset(struct qmi_handle *qmi)
+{
+	struct sockaddr_qrtr sq;
+	struct qmi_service *svc;
+	struct socket *sock;
+
+	sock = qmi_sock_create(qmi, &sq);
+	if (IS_ERR(sock))
+		return;
+
+	mutex_lock(&qmi->sock_lock);
+	sock_release(qmi->sock);
+	qmi->sock = NULL;
+	mutex_unlock(&qmi->sock_lock);
+
+	qmi_recv_del_server(qmi, -1, -1);
+
+	if (qmi->ops.net_reset)
+		qmi->ops.net_reset(qmi);
+
+	mutex_lock(&qmi->sock_lock);
+	qmi->sock = sock;
+	qmi->sq = sq;
+	mutex_unlock(&qmi->sock_lock);
+
+	list_for_each_entry(svc, &qmi->lookups, list_node)
+		qmi_send_new_lookup(qmi, svc);
+
+	list_for_each_entry(svc, &qmi->services, list_node)
+		qmi_send_new_server(qmi, svc);
+}
+
+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(&qmi->txn_lock);
+		txn = idr_find(&qmi->txns, hdr->txn_id);
+		if (txn)
+			mutex_lock(&txn->lock);
+		mutex_unlock(&qmi->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(&txn->completion);
+
+		mutex_unlock(&txn->lock);
+	} else if (txn) {
+		qmi_invoke_handler(qmi, sq, txn, buf, len);
+
+		mutex_unlock(&txn->lock);
+	} else {
+		/* Create a txn based on the txn_id of the incoming message */
+		memset(&tmp_txn, 0, sizeof(tmp_txn));
+		tmp_txn.id = hdr->txn_id;
+
+		qmi_invoke_handler(qmi, sq, &tmp_txn, buf, len);
+	}
+}
+
+static void qmi_data_ready_work(struct work_struct *work)
+{
+	struct qmi_handle *qmi = container_of(work, struct qmi_handle, work);
+	struct qmi_ops *ops = &qmi->ops;
+	struct sockaddr_qrtr sq;
+	struct msghdr msg = { .msg_name = &sq, .msg_namelen = sizeof(sq) };
+	struct kvec iv;
+	ssize_t msglen;
+
+	for (;;) {
+		iv.iov_base = qmi->recv_buf;
+		iv.iov_len = qmi->recv_buf_size;
+
+		mutex_lock(&qmi->sock_lock);
+		if (qmi->sock)
+			msglen = kernel_recvmsg(qmi->sock, &msg, &iv, 1,
+						iv.iov_len, MSG_DONTWAIT);
+		else
+			msglen = -EPIPE;
+		mutex_unlock(&qmi->sock_lock);
+		if (msglen == -EAGAIN)
+			break;
+
+		if (msglen == -ENETRESET) {
+			qmi_handle_net_reset(qmi);
+
+			/* The old qmi->sock is gone, our work is done */
+			break;
+		}
+
+		if (msglen < 0) {
+			pr_err("qmi recvmsg failed: %zd\n", msglen);
+			break;
+		}
+
+		if (sq.sq_node == qmi->sq.sq_node &&
+		    sq.sq_port == QRTR_PORT_CTRL) {
+			qmi_recv_ctrl_pkt(qmi, qmi->recv_buf, msglen);
+		} else if (ops->msg_handler) {
+			ops->msg_handler(qmi, &sq, qmi->recv_buf, msglen);
+		} else {
+			qmi_handle_message(qmi, &sq, qmi->recv_buf, msglen);
+		}
+	}
+}
+
+static void qmi_data_ready(struct sock *sk)
+{
+	struct qmi_handle *qmi = sk->sk_user_data;
+
+	/*
+	 * This will be NULL if we receive data while being in
+	 * qmi_handle_release()
+	 */
+	if (!qmi)
+		return;
+
+	queue_work(qmi->wq, &qmi->work);
+}
+
+static struct socket *qmi_sock_create(struct qmi_handle *qmi,
+				      struct sockaddr_qrtr *sq)
+{
+	struct socket *sock;
+	int sl = sizeof(*sq);
+	int ret;
+
+	ret = sock_create_kern(&init_net, AF_QIPCRTR, SOCK_DGRAM,
+			       PF_QIPCRTR, &sock);
+	if (ret < 0)
+		return ERR_PTR(ret);
+
+	ret = kernel_getsockname(sock, (struct sockaddr *)sq, &sl);
+	if (ret < 0) {
+		sock_release(sock);
+		return ERR_PTR(ret);
+	}
+
+	sock->sk->sk_user_data = qmi;
+	sock->sk->sk_data_ready = qmi_data_ready;
+	sock->sk->sk_error_report = qmi_data_ready;
+
+	return sock;
+}
+
+/**
+ * qmi_handle_init() - initialize a QMI client handle
+ * @qmi:	QMI handle to initialize
+ * @max_msg_len: maximum size of incoming message
+ * @handlers:	NULL-terminated list of QMI message handlers
+ *
+ * Returns 0 on success, negative errno on failure.
+ *
+ * This initializes the QMI client handle to allow sending and receiving QMI
+ * messages. As messages are received the appropriate handler will be invoked.
+ */
+int qmi_handle_init(struct qmi_handle *qmi, size_t recv_buf_size,
+		    struct qmi_ops *ops, struct qmi_msg_handler *handlers)
+{
+	int ret;
+
+	mutex_init(&qmi->txn_lock);
+	mutex_init(&qmi->sock_lock);
+
+	idr_init(&qmi->txns);
+
+	INIT_LIST_HEAD(&qmi->lookups);
+	INIT_LIST_HEAD(&qmi->lookup_results);
+	INIT_LIST_HEAD(&qmi->services);
+
+	INIT_WORK(&qmi->work, qmi_data_ready_work);
+
+	qmi->handlers = handlers;
+	if (ops)
+		qmi->ops = *ops;
+
+	if (recv_buf_size < sizeof(struct qrtr_ctrl_pkt))
+		recv_buf_size = sizeof(struct qrtr_ctrl_pkt);
+	else
+		recv_buf_size += sizeof(struct qmi_header);
+
+	qmi->recv_buf_size = recv_buf_size;
+	qmi->recv_buf = kzalloc(recv_buf_size, GFP_KERNEL);
+	if (!qmi->recv_buf)
+		return -ENOMEM;
+
+	qmi->wq = alloc_workqueue("qmi_msg_handler", WQ_UNBOUND, 1);
+	if (!qmi->wq) {
+		ret = -ENOMEM;
+		goto err_free_recv_buf;
+	}
+
+	qmi->sock = qmi_sock_create(qmi, &qmi->sq);
+	if (IS_ERR(qmi->sock)) {
+		pr_err("failed to create QMI socket\n");
+		ret = PTR_ERR(qmi->sock);
+		goto err_destroy_wq;
+	}
+
+	return 0;
+
+err_destroy_wq:
+	destroy_workqueue(qmi->wq);
+err_free_recv_buf:
+	kfree(qmi->recv_buf);
+
+	return ret;
+}
+EXPORT_SYMBOL(qmi_handle_init);
+
+/**
+ * qmi_handle_release() - release the QMI client handle
+ * @qmi:	QMI client handle
+ *
+ * This closes the underlying socket and stops any handling of QMI messages.
+ */
+void qmi_handle_release(struct qmi_handle *qmi)
+{
+	struct socket *sock = qmi->sock;
+	struct qmi_service *svc, *tmp;
+
+	sock->sk->sk_user_data = NULL;
+	cancel_work_sync(&qmi->work);
+
+	qmi_recv_del_server(qmi, -1, -1);
+
+	mutex_lock(&qmi->sock_lock);
+	sock_release(sock);
+	qmi->sock = NULL;
+	mutex_unlock(&qmi->sock_lock);
+
+	destroy_workqueue(qmi->wq);
+
+	idr_destroy(&qmi->txns);
+
+	kfree(qmi->recv_buf);
+
+	/* Free registered lookup requests */
+	list_for_each_entry_safe(svc, tmp, &qmi->lookups, list_node) {
+		list_del(&svc->list_node);
+		kfree(svc);
+	}
+
+	/* Free registered service information */
+	list_for_each_entry_safe(svc, tmp, &qmi->services, list_node) {
+		list_del(&svc->list_node);
+		kfree(svc);
+	}
+}
+EXPORT_SYMBOL(qmi_handle_release);
+
+/**
+ * qmi_send_message() - send a QMI message
+ * @qmi:	QMI client handle
+ * @sq:		destination sockaddr
+ * @txn:	transaction object to use for the message
+ * @type:	type of message to send
+ * @msg_id:	message id
+ * @len:	max length of the QMI message
+ * @ei:		QMI message description
+ * @c_struct:	object to be encoded
+ *
+ * Returns 0 on success, negative errno on failure.
+ *
+ * This function encodes @c_struct using @ei into a message of type @type,
+ * with @msg_id and @txn into a buffer of maximum size @len, and sends this to
+ * @sq.
+ */
+static ssize_t qmi_send_message(struct qmi_handle *qmi,
+				struct sockaddr_qrtr *sq, struct qmi_txn *txn,
+				int type, int msg_id, size_t len,
+				struct qmi_elem_info *ei, const void *c_struct)
+{
+	struct msghdr msghdr = {0};
+	struct kvec iv;
+	void *msg;
+	int ret;
+
+	msg = qmi_encode_message(type,
+				 msg_id, &len,
+				 txn->id, ei,
+				 c_struct);
+	if (IS_ERR(msg))
+		return PTR_ERR(msg);
+
+	iv.iov_base = msg;
+	iv.iov_len = len;
+
+	if (sq) {
+		msghdr.msg_name = sq;
+		msghdr.msg_namelen = sizeof(*sq);
+	}
+
+	mutex_lock(&qmi->sock_lock);
+	if (qmi->sock) {
+		ret = kernel_sendmsg(qmi->sock, &msghdr, &iv, 1, len);
+		if (ret < 0)
+			pr_err("failed to send QMI message\n");
+	} else {
+		ret = -EPIPE;
+	}
+	mutex_unlock(&qmi->sock_lock);
+
+	kfree(msg);
+
+	return ret < 0 ? ret : 0;
+}
+
+/**
+ * qmi_send_request() - send a request QMI message
+ * @qmi:	QMI client handle
+ * @sq:		destination sockaddr
+ * @txn:	transaction object to use for the message
+ * @msg_id:	message id
+ * @len:	max length of the QMI message
+ * @ei:		QMI message description
+ * @c_struct:	object to be encoded
+ *
+ * Returns 0 on success, negative errno on failure.
+ */
+ssize_t qmi_send_request(struct qmi_handle *qmi, struct sockaddr_qrtr *sq,
+			 struct qmi_txn *txn, int msg_id, size_t len,
+			 struct qmi_elem_info *ei, const void *c_struct)
+{
+	return qmi_send_message(qmi, sq, txn, QMI_REQUEST, msg_id, len, ei,
+				c_struct);
+}
+EXPORT_SYMBOL(qmi_send_request);
+
+/**
+ * qmi_send_response() - send a response QMI message
+ * @qmi:	QMI client handle
+ * @sq:		destination sockaddr
+ * @txn:	transaction object to use for the message
+ * @msg_id:	message id
+ * @len:	max length of the QMI message
+ * @ei:		QMI message description
+ * @c_struct:	object to be encoded
+ *
+ * Returns 0 on success, negative errno on failure.
+ */
+ssize_t qmi_send_response(struct qmi_handle *qmi, struct sockaddr_qrtr *sq,
+			  struct qmi_txn *txn, int msg_id, size_t len,
+			  struct qmi_elem_info *ei, const void *c_struct)
+{
+	return qmi_send_message(qmi, sq, txn, QMI_RESPONSE, msg_id, len, ei,
+				c_struct);
+}
+EXPORT_SYMBOL(qmi_send_response);
+
+/**
+ * qmi_send_indication() - send an indication QMI message
+ * @qmi:	QMI client handle
+ * @sq:		destination sockaddr
+ * @txn:	transaction object to use for the message
+ * @msg_id:	message id
+ * @len:	max length of the QMI message
+ * @ei:		QMI message description
+ * @c_struct:	object to be encoded
+ *
+ * Returns 0 on success, negative errno on failure.
+ */
+ssize_t qmi_send_indication(struct qmi_handle *qmi, struct sockaddr_qrtr *sq,
+			    int msg_id, size_t len, struct qmi_elem_info *ei,
+			    const void *c_struct)
+{
+	struct qmi_txn txn;
+	ssize_t rval;
+	int ret;
+
+	ret = qmi_txn_init(qmi, &txn, NULL, NULL);
+	if (ret < 0)
+		return ret;
+
+	rval = qmi_send_message(qmi, sq, &txn, QMI_INDICATION, msg_id, len, ei,
+				c_struct);
+
+	/* We don't care about future messages on this txn */
+	qmi_txn_cancel(&txn);
+
+	return rval;
+}
+EXPORT_SYMBOL(qmi_send_indication);
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
@@ -105,6 +105,158 @@  struct qmi_response_type_v01 {
 
 extern struct qmi_elem_info qmi_response_type_v01_ei[];
 
+/**
+ * struct qmi_service - context to track lookup-results
+ * @service:	service type
+ * @version:	version of the @service
+ * @instance:	instance id of the @service
+ * @node:	node of the service
+ * @port:	port of the service
+ * @priv:	handle for client's use
+ * @list_node:	list_head for house keeping
+ */
+struct qmi_service {
+	unsigned int service;
+	unsigned int version;
+	unsigned int instance;
+
+	unsigned int node;
+	unsigned int port;
+
+	void *priv;
+	struct list_head list_node;
+};
+
+struct qmi_handle;
+
+/**
+ * struct qmi_ops - callbacks for qmi_handle
+ * @new_server:		inform client of a new_server lookup-result, returning
+ *                      successfully from this call causes the library to call
+ *                      @del_server as the service is removed from the
+ *                      lookup-result. @priv of the qmi_service can be used by
+ *                      the client
+ * @del_server:		inform client of a del_server lookup-result
+ * @net_reset:		inform client that the name service was restarted and
+ *                      that and any state needs to be released
+ * @msg_handler:	invoked for incoming messages, allows a client to
+ *                      override the usual QMI message handler
+ * @bye:                inform a client that all clients from a node are gone
+ * @del_client:         inform a client that a particular client is gone
+ */
+struct qmi_ops {
+	int (*new_server)(struct qmi_handle *qmi, struct qmi_service *svc);
+	void (*del_server)(struct qmi_handle *qmi, struct qmi_service *svc);
+	void (*net_reset)(struct qmi_handle *qmi);
+	void (*msg_handler)(struct qmi_handle *qmi, struct sockaddr_qrtr *sq,
+			    const void *data, size_t count);
+	void (*bye)(struct qmi_handle *qmi, unsigned int node);
+	void (*del_client)(struct qmi_handle *qmi,
+			   unsigned int node, unsigned int port);
+};
+
+/**
+ * struct qmi_txn - transaction context
+ * @qmi:	QMI handle this transaction is associated with
+ * @id:		transaction id
+ * @lock:	for synchronization between handler and waiter of messages
+ * @completion:	completion object as the transaction receives a response
+ * @result:	result code for the completed transaction
+ * @ei:		description of the QMI encoded response (optional)
+ * @dest:	destination buffer to decode message into (optional)
+ */
+struct qmi_txn {
+	struct qmi_handle *qmi;
+
+	int id;
+
+	struct mutex lock;
+	struct completion completion;
+	int result;
+
+	struct qmi_elem_info *ei;
+	void *dest;
+};
+
+/**
+ * struct qmi_msg_handler - description of QMI message handler
+ * @type:	type of message
+ * @msg_id:	message id
+ * @ei:		description of the QMI encoded message
+ * @decoded_size:	size of the decoded object
+ * @fn:		function to invoke as the message is decoded
+ */
+struct qmi_msg_handler {
+	unsigned int type;
+	unsigned int msg_id;
+
+	struct qmi_elem_info *ei;
+
+	size_t decoded_size;
+	void (*fn)(struct qmi_handle *qmi, struct sockaddr_qrtr *sq,
+		   struct qmi_txn *txn, const void *decoded);
+};
+
+/**
+ * struct qmi_handle - QMI context
+ * @sock:	socket handle
+ * @sock_lock:	synchronization of @sock modifications
+ * @sq:		sockaddr of @sock
+ * @work:	work for handling incoming messages
+ * @wq:		workqueue to post @work on
+ * @recv_buf:	scratch buffer for handling incoming messages
+ * @recv_buf_size:	size of @recv_buf
+ * @lookups:		list of registered lookup requests
+ * @lookup_results:	list of lookup-results advertised to the client
+ * @services:		list of registered services (by this client)
+ * @ops:	reference to callbacks
+ * @txns:	outstanding transactions
+ * @txn_lock:	lock for modifications of @txns
+ * @handlers:	list of handlers for incoming messages
+ */
+struct qmi_handle {
+	struct socket *sock;
+	struct mutex sock_lock;
+
+	struct sockaddr_qrtr sq;
+
+	struct work_struct work;
+	struct workqueue_struct *wq;
+
+	void *recv_buf;
+	size_t recv_buf_size;
+
+	struct list_head lookups;
+	struct list_head lookup_results;
+	struct list_head services;
+
+	struct qmi_ops ops;
+
+	struct idr txns;
+	struct mutex txn_lock;
+
+	struct qmi_msg_handler *handlers;
+};
+
+int qmi_add_lookup(struct qmi_handle *qmi, unsigned int service,
+		   unsigned int version, unsigned int instance);
+int qmi_add_server(struct qmi_handle *qmi, unsigned int service,
+		   unsigned int version, unsigned int instance);
+
+int qmi_handle_init(struct qmi_handle *qmi, size_t max_msg_len,
+		    struct qmi_ops *ops, struct qmi_msg_handler *handlers);
+void qmi_handle_release(struct qmi_handle *qmi);
+
+ssize_t qmi_send_request(struct qmi_handle *qmi, struct sockaddr_qrtr *sq,
+			 struct qmi_txn *txn, int msg_id, size_t len,
+			 struct qmi_elem_info *ei, const void *c_struct);
+ssize_t qmi_send_response(struct qmi_handle *qmi, struct sockaddr_qrtr *sq,
+			  struct qmi_txn *txn, int msg_id, size_t len,
+			  struct qmi_elem_info *ei, const void *c_struct);
+ssize_t qmi_send_indication(struct qmi_handle *qmi, struct sockaddr_qrtr *sq,
+			    int msg_id, size_t len, struct qmi_elem_info *ei,
+			    const void *c_struct);
+
 void *qmi_encode_message(int type, unsigned int msg_id, size_t *len,
 			 unsigned int txn_id, struct qmi_elem_info *ei,
 			 const void *c_struct);
@@ -112,4 +264,9 @@  void *qmi_encode_message(int type, unsigned int msg_id, size_t *len,
 int qmi_decode_message(const void *buf, size_t len,
 		       struct qmi_elem_info *ei, void *c_struct);
 
+int qmi_txn_init(struct qmi_handle *qmi, struct qmi_txn *txn,
+		 struct qmi_elem_info *ei, void *c_struct);
+int qmi_txn_wait(struct qmi_txn *txn, unsigned long timeout);
+void qmi_txn_cancel(struct qmi_txn *txn);
+
 #endif