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@xxxxxxxx>
> ---
> 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
>
