* [PATCH] soc: qmi: move tlv-micros to header file
@ 2020-04-13 3:57 Wang Wenhu
2020-05-14 8:32 ` 王文虎
2020-05-18 18:10 ` [PATCH] " Bjorn Andersson
0 siblings, 2 replies; 3+ messages in thread
From: Wang Wenhu @ 2020-04-13 3:57 UTC (permalink / raw)
To: Andy Gross, Bjorn Andersson, Sibi Sankar, Wang Wenhu,
linux-arm-msm, linux-kernel
Cc: kernel
It's highly helpful to move the definitions of TLV related micros
into header file for user reference. The OPTIONAL_TLV_TYPE_START
should be known to any user that might define messages containing
optional fields. SIZE fields are the same, for users to calculate
message buffer length.
While encoding messages, Type always occurs together with Length.
So the better way is to use TL_SIZE, rather than (T_SIZE + L_SIZE).
Signed-off-by: Wang Wenhu <wenhu.wang@vivo.com>
---
drivers/soc/qcom/qmi_encdec.c | 28 ++++++++++++----------------
include/linux/soc/qcom/qmi.h | 5 +++++
2 files changed, 17 insertions(+), 16 deletions(-)
diff --git a/drivers/soc/qcom/qmi_encdec.c b/drivers/soc/qcom/qmi_encdec.c
index 3aaab71d1b2c..a674c68efab2 100644
--- a/drivers/soc/qcom/qmi_encdec.c
+++ b/drivers/soc/qcom/qmi_encdec.c
@@ -53,10 +53,6 @@ do { \
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);
@@ -142,7 +138,7 @@ static int qmi_calc_min_msg_len(struct qmi_elem_info *ei_array,
* nested structure.
*/
if (level == 1)
- min_msg_len += (TLV_TYPE_SIZE + TLV_LEN_SIZE);
+ min_msg_len += QMI_TLV_TL_SIZE;
}
return min_msg_len;
@@ -253,8 +249,7 @@ static int qmi_encode_string_elem(struct qmi_elem_info *ei_array,
}
if (enc_level == 1) {
- if (string_len + TLV_LEN_SIZE + TLV_TYPE_SIZE >
- out_buf_len) {
+ if (string_len + QMI_TLV_TL_SIZE > out_buf_len) {
pr_err("%s: Output len %d > Out Buf len %d\n",
__func__, string_len, out_buf_len);
return -ETOOSMALL;
@@ -311,7 +306,7 @@ static int qmi_encode(struct qmi_elem_info *ei_array, void *out_buf,
tlv_pointer = buf_dst;
tlv_len = 0;
if (enc_level == 1)
- buf_dst = buf_dst + (TLV_LEN_SIZE + TLV_TYPE_SIZE);
+ buf_dst = buf_dst + QMI_TLV_TL_SIZE;
while (temp_ei->data_type != QMI_EOTI) {
buf_src = in_c_struct + temp_ei->offset;
@@ -342,8 +337,8 @@ static int qmi_encode(struct qmi_elem_info *ei_array, void *out_buf,
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) {
+ if ((data_len_sz + encoded_bytes + QMI_TLV_TL_SIZE) >
+ out_buf_len) {
pr_err("%s: Too Small Buffer @DATA_LEN\n",
__func__);
return -ETOOSMALL;
@@ -367,7 +362,7 @@ static int qmi_encode(struct qmi_elem_info *ei_array, void *out_buf,
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) >
+ encoded_bytes + QMI_TLV_TL_SIZE) >
out_buf_len) {
pr_err("%s: Too Small Buffer @data_type:%d\n",
__func__, temp_ei->data_type);
@@ -410,10 +405,10 @@ static int qmi_encode(struct qmi_elem_info *ei_array, void *out_buf,
if (encode_tlv && enc_level == 1) {
QMI_ENCDEC_ENCODE_TLV(tlv_type, tlv_len, tlv_pointer);
- encoded_bytes += (TLV_TYPE_SIZE + TLV_LEN_SIZE);
+ encoded_bytes += QMI_TLV_TL_SIZE;
tlv_pointer = buf_dst;
tlv_len = 0;
- buf_dst = buf_dst + TLV_LEN_SIZE + TLV_TYPE_SIZE;
+ buf_dst = buf_dst + QMI_TLV_TL_SIZE;
encode_tlv = 0;
}
}
@@ -613,10 +608,11 @@ static int qmi_decode(struct qmi_elem_info *ei_array, void *out_c_struct,
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);
+ buf_src += QMI_TLV_TL_SIZE;
+ decoded_bytes += QMI_TLV_TL_SIZE;
temp_ei = find_ei(ei_array, tlv_type);
- if (!temp_ei && tlv_type < OPTIONAL_TLV_TYPE_START) {
+ if (!temp_ei && tlv_type <
+ QMI_OPTIONAL_TLV_TYPE_START) {
pr_err("%s: Inval element info\n", __func__);
return -EINVAL;
} else if (!temp_ei) {
diff --git a/include/linux/soc/qcom/qmi.h b/include/linux/soc/qcom/qmi.h
index e712f94b89fc..eb53aebdf45e 100644
--- a/include/linux/soc/qcom/qmi.h
+++ b/include/linux/soc/qcom/qmi.h
@@ -34,6 +34,11 @@ struct qmi_header {
#define QMI_INDICATION 4
#define QMI_COMMON_TLV_TYPE 0
+#define QMI_OPTIONAL_TLV_TYPE_START 0x10
+
+#define QMI_TLV_LEN_SIZE sizeof(u16)
+#define QMI_TLV_TYPE_SIZE sizeof(u8)
+#define QMI_TLV_TL_SIZE (QMI_TLV_LEN_SIZE + QMI_TLV_TYPE_SIZE)
enum qmi_elem_type {
QMI_EOTI,
--
2.17.1
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re:[PATCH] soc: qmi: move tlv-micros to header file
2020-04-13 3:57 [PATCH] soc: qmi: move tlv-micros to header file Wang Wenhu
@ 2020-05-14 8:32 ` 王文虎
2020-05-18 18:10 ` [PATCH] " Bjorn Andersson
1 sibling, 0 replies; 3+ messages in thread
From: 王文虎 @ 2020-05-14 8:32 UTC (permalink / raw)
To: Wang Wenhu
Cc: Andy Gross, Bjorn Andersson, Sibi Sankar, linux-arm-msm,
linux-kernel, kernel
Hi all,
Any comments here?
Regards,
Wenhu
>It's highly helpful to move the definitions of TLV related micros
>into header file for user reference. The OPTIONAL_TLV_TYPE_START
>should be known to any user that might define messages containing
>optional fields. SIZE fields are the same, for users to calculate
>message buffer length.
>
>While encoding messages, Type always occurs together with Length.
>So the better way is to use TL_SIZE, rather than (T_SIZE + L_SIZE).
>
>Signed-off-by: Wang Wenhu <wenhu.wang@vivo.com>
>---
> drivers/soc/qcom/qmi_encdec.c | 28 ++++++++++++----------------
> include/linux/soc/qcom/qmi.h | 5 +++++
> 2 files changed, 17 insertions(+), 16 deletions(-)
>
>diff --git a/drivers/soc/qcom/qmi_encdec.c b/drivers/soc/qcom/qmi_encdec.c
>index 3aaab71d1b2c..a674c68efab2 100644
>--- a/drivers/soc/qcom/qmi_encdec.c
>+++ b/drivers/soc/qcom/qmi_encdec.c
>@@ -53,10 +53,6 @@ do { \
> 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);
>@@ -142,7 +138,7 @@ static int qmi_calc_min_msg_len(struct qmi_elem_info *ei_array,
> * nested structure.
> */
> if (level == 1)
>- min_msg_len += (TLV_TYPE_SIZE + TLV_LEN_SIZE);
>+ min_msg_len += QMI_TLV_TL_SIZE;
> }
>
> return min_msg_len;
>@@ -253,8 +249,7 @@ static int qmi_encode_string_elem(struct qmi_elem_info *ei_array,
> }
>
> if (enc_level == 1) {
>- if (string_len + TLV_LEN_SIZE + TLV_TYPE_SIZE >
>- out_buf_len) {
>+ if (string_len + QMI_TLV_TL_SIZE > out_buf_len) {
> pr_err("%s: Output len %d > Out Buf len %d\n",
> __func__, string_len, out_buf_len);
> return -ETOOSMALL;
>@@ -311,7 +306,7 @@ static int qmi_encode(struct qmi_elem_info *ei_array, void *out_buf,
> tlv_pointer = buf_dst;
> tlv_len = 0;
> if (enc_level == 1)
>- buf_dst = buf_dst + (TLV_LEN_SIZE + TLV_TYPE_SIZE);
>+ buf_dst = buf_dst + QMI_TLV_TL_SIZE;
>
> while (temp_ei->data_type != QMI_EOTI) {
> buf_src = in_c_struct + temp_ei->offset;
>@@ -342,8 +337,8 @@ static int qmi_encode(struct qmi_elem_info *ei_array, void *out_buf,
> 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) {
>+ if ((data_len_sz + encoded_bytes + QMI_TLV_TL_SIZE) >
>+ out_buf_len) {
> pr_err("%s: Too Small Buffer @DATA_LEN\n",
> __func__);
> return -ETOOSMALL;
>@@ -367,7 +362,7 @@ static int qmi_encode(struct qmi_elem_info *ei_array, void *out_buf,
> 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) >
>+ encoded_bytes + QMI_TLV_TL_SIZE) >
> out_buf_len) {
> pr_err("%s: Too Small Buffer @data_type:%d\n",
> __func__, temp_ei->data_type);
>@@ -410,10 +405,10 @@ static int qmi_encode(struct qmi_elem_info *ei_array, void *out_buf,
>
> if (encode_tlv && enc_level == 1) {
> QMI_ENCDEC_ENCODE_TLV(tlv_type, tlv_len, tlv_pointer);
>- encoded_bytes += (TLV_TYPE_SIZE + TLV_LEN_SIZE);
>+ encoded_bytes += QMI_TLV_TL_SIZE;
> tlv_pointer = buf_dst;
> tlv_len = 0;
>- buf_dst = buf_dst + TLV_LEN_SIZE + TLV_TYPE_SIZE;
>+ buf_dst = buf_dst + QMI_TLV_TL_SIZE;
> encode_tlv = 0;
> }
> }
>@@ -613,10 +608,11 @@ static int qmi_decode(struct qmi_elem_info *ei_array, void *out_c_struct,
> 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);
>+ buf_src += QMI_TLV_TL_SIZE;
>+ decoded_bytes += QMI_TLV_TL_SIZE;
> temp_ei = find_ei(ei_array, tlv_type);
>- if (!temp_ei && tlv_type < OPTIONAL_TLV_TYPE_START) {
>+ if (!temp_ei && tlv_type <
>+ QMI_OPTIONAL_TLV_TYPE_START) {
> pr_err("%s: Inval element info\n", __func__);
> return -EINVAL;
> } else if (!temp_ei) {
>diff --git a/include/linux/soc/qcom/qmi.h b/include/linux/soc/qcom/qmi.h
>index e712f94b89fc..eb53aebdf45e 100644
>--- a/include/linux/soc/qcom/qmi.h
>+++ b/include/linux/soc/qcom/qmi.h
>@@ -34,6 +34,11 @@ struct qmi_header {
> #define QMI_INDICATION 4
>
> #define QMI_COMMON_TLV_TYPE 0
>+#define QMI_OPTIONAL_TLV_TYPE_START 0x10
>+
>+#define QMI_TLV_LEN_SIZE sizeof(u16)
>+#define QMI_TLV_TYPE_SIZE sizeof(u8)
>+#define QMI_TLV_TL_SIZE (QMI_TLV_LEN_SIZE + QMI_TLV_TYPE_SIZE)
>
> enum qmi_elem_type {
> QMI_EOTI,
>--
>2.17.1
>
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] soc: qmi: move tlv-micros to header file
2020-04-13 3:57 [PATCH] soc: qmi: move tlv-micros to header file Wang Wenhu
2020-05-14 8:32 ` 王文虎
@ 2020-05-18 18:10 ` Bjorn Andersson
1 sibling, 0 replies; 3+ messages in thread
From: Bjorn Andersson @ 2020-05-18 18:10 UTC (permalink / raw)
To: Wang Wenhu; +Cc: Andy Gross, Sibi Sankar, linux-arm-msm, linux-kernel, kernel
On Sun 12 Apr 20:57 PDT 2020, Wang Wenhu wrote:
> It's highly helpful to move the definitions of TLV related micros
> into header file for user reference. The OPTIONAL_TLV_TYPE_START
> should be known to any user that might define messages containing
> optional fields. SIZE fields are the same, for users to calculate
> message buffer length.
>
> While encoding messages, Type always occurs together with Length.
> So the better way is to use TL_SIZE, rather than (T_SIZE + L_SIZE).
>
> Signed-off-by: Wang Wenhu <wenhu.wang@vivo.com>
> ---
> drivers/soc/qcom/qmi_encdec.c | 28 ++++++++++++----------------
> include/linux/soc/qcom/qmi.h | 5 +++++
> 2 files changed, 17 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/soc/qcom/qmi_encdec.c b/drivers/soc/qcom/qmi_encdec.c
> index 3aaab71d1b2c..a674c68efab2 100644
> --- a/drivers/soc/qcom/qmi_encdec.c
> +++ b/drivers/soc/qcom/qmi_encdec.c
> @@ -53,10 +53,6 @@ do { \
> decoded_bytes += rc; \
> } while (0)
>
> -#define TLV_LEN_SIZE sizeof(u16)
> -#define TLV_TYPE_SIZE sizeof(u8)
Sorry, but as far as I can tell these properties should be hidden from
the clients by the encoder/decoder helpers. Can help me understand when
these could be useful to have outside this file?
> -#define OPTIONAL_TLV_TYPE_START 0x10
This has some meaning outside, but whenever I've written (typically
generated) qmi_elem_info arrays I just use the TVL TYPE as a number from
the datasheets directly. Can you please give an example of when it's
useful for a client to express a TLV relative to the "optional start"?
Thanks,
Bjorn
> -
> 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);
> @@ -142,7 +138,7 @@ static int qmi_calc_min_msg_len(struct qmi_elem_info *ei_array,
> * nested structure.
> */
> if (level == 1)
> - min_msg_len += (TLV_TYPE_SIZE + TLV_LEN_SIZE);
> + min_msg_len += QMI_TLV_TL_SIZE;
> }
>
> return min_msg_len;
> @@ -253,8 +249,7 @@ static int qmi_encode_string_elem(struct qmi_elem_info *ei_array,
> }
>
> if (enc_level == 1) {
> - if (string_len + TLV_LEN_SIZE + TLV_TYPE_SIZE >
> - out_buf_len) {
> + if (string_len + QMI_TLV_TL_SIZE > out_buf_len) {
> pr_err("%s: Output len %d > Out Buf len %d\n",
> __func__, string_len, out_buf_len);
> return -ETOOSMALL;
> @@ -311,7 +306,7 @@ static int qmi_encode(struct qmi_elem_info *ei_array, void *out_buf,
> tlv_pointer = buf_dst;
> tlv_len = 0;
> if (enc_level == 1)
> - buf_dst = buf_dst + (TLV_LEN_SIZE + TLV_TYPE_SIZE);
> + buf_dst = buf_dst + QMI_TLV_TL_SIZE;
>
> while (temp_ei->data_type != QMI_EOTI) {
> buf_src = in_c_struct + temp_ei->offset;
> @@ -342,8 +337,8 @@ static int qmi_encode(struct qmi_elem_info *ei_array, void *out_buf,
> 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) {
> + if ((data_len_sz + encoded_bytes + QMI_TLV_TL_SIZE) >
> + out_buf_len) {
> pr_err("%s: Too Small Buffer @DATA_LEN\n",
> __func__);
> return -ETOOSMALL;
> @@ -367,7 +362,7 @@ static int qmi_encode(struct qmi_elem_info *ei_array, void *out_buf,
> 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) >
> + encoded_bytes + QMI_TLV_TL_SIZE) >
> out_buf_len) {
> pr_err("%s: Too Small Buffer @data_type:%d\n",
> __func__, temp_ei->data_type);
> @@ -410,10 +405,10 @@ static int qmi_encode(struct qmi_elem_info *ei_array, void *out_buf,
>
> if (encode_tlv && enc_level == 1) {
> QMI_ENCDEC_ENCODE_TLV(tlv_type, tlv_len, tlv_pointer);
> - encoded_bytes += (TLV_TYPE_SIZE + TLV_LEN_SIZE);
> + encoded_bytes += QMI_TLV_TL_SIZE;
> tlv_pointer = buf_dst;
> tlv_len = 0;
> - buf_dst = buf_dst + TLV_LEN_SIZE + TLV_TYPE_SIZE;
> + buf_dst = buf_dst + QMI_TLV_TL_SIZE;
> encode_tlv = 0;
> }
> }
> @@ -613,10 +608,11 @@ static int qmi_decode(struct qmi_elem_info *ei_array, void *out_c_struct,
> 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);
> + buf_src += QMI_TLV_TL_SIZE;
> + decoded_bytes += QMI_TLV_TL_SIZE;
> temp_ei = find_ei(ei_array, tlv_type);
> - if (!temp_ei && tlv_type < OPTIONAL_TLV_TYPE_START) {
> + if (!temp_ei && tlv_type <
> + QMI_OPTIONAL_TLV_TYPE_START) {
> pr_err("%s: Inval element info\n", __func__);
> return -EINVAL;
> } else if (!temp_ei) {
> diff --git a/include/linux/soc/qcom/qmi.h b/include/linux/soc/qcom/qmi.h
> index e712f94b89fc..eb53aebdf45e 100644
> --- a/include/linux/soc/qcom/qmi.h
> +++ b/include/linux/soc/qcom/qmi.h
> @@ -34,6 +34,11 @@ struct qmi_header {
> #define QMI_INDICATION 4
>
> #define QMI_COMMON_TLV_TYPE 0
> +#define QMI_OPTIONAL_TLV_TYPE_START 0x10
> +
> +#define QMI_TLV_LEN_SIZE sizeof(u16)
> +#define QMI_TLV_TYPE_SIZE sizeof(u8)
> +#define QMI_TLV_TL_SIZE (QMI_TLV_LEN_SIZE + QMI_TLV_TYPE_SIZE)
>
> enum qmi_elem_type {
> QMI_EOTI,
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-05-18 18:11 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-13 3:57 [PATCH] soc: qmi: move tlv-micros to header file Wang Wenhu
2020-05-14 8:32 ` 王文虎
2020-05-18 18:10 ` [PATCH] " Bjorn Andersson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).