Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 18 May 2020 11:10:10 -0700
From:   Bjorn Andersson <bjorn.andersson@linaro.org>
To:     Wang Wenhu <wenhu.wang@vivo.com>
Cc:     Andy Gross <agross@kernel.org>, Sibi Sankar <sibis@codeaurora.org>,
        linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org,
        kernel@vivo.com
Subject: Re: [PATCH] soc: qmi: move tlv-micros to header file
Message-ID: <20200518181010.GC2165@builder.lan>
References: <20200413035758.60238-1-wenhu.wang@vivo.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200413035758.60238-1-wenhu.wang@vivo.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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
>