* [PATCH v4 1/5] soc: qcom: Introduce QMI encoder/decoder
2017-11-30 1:16 [PATCH v4 0/5] In-kernel QMI helpers and sysmon Bjorn Andersson
@ 2017-11-30 1:16 ` Bjorn Andersson
2017-11-30 17:17 ` Chris Lew
2017-12-01 9:10 ` Jitendra Sharma
2017-11-30 1:16 ` [PATCH v4 2/5] soc: qcom: Introduce QMI helpers Bjorn Andersson
` (3 subsequent siblings)
4 siblings, 2 replies; 24+ messages in thread
From: Bjorn Andersson @ 2017-11-30 1:16 UTC (permalink / raw)
To: Andy Gross, Ohad Ben-Cohen, Bjorn Andersson
Cc: Arun Kumar Neelakantam, Chris Lew, linux-kernel, linux-arm-msm,
linux-soc, linux-remoteproc
Add the helper library for encoding and decoding QMI encoded messages.
The implementation is taken from lib/qmi_encdec.c of the Qualcomm kernel
(msm-3.18).
Modifications has been made to the public API, source buffers has been
made const and the debug-logging part was omitted, for now.
Tested-By: Chris Lew <clew@codeaurora.org>
Tested-By: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
Changes since v3:
- Moved depends on ARCH_QCOM from patch 2
- Kerneldoc updates
- Style updates
- Dropped qrtr.h include from header file
- Rename is_array to array_type
Changes since v2:
- Checkpatch fixes
Changes since v1:
- None
drivers/soc/qcom/Kconfig | 9 +
drivers/soc/qcom/Makefile | 2 +
drivers/soc/qcom/qmi_encdec.c | 826 ++++++++++++++++++++++++++++++++++++++++++
include/linux/soc/qcom/qmi.h | 114 ++++++
4 files changed, 951 insertions(+)
create mode 100644 drivers/soc/qcom/qmi_encdec.c
create mode 100644 include/linux/soc/qcom/qmi.h
diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index b81374bb6713..2411df0427d9 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -35,6 +35,15 @@ config QCOM_PM
modes. It interface with various system drivers to put the cores in
low power modes.
+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
+ clients and this helpers provide the common functionality needed for
+ doing this from a kernel driver.
+
config QCOM_RMTFS_MEM
tristate "Qualcomm Remote Filesystem memory driver"
depends on ARCH_QCOM
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index 40c56f67e94a..37f85b45d0a1 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -3,6 +3,8 @@ obj-$(CONFIG_QCOM_GLINK_SSR) += glink_ssr.o
obj-$(CONFIG_QCOM_GSBI) += qcom_gsbi.o
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
obj-$(CONFIG_QCOM_RMTFS_MEM) += rmtfs_mem.o
obj-$(CONFIG_QCOM_SMD_RPM) += smd-rpm.o
obj-$(CONFIG_QCOM_SMEM) += smem.o
diff --git a/drivers/soc/qcom/qmi_encdec.c b/drivers/soc/qcom/qmi_encdec.c
new file mode 100644
index 000000000000..a197fc0114c3
--- /dev/null
+++ b/drivers/soc/qcom/qmi_encdec.c
@@ -0,0 +1,826 @@
+/*
+ * Copyright (c) 2012-2015, The Linux Foundation. All rights reserved.
+ * Copyright (C) 2017 Linaro Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * 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/slab.h>
+#include <linux/uaccess.h>
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/errno.h>
+#include <linux/string.h>
+#include <linux/soc/qcom/qmi.h>
+
+#define QMI_ENCDEC_ENCODE_TLV(type, length, p_dst) do { \
+ *p_dst++ = type; \
+ *p_dst++ = ((u8)((length) & 0xFF)); \
+ *p_dst++ = ((u8)(((length) >> 8) & 0xFF)); \
+} while (0)
+
+#define QMI_ENCDEC_DECODE_TLV(p_type, p_length, p_src) do { \
+ *p_type = (u8)*p_src++; \
+ *p_length = (u8)*p_src++; \
+ *p_length |= ((u8)*p_src) << 8; \
+} while (0)
+
+#define QMI_ENCDEC_ENCODE_N_BYTES(p_dst, p_src, size) \
+do { \
+ memcpy(p_dst, p_src, size); \
+ p_dst = (u8 *)p_dst + size; \
+ p_src = (u8 *)p_src + size; \
+} while (0)
+
+#define QMI_ENCDEC_DECODE_N_BYTES(p_dst, p_src, size) \
+do { \
+ memcpy(p_dst, p_src, size); \
+ p_dst = (u8 *)p_dst + size; \
+ p_src = (u8 *)p_src + size; \
+} while (0)
+
+#define UPDATE_ENCODE_VARIABLES(temp_si, buf_dst, \
+ encoded_bytes, tlv_len, encode_tlv, rc) \
+do { \
+ buf_dst = (u8 *)buf_dst + rc; \
+ encoded_bytes += rc; \
+ tlv_len += rc; \
+ temp_si = temp_si + 1; \
+ encode_tlv = 1; \
+} while (0)
+
+#define UPDATE_DECODE_VARIABLES(buf_src, decoded_bytes, rc) \
+do { \
+ buf_src = (u8 *)buf_src + rc; \
+ decoded_bytes += rc; \
+} while (0)
+
+#define TLV_LEN_SIZE sizeof(u16)
+#define TLV_TYPE_SIZE sizeof(u8)
+#define OPTIONAL_TLV_TYPE_START 0x10
+
+static int qmi_encode(struct qmi_elem_info *ei_array, void *out_buf,
+ const void *in_c_struct, u32 out_buf_len,
+ int enc_level);
+
+static int qmi_decode(struct qmi_elem_info *ei_array, void *out_c_struct,
+ const void *in_buf, u32 in_buf_len, int dec_level);
+
+/**
+ * skip_to_next_elem() - Skip to next element in the structure to be encoded
+ * @ei_array: Struct info describing the element to be skipped.
+ * @level: Depth level of encoding/decoding to identify nested structures.
+ *
+ * This function is used while encoding optional elements. If the flag
+ * corresponding to an optional element is not set, then encoding the
+ * optional element can be skipped. This function can be used to perform
+ * that operation.
+ *
+ * Return: struct info of the next element that can be encoded.
+ */
+static struct qmi_elem_info *skip_to_next_elem(struct qmi_elem_info *ei_array,
+ int level)
+{
+ struct qmi_elem_info *temp_ei = ei_array;
+ u8 tlv_type;
+
+ if (level > 1) {
+ temp_ei = temp_ei + 1;
+ } else {
+ do {
+ tlv_type = temp_ei->tlv_type;
+ temp_ei = temp_ei + 1;
+ } while (tlv_type == temp_ei->tlv_type);
+ }
+
+ return temp_ei;
+}
+
+/**
+ * qmi_calc_min_msg_len() - Calculate the minimum length of a QMI message
+ * @ei_array: Struct info array describing the structure.
+ * @level: Level to identify the depth of the nested structures.
+ *
+ * Return: Expected minimum length of the QMI message or 0 on error.
+ */
+static int qmi_calc_min_msg_len(struct qmi_elem_info *ei_array,
+ int level)
+{
+ int min_msg_len = 0;
+ struct qmi_elem_info *temp_ei = ei_array;
+
+ if (!ei_array)
+ return min_msg_len;
+
+ while (temp_ei->data_type != QMI_EOTI) {
+ /* Optional elements do not count in minimum length */
+ if (temp_ei->data_type == QMI_OPT_FLAG) {
+ temp_ei = skip_to_next_elem(temp_ei, level);
+ continue;
+ }
+
+ if (temp_ei->data_type == QMI_DATA_LEN) {
+ min_msg_len += (temp_ei->elem_size == sizeof(u8) ?
+ sizeof(u8) : sizeof(u16));
+ temp_ei++;
+ continue;
+ } else if (temp_ei->data_type == QMI_STRUCT) {
+ min_msg_len += qmi_calc_min_msg_len(temp_ei->ei_array,
+ (level + 1));
+ temp_ei++;
+ } else if (temp_ei->data_type == QMI_STRING) {
+ if (level > 1)
+ min_msg_len += temp_ei->elem_len <= U8_MAX ?
+ sizeof(u8) : sizeof(u16);
+ min_msg_len += temp_ei->elem_len * temp_ei->elem_size;
+ temp_ei++;
+ } else {
+ min_msg_len += (temp_ei->elem_len * temp_ei->elem_size);
+ temp_ei++;
+ }
+
+ /*
+ * Type & Length info. not prepended for elements in the
+ * nested structure.
+ */
+ if (level == 1)
+ min_msg_len += (TLV_TYPE_SIZE + TLV_LEN_SIZE);
+ }
+
+ return min_msg_len;
+}
+
+/**
+ * qmi_encode_basic_elem() - Encodes elements of basic/primary data type
+ * @buf_dst: Buffer to store the encoded information.
+ * @buf_src: Buffer containing the elements to be encoded.
+ * @elem_len: Number of elements, in the buf_src, to be encoded.
+ * @elem_size: Size of a single instance of the element to be encoded.
+ *
+ * This function encodes the "elem_len" number of data elements, each of
+ * size "elem_size" bytes from the source buffer "buf_src" and stores the
+ * encoded information in the destination buffer "buf_dst". The elements are
+ * of primary data type which include u8 - u64 or similar. This
+ * function returns the number of bytes of encoded information.
+ *
+ * Return: The number of bytes of encoded information.
+ */
+static int qmi_encode_basic_elem(void *buf_dst, const void *buf_src,
+ u32 elem_len, u32 elem_size)
+{
+ u32 i, rc = 0;
+
+ for (i = 0; i < elem_len; i++) {
+ QMI_ENCDEC_ENCODE_N_BYTES(buf_dst, buf_src, elem_size);
+ rc += elem_size;
+ }
+
+ return rc;
+}
+
+/**
+ * qmi_encode_struct_elem() - Encodes elements of struct data type
+ * @ei_array: Struct info array descibing the struct element.
+ * @buf_dst: Buffer to store the encoded information.
+ * @buf_src: Buffer containing the elements to be encoded.
+ * @elem_len: Number of elements, in the buf_src, to be encoded.
+ * @out_buf_len: Available space in the encode buffer.
+ * @enc_level: Depth of the nested structure from the main structure.
+ *
+ * This function encodes the "elem_len" number of struct elements, each of
+ * size "ei_array->elem_size" bytes from the source buffer "buf_src" and
+ * stores the encoded information in the destination buffer "buf_dst". The
+ * elements are of struct data type which includes any C structure. This
+ * function returns the number of bytes of encoded information.
+ *
+ * Return: The number of bytes of encoded information on success or negative
+ * errno on error.
+ */
+static int qmi_encode_struct_elem(struct qmi_elem_info *ei_array,
+ void *buf_dst, const void *buf_src,
+ u32 elem_len, u32 out_buf_len,
+ int enc_level)
+{
+ int i, rc, encoded_bytes = 0;
+ struct qmi_elem_info *temp_ei = ei_array;
+
+ for (i = 0; i < elem_len; i++) {
+ rc = qmi_encode(temp_ei->ei_array, buf_dst, buf_src,
+ out_buf_len - encoded_bytes, enc_level);
+ if (rc < 0) {
+ pr_err("%s: STRUCT Encode failure\n", __func__);
+ return rc;
+ }
+ buf_dst = buf_dst + rc;
+ buf_src = buf_src + temp_ei->elem_size;
+ encoded_bytes += rc;
+ }
+
+ return encoded_bytes;
+}
+
+/**
+ * qmi_encode_string_elem() - Encodes elements of string data type
+ * @ei_array: Struct info array descibing the string element.
+ * @buf_dst: Buffer to store the encoded information.
+ * @buf_src: Buffer containing the elements to be encoded.
+ * @out_buf_len: Available space in the encode buffer.
+ * @enc_level: Depth of the string element from the main structure.
+ *
+ * This function encodes a string element of maximum length "ei_array->elem_len"
+ * bytes from the source buffer "buf_src" and stores the encoded information in
+ * the destination buffer "buf_dst". This function returns the number of bytes
+ * of encoded information.
+ *
+ * Return: The number of bytes of encoded information on success or negative
+ * errno on error.
+ */
+static int qmi_encode_string_elem(struct qmi_elem_info *ei_array,
+ void *buf_dst, const void *buf_src,
+ u32 out_buf_len, int enc_level)
+{
+ int rc;
+ int encoded_bytes = 0;
+ struct qmi_elem_info *temp_ei = ei_array;
+ u32 string_len = 0;
+ u32 string_len_sz = 0;
+
+ string_len = strlen(buf_src);
+ string_len_sz = temp_ei->elem_len <= U8_MAX ?
+ sizeof(u8) : sizeof(u16);
+ if (string_len > temp_ei->elem_len) {
+ pr_err("%s: String to be encoded is longer - %d > %d\n",
+ __func__, string_len, temp_ei->elem_len);
+ return -EINVAL;
+ }
+
+ if (enc_level == 1) {
+ if (string_len + TLV_LEN_SIZE + TLV_TYPE_SIZE >
+ out_buf_len) {
+ pr_err("%s: Output len %d > Out Buf len %d\n",
+ __func__, string_len, out_buf_len);
+ return -ETOOSMALL;
+ }
+ } else {
+ if (string_len + string_len_sz > out_buf_len) {
+ pr_err("%s: Output len %d > Out Buf len %d\n",
+ __func__, string_len, out_buf_len);
+ return -ETOOSMALL;
+ }
+ rc = qmi_encode_basic_elem(buf_dst, &string_len,
+ 1, string_len_sz);
+ encoded_bytes += rc;
+ }
+
+ rc = qmi_encode_basic_elem(buf_dst + encoded_bytes, buf_src,
+ string_len, temp_ei->elem_size);
+ encoded_bytes += rc;
+
+ return encoded_bytes;
+}
+
+/**
+ * qmi_encode() - Core Encode Function
+ * @ei_array: Struct info array describing the structure to be encoded.
+ * @out_buf: Buffer to hold the encoded QMI message.
+ * @in_c_struct: Pointer to the C structure to be encoded.
+ * @out_buf_len: Available space in the encode buffer.
+ * @enc_level: Encode level to indicate the depth of the nested structure,
+ * within the main structure, being encoded.
+ *
+ * Return: The number of bytes of encoded information on success or negative
+ * errno on error.
+ */
+static int qmi_encode(struct qmi_elem_info *ei_array, void *out_buf,
+ const void *in_c_struct, u32 out_buf_len,
+ int enc_level)
+{
+ struct qmi_elem_info *temp_ei = ei_array;
+ u8 opt_flag_value = 0;
+ u32 data_len_value = 0, data_len_sz;
+ u8 *buf_dst = (u8 *)out_buf;
+ u8 *tlv_pointer;
+ u32 tlv_len;
+ u8 tlv_type;
+ u32 encoded_bytes = 0;
+ const void *buf_src;
+ int encode_tlv = 0;
+ int rc;
+
+ if (!ei_array)
+ return 0;
+
+ tlv_pointer = buf_dst;
+ tlv_len = 0;
+ if (enc_level == 1)
+ buf_dst = buf_dst + (TLV_LEN_SIZE + TLV_TYPE_SIZE);
+
+ while (temp_ei->data_type != QMI_EOTI) {
+ buf_src = in_c_struct + temp_ei->offset;
+ tlv_type = temp_ei->tlv_type;
+
+ if (temp_ei->array_type == NO_ARRAY) {
+ data_len_value = 1;
+ } else if (temp_ei->array_type == STATIC_ARRAY) {
+ data_len_value = temp_ei->elem_len;
+ } else if (data_len_value <= 0 ||
+ temp_ei->elem_len < data_len_value) {
+ pr_err("%s: Invalid data length\n", __func__);
+ return -EINVAL;
+ }
+
+ switch (temp_ei->data_type) {
+ case QMI_OPT_FLAG:
+ rc = qmi_encode_basic_elem(&opt_flag_value, buf_src,
+ 1, sizeof(u8));
+ if (opt_flag_value)
+ temp_ei = temp_ei + 1;
+ else
+ temp_ei = skip_to_next_elem(temp_ei, enc_level);
+ break;
+
+ case QMI_DATA_LEN:
+ memcpy(&data_len_value, buf_src, temp_ei->elem_size);
+ data_len_sz = temp_ei->elem_size == sizeof(u8) ?
+ sizeof(u8) : sizeof(u16);
+ /* Check to avoid out of range buffer access */
+ if ((data_len_sz + encoded_bytes + TLV_LEN_SIZE +
+ TLV_TYPE_SIZE) > out_buf_len) {
+ pr_err("%s: Too Small Buffer @DATA_LEN\n",
+ __func__);
+ return -ETOOSMALL;
+ }
+ rc = qmi_encode_basic_elem(buf_dst, &data_len_value,
+ 1, data_len_sz);
+ UPDATE_ENCODE_VARIABLES(temp_ei, buf_dst,
+ encoded_bytes, tlv_len,
+ encode_tlv, rc);
+ if (!data_len_value)
+ temp_ei = skip_to_next_elem(temp_ei, enc_level);
+ else
+ encode_tlv = 0;
+ break;
+
+ case QMI_UNSIGNED_1_BYTE:
+ case QMI_UNSIGNED_2_BYTE:
+ case QMI_UNSIGNED_4_BYTE:
+ case QMI_UNSIGNED_8_BYTE:
+ case QMI_SIGNED_2_BYTE_ENUM:
+ case QMI_SIGNED_4_BYTE_ENUM:
+ /* Check to avoid out of range buffer access */
+ if (((data_len_value * temp_ei->elem_size) +
+ encoded_bytes + TLV_LEN_SIZE + TLV_TYPE_SIZE) >
+ out_buf_len) {
+ pr_err("%s: Too Small Buffer @data_type:%d\n",
+ __func__, temp_ei->data_type);
+ return -ETOOSMALL;
+ }
+ rc = qmi_encode_basic_elem(buf_dst, buf_src,
+ data_len_value,
+ temp_ei->elem_size);
+ UPDATE_ENCODE_VARIABLES(temp_ei, buf_dst,
+ encoded_bytes, tlv_len,
+ encode_tlv, rc);
+ break;
+
+ case QMI_STRUCT:
+ rc = qmi_encode_struct_elem(temp_ei, buf_dst, buf_src,
+ data_len_value,
+ out_buf_len - encoded_bytes,
+ enc_level + 1);
+ if (rc < 0)
+ return rc;
+ UPDATE_ENCODE_VARIABLES(temp_ei, buf_dst,
+ encoded_bytes, tlv_len,
+ encode_tlv, rc);
+ break;
+
+ case QMI_STRING:
+ rc = qmi_encode_string_elem(temp_ei, buf_dst, buf_src,
+ out_buf_len - encoded_bytes,
+ enc_level);
+ if (rc < 0)
+ return rc;
+ UPDATE_ENCODE_VARIABLES(temp_ei, buf_dst,
+ encoded_bytes, tlv_len,
+ encode_tlv, rc);
+ break;
+ default:
+ pr_err("%s: Unrecognized data type\n", __func__);
+ return -EINVAL;
+ }
+
+ if (encode_tlv && enc_level == 1) {
+ QMI_ENCDEC_ENCODE_TLV(tlv_type, tlv_len, tlv_pointer);
+ encoded_bytes += (TLV_TYPE_SIZE + TLV_LEN_SIZE);
+ tlv_pointer = buf_dst;
+ tlv_len = 0;
+ buf_dst = buf_dst + TLV_LEN_SIZE + TLV_TYPE_SIZE;
+ encode_tlv = 0;
+ }
+ }
+
+ return encoded_bytes;
+}
+
+/**
+ * qmi_decode_basic_elem() - Decodes elements of basic/primary data type
+ * @buf_dst: Buffer to store the decoded element.
+ * @buf_src: Buffer containing the elements in QMI wire format.
+ * @elem_len: Number of elements to be decoded.
+ * @elem_size: Size of a single instance of the element to be decoded.
+ *
+ * This function decodes the "elem_len" number of elements in QMI wire format,
+ * each of size "elem_size" bytes from the source buffer "buf_src" and stores
+ * the decoded elements in the destination buffer "buf_dst". The elements are
+ * of primary data type which include u8 - u64 or similar. This
+ * function returns the number of bytes of decoded information.
+ *
+ * Return: The total size of the decoded data elements, in bytes.
+ */
+static int qmi_decode_basic_elem(void *buf_dst, const void *buf_src,
+ u32 elem_len, u32 elem_size)
+{
+ u32 i, rc = 0;
+
+ for (i = 0; i < elem_len; i++) {
+ QMI_ENCDEC_DECODE_N_BYTES(buf_dst, buf_src, elem_size);
+ rc += elem_size;
+ }
+
+ return rc;
+}
+
+/**
+ * qmi_decode_struct_elem() - Decodes elements of struct data type
+ * @ei_array: Struct info array descibing the struct element.
+ * @buf_dst: Buffer to store the decoded element.
+ * @buf_src: Buffer containing the elements in QMI wire format.
+ * @elem_len: Number of elements to be decoded.
+ * @tlv_len: Total size of the encoded inforation corresponding to
+ * this struct element.
+ * @dec_level: Depth of the nested structure from the main structure.
+ *
+ * This function decodes the "elem_len" number of elements in QMI wire format,
+ * each of size "(tlv_len/elem_len)" bytes from the source buffer "buf_src"
+ * and stores the decoded elements in the destination buffer "buf_dst". The
+ * elements are of struct data type which includes any C structure. This
+ * function returns the number of bytes of decoded information.
+ *
+ * Return: The total size of the decoded data elements on success, negative
+ * errno on error.
+ */
+static int qmi_decode_struct_elem(struct qmi_elem_info *ei_array,
+ void *buf_dst, const void *buf_src,
+ u32 elem_len, u32 tlv_len,
+ int dec_level)
+{
+ int i, rc, decoded_bytes = 0;
+ struct qmi_elem_info *temp_ei = ei_array;
+
+ for (i = 0; i < elem_len && decoded_bytes < tlv_len; i++) {
+ rc = qmi_decode(temp_ei->ei_array, buf_dst, buf_src,
+ tlv_len - decoded_bytes, dec_level);
+ if (rc < 0)
+ return rc;
+ buf_src = buf_src + rc;
+ buf_dst = buf_dst + temp_ei->elem_size;
+ decoded_bytes += rc;
+ }
+
+ if ((dec_level <= 2 && decoded_bytes != tlv_len) ||
+ (dec_level > 2 && (i < elem_len || decoded_bytes > tlv_len))) {
+ pr_err("%s: Fault in decoding: dl(%d), db(%d), tl(%d), i(%d), el(%d)\n",
+ __func__, dec_level, decoded_bytes, tlv_len,
+ i, elem_len);
+ return -EFAULT;
+ }
+
+ return decoded_bytes;
+}
+
+/**
+ * qmi_decode_string_elem() - Decodes elements of string data type
+ * @ei_array: Struct info array descibing the string element.
+ * @buf_dst: Buffer to store the decoded element.
+ * @buf_src: Buffer containing the elements in QMI wire format.
+ * @tlv_len: Total size of the encoded inforation corresponding to
+ * this string element.
+ * @dec_level: Depth of the string element from the main structure.
+ *
+ * This function decodes the string element of maximum length
+ * "ei_array->elem_len" from the source buffer "buf_src" and puts it into
+ * the destination buffer "buf_dst". This function returns number of bytes
+ * decoded from the input buffer.
+ *
+ * Return: The total size of the decoded data elements on success, negative
+ * errno on error.
+ */
+static int qmi_decode_string_elem(struct qmi_elem_info *ei_array,
+ void *buf_dst, const void *buf_src,
+ u32 tlv_len, int dec_level)
+{
+ int rc;
+ int decoded_bytes = 0;
+ u32 string_len = 0;
+ u32 string_len_sz = 0;
+ struct qmi_elem_info *temp_ei = ei_array;
+
+ if (dec_level == 1) {
+ string_len = tlv_len;
+ } else {
+ string_len_sz = temp_ei->elem_len <= U8_MAX ?
+ sizeof(u8) : sizeof(u16);
+ rc = qmi_decode_basic_elem(&string_len, buf_src,
+ 1, string_len_sz);
+ decoded_bytes += rc;
+ }
+
+ if (string_len > temp_ei->elem_len) {
+ pr_err("%s: String len %d > Max Len %d\n",
+ __func__, string_len, temp_ei->elem_len);
+ return -ETOOSMALL;
+ } else if (string_len > tlv_len) {
+ pr_err("%s: String len %d > Input Buffer Len %d\n",
+ __func__, string_len, tlv_len);
+ return -EFAULT;
+ }
+
+ rc = qmi_decode_basic_elem(buf_dst, buf_src + decoded_bytes,
+ string_len, temp_ei->elem_size);
+ *((char *)buf_dst + string_len) = '\0';
+ decoded_bytes += rc;
+
+ return decoded_bytes;
+}
+
+/**
+ * find_ei() - Find element info corresponding to TLV Type
+ * @ei_array: Struct info array of the message being decoded.
+ * @type: TLV Type of the element being searched.
+ *
+ * Every element that got encoded in the QMI message will have a type
+ * information associated with it. While decoding the QMI message,
+ * this function is used to find the struct info regarding the element
+ * that corresponds to the type being decoded.
+ *
+ * Return: Pointer to struct info, if found
+ */
+static struct qmi_elem_info *find_ei(struct qmi_elem_info *ei_array,
+ u32 type)
+{
+ struct qmi_elem_info *temp_ei = ei_array;
+
+ while (temp_ei->data_type != QMI_EOTI) {
+ if (temp_ei->tlv_type == (u8)type)
+ return temp_ei;
+ temp_ei = temp_ei + 1;
+ }
+
+ return NULL;
+}
+
+/**
+ * qmi_decode() - Core Decode Function
+ * @ei_array: Struct info array describing the structure to be decoded.
+ * @out_c_struct: Buffer to hold the decoded C struct
+ * @in_buf: Buffer containing the QMI message to be decoded
+ * @in_buf_len: Length of the QMI message to be decoded
+ * @dec_level: Decode level to indicate the depth of the nested structure,
+ * within the main structure, being decoded
+ *
+ * Return: The number of bytes of decoded information on success, negative
+ * errno on error.
+ */
+static int qmi_decode(struct qmi_elem_info *ei_array, void *out_c_struct,
+ const void *in_buf, u32 in_buf_len,
+ int dec_level)
+{
+ struct qmi_elem_info *temp_ei = ei_array;
+ u8 opt_flag_value = 1;
+ u32 data_len_value = 0, data_len_sz = 0;
+ u8 *buf_dst = out_c_struct;
+ const u8 *tlv_pointer;
+ u32 tlv_len = 0;
+ u32 tlv_type;
+ u32 decoded_bytes = 0;
+ const void *buf_src = in_buf;
+ int rc;
+
+ while (decoded_bytes < in_buf_len) {
+ if (dec_level >= 2 && temp_ei->data_type == QMI_EOTI)
+ return decoded_bytes;
+
+ if (dec_level == 1) {
+ tlv_pointer = buf_src;
+ QMI_ENCDEC_DECODE_TLV(&tlv_type,
+ &tlv_len, tlv_pointer);
+ buf_src += (TLV_TYPE_SIZE + TLV_LEN_SIZE);
+ decoded_bytes += (TLV_TYPE_SIZE + TLV_LEN_SIZE);
+ temp_ei = find_ei(ei_array, tlv_type);
+ if (!temp_ei && tlv_type < OPTIONAL_TLV_TYPE_START) {
+ pr_err("%s: Inval element info\n", __func__);
+ return -EINVAL;
+ } else if (!temp_ei) {
+ UPDATE_DECODE_VARIABLES(buf_src,
+ decoded_bytes, tlv_len);
+ continue;
+ }
+ } else {
+ /*
+ * No length information for elements in nested
+ * structures. So use remaining decodable buffer space.
+ */
+ tlv_len = in_buf_len - decoded_bytes;
+ }
+
+ buf_dst = out_c_struct + temp_ei->offset;
+ if (temp_ei->data_type == QMI_OPT_FLAG) {
+ memcpy(buf_dst, &opt_flag_value, sizeof(u8));
+ temp_ei = temp_ei + 1;
+ buf_dst = out_c_struct + temp_ei->offset;
+ }
+
+ if (temp_ei->data_type == QMI_DATA_LEN) {
+ data_len_sz = temp_ei->elem_size == sizeof(u8) ?
+ sizeof(u8) : sizeof(u16);
+ rc = qmi_decode_basic_elem(&data_len_value, buf_src,
+ 1, data_len_sz);
+ memcpy(buf_dst, &data_len_value, sizeof(u32));
+ temp_ei = temp_ei + 1;
+ buf_dst = out_c_struct + temp_ei->offset;
+ tlv_len -= data_len_sz;
+ UPDATE_DECODE_VARIABLES(buf_src, decoded_bytes, rc);
+ }
+
+ if (temp_ei->array_type == NO_ARRAY) {
+ data_len_value = 1;
+ } else if (temp_ei->array_type == STATIC_ARRAY) {
+ data_len_value = temp_ei->elem_len;
+ } else if (data_len_value > temp_ei->elem_len) {
+ pr_err("%s: Data len %d > max spec %d\n",
+ __func__, data_len_value, temp_ei->elem_len);
+ return -ETOOSMALL;
+ }
+
+ switch (temp_ei->data_type) {
+ case QMI_UNSIGNED_1_BYTE:
+ case QMI_UNSIGNED_2_BYTE:
+ case QMI_UNSIGNED_4_BYTE:
+ case QMI_UNSIGNED_8_BYTE:
+ case QMI_SIGNED_2_BYTE_ENUM:
+ case QMI_SIGNED_4_BYTE_ENUM:
+ rc = qmi_decode_basic_elem(buf_dst, buf_src,
+ data_len_value,
+ temp_ei->elem_size);
+ UPDATE_DECODE_VARIABLES(buf_src, decoded_bytes, rc);
+ break;
+
+ case QMI_STRUCT:
+ rc = qmi_decode_struct_elem(temp_ei, buf_dst, buf_src,
+ data_len_value, tlv_len,
+ dec_level + 1);
+ if (rc < 0)
+ return rc;
+ UPDATE_DECODE_VARIABLES(buf_src, decoded_bytes, rc);
+ break;
+
+ case QMI_STRING:
+ rc = qmi_decode_string_elem(temp_ei, buf_dst, buf_src,
+ tlv_len, dec_level);
+ if (rc < 0)
+ return rc;
+ UPDATE_DECODE_VARIABLES(buf_src, decoded_bytes, rc);
+ break;
+
+ default:
+ pr_err("%s: Unrecognized data type\n", __func__);
+ return -EINVAL;
+ }
+ temp_ei = temp_ei + 1;
+ }
+
+ return decoded_bytes;
+}
+
+/**
+ * qmi_encode_message() - Encode C structure as QMI encoded message
+ * @type: Type of QMI message
+ * @msg_id: Message ID of the message
+ * @len: Passed as max length of the message, updated to actual size
+ * @txn_id: Transaction ID
+ * @ei: QMI message descriptor
+ * @c_struct: Reference to structure to encode
+ *
+ * Return: Buffer with encoded message, or negative ERR_PTR() on error
+ */
+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)
+{
+ struct qmi_header *hdr;
+ ssize_t msglen = 0;
+ void *msg;
+ int ret;
+
+ /* Check the possibility of a zero length QMI message */
+ if (!c_struct) {
+ ret = qmi_calc_min_msg_len(ei, 1);
+ if (ret) {
+ pr_err("%s: Calc. len %d != 0, but NULL c_struct\n",
+ __func__, ret);
+ return ERR_PTR(-EINVAL);
+ }
+ }
+
+ msg = kzalloc(sizeof(*hdr) + *len, GFP_KERNEL);
+ if (!msg)
+ return ERR_PTR(-ENOMEM);
+
+ /* Encode message, if we have a message */
+ if (c_struct) {
+ msglen = qmi_encode(ei, msg + sizeof(*hdr), c_struct, *len, 1);
+ if (msglen < 0) {
+ kfree(msg);
+ return ERR_PTR(msglen);
+ }
+ }
+
+ hdr = msg;
+ hdr->type = type;
+ hdr->txn_id = txn_id;
+ hdr->msg_id = msg_id;
+ hdr->msg_len = msglen;
+
+ *len = sizeof(*hdr) + msglen;
+
+ return msg;
+}
+EXPORT_SYMBOL(qmi_encode_message);
+
+/**
+ * qmi_decode_message() - Decode QMI encoded message to C structure
+ * @buf: Buffer with encoded message
+ * @len: Amount of data in @buf
+ * @ei: QMI message descriptor
+ * @c_struct: Reference to structure to decode into
+ *
+ * Return: The number of bytes of decoded information on success, negative
+ * errno on error.
+ */
+int qmi_decode_message(const void *buf, size_t len,
+ struct qmi_elem_info *ei, void *c_struct)
+{
+ if (!ei)
+ return -EINVAL;
+
+ if (!c_struct || !buf || !len)
+ return -EINVAL;
+
+ return qmi_decode(ei, c_struct, buf + sizeof(struct qmi_header),
+ len - sizeof(struct qmi_header), 1);
+}
+EXPORT_SYMBOL(qmi_decode_message);
+
+/* Common header in all QMI responses */
+struct qmi_elem_info qmi_response_type_v01_ei[] = {
+ {
+ .data_type = QMI_SIGNED_2_BYTE_ENUM,
+ .elem_len = 1,
+ .elem_size = sizeof(u16),
+ .array_type = NO_ARRAY,
+ .tlv_type = QMI_COMMON_TLV_TYPE,
+ .offset = offsetof(struct qmi_response_type_v01, result),
+ .ei_array = NULL,
+ },
+ {
+ .data_type = QMI_SIGNED_2_BYTE_ENUM,
+ .elem_len = 1,
+ .elem_size = sizeof(u16),
+ .array_type = NO_ARRAY,
+ .tlv_type = QMI_COMMON_TLV_TYPE,
+ .offset = offsetof(struct qmi_response_type_v01, error),
+ .ei_array = NULL,
+ },
+ {
+ .data_type = QMI_EOTI,
+ .elem_len = 0,
+ .elem_size = 0,
+ .array_type = NO_ARRAY,
+ .tlv_type = QMI_COMMON_TLV_TYPE,
+ .offset = 0,
+ .ei_array = NULL,
+ },
+};
+EXPORT_SYMBOL(qmi_response_type_v01_ei);
+
+MODULE_DESCRIPTION("QMI encoder/decoder helper");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/soc/qcom/qmi.h b/include/linux/soc/qcom/qmi.h
new file mode 100644
index 000000000000..1b66e9a6074f
--- /dev/null
+++ b/include/linux/soc/qcom/qmi.h
@@ -0,0 +1,114 @@
+/*
+ * Copyright (c) 2012-2014, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2017, Linaro Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * 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.
+ */
+#ifndef __QMI_HELPERS_H__
+#define __QMI_HELPERS_H__
+
+#include <linux/types.h>
+
+/**
+ * qmi_header - wireformat header of QMI messages
+ * @type: type of message
+ * @txn_id: transaction id
+ * @msg_id: message id
+ * @msg_len: length of message payload following header
+ */
+struct qmi_header {
+ u8 type;
+ u16 txn_id;
+ u16 msg_id;
+ u16 msg_len;
+} __packed;
+
+#define QMI_REQUEST 0
+#define QMI_RESPONSE 2
+#define QMI_INDICATION 4
+
+#define QMI_COMMON_TLV_TYPE 0
+
+enum qmi_elem_type {
+ QMI_EOTI,
+ QMI_OPT_FLAG,
+ QMI_DATA_LEN,
+ QMI_UNSIGNED_1_BYTE,
+ QMI_UNSIGNED_2_BYTE,
+ QMI_UNSIGNED_4_BYTE,
+ QMI_UNSIGNED_8_BYTE,
+ QMI_SIGNED_2_BYTE_ENUM,
+ QMI_SIGNED_4_BYTE_ENUM,
+ QMI_STRUCT,
+ QMI_STRING,
+};
+
+enum qmi_array_type {
+ NO_ARRAY,
+ STATIC_ARRAY,
+ VAR_LEN_ARRAY,
+};
+
+/**
+ * struct qmi_elem_info - describes how to encode a single QMI element
+ * @data_type: Data type of this element.
+ * @elem_len: Array length of this element, if an array.
+ * @elem_size: Size of a single instance of this data type.
+ * @array_type: Array type of this element.
+ * @tlv_type: QMI message specific type to identify which element
+ * is present in an incoming message.
+ * @offset: Specifies the offset of the first instance of this
+ * element in the data structure.
+ * @ei_array: Null-terminated array of @qmi_elem_info to describe nested
+ * structures.
+ */
+struct qmi_elem_info {
+ enum qmi_elem_type data_type;
+ u32 elem_len;
+ u32 elem_size;
+ enum qmi_array_type array_type;
+ u8 tlv_type;
+ u32 offset;
+ struct qmi_elem_info *ei_array;
+};
+
+#define QMI_RESULT_SUCCESS_V01 0
+#define QMI_RESULT_FAILURE_V01 1
+
+#define QMI_ERR_NONE_V01 0
+#define QMI_ERR_MALFORMED_MSG_V01 1
+#define QMI_ERR_NO_MEMORY_V01 2
+#define QMI_ERR_INTERNAL_V01 3
+#define QMI_ERR_CLIENT_IDS_EXHAUSTED_V01 5
+#define QMI_ERR_INVALID_ID_V01 41
+#define QMI_ERR_ENCODING_V01 58
+#define QMI_ERR_INCOMPATIBLE_STATE_V01 90
+#define QMI_ERR_NOT_SUPPORTED_V01 94
+
+/**
+ * qmi_response_type_v01 - common response header (decoded)
+ * @result: result of the transaction
+ * @error: error value, when @result is QMI_RESULT_FAILURE_V01
+ */
+struct qmi_response_type_v01 {
+ u16 result;
+ u16 error;
+};
+
+extern struct qmi_elem_info qmi_response_type_v01_ei[];
+
+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);
+
+int qmi_decode_message(const void *buf, size_t len,
+ struct qmi_elem_info *ei, void *c_struct);
+
+#endif
--
2.15.0
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH v4 1/5] soc: qcom: Introduce QMI encoder/decoder
2017-11-30 1:16 ` [PATCH v4 1/5] soc: qcom: Introduce QMI encoder/decoder Bjorn Andersson
@ 2017-11-30 17:17 ` Chris Lew
2017-12-01 9:10 ` Jitendra Sharma
1 sibling, 0 replies; 24+ messages in thread
From: Chris Lew @ 2017-11-30 17:17 UTC (permalink / raw)
To: Bjorn Andersson, Andy Gross, Ohad Ben-Cohen
Cc: Arun Kumar Neelakantam, linux-kernel, linux-arm-msm, linux-soc,
linux-remoteproc
On 11/29/2017 5:16 PM, Bjorn Andersson wrote:
> Add the helper library for encoding and decoding QMI encoded messages.
> The implementation is taken from lib/qmi_encdec.c of the Qualcomm kernel
> (msm-3.18).
>
> Modifications has been made to the public API, source buffers has been
> made const and the debug-logging part was omitted, for now.
>
> Tested-By: Chris Lew <clew@codeaurora.org>
> Tested-By: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
Acked-By: Chris Lew <clew@codeaurora.org>
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v4 1/5] soc: qcom: Introduce QMI encoder/decoder
2017-11-30 1:16 ` [PATCH v4 1/5] soc: qcom: Introduce QMI encoder/decoder Bjorn Andersson
2017-11-30 17:17 ` Chris Lew
@ 2017-12-01 9:10 ` Jitendra Sharma
2017-12-05 17:33 ` Bjorn Andersson
1 sibling, 1 reply; 24+ messages in thread
From: Jitendra Sharma @ 2017-12-01 9:10 UTC (permalink / raw)
To: Bjorn Andersson, Andy Gross, Ohad Ben-Cohen
Cc: Arun Kumar Neelakantam, Chris Lew, linux-kernel, linux-arm-msm,
linux-soc, linux-remoteproc
Hi Bjorn,
Few minor comments ..
On 11/30/2017 6:46 AM, Bjorn Andersson wrote:
> Add the helper library for encoding and decoding QMI encoded messages.
> The implementation is taken from lib/qmi_encdec.c of the Qualcomm kernel
> (msm-3.18).
>
> Modifications has been made to the public API, source buffers has been
> made const and the debug-logging part was omitted, for now.
>
> Tested-By: Chris Lew <clew@codeaurora.org>
> Tested-By: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
>
> Changes since v3:
> - Moved depends on ARCH_QCOM from patch 2
> - Kerneldoc updates
> - Style updates
> - Dropped qrtr.h include from header file
> - Rename is_array to array_type
>
> Changes since v2:
> - Checkpatch fixes
>
> Changes since v1:
> - None
>
> drivers/soc/qcom/Kconfig | 9 +
> drivers/soc/qcom/Makefile | 2 +
> drivers/soc/qcom/qmi_encdec.c | 826 ++++++++++++++++++++++++++++++++++++++++++
> include/linux/soc/qcom/qmi.h | 114 ++++++
> 4 files changed, 951 insertions(+)
> create mode 100644 drivers/soc/qcom/qmi_encdec.c
> create mode 100644 include/linux/soc/qcom/qmi.h
>
> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> index b81374bb6713..2411df0427d9 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -35,6 +35,15 @@ config QCOM_PM
> modes. It interface with various system drivers to put the cores in
> low power modes.
>
> +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
> + clients and this helpers provide the common functionality needed for
> + doing this from a kernel driver.
> +
> config QCOM_RMTFS_MEM
> tristate "Qualcomm Remote Filesystem memory driver"
> depends on ARCH_QCOM
> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> index 40c56f67e94a..37f85b45d0a1 100644
> --- a/drivers/soc/qcom/Makefile
> +++ b/drivers/soc/qcom/Makefile
> @@ -3,6 +3,8 @@ obj-$(CONFIG_QCOM_GLINK_SSR) += glink_ssr.o
> obj-$(CONFIG_QCOM_GSBI) += qcom_gsbi.o
> 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
> obj-$(CONFIG_QCOM_RMTFS_MEM) += rmtfs_mem.o
> obj-$(CONFIG_QCOM_SMD_RPM) += smd-rpm.o
> obj-$(CONFIG_QCOM_SMEM) += smem.o
> diff --git a/drivers/soc/qcom/qmi_encdec.c b/drivers/soc/qcom/qmi_encdec.c
> new file mode 100644
> index 000000000000..a197fc0114c3
> --- /dev/null
> +++ b/drivers/soc/qcom/qmi_encdec.c
> @@ -0,0 +1,826 @@
> +/*
> + * Copyright (c) 2012-2015, The Linux Foundation. All rights reserved.
> + * Copyright (C) 2017 Linaro Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * 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/slab.h>
> +#include <linux/uaccess.h>
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/errno.h>
> +#include <linux/string.h>
> +#include <linux/soc/qcom/qmi.h>
> +
> +#define QMI_ENCDEC_ENCODE_TLV(type, length, p_dst) do { \
> + *p_dst++ = type; \
> + *p_dst++ = ((u8)((length) & 0xFF)); \
> + *p_dst++ = ((u8)(((length) >> 8) & 0xFF)); \
> +} while (0)
> +
> +#define QMI_ENCDEC_DECODE_TLV(p_type, p_length, p_src) do { \
> + *p_type = (u8)*p_src++; \
> + *p_length = (u8)*p_src++; \
> + *p_length |= ((u8)*p_src) << 8; \
> +} while (0)
> +
> +#define QMI_ENCDEC_ENCODE_N_BYTES(p_dst, p_src, size) \
> +do { \
> + memcpy(p_dst, p_src, size); \
> + p_dst = (u8 *)p_dst + size; \
> + p_src = (u8 *)p_src + size; \
> +} while (0)
> +
> +#define QMI_ENCDEC_DECODE_N_BYTES(p_dst, p_src, size) \
> +do { \
> + memcpy(p_dst, p_src, size); \
> + p_dst = (u8 *)p_dst + size; \
> + p_src = (u8 *)p_src + size; \
> +} while (0)
> +
> +#define UPDATE_ENCODE_VARIABLES(temp_si, buf_dst, \
> + encoded_bytes, tlv_len, encode_tlv, rc) \
> +do { \
> + buf_dst = (u8 *)buf_dst + rc; \
> + encoded_bytes += rc; \
> + tlv_len += rc; \
> + temp_si = temp_si + 1; \
> + encode_tlv = 1; \
> +} while (0)
> +
> +#define UPDATE_DECODE_VARIABLES(buf_src, decoded_bytes, rc) \
> +do { \
> + buf_src = (u8 *)buf_src + rc; \
> + decoded_bytes += rc; \
> +} while (0)
> +
> +#define TLV_LEN_SIZE sizeof(u16)
> +#define TLV_TYPE_SIZE sizeof(u8)
> +#define OPTIONAL_TLV_TYPE_START 0x10
> +
> +static int qmi_encode(struct qmi_elem_info *ei_array, void *out_buf,
> + const void *in_c_struct, u32 out_buf_len,
> + int enc_level);
> +
> +static int qmi_decode(struct qmi_elem_info *ei_array, void *out_c_struct,
> + const void *in_buf, u32 in_buf_len, int dec_level);
> +
> +/**
> + * skip_to_next_elem() - Skip to next element in the structure to be encoded
> + * @ei_array: Struct info describing the element to be skipped.
> + * @level: Depth level of encoding/decoding to identify nested structures.
> + *
> + * This function is used while encoding optional elements. If the flag
> + * corresponding to an optional element is not set, then encoding the
> + * optional element can be skipped. This function can be used to perform
> + * that operation.
> + *
> + * Return: struct info of the next element that can be encoded.
> + */
> +static struct qmi_elem_info *skip_to_next_elem(struct qmi_elem_info *ei_array,
> + int level)
> +{
> + struct qmi_elem_info *temp_ei = ei_array;
> + u8 tlv_type;
> +
> + if (level > 1) {
> + temp_ei = temp_ei + 1;
> + } else {
> + do {
> + tlv_type = temp_ei->tlv_type;
> + temp_ei = temp_ei + 1;
> + } while (tlv_type == temp_ei->tlv_type);
> + }
> +
> + return temp_ei;
> +}
> +
> +/**
> + * qmi_calc_min_msg_len() - Calculate the minimum length of a QMI message
> + * @ei_array: Struct info array describing the structure.
> + * @level: Level to identify the depth of the nested structures.
> + *
> + * Return: Expected minimum length of the QMI message or 0 on error.
> + */
> +static int qmi_calc_min_msg_len(struct qmi_elem_info *ei_array,
> + int level)
> +{
> + int min_msg_len = 0;
min_msg_len should be u32 as it would not be negative
> + struct qmi_elem_info *temp_ei = ei_array;
> +
> + if (!ei_array)
> + return min_msg_len;
> +
> + while (temp_ei->data_type != QMI_EOTI) {
> + /* Optional elements do not count in minimum length */
> + if (temp_ei->data_type == QMI_OPT_FLAG) {
> + temp_ei = skip_to_next_elem(temp_ei, level);
> + continue;
> + }
> +
> + if (temp_ei->data_type == QMI_DATA_LEN) {
> + min_msg_len += (temp_ei->elem_size == sizeof(u8) ?
> + sizeof(u8) : sizeof(u16));
> + temp_ei++;
> + continue;
> + } else if (temp_ei->data_type == QMI_STRUCT) {
> + min_msg_len += qmi_calc_min_msg_len(temp_ei->ei_array,
> + (level + 1));
> + temp_ei++;
> + } else if (temp_ei->data_type == QMI_STRING) {
> + if (level > 1)
> + min_msg_len += temp_ei->elem_len <= U8_MAX ?
> + sizeof(u8) : sizeof(u16);
> + min_msg_len += temp_ei->elem_len * temp_ei->elem_size;
> + temp_ei++;
> + } else {
> + min_msg_len += (temp_ei->elem_len * temp_ei->elem_size);
> + temp_ei++;
> + }
> +
> + /*
> + * Type & Length info. not prepended for elements in the
> + * nested structure.
> + */
> + if (level == 1)
> + min_msg_len += (TLV_TYPE_SIZE + TLV_LEN_SIZE);
> + }
> +
> + return min_msg_len;
> +}
> +
> +/**
> + * qmi_encode_basic_elem() - Encodes elements of basic/primary data type
> + * @buf_dst: Buffer to store the encoded information.
> + * @buf_src: Buffer containing the elements to be encoded.
> + * @elem_len: Number of elements, in the buf_src, to be encoded.
> + * @elem_size: Size of a single instance of the element to be encoded.
> + *
> + * This function encodes the "elem_len" number of data elements, each of
> + * size "elem_size" bytes from the source buffer "buf_src" and stores the
> + * encoded information in the destination buffer "buf_dst". The elements are
> + * of primary data type which include u8 - u64 or similar. This
> + * function returns the number of bytes of encoded information.
> + *
> + * Return: The number of bytes of encoded information.
> + */
> +static int qmi_encode_basic_elem(void *buf_dst, const void *buf_src,
> + u32 elem_len, u32 elem_size)
> +{
> + u32 i, rc = 0;
Function return type should not be int
> +
> + for (i = 0; i < elem_len; i++) {
> + QMI_ENCDEC_ENCODE_N_BYTES(buf_dst, buf_src, elem_size);
> + rc += elem_size;
> + }
> +
> + return rc;
> +}
> +
> +/**
> + * qmi_encode_struct_elem() - Encodes elements of struct data type
> + * @ei_array: Struct info array descibing the struct element.
> + * @buf_dst: Buffer to store the encoded information.
> + * @buf_src: Buffer containing the elements to be encoded.
> + * @elem_len: Number of elements, in the buf_src, to be encoded.
> + * @out_buf_len: Available space in the encode buffer.
> + * @enc_level: Depth of the nested structure from the main structure.
> + *
> + * This function encodes the "elem_len" number of struct elements, each of
> + * size "ei_array->elem_size" bytes from the source buffer "buf_src" and
> + * stores the encoded information in the destination buffer "buf_dst". The
> + * elements are of struct data type which includes any C structure. This
> + * function returns the number of bytes of encoded information.
> + *
> + * Return: The number of bytes of encoded information on success or negative
> + * errno on error.
> + */
> +static int qmi_encode_struct_elem(struct qmi_elem_info *ei_array,
> + void *buf_dst, const void *buf_src,
> + u32 elem_len, u32 out_buf_len,
> + int enc_level)
> +{
> + int i, rc, encoded_bytes = 0;
> + struct qmi_elem_info *temp_ei = ei_array;
> +
> + for (i = 0; i < elem_len; i++) {
> + rc = qmi_encode(temp_ei->ei_array, buf_dst, buf_src,
> + out_buf_len - encoded_bytes, enc_level);
> + if (rc < 0) {
> + pr_err("%s: STRUCT Encode failure\n", __func__);
> + return rc;
> + }
> + buf_dst = buf_dst + rc;
> + buf_src = buf_src + temp_ei->elem_size;
> + encoded_bytes += rc;
> + }
> +
> + return encoded_bytes;
> +}
> +
> +/**
> + * qmi_encode_string_elem() - Encodes elements of string data type
> + * @ei_array: Struct info array descibing the string element.
> + * @buf_dst: Buffer to store the encoded information.
> + * @buf_src: Buffer containing the elements to be encoded.
> + * @out_buf_len: Available space in the encode buffer.
> + * @enc_level: Depth of the string element from the main structure.
> + *
> + * This function encodes a string element of maximum length "ei_array->elem_len"
> + * bytes from the source buffer "buf_src" and stores the encoded information in
> + * the destination buffer "buf_dst". This function returns the number of bytes
> + * of encoded information.
> + *
> + * Return: The number of bytes of encoded information on success or negative
> + * errno on error.
> + */
> +static int qmi_encode_string_elem(struct qmi_elem_info *ei_array,
> + void *buf_dst, const void *buf_src,
> + u32 out_buf_len, int enc_level)
> +{
> + int rc;
> + int encoded_bytes = 0;
change rc,encoded_bytes to u32
> + struct qmi_elem_info *temp_ei = ei_array;
> + u32 string_len = 0;
> + u32 string_len_sz = 0;
> +
> + string_len = strlen(buf_src);
> + string_len_sz = temp_ei->elem_len <= U8_MAX ?
> + sizeof(u8) : sizeof(u16);
> + if (string_len > temp_ei->elem_len) {
> + pr_err("%s: String to be encoded is longer - %d > %d\n",
> + __func__, string_len, temp_ei->elem_len);
> + return -EINVAL;
> + }
> +
> + if (enc_level == 1) {
> + if (string_len + TLV_LEN_SIZE + TLV_TYPE_SIZE >
> + out_buf_len) {
> + pr_err("%s: Output len %d > Out Buf len %d\n",
> + __func__, string_len, out_buf_len);
> + return -ETOOSMALL;
> + }
> + } else {
> + if (string_len + string_len_sz > out_buf_len) {
> + pr_err("%s: Output len %d > Out Buf len %d\n",
> + __func__, string_len, out_buf_len);
> + return -ETOOSMALL;
> + }
> + rc = qmi_encode_basic_elem(buf_dst, &string_len,
> + 1, string_len_sz);
> + encoded_bytes += rc;
> + }
> +
> + rc = qmi_encode_basic_elem(buf_dst + encoded_bytes, buf_src,
> + string_len, temp_ei->elem_size);
> + encoded_bytes += rc;
> +
> + return encoded_bytes;
> +}
> +
> +/**
> + * qmi_encode() - Core Encode Function
> + * @ei_array: Struct info array describing the structure to be encoded.
> + * @out_buf: Buffer to hold the encoded QMI message.
> + * @in_c_struct: Pointer to the C structure to be encoded.
> + * @out_buf_len: Available space in the encode buffer.
> + * @enc_level: Encode level to indicate the depth of the nested structure,
> + * within the main structure, being encoded.
> + *
> + * Return: The number of bytes of encoded information on success or negative
> + * errno on error.
> + */
> +static int qmi_encode(struct qmi_elem_info *ei_array, void *out_buf,
> + const void *in_c_struct, u32 out_buf_len,
> + int enc_level)
> +{
> + struct qmi_elem_info *temp_ei = ei_array;
> + u8 opt_flag_value = 0;
> + u32 data_len_value = 0, data_len_sz;
> + u8 *buf_dst = (u8 *)out_buf;
> + u8 *tlv_pointer;
> + u32 tlv_len;
> + u8 tlv_type;
> + u32 encoded_bytes = 0;
> + const void *buf_src;
> + int encode_tlv = 0;
> + int rc;
> +
> + if (!ei_array)
> + return 0;
> +
> + tlv_pointer = buf_dst;
> + tlv_len = 0;
> + if (enc_level == 1)
> + buf_dst = buf_dst + (TLV_LEN_SIZE + TLV_TYPE_SIZE);
> +
> + while (temp_ei->data_type != QMI_EOTI) {
> + buf_src = in_c_struct + temp_ei->offset;
> + tlv_type = temp_ei->tlv_type;
> +
> + if (temp_ei->array_type == NO_ARRAY) {
> + data_len_value = 1;
> + } else if (temp_ei->array_type == STATIC_ARRAY) {
> + data_len_value = temp_ei->elem_len;
> + } else if (data_len_value <= 0 ||
> + temp_ei->elem_len < data_len_value) {
> + pr_err("%s: Invalid data length\n", __func__);
> + return -EINVAL;
> + }
> +
> + switch (temp_ei->data_type) {
> + case QMI_OPT_FLAG:
> + rc = qmi_encode_basic_elem(&opt_flag_value, buf_src,
> + 1, sizeof(u8));
> + if (opt_flag_value)
> + temp_ei = temp_ei + 1;
> + else
> + temp_ei = skip_to_next_elem(temp_ei, enc_level);
> + break;
> +
> + case QMI_DATA_LEN:
> + memcpy(&data_len_value, buf_src, temp_ei->elem_size);
> + data_len_sz = temp_ei->elem_size == sizeof(u8) ?
> + sizeof(u8) : sizeof(u16);
> + /* Check to avoid out of range buffer access */
> + if ((data_len_sz + encoded_bytes + TLV_LEN_SIZE +
> + TLV_TYPE_SIZE) > out_buf_len) {
> + pr_err("%s: Too Small Buffer @DATA_LEN\n",
> + __func__);
> + return -ETOOSMALL;
> + }
> + rc = qmi_encode_basic_elem(buf_dst, &data_len_value,
> + 1, data_len_sz);
> + UPDATE_ENCODE_VARIABLES(temp_ei, buf_dst,
> + encoded_bytes, tlv_len,
> + encode_tlv, rc);
> + if (!data_len_value)
> + temp_ei = skip_to_next_elem(temp_ei, enc_level);
> + else
> + encode_tlv = 0;
> + break;
> +
> + case QMI_UNSIGNED_1_BYTE:
> + case QMI_UNSIGNED_2_BYTE:
> + case QMI_UNSIGNED_4_BYTE:
> + case QMI_UNSIGNED_8_BYTE:
> + case QMI_SIGNED_2_BYTE_ENUM:
> + case QMI_SIGNED_4_BYTE_ENUM:
> + /* Check to avoid out of range buffer access */
> + if (((data_len_value * temp_ei->elem_size) +
> + encoded_bytes + TLV_LEN_SIZE + TLV_TYPE_SIZE) >
> + out_buf_len) {
> + pr_err("%s: Too Small Buffer @data_type:%d\n",
> + __func__, temp_ei->data_type);
> + return -ETOOSMALL;
> + }
> + rc = qmi_encode_basic_elem(buf_dst, buf_src,
> + data_len_value,
> + temp_ei->elem_size);
> + UPDATE_ENCODE_VARIABLES(temp_ei, buf_dst,
> + encoded_bytes, tlv_len,
> + encode_tlv, rc);
> + break;
> +
> + case QMI_STRUCT:
> + rc = qmi_encode_struct_elem(temp_ei, buf_dst, buf_src,
> + data_len_value,
> + out_buf_len - encoded_bytes,
> + enc_level + 1);
> + if (rc < 0)
> + return rc;
> + UPDATE_ENCODE_VARIABLES(temp_ei, buf_dst,
> + encoded_bytes, tlv_len,
> + encode_tlv, rc);
> + break;
> +
> + case QMI_STRING:
> + rc = qmi_encode_string_elem(temp_ei, buf_dst, buf_src,
> + out_buf_len - encoded_bytes,
> + enc_level);
> + if (rc < 0)
> + return rc;
> + UPDATE_ENCODE_VARIABLES(temp_ei, buf_dst,
> + encoded_bytes, tlv_len,
> + encode_tlv, rc);
> + break;
> + default:
> + pr_err("%s: Unrecognized data type\n", __func__);
> + return -EINVAL;
> + }
> +
> + if (encode_tlv && enc_level == 1) {
> + QMI_ENCDEC_ENCODE_TLV(tlv_type, tlv_len, tlv_pointer);
> + encoded_bytes += (TLV_TYPE_SIZE + TLV_LEN_SIZE);
> + tlv_pointer = buf_dst;
> + tlv_len = 0;
> + buf_dst = buf_dst + TLV_LEN_SIZE + TLV_TYPE_SIZE;
> + encode_tlv = 0;
> + }
> + }
> +
> + return encoded_bytes;
> +}
> +
> +/**
> + * qmi_decode_basic_elem() - Decodes elements of basic/primary data type
> + * @buf_dst: Buffer to store the decoded element.
> + * @buf_src: Buffer containing the elements in QMI wire format.
> + * @elem_len: Number of elements to be decoded.
> + * @elem_size: Size of a single instance of the element to be decoded.
> + *
> + * This function decodes the "elem_len" number of elements in QMI wire format,
> + * each of size "elem_size" bytes from the source buffer "buf_src" and stores
> + * the decoded elements in the destination buffer "buf_dst". The elements are
> + * of primary data type which include u8 - u64 or similar. This
> + * function returns the number of bytes of decoded information.
> + *
> + * Return: The total size of the decoded data elements, in bytes.
> + */
> +static int qmi_decode_basic_elem(void *buf_dst, const void *buf_src,
> + u32 elem_len, u32 elem_size)
> +{
> + u32 i, rc = 0;
Return type should not be int
> +
> + for (i = 0; i < elem_len; i++) {
> + QMI_ENCDEC_DECODE_N_BYTES(buf_dst, buf_src, elem_size);
> + rc += elem_size;
> + }
> +
> + return rc;
> +}
> +
> +/**
> + * qmi_decode_struct_elem() - Decodes elements of struct data type
> + * @ei_array: Struct info array descibing the struct element.
> + * @buf_dst: Buffer to store the decoded element.
> + * @buf_src: Buffer containing the elements in QMI wire format.
> + * @elem_len: Number of elements to be decoded.
> + * @tlv_len: Total size of the encoded inforation corresponding to
> + * this struct element.
> + * @dec_level: Depth of the nested structure from the main structure.
> + *
> + * This function decodes the "elem_len" number of elements in QMI wire format,
> + * each of size "(tlv_len/elem_len)" bytes from the source buffer "buf_src"
> + * and stores the decoded elements in the destination buffer "buf_dst". The
> + * elements are of struct data type which includes any C structure. This
> + * function returns the number of bytes of decoded information.
> + *
> + * Return: The total size of the decoded data elements on success, negative
> + * errno on error.
> + */
> +static int qmi_decode_struct_elem(struct qmi_elem_info *ei_array,
> + void *buf_dst, const void *buf_src,
> + u32 elem_len, u32 tlv_len,
> + int dec_level)
> +{
> + int i, rc, decoded_bytes = 0;
rc,decoded_bytes should be unsigned and change appropriate function
return type
> + struct qmi_elem_info *temp_ei = ei_array;
> +
> + for (i = 0; i < elem_len && decoded_bytes < tlv_len; i++) {
> + rc = qmi_decode(temp_ei->ei_array, buf_dst, buf_src,
> + tlv_len - decoded_bytes, dec_level);
> + if (rc < 0)
> + return rc;
> + buf_src = buf_src + rc;
> + buf_dst = buf_dst + temp_ei->elem_size;
> + decoded_bytes += rc;
> + }
> +
> + if ((dec_level <= 2 && decoded_bytes != tlv_len) ||
> + (dec_level > 2 && (i < elem_len || decoded_bytes > tlv_len))) {
> + pr_err("%s: Fault in decoding: dl(%d), db(%d), tl(%d), i(%d), el(%d)\n",
> + __func__, dec_level, decoded_bytes, tlv_len,
> + i, elem_len);
> + return -EFAULT;
> + }
> +
> + return decoded_bytes;
> +}
> +
> +/**
> + * qmi_decode_string_elem() - Decodes elements of string data type
> + * @ei_array: Struct info array descibing the string element.
> + * @buf_dst: Buffer to store the decoded element.
> + * @buf_src: Buffer containing the elements in QMI wire format.
> + * @tlv_len: Total size of the encoded inforation corresponding to
> + * this string element.
> + * @dec_level: Depth of the string element from the main structure.
> + *
> + * This function decodes the string element of maximum length
> + * "ei_array->elem_len" from the source buffer "buf_src" and puts it into
> + * the destination buffer "buf_dst". This function returns number of bytes
> + * decoded from the input buffer.
> + *
> + * Return: The total size of the decoded data elements on success, negative
> + * errno on error.
> + */
> +static int qmi_decode_string_elem(struct qmi_elem_info *ei_array,
> + void *buf_dst, const void *buf_src,
> + u32 tlv_len, int dec_level)
> +{
> + int rc;
> + int decoded_bytes = 0;
same as above
> + u32 string_len = 0;
> + u32 string_len_sz = 0;
> + struct qmi_elem_info *temp_ei = ei_array;
> +
> + if (dec_level == 1) {
> + string_len = tlv_len;
> + } else {
> + string_len_sz = temp_ei->elem_len <= U8_MAX ?
> + sizeof(u8) : sizeof(u16);
> + rc = qmi_decode_basic_elem(&string_len, buf_src,
> + 1, string_len_sz);
> + decoded_bytes += rc;
> + }
> +
> + if (string_len > temp_ei->elem_len) {
> + pr_err("%s: String len %d > Max Len %d\n",
> + __func__, string_len, temp_ei->elem_len);
> + return -ETOOSMALL;
> + } else if (string_len > tlv_len) {
> + pr_err("%s: String len %d > Input Buffer Len %d\n",
> + __func__, string_len, tlv_len);
> + return -EFAULT;
> + }
> +
> + rc = qmi_decode_basic_elem(buf_dst, buf_src + decoded_bytes,
> + string_len, temp_ei->elem_size);
> + *((char *)buf_dst + string_len) = '\0';
> + decoded_bytes += rc;
> +
> + return decoded_bytes;
> +}
> +
> +/**
> + * find_ei() - Find element info corresponding to TLV Type
> + * @ei_array: Struct info array of the message being decoded.
> + * @type: TLV Type of the element being searched.
> + *
> + * Every element that got encoded in the QMI message will have a type
> + * information associated with it. While decoding the QMI message,
> + * this function is used to find the struct info regarding the element
> + * that corresponds to the type being decoded.
> + *
> + * Return: Pointer to struct info, if found
> + */
> +static struct qmi_elem_info *find_ei(struct qmi_elem_info *ei_array,
> + u32 type)
> +{
> + struct qmi_elem_info *temp_ei = ei_array;
> +
> + while (temp_ei->data_type != QMI_EOTI) {
> + if (temp_ei->tlv_type == (u8)type)
> + return temp_ei;
> + temp_ei = temp_ei + 1;
> + }
> +
> + return NULL;
> +}
> +
> +/**
> + * qmi_decode() - Core Decode Function
> + * @ei_array: Struct info array describing the structure to be decoded.
> + * @out_c_struct: Buffer to hold the decoded C struct
> + * @in_buf: Buffer containing the QMI message to be decoded
> + * @in_buf_len: Length of the QMI message to be decoded
> + * @dec_level: Decode level to indicate the depth of the nested structure,
> + * within the main structure, being decoded
> + *
> + * Return: The number of bytes of decoded information on success, negative
> + * errno on error.
> + */
> +static int qmi_decode(struct qmi_elem_info *ei_array, void *out_c_struct,
> + const void *in_buf, u32 in_buf_len,
> + int dec_level)
> +{
> + struct qmi_elem_info *temp_ei = ei_array;
> + u8 opt_flag_value = 1;
> + u32 data_len_value = 0, data_len_sz = 0;
> + u8 *buf_dst = out_c_struct;
> + const u8 *tlv_pointer;
> + u32 tlv_len = 0;
> + u32 tlv_type;
> + u32 decoded_bytes = 0;
> + const void *buf_src = in_buf;
> + int rc;
> +
> + while (decoded_bytes < in_buf_len) {
> + if (dec_level >= 2 && temp_ei->data_type == QMI_EOTI)
> + return decoded_bytes;
> +
> + if (dec_level == 1) {
> + tlv_pointer = buf_src;
> + QMI_ENCDEC_DECODE_TLV(&tlv_type,
> + &tlv_len, tlv_pointer);
> + buf_src += (TLV_TYPE_SIZE + TLV_LEN_SIZE);
> + decoded_bytes += (TLV_TYPE_SIZE + TLV_LEN_SIZE);
> + temp_ei = find_ei(ei_array, tlv_type);
> + if (!temp_ei && tlv_type < OPTIONAL_TLV_TYPE_START) {
> + pr_err("%s: Inval element info\n", __func__);
> + return -EINVAL;
> + } else if (!temp_ei) {
> + UPDATE_DECODE_VARIABLES(buf_src,
> + decoded_bytes, tlv_len);
> + continue;
> + }
> + } else {
> + /*
> + * No length information for elements in nested
> + * structures. So use remaining decodable buffer space.
> + */
> + tlv_len = in_buf_len - decoded_bytes;
> + }
> +
> + buf_dst = out_c_struct + temp_ei->offset;
> + if (temp_ei->data_type == QMI_OPT_FLAG) {
> + memcpy(buf_dst, &opt_flag_value, sizeof(u8));
> + temp_ei = temp_ei + 1;
> + buf_dst = out_c_struct + temp_ei->offset;
> + }
> +
> + if (temp_ei->data_type == QMI_DATA_LEN) {
> + data_len_sz = temp_ei->elem_size == sizeof(u8) ?
> + sizeof(u8) : sizeof(u16);
> + rc = qmi_decode_basic_elem(&data_len_value, buf_src,
> + 1, data_len_sz);
> + memcpy(buf_dst, &data_len_value, sizeof(u32));
> + temp_ei = temp_ei + 1;
> + buf_dst = out_c_struct + temp_ei->offset;
> + tlv_len -= data_len_sz;
> + UPDATE_DECODE_VARIABLES(buf_src, decoded_bytes, rc);
> + }
> +
> + if (temp_ei->array_type == NO_ARRAY) {
> + data_len_value = 1;
> + } else if (temp_ei->array_type == STATIC_ARRAY) {
> + data_len_value = temp_ei->elem_len;
> + } else if (data_len_value > temp_ei->elem_len) {
> + pr_err("%s: Data len %d > max spec %d\n",
> + __func__, data_len_value, temp_ei->elem_len);
> + return -ETOOSMALL;
> + }
> +
> + switch (temp_ei->data_type) {
> + case QMI_UNSIGNED_1_BYTE:
> + case QMI_UNSIGNED_2_BYTE:
> + case QMI_UNSIGNED_4_BYTE:
> + case QMI_UNSIGNED_8_BYTE:
> + case QMI_SIGNED_2_BYTE_ENUM:
> + case QMI_SIGNED_4_BYTE_ENUM:
> + rc = qmi_decode_basic_elem(buf_dst, buf_src,
> + data_len_value,
> + temp_ei->elem_size);
> + UPDATE_DECODE_VARIABLES(buf_src, decoded_bytes, rc);
> + break;
> +
> + case QMI_STRUCT:
> + rc = qmi_decode_struct_elem(temp_ei, buf_dst, buf_src,
> + data_len_value, tlv_len,
> + dec_level + 1);
> + if (rc < 0)
> + return rc;
> + UPDATE_DECODE_VARIABLES(buf_src, decoded_bytes, rc);
> + break;
> +
> + case QMI_STRING:
> + rc = qmi_decode_string_elem(temp_ei, buf_dst, buf_src,
> + tlv_len, dec_level);
> + if (rc < 0)
> + return rc;
> + UPDATE_DECODE_VARIABLES(buf_src, decoded_bytes, rc);
> + break;
> +
> + default:
> + pr_err("%s: Unrecognized data type\n", __func__);
> + return -EINVAL;
> + }
> + temp_ei = temp_ei + 1;
> + }
> +
> + return decoded_bytes;
> +}
> +
> +/**
> + * qmi_encode_message() - Encode C structure as QMI encoded message
> + * @type: Type of QMI message
> + * @msg_id: Message ID of the message
> + * @len: Passed as max length of the message, updated to actual size
> + * @txn_id: Transaction ID
> + * @ei: QMI message descriptor
> + * @c_struct: Reference to structure to encode
> + *
> + * Return: Buffer with encoded message, or negative ERR_PTR() on error
> + */
> +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)
> +{
> + struct qmi_header *hdr;
> + ssize_t msglen = 0;
> + void *msg;
> + int ret;
> +
> + /* Check the possibility of a zero length QMI message */
> + if (!c_struct) {
> + ret = qmi_calc_min_msg_len(ei, 1);
> + if (ret) {
> + pr_err("%s: Calc. len %d != 0, but NULL c_struct\n",
> + __func__, ret);
> + return ERR_PTR(-EINVAL);
> + }
> + }
> +
> + msg = kzalloc(sizeof(*hdr) + *len, GFP_KERNEL);
> + if (!msg)
> + return ERR_PTR(-ENOMEM);
> +
> + /* Encode message, if we have a message */
> + if (c_struct) {
> + msglen = qmi_encode(ei, msg + sizeof(*hdr), c_struct, *len, 1);
> + if (msglen < 0) {
> + kfree(msg);
> + return ERR_PTR(msglen);
> + }
> + }
> +
> + hdr = msg;
> + hdr->type = type;
> + hdr->txn_id = txn_id;
> + hdr->msg_id = msg_id;
> + hdr->msg_len = msglen;
> +
> + *len = sizeof(*hdr) + msglen;
> +
> + return msg;
> +}
> +EXPORT_SYMBOL(qmi_encode_message);
> +
> +/**
> + * qmi_decode_message() - Decode QMI encoded message to C structure
> + * @buf: Buffer with encoded message
> + * @len: Amount of data in @buf
> + * @ei: QMI message descriptor
> + * @c_struct: Reference to structure to decode into
> + *
> + * Return: The number of bytes of decoded information on success, negative
> + * errno on error.
> + */
> +int qmi_decode_message(const void *buf, size_t len,
> + struct qmi_elem_info *ei, void *c_struct)
> +{
> + if (!ei)
> + return -EINVAL;
> +
> + if (!c_struct || !buf || !len)
> + return -EINVAL;
> +
> + return qmi_decode(ei, c_struct, buf + sizeof(struct qmi_header),
> + len - sizeof(struct qmi_header), 1);
> +}
> +EXPORT_SYMBOL(qmi_decode_message);
> +
> +/* Common header in all QMI responses */
> +struct qmi_elem_info qmi_response_type_v01_ei[] = {
> + {
> + .data_type = QMI_SIGNED_2_BYTE_ENUM,
> + .elem_len = 1,
> + .elem_size = sizeof(u16),
> + .array_type = NO_ARRAY,
> + .tlv_type = QMI_COMMON_TLV_TYPE,
> + .offset = offsetof(struct qmi_response_type_v01, result),
> + .ei_array = NULL,
> + },
> + {
> + .data_type = QMI_SIGNED_2_BYTE_ENUM,
> + .elem_len = 1,
> + .elem_size = sizeof(u16),
> + .array_type = NO_ARRAY,
> + .tlv_type = QMI_COMMON_TLV_TYPE,
> + .offset = offsetof(struct qmi_response_type_v01, error),
> + .ei_array = NULL,
> + },
> + {
> + .data_type = QMI_EOTI,
> + .elem_len = 0,
> + .elem_size = 0,
> + .array_type = NO_ARRAY,
> + .tlv_type = QMI_COMMON_TLV_TYPE,
> + .offset = 0,
> + .ei_array = NULL,
> + },
> +};
> +EXPORT_SYMBOL(qmi_response_type_v01_ei);
> +
> +MODULE_DESCRIPTION("QMI encoder/decoder helper");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/soc/qcom/qmi.h b/include/linux/soc/qcom/qmi.h
> new file mode 100644
> index 000000000000..1b66e9a6074f
> --- /dev/null
> +++ b/include/linux/soc/qcom/qmi.h
> @@ -0,0 +1,114 @@
> +/*
> + * Copyright (c) 2012-2014, The Linux Foundation. All rights reserved.
> + * Copyright (c) 2017, Linaro Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * 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.
> + */
> +#ifndef __QMI_HELPERS_H__
> +#define __QMI_HELPERS_H__
> +
> +#include <linux/types.h>
> +
> +/**
> + * qmi_header - wireformat header of QMI messages
> + * @type: type of message
> + * @txn_id: transaction id
> + * @msg_id: message id
> + * @msg_len: length of message payload following header
> + */
> +struct qmi_header {
> + u8 type;
> + u16 txn_id;
> + u16 msg_id;
> + u16 msg_len;
> +} __packed;
> +
> +#define QMI_REQUEST 0
> +#define QMI_RESPONSE 2
> +#define QMI_INDICATION 4
> +
> +#define QMI_COMMON_TLV_TYPE 0
> +
> +enum qmi_elem_type {
> + QMI_EOTI,
> + QMI_OPT_FLAG,
> + QMI_DATA_LEN,
> + QMI_UNSIGNED_1_BYTE,
> + QMI_UNSIGNED_2_BYTE,
> + QMI_UNSIGNED_4_BYTE,
> + QMI_UNSIGNED_8_BYTE,
> + QMI_SIGNED_2_BYTE_ENUM,
> + QMI_SIGNED_4_BYTE_ENUM,
> + QMI_STRUCT,
> + QMI_STRING,
> +};
> +
> +enum qmi_array_type {
> + NO_ARRAY,
> + STATIC_ARRAY,
> + VAR_LEN_ARRAY,
> +};
> +
> +/**
> + * struct qmi_elem_info - describes how to encode a single QMI element
> + * @data_type: Data type of this element.
> + * @elem_len: Array length of this element, if an array.
> + * @elem_size: Size of a single instance of this data type.
> + * @array_type: Array type of this element.
> + * @tlv_type: QMI message specific type to identify which element
> + * is present in an incoming message.
> + * @offset: Specifies the offset of the first instance of this
> + * element in the data structure.
> + * @ei_array: Null-terminated array of @qmi_elem_info to describe nested
> + * structures.
> + */
> +struct qmi_elem_info {
> + enum qmi_elem_type data_type;
> + u32 elem_len;
> + u32 elem_size;
> + enum qmi_array_type array_type;
> + u8 tlv_type;
> + u32 offset;
> + struct qmi_elem_info *ei_array;
> +};
> +
> +#define QMI_RESULT_SUCCESS_V01 0
> +#define QMI_RESULT_FAILURE_V01 1
> +
> +#define QMI_ERR_NONE_V01 0
> +#define QMI_ERR_MALFORMED_MSG_V01 1
> +#define QMI_ERR_NO_MEMORY_V01 2
> +#define QMI_ERR_INTERNAL_V01 3
> +#define QMI_ERR_CLIENT_IDS_EXHAUSTED_V01 5
> +#define QMI_ERR_INVALID_ID_V01 41
> +#define QMI_ERR_ENCODING_V01 58
> +#define QMI_ERR_INCOMPATIBLE_STATE_V01 90
> +#define QMI_ERR_NOT_SUPPORTED_V01 94
> +
> +/**
> + * qmi_response_type_v01 - common response header (decoded)
> + * @result: result of the transaction
> + * @error: error value, when @result is QMI_RESULT_FAILURE_V01
> + */
> +struct qmi_response_type_v01 {
> + u16 result;
> + u16 error;
> +};
> +
> +extern struct qmi_elem_info qmi_response_type_v01_ei[];
> +
> +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);
> +
> +int qmi_decode_message(const void *buf, size_t len,
> + struct qmi_elem_info *ei, void *c_struct);
> +
> +#endif
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v4 1/5] soc: qcom: Introduce QMI encoder/decoder
2017-12-01 9:10 ` Jitendra Sharma
@ 2017-12-05 17:33 ` Bjorn Andersson
0 siblings, 0 replies; 24+ messages in thread
From: Bjorn Andersson @ 2017-12-05 17:33 UTC (permalink / raw)
To: Jitendra Sharma
Cc: Andy Gross, Ohad Ben-Cohen, Arun Kumar Neelakantam, Chris Lew,
linux-kernel, linux-arm-msm, linux-soc, linux-remoteproc
On Fri 01 Dec 01:10 PST 2017, Jitendra Sharma wrote:
> Hi Bjorn,
>
Hi Jitendra,
> On 11/30/2017 6:46 AM, Bjorn Andersson wrote:
> > +static int qmi_calc_min_msg_len(struct qmi_elem_info *ei_array,
> > + int level)
> > +{
> > + int min_msg_len = 0;
> min_msg_len should be u32 as it would not be negative
IMHO all these sizes should be size_t and there are a few places where
we should add "const" specifier to pointers. But I refrained from
"fixing" these up in an effort to keep things as close to the downstream
implementation as possible.
If Andy agrees I'm hoping that we can take this version and then follow
up with a separate patch, making the review more explicit and make it
easier to track down any potential regressions from such a change.
Thank you for the review!
Regards,
Bjorn
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v4 2/5] soc: qcom: Introduce QMI helpers
2017-11-30 1:16 [PATCH v4 0/5] In-kernel QMI helpers and sysmon Bjorn Andersson
2017-11-30 1:16 ` [PATCH v4 1/5] soc: qcom: Introduce QMI encoder/decoder Bjorn Andersson
@ 2017-11-30 1:16 ` Bjorn Andersson
2017-11-30 8:18 ` Philippe Ombredanne
2017-11-30 17:33 ` Chris Lew
2017-11-30 1:16 ` [PATCH v4 3/5] remoteproc: Pass type of shutdown to subdev remove Bjorn Andersson
` (2 subsequent siblings)
4 siblings, 2 replies; 24+ messages in thread
From: Bjorn Andersson @ 2017-11-30 1:16 UTC (permalink / raw)
To: Andy Gross, Ohad Ben-Cohen, Bjorn Andersson
Cc: Arun Kumar Neelakantam, Chris Lew, linux-kernel, linux-arm-msm,
linux-soc, linux-remoteproc
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.
Tested-By: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
Changes since v3:
- Don't invoke handler for responses without registered txn
- Kerneldoc updates
- Make qmi_handle_init parameters const
- Added appropriate includes and forward declarations in header file
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/Makefile | 2 +-
drivers/soc/qcom/qmi_interface.c | 857 +++++++++++++++++++++++++++++++++++++++
include/linux/soc/qcom/qmi.h | 165 ++++++++
3 files changed, 1023 insertions(+), 1 deletion(-)
create mode 100644 drivers/soc/qcom/qmi_interface.c
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index 37f85b45d0a1..dcebf2814e6d 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -4,7 +4,7 @@ obj-$(CONFIG_QCOM_GSBI) += qcom_gsbi.o
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_encdec.o qmi_interface.o
obj-$(CONFIG_QCOM_RMTFS_MEM) += rmtfs_mem.o
obj-$(CONFIG_QCOM_SMD_RPM) += smd-rpm.o
obj-$(CONFIG_QCOM_SMEM) += smem.o
diff --git a/drivers/soc/qcom/qmi_interface.c b/drivers/soc/qcom/qmi_interface.c
new file mode 100644
index 000000000000..4027b52b0834
--- /dev/null
+++ b/drivers/soc/qcom/qmi_interface.c
@@ -0,0 +1,857 @@
+/*
+ * 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/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
+ * @service: service id of the new server
+ * @instance: instance id of the new server
+ * @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 = { };
+ 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
+ * @version: version number of the request
+ *
+ * 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.
+ *
+ * Return: 0 on success, negative errno on failure.
+ */
+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 = { };
+ 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 = cpu_to_le32(qmi->sq.sq_node);
+ pkt.server.port = cpu_to_le32(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
+ * @version: version of the service
+ *
+ * Register a new service with the name service. This allows clients to find
+ * and start sending messages to the client associated with @qmi.
+ *
+ * Return: 0 on success, negative errno on failure.
+ */
+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)
+ *
+ * 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.
+ *
+ * Return: Transaction id on success, negative errno on failure.
+ */
+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
+ *
+ * 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.
+ *
+ * Return: the transaction response on success, negative errno on failure.
+ */
+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)
+{
+ const 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);
+
+ /* Ignore unexpected responses */
+ if (!txn) {
+ mutex_unlock(&qmi->txn_lock);
+ return;
+ }
+
+ mutex_lock(&txn->lock);
+ mutex_unlock(&qmi->txn_lock);
+
+ if (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);
+ } else {
+ 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
+ * @recv_buf_size: maximum size of incoming message
+ * @ops: reference to callbacks for QRTR notifications
+ * @handlers: NULL-terminated list of QMI message handlers
+ *
+ * This initializes the QMI client handle to allow sending and receiving QMI
+ * messages. As messages are received the appropriate handler will be invoked.
+ *
+ * Return: 0 on success, negative errno on failure.
+ */
+int qmi_handle_init(struct qmi_handle *qmi, size_t recv_buf_size,
+ const struct qmi_ops *ops,
+ const 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
+ *
+ * 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.
+ *
+ * Return: 0 on success, negative errno on failure.
+ */
+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 = {};
+ 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
+ *
+ * Return: 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
+ *
+ * Return: 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
+ * @msg_id: message id
+ * @len: max length of the QMI message
+ * @ei: QMI message description
+ * @c_struct: object to be encoded
+ *
+ * Return: 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 1b66e9a6074f..f6e5d31d2829 100644
--- a/include/linux/soc/qcom/qmi.h
+++ b/include/linux/soc/qcom/qmi.h
@@ -14,7 +14,14 @@
#ifndef __QMI_HELPERS_H__
#define __QMI_HELPERS_H__
+#include <linux/completion.h>
+#include <linux/idr.h>
+#include <linux/list.h>
+#include <linux/qrtr.h>
#include <linux/types.h>
+#include <linux/workqueue.h>
+
+struct socket;
/**
* qmi_header - wireformat header of QMI messages
@@ -104,6 +111,159 @@ 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;
+
+ const 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,
+ const struct qmi_ops *ops,
+ const 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);
@@ -111,4 +271,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
--
2.15.0
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH v4 2/5] soc: qcom: Introduce QMI helpers
2017-11-30 1:16 ` [PATCH v4 2/5] soc: qcom: Introduce QMI helpers Bjorn Andersson
@ 2017-11-30 8:18 ` Philippe Ombredanne
2017-12-01 5:35 ` Bjorn Andersson
2017-11-30 17:33 ` Chris Lew
1 sibling, 1 reply; 24+ messages in thread
From: Philippe Ombredanne @ 2017-11-30 8:18 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Andy Gross, Ohad Ben-Cohen, Arun Kumar Neelakantam, Chris Lew,
LKML, linux-arm-msm, linux-soc, linux-remoteproc
Bjorn,
On Thu, Nov 30, 2017 at 2:16 AM, Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
[]
> diff --git a/drivers/soc/qcom/qmi_interface.c b/drivers/soc/qcom/qmi_interface.c
> new file mode 100644
> index 000000000000..4027b52b0834
> --- /dev/null
> +++ b/drivers/soc/qcom/qmi_interface.c
> @@ -0,0 +1,857 @@
> +/*
> + * 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.
> + */
Could it make sense, especially for new files, to use the new SPDX ids
and avoid adding boilerplate that will need to be cleaned up later?
e.g. something like this instead, using the new conventions started by
greg-kh and by documented tglx?
NB: the // comment style is not a mistake and is what Linus wants
there. See the threads on this topic.
> @@ -0,0 +1,857 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2017 Linaro Ltd.
> + */
Isn't this shorter and better? :P
BTW, if you need help to fix this on the rest of Linaro contributed
code, I maintain a tool that can help there.
--
Cordially
Philippe Ombredanne, the licensing janitor bot
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v4 2/5] soc: qcom: Introduce QMI helpers
2017-11-30 8:18 ` Philippe Ombredanne
@ 2017-12-01 5:35 ` Bjorn Andersson
2017-12-01 7:48 ` Philippe Ombredanne
0 siblings, 1 reply; 24+ messages in thread
From: Bjorn Andersson @ 2017-12-01 5:35 UTC (permalink / raw)
To: Philippe Ombredanne
Cc: Andy Gross, Ohad Ben-Cohen, Arun Kumar Neelakantam, Chris Lew,
LKML, linux-arm-msm, linux-soc, linux-remoteproc
On Thu 30 Nov 00:18 PST 2017, Philippe Ombredanne wrote:
> Bjorn,
>
> On Thu, Nov 30, 2017 at 2:16 AM, Bjorn Andersson
> <bjorn.andersson@linaro.org> wrote:
> []
> > diff --git a/drivers/soc/qcom/qmi_interface.c b/drivers/soc/qcom/qmi_interface.c
> > new file mode 100644
> > index 000000000000..4027b52b0834
> > --- /dev/null
> > +++ b/drivers/soc/qcom/qmi_interface.c
> > @@ -0,0 +1,857 @@
> > +/*
> > + * 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.
> > + */
>
> Could it make sense, especially for new files, to use the new SPDX ids
> and avoid adding boilerplate that will need to be cleaned up later?
> e.g. something like this instead, using the new conventions started by
> greg-kh and by documented tglx?
>
> NB: the // comment style is not a mistake and is what Linus wants
> there. See the threads on this topic.
>
> > @@ -0,0 +1,857 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2017 Linaro Ltd.
> > + */
>
> Isn't this shorter and better? :P
>
That sounds very reasonable, I will update the patches.
> BTW, if you need help to fix this on the rest of Linaro contributed
> code, I maintain a tool that can help there.
>
I haven't seen any guidelines on how this should be introduced
throughout the kernel, should I make a similar push for the subsystems I
maintain?
Regards,
Bjorn
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v4 2/5] soc: qcom: Introduce QMI helpers
2017-12-01 5:35 ` Bjorn Andersson
@ 2017-12-01 7:48 ` Philippe Ombredanne
0 siblings, 0 replies; 24+ messages in thread
From: Philippe Ombredanne @ 2017-12-01 7:48 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Andy Gross, Ohad Ben-Cohen, Arun Kumar Neelakantam, Chris Lew,
LKML, linux-arm-msm, linux-soc, linux-remoteproc,
Thomas Gleixner, Greg Kroah-Hartman, Jonathan Corbet
On Fri, Dec 1, 2017 at 6:35 AM, Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
> On Thu 30 Nov 00:18 PST 2017, Philippe Ombredanne wrote:
>
>> Bjorn,
>>
>> On Thu, Nov 30, 2017 at 2:16 AM, Bjorn Andersson
>> <bjorn.andersson@linaro.org> wrote:
>> []
>> > diff --git a/drivers/soc/qcom/qmi_interface.c b/drivers/soc/qcom/qmi_interface.c
>> > new file mode 100644
>> > index 000000000000..4027b52b0834
>> > --- /dev/null
>> > +++ b/drivers/soc/qcom/qmi_interface.c
>> > @@ -0,0 +1,857 @@
>> > +/*
>> > + * 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.
>> > + */
>>
>> Could it make sense, especially for new files, to use the new SPDX ids
>> and avoid adding boilerplate that will need to be cleaned up later?
>> e.g. something like this instead, using the new conventions started by
>> greg-kh and by documented tglx?
>>
>> NB: the // comment style is not a mistake and is what Linus wants
>> there. See the threads on this topic.
>>
>> > @@ -0,0 +1,857 @@
>> > +// SPDX-License-Identifier: GPL-2.0
>> > +/*
>> > + * Copyright (C) 2017 Linaro Ltd.
>> > + */
>>
>> Isn't this shorter and better? :P
>>
>
> That sounds very reasonable, I will update the patches.
Note that when you only have a copyright left in a header comment
after removing the boilerplate, you might want to consider this even
shorter and cleaner form instead. But that's just me telling this:
>> > +// SPDX-License-Identifier: GPL-2.0
>> > +// Copyright (C) 2017 Linaro Ltd.
>> BTW, if you need help to fix this on the rest of Linaro contributed
>> code, I maintain a tool that can help there.
>>
>
> I haven't seen any guidelines on how this should be introduced
> throughout the kernel,
Thomas has sent a first doc patch [1] set. AFAIK he is working on an
updated version whenever his real time body clock yields a few ticks
so he can wrap this out. But his real time clock only rarely yield
ticks outside of hard real time coding work ;)
Jonathan also wrote a nice background article on the topic at LWN [2].
I heard this was also briefly discussed among maintainers at the
plumbers meetup in Los Angeles: for me I only briefly discussed this
with Linus and Greg just before the meetup, but I could not stay
longer.
> should I make a similar push for the subsystems I
> maintain?
That would be great! And beside the fact it always feels good to
delete lines of code for once, this is one of the rare areas where you
can kill some lines and be pretty sure it will still compile and run
afterwards .... well.... in most cases: Greg and Thomas found funny
cases of .h headers included in assembly where using the // comment
style was actually making things break weirdly and the compilation
would go down in flames with a beautiful tail spin : in these cases,
the convention is to use /**/ comments instead)
Again, if you need help with this, I can modestly chip in with my
scancode-toolkit tool and good intentions.
[1] https://marc.info/?l=linux-kernel&m=151051532322831&w=2
[2] https://lwn.net/SubscriberLink/739183/262749cbe307ddc7/
--
Cordially
Philippe Ombredanne, a happy licensing comments cleaner
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v4 2/5] soc: qcom: Introduce QMI helpers
2017-11-30 1:16 ` [PATCH v4 2/5] soc: qcom: Introduce QMI helpers Bjorn Andersson
2017-11-30 8:18 ` Philippe Ombredanne
@ 2017-11-30 17:33 ` Chris Lew
1 sibling, 0 replies; 24+ messages in thread
From: Chris Lew @ 2017-11-30 17:33 UTC (permalink / raw)
To: Bjorn Andersson, Andy Gross, Ohad Ben-Cohen
Cc: Arun Kumar Neelakantam, linux-kernel, linux-arm-msm, linux-soc,
linux-remoteproc
On 11/29/2017 5:16 PM, 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.
>
> Tested-By: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
Acked-By: Chris Lew <clew@codeaurora.org>
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v4 3/5] remoteproc: Pass type of shutdown to subdev remove
2017-11-30 1:16 [PATCH v4 0/5] In-kernel QMI helpers and sysmon Bjorn Andersson
2017-11-30 1:16 ` [PATCH v4 1/5] soc: qcom: Introduce QMI encoder/decoder Bjorn Andersson
2017-11-30 1:16 ` [PATCH v4 2/5] soc: qcom: Introduce QMI helpers Bjorn Andersson
@ 2017-11-30 1:16 ` Bjorn Andersson
2017-11-30 17:35 ` Chris Lew
2017-12-01 14:50 ` Arnaud Pouliquen
2017-11-30 1:16 ` [PATCH v4 4/5] remoteproc: qcom: Introduce sysmon Bjorn Andersson
2017-11-30 1:16 ` [PATCH v4 5/5] samples: Introduce Qualcomm QMI sample client Bjorn Andersson
4 siblings, 2 replies; 24+ messages in thread
From: Bjorn Andersson @ 2017-11-30 1:16 UTC (permalink / raw)
To: Andy Gross, Ohad Ben-Cohen, Bjorn Andersson
Cc: Arun Kumar Neelakantam, Chris Lew, linux-kernel, linux-arm-msm,
linux-soc, linux-remoteproc
remoteproc instances can be stopped either by invoking shutdown or by an
attempt to recover from a crash. For some subdev types it's expected to
clean up gracefully during a shutdown, but are unable to do so during a
crash - so pass this information to the subdev remove functions.
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
Changes since v3:
- None
Changes since v2:
- None
Changes since v1:
- New patch
drivers/remoteproc/qcom_common.c | 6 +++---
drivers/remoteproc/remoteproc_core.c | 18 +++++++++---------
include/linux/remoteproc.h | 4 ++--
3 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/drivers/remoteproc/qcom_common.c b/drivers/remoteproc/qcom_common.c
index d487040b528b..7a862d41503e 100644
--- a/drivers/remoteproc/qcom_common.c
+++ b/drivers/remoteproc/qcom_common.c
@@ -60,7 +60,7 @@ static int glink_subdev_probe(struct rproc_subdev *subdev)
return IS_ERR(glink->edge) ? PTR_ERR(glink->edge) : 0;
}
-static void glink_subdev_remove(struct rproc_subdev *subdev)
+static void glink_subdev_remove(struct rproc_subdev *subdev, bool graceful)
{
struct qcom_rproc_glink *glink = to_glink_subdev(subdev);
@@ -107,7 +107,7 @@ static int smd_subdev_probe(struct rproc_subdev *subdev)
return PTR_ERR_OR_ZERO(smd->edge);
}
-static void smd_subdev_remove(struct rproc_subdev *subdev)
+static void smd_subdev_remove(struct rproc_subdev *subdev, bool graceful)
{
struct qcom_rproc_subdev *smd = to_smd_subdev(subdev);
@@ -176,7 +176,7 @@ static int ssr_notify_start(struct rproc_subdev *subdev)
return 0;
}
-static void ssr_notify_stop(struct rproc_subdev *subdev)
+static void ssr_notify_stop(struct rproc_subdev *subdev, bool graceful)
{
struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index eab14b414bf0..3146e965ca47 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -307,7 +307,7 @@ static int rproc_vdev_do_probe(struct rproc_subdev *subdev)
return rproc_add_virtio_dev(rvdev, rvdev->id);
}
-static void rproc_vdev_do_remove(struct rproc_subdev *subdev)
+static void rproc_vdev_do_remove(struct rproc_subdev *subdev, bool graceful)
{
struct rproc_vdev *rvdev = container_of(subdev, struct rproc_vdev, subdev);
@@ -785,17 +785,17 @@ static int rproc_probe_subdevices(struct rproc *rproc)
unroll_registration:
list_for_each_entry_continue_reverse(subdev, &rproc->subdevs, node)
- subdev->remove(subdev);
+ subdev->remove(subdev, false);
return ret;
}
-static void rproc_remove_subdevices(struct rproc *rproc)
+static void rproc_remove_subdevices(struct rproc *rproc, bool graceful)
{
struct rproc_subdev *subdev;
list_for_each_entry_reverse(subdev, &rproc->subdevs, node)
- subdev->remove(subdev);
+ subdev->remove(subdev, graceful);
}
/**
@@ -1013,13 +1013,13 @@ static int rproc_trigger_auto_boot(struct rproc *rproc)
return ret;
}
-static int rproc_stop(struct rproc *rproc)
+static int rproc_stop(struct rproc *rproc, bool graceful)
{
struct device *dev = &rproc->dev;
int ret;
/* remove any subdevices for the remote processor */
- rproc_remove_subdevices(rproc);
+ rproc_remove_subdevices(rproc, graceful);
/* power off the remote processor */
ret = rproc->ops->stop(rproc);
@@ -1063,7 +1063,7 @@ int rproc_trigger_recovery(struct rproc *rproc)
if (ret)
return ret;
- ret = rproc_stop(rproc);
+ ret = rproc_stop(rproc, false);
if (ret)
goto unlock_mutex;
@@ -1216,7 +1216,7 @@ void rproc_shutdown(struct rproc *rproc)
if (!atomic_dec_and_test(&rproc->power))
goto out;
- ret = rproc_stop(rproc);
+ ret = rproc_stop(rproc, true);
if (ret) {
atomic_inc(&rproc->power);
goto out;
@@ -1550,7 +1550,7 @@ EXPORT_SYMBOL(rproc_del);
void rproc_add_subdev(struct rproc *rproc,
struct rproc_subdev *subdev,
int (*probe)(struct rproc_subdev *subdev),
- void (*remove)(struct rproc_subdev *subdev))
+ void (*remove)(struct rproc_subdev *subdev, bool graceful))
{
subdev->probe = probe;
subdev->remove = remove;
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 44e630eb3d94..20a9467744ea 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -456,7 +456,7 @@ struct rproc_subdev {
struct list_head node;
int (*probe)(struct rproc_subdev *subdev);
- void (*remove)(struct rproc_subdev *subdev);
+ void (*remove)(struct rproc_subdev *subdev, bool graceful);
};
/* we currently support only two vrings per rvdev */
@@ -539,7 +539,7 @@ static inline struct rproc *vdev_to_rproc(struct virtio_device *vdev)
void rproc_add_subdev(struct rproc *rproc,
struct rproc_subdev *subdev,
int (*probe)(struct rproc_subdev *subdev),
- void (*remove)(struct rproc_subdev *subdev));
+ void (*remove)(struct rproc_subdev *subdev, bool graceful));
void rproc_remove_subdev(struct rproc *rproc, struct rproc_subdev *subdev);
--
2.15.0
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH v4 3/5] remoteproc: Pass type of shutdown to subdev remove
2017-11-30 1:16 ` [PATCH v4 3/5] remoteproc: Pass type of shutdown to subdev remove Bjorn Andersson
@ 2017-11-30 17:35 ` Chris Lew
2017-12-01 14:50 ` Arnaud Pouliquen
1 sibling, 0 replies; 24+ messages in thread
From: Chris Lew @ 2017-11-30 17:35 UTC (permalink / raw)
To: Bjorn Andersson, Andy Gross, Ohad Ben-Cohen
Cc: Arun Kumar Neelakantam, linux-kernel, linux-arm-msm, linux-soc,
linux-remoteproc
On 11/29/2017 5:16 PM, Bjorn Andersson wrote:
> remoteproc instances can be stopped either by invoking shutdown or by an
> attempt to recover from a crash. For some subdev types it's expected to
> clean up gracefully during a shutdown, but are unable to do so during a
> crash - so pass this information to the subdev remove functions.
>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
Acked-By: Chris Lew <clew@codeaurora.org>
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v4 3/5] remoteproc: Pass type of shutdown to subdev remove
2017-11-30 1:16 ` [PATCH v4 3/5] remoteproc: Pass type of shutdown to subdev remove Bjorn Andersson
2017-11-30 17:35 ` Chris Lew
@ 2017-12-01 14:50 ` Arnaud Pouliquen
2017-12-05 6:46 ` Bjorn Andersson
1 sibling, 1 reply; 24+ messages in thread
From: Arnaud Pouliquen @ 2017-12-01 14:50 UTC (permalink / raw)
To: Bjorn Andersson, Andy Gross, Ohad Ben-Cohen
Cc: Arun Kumar Neelakantam, Chris Lew, linux-kernel, linux-arm-msm,
linux-soc, linux-remoteproc
hello Bjorn,
Sorry for these late remarks/questions
On 11/30/2017 02:16 AM, Bjorn Andersson wrote:
> remoteproc instances can be stopped either by invoking shutdown or by an
> attempt to recover from a crash. For some subdev types it's expected to
> clean up gracefully during a shutdown, but are unable to do so during a
> crash - so pass this information to the subdev remove functions.
>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
>
> Changes since v3:
> - None
>
> Changes since v2:
> - None
>
> Changes since v1:
> - New patch
>
> drivers/remoteproc/qcom_common.c | 6 +++---
> drivers/remoteproc/remoteproc_core.c | 18 +++++++++---------
> include/linux/remoteproc.h | 4 ++--
> 3 files changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/remoteproc/qcom_common.c b/drivers/remoteproc/qcom_common.c
> index d487040b528b..7a862d41503e 100644
> --- a/drivers/remoteproc/qcom_common.c
> +++ b/drivers/remoteproc/qcom_common.c
> @@ -60,7 +60,7 @@ static int glink_subdev_probe(struct rproc_subdev *subdev)
> return IS_ERR(glink->edge) ? PTR_ERR(glink->edge) : 0;
> }
>
> -static void glink_subdev_remove(struct rproc_subdev *subdev)
> +static void glink_subdev_remove(struct rproc_subdev *subdev, bool graceful)
> {
> struct qcom_rproc_glink *glink = to_glink_subdev(subdev);
>
> @@ -107,7 +107,7 @@ static int smd_subdev_probe(struct rproc_subdev *subdev)
> return PTR_ERR_OR_ZERO(smd->edge);
> }
>
> -static void smd_subdev_remove(struct rproc_subdev *subdev)
> +static void smd_subdev_remove(struct rproc_subdev *subdev, bool graceful)
> {
> struct qcom_rproc_subdev *smd = to_smd_subdev(subdev);
>
> @@ -176,7 +176,7 @@ static int ssr_notify_start(struct rproc_subdev *subdev)
> return 0;
> }
>
> -static void ssr_notify_stop(struct rproc_subdev *subdev)
> +static void ssr_notify_stop(struct rproc_subdev *subdev, bool graceful)
> {
> struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
>
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index eab14b414bf0..3146e965ca47 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -307,7 +307,7 @@ static int rproc_vdev_do_probe(struct rproc_subdev *subdev)
> return rproc_add_virtio_dev(rvdev, rvdev->id);
> }
>
> -static void rproc_vdev_do_remove(struct rproc_subdev *subdev)
> +static void rproc_vdev_do_remove(struct rproc_subdev *subdev, bool graceful)
> {
> struct rproc_vdev *rvdev = container_of(subdev, struct rproc_vdev, subdev);
>
> @@ -785,17 +785,17 @@ static int rproc_probe_subdevices(struct rproc *rproc)
>
> unroll_registration:
> list_for_each_entry_continue_reverse(subdev, &rproc->subdevs, node)
> - subdev->remove(subdev);
> + subdev->remove(subdev, false);
Why do you need to do a non graceful remove in this case? This could
lead to side effect like memory leakage...
>
> return ret;
> }
>
> -static void rproc_remove_subdevices(struct rproc *rproc)
> +static void rproc_remove_subdevices(struct rproc *rproc, bool graceful)
> {
> struct rproc_subdev *subdev;
>
> list_for_each_entry_reverse(subdev, &rproc->subdevs, node)
> - subdev->remove(subdev);
> + subdev->remove(subdev, graceful);
> }
>
> /**
> @@ -1013,13 +1013,13 @@ static int rproc_trigger_auto_boot(struct rproc *rproc)
> return ret;
> }
>
> -static int rproc_stop(struct rproc *rproc)
> +static int rproc_stop(struct rproc *rproc, bool graceful)
> {
> struct device *dev = &rproc->dev;
> int ret;
>
> /* remove any subdevices for the remote processor */
> - rproc_remove_subdevices(rproc);
> + rproc_remove_subdevices(rproc, graceful);
>
> /* power off the remote processor */
> ret = rproc->ops->stop(rproc);
> @@ -1063,7 +1063,7 @@ int rproc_trigger_recovery(struct rproc *rproc)
> if (ret)
> return ret;
>
> - ret = rproc_stop(rproc);
> + ret = rproc_stop(rproc, false);
> if (ret)
> goto unlock_mutex;
>
> @@ -1216,7 +1216,7 @@ void rproc_shutdown(struct rproc *rproc)
> if (!atomic_dec_and_test(&rproc->power))
> goto out;
>
> - ret = rproc_stop(rproc);
> + ret = rproc_stop(rproc, true);
> if (ret) {
> atomic_inc(&rproc->power);
> goto out;
> @@ -1550,7 +1550,7 @@ EXPORT_SYMBOL(rproc_del);
> void rproc_add_subdev(struct rproc *rproc,
> struct rproc_subdev *subdev,
> int (*probe)(struct rproc_subdev *subdev),
> - void (*remove)(struct rproc_subdev *subdev))
> + void (*remove)(struct rproc_subdev *subdev, bool graceful))
> {
> subdev->probe = probe;
> subdev->remove = remove;
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index 44e630eb3d94..20a9467744ea 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -456,7 +456,7 @@ struct rproc_subdev {
> struct list_head node;
>
> int (*probe)(struct rproc_subdev *subdev);
> - void (*remove)(struct rproc_subdev *subdev);
> + void (*remove)(struct rproc_subdev *subdev, bool graceful);
What about adding a new ops instead of a parameter, like a recovery
callback?
Regard
Arnaud
> };
>
> /* we currently support only two vrings per rvdev */
> @@ -539,7 +539,7 @@ static inline struct rproc *vdev_to_rproc(struct virtio_device *vdev)
> void rproc_add_subdev(struct rproc *rproc,
> struct rproc_subdev *subdev,
> int (*probe)(struct rproc_subdev *subdev),
> - void (*remove)(struct rproc_subdev *subdev));
> + void (*remove)(struct rproc_subdev *subdev, bool graceful));
>
> void rproc_remove_subdev(struct rproc *rproc, struct rproc_subdev *subdev);
>
>
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v4 3/5] remoteproc: Pass type of shutdown to subdev remove
2017-12-01 14:50 ` Arnaud Pouliquen
@ 2017-12-05 6:46 ` Bjorn Andersson
2017-12-05 10:54 ` Arnaud Pouliquen
0 siblings, 1 reply; 24+ messages in thread
From: Bjorn Andersson @ 2017-12-05 6:46 UTC (permalink / raw)
To: Arnaud Pouliquen
Cc: Andy Gross, Ohad Ben-Cohen, Arun Kumar Neelakantam, Chris Lew,
linux-kernel, linux-arm-msm, linux-soc, linux-remoteproc
On Fri 01 Dec 06:50 PST 2017, Arnaud Pouliquen wrote:
> hello Bjorn,
>
> Sorry for these late remarks/questions
>
No worries, I'm happy to see you reading the patch!
>
> On 11/30/2017 02:16 AM, Bjorn Andersson wrote:
[..]
> > diff --git a/drivers/remoteproc/qcom_common.c b/drivers/remoteproc/qcom_common.c
[..]
> > @@ -785,17 +785,17 @@ static int rproc_probe_subdevices(struct rproc *rproc)
> >
> > unroll_registration:
> > list_for_each_entry_continue_reverse(subdev, &rproc->subdevs, node)
> > - subdev->remove(subdev);
> > + subdev->remove(subdev, false);
> Why do you need to do a non graceful remove in this case? This could
> lead to side effect like memory leakage...
>
Regardless of this being true or false resources should always be
reclaimed.
The reason for introducing this is that the modem in the Qualcomm
platforms implements persistent storage and it's preferred to tell it to
flush the latest data to the storage server (on the Linux side) before
pulling the plug. But in the case of a firmware crash this mechanism
will not be operational and there's no point in attempting this
"graceful shutdown".
[..]
> > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> > index 44e630eb3d94..20a9467744ea 100644
> > --- a/include/linux/remoteproc.h
> > +++ b/include/linux/remoteproc.h
> > @@ -456,7 +456,7 @@ struct rproc_subdev {
> > struct list_head node;
> >
> > int (*probe)(struct rproc_subdev *subdev);
> > - void (*remove)(struct rproc_subdev *subdev);
> > + void (*remove)(struct rproc_subdev *subdev, bool graceful);
> What about adding a new ops instead of a parameter, like a recovery
> callback?
>
I think that for symmetry purposes it should be probe/remove in both
code paths. A possible alternative to the proposal would be to introduce
an operation "request_shutdown()" the would be called in the proposed
graceful code path.
However, in the Qualcomm SMD and GLINK (conceptually equivalent to
virtio-rpmsg) it is possible to open and close communication channels
and it's conceivable to see that the graceful case would close all
channels cleanly while the non-graceful case would just remove the rpmsg
devices (and leave the channel states/memory as is).
In this case a "request_shutdown()" would complicate things, compared to
the boolean.
Regards,
Bjorn
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v4 3/5] remoteproc: Pass type of shutdown to subdev remove
2017-12-05 6:46 ` Bjorn Andersson
@ 2017-12-05 10:54 ` Arnaud Pouliquen
2017-12-05 17:17 ` Bjorn Andersson
0 siblings, 1 reply; 24+ messages in thread
From: Arnaud Pouliquen @ 2017-12-05 10:54 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Andy Gross, Ohad Ben-Cohen, Arun Kumar Neelakantam, Chris Lew,
linux-kernel, linux-arm-msm, linux-soc, linux-remoteproc
On 12/05/2017 07:46 AM, Bjorn Andersson wrote:
> On Fri 01 Dec 06:50 PST 2017, Arnaud Pouliquen wrote:
>
>> hello Bjorn,
>>
>> Sorry for these late remarks/questions
>>
>
> No worries, I'm happy to see you reading the patch!
>
>>
>> On 11/30/2017 02:16 AM, Bjorn Andersson wrote:
> [..]
>>> diff --git a/drivers/remoteproc/qcom_common.c b/drivers/remoteproc/qcom_common.c
> [..]
>>> @@ -785,17 +785,17 @@ static int rproc_probe_subdevices(struct rproc *rproc)
>>>
>>> unroll_registration:
>>> list_for_each_entry_continue_reverse(subdev, &rproc->subdevs, node)
>>> - subdev->remove(subdev);
>>> + subdev->remove(subdev, false);
>> Why do you need to do a non graceful remove in this case? This could
>> lead to side effect like memory leakage...
>>
>
> Regardless of this being true or false resources should always be
> reclaimed.
>
> The reason for introducing this is that the modem in the Qualcomm
> platforms implements persistent storage and it's preferred to tell it to
> flush the latest data to the storage server (on the Linux side) before
> pulling the plug. But in the case of a firmware crash this mechanism
> will not be operational and there's no point in attempting this
> "graceful shutdown".
I understand your usecase for Qualcomm, but in rproc_probe_subdevices
there is not crash of the remote firmware , so remove should be graceful.
>
> [..]
>>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
>>> index 44e630eb3d94..20a9467744ea 100644
>>> --- a/include/linux/remoteproc.h
>>> +++ b/include/linux/remoteproc.h
>>> @@ -456,7 +456,7 @@ struct rproc_subdev {
>>> struct list_head node;
>>>
>>> int (*probe)(struct rproc_subdev *subdev);
>>> - void (*remove)(struct rproc_subdev *subdev);
>>> + void (*remove)(struct rproc_subdev *subdev, bool graceful);
>> What about adding a new ops instead of a parameter, like a recovery
>> callback?
>>
>
> I think that for symmetry purposes it should be probe/remove in both
> code paths. A possible alternative to the proposal would be to introduce
> an operation "request_shutdown()" the would be called in the proposed
> graceful code path.
>
>
> However, in the Qualcomm SMD and GLINK (conceptually equivalent to
> virtio-rpmsg) it is possible to open and close communication channels
> and it's conceivable to see that the graceful case would close all
> channels cleanly while the non-graceful case would just remove the rpmsg
> devices (and leave the channel states/memory as is).
>
> In this case a "request_shutdown()" would complicate things, compared to
> the boolean.
>
I would be more for a specific ops that inform sub-dev on a crash. This
would allow sub-dev to perform specific action (for instance dump) and
store crash information, then on remove, sub_dev would perform specific
action.
This could be something like "trigger_recovery" that is propagated to
the sub-dev.
That would sound more flexible from my point of view.
Regards
Arnaud
> Regards,
> Bjorn
>
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v4 3/5] remoteproc: Pass type of shutdown to subdev remove
2017-12-05 10:54 ` Arnaud Pouliquen
@ 2017-12-05 17:17 ` Bjorn Andersson
2017-12-06 8:55 ` Arnaud Pouliquen
0 siblings, 1 reply; 24+ messages in thread
From: Bjorn Andersson @ 2017-12-05 17:17 UTC (permalink / raw)
To: Arnaud Pouliquen
Cc: Andy Gross, Ohad Ben-Cohen, Arun Kumar Neelakantam, Chris Lew,
linux-kernel, linux-arm-msm, linux-soc, linux-remoteproc
On Tue 05 Dec 02:54 PST 2017, Arnaud Pouliquen wrote:
>
>
> On 12/05/2017 07:46 AM, Bjorn Andersson wrote:
> > On Fri 01 Dec 06:50 PST 2017, Arnaud Pouliquen wrote:
> >
> >> hello Bjorn,
> >>
> >> Sorry for these late remarks/questions
> >>
> >
> > No worries, I'm happy to see you reading the patch!
> >
> >>
> >> On 11/30/2017 02:16 AM, Bjorn Andersson wrote:
> > [..]
> >>> diff --git a/drivers/remoteproc/qcom_common.c b/drivers/remoteproc/qcom_common.c
> > [..]
> >>> @@ -785,17 +785,17 @@ static int rproc_probe_subdevices(struct rproc *rproc)
> >>>
> >>> unroll_registration:
> >>> list_for_each_entry_continue_reverse(subdev, &rproc->subdevs, node)
> >>> - subdev->remove(subdev);
> >>> + subdev->remove(subdev, false);
> >> Why do you need to do a non graceful remove in this case? This could
> >> lead to side effect like memory leakage...
> >>
> >
> > Regardless of this being true or false resources should always be
> > reclaimed.
> >
> > The reason for introducing this is that the modem in the Qualcomm
> > platforms implements persistent storage and it's preferred to tell it to
> > flush the latest data to the storage server (on the Linux side) before
> > pulling the plug. But in the case of a firmware crash this mechanism
> > will not be operational and there's no point in attempting this
> > "graceful shutdown".
> I understand your usecase for Qualcomm, but in rproc_probe_subdevices
> there is not crash of the remote firmware , so remove should be graceful.
>
Now I get your point, sorry. I agree with you, as this is triggering a
clean stop of the system this should be marked "graceful".
Will update, thanks.
> >
> > [..]
> >>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> >>> index 44e630eb3d94..20a9467744ea 100644
> >>> --- a/include/linux/remoteproc.h
> >>> +++ b/include/linux/remoteproc.h
> >>> @@ -456,7 +456,7 @@ struct rproc_subdev {
> >>> struct list_head node;
> >>>
> >>> int (*probe)(struct rproc_subdev *subdev);
> >>> - void (*remove)(struct rproc_subdev *subdev);
> >>> + void (*remove)(struct rproc_subdev *subdev, bool graceful);
> >> What about adding a new ops instead of a parameter, like a recovery
> >> callback?
> >>
> >
> > I think that for symmetry purposes it should be probe/remove in both
> > code paths. A possible alternative to the proposal would be to introduce
> > an operation "request_shutdown()" the would be called in the proposed
> > graceful code path.
> >
> >
> > However, in the Qualcomm SMD and GLINK (conceptually equivalent to
> > virtio-rpmsg) it is possible to open and close communication channels
> > and it's conceivable to see that the graceful case would close all
> > channels cleanly while the non-graceful case would just remove the rpmsg
> > devices (and leave the channel states/memory as is).
> >
> > In this case a "request_shutdown()" would complicate things, compared to
> > the boolean.
> >
> I would be more for a specific ops that inform sub-dev on a crash. This
> would allow sub-dev to perform specific action (for instance dump) and
> store crash information, then on remove, sub_dev would perform specific
> action.
There is a separate discussion (although dormant) on how to gather core
dumps, which should cover these cases.
> This could be something like "trigger_recovery" that is propagated to
> the sub-dev.
>
Right, this step does make sense, but is the opposite of what I need -
i.e. a means to trigger a clean shutdown.
> That would sound more flexible from my point of view.
>
At this point I see this flexibility as unnecessary complexity, if such
need show up (beyond the core dump gathering) we should bring this up
again.
Regards,
Bjorn
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v4 3/5] remoteproc: Pass type of shutdown to subdev remove
2017-12-05 17:17 ` Bjorn Andersson
@ 2017-12-06 8:55 ` Arnaud Pouliquen
2017-12-06 21:53 ` Bjorn Andersson
0 siblings, 1 reply; 24+ messages in thread
From: Arnaud Pouliquen @ 2017-12-06 8:55 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Andy Gross, Ohad Ben-Cohen, Arun Kumar Neelakantam, Chris Lew,
linux-kernel, linux-arm-msm, linux-soc, linux-remoteproc
Hello,
I saw your new version but i 'm answering to this one to continue
discussion.
On 12/05/2017 06:17 PM, Bjorn Andersson wrote:
> On Tue 05 Dec 02:54 PST 2017, Arnaud Pouliquen wrote:
>
>>
>>
>> On 12/05/2017 07:46 AM, Bjorn Andersson wrote:
>>> On Fri 01 Dec 06:50 PST 2017, Arnaud Pouliquen wrote:
>>>
>>>> hello Bjorn,
>>>>
>>>> Sorry for these late remarks/questions
>>>>
>>>
>>> No worries, I'm happy to see you reading the patch!
>>>
>>>>
>>>> On 11/30/2017 02:16 AM, Bjorn Andersson wrote:
>>> [..]
>>>>> diff --git a/drivers/remoteproc/qcom_common.c b/drivers/remoteproc/qcom_common.c
>>> [..]
>>>>> @@ -785,17 +785,17 @@ static int rproc_probe_subdevices(struct rproc *rproc)
>>>>>
>>>>> unroll_registration:
>>>>> list_for_each_entry_continue_reverse(subdev, &rproc->subdevs, node)
>>>>> - subdev->remove(subdev);
>>>>> + subdev->remove(subdev, false);
>>>> Why do you need to do a non graceful remove in this case? This could
>>>> lead to side effect like memory leakage...
>>>>
>>>
>>> Regardless of this being true or false resources should always be
>>> reclaimed.
>>>
>>> The reason for introducing this is that the modem in the Qualcomm
>>> platforms implements persistent storage and it's preferred to tell it to
>>> flush the latest data to the storage server (on the Linux side) before
>>> pulling the plug. But in the case of a firmware crash this mechanism
>>> will not be operational and there's no point in attempting this
>>> "graceful shutdown".
>> I understand your usecase for Qualcomm, but in rproc_probe_subdevices
>> there is not crash of the remote firmware , so remove should be graceful.
>>
>
> Now I get your point, sorry. I agree with you, as this is triggering a
> clean stop of the system this should be marked "graceful".
>
> Will update, thanks.
>
>>>
>>> [..]
>>>>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
>>>>> index 44e630eb3d94..20a9467744ea 100644
>>>>> --- a/include/linux/remoteproc.h
>>>>> +++ b/include/linux/remoteproc.h
>>>>> @@ -456,7 +456,7 @@ struct rproc_subdev {
>>>>> struct list_head node;
>>>>>
>>>>> int (*probe)(struct rproc_subdev *subdev);
>>>>> - void (*remove)(struct rproc_subdev *subdev);
>>>>> + void (*remove)(struct rproc_subdev *subdev, bool graceful);
>>>> What about adding a new ops instead of a parameter, like a recovery
>>>> callback?
>>>>
>>>
>>> I think that for symmetry purposes it should be probe/remove in both
>>> code paths. A possible alternative to the proposal would be to introduce
>>> an operation "request_shutdown()" the would be called in the proposed
>>> graceful code path.
>>>
>>>
>>> However, in the Qualcomm SMD and GLINK (conceptually equivalent to
>>> virtio-rpmsg) it is possible to open and close communication channels
>>> and it's conceivable to see that the graceful case would close all
>>> channels cleanly while the non-graceful case would just remove the rpmsg
>>> devices (and leave the channel states/memory as is).
>>>
>>> In this case a "request_shutdown()" would complicate things, compared to
>>> the boolean.
>>>
>> I would be more for a specific ops that inform sub-dev on a crash. This
>> would allow sub-dev to perform specific action (for instance dump) and
>> store crash information, then on remove, sub_dev would perform specific
>> action.
>
> There is a separate discussion (although dormant) on how to gather core
> dumps, which should cover these cases.
>
>> This could be something like "trigger_recovery" that is propagated to
>> the sub-dev.
>>
>
> Right, this step does make sense, but is the opposite of what I need -
> i.e. a means to trigger a clean shutdown.
Could you clarify this point? i do not see my proposal as the opposite.
In your proposal:
- rproc_trigger_recovery: graceful is set to false
- rproc_shutdown: Graceful is set to true
My proposal is to call an new ops (if defined) before the stop in
rproc_trigger_recovery. If you set a local variable in Qualcomm subdev
drivers this should do the job for your need.
I tried to have a look in Qualcomm part to understand implementation.
but seems that you just add the parameter for time being.
I think that main point that bother me here, is the that the "graceful"
mode should be the normal mode. And in your implementation this look
like the exception mode. Perhaps more a feeling than anything else...but
if you decide to keep argument i would propose to inverse logic using
something like "rproc_crashed" instead of "graceful".
>
>> That would sound more flexible from my point of view.
>>
>
> At this point I see this flexibility as unnecessary complexity, if such
> need show up (beyond the core dump gathering) we should bring this up
> again.
I let you decide what is the best solution.
My concerns is to be sure that your solution is enough generic and not
too Qualcomm platform oriented. As you mentioned in OpenAMP meeting it
is quite difficult to come back on an implementation , especially if it
impacts the API.
Regards
Arnaud
>
> Regards,
> Bjorn
>
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v4 3/5] remoteproc: Pass type of shutdown to subdev remove
2017-12-06 8:55 ` Arnaud Pouliquen
@ 2017-12-06 21:53 ` Bjorn Andersson
2017-12-07 12:14 ` Arnaud Pouliquen
0 siblings, 1 reply; 24+ messages in thread
From: Bjorn Andersson @ 2017-12-06 21:53 UTC (permalink / raw)
To: Arnaud Pouliquen
Cc: Andy Gross, Ohad Ben-Cohen, Arun Kumar Neelakantam, Chris Lew,
linux-kernel, linux-arm-msm, linux-soc, linux-remoteproc
On Wed 06 Dec 00:55 PST 2017, Arnaud Pouliquen wrote:
> Hello,
>
> I saw your new version but i 'm answering to this one to continue
> discussion.
>
That's fine.
> On 12/05/2017 06:17 PM, Bjorn Andersson wrote:
> > On Tue 05 Dec 02:54 PST 2017, Arnaud Pouliquen wrote:
> >> On 12/05/2017 07:46 AM, Bjorn Andersson wrote:
> >>> On Fri 01 Dec 06:50 PST 2017, Arnaud Pouliquen wrote:
> >>>>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> >>>>> index 44e630eb3d94..20a9467744ea 100644
> >>>>> --- a/include/linux/remoteproc.h
> >>>>> +++ b/include/linux/remoteproc.h
> >>>>> @@ -456,7 +456,7 @@ struct rproc_subdev {
> >>>>> struct list_head node;
> >>>>>
> >>>>> int (*probe)(struct rproc_subdev *subdev);
> >>>>> - void (*remove)(struct rproc_subdev *subdev);
> >>>>> + void (*remove)(struct rproc_subdev *subdev, bool graceful);
> >>>> What about adding a new ops instead of a parameter, like a recovery
> >>>> callback?
> >>>>
> >>>
> >>> I think that for symmetry purposes it should be probe/remove in both
> >>> code paths. A possible alternative to the proposal would be to introduce
> >>> an operation "request_shutdown()" the would be called in the proposed
> >>> graceful code path.
> >>>
> >>>
> >>> However, in the Qualcomm SMD and GLINK (conceptually equivalent to
> >>> virtio-rpmsg) it is possible to open and close communication channels
> >>> and it's conceivable to see that the graceful case would close all
> >>> channels cleanly while the non-graceful case would just remove the rpmsg
> >>> devices (and leave the channel states/memory as is).
> >>>
> >>> In this case a "request_shutdown()" would complicate things, compared to
> >>> the boolean.
> >>>
> >> I would be more for a specific ops that inform sub-dev on a crash. This
> >> would allow sub-dev to perform specific action (for instance dump) and
> >> store crash information, then on remove, sub_dev would perform specific
> >> action.
> >
> > There is a separate discussion (although dormant) on how to gather core
> > dumps, which should cover these cases.
> >
> >> This could be something like "trigger_recovery" that is propagated to
> >> the sub-dev.
> >>
> >
> > Right, this step does make sense, but is the opposite of what I need -
> > i.e. a means to trigger a clean shutdown.
> Could you clarify this point? i do not see my proposal as the opposite.
> In your proposal:
> - rproc_trigger_recovery: graceful is set to false
> - rproc_shutdown: Graceful is set to true
>
Correct
> My proposal is to call an new ops (if defined) before the stop in
> rproc_trigger_recovery. If you set a local variable in Qualcomm subdev
> drivers this should do the job for your need.
>
In all use cases that comes to mind the gracefulness makes one step of
the teardown operation kick in/be optional, it's not a separate
operation. I don't see the benefit of enforcing the subdev to keep this
state.
> I tried to have a look in Qualcomm part to understand implementation.
> but seems that you just add the parameter for time being.
>
The following patch, adding sysmon, make use of this. But I have yet to
post patches that affects the SMD and GLINK implementations.
> I think that main point that bother me here, is the that the "graceful"
> mode should be the normal mode. And in your implementation this look
> like the exception mode.
I agree, I consider the recovery path to be the exception, but I'm not
seeing the necessity of making the "true" state of this parameter mean
"yes we have an exception".
Regardless of the value here, the remove() function's purpose is to
clean up resources/state. But in the case of a graceful shutdown (i.e.
not recovery path) the subdevices can be expected to tear things down in
a fashion that permits the remote side to act, so if anything this
"true" would imply that this extra steps should be taken.
> Perhaps more a feeling than anything else...but if you decide to keep
> argument i would propose to inverse logic using something like
> "rproc_crashed" instead of "graceful".
>
The difference between "this is a graceful shutdown, let's inform the
remote" and "this is not a crash, let's inform the remote". The prior
sounds much more to the point in my view.
> >
> >> That would sound more flexible from my point of view.
> >>
> >
> > At this point I see this flexibility as unnecessary complexity, if such
> > need show up (beyond the core dump gathering) we should bring this up
> > again.
>
> I let you decide what is the best solution.
> My concerns is to be sure that your solution is enough generic and not
> too Qualcomm platform oriented.
That's a fair concern. I'm very interested in finding more complex use
cases that requires this type of logic, to see if this is generic
enough.
Note that the discussions that we've had related to e.g. clocks that
should be on during the life of the remote would not fit into the
current subdev life cycle anyways, so this either needs to be
complemented or extended.
> As you mentioned in OpenAMP meeting it is quite difficult to come back
> on an implementation , especially if it impacts the API.
>
No, what I said is that it's impossible to go back once we have changed
the file format, the interface towards the remote or the interface
towards user space. All it takes to change an interface within the
kernel is a single patch.
Regards,
Bjorn
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v4 3/5] remoteproc: Pass type of shutdown to subdev remove
2017-12-06 21:53 ` Bjorn Andersson
@ 2017-12-07 12:14 ` Arnaud Pouliquen
0 siblings, 0 replies; 24+ messages in thread
From: Arnaud Pouliquen @ 2017-12-07 12:14 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Andy Gross, Ohad Ben-Cohen, Arun Kumar Neelakantam, Chris Lew,
linux-kernel, linux-arm-msm, linux-soc, linux-remoteproc
On 12/06/2017 10:53 PM, Bjorn Andersson wrote:
> On Wed 06 Dec 00:55 PST 2017, Arnaud Pouliquen wrote:
>
>> Hello,
>>
>> I saw your new version but i 'm answering to this one to continue
>> discussion.
>>
>
> That's fine.
>
>> On 12/05/2017 06:17 PM, Bjorn Andersson wrote:
>>> On Tue 05 Dec 02:54 PST 2017, Arnaud Pouliquen wrote:
>>>> On 12/05/2017 07:46 AM, Bjorn Andersson wrote:
>>>>> On Fri 01 Dec 06:50 PST 2017, Arnaud Pouliquen wrote:
>>>>>>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
>>>>>>> index 44e630eb3d94..20a9467744ea 100644
>>>>>>> --- a/include/linux/remoteproc.h
>>>>>>> +++ b/include/linux/remoteproc.h
>>>>>>> @@ -456,7 +456,7 @@ struct rproc_subdev {
>>>>>>> struct list_head node;
>>>>>>>
>>>>>>> int (*probe)(struct rproc_subdev *subdev);
>>>>>>> - void (*remove)(struct rproc_subdev *subdev);
>>>>>>> + void (*remove)(struct rproc_subdev *subdev, bool graceful);
>>>>>> What about adding a new ops instead of a parameter, like a recovery
>>>>>> callback?
>>>>>>
>>>>>
>>>>> I think that for symmetry purposes it should be probe/remove in both
>>>>> code paths. A possible alternative to the proposal would be to introduce
>>>>> an operation "request_shutdown()" the would be called in the proposed
>>>>> graceful code path.
>>>>>
>>>>>
>>>>> However, in the Qualcomm SMD and GLINK (conceptually equivalent to
>>>>> virtio-rpmsg) it is possible to open and close communication channels
>>>>> and it's conceivable to see that the graceful case would close all
>>>>> channels cleanly while the non-graceful case would just remove the rpmsg
>>>>> devices (and leave the channel states/memory as is).
>>>>>
>>>>> In this case a "request_shutdown()" would complicate things, compared to
>>>>> the boolean.
>>>>>
>>>> I would be more for a specific ops that inform sub-dev on a crash. This
>>>> would allow sub-dev to perform specific action (for instance dump) and
>>>> store crash information, then on remove, sub_dev would perform specific
>>>> action.
>>>
>>> There is a separate discussion (although dormant) on how to gather core
>>> dumps, which should cover these cases.
>>>
>>>> This could be something like "trigger_recovery" that is propagated to
>>>> the sub-dev.
>>>>
>>>
>>> Right, this step does make sense, but is the opposite of what I need -
>>> i.e. a means to trigger a clean shutdown.
>> Could you clarify this point? i do not see my proposal as the opposite.
>> In your proposal:
>> - rproc_trigger_recovery: graceful is set to false
>> - rproc_shutdown: Graceful is set to true
>>
>
> Correct
>
>> My proposal is to call an new ops (if defined) before the stop in
>> rproc_trigger_recovery. If you set a local variable in Qualcomm subdev
>> drivers this should do the job for your need.
>>
>
> In all use cases that comes to mind the gracefulness makes one step of
> the teardown operation kick in/be optional, it's not a separate
> operation. I don't see the benefit of enforcing the subdev to keep this
> state.
>
>> I tried to have a look in Qualcomm part to understand implementation.
>> but seems that you just add the parameter for time being.
>>
>
> The following patch, adding sysmon, make use of this. But I have yet to
> post patches that affects the SMD and GLINK implementations.
>
>> I think that main point that bother me here, is the that the "graceful"
>> mode should be the normal mode. And in your implementation this look
>> like the exception mode.
>
> I agree, I consider the recovery path to be the exception, but I'm not
> seeing the necessity of making the "true" state of this parameter mean
> "yes we have an exception".
>
> Regardless of the value here, the remove() function's purpose is to
> clean up resources/state. But in the case of a graceful shutdown (i.e.
> not recovery path) the subdevices can be expected to tear things down in
> a fashion that permits the remote side to act, so if anything this
> "true" would imply that this extra steps should be taken.
>
>> Perhaps more a feeling than anything else...but if you decide to keep
>> argument i would propose to inverse logic using something like
>> "rproc_crashed" instead of "graceful".
>>
>
> The difference between "this is a graceful shutdown, let's inform the
> remote" and "this is not a crash, let's inform the remote". The prior
> sounds much more to the point in my view.
On the other hand, i consider "graceful" as a normal mode that is
implemented in kernel. By informing sub-dev that "this is a graceful
shutdown", you just precise a behavior that is already implemented by
default.
And in case of recovery, you inform sub-dev that "this is not a graceful
shutdown". What does it means "this is not a graceful shutdown"?...This
is confusing, from my point of view.
for this reason, I still don't like the "graceful" wording too much, but
this is a personal feeling, not a strong argument. :)
So, if you are not convince by my last argument and nobody else
comments, consider it as OK for me.
>
>>>
>>>> That would sound more flexible from my point of view.
>>>>
>>>
>>> At this point I see this flexibility as unnecessary complexity, if such
>>> need show up (beyond the core dump gathering) we should bring this up
>>> again.
>>
>> I let you decide what is the best solution.
>> My concerns is to be sure that your solution is enough generic and not
>> too Qualcomm platform oriented.
>
> That's a fair concern. I'm very interested in finding more complex use
> cases that requires this type of logic, to see if this is generic
> enough.
>
> Note that the discussions that we've had related to e.g. clocks that
> should be on during the life of the remote would not fit into the
> current subdev life cycle anyways, so this either needs to be
> complemented or extended.
>
Today the only other use case, i would have in mind (except dump) is the
management of a "hot" restart on a crash... but no concrete example.
>> As you mentioned in OpenAMP meeting it is quite difficult to come back
>> on an implementation , especially if it impacts the API.
>>
>
> No, what I said is that it's impossible to go back once we have changed
> the file format, the interface towards the remote or the interface
> towards user space. All it takes to change an interface within the
> kernel is a single patch.
>
Thanks for this clarification.
Regards,
Arnaud
> Regards,
> Bjorn
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v4 4/5] remoteproc: qcom: Introduce sysmon
2017-11-30 1:16 [PATCH v4 0/5] In-kernel QMI helpers and sysmon Bjorn Andersson
` (2 preceding siblings ...)
2017-11-30 1:16 ` [PATCH v4 3/5] remoteproc: Pass type of shutdown to subdev remove Bjorn Andersson
@ 2017-11-30 1:16 ` Bjorn Andersson
2017-12-01 1:52 ` Chris Lew
2017-11-30 1:16 ` [PATCH v4 5/5] samples: Introduce Qualcomm QMI sample client Bjorn Andersson
4 siblings, 1 reply; 24+ messages in thread
From: Bjorn Andersson @ 2017-11-30 1:16 UTC (permalink / raw)
To: Andy Gross, Ohad Ben-Cohen, Bjorn Andersson
Cc: Arun Kumar Neelakantam, Chris Lew, linux-kernel, linux-arm-msm,
linux-soc, linux-remoteproc
The sysmon client communicates either via a dedicated SMD/GLINK channel
or via QMI encoded messages over IPCROUTER with remote processors in
order to perform graceful shutdown and inform about other remote
processors shutting down.
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
Changes since v3:
- Kerneldoc updates
- Style fixes
Changes since v2:
- Checkpatch fixes
Changes since v1:
- New patch
drivers/remoteproc/Kconfig | 17 ++
drivers/remoteproc/Makefile | 1 +
drivers/remoteproc/qcom_adsp_pil.c | 12 +
drivers/remoteproc/qcom_common.h | 21 ++
drivers/remoteproc/qcom_q6v5_pil.c | 3 +
drivers/remoteproc/qcom_sysmon.c | 587 +++++++++++++++++++++++++++++++++++++
drivers/remoteproc/qcom_wcnss.c | 4 +
7 files changed, 645 insertions(+)
create mode 100644 drivers/remoteproc/qcom_sysmon.c
diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index b609e1d3654b..6556df21318e 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -90,6 +90,7 @@ config QCOM_ADSP_PIL
depends on QCOM_SMEM
depends on RPMSG_QCOM_SMD || (COMPILE_TEST && RPMSG_QCOM_SMD=n)
depends on RPMSG_QCOM_GLINK_SMEM || RPMSG_QCOM_GLINK_SMEM=n
+ depends on QCOM_SYSMON || QCOM_SYSMON=n
select MFD_SYSCON
select QCOM_MDT_LOADER
select QCOM_RPROC_COMMON
@@ -107,6 +108,7 @@ config QCOM_Q6V5_PIL
depends on QCOM_SMEM
depends on RPMSG_QCOM_SMD || (COMPILE_TEST && RPMSG_QCOM_SMD=n)
depends on RPMSG_QCOM_GLINK_SMEM || RPMSG_QCOM_GLINK_SMEM=n
+ depends on QCOM_SYSMON || QCOM_SYSMON=n
select MFD_SYSCON
select QCOM_RPROC_COMMON
select QCOM_SCM
@@ -114,12 +116,27 @@ config QCOM_Q6V5_PIL
Say y here to support the Qualcomm Peripherial Image Loader for the
Hexagon V5 based remote processors.
+config QCOM_SYSMON
+ tristate "Qualcomm sysmon driver"
+ depends on RPMSG
+ depends on ARCH_QCOM
+ select QCOM_QMI_HELPERS
+ help
+ The sysmon driver implements a sysmon QMI client and a handler for
+ the sys_mon SMD and GLINK channel, which are used for graceful
+ shutdown, retrieving failure information and propagating information
+ about other subsystems being shut down.
+
+ Say y here if your system runs firmware on any other subsystems, e.g.
+ modem or DSP.
+
config QCOM_WCNSS_PIL
tristate "Qualcomm WCNSS Peripheral Image Loader"
depends on OF && ARCH_QCOM
depends on RPMSG_QCOM_SMD || (COMPILE_TEST && RPMSG_QCOM_SMD=n)
depends on RPMSG_QCOM_GLINK_SMEM || RPMSG_QCOM_GLINK_SMEM=n
depends on QCOM_SMEM
+ depends on QCOM_SYSMON || QCOM_SYSMON=n
select QCOM_MDT_LOADER
select QCOM_RPROC_COMMON
select QCOM_SCM
diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
index 6e16450ce11f..02627ede8d4a 100644
--- a/drivers/remoteproc/Makefile
+++ b/drivers/remoteproc/Makefile
@@ -17,6 +17,7 @@ obj-$(CONFIG_KEYSTONE_REMOTEPROC) += keystone_remoteproc.o
obj-$(CONFIG_QCOM_ADSP_PIL) += qcom_adsp_pil.o
obj-$(CONFIG_QCOM_RPROC_COMMON) += qcom_common.o
obj-$(CONFIG_QCOM_Q6V5_PIL) += qcom_q6v5_pil.o
+obj-$(CONFIG_QCOM_SYSMON) += qcom_sysmon.o
obj-$(CONFIG_QCOM_WCNSS_PIL) += qcom_wcnss_pil.o
qcom_wcnss_pil-y += qcom_wcnss.o
qcom_wcnss_pil-y += qcom_wcnss_iris.o
diff --git a/drivers/remoteproc/qcom_adsp_pil.c b/drivers/remoteproc/qcom_adsp_pil.c
index 3f6af54dbc96..45e7e66604d4 100644
--- a/drivers/remoteproc/qcom_adsp_pil.c
+++ b/drivers/remoteproc/qcom_adsp_pil.c
@@ -38,7 +38,10 @@ struct adsp_data {
const char *firmware_name;
int pas_id;
bool has_aggre2_clk;
+
const char *ssr_name;
+ const char *sysmon_name;
+ int ssctl_id;
};
struct qcom_adsp {
@@ -75,6 +78,7 @@ struct qcom_adsp {
struct qcom_rproc_glink glink_subdev;
struct qcom_rproc_subdev smd_subdev;
struct qcom_rproc_ssr ssr_subdev;
+ struct qcom_sysmon *sysmon;
};
static int adsp_load(struct rproc *rproc, const struct firmware *fw)
@@ -404,6 +408,9 @@ static int adsp_probe(struct platform_device *pdev)
qcom_add_glink_subdev(rproc, &adsp->glink_subdev);
qcom_add_smd_subdev(rproc, &adsp->smd_subdev);
qcom_add_ssr_subdev(rproc, &adsp->ssr_subdev, desc->ssr_name);
+ adsp->sysmon = qcom_add_sysmon_subdev(rproc,
+ desc->sysmon_name,
+ desc->ssctl_id);
ret = rproc_add(rproc);
if (ret)
@@ -425,6 +432,7 @@ static int adsp_remove(struct platform_device *pdev)
rproc_del(adsp->rproc);
qcom_remove_glink_subdev(adsp->rproc, &adsp->glink_subdev);
+ qcom_remove_sysmon_subdev(adsp->sysmon);
qcom_remove_smd_subdev(adsp->rproc, &adsp->smd_subdev);
qcom_remove_ssr_subdev(adsp->rproc, &adsp->ssr_subdev);
rproc_free(adsp->rproc);
@@ -438,6 +446,8 @@ static const struct adsp_data adsp_resource_init = {
.pas_id = 1,
.has_aggre2_clk = false,
.ssr_name = "lpass",
+ .sysmon_name = "adsp",
+ .ssctl_id = 0x14,
};
static const struct adsp_data slpi_resource_init = {
@@ -446,6 +456,8 @@ static const struct adsp_data slpi_resource_init = {
.pas_id = 12,
.has_aggre2_clk = true,
.ssr_name = "dsps",
+ .sysmon_name = "slpi",
+ .ssctl_id = 0x16,
};
static const struct of_device_id adsp_of_match[] = {
diff --git a/drivers/remoteproc/qcom_common.h b/drivers/remoteproc/qcom_common.h
index 832e20271664..541586e528b3 100644
--- a/drivers/remoteproc/qcom_common.h
+++ b/drivers/remoteproc/qcom_common.h
@@ -4,6 +4,9 @@
#include <linux/remoteproc.h>
#include "remoteproc_internal.h"
+#include <linux/soc/qcom/qmi.h>
+
+struct qcom_sysmon;
struct qcom_rproc_glink {
struct rproc_subdev subdev;
@@ -41,4 +44,22 @@ void qcom_add_ssr_subdev(struct rproc *rproc, struct qcom_rproc_ssr *ssr,
const char *ssr_name);
void qcom_remove_ssr_subdev(struct rproc *rproc, struct qcom_rproc_ssr *ssr);
+#if IS_ENABLED(CONFIG_QCOM_SYSMON)
+struct qcom_sysmon *qcom_add_sysmon_subdev(struct rproc *rproc,
+ const char *name,
+ int ssctl_instance);
+void qcom_remove_sysmon_subdev(struct qcom_sysmon *sysmon);
+#else
+static inline struct qcom_sysmon *qcom_add_sysmon_subdev(struct rproc *rproc,
+ const char *name,
+ int ssctl_instance)
+{
+ return NULL;
+}
+
+static inline void qcom_remove_sysmon_subdev(struct qcom_sysmon *sysmon)
+{
+}
+#endif
+
#endif
diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
index 8a3fa2bcc9f6..342d5eb5d6ed 100644
--- a/drivers/remoteproc/qcom_q6v5_pil.c
+++ b/drivers/remoteproc/qcom_q6v5_pil.c
@@ -168,6 +168,7 @@ struct q6v5 {
struct qcom_rproc_subdev smd_subdev;
struct qcom_rproc_ssr ssr_subdev;
+ struct qcom_sysmon *sysmon;
bool need_mem_protection;
int mpss_perm;
int mba_perm;
@@ -1231,6 +1232,7 @@ static int q6v5_probe(struct platform_device *pdev)
qproc->mba_perm = BIT(QCOM_SCM_VMID_HLOS);
qcom_add_smd_subdev(rproc, &qproc->smd_subdev);
qcom_add_ssr_subdev(rproc, &qproc->ssr_subdev, "mpss");
+ qproc->sysmon = qcom_add_sysmon_subdev(rproc, "modem", 0x12);
ret = rproc_add(rproc);
if (ret)
@@ -1250,6 +1252,7 @@ static int q6v5_remove(struct platform_device *pdev)
rproc_del(qproc->rproc);
+ qcom_remove_sysmon_subdev(qproc->sysmon);
qcom_remove_smd_subdev(qproc->rproc, &qproc->smd_subdev);
qcom_remove_ssr_subdev(qproc->rproc, &qproc->ssr_subdev);
rproc_free(qproc->rproc);
diff --git a/drivers/remoteproc/qcom_sysmon.c b/drivers/remoteproc/qcom_sysmon.c
new file mode 100644
index 000000000000..4bfc5c422f3d
--- /dev/null
+++ b/drivers/remoteproc/qcom_sysmon.c
@@ -0,0 +1,587 @@
+/*
+ * Copyright (c) 2017, Linaro Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * 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/firmware.h>
+#include <linux/module.h>
+#include <linux/notifier.h>
+#include <linux/slab.h>
+#include <linux/io.h>
+#include <linux/notifier.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/remoteproc/qcom_rproc.h>
+#include <linux/rpmsg.h>
+
+#include "qcom_common.h"
+
+static BLOCKING_NOTIFIER_HEAD(sysmon_notifiers);
+
+struct qcom_sysmon {
+ struct rproc_subdev subdev;
+ struct rproc *rproc;
+
+ struct list_head node;
+
+ const char *name;
+
+ int ssctl_version;
+ int ssctl_instance;
+
+ struct notifier_block nb;
+
+ struct device *dev;
+
+ struct rpmsg_endpoint *ept;
+ struct completion comp;
+ struct mutex lock;
+
+ bool ssr_ack;
+
+ struct qmi_handle qmi;
+ struct sockaddr_qrtr ssctl;
+};
+
+static DEFINE_MUTEX(sysmon_lock);
+static LIST_HEAD(sysmon_list);
+
+/**
+ * sysmon_send_event() - send notification of other remote's SSR event
+ * @sysmon: sysmon context
+ * @name: other remote's name
+ */
+static void sysmon_send_event(struct qcom_sysmon *sysmon, const char *name)
+{
+ char req[50];
+ int len;
+ int ret;
+
+ len = snprintf(req, sizeof(req), "ssr:%s:before_shutdown", name);
+ if (len >= sizeof(req))
+ return;
+
+ mutex_lock(&sysmon->lock);
+ reinit_completion(&sysmon->comp);
+ sysmon->ssr_ack = false;
+
+ ret = rpmsg_send(sysmon->ept, req, len);
+ if (ret < 0) {
+ dev_err(sysmon->dev, "failed to send sysmon event\n");
+ goto out_unlock;
+ }
+
+ ret = wait_for_completion_timeout(&sysmon->comp,
+ msecs_to_jiffies(5000));
+ if (!ret) {
+ dev_err(sysmon->dev, "timeout waiting for sysmon ack\n");
+ goto out_unlock;
+ }
+
+ if (!sysmon->ssr_ack)
+ dev_err(sysmon->dev, "unexpected response to sysmon event\n");
+
+out_unlock:
+ mutex_unlock(&sysmon->lock);
+}
+
+/**
+ * sysmon_request_shutdown() - request graceful shutdown of remote
+ * @sysmon: sysmon context
+ */
+static void sysmon_request_shutdown(struct qcom_sysmon *sysmon)
+{
+ char *req = "ssr:shutdown";
+ int ret;
+
+ mutex_lock(&sysmon->lock);
+ reinit_completion(&sysmon->comp);
+ sysmon->ssr_ack = false;
+
+ ret = rpmsg_send(sysmon->ept, req, strlen(req) + 1);
+ if (ret < 0) {
+ dev_err(sysmon->dev, "send sysmon shutdown request failed\n");
+ goto out_unlock;
+ }
+
+ ret = wait_for_completion_timeout(&sysmon->comp,
+ msecs_to_jiffies(5000));
+ if (!ret) {
+ dev_err(sysmon->dev, "timeout waiting for sysmon ack\n");
+ goto out_unlock;
+ }
+
+ if (!sysmon->ssr_ack)
+ dev_err(sysmon->dev,
+ "unexpected response to sysmon shutdown request\n");
+
+out_unlock:
+ mutex_unlock(&sysmon->lock);
+}
+
+static int sysmon_callback(struct rpmsg_device *rpdev, void *data, int count,
+ void *priv, u32 addr)
+{
+ struct qcom_sysmon *sysmon = priv;
+ const char *ssr_ack = "ssr:ack";
+ const int ssr_ack_len = strlen(ssr_ack) + 1;
+
+ if (!sysmon)
+ return -EINVAL;
+
+ if (count >= ssr_ack_len && !memcmp(data, ssr_ack, ssr_ack_len))
+ sysmon->ssr_ack = true;
+
+ complete(&sysmon->comp);
+
+ return 0;
+}
+
+#define SSCTL_SHUTDOWN_REQ 0x21
+#define SSCTL_SUBSYS_EVENT_REQ 0x23
+
+#define SSCTL_MAX_MSG_LEN 7
+
+#define SSCTL_SUBSYS_NAME_LENGTH 15
+
+enum {
+ SSCTL_SSR_EVENT_BEFORE_POWERUP,
+ SSCTL_SSR_EVENT_AFTER_POWERUP,
+ SSCTL_SSR_EVENT_BEFORE_SHUTDOWN,
+ SSCTL_SSR_EVENT_AFTER_SHUTDOWN,
+};
+
+enum {
+ SSCTL_SSR_EVENT_FORCED,
+ SSCTL_SSR_EVENT_GRACEFUL,
+};
+
+struct ssctl_shutdown_resp {
+ struct qmi_response_type_v01 resp;
+};
+
+static struct qmi_elem_info ssctl_shutdown_resp_ei[] = {
+ {
+ .data_type = QMI_STRUCT,
+ .elem_len = 1,
+ .elem_size = sizeof(struct qmi_response_type_v01),
+ .array_type = NO_ARRAY,
+ .tlv_type = 0x02,
+ .offset = offsetof(struct ssctl_shutdown_resp, resp),
+ .ei_array = qmi_response_type_v01_ei,
+ },
+ {}
+};
+
+struct ssctl_subsys_event_req {
+ u8 subsys_name_len;
+ char subsys_name[SSCTL_SUBSYS_NAME_LENGTH];
+ u32 event;
+ u8 evt_driven_valid;
+ u32 evt_driven;
+};
+
+static struct qmi_elem_info ssctl_subsys_event_req_ei[] = {
+ {
+ .data_type = QMI_DATA_LEN,
+ .elem_len = 1,
+ .elem_size = sizeof(uint8_t),
+ .array_type = NO_ARRAY,
+ .tlv_type = 0x01,
+ .offset = offsetof(struct ssctl_subsys_event_req,
+ subsys_name_len),
+ .ei_array = NULL,
+ },
+ {
+ .data_type = QMI_UNSIGNED_1_BYTE,
+ .elem_len = SSCTL_SUBSYS_NAME_LENGTH,
+ .elem_size = sizeof(char),
+ .array_type = VAR_LEN_ARRAY,
+ .tlv_type = 0x01,
+ .offset = offsetof(struct ssctl_subsys_event_req,
+ subsys_name),
+ .ei_array = NULL,
+ },
+ {
+ .data_type = QMI_SIGNED_4_BYTE_ENUM,
+ .elem_len = 1,
+ .elem_size = sizeof(uint32_t),
+ .array_type = NO_ARRAY,
+ .tlv_type = 0x02,
+ .offset = offsetof(struct ssctl_subsys_event_req,
+ event),
+ .ei_array = NULL,
+ },
+ {
+ .data_type = QMI_OPT_FLAG,
+ .elem_len = 1,
+ .elem_size = sizeof(uint8_t),
+ .array_type = NO_ARRAY,
+ .tlv_type = 0x10,
+ .offset = offsetof(struct ssctl_subsys_event_req,
+ evt_driven_valid),
+ .ei_array = NULL,
+ },
+ {
+ .data_type = QMI_SIGNED_4_BYTE_ENUM,
+ .elem_len = 1,
+ .elem_size = sizeof(uint32_t),
+ .array_type = NO_ARRAY,
+ .tlv_type = 0x10,
+ .offset = offsetof(struct ssctl_subsys_event_req,
+ evt_driven),
+ .ei_array = NULL,
+ },
+ {}
+};
+
+struct ssctl_subsys_event_resp {
+ struct qmi_response_type_v01 resp;
+};
+
+static struct qmi_elem_info ssctl_subsys_event_resp_ei[] = {
+ {
+ .data_type = QMI_STRUCT,
+ .elem_len = 1,
+ .elem_size = sizeof(struct qmi_response_type_v01),
+ .array_type = NO_ARRAY,
+ .tlv_type = 0x02,
+ .offset = offsetof(struct ssctl_subsys_event_resp,
+ resp),
+ .ei_array = qmi_response_type_v01_ei,
+ },
+ {}
+};
+
+/**
+ * ssctl_request_shutdown() - request shutdown via SSCTL QMI service
+ * @sysmon: sysmon context
+ */
+static void ssctl_request_shutdown(struct qcom_sysmon *sysmon)
+{
+ struct ssctl_shutdown_resp resp;
+ struct qmi_txn txn;
+ int ret;
+
+ ret = qmi_txn_init(&sysmon->qmi, &txn, ssctl_shutdown_resp_ei, &resp);
+ if (ret < 0) {
+ dev_err(sysmon->dev, "failed to allocate QMI txn\n");
+ return;
+ }
+
+ ret = qmi_send_request(&sysmon->qmi, &sysmon->ssctl, &txn,
+ SSCTL_SHUTDOWN_REQ, 0, NULL, NULL);
+ if (ret < 0) {
+ dev_err(sysmon->dev, "failed to send shutdown request\n");
+ qmi_txn_cancel(&txn);
+ return;
+ }
+
+ ret = qmi_txn_wait(&txn, 5 * HZ);
+ if (ret < 0)
+ dev_err(sysmon->dev, "failed receiving QMI response\n");
+ else if (resp.resp.result)
+ dev_err(sysmon->dev, "shutdown request failed\n");
+ else
+ dev_dbg(sysmon->dev, "shutdown request completed\n");
+}
+
+/**
+ * ssctl_send_event() - send notification of other remote's SSR event
+ * @sysmon: sysmon context
+ * @name: other remote's name
+ */
+static void ssctl_send_event(struct qcom_sysmon *sysmon, const char *name)
+{
+ struct ssctl_subsys_event_resp resp;
+ struct ssctl_subsys_event_req req;
+ struct qmi_txn txn;
+ int ret;
+
+ memset(&resp, 0, sizeof(resp));
+ ret = qmi_txn_init(&sysmon->qmi, &txn, ssctl_subsys_event_resp_ei, &resp);
+ if (ret < 0) {
+ dev_err(sysmon->dev, "failed to allocate QMI txn\n");
+ return;
+ }
+
+ memset(&req, 0, sizeof(req));
+ strlcpy(req.subsys_name, name, sizeof(req.subsys_name));
+ req.subsys_name_len = strlen(req.subsys_name);
+ req.event = SSCTL_SSR_EVENT_BEFORE_SHUTDOWN;
+ req.evt_driven_valid = true;
+ req.evt_driven = SSCTL_SSR_EVENT_FORCED;
+
+ ret = qmi_send_request(&sysmon->qmi, &sysmon->ssctl, &txn,
+ SSCTL_SUBSYS_EVENT_REQ, 40,
+ ssctl_subsys_event_req_ei, &req);
+ if (ret < 0) {
+ dev_err(sysmon->dev, "failed to send shutdown request\n");
+ qmi_txn_cancel(&txn);
+ return;
+ }
+
+ ret = qmi_txn_wait(&txn, 5 * HZ);
+ if (ret < 0)
+ dev_err(sysmon->dev, "failed receiving QMI response\n");
+ else if (resp.resp.result)
+ dev_err(sysmon->dev, "ssr event send failed\n");
+ else
+ dev_dbg(sysmon->dev, "ssr event send completed\n");
+}
+
+/**
+ * ssctl_new_server() - QMI callback indicating a new service
+ * @qmi: QMI handle
+ * @svc: service information
+ *
+ * Return: 0 if we're interested in this service, -EINVAL otherwise.
+ */
+static int ssctl_new_server(struct qmi_handle *qmi, struct qmi_service *svc)
+{
+ struct qcom_sysmon *sysmon = container_of(qmi, struct qcom_sysmon, qmi);
+
+ switch (svc->version) {
+ case 1:
+ if (svc->instance != 0)
+ return -EINVAL;
+ if (strcmp(sysmon->name, "modem"))
+ return -EINVAL;
+ break;
+ case 2:
+ if (svc->instance != sysmon->ssctl_instance)
+ return -EINVAL;
+ break;
+ default:
+ return -EINVAL;
+ };
+
+ sysmon->ssctl_version = svc->version;
+
+ sysmon->ssctl.sq_family = AF_QIPCRTR;
+ sysmon->ssctl.sq_node = svc->node;
+ sysmon->ssctl.sq_port = svc->port;
+
+ svc->priv = sysmon;
+
+ return 0;
+}
+
+/**
+ * ssctl_del_server() - QMI callback indicating that @svc is removed
+ * @qmi: QMI handle
+ * @svc: service information
+ */
+static void ssctl_del_server(struct qmi_handle *qmi, struct qmi_service *svc)
+{
+ struct qcom_sysmon *sysmon = svc->priv;
+
+ sysmon->ssctl_version = 0;
+}
+
+static const struct qmi_ops ssctl_ops = {
+ .new_server = ssctl_new_server,
+ .del_server = ssctl_del_server,
+};
+
+static int sysmon_start(struct rproc_subdev *subdev)
+{
+ return 0;
+}
+
+static void sysmon_stop(struct rproc_subdev *subdev, bool graceful)
+{
+ struct qcom_sysmon *sysmon = container_of(subdev, struct qcom_sysmon, subdev);
+
+ blocking_notifier_call_chain(&sysmon_notifiers, 0, (void *)sysmon->name);
+
+ /* Don't request graceful shutdown if we've crashed */
+ if (!graceful)
+ return;
+
+ if (sysmon->ssctl_version)
+ ssctl_request_shutdown(sysmon);
+ else if (sysmon->ept)
+ sysmon_request_shutdown(sysmon);
+}
+
+/**
+ * sysmon_notify() - notify sysmon target of another's SSR
+ * @nb: notifier_block associated with sysmon instance
+ * @event: unused
+ * @data: SSR identifier of the remote that is going down
+ */
+static int sysmon_notify(struct notifier_block *nb, unsigned long event,
+ void *data)
+{
+ struct qcom_sysmon *sysmon = container_of(nb, struct qcom_sysmon, nb);
+ struct rproc *rproc = sysmon->rproc;
+ const char *ssr_name = data;
+
+ /* Skip non-running rprocs and the originating instance */
+ if (rproc->state != RPROC_RUNNING || !strcmp(data, sysmon->name)) {
+ dev_dbg(sysmon->dev, "not notifying %s\n", sysmon->name);
+ return NOTIFY_DONE;
+ }
+
+ /* Only SSCTL version 2 supports SSR events */
+ if (sysmon->ssctl_version == 2)
+ ssctl_send_event(sysmon, ssr_name);
+ else if (sysmon->ept)
+ sysmon_send_event(sysmon, ssr_name);
+
+ return NOTIFY_DONE;
+}
+
+/**
+ * qcom_add_sysmon_subdev() - create a sysmon subdev for the given remoteproc
+ * @rproc: rproc context to associate the subdev with
+ * @name: name of this subdev, to use in SSR
+ * @ssctl_instance: instance id of the ssctl QMI service
+ *
+ * Return: A new qcom_sysmon object, or NULL on failure
+ */
+struct qcom_sysmon *qcom_add_sysmon_subdev(struct rproc *rproc,
+ const char *name,
+ int ssctl_instance)
+{
+ struct qcom_sysmon *sysmon;
+ int ret;
+
+ sysmon = kzalloc(sizeof(*sysmon), GFP_KERNEL);
+ if (!sysmon)
+ return NULL;
+
+ sysmon->dev = rproc->dev.parent;
+ sysmon->rproc = rproc;
+
+ sysmon->name = name;
+ sysmon->ssctl_instance = ssctl_instance;
+
+ init_completion(&sysmon->comp);
+ mutex_init(&sysmon->lock);
+
+ ret = qmi_handle_init(&sysmon->qmi, SSCTL_MAX_MSG_LEN, &ssctl_ops, NULL);
+ if (ret < 0) {
+ dev_err(sysmon->dev, "failed to initialize qmi handle\n");
+ kfree(sysmon);
+ return NULL;
+ }
+
+ qmi_add_lookup(&sysmon->qmi, 43, 0, 0);
+
+ rproc_add_subdev(rproc, &sysmon->subdev, sysmon_start, sysmon_stop);
+
+ sysmon->nb.notifier_call = sysmon_notify;
+ blocking_notifier_chain_register(&sysmon_notifiers, &sysmon->nb);
+
+ mutex_lock(&sysmon_lock);
+ list_add(&sysmon->node, &sysmon_list);
+ mutex_unlock(&sysmon_lock);
+
+ return sysmon;
+}
+EXPORT_SYMBOL_GPL(qcom_add_sysmon_subdev);
+
+/**
+ * qcom_remove_sysmon_subdev() - release a qcom_sysmon
+ * @sysmon: sysmon context, as retrieved by qcom_add_sysmon_subdev()
+ */
+void qcom_remove_sysmon_subdev(struct qcom_sysmon *sysmon)
+{
+ if (!sysmon)
+ return;
+
+ mutex_lock(&sysmon_lock);
+ list_del(&sysmon->node);
+ mutex_unlock(&sysmon_lock);
+
+ blocking_notifier_chain_unregister(&sysmon_notifiers, &sysmon->nb);
+
+ rproc_remove_subdev(sysmon->rproc, &sysmon->subdev);
+
+ qmi_handle_release(&sysmon->qmi);
+
+ kfree(sysmon);
+}
+EXPORT_SYMBOL_GPL(qcom_remove_sysmon_subdev);
+
+/**
+ * sysmon_probe() - probe sys_mon channel
+ * @rpdev: rpmsg device handle
+ *
+ * Find the sysmon context associated with the ancestor remoteproc and assign
+ * this rpmsg device with said sysmon context.
+ *
+ * Return: 0 on success, negative errno on failure.
+ */
+static int sysmon_probe(struct rpmsg_device *rpdev)
+{
+ struct qcom_sysmon *sysmon;
+ struct rproc *rproc;
+
+ rproc = rproc_get_by_child(&rpdev->dev);
+ if (!rproc) {
+ dev_err(&rpdev->dev, "sysmon device not child of rproc\n");
+ return -EINVAL;
+ }
+
+ mutex_lock(&sysmon_lock);
+ list_for_each_entry(sysmon, &sysmon_list, node) {
+ if (sysmon->rproc == rproc)
+ goto found;
+ }
+ mutex_unlock(&sysmon_lock);
+
+ dev_err(&rpdev->dev, "no sysmon associated with parent rproc\n");
+
+ return -EINVAL;
+
+found:
+ mutex_unlock(&sysmon_lock);
+
+ rpdev->ept->priv = sysmon;
+ sysmon->ept = rpdev->ept;
+
+ return 0;
+}
+
+/**
+ * sysmon_remove() - sys_mon channel remove handler
+ * @rpdev: rpmsg device handle
+ *
+ * Disassociate the rpmsg device with the sysmon instance.
+ */
+static void sysmon_remove(struct rpmsg_device *rpdev)
+{
+ struct qcom_sysmon *sysmon = rpdev->ept->priv;
+
+ sysmon->ept = NULL;
+}
+
+static const struct rpmsg_device_id sysmon_match[] = {
+ { "sys_mon" },
+ {}
+};
+
+static struct rpmsg_driver sysmon_driver = {
+ .probe = sysmon_probe,
+ .remove = sysmon_remove,
+ .callback = sysmon_callback,
+ .id_table = sysmon_match,
+ .drv = {
+ .name = "qcom_sysmon",
+ },
+};
+
+module_rpmsg_driver(sysmon_driver);
+
+MODULE_DESCRIPTION("Qualcomm sysmon driver");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/remoteproc/qcom_wcnss.c b/drivers/remoteproc/qcom_wcnss.c
index c7686393d505..dc79239c43f9 100644
--- a/drivers/remoteproc/qcom_wcnss.c
+++ b/drivers/remoteproc/qcom_wcnss.c
@@ -40,6 +40,7 @@
#define WCNSS_CRASH_REASON_SMEM 422
#define WCNSS_FIRMWARE_NAME "wcnss.mdt"
#define WCNSS_PAS_ID 6
+#define WCNSS_SSCTL_ID 0x13
#define WCNSS_SPARE_NVBIN_DLND BIT(25)
@@ -98,6 +99,7 @@ struct qcom_wcnss {
size_t mem_size;
struct qcom_rproc_subdev smd_subdev;
+ struct qcom_sysmon *sysmon;
};
static const struct wcnss_data riva_data = {
@@ -557,6 +559,7 @@ static int wcnss_probe(struct platform_device *pdev)
}
qcom_add_smd_subdev(rproc, &wcnss->smd_subdev);
+ wcnss->sysmon = qcom_add_sysmon_subdev(rproc, "wcnss", WCNSS_SSCTL_ID);
ret = rproc_add(rproc);
if (ret)
@@ -579,6 +582,7 @@ static int wcnss_remove(struct platform_device *pdev)
qcom_smem_state_put(wcnss->state);
rproc_del(wcnss->rproc);
+ qcom_remove_sysmon_subdev(wcnss->sysmon);
qcom_remove_smd_subdev(wcnss->rproc, &wcnss->smd_subdev);
rproc_free(wcnss->rproc);
--
2.15.0
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH v4 4/5] remoteproc: qcom: Introduce sysmon
2017-11-30 1:16 ` [PATCH v4 4/5] remoteproc: qcom: Introduce sysmon Bjorn Andersson
@ 2017-12-01 1:52 ` Chris Lew
2017-12-01 5:31 ` Bjorn Andersson
0 siblings, 1 reply; 24+ messages in thread
From: Chris Lew @ 2017-12-01 1:52 UTC (permalink / raw)
To: Bjorn Andersson, Andy Gross, Ohad Ben-Cohen
Cc: Arun Kumar Neelakantam, linux-kernel, linux-arm-msm, linux-soc,
linux-remoteproc
On 11/29/2017 5:16 PM, Bjorn Andersson wrote:
> The sysmon client communicates either via a dedicated SMD/GLINK channel
> or via QMI encoded messages over IPCROUTER with remote processors in
> order to perform graceful shutdown and inform about other remote
> processors shutting down.
>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
Acked-By: Chris Lew <clew@codeaurora.org>
Looked into who is listening to the other notifications besides
BEFORE_SHUTDOWN and I think subsequent patches will need to add support
for at least AFTER_POWERUP.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v4 4/5] remoteproc: qcom: Introduce sysmon
2017-12-01 1:52 ` Chris Lew
@ 2017-12-01 5:31 ` Bjorn Andersson
0 siblings, 0 replies; 24+ messages in thread
From: Bjorn Andersson @ 2017-12-01 5:31 UTC (permalink / raw)
To: Chris Lew
Cc: Andy Gross, Ohad Ben-Cohen, Arun Kumar Neelakantam, linux-kernel,
linux-arm-msm, linux-soc, linux-remoteproc
On Thu 30 Nov 17:52 PST 2017, Chris Lew wrote:
>
>
> On 11/29/2017 5:16 PM, Bjorn Andersson wrote:
> > The sysmon client communicates either via a dedicated SMD/GLINK channel
> > or via QMI encoded messages over IPCROUTER with remote processors in
> > order to perform graceful shutdown and inform about other remote
> > processors shutting down.
> >
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > ---
> Acked-By: Chris Lew <clew@codeaurora.org>
>
Thanks
> Looked into who is listening to the other notifications besides
> BEFORE_SHUTDOWN and I think subsequent patches will need to add support for
> at least AFTER_POWERUP.
>
Agreed.
Regards,
Bjorn
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v4 5/5] samples: Introduce Qualcomm QMI sample client
2017-11-30 1:16 [PATCH v4 0/5] In-kernel QMI helpers and sysmon Bjorn Andersson
` (3 preceding siblings ...)
2017-11-30 1:16 ` [PATCH v4 4/5] remoteproc: qcom: Introduce sysmon Bjorn Andersson
@ 2017-11-30 1:16 ` Bjorn Andersson
2017-11-30 17:36 ` Chris Lew
4 siblings, 1 reply; 24+ messages in thread
From: Bjorn Andersson @ 2017-11-30 1:16 UTC (permalink / raw)
To: Andy Gross, Ohad Ben-Cohen, Bjorn Andersson
Cc: Arun Kumar Neelakantam, Chris Lew, linux-kernel, linux-arm-msm,
linux-soc, linux-remoteproc
Introduce a sample driver that register for server notifications and
spawn clients for each available test service (service 15). The spawned
clients implements the interface for encoding "ping" and "data" requests
and decode the responses from the remote.
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
Changes since v3:
- Kerneldoc updates
Changes since v2:
- Fix some kerneldoc typos
- Checkpatch fixes
Changes since v1:
- Adapted to updated QMI helper interface
- Moved user space interface to debugfs
samples/Kconfig | 9 +
samples/Makefile | 2 +-
samples/qmi/Makefile | 1 +
samples/qmi/qmi_sample_client.c | 631 ++++++++++++++++++++++++++++++++++++++++
4 files changed, 642 insertions(+), 1 deletion(-)
create mode 100644 samples/qmi/Makefile
create mode 100644 samples/qmi/qmi_sample_client.c
diff --git a/samples/Kconfig b/samples/Kconfig
index c332a3b9de05..4cb8af2f810f 100644
--- a/samples/Kconfig
+++ b/samples/Kconfig
@@ -62,6 +62,15 @@ config SAMPLE_KDB
Build an example of how to dynamically add the hello
command to the kdb shell.
+config SAMPLE_QMI_CLIENT
+ tristate "Build qmi client sample -- loadable modules only"
+ depends on m
+ depends on ARCH_QCOM
+ select QCOM_QMI_HELPERS
+ help
+ Build an QMI client sample driver, which demonstrates how to
+ communicate with a remote QRTR service, using QMI encoded messages.
+
config SAMPLE_RPMSG_CLIENT
tristate "Build rpmsg client sample -- loadable modules only"
depends on RPMSG && m
diff --git a/samples/Makefile b/samples/Makefile
index db54e766ddb1..a30833a2a19e 100644
--- a/samples/Makefile
+++ b/samples/Makefile
@@ -3,4 +3,4 @@
obj-$(CONFIG_SAMPLES) += kobject/ kprobes/ trace_events/ livepatch/ \
hw_breakpoint/ kfifo/ kdb/ hidraw/ rpmsg/ seccomp/ \
configfs/ connector/ v4l/ trace_printk/ blackfin/ \
- vfio-mdev/ statx/
+ vfio-mdev/ statx/ qmi/
diff --git a/samples/qmi/Makefile b/samples/qmi/Makefile
new file mode 100644
index 000000000000..2b111d2769df
--- /dev/null
+++ b/samples/qmi/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_SAMPLE_QMI_CLIENT) += qmi_sample_client.o
diff --git a/samples/qmi/qmi_sample_client.c b/samples/qmi/qmi_sample_client.c
new file mode 100644
index 000000000000..3ff722ced52c
--- /dev/null
+++ b/samples/qmi/qmi_sample_client.c
@@ -0,0 +1,631 @@
+/*
+ * Sample in-kernel QMI client driver
+ *
+ * Copyright (c) 2013-2014, The Linux Foundation. All rights reserved.
+ * 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/debugfs.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/soc/qcom/qmi.h>
+
+#define PING_REQ1_TLV_TYPE 0x1
+#define PING_RESP1_TLV_TYPE 0x2
+#define PING_OPT1_TLV_TYPE 0x10
+#define PING_OPT2_TLV_TYPE 0x11
+
+#define DATA_REQ1_TLV_TYPE 0x1
+#define DATA_RESP1_TLV_TYPE 0x2
+#define DATA_OPT1_TLV_TYPE 0x10
+#define DATA_OPT2_TLV_TYPE 0x11
+
+#define TEST_MED_DATA_SIZE_V01 8192
+#define TEST_MAX_NAME_SIZE_V01 255
+
+#define TEST_PING_REQ_MSG_ID_V01 0x20
+#define TEST_DATA_REQ_MSG_ID_V01 0x21
+
+#define TEST_PING_REQ_MAX_MSG_LEN_V01 266
+#define TEST_DATA_REQ_MAX_MSG_LEN_V01 8456
+
+struct test_name_type_v01 {
+ u32 name_len;
+ char name[TEST_MAX_NAME_SIZE_V01];
+};
+
+static struct qmi_elem_info test_name_type_v01_ei[] = {
+ {
+ .data_type = QMI_DATA_LEN,
+ .elem_len = 1,
+ .elem_size = sizeof(u8),
+ .array_type = NO_ARRAY,
+ .tlv_type = QMI_COMMON_TLV_TYPE,
+ .offset = offsetof(struct test_name_type_v01,
+ name_len),
+ },
+ {
+ .data_type = QMI_UNSIGNED_1_BYTE,
+ .elem_len = TEST_MAX_NAME_SIZE_V01,
+ .elem_size = sizeof(char),
+ .array_type = VAR_LEN_ARRAY,
+ .tlv_type = QMI_COMMON_TLV_TYPE,
+ .offset = offsetof(struct test_name_type_v01,
+ name),
+ },
+ {}
+};
+
+struct test_ping_req_msg_v01 {
+ char ping[4];
+
+ u8 client_name_valid;
+ struct test_name_type_v01 client_name;
+};
+
+static struct qmi_elem_info test_ping_req_msg_v01_ei[] = {
+ {
+ .data_type = QMI_UNSIGNED_1_BYTE,
+ .elem_len = 4,
+ .elem_size = sizeof(char),
+ .array_type = STATIC_ARRAY,
+ .tlv_type = PING_REQ1_TLV_TYPE,
+ .offset = offsetof(struct test_ping_req_msg_v01,
+ ping),
+ },
+ {
+ .data_type = QMI_OPT_FLAG,
+ .elem_len = 1,
+ .elem_size = sizeof(u8),
+ .array_type = NO_ARRAY,
+ .tlv_type = PING_OPT1_TLV_TYPE,
+ .offset = offsetof(struct test_ping_req_msg_v01,
+ client_name_valid),
+ },
+ {
+ .data_type = QMI_STRUCT,
+ .elem_len = 1,
+ .elem_size = sizeof(struct test_name_type_v01),
+ .array_type = NO_ARRAY,
+ .tlv_type = PING_OPT1_TLV_TYPE,
+ .offset = offsetof(struct test_ping_req_msg_v01,
+ client_name),
+ .ei_array = test_name_type_v01_ei,
+ },
+ {}
+};
+
+struct test_ping_resp_msg_v01 {
+ struct qmi_response_type_v01 resp;
+
+ u8 pong_valid;
+ char pong[4];
+
+ u8 service_name_valid;
+ struct test_name_type_v01 service_name;
+};
+
+static struct qmi_elem_info test_ping_resp_msg_v01_ei[] = {
+ {
+ .data_type = QMI_STRUCT,
+ .elem_len = 1,
+ .elem_size = sizeof(struct qmi_response_type_v01),
+ .array_type = NO_ARRAY,
+ .tlv_type = PING_RESP1_TLV_TYPE,
+ .offset = offsetof(struct test_ping_resp_msg_v01,
+ resp),
+ .ei_array = qmi_response_type_v01_ei,
+ },
+ {
+ .data_type = QMI_OPT_FLAG,
+ .elem_len = 1,
+ .elem_size = sizeof(u8),
+ .array_type = NO_ARRAY,
+ .tlv_type = PING_OPT1_TLV_TYPE,
+ .offset = offsetof(struct test_ping_resp_msg_v01,
+ pong_valid),
+ },
+ {
+ .data_type = QMI_UNSIGNED_1_BYTE,
+ .elem_len = 4,
+ .elem_size = sizeof(char),
+ .array_type = STATIC_ARRAY,
+ .tlv_type = PING_OPT1_TLV_TYPE,
+ .offset = offsetof(struct test_ping_resp_msg_v01,
+ pong),
+ },
+ {
+ .data_type = QMI_OPT_FLAG,
+ .elem_len = 1,
+ .elem_size = sizeof(u8),
+ .array_type = NO_ARRAY,
+ .tlv_type = PING_OPT2_TLV_TYPE,
+ .offset = offsetof(struct test_ping_resp_msg_v01,
+ service_name_valid),
+ },
+ {
+ .data_type = QMI_STRUCT,
+ .elem_len = 1,
+ .elem_size = sizeof(struct test_name_type_v01),
+ .array_type = NO_ARRAY,
+ .tlv_type = PING_OPT2_TLV_TYPE,
+ .offset = offsetof(struct test_ping_resp_msg_v01,
+ service_name),
+ .ei_array = test_name_type_v01_ei,
+ },
+ {}
+};
+
+struct test_data_req_msg_v01 {
+ u32 data_len;
+ u8 data[TEST_MED_DATA_SIZE_V01];
+
+ u8 client_name_valid;
+ struct test_name_type_v01 client_name;
+};
+
+static struct qmi_elem_info test_data_req_msg_v01_ei[] = {
+ {
+ .data_type = QMI_DATA_LEN,
+ .elem_len = 1,
+ .elem_size = sizeof(u32),
+ .array_type = NO_ARRAY,
+ .tlv_type = DATA_REQ1_TLV_TYPE,
+ .offset = offsetof(struct test_data_req_msg_v01,
+ data_len),
+ },
+ {
+ .data_type = QMI_UNSIGNED_1_BYTE,
+ .elem_len = TEST_MED_DATA_SIZE_V01,
+ .elem_size = sizeof(u8),
+ .array_type = VAR_LEN_ARRAY,
+ .tlv_type = DATA_REQ1_TLV_TYPE,
+ .offset = offsetof(struct test_data_req_msg_v01,
+ data),
+ },
+ {
+ .data_type = QMI_OPT_FLAG,
+ .elem_len = 1,
+ .elem_size = sizeof(u8),
+ .array_type = NO_ARRAY,
+ .tlv_type = DATA_OPT1_TLV_TYPE,
+ .offset = offsetof(struct test_data_req_msg_v01,
+ client_name_valid),
+ },
+ {
+ .data_type = QMI_STRUCT,
+ .elem_len = 1,
+ .elem_size = sizeof(struct test_name_type_v01),
+ .array_type = NO_ARRAY,
+ .tlv_type = DATA_OPT1_TLV_TYPE,
+ .offset = offsetof(struct test_data_req_msg_v01,
+ client_name),
+ .ei_array = test_name_type_v01_ei,
+ },
+ {}
+};
+
+struct test_data_resp_msg_v01 {
+ struct qmi_response_type_v01 resp;
+
+ u8 data_valid;
+ u32 data_len;
+ u8 data[TEST_MED_DATA_SIZE_V01];
+
+ u8 service_name_valid;
+ struct test_name_type_v01 service_name;
+};
+
+static struct qmi_elem_info test_data_resp_msg_v01_ei[] = {
+ {
+ .data_type = QMI_STRUCT,
+ .elem_len = 1,
+ .elem_size = sizeof(struct qmi_response_type_v01),
+ .array_type = NO_ARRAY,
+ .tlv_type = DATA_RESP1_TLV_TYPE,
+ .offset = offsetof(struct test_data_resp_msg_v01,
+ resp),
+ .ei_array = qmi_response_type_v01_ei,
+ },
+ {
+ .data_type = QMI_OPT_FLAG,
+ .elem_len = 1,
+ .elem_size = sizeof(u8),
+ .array_type = NO_ARRAY,
+ .tlv_type = DATA_OPT1_TLV_TYPE,
+ .offset = offsetof(struct test_data_resp_msg_v01,
+ data_valid),
+ },
+ {
+ .data_type = QMI_DATA_LEN,
+ .elem_len = 1,
+ .elem_size = sizeof(u32),
+ .array_type = NO_ARRAY,
+ .tlv_type = DATA_OPT1_TLV_TYPE,
+ .offset = offsetof(struct test_data_resp_msg_v01,
+ data_len),
+ },
+ {
+ .data_type = QMI_UNSIGNED_1_BYTE,
+ .elem_len = TEST_MED_DATA_SIZE_V01,
+ .elem_size = sizeof(u8),
+ .array_type = VAR_LEN_ARRAY,
+ .tlv_type = DATA_OPT1_TLV_TYPE,
+ .offset = offsetof(struct test_data_resp_msg_v01,
+ data),
+ },
+ {
+ .data_type = QMI_OPT_FLAG,
+ .elem_len = 1,
+ .elem_size = sizeof(u8),
+ .array_type = NO_ARRAY,
+ .tlv_type = DATA_OPT2_TLV_TYPE,
+ .offset = offsetof(struct test_data_resp_msg_v01,
+ service_name_valid),
+ },
+ {
+ .data_type = QMI_STRUCT,
+ .elem_len = 1,
+ .elem_size = sizeof(struct test_name_type_v01),
+ .array_type = NO_ARRAY,
+ .tlv_type = DATA_OPT2_TLV_TYPE,
+ .offset = offsetof(struct test_data_resp_msg_v01,
+ service_name),
+ .ei_array = test_name_type_v01_ei,
+ },
+ {}
+};
+
+/*
+ * ping_write() - ping_pong debugfs file write handler
+ * @file: debugfs file context
+ * @user_buf: reference to the user data (ignored)
+ * @count: number of bytes in @user_buf
+ * @ppos: offset in @file to write
+ *
+ * This function allows user space to send out a ping_pong QMI encoded message
+ * to the associated remote test service and will return with the result of the
+ * transaction. It serves as an example of how to provide a custom response
+ * handler.
+ *
+ * Return: @count, or negative errno on failure.
+ */
+static ssize_t ping_write(struct file *file, const char __user *user_buf,
+ size_t count, loff_t *ppos)
+{
+ struct qmi_handle *qmi = file->private_data;
+ struct test_ping_req_msg_v01 req = {};
+ struct qmi_txn txn;
+ int ret;
+
+ memcpy(req.ping, "ping", sizeof(req.ping));
+
+ ret = qmi_txn_init(qmi, &txn, NULL, NULL);
+ if (ret < 0)
+ return ret;
+
+ ret = qmi_send_request(qmi, NULL, &txn,
+ TEST_PING_REQ_MSG_ID_V01,
+ TEST_PING_REQ_MAX_MSG_LEN_V01,
+ test_ping_req_msg_v01_ei, &req);
+ if (ret < 0) {
+ qmi_txn_cancel(&txn);
+ return ret;
+ }
+
+ ret = qmi_txn_wait(&txn, 5 * HZ);
+ if (ret < 0)
+ count = ret;
+
+ return count;
+}
+
+static const struct file_operations ping_fops = {
+ .open = simple_open,
+ .write = ping_write,
+};
+
+static void ping_pong_cb(struct qmi_handle *qmi, struct sockaddr_qrtr *sq,
+ struct qmi_txn *txn, const void *data)
+{
+ const struct test_ping_resp_msg_v01 *resp = data;
+
+ if (!txn) {
+ pr_err("spurious ping response\n");
+ return;
+ }
+
+ if (resp->resp.result == QMI_RESULT_FAILURE_V01)
+ txn->result = -ENXIO;
+ else if (!resp->pong_valid || memcmp(resp->pong, "pong", 4))
+ txn->result = -EINVAL;
+
+ complete(&txn->completion);
+}
+
+/*
+ * data_write() - data debugfs file write handler
+ * @file: debugfs file context
+ * @user_buf: reference to the user data
+ * @count: number of bytes in @user_buf
+ * @ppos: offset in @file to write
+ *
+ * This function allows user space to send out a data QMI encoded message to
+ * the associated remote test service and will return with the result of the
+ * transaction. It serves as an example of how to have the QMI helpers decode a
+ * transaction response into a provided object automatically.
+ *
+ * Return: @count, or negative errno on failure.
+ */
+static ssize_t data_write(struct file *file, const char __user *user_buf,
+ size_t count, loff_t *ppos)
+
+{
+ struct qmi_handle *qmi = file->private_data;
+ struct test_data_resp_msg_v01 *resp;
+ struct test_data_req_msg_v01 *req;
+ struct qmi_txn txn;
+ int ret;
+
+ req = kzalloc(sizeof(*req), GFP_KERNEL);
+ if (!req)
+ return -ENOMEM;
+
+ resp = kzalloc(sizeof(*resp), GFP_KERNEL);
+ if (!resp) {
+ kfree(req);
+ return -ENOMEM;
+ }
+
+ req->data_len = min_t(size_t, sizeof(req->data), count);
+ if (copy_from_user(req->data, user_buf, req->data_len)) {
+ ret = -EFAULT;
+ goto out;
+ }
+
+ ret = qmi_txn_init(qmi, &txn, test_data_resp_msg_v01_ei, resp);
+ if (ret < 0)
+ goto out;
+
+ ret = qmi_send_request(qmi, NULL, &txn,
+ TEST_DATA_REQ_MSG_ID_V01,
+ TEST_DATA_REQ_MAX_MSG_LEN_V01,
+ test_data_req_msg_v01_ei, req);
+ if (ret < 0) {
+ qmi_txn_cancel(&txn);
+ goto out;
+ }
+
+ ret = qmi_txn_wait(&txn, 5 * HZ);
+ if (ret < 0) {
+ goto out;
+ } else if (!resp->data_valid ||
+ resp->data_len != req->data_len ||
+ memcmp(resp->data, req->data, req->data_len)) {
+ pr_err("response data doesn't match expectation\n");
+ ret = -EINVAL;
+ goto out;
+ }
+
+ ret = count;
+
+out:
+ kfree(resp);
+ kfree(req);
+
+ return ret;
+}
+
+static const struct file_operations data_fops = {
+ .open = simple_open,
+ .write = data_write,
+};
+
+static struct qmi_msg_handler qmi_sample_handlers[] = {
+ {
+ .type = QMI_RESPONSE,
+ .msg_id = TEST_PING_REQ_MSG_ID_V01,
+ .ei = test_ping_resp_msg_v01_ei,
+ .decoded_size = sizeof(struct test_ping_req_msg_v01),
+ .fn = ping_pong_cb
+ },
+ {}
+};
+
+struct qmi_sample {
+ struct qmi_handle qmi;
+
+ struct dentry *de_dir;
+ struct dentry *de_data;
+ struct dentry *de_ping;
+};
+
+static struct dentry *qmi_debug_dir;
+
+static int qmi_sample_probe(struct platform_device *pdev)
+{
+ struct sockaddr_qrtr *sq;
+ struct qmi_sample *sample;
+ char path[20];
+ int ret;
+
+ sample = devm_kzalloc(&pdev->dev, sizeof(*sample), GFP_KERNEL);
+ if (!sample)
+ return -ENOMEM;
+
+ ret = qmi_handle_init(&sample->qmi, TEST_DATA_REQ_MAX_MSG_LEN_V01,
+ NULL,
+ qmi_sample_handlers);
+ if (ret < 0)
+ return ret;
+
+ sq = dev_get_platdata(&pdev->dev);
+ ret = kernel_connect(sample->qmi.sock, (struct sockaddr *)sq,
+ sizeof(*sq), 0);
+ if (ret < 0) {
+ pr_err("failed to connect to remote service port\n");
+ goto err_release_qmi_handle;
+ }
+
+ snprintf(path, sizeof(path), "%d:%d", sq->sq_node, sq->sq_port);
+
+ sample->de_dir = debugfs_create_dir(path, qmi_debug_dir);
+ if (IS_ERR(sample->de_dir)) {
+ ret = PTR_ERR(sample->de_dir);
+ goto err_release_qmi_handle;
+ }
+
+ sample->de_data = debugfs_create_file("data", 0600, sample->de_dir,
+ sample, &data_fops);
+ if (IS_ERR(sample->de_data)) {
+ ret = PTR_ERR(sample->de_data);
+ goto err_remove_de_dir;
+ }
+
+ sample->de_ping = debugfs_create_file("ping", 0600, sample->de_dir,
+ sample, &ping_fops);
+ if (IS_ERR(sample->de_ping)) {
+ ret = PTR_ERR(sample->de_ping);
+ goto err_remove_de_data;
+ }
+
+ platform_set_drvdata(pdev, sample);
+
+ return 0;
+
+err_remove_de_data:
+ debugfs_remove(sample->de_data);
+err_remove_de_dir:
+ debugfs_remove(sample->de_dir);
+err_release_qmi_handle:
+ qmi_handle_release(&sample->qmi);
+
+ return ret;
+}
+
+static int qmi_sample_remove(struct platform_device *pdev)
+{
+ struct qmi_sample *sample = platform_get_drvdata(pdev);
+
+ debugfs_remove(sample->de_ping);
+ debugfs_remove(sample->de_data);
+ debugfs_remove(sample->de_dir);
+
+ qmi_handle_release(&sample->qmi);
+
+ return 0;
+}
+
+static struct platform_driver qmi_sample_driver = {
+ .probe = qmi_sample_probe,
+ .remove = qmi_sample_remove,
+ .driver = {
+ .name = "qmi_sample_client",
+ },
+};
+
+static int qmi_sample_new_server(struct qmi_handle *qmi,
+ struct qmi_service *service)
+{
+ struct platform_device *pdev;
+ struct sockaddr_qrtr sq = { AF_QIPCRTR, service->node, service->port };
+ int ret;
+
+ pdev = platform_device_alloc("qmi_sample_client", PLATFORM_DEVID_AUTO);
+ if (!pdev)
+ return -ENOMEM;
+
+ ret = platform_device_add_data(pdev, &sq, sizeof(sq));
+ if (ret)
+ goto err_put_device;
+
+ ret = platform_device_add(pdev);
+ if (ret)
+ goto err_put_device;
+
+ service->priv = pdev;
+
+ return 0;
+
+err_put_device:
+ platform_device_put(pdev);
+
+ return ret;
+}
+
+static void qmi_sample_del_server(struct qmi_handle *qmi,
+ struct qmi_service *service)
+{
+ struct platform_device *pdev = service->priv;
+
+ platform_device_unregister(pdev);
+}
+
+static struct qmi_handle lookup_client;
+
+static struct qmi_ops lookup_ops = {
+ .new_server = qmi_sample_new_server,
+ .del_server = qmi_sample_del_server,
+};
+
+static int qmi_sample_init(void)
+{
+ int ret;
+
+ qmi_debug_dir = debugfs_create_dir("qmi_sample", NULL);
+ if (IS_ERR(qmi_debug_dir)) {
+ pr_err("failed to create qmi_sample dir\n");
+ return PTR_ERR(qmi_debug_dir);
+ }
+
+ ret = platform_driver_register(&qmi_sample_driver);
+ if (ret)
+ goto err_remove_debug_dir;
+
+ ret = qmi_handle_init(&lookup_client, 0, &lookup_ops, NULL);
+ if (ret < 0)
+ goto err_unregister_driver;
+
+ qmi_add_lookup(&lookup_client, 15, 0, 0);
+
+ return 0;
+
+err_unregister_driver:
+ platform_driver_unregister(&qmi_sample_driver);
+err_remove_debug_dir:
+ debugfs_remove(qmi_debug_dir);
+
+ return ret;
+}
+
+static void qmi_sample_exit(void)
+{
+ qmi_handle_release(&lookup_client);
+
+ platform_driver_unregister(&qmi_sample_driver);
+
+ debugfs_remove(qmi_debug_dir);
+}
+
+module_init(qmi_sample_init);
+module_exit(qmi_sample_exit);
+
+MODULE_DESCRIPTION("Sample QMI client driver");
+MODULE_LICENSE("GPL v2");
--
2.15.0
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH v4 5/5] samples: Introduce Qualcomm QMI sample client
2017-11-30 1:16 ` [PATCH v4 5/5] samples: Introduce Qualcomm QMI sample client Bjorn Andersson
@ 2017-11-30 17:36 ` Chris Lew
0 siblings, 0 replies; 24+ messages in thread
From: Chris Lew @ 2017-11-30 17:36 UTC (permalink / raw)
To: Bjorn Andersson, Andy Gross, Ohad Ben-Cohen
Cc: Arun Kumar Neelakantam, linux-kernel, linux-arm-msm, linux-soc,
linux-remoteproc
On 11/29/2017 5:16 PM, Bjorn Andersson wrote:
> Introduce a sample driver that register for server notifications and
> spawn clients for each available test service (service 15). The spawned
> clients implements the interface for encoding "ping" and "data" requests
> and decode the responses from the remote.
>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
Acked-By: Chris Lew <clew@codeaurora.org>
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 24+ messages in thread