Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <b46f6c1c-0b3d-0960-b655-46567ede41d0@quicinc.com>
Date:   Thu, 18 Aug 2022 14:10:52 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.12.0
Subject: Re: [PATCH 28/50] wifi: ath12k: add htc.h
Content-Language: en-US
To:     Kalle Valo <kvalo@kernel.org>, <linux-wireless@vger.kernel.org>
CC:     <ath12k@lists.infradead.org>
References: <20220812161003.27279-1-kvalo@kernel.org>
 <20220812161003.27279-29-kvalo@kernel.org>
From:   Jeff Johnson <quic_jjohnson@quicinc.com>
In-Reply-To: <20220812161003.27279-29-kvalo@kernel.org>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 8/12/2022 9:09 AM, Kalle Valo wrote:
> From: Kalle Valo <quic_kvalo@quicinc.com>
> 
> (Patches split into one patch per file for easier review, but the final
> commit will be one big patch. See the cover letter for more info.)
> 
> Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>
> ---
>   drivers/net/wireless/ath/ath12k/htc.h | 311 ++++++++++++++++++++++++++++++++++
>   1 file changed, 311 insertions(+)
> 
> diff --git a/drivers/net/wireless/ath/ath12k/htc.h b/drivers/net/wireless/ath/ath12k/htc.h

snip

> +#define ATH12K_HTC_CONN_FLAGS_THRESHOLD_LEVEL_MASK GENMASK(1, 0)
> +#define ATH12K_HTC_CONN_FLAGS_RECV_ALLOC GENMASK(15, 8)
> +
> +enum ath12k_htc_conn_flags {
> +	ATH12K_HTC_CONN_FLAGS_THRESHOLD_LEVEL_ONE_FOURTH    = 0x0,
> +	ATH12K_HTC_CONN_FLAGS_THRESHOLD_LEVEL_ONE_HALF      = 0x1,
> +	ATH12K_HTC_CONN_FLAGS_THRESHOLD_LEVEL_THREE_FOURTHS = 0x2,
> +	ATH12K_HTC_CONN_FLAGS_THRESHOLD_LEVEL_UNITY         = 0x3,

it seems strange to call a 2-bit field a flag

> +	ATH12K_HTC_CONN_FLAGS_REDUCE_CREDIT_DRIBBLE    = 1 << 2,
> +	ATH12K_HTC_CONN_FLAGS_DISABLE_CREDIT_FLOW_CTRL = 1 << 3
> +};

and it seems strange to have an enum which incorporates the values of 
the ATH12K_HTC_CONN_FLAGS_THRESHOLD_LEVEL_MASK along with definitions of 
other mask flags.

perhaps these would be more logically defined as macros just after 
ATH12K_HTC_CONN_FLAGS_THRESHOLD_LEVEL_MASK:
#define ATH12K_HTC_CONN_FLAGS_THRESHOLD_LEVEL_MASK GENMASK(1, 0)
#define ATH12K_HTC_CONN_FLAGS_REDUCE_CREDIT_DRIBBLE BIT(2)
#define ATH12K_HTC_CONN_FLAGS_DISABLE_CREDIT_FLOW_CTRL BIT(3)

and then rename ath12k_htc_conn_flags to be more specific that it just 
has threshold level values?

> +
> +enum ath12k_htc_conn_svc_status {
> +	ATH12K_HTC_CONN_SVC_STATUS_SUCCESS      = 0,
> +	ATH12K_HTC_CONN_SVC_STATUS_NOT_FOUND    = 1,
> +	ATH12K_HTC_CONN_SVC_STATUS_FAILED       = 2,
> +	ATH12K_HTC_CONN_SVC_STATUS_NO_RESOURCES = 3,
> +	ATH12K_HTC_CONN_SVC_STATUS_NO_MORE_EP   = 4
> +};
> +
> +struct ath12k_htc_ready {
> +	__le32 id_credit_count;
> +	__le32 size_ep;
> +} __packed;
> +
> +struct ath12k_htc_ready_extended {
> +	struct ath12k_htc_ready base;
> +	__le32 ver_bundle;
> +} __packed;
> +
> +struct ath12k_htc_conn_svc {
> +	__le32 msg_svc_id;
> +	__le32 flags_len;
> +} __packed;
> +
> +struct ath12k_htc_conn_svc_resp {
> +	__le32 msg_svc_id;
> +	__le32 flags_len;
> +	__le32 svc_meta_pad;
> +} __packed;
> +
> +struct ath12k_htc_setup_complete_extended {
> +	__le32 msg_id;

is there a reason this isn't msg_svc_id to be consistent with the other 
htc messages?

or even have every htc msg begin with a struct ath12k_htc_msg hdr?

perhaps also consider some naming conventions similar to what were 
adopted for WMI?

> +	__le32 flags;
> +	__le32 max_msgs_per_bundled_recv;
> +} __packed;
> +
> +struct ath12k_htc_msg {
> +	__le32 msg_svc_id;
> +	__le32 flags_len;
> +} __packed __aligned(4);
> +
> +enum ath12k_htc_record_id {
> +	ATH12K_HTC_RECORD_NULL    = 0,
> +	ATH12K_HTC_RECORD_CREDITS = 1
> +};
> +
> +struct ath12k_htc_record_hdr {
> +	u8 id; /* @enum ath12k_htc_record_id */
> +	u8 len;
> +	u8 pad0;
> +	u8 pad1;
> +} __packed;
> +
> +struct ath12k_htc_credit_report {
> +	u8 eid; /* @enum ath12k_htc_ep_id */
> +	u8 credits;
> +	u8 pad0;
> +	u8 pad1;
> +} __packed;
> +
> +struct ath12k_htc_record {
> +	struct ath12k_htc_record_hdr hdr;
> +	union {
> +		struct ath12k_htc_credit_report credit_report[0];
> +		u8 pauload[0];

s/pauload/payload/?

s/[0]/[]/ per 
<https://www.kernel.org/doc/html/latest/process/deprecated.html#zero-length-and-one-element-arrays>

> +	};
> +} __packed __aligned(4);
> +
> +/* note: the trailer offset is dynamic depending
> + * on payload length. this is only a struct layout draft
> + */
> +struct ath12k_htc_frame {
> +	struct ath12k_htc_hdr hdr;
> +	union {
> +		struct ath12k_htc_msg msg;
> +		u8 payload[0];

s/[0]/[]/

> +	};
> +	struct ath12k_htc_record trailer[0];


s/[0]/[]/

but this struct makes no sense. you can't have a variable-length payload 
that isn't at the end of the struct

if this is just supposed to be some sort of pseudo-documentation then 
perhaps put it in a comment so that the compiler won't see this?

or perhaps even better draw an ASCII representation?

> +} __packed __aligned(4);

rest snipped